From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mx.groups.io with SMTP id smtpd.web10.20319.1582548611092857323 for ; Mon, 24 Feb 2020 04:50:11 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Feb 2020 04:50:10 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,480,1574150400"; d="scan'208";a="349914706" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 24 Feb 2020 04:50:10 -0800 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 24 Feb 2020 04:50:10 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 24 Feb 2020 04:50:09 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.5]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.222]) with mapi id 14.03.0439.000; Mon, 24 Feb 2020 20:50:07 +0800 From: "Wu, Hao A" To: "devel@edk2.groups.io" , "gaurav.jain@nxp.com" , Ard Biesheuvel CC: "Wang, Jian J" , "Ni, Ray" , 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/TyAAAFiDUAAAdEXw Date: Mon, 24 Feb 2020 12:50:07 +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: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of > Gaurav Jain > Sent: Monday, February 24, 2020 4:43 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 >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Monday, February 24, 2020 1:56 PM > > 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. > > > > Caution: EXT Email > > > > > -----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. > > > > > > > > > > I think the above check for 'Attributes' can be dropped. > > > > I found that the implementation of the PciIoGetBarAttributes() > > > > function > > > does not > > > > expose any configurable attributes. So the logic can fall through = to > > > > the > > > ASSERT > > > > (for DEBUG images) and then returns EFI_UNSUPPORTED. > > > > > > I agree that PciIoGetBarAttributes() function sets *Supports as 0. > > > But In SCT Test for SetBarAttributes, there is a test case for > > > Unsupported Attribute which expects EFI_UNSUPPORTED. If I drop this > > > check, ASSERT will 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 > supported > > bits specified by: > > EFI_PCI_DEVICE_ENABLE | > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE > > > > Is it relating with the SCT test case? >=20 > In PciIoAttributes() function, I can see the code > #define DEV_SUPPORTED_ATTRIBUTES \ > (EFI_PCI_DEVICE_ENABLE | > EFI_PCI_IO_ATTRIBUTE_DUAL_ADDRESS_CYCLE) > So I used the same bits in PciIoSetBarAttributes() to have a check for v= alid > attributes. Got it. I am fine to put the below check for 'Attributes' in PciIoAttribut= es(): if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) !=3D 0) { return EFI_UNSUPPORTED; } Since the definition "DEV_SUPPORTED_ATTRIBUTES" will be used multiple time= s in the driver, I suggest to remove the duplicate definitions in each function= and place it under file NonDiscoverablePciDeviceIo.h. Hello Ard, do you have any concern for this? Thanks. Best Regards, Hao Wu >=20 > In SCT Test code > First get the Bar attributes and set one of Unsupported attribute bit. > Call PciIoSetBarAttributes() with Unsupported attribute and in return, t= est > expects EFI_UNSUPPORTED. >=20 > Regards > Gaurav Jain > > > > Best Regards, > > Hao Wu > > > > > > > > > > > -----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 t= he > > > > next 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 catc= h > > > > the upcoming stable tag. > > > > > > > > My personal take is that the patch is more like a code refinement > > > > rather > > > than a > > > > bug fix. > > > > > > > > Could you help to make a final call for this one? Thanks in advanc= e. > > > > > > > > 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_ER= ROR > > > > > + (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_ER= ROR > > > > > + (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 > > > > EfiPciIoWidthMaximum > > > are > > > > all valid. The above check seems to strict. > > > > > > 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_ER= ROR > > > > > + (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 > > > > EfiPciIoWidthMaximum > > > are > > > > all valid. The above check seems to strict. > > > > > > 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_ER= ROR > > > > > + (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() > > > > function > > > does not > > > > expose any configurable attributes. So the logic can fall through = to > > > > the > > > 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_ERR= OR > > > > > + (Status)) { > > > > > + return Status; > > > > > + } > > > > > + > > > > > + if (*Offset + (Count << (Width & 0x3)) > Desc->AddrLen) { > > > > > + return EFI_UNSUPPORTED; > > > > > + } > > > > > + > > > > > ASSERT (FALSE); > > > > > return EFI_UNSUPPORTED; > > > > > } > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > > > > >=20 >=20 >=20