From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mx.groups.io with SMTP id smtpd.web11.18217.1582532778982652250 for ; Mon, 24 Feb 2020 00:26:19 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.100, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2020 00:26:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,479,1574150400"; d="scan'208";a="384072374" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 24 Feb 2020 00:26:17 -0800 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 24 Feb 2020 00:26:17 -0800 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Mon, 24 Feb 2020 00:26:17 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Mon, 24 Feb 2020 00:26:16 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.141]) with mapi id 14.03.0439.000; Mon, 24 Feb 2020 16:26:15 +0800 From: "Wu, Hao A" To: Gaurav Jain , "devel@edk2.groups.io" , "Gao, Liming" , "afish@apple.com" , "lersek@redhat.com" , "leif@nuviainc.com" , "Kinney, Michael D" CC: "Wang, Jian J" , "Ni, Ray" , "Ard Biesheuvel" , Pankaj Bansal Subject: Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Topic: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. Thread-Index: AdXoVX1UfqTUDarfQqSjaIalQXdrhgCiUCDgAAM/TyA= Date: Mon, 24 Feb 2020 08:26:14 +0000 Message-ID: References: In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Gaurav Jain [mailto:gaurav.jain@nxp.com] > Sent: Monday, February 24, 2020 3:04 PM > To: Wu, Hao A; devel@edk2.groups.io; Gao, Liming; afish@apple.com; > lersek@redhat.com; leif@nuviainc.com; Kinney, Michael D > Cc: Wang, Jian J; Ni, Ray; Ard Biesheuvel; Pankaj Bansal > Subject: RE: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] > MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. >=20 >=20 > > I think the above check for 'Attributes' can be dropped. > > I found that the implementation of the PciIoGetBarAttributes() functio= n > does not > > expose any configurable attributes. So the logic can fall through to t= he > ASSERT > > (for DEBUG images) and then returns EFI_UNSUPPORTED. >=20 > I agree that PciIoGetBarAttributes() function sets *Supports as 0. > But In SCT Test for SetBarAttributes, there is a test case for Unsupport= ed > Attribute which expects EFI_UNSUPPORTED. If I drop this check, ASSERT wi= ll > come, which is not expected. > Can we keep check for 'Attributes'? Oh, I forgot that. I have one question, is there any special reason for you to pick the suppo= rted bits specified by: EFI_PCI_DEVICE_ENABLE | EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE Is it relating with the SCT test case? Best Regards, Hao Wu >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Friday, February 21, 2020 6:53 AM > > To: devel@edk2.groups.io; Gaurav Jain ; Gao, > Liming > > ; afish@apple.com; lersek@redhat.com; > > leif@nuviainc.com; Kinney, Michael D > > Cc: Wang, Jian J ; Ni, Ray ; = Ard > > Biesheuvel ; Pankaj Bansal > > > > Subject: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] > > MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. > > > > Caution: EXT Email > > > > A couple of inline comments below. Please help to handle them in the n= ext > > version of patch. > > With them addressed, > > Reviewed-by: Hao A Wu > > > > > > Hello Liming and Stewards, > > > > I would like to confirm with you for whether the patch should catch th= e > > upcoming stable tag. > > > > My personal take is that the patch is more like a code refinement rath= er > than a > > bug fix. > > > > Could you help to make a final call for this one? Thanks in advance. > > > > Best Regards, > > Hao Wu > > > > > > > -----Original Message----- > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf > Of > > > Gaurav Jain > > > Sent: Thursday, February 20, 2020 11:40 PM > > > To: devel@edk2.groups.io > > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Ard Biesheuvel; Pankaj Bansal; > > > Gaurav Jain > > > Subject: [edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts > > > in SCT PCIIO Protocol Test. > > > > > > ASSERT in PollMem_Conf, CopyMem_Conf, SetBarAttributes_Conf > > > Conformance Test. > > > SCT Test expect return as Invalid Parameter or Unsupported. > > > Added Checks for Function Parameters. > > > return Invalid or Unsupported if Check fails. > > > > > > Added Checks in PciIoPollIo(), PciIoIoRead() > > > PciIoIoWrite() > > > > > > Signed-off-by: Gaurav Jain > > > --- > > > > > > Notes: > > > v2 > > > - Reverted ASSERT(FALSE) code. > > > - Added Checks for Width, BarIndex, Buffer, > > > Address range in PciIoIoRead, PciIoIoWrite. > > > - Added Checks for Width, BarIndex, Result, > > > Address range in PciIoPollIo, PciIoPollMem, > > > PciIoCopyMem. > > > - Added Checks for Attributes, BarIndex, > > > Address range in PciIoSetBarAttributes. > > > > > > .../NonDiscoverablePciDeviceIo.c | 180 +++++++++++++++= +++ > > > 1 file changed, 180 insertions(+) > > > > > > diff --git > > > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > > ciDeviceIo.c > > > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > > ciDeviceIo.c > > > index 2d55c9699322..4dd804356021 100644 > > > --- > > > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > > ciDeviceIo.c > > > +++ > > > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > > ciDeviceIo.c > > > @@ -93,6 +93,35 @@ PciIoPollMem ( > > > OUT UINT64 *Result > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > > + UINTN Count; > > > + EFI_STATUS Status; > > > + > > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + if (BarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Result =3D=3D NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Count =3D 1; > > > + > > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > > + (Status)) { > > > + return Status; > > > + } > > > + > > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > @@ -126,6 +155,35 @@ PciIoPollIo ( > > > OUT UINT64 *Result > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > > + UINTN Count; > > > + EFI_STATUS Status; > > > + > > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + if (BarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Result =3D=3D NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Count =3D 1; > > > + > > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > > + (Status)) { > > > + return Status; > > > + } > > > + > > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > @@ -396,6 +454,33 @@ PciIoIoRead ( > > > IN OUT VOID *Buffer > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > > + EFI_STATUS Status; > > > + > > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > > > > For PciIoIoRead(), I think enum values smaller than EfiPciIoWidthMaxim= um > are > > all valid. The above check seems to strict. >=20 > Will address this in v3. > > > > > > > + > > > + if (BarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Buffer =3D=3D NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > > + (Status)) { > > > + return Status; > > > + } > > > + > > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > @@ -425,6 +510,33 @@ PciIoIoWrite ( > > > IN OUT VOID *Buffer > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > > + EFI_STATUS Status; > > > + > > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > > > > For PciIoIoWrite(), I think enum values smaller than EfiPciIoWidthMaxi= mum > are > > all valid. The above check seems to strict. >=20 > Will address this in v3. > > > > > > > + > > > + if (BarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Buffer =3D=3D NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status =3D GetBarResource (Dev, BarIndex, &Desc); if (EFI_ERROR > > > + (Status)) { > > > + return Status; > > > + } > > > + > > > + if (Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > @@ -556,6 +668,40 @@ PciIoCopyMem ( > > > IN UINTN Count > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *DestDesc; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *SrcDesc; > > > + EFI_STATUS Status; > > > + > > > + if ((UINT32)Width > EfiPciIoWidthUint64) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + if (DestBarIndex >=3D PCI_MAX_BAR || > > > + SrcBarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status =3D GetBarResource (Dev, DestBarIndex, &DestDesc); if > > > + (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > + > > > + if (DestOffset + (Count << (Width & 0x3)) > DestDesc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + Status =3D GetBarResource (Dev, SrcBarIndex, &SrcDesc); if > > > + (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > + > > > + if (SrcOffset + (Count << (Width & 0x3)) > SrcDesc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > @@ -1414,6 +1560,40 @@ PciIoSetBarAttributes ( > > > IN OUT UINT64 *Length > > > ) > > > { > > > + NON_DISCOVERABLE_PCI_DEVICE *Dev; > > > + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Desc; > > > + EFI_PCI_IO_PROTOCOL_WIDTH Width; > > > + UINTN Count; > > > + EFI_STATUS Status; > > > + > > > + #define DEV_SUPPORTED_ATTRIBUTES \ > > > + (EFI_PCI_DEVICE_ENABLE | > > > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) > > > + > > > + if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) !=3D 0) { > > > + return EFI_UNSUPPORTED; > > > + } > > > > > > I think the above check for 'Attributes' can be dropped. > > I found that the implementation of the PciIoGetBarAttributes() functio= n > does not > > expose any configurable attributes. So the logic can fall through to t= he > ASSERT > > (for DEBUG images) and then returns EFI_UNSUPPORTED. > > > > Best Regards, > > HaoWu > > > > > > > + > > > + if (BarIndex >=3D PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Offset =3D=3D NULL || Length =3D=3D NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev =3D NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Width =3D EfiPciIoWidthUint8; > > > + Count =3D (UINT32) *Length; > > > + > > > + Status =3D GetBarResource(Dev, BarIndex, &Desc); if (EFI_ERROR > > > + (Status)) { > > > + return Status; > > > + } > > > + > > > + if (*Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > ASSERT (FALSE); > > > return EFI_UNSUPPORTED; > > > } > > > -- > > > 2.17.1 > > > > > > > > >=20