From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::233; helo=mail-io0-x233.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x233.google.com (mail-io0-x233.google.com [IPv6:2607:f8b0:4001:c06::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C57DE2257C2BD for ; Mon, 5 Mar 2018 00:17:06 -0800 (PST) Received: by mail-io0-x233.google.com with SMTP id l12so17008582ioc.10 for ; Mon, 05 Mar 2018 00:23:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=uswl7G5vOFnOYhTrP4xiNsGBQmU5lzjRKmgkGdLfugI=; b=ZCDoLAjkXqKcCEScbYhzRI5HpbmxDWB2QtqibgZnAFLG1Y3uKsWLU9fXADDSCFCM63 4TEpqI1DGT7IdfQte3qX0VXraO/BbYTozuLewlPNKDnmoF1X7wKCQM+k8TTwmlu3itR1 1QsJwCvrffhHSH/6ldH+OsNvUDtjxsODdSI9g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=uswl7G5vOFnOYhTrP4xiNsGBQmU5lzjRKmgkGdLfugI=; b=gllCNJm/Org3UK79dhawyi2tde2kfTyiG/3D53nrlz0lLcHLCvlH6ttdqKikSbcDti TC/jSjq5t2S0mZmsNy8dQ2hMaMWJVeQlrkTbXKY4kTKAAhULkHyJWPtE0B815j6sc2eK +rqQB/+jpFGO5B4peitLTli/ccEaKA+othqVV+v9k6VSu0deFN2orebLnZn57fCP3HJD 8mBvS5ww2OMtjXPml1ey85F8ID8Mb1MWKUzMo/BvEt4EfN2UBDdPVmVYcogS068eHM6g 6kEp+kVsG0krnCo+O0FHpa7h0hDarpry59CnudIqUkxQZWtpm/8GMlEkf7vKE2yYe0sr YCow== X-Gm-Message-State: AElRT7GGbQmZL0wX2C9sVIordwTqYP1XeYrcdphxIVnciiwT8sdtgesW ZtG7OOyb6SNjHwzOYdRJqHB/nA== X-Google-Smtp-Source: AG47ELvgUW0sjhoQLTcXUblSdwcJnXk5j7Hx8SsSSL26hlIkxx9GbRmR0C6nFkvw56tMJyXH0atonQ== X-Received: by 10.107.198.151 with SMTP id w145mr15862692iof.255.1520238198371; Mon, 05 Mar 2018 00:23:18 -0800 (PST) Received: from SZX1000114654 ([45.56.152.76]) by smtp.gmail.com with ESMTPSA id e94sm4291575itd.21.2018.03.05.00.23.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Mar 2018 00:23:17 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Mon, 5 Mar 2018 16:23:13 +0800 To: Laszlo Ersek Cc: Guo Heyi , edk2-devel@lists.01.org, Jordan Justen , Anthony Perard , Julien Grall , Ruiyu Ni , Ard Biesheuvel Message-ID: <20180305082313.GB86523@SZX1000114654> 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> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) 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: Mon, 05 Mar 2018 08:17:07 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Mar 02, 2018 at 11:19:31AM +0100, Laszlo Ersek wrote: > 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. OK, I can do that in the next version of patch. Thanks, Heyi > > Thanks! > Laszlo