> -----Original Message----- > From: Gaurav Jain [mailto:gaurav.j...@nxp.com] > Sent: Monday, February 24, 2020 3:04 PM > To: Wu, Hao A; devel@edk2.groups.io; Gao, Liming; af...@apple.com; > ler...@redhat.com; l...@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? Best Regards, Hao Wu > > > -----Original Message----- > > From: Wu, Hao A <hao.a...@intel.com> > > Sent: Friday, February 21, 2020 6:53 AM > > To: devel@edk2.groups.io; Gaurav Jain <gaurav.j...@nxp.com>; Gao, > Liming > > <liming....@intel.com>; af...@apple.com; ler...@redhat.com; > > l...@nuviainc.com; Kinney, Michael D <michael.d.kin...@intel.com> > > Cc: Wang, Jian J <jian.j.w...@intel.com>; Ni, Ray <ray...@intel.com>; Ard > > Biesheuvel <ard.biesheu...@linaro.org>; Pankaj Bansal > > <pankaj.ban...@nxp.com> > > 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 next > > version of patch. > > With them addressed, > > Reviewed-by: Hao A Wu <hao.a...@intel.com> > > > > > > Hello Liming and Stewards, > > > > I would like to confirm with you for whether the patch should catch 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 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 <gaurav.j...@nxp.com> > > > --- > > > > > > 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 >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Result == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Count = 1; > > > + > > > + Status = 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 >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Result == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Count = 1; > > > + > > > + Status = 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 EfiPciIoWidthMaximum > are > > all valid. The above check seems to strict. > > Will address this in v3. > > > > > > > + > > > + if (BarIndex >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Buffer == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status = 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 EfiPciIoWidthMaximum > are > > all valid. The above check seems to strict. > > Will address this in v3. > > > > > > > + > > > + if (BarIndex >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Buffer == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status = 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 >= PCI_MAX_BAR || > > > + SrcBarIndex >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + > > > + Status = GetBarResource (Dev, DestBarIndex, &DestDesc); if > > > + (EFI_ERROR (Status)) { > > > + return Status; > > > + } > > > + > > > + if (DestOffset + (Count << (Width & 0x3)) > DestDesc->AddrLen) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + Status = 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)) != 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 >= PCI_MAX_BAR) { > > > + return EFI_UNSUPPORTED; > > > + } > > > + > > > + if (Offset == NULL || Length == NULL) { > > > + return EFI_INVALID_PARAMETER; > > > + } > > > + > > > + Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO(This); > > > + Width = EfiPciIoWidthUint8; > > > + Count = (UINT32) *Length; > > > + > > > + Status = 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 > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#54752): https://edk2.groups.io/g/devel/message/54752 Mute This Topic: https://groups.io/mt/71506527/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
