From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c05::243; helo=mail-pg0-x243.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pg0-x243.google.com (mail-pg0-x243.google.com [IPv6:2607:f8b0:400e:c05::243]) (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 F3B072242382A for ; Thu, 1 Mar 2018 02:42:50 -0800 (PST) Received: by mail-pg0-x243.google.com with SMTP id g12so2151554pgs.0 for ; Thu, 01 Mar 2018 02:48:59 -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=HuTojR8PUf6OP5Gnuc9KLEELGnSHAHf5l/0Hf82HzAc=; b=H4dfKQoc9GpjCR1wqfbHKou2IAGWDypA4K3rexcoqZeZTfgGn8xad9481SBfe0HZtY rEltaZC+Ncr1KLdHiKcirFwGh6GFAGBVoXNut7l+8NHI2TCyY3HynrSeqinYZqeV2n0b d3283WplWrtlMiFeaKaC5Kfj9scKrkIRlwSAo= 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=HuTojR8PUf6OP5Gnuc9KLEELGnSHAHf5l/0Hf82HzAc=; b=cvlUft5dcWMXgpYFEjOKgoMauzCxVAGMZo7xgf0arZ4Cd4XYSNujOxOgXqFLNWxC2r 4Pemdz5Uih++lHXn1GO2qD9poIgEvxfxNWJkfmeYCVd8hDZctcBlOn2+dWRXAONGlrBa yQRY69uKBUTqeQe/YDQ9jAkyRRSH1zDEKz00L20WaMaU8Qs2goNSwBjaZKdz7p4n2hsz mJ5xdo5GrGcNCFBHydiD9QQQqStunRmDpf16Hzls/bdCidW6FcdR3JUpva9OpKe+cJzp KrfFQSOAAwdlqFD6tEPzW0dwiOYiUFuHBNtUrWd39zyiFzBiYYlmri8gIuhcRoXh2cUq Royw== X-Gm-Message-State: APf1xPAEAVkhK4nlJ/jqHvJp1Q3ZICn/e1aRyWC5f/QzN8CAC2avzQz6 tYb5VauQllZzjVePAIZEPmaYCQ== X-Google-Smtp-Source: AG47ELuvygUNSkOcezmaatLA3TLcsf24MQvhvwVASTddWhIS2V2KAN9/8x3+QboTM05bFubf3+Wdqg== X-Received: by 10.98.55.7 with SMTP id e7mr1515771pfa.112.1519901338850; Thu, 01 Mar 2018 02:48:58 -0800 (PST) Received: from SZX1000114654 ([45.56.152.115]) by smtp.gmail.com with ESMTPSA id 76sm8366135pfp.53.2018.03.01.02.48.56 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 01 Mar 2018 02:48:58 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 1 Mar 2018 18:48:54 +0800 To: Laszlo Ersek Cc: Heyi Guo , edk2-devel@lists.01.org, Jordan Justen , Anthony Perard , Julien Grall , Ruiyu Ni , Ard Biesheuvel Message-ID: <20180301104854.GD39361@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> MIME-Version: 1.0 In-Reply-To: <68122225-c3d3-14e6-053c-538db503e8f8@redhat.com> 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: Thu, 01 Mar 2018 10:42:51 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Mar 01, 2018 at 11:17:30AM +0100, Laszlo Ersek wrote: > Hello Heyi, > > On 03/01/18 07:57, Heyi Guo wrote: > > Use ZeroMem to initialize all fields in temporary > > PCI_ROOT_BRIDGE_APERTURE variables to zero. This is not mandatory but > > is helpful for future extension: when we add new fields to > > PCI_ROOT_BRIDGE_APERTURE and the default value of these fields can > > safely be zero, this code will not suffer from an additional > > change. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Heyi Guo > > > > Cc: Jordan Justen > > Cc: Anthony Perard > > Cc: Julien Grall > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Cc: Ard Biesheuvel > > --- > > OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 4 ++++ > > OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 5 +++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > > index ff837035caff..4a650a4c6df9 100644 > > --- a/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > > +++ b/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c > > @@ -217,6 +217,10 @@ PciHostBridgeGetRootBridges ( > > PCI_ROOT_BRIDGE_APERTURE Mem; > > PCI_ROOT_BRIDGE_APERTURE MemAbove4G; > > > > + ZeroMem (&Io, sizeof (Io)); > > + ZeroMem (&Mem, sizeof (Mem)); > > + ZeroMem (&MemAbove4G, sizeof (MemAbove4G)); > > + > > if (PcdGetBool (PcdPciDisableBusEnumeration)) { > > return ScanForRootBridges (Count); > > } > > This is OK. (Although for a trivial perf improvement, you could move the > ZeroMem() calls after the PcdGetBool() / return. Not necessary, up to > you.) > > However: > > > 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. Thanks, Heyi > > Thanks! > Laszlo