[PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
@ 2021-01-08  4:05 Sumana Venur
  2021-01-08  8:16 ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 6+ messages in thread
From: Sumana Venur @ 2021-01-08  4:05 UTC (permalink / raw)
  To: devel; +Cc: Sumana Venur

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989

Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
This needs to be wider, at least 32bit reads to reduce the time for oprom shadow

Signed-off-by: Sumana Venur <sumana.venur@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index c994ed5fe3..a981f93f43 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -521,9 +521,9 @@ LoadOpRomImage (
     //
     PciDevice->PciRootBridgeIo->Mem.Read (
                                       PciDevice->PciRootBridgeIo,
-                                      EfiPciWidthUint8,
+                                      EfiPciWidthUint32,
                                       RomBar,
-                                      (UINT32) RomImageSize,
+                                      (UINT32) RomImageSize/sizeof(UINT32),
                                       Image
                                       );
     RomInMemory = Image;
-- 
2.16.2.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
  2021-01-08  4:05 [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow Sumana Venur
@ 2021-01-08  8:16 ` Laszlo Ersek
  2021-01-10  7:52   ` Ni, Ray
  0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-01-08  8:16 UTC (permalink / raw)
  To: devel, sumana.venur; +Cc: Jian J Wang, Hao A Wu, Ray Ni

Adding PciBusDxe reviewers.

Thanks
Laszlo

On 01/08/21 05:05, Sumana Venur wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989
> 
> Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
> This needs to be wider, at least 32bit reads to reduce the time for oprom shadow
> 
> Signed-off-by: Sumana Venur <sumana.venur@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c994ed5fe3..a981f93f43 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -521,9 +521,9 @@ LoadOpRomImage (
>      //
>      PciDevice->PciRootBridgeIo->Mem.Read (
>                                        PciDevice->PciRootBridgeIo,
> -                                      EfiPciWidthUint8,
> +                                      EfiPciWidthUint32,
>                                        RomBar,
> -                                      (UINT32) RomImageSize,
> +                                      (UINT32) RomImageSize/sizeof(UINT32),
>                                        Image
>                                        );
>      RomInMemory = Image;
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
  2021-01-08  8:16 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-10  7:52   ` Ni, Ray
  0 siblings, 0 replies; 6+ messages in thread
From: Ni, Ray @ 2021-01-10  7:52 UTC (permalink / raw)
  To: Laszlo Ersek, devel@edk2.groups.io, Venur, Sumana; +Cc: Wang, Jian J, Wu, Hao A

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, January 8, 2021 4:17 PM
> To: devel@edk2.groups.io; Venur, Sumana <sumana.venur@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom
> shadow
> 
> Adding PciBusDxe reviewers.
> 
> Thanks
> Laszlo
> 
> On 01/08/21 05:05, Sumana Venur wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989
> >
> > Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
> > This needs to be wider, at least 32bit reads to reduce the time for oprom shadow
> >
> > Signed-off-by: Sumana Venur <sumana.venur@intel.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > index c994ed5fe3..a981f93f43 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > @@ -521,9 +521,9 @@ LoadOpRomImage (
> >      //
> >      PciDevice->PciRootBridgeIo->Mem.Read (
> >                                        PciDevice->PciRootBridgeIo,
> > -                                      EfiPciWidthUint8,
> > +                                      EfiPciWidthUint32,
> >                                        RomBar,
> > -                                      (UINT32) RomImageSize,
> > +                                      (UINT32) RomImageSize/sizeof(UINT32),
> >                                        Image
> >                                        );
> >      RomInMemory = Image;
> >


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
@ 2021-01-08  2:32 Sumana Venur
  2021-01-08  3:31 ` [edk2-devel] " Michael D Kinney
  0 siblings, 1 reply; 6+ messages in thread
From: Sumana Venur @ 2021-01-08  2:32 UTC (permalink / raw)
  To: devel; +Cc: Sumana Venur

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989

Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
This needs to be wider, at least 32bit reads to reduce the time for oprom shadow

Signed-off-by: Sumana Venur <sumana.venur@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index c994ed5fe3..19dceb3f91 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -521,9 +521,9 @@ LoadOpRomImage (
     //
     PciDevice->PciRootBridgeIo->Mem.Read (
                                       PciDevice->PciRootBridgeIo,
-                                      EfiPciWidthUint8,
+                                      EfiPciWidthUint32,
                                       RomBar,
-                                      (UINT32) RomImageSize,
+                                      (UINT32) RomImageSize/4,
                                       Image
                                       );
     RomInMemory = Image;
-- 
2.16.2.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
  2021-01-08  2:32 Sumana Venur
@ 2021-01-08  3:31 ` Michael D Kinney
  0 siblings, 0 replies; 6+ messages in thread
From: Michael D Kinney @ 2021-01-08  3:31 UTC (permalink / raw)
  To: devel@edk2.groups.io, Venur, Sumana, Kinney, Michael D

Sumana,

One minor change below.

From reviewing the code before this point, it looks like RomBar, RomImageSize, and Image are all aligned
no checking or handling of unaligned start or end needs to be added.

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sumana Venur
> Sent: Thursday, January 7, 2021 6:33 PM
> To: devel@edk2.groups.io
> Cc: Venur, Sumana <sumana.venur@intel.com>
> Subject: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989
> 
> Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
> This needs to be wider, at least 32bit reads to reduce the time for oprom shadow
> 
> Signed-off-by: Sumana Venur <sumana.venur@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c994ed5fe3..19dceb3f91 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -521,9 +521,9 @@ LoadOpRomImage (
>      //
>      PciDevice->PciRootBridgeIo->Mem.Read (
>                                        PciDevice->PciRootBridgeIo,
> -                                      EfiPciWidthUint8,
> +                                      EfiPciWidthUint32,
>                                        RomBar,
> -                                      (UINT32) RomImageSize,
> +                                      (UINT32) RomImageSize/4,

Recommend using: RomImageSize / sizeof (UINT32)

>                                        Image
>                                        );
>      RomInMemory = Image;
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
@ 2021-01-07  6:29 Sumana Venur
  2021-01-07 13:31 ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 6+ messages in thread
From: Sumana Venur @ 2021-01-07  6:29 UTC (permalink / raw)
  To: devel; +Cc: Sumana Venur

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989

Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
This needs to be wider, at least 32bit (even 64bit) reads to reduce the time for oprom shadow

Signed-off-by: Sumana Venur <sumana.venur@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index c994ed5fe3..fe6c7db417 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -521,7 +521,7 @@ LoadOpRomImage (
     //
     PciDevice->PciRootBridgeIo->Mem.Read (
                                       PciDevice->PciRootBridgeIo,
-                                      EfiPciWidthUint8,
+                                      EfiPciWidthUint64,
                                       RomBar,
                                       (UINT32) RomImageSize,
                                       Image
-- 
2.16.2.windows.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
  2021-01-07  6:29 Sumana Venur
@ 2021-01-07 13:31 ` Laszlo Ersek
  2021-01-08  2:37   ` Sumana Venur
  0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2021-01-07 13:31 UTC (permalink / raw)
  To: devel, sumana.venur

On 01/07/21 07:29, Sumana Venur wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2989
> 
> Long times spent on shadowing oprom from graphics card to system memory. We are currently using 8 bit read cycles.
> This needs to be wider, at least 32bit (even 64bit) reads to reduce the time for oprom shadow
> 
> Signed-off-by: Sumana Venur <sumana.venur@intel.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c994ed5fe3..fe6c7db417 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -521,7 +521,7 @@ LoadOpRomImage (
>      //
>      PciDevice->PciRootBridgeIo->Mem.Read (
>                                        PciDevice->PciRootBridgeIo,
> -                                      EfiPciWidthUint8,
> +                                      EfiPciWidthUint64,
>                                        RomBar,
>                                        (UINT32) RomImageSize,
>                                        Image
> 

This has three bugs:

(1) From the UEFI specification v2.8B,
EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.Mem.Read():

    The caller is responsible for satisfying any alignment and memory
    width restrictions that a PCI Root Bridge on a platform might
    require. For example on some platforms, width requests of
    EfiPciWidthUint64 do not work.

I don't think the patch ensures that EfiPciWidthUint64 is safe on the
particular platform.

(2) RomImageSize (the Count parameter) is expressed in bytes, currently.
If you scale up the access width, you have to scale down the access count.

(3) If you use an access size that is larger than 1 byte, you have to
handle the tail separately. For example, if the access width is
EfiPciWidthUint32, and the ROM image size is (4*k+3) bytes, then the
tail needs to be handled separately.

Laszlo


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow
  2021-01-07 13:31 ` [edk2-devel] " Laszlo Ersek
@ 2021-01-08  2:37   ` Sumana Venur
  0 siblings, 0 replies; 6+ messages in thread
From: Sumana Venur @ 2021-01-08  2:37 UTC (permalink / raw)
  To: Laszlo Ersek, devel

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]

I have submitted an updated patch.
1. Reduced the width to 32 to be safer
2. Sorry about not scaling the count down, missed it while preparing patch.
3. According to Pci Firmware Spec 3.0, section 5.1.2 Expansion Rom Image length is in units of 512 bytes, so I understand that we dont need to handle the tail

[-- Attachment #2: Type: text/html, Size: 325 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-10  7:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-08  4:05 [PATCH] MdeModulePkg/Bus/Pci/PciBusDxe: Increase the width of the data read during oprom shadow Sumana Venur
2021-01-08  8:16 ` [edk2-devel] " Laszlo Ersek
2021-01-10  7:52   ` Ni, Ray
  -- strict thread matches above, loose matches on Subject: below --
2021-01-08  2:32 Sumana Venur
2021-01-08  3:31 ` [edk2-devel] " Michael D Kinney
2021-01-07  6:29 Sumana Venur
2021-01-07 13:31 ` [edk2-devel] " Laszlo Ersek
2021-01-08  2:37   ` Sumana Venur

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox