From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x232.google.com (mail-it0-x232.google.com [IPv6:2607:f8b0:4001:c0b::232]) (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 4C5A621CEB0FE for ; Mon, 11 Sep 2017 21:29:23 -0700 (PDT) Received: by mail-it0-x232.google.com with SMTP id c195so25105731itb.1 for ; Mon, 11 Sep 2017 21:32:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=fg0ASr1RcEcS9OYp8EqAXk5I1Aj0TlzUuDfn58LFJzY=; b=Tx0sB9S7AnS1EsBtry1HANZxLBi4/5MpK0hX9ECnbJHwPitORRREva16D7WFvbvw0/ OZsX7A4qAT0kCnl30re/KF7oyl4ASnGzfYB/iCHo284ViGI6xzyUA6CMK+gu11784FCs H20sT7MzT6HiTNR7XA83KV08UM2CF8EqYLFM8= 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=fg0ASr1RcEcS9OYp8EqAXk5I1Aj0TlzUuDfn58LFJzY=; b=TBWzGyY2/llt3hDSHxRhpj6Vma5AT+I9eBSdSdTmoGN8D5V95EaTOkvw+N8vFY+25F CPlnGVuBsiazXILvo/Lkacvi0QHdIwvZyLvPV80I0wTGxscXsCNHkybJW1rldOms3EEZ bKq3RJe4WUiSPWATz9Gc2XCQOeOce79dxhf64J1F9Ux1ROIO5UgXHV5P30aTtUR+Etbn I+eueRxI+1xqOb+bTWoMPv7RxJUfmQgXawjCDGTOnne7reJbFESgTRYra9BftWmOSsD+ DPtlfyCMajNVPk8Hk08vDK7dLijILTWiNsss6Y4Kk2DCRVcJCyQp1G/gbuGJ/XNgvaOL 4xEA== X-Gm-Message-State: AHPjjUj4Bn19Al4g4JNKuwExkXOV7kJbWA8r65vLnrm9sMi/u+IFo4Zm lOjQCLmiVYAihNG+vJgL89BYGwCgB9qYSv4kf6nMbw== X-Google-Smtp-Source: AOwi7QBI/qmQFynv7Wr6YPwmCmW1KT2AmqG0Z5rxNZ5vvr3faliZQuj43sOpK5X4DvJSj3JjHmwKRYgSmoIv7dnX3AI= X-Received: by 10.36.11.196 with SMTP id 187mr16384251itd.48.1505190738494; Mon, 11 Sep 2017 21:32:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.152.18 with HTTP; Mon, 11 Sep 2017 21:32:17 -0700 (PDT) In-Reply-To: <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> References: <20170911050121.85708-1-ruiyu.ni@intel.com> <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> From: Ard Biesheuvel Date: Tue, 12 Sep 2017 05:32:17 +0100 Message-ID: To: Laszlo Ersek Cc: Ruiyu Ni , "edk2-devel@lists.01.org" , Benjamin Herrenschmidt , Andrew Fish , Dong Wei , Bartosz Szczepanek Subject: Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Sep 2017 04:29:23 -0000 Content-Type: text/plain; charset="UTF-8" On 11 September 2017 at 07:46, Laszlo Ersek wrote: > On 09/11/17 07:01, Ruiyu Ni wrote: >> Per the UEFI Spec, GetBarAttributes() should return the Host address. >> But current implementation returns the address read from the BAR, >> which is the Device address. >> Per the description of AddressTranslationOffset in ACPI spec: >> "For bridges that translate addresses across the bridge, this is the >> offset that must be added to the address on the secondary side to >> obtain the address on the primary side." > > The ACPI spec also says: > > "Non-bridge devices must list 0 for all Address Translation offset bits." > > However, the UEFI spec (v2.7) says, under > EFI_PCI_IO_PROTOCOL.GetBarAttributes(): > > "The ACPI Specification does not define how to the use the Address > Translation Offset for non-bridge devices. The UEFI Specification is > extending the definition of Address Translation Offset to support > systems that have different mapping for HostAddress and DeviceAddress. > [...] Address Translation Offset. Offset to apply to the Starting > address of a BAR to convert it to a PCI address. This value is zero > unless the HostAddress and DeviceAddress for the BAR are different." > > So, I think the patch is correct, but the commit message should not > refer to the ACPI spec. It should refer to / quote the UEFI spec only. > >> HostAddress = DeviceAddress + AddressTranslationOffset. > > The sentences from the UEFI spec are "Address Translation Offset. Offset > to apply to the Starting address of a BAR to convert it to a PCI > address", and "Address Range Minimum. Starting address of BAR." > > To me this seems to imply that AddrRangeMin is already a host address, and > > DeviceAddress = AddrRangeMin + AddressTranslationOffset > >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ruiyu Ni >> Cc: Ard Biesheuvel >> Cc: Benjamin Herrenschmidt >> Cc: Andrew Fish >> Cc: Dong Wei >> Cc: Laszlo Ersek >> Cc: Bartosz Szczepanek >> --- >> MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> index cc7125e4fc..852d35d710 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> @@ -1955,7 +1955,7 @@ PciIoGetBarAttributes ( >> End->Checksum = 0; >> >> // >> - // Get the Address Translation Offset >> + // Get the Address Translation Offset and convert the Device address to Host address. >> // >> if (Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { >> Descriptor->AddrTranslationOffset = GetMmioAddressTranslationOffset ( >> @@ -1967,6 +1967,7 @@ PciIoGetBarAttributes ( >> FreePool (Descriptor); >> return EFI_UNSUPPORTED; >> } >> + Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset; >> } >> } >> >> > > Actually, let me circle back to the initial problem here (apologies if > it's too late for that) -- why are we adding the offset inside the > GetBarAttributes() function? Isn't it the caller's responsibility to do > the addition after GetBarAttributes() returns? > AddrRangeMin is indeed already defined to be a host address, which means the code that returns it should apply the offset to the raw BAR value.