From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5EA98210FBEF0 for ; Fri, 24 Aug 2018 01:16:53 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Aug 2018 01:16:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,281,1531810800"; d="scan'208";a="65677058" Received: from shzintpr02.sh.intel.com (HELO [10.253.24.58]) ([10.239.4.160]) by fmsmga008.fm.intel.com with ESMTP; 24 Aug 2018 01:16:38 -0700 To: Ruiyu Ni , edk2-devel@lists.01.org References: <20180823025353.137924-1-ruiyu.ni@intel.com> <20180823025353.137924-3-ruiyu.ni@intel.com> Cc: star.zeng@intel.com From: "Zeng, Star" Message-ID: Date: Fri, 24 Aug 2018 16:16:07 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20180823025353.137924-3-ruiyu.ni@intel.com> Subject: Re: [PATCH 2/2] MdeModulePkg/PciBus: Restrict one VGA per HostBridge not Segment X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Aug 2018 08:16:53 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Star Zeng > --- > 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 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 >