From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-x241.google.com (mail-vk0-x241.google.com [IPv6:2607:f8b0:400c:c05::241]) (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 A618681AF2 for ; Mon, 16 Jan 2017 10:44:54 -0800 (PST) Received: by mail-vk0-x241.google.com with SMTP id 23so9348068vkc.2 for ; Mon, 16 Jan 2017 10:44:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=SA86lb6rNRfgMnZuj+7dvSeyUDIBqdBEYoTZMQzHFJA=; b=CvZHZXagTIJCn9xxGiVzmxzX3xYDWbTtjZwmUalYqgUilaWls9N+jnvIq9ICvW40yN EnfxHvNN2UtdQwYF7xfBFUw3t6tCr3qilyMHqhIUdB91thuNsNrMNmkv5Phx5vIz26st OjtuvXhBIEkP8VcJZBi52lPQq4GYMl+eI3b9bDtOFQfyBJoDwPbxs01Q1KVgUQQcb6va 0MqdOLtqwMGg+ty7AstRCKapKbv3U6rK//2rm1vmqlUY/IXCcaGdOY2ZwaGJR70E/f1W YGSYUQ/UFJx+6bH6YI8FmhP/158Nhgmtyvs26rt9txIYoDKHJxir1a+J0G7kS+pPBNoy kfLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=SA86lb6rNRfgMnZuj+7dvSeyUDIBqdBEYoTZMQzHFJA=; b=uXfs8Z8jVwCU9TZnkrKiBECF1xOtYSogfpK4zsi95RsZM0uqFWggh40skpgkL6h9EV q+ZqFjx8P+MQqS7jOu/SZtcgFsNtm2eAvGZRYR7cSW9GLv89uo/rCacCvAE6OOttxwOR WZbnRl7hJb8WhnpVO1zMgxwD2nMwP7P8uHSDlRNqdnnltQLtfRCe/oQzKydam50w+9Sd L5nzfeQhPPXRnWqOn20vohoZXhTStu0qnPv8dIVG4KScpLjhGiwysloUN5QScXJz/daZ kJnlNvpSW6KzJ3Gze6cbBwMAWusjRfIbC+JYkYYLr71yHZEyEYNNaPpvBg+h/fWSZ1Zq RE/A== X-Gm-Message-State: AIkVDXJSzCu3X0dGmpaGsMFl76i368AELcoxjwpDYBCEkMGv4a3/d+5Zsdxm9TRlmwd2SmObBx7ViOuLItXn9A== X-Received: by 10.31.170.15 with SMTP id t15mr1787556vke.6.1484592294314; Mon, 16 Jan 2017 10:44:54 -0800 (PST) MIME-Version: 1.0 Received: by 10.31.151.68 with HTTP; Mon, 16 Jan 2017 10:44:53 -0800 (PST) In-Reply-To: References: <20170116180940.GO25883@bivouac.eciton.net> From: Michael Zimmermann Date: Mon, 16 Jan 2017 19:44:53 +0100 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH] ArmPlatformPkg: MemoryInitPei: fix wrong check for system memory X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Jan 2017 18:44:54 -0000 Content-Type: text/plain; charset=UTF-8 mh after thinking about it, wouldn't it be better to swap the '<=' operands instead of changing the operator? this way the PCD memory would be on the left side for both comparisons which seems more clear. On Mon, Jan 16, 2017 at 7:43 PM, Michael Zimmermann wrote: > The code in question is supposed to check if the memory range defined > in the PCD is fully included in the memory resource descriptor of the > current iteration. The condition for that is wrong. example: > PcdSystemMemoryBase = 0x80000000 > PcdSystemMemorySize = 0x10000000 > ResourceDescriptor->PhysicalStart = 0x40000000 > NextHob.ResourceDescriptor->ResourceLength = 0x10000000 > > obviously, the resource descriptor doesn't contain the system memory. > but the old logic would have selected this Hob because: > 0x80000000>=0x40000000 && 0x50000000 <= 0x90000000 > The second comparison probably was supposed to have the > ResourceDescriptor values on the right side, > I fixed it by just inverting the direction of the comparison which then becomes: > 0x80000000>=0x40000000 && 0x50000000 >= 0x90000000 > > And this comparison now successfully fails but will still be > successful for the case we're checking for. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Michael Zimmermann > --- > ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > index 2feb11f..8f67812 100644 > --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c > @@ -103,7 +103,7 @@ MemoryPeim ( > while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, > NextHob.Raw)) != NULL) { > if ((NextHob.ResourceDescriptor->ResourceType == > EFI_RESOURCE_SYSTEM_MEMORY) && > (PcdGet64 (PcdSystemMemoryBase) >= > NextHob.ResourceDescriptor->PhysicalStart) && > - (NextHob.ResourceDescriptor->PhysicalStart + > NextHob.ResourceDescriptor->ResourceLength <= PcdGet64 > (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) > + (NextHob.ResourceDescriptor->PhysicalStart + > NextHob.ResourceDescriptor->ResourceLength >= PcdGet64 > (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) > { > Found = TRUE; > break; > > On Mon, Jan 16, 2017 at 7:09 PM, Leif Lindholm wrote: >> Hi Michael, >> >> Could you provide a message body explaining the error that this patch >> fixes so that I don't sprain my brain trying to figure it out? >> >> On Thu, Jan 12, 2017 at 04:46:57PM +0100, Michael Zimmermann wrote: >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Michael Zimmermann >>> --- >>> ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c >>> b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c >>> index 2feb11f21d..8f6781212e 100644 >>> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c >>> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c >>> @@ -103,7 +103,7 @@ MemoryPeim ( >>> while ((NextHob.Raw = GetNextHob (EFI_HOB_TYPE_RESOURCE_DESCRIPTOR, >>> NextHob.Raw)) != NULL) { >>> if ((NextHob.ResourceDescriptor->ResourceType == >>> EFI_RESOURCE_SYSTEM_MEMORY) && >>> (PcdGet64 (PcdSystemMemoryBase) >= >>> NextHob.ResourceDescriptor->PhysicalStart) && >>> - (NextHob.ResourceDescriptor->PhysicalStart + >>> NextHob.ResourceDescriptor->ResourceLength <= PcdGet64 >>> (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) >> >> It looks like the patch got corrupted on submission (lines wrapped). >> Could you resend it please? >> >>> + (NextHob.ResourceDescriptor->PhysicalStart + >>> NextHob.ResourceDescriptor->ResourceLength >= PcdGet64 >>> (PcdSystemMemoryBase) + PcdGet64 (PcdSystemMemorySize))) >>> { >>> Found = TRUE; >>> break; >>> -- >>> 2.11.0 >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel