From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 70518208F7AD7 for ; Tue, 12 Sep 2017 01:37:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8E4D94E4C3; Tue, 12 Sep 2017 08:40:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 8E4D94E4C3 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-50.rdu2.redhat.com [10.10.120.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id E86285E1A9; Tue, 12 Sep 2017 08:40:33 +0000 (UTC) To: Ard Biesheuvel , "Ni, Ruiyu" Cc: "edk2-devel@lists.01.org" , Dong Wei , Benjamin Herrenschmidt , Andrew Fish References: <20170911050121.85708-1-ruiyu.ni@intel.com> <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BA2BABB@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <5f8f5a1b-a170-c20b-91e0-6e813faf0527@redhat.com> Date: Tue, 12 Sep 2017 10:40:32 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 12 Sep 2017 08:40:35 +0000 (UTC) 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 08:37:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/12/17 08:44, Ard Biesheuvel wrote: > On 12 September 2017 at 06:01, Ni, Ruiyu wrote: >> Laszlo, >> Your understanding is: DeviceAddress = HostAddress + AddressTranslationOffset >> But my patch assumes: HostAddress = DeviceAddress + AddressTranslationOffset >> >> They are totally different. If I follow your understanding, the patch is wrong! >> Since UEFI spec doesn't describe "apply to" in sentence " Offset to apply to the >> Starting address of a BAR to convert it to a PCI address" very clearly, I quoted >> the statement from ACPI spec. >> Your understanding to "apply to" is "add", my understanding is "minus". >> > > Even though we are stretching the ACPI definition of a QWord > descriptor beyond its original meaning, I don't think there is a lot > of ambiguity here, to be honest. The AddrRangeMin field contains the > address on the secondary side of a bridge, and the primary value can > be obtained by 'applying' the ATO. In my opinion, applying a (positive > or negative) offset implies addition, not subtraction, as subtraction > involves negating the second addend before applying it. And the > secondary side of the host bridge is clearly the PCI side. Wait, now I'm even more confused. (1) Up-thread you wrote, "AddrRangeMin is indeed already defined to be a host address [...]". (2) Here you write, "the secondary side of the host bridge is clearly the PCI side [...] The AddrRangeMin field contains the address on the secondary side of a bridge". --> This means that AddrRangeMin is a PCI address. Thus, to me these statements appear to conflict. > It does mean the offset field is signed, though. > > So I don't agree with the conclusion that no clarification is > required. We need to make sure the spec is crystal clear in this > regard. But I do agree with the change, I think it is the only > solution that makes sense. My understanding of "Table 121. QWORD Address Space Descriptor" is: - AddrRangeMin --> host address. - ATO --> the UINT64 value that the *caller* of GetBarAttributes() has to add, in UINT64 modular arithmetic, to AddrRangeMin, to calculate the PCI address, after GetBarAttributes() returns. Now, if I understand the *patch* correctly, - the current (pre-patch) code returns a PCI address in "Descriptor->AddrRangeMin", which is wrong, - in addition, we already have the ATO, in "Descriptor->AddrTranslationOffset", that we have to add to the PCI address, to end up with a host address. If that's the case, then I think the patch is good, but it is incomplete. Namely, - To return a host address to the caller in "Descriptor->AddrRangeMin", we add the ATO to it, fetched from the Root Bridge IO protocol. Great. - However, think of what happens when the caller wants to recompute the PCI address! According to the UEFI spec, the ATO that the caller gets in the QWORD descriptor has to be *added* to AddrRangeMin. This means that, the client code would ultimately result in: ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + OriginalATO This makes no sense. In order to end up with the original PCI address, the client side ATO must be the modular UINT64 *negative* of the original ATO, so that they ultimately cancel out on the client side, like this: ClientSidePciAddress == (OriginalPciAddress + OriginalATO) + ClientSideATO == (OriginalPciAddress + OriginalATO) + (-OriginalATO) == OriginalPciAddress Therefore, I think that the patch must, *in addition*, negate the ATO before returning, like this: + Descriptor->AddrRangeMin += Descriptor->AddrTranslationOffset; + Descriptor->AddrTranslationOffset = (-Descriptor->AddrTranslationOffset); Thanks Laszlo