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


Gaurav Jain
 

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



Wu, Hao A
 

-----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



Gaurav Jain
 

-----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



Wu, Hao A
 

-----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