* Re: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
@ 2020-02-21 1:23 Wu, Hao A
2020-02-24 7:04 ` [EXT] " Gaurav Jain
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2020-02-21 1:23 UTC (permalink / raw)
To: devel@edk2.groups.io, gaurav.jain@nxp.com, Gao, Liming,
afish@apple.com, lersek@redhat.com, leif@nuviainc.com,
Kinney, Michael D
Cc: Wang, Jian J, Ni, Ray, Ard Biesheuvel, Pankaj Bansal
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.
> +
> + 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.
> +
> + 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
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
2020-02-21 1:23 [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test Wu, Hao A
@ 2020-02-24 7:04 ` Gaurav Jain
2020-02-24 8:26 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Gaurav Jain @ 2020-02-24 7:04 UTC (permalink / raw)
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
> 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'?
> -----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
> >
> >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
2020-02-24 7:04 ` [EXT] " Gaurav Jain
@ 2020-02-24 8:26 ` Wu, Hao A
2020-02-24 8:42 ` Gaurav Jain
0 siblings, 1 reply; 5+ messages in thread
From: Wu, Hao A @ 2020-02-24 8:26 UTC (permalink / raw)
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
> -----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?
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
> > >
> > >
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
2020-02-24 8:26 ` Wu, Hao A
@ 2020-02-24 8:42 ` Gaurav Jain
2020-02-24 12:50 ` Wu, Hao A
0 siblings, 1 reply; 5+ messages in thread
From: Gaurav Jain @ 2020-02-24 8:42 UTC (permalink / raw)
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
> -----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
> > > >
> > > >
> > > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [EXT] RE: [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test.
2020-02-24 8:42 ` Gaurav Jain
@ 2020-02-24 12:50 ` Wu, Hao A
0 siblings, 0 replies; 5+ messages in thread
From: Wu, Hao A @ 2020-02-24 12:50 UTC (permalink / raw)
To: devel@edk2.groups.io, gaurav.jain@nxp.com, Ard Biesheuvel
Cc: Wang, Jian J, Ni, Ray, Pankaj Bansal
> -----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.
>
>
>
> > -----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.
Got it. I am fine to put the below check for 'Attributes' in PciIoAttributes():
if ((Attributes & (~DEV_SUPPORTED_ATTRIBUTES)) != 0) {
return EFI_UNSUPPORTED;
}
Since the definition "DEV_SUPPORTED_ATTRIBUTES" will be used multiple times 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
>
> 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
> > > > >
> > > > >
> > > > >
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-24 12:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-21 1:23 [edk2-stable202002][edk2-devel] [PATCH v2 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test Wu, Hao A
2020-02-24 7:04 ` [EXT] " Gaurav Jain
2020-02-24 8:26 ` Wu, Hao A
2020-02-24 8:42 ` Gaurav Jain
2020-02-24 12:50 ` Wu, Hao A
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox