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

["Zeng, Star" <star.zeng@intel.com>]

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
>