From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::234]) (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 9198E21CEB0E3 for ; Tue, 12 Sep 2017 08:46:16 -0700 (PDT) Received: by mail-io0-x234.google.com with SMTP id k101so7779896iod.0 for ; Tue, 12 Sep 2017 08:49:13 -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=Rt+txgOedbv+BJbvYU4NHX2C0d5JYROAmxjcn/7utac=; b=DscjH4nOdD1v0/xfJ7+hCkMoEOIMKqT7iF2zzLhDBTZ6Q1ClEeRsnb0TMGKWKs5d7q w75hk9aZU4cL4g62dFlgWNgjDuC3GBgwjxQ44ot2QndEUYvg2Qtf89O54S4Vjj1RdZpG NBC5wL8RsGCw9GepmbW4EybzrkeDninAhrlXw= 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=Rt+txgOedbv+BJbvYU4NHX2C0d5JYROAmxjcn/7utac=; b=RN9MFvyuOWZzlEpRMb73zLF42oyIyq/MPFBcj22FtbAD8NrB8NYXwoCWd37DEaFShf e59ZkbN1Q4btVAGhQ7rnuCvxX9Q9VVJB4bmPq2VFAPnzUgYosnRx2OzixMMSciMyXD+s Xcz3ZWTyLWaP8Hd/+CDaiUFa0s+qu9IJSeofBblfU7INRVagi7qoSkLEobRd8ISywQ3l TN6E/m4n+uWhM8i09oZuoo8uHtgRlZg+8/s/adT0D3XvYI0dcN8QLRpQn4m72DSfrS8l VNjKNpQ0Kji09pEOXxRSuFyOWOr8bdI1gSQHbDSY+P3Z99ME/jQK45RjjpiAHZr1cJCE I28w== X-Gm-Message-State: AHPjjUj9X+T4K4PNW/1O9PulZYUR/iKr8JQcF+bca0tId6AMO6FJ6XbT Nz7LKMBvgTYQbQfYUCaXMWglPvqgDQB8 X-Google-Smtp-Source: AOwi7QAa9v5C5WujiLk/cmYHDY5yltj3KQNfuWdwZCU4YhDP/V2ux6dOseUJtMa9fcymr9mb8hOYLdfAkVMCtqqM3JQ= X-Received: by 10.107.154.71 with SMTP id c68mr22734362ioe.95.1505231352743; Tue, 12 Sep 2017 08:49:12 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.152.18 with HTTP; Tue, 12 Sep 2017 08:49:12 -0700 (PDT) In-Reply-To: <5f8f5a1b-a170-c20b-91e0-6e813faf0527@redhat.com> References: <20170911050121.85708-1-ruiyu.ni@intel.com> <7fbd6e6b-0577-f470-3e89-f785ddd5dee1@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5BA2BABB@SHSMSX104.ccr.corp.intel.com> <5f8f5a1b-a170-c20b-91e0-6e813faf0527@redhat.com> From: Ard Biesheuvel Date: Tue, 12 Sep 2017 08:49:12 -0700 Message-ID: To: Laszlo Ersek Cc: "Ni, Ruiyu" , "edk2-devel@lists.01.org" , Benjamin Herrenschmidt , Dong Wei , Andrew Fish 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 15:46:16 -0000 Content-Type: text/plain; charset="UTF-8" On 12 September 2017 at 01:40, Laszlo Ersek wrote: > 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 [...]". > Yes. > (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. > Right. Now *I* am even more confused. > Thus, to me these statements appear to conflict. > Yes they do, apologies. >> 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); > Ugh. I think you're right. But now, I am no longer convinced AddrRangeMin should contain the host address, given that we are inverting the sense of both the AddrRangeMin field and the translation offset. So IIUC, if we were to decide that AddrRangeMin contains the raw BAR value, and the translation offset that needs to be applied to produce the CPU address is added to it, we are quite close to the intent of the definition of QWord, and our PCI I/O code is correct. Only in this case, we need to fix all users of the protocol (i.e., GOP producers) Given the low likelihood that this ever worked correctly for cases where the translation offset != 0, I think that is perhaps the best course of action. Apologies for adding to the confusion.