Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.

["Gaurav Jain" <gaurav.jain@nxp.com>]

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Monday, February 24, 2020 1:56 PM
> To: Gaurav Jain <gaurav.jain@nxp.com>; devel@edk2.groups.io; Gao, Liming
> <liming.gao@intel.com>; afish@apple.com; lersek@redhat.com;
> leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray <ray.ni@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Pankaj Bansal
> <pankaj.bansal@nxp.com>
> 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?

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 valid attributes.

In SCT Test code
First get the Bar attributes and set one of Unsupported attribute bit.
Call PciIoSetBarAttributes() with Unsupported attribute and in return, test expects EFI_UNSUPPORTED.

Regards
Gaurav Jain
> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > > -----Original Message-----
> > > From: Wu, Hao A <hao.a.wu@intel.com>
> > > Sent: Friday, February 21, 2020 6:53 AM
> > > To: devel@edk2.groups.io; Gaurav Jain <gaurav.jain@nxp.com>; Gao,
> > Liming
> > > <liming.gao@intel.com>; afish@apple.com; lersek@redhat.com;
> > > leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Ni, Ray
> > > <ray.ni@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> > > Pankaj Bansal <pankaj.bansal@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.wu@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.jain@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
> > > >
> > > >
> > > >