From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 6C5CA22492755 for ; Fri, 2 Mar 2018 02:13:25 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 055F54075168; Fri, 2 Mar 2018 10:19:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-104.rdu2.redhat.com [10.10.120.104]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8AE0F213AEF8; Fri, 2 Mar 2018 10:19:32 +0000 (UTC) To: Guo Heyi Cc: edk2-devel@lists.01.org, Jordan Justen , Anthony Perard , Julien Grall , Ruiyu Ni , Ard Biesheuvel References: <1519887444-75510-1-git-send-email-heyi.guo@linaro.org> <1519887444-75510-3-git-send-email-heyi.guo@linaro.org> <68122225-c3d3-14e6-053c-538db503e8f8@redhat.com> <20180301104854.GD39361@SZX1000114654> From: Laszlo Ersek Message-ID: Date: Fri, 2 Mar 2018 11:19:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180301104854.GD39361@SZX1000114654> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 02 Mar 2018 10:19:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Fri, 02 Mar 2018 10:19:34 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v5 2/6] OvmfPkg/PciHostBridgeLib: Init PCI aperture to 0 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Mar 2018 10:13:25 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 03/01/18 11:48, Guo Heyi wrote: > On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote: >> On 03/01/18 07:57, Heyi Guo wrote: >>> diff --git a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c >>> index 31c63ae19e0a..aaf101dfcb0e 100644 >>> --- a/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c >>> +++ b/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c >>> @@ -193,6 +193,11 @@ ScanForRootBridges ( >>> >>> *NumberOfRootBridges = 0; >>> RootBridges = NULL; >>> + ZeroMem (&Io, sizeof (Io)); >>> + ZeroMem (&Mem, sizeof (Mem)); >>> + ZeroMem (&MemAbove4G, sizeof (MemAbove4G)); >>> + ZeroMem (&PMem, sizeof (PMem)); >>> + ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G)); >>> >>> // >>> // After scanning all the PCI devices on the PCI root bridge's primary bus, >>> >> >> these ZeroMem() calls are not in the correct place. Please move them >> into the "PrimaryBus" loop just underneath. That loop works like >> this: >> >> For each primary bus: >> >> (1) set all of the aperture variables to "nonexistent": >> >> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64; >> Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0; >> >> (2) accumulate the BARs of the devices on the bus into the aperture >> variables >> >> (3) call InitRootBridge() with the aperture variables >> >> >> That is, the ZeroMem() calls that you are adding have to be part of >> step (1). So, please replace the assignments >> >> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64; >> Io.Limit = Mem.Limit = MemAbove4G.Limit = PMem.Limit = PMemAbove4G.Limit = 0; >> >> with >> >> ZeroMem (&Io, sizeof (Io)); >> ZeroMem (&Mem, sizeof (Mem)); >> ZeroMem (&MemAbove4G, sizeof (MemAbove4G)); >> ZeroMem (&PMem, sizeof (PMem)); >> ZeroMem (&PMemAbove4G, sizeof (PMemAbove4G)); >> Io.Base = Mem.Base = MemAbove4G.Base = PMem.Base = PMemAbove4G.Base = MAX_UINT64; > > Will it cause functional issue? > > My idea of making the change is like this: > > 1. ZeroMem() is used to initialize all fields of APERTURE to 0; it can > make it in the current place of the patch; > > 2. In the loop, some fields may be changed by the end of each > iteration, and it is the responsibility of the existing code to > re-initialize the changed fields to expected values explicitly. It > seems not necessary to re-initialize the other fields which will > not be changed. > > However, your advice may be better that merges the initialization code > together. I can make the change in the next version of patch. Yes, if it's not a big problem for you, please implement my request. Going forward I wouldn't like to depend on such intricate details as described in your point (2). Namely, in any other C project, I would suggest that we write: for (PrimaryBus = 0; PrimaryBus <= PCI_MAX_BUS; PrimaryBus = SubBus + 1) { PCI_ROOT_BRIDGE_APERTURE Io = { .Base = MAX_UINT64 }, Mem = Io, MemAbove4G = Io, PMem = Io, PMemAbove4G = Io; /* ... */ } In other words, I would: - move the definition of the structs into the loop (sort of accepted, but not really liked in edk2), - use real C initialization (forbidden in edk2), - use designated initializers for the first object, which clears the unlisted fields (C99, forbidden in edk2), - initialize the rest of the structs from the first struct where I used the designated initializer explicitly. Moving the ZeroMem() into the loop is the closest approximation of this, for edk2. Thanks! Laszlo