[PATCH 0/2] MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment

[PATCH 0/2] MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment

[Ruiyu Ni]

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

Today's restriction of VGA device is to have only one VGA device
enabled per PCI segment. It's not correct because several segments
may share one IO / MMIO address space.
We should restrict to have one VGA per Host Bridge because each
Host Bridge has its only IO / MMIO address space.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>

Ruiyu Ni (2):
  MdeModulePkg/PciBus: Refine ActiveVGADeviceOnTheRootBridge
  MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment

 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 55 +++++++++++------------
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 20 ++++-----
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c            |  4 +-
 3 files changed, 38 insertions(+), 41 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/2] MdeModulePkg/PciBus: Refine ActiveVGADeviceOnTheRootBridge

[Ruiyu Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1109
The patch doesn't change any behavior of this function.
It just renames the function to LocateVgaDevice() and renames
some parameters and local variables.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 35 +++++++++++------------
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 10 +++----
 2 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index f7039da992..fdec0bcd53 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -1002,7 +1002,7 @@ ActiveVGADeviceOnTheSameSegment (
 
     if (Temp->PciRootBridgeIo->SegmentNumber == VgaDevice->PciRootBridgeIo->SegmentNumber) {
 
-      Temp = ActiveVGADeviceOnTheRootBridge (Temp);
+      Temp = LocateVgaDevice (Temp);
 
       if (Temp != NULL) {
         return Temp;
@@ -1016,41 +1016,41 @@ ActiveVGADeviceOnTheSameSegment (
 }
 
 /**
-  Get the active VGA device on the root bridge.
+  Locate the active VGA device under the bridge.
 
-  @param RootBridge  PCI IO instance for the root bridge.
+  @param Bridge  PCI IO instance for the bridge.
 
   @return The active VGA device.
 
 **/
 PCI_IO_DEVICE *
-ActiveVGADeviceOnTheRootBridge (
-  IN PCI_IO_DEVICE        *RootBridge
+LocateVgaDevice (
+  IN PCI_IO_DEVICE        *Bridge
   )
 {
   LIST_ENTRY      *CurrentLink;
-  PCI_IO_DEVICE   *Temp;
+  PCI_IO_DEVICE   *PciIo;
 
-  CurrentLink = RootBridge->ChildList.ForwardLink;
+  CurrentLink = Bridge->ChildList.ForwardLink;
 
-  while (CurrentLink != NULL && CurrentLink != &RootBridge->ChildList) {
+  while (CurrentLink != NULL && CurrentLink != &Bridge->ChildList) {
 
-    Temp = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
+    PciIo = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
 
-    if (IS_PCI_VGA(&Temp->Pci) &&
-        (Temp->Attributes &
+    if (IS_PCI_VGA(&PciIo->Pci) &&
+        (PciIo->Attributes &
          (EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY |
           EFI_PCI_IO_ATTRIBUTE_VGA_IO     |
           EFI_PCI_IO_ATTRIBUTE_VGA_IO_16)) != 0) {
-      return Temp;
+      return PciIo;
     }
 
-    if (IS_PCI_BRIDGE (&Temp->Pci)) {
+    if (IS_PCI_BRIDGE (&PciIo->Pci)) {
 
-      Temp = ActiveVGADeviceOnTheRootBridge (Temp);
+      PciIo = LocateVgaDevice (PciIo);
 
-      if (Temp != NULL) {
-        return Temp;
+      if (PciIo != NULL) {
+        return PciIo;
       }
     }
 
@@ -1060,6 +1060,3 @@ ActiveVGADeviceOnTheRootBridge (
   return NULL;
 }
 
-
-
-
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
index c282381f85..1ec2178a21 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
@@ -1,7 +1,7 @@
 /** @file
   Supporting functions declaration for PCI devices management.
 
-Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -243,16 +243,16 @@ ActiveVGADeviceOnTheSameSegment (
   );
 
 /**
-  Get the active VGA device on the root bridge.
+  Locate the active VGA device under the bridge.
 
-  @param RootBridge  PCI IO instance for the root bridge.
+  @param Bridge  PCI IO instance for the bridge.
 
   @return The active VGA device.
 
 **/
 PCI_IO_DEVICE *
-ActiveVGADeviceOnTheRootBridge (
-  IN PCI_IO_DEVICE        *RootBridge
+LocateVgaDevice (
+  IN PCI_IO_DEVICE        *Bridge
   );
 
 
-- 
2.16.1.windows.1

[PATCH 2/2] MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment

[Ruiyu Ni]

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

Today's restriction of VGA device is to have only one VGA device
enabled per PCI segment. It's not correct because several segments
may share one IO / MMIO address space.
We should restrict to have one VGA per Host Bridge because each
Host Bridge has its only IO / MMIO address space.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 22 +++++++++++-----------
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 10 +++++-----
 MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c            |  4 ++--
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index fdec0bcd53..fbcc53e41a 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -979,33 +979,33 @@ PciDeviceExisted (
 }
 
 /**
-  Get the active VGA device on the same segment.
+  Get the active VGA device on the specified Host Bridge.
 
-  @param VgaDevice    PCI IO instance for the VGA device.
+  @param HostBridgeHandle    Host Bridge handle.
 
-  @return The active VGA device on the same segment.
+  @return The active VGA device on the specified Host Bridge.
 
 **/
 PCI_IO_DEVICE *
-ActiveVGADeviceOnTheSameSegment (
-  IN PCI_IO_DEVICE        *VgaDevice
+LocateVgaDeviceOnHostBridge (
+  IN EFI_HANDLE           HostBridgeHandle
   )
 {
   LIST_ENTRY      *CurrentLink;
-  PCI_IO_DEVICE   *Temp;
+  PCI_IO_DEVICE   *PciIo;
 
   CurrentLink = mPciDevicePool.ForwardLink;
 
   while (CurrentLink != NULL && CurrentLink != &mPciDevicePool) {
 
-    Temp = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
+    PciIo = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
 
-    if (Temp->PciRootBridgeIo->SegmentNumber == VgaDevice->PciRootBridgeIo->SegmentNumber) {
+    if (PciIo->PciRootBridgeIo->ParentHandle== HostBridgeHandle) {
 
-      Temp = LocateVgaDevice (Temp);
+      PciIo = LocateVgaDevice (PciIo);
 
-      if (Temp != NULL) {
-        return Temp;
+      if (PciIo != NULL) {
+        return PciIo;
       }
     }
 
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
index 1ec2178a21..b45d2a5d77 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
@@ -230,16 +230,16 @@ PciDeviceExisted (
   );
 
 /**
-  Get the active VGA device on the same segment.
+  Get the active VGA device on the specified Host Bridge.
 
-  @param VgaDevice    PCI IO instance for the VGA device.
+  @param HostBridgeHandle    Host Bridge handle.
 
-  @return The active VGA device on the same segment.
+  @return The active VGA device on the specified Host Bridge.
 
 **/
 PCI_IO_DEVICE *
-ActiveVGADeviceOnTheSameSegment (
-  IN PCI_IO_DEVICE        *VgaDevice
+LocateVgaDeviceOnHostBridge (
+  IN EFI_HANDLE           HostBridgeHandle
   );
 
 /**
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
index 291578c63c..333b875ff0 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
@@ -1415,7 +1415,7 @@ SupportPaletteSnoopAttributes (
   //
   // Get the boot VGA on the same segement
   //
-  Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
+  Temp = LocateVgaDeviceOnHostBridge (PciIoDevice->PciRootBridgeIo->ParentHandle);
 
   if (Temp == NULL) {
     //
@@ -1670,7 +1670,7 @@ PciIoAttributes (
         //
         // Check if there have been an active VGA device on the same segment
         //
-        Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
+        Temp = LocateVgaDeviceOnHostBridge (PciIoDevice->PciRootBridgeIo->ParentHandle);
         if (Temp != NULL && Temp != PciIoDevice) {
           //
           // An active VGA has been detected, so can not enable another
-- 
2.16.1.windows.1

Re: [PATCH 1/2] MdeModulePkg/PciBus: Refine ActiveVGADeviceOnTheRootBridge

[Zeng, Star]

On 2018/8/23 10:53, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1109
> The patch doesn't change any behavior of this function.
> It just renames the function to LocateVgaDevice() and renames
> some parameters and local variables.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 35 +++++++++++------------
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 10 +++----
>   2 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> index f7039da992..fdec0bcd53 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> @@ -1002,7 +1002,7 @@ ActiveVGADeviceOnTheSameSegment (
>   
>       if (Temp->PciRootBridgeIo->SegmentNumber == VgaDevice->PciRootBridgeIo->SegmentNumber) {
>   
> -      Temp = ActiveVGADeviceOnTheRootBridge (Temp);
> +      Temp = LocateVgaDevice (Temp);
>   
>         if (Temp != NULL) {
>           return Temp;
> @@ -1016,41 +1016,41 @@ ActiveVGADeviceOnTheSameSegment (
>   }
>   
>   /**
> -  Get the active VGA device on the root bridge.
> +  Locate the active VGA device under the bridge.
>   
> -  @param RootBridge  PCI IO instance for the root bridge.
> +  @param Bridge  PCI IO instance for the bridge.
>   
>     @return The active VGA device.
>   
>   **/
>   PCI_IO_DEVICE *
> -ActiveVGADeviceOnTheRootBridge (
> -  IN PCI_IO_DEVICE        *RootBridge
> +LocateVgaDevice (
> +  IN PCI_IO_DEVICE        *Bridge
>     )
>   {
>     LIST_ENTRY      *CurrentLink;
> -  PCI_IO_DEVICE   *Temp;
> +  PCI_IO_DEVICE   *PciIo;

How about using PciIoDev as the local variable name? PciIo makes me 
regard it for PciIo protocol at the first glance. :)

With the new name,
Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star

>   
> -  CurrentLink = RootBridge->ChildList.ForwardLink;
> +  CurrentLink = Bridge->ChildList.ForwardLink;
>   
> -  while (CurrentLink != NULL && CurrentLink != &RootBridge->ChildList) {
> +  while (CurrentLink != NULL && CurrentLink != &Bridge->ChildList) {
>   
> -    Temp = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
> +    PciIo = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
>   
> -    if (IS_PCI_VGA(&Temp->Pci) &&
> -        (Temp->Attributes &
> +    if (IS_PCI_VGA(&PciIo->Pci) &&
> +        (PciIo->Attributes &
>            (EFI_PCI_IO_ATTRIBUTE_VGA_MEMORY |
>             EFI_PCI_IO_ATTRIBUTE_VGA_IO     |
>             EFI_PCI_IO_ATTRIBUTE_VGA_IO_16)) != 0) {
> -      return Temp;
> +      return PciIo;
>       }
>   
> -    if (IS_PCI_BRIDGE (&Temp->Pci)) {
> +    if (IS_PCI_BRIDGE (&PciIo->Pci)) {
>   
> -      Temp = ActiveVGADeviceOnTheRootBridge (Temp);
> +      PciIo = LocateVgaDevice (PciIo);
>   
> -      if (Temp != NULL) {
> -        return Temp;
> +      if (PciIo != NULL) {
> +        return PciIo;
>         }
>       }
>   
> @@ -1060,6 +1060,3 @@ ActiveVGADeviceOnTheRootBridge (
>     return NULL;
>   }
>   
> -
> -
> -
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> index c282381f85..1ec2178a21 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> @@ -1,7 +1,7 @@
>   /** @file
>     Supporting functions declaration for PCI devices management.
>   
> -Copyright (c) 2006 - 2009, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD License
>   which accompanies this distribution.  The full text of the license may be found at
> @@ -243,16 +243,16 @@ ActiveVGADeviceOnTheSameSegment (
>     );
>   
>   /**
> -  Get the active VGA device on the root bridge.
> +  Locate the active VGA device under the bridge.
>   
> -  @param RootBridge  PCI IO instance for the root bridge.
> +  @param Bridge  PCI IO instance for the bridge.
>   
>     @return The active VGA device.
>   
>   **/
>   PCI_IO_DEVICE *
> -ActiveVGADeviceOnTheRootBridge (
> -  IN PCI_IO_DEVICE        *RootBridge
> +LocateVgaDevice (
> +  IN PCI_IO_DEVICE        *Bridge
>     );
>   
>   
> 

Re: [PATCH 2/2] MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment

[Zeng, Star]

On 2018/8/23 10:53, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1109
> 
> Today's restriction of VGA device is to have only one VGA device
> enabled per PCI segment. It's not correct because several segments
> may share one IO / MMIO address space.
> We should restrict to have one VGA per Host Bridge because each
> Host Bridge has its only IO / MMIO address space.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 22 +++++++++++-----------
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h | 10 +++++-----
>   MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c            |  4 ++--
>   3 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> index fdec0bcd53..fbcc53e41a 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> @@ -979,33 +979,33 @@ PciDeviceExisted (
>   }
>   
>   /**
> -  Get the active VGA device on the same segment.
> +  Get the active VGA device on the specified Host Bridge.
>   
> -  @param VgaDevice    PCI IO instance for the VGA device.
> +  @param HostBridgeHandle    Host Bridge handle.
>   
> -  @return The active VGA device on the same segment.
> +  @return The active VGA device on the specified Host Bridge.
>   
>   **/
>   PCI_IO_DEVICE *
> -ActiveVGADeviceOnTheSameSegment (
> -  IN PCI_IO_DEVICE        *VgaDevice
> +LocateVgaDeviceOnHostBridge (
> +  IN EFI_HANDLE           HostBridgeHandle
>     )
>   {
>     LIST_ENTRY      *CurrentLink;
> -  PCI_IO_DEVICE   *Temp;
> +  PCI_IO_DEVICE   *PciIo;

How about using PciIoDev as the local variable name? PciIo makes me 
regard it for PciIo protocol at the first glance. :)

There are still two minor comments for the code pieces below.

>   
>     CurrentLink = mPciDevicePool.ForwardLink;
>   
>     while (CurrentLink != NULL && CurrentLink != &mPciDevicePool) {
>   
> -    Temp = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
> +    PciIo = PCI_IO_DEVICE_FROM_LINK (CurrentLink);
>   
> -    if (Temp->PciRootBridgeIo->SegmentNumber == VgaDevice->PciRootBridgeIo->SegmentNumber) {
> +    if (PciIo->PciRootBridgeIo->ParentHandle== HostBridgeHandle) {
>   
> -      Temp = LocateVgaDevice (Temp);
> +      PciIo = LocateVgaDevice (PciIo);
>   
> -      if (Temp != NULL) {
> -        return Temp;
> +      if (PciIo != NULL) {
> +        return PciIo;
>         }
>       }
>   
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> index 1ec2178a21..b45d2a5d77 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.h
> @@ -230,16 +230,16 @@ PciDeviceExisted (
>     );
>   
>   /**
> -  Get the active VGA device on the same segment.
> +  Get the active VGA device on the specified Host Bridge.
>   
> -  @param VgaDevice    PCI IO instance for the VGA device.
> +  @param HostBridgeHandle    Host Bridge handle.
>   
> -  @return The active VGA device on the same segment.
> +  @return The active VGA device on the specified Host Bridge.
>   
>   **/
>   PCI_IO_DEVICE *
> -ActiveVGADeviceOnTheSameSegment (
> -  IN PCI_IO_DEVICE        *VgaDevice
> +LocateVgaDeviceOnHostBridge (
> +  IN EFI_HANDLE           HostBridgeHandle
>     );
>   
>   /**
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> index 291578c63c..333b875ff0 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c
> @@ -1415,7 +1415,7 @@ SupportPaletteSnoopAttributes (
>     //
>     // Get the boot VGA on the same segement

Please also update this code comment.

>     //
> -  Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
> +  Temp = LocateVgaDeviceOnHostBridge (PciIoDevice->PciRootBridgeIo->ParentHandle);
>   
>     if (Temp == NULL) {
>       //
> @@ -1670,7 +1670,7 @@ PciIoAttributes (
>           //
>           // Check if there have been an active VGA device on the same segment

Please also update this code comment.

With the new name and code comments updated,
Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star

>           //
> -        Temp = ActiveVGADeviceOnTheSameSegment (PciIoDevice);
> +        Temp = LocateVgaDeviceOnHostBridge (PciIoDevice->PciRootBridgeIo->ParentHandle);
>           if (Temp != NULL && Temp != PciIoDevice) {
>             //
>             // An active VGA has been detected, so can not enable another
>