[PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ruiyu Ni]

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."
HostAddress = DeviceAddress + AddressTranslationOffset.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>
Cc: Andrew Fish <afish@apple.com>
Cc: Dong Wei <Dong.Wei@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Bartosz Szczepanek <bsz@semihalf.com>
---
 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;
     }
   }
 
-- 
2.12.2.windows.2

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Laszlo Ersek]

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 <ruiyu.ni@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Dong Wei <Dong.Wei@arm.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Bartosz Szczepanek <bsz@semihalf.com>
> ---
>  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?

I mean if a PCI driver author reads the UEFI 2.7 spec, the spec seems to
give that impression.

(I'm sorry if I should have raised these questions last week -- I don't
wish to block this patch. Please go ahead if I'm wrong.)

Thanks
Laszlo

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ard Biesheuvel]

On 11 September 2017 at 07:46, Laszlo Ersek <lersek@redhat.com> 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 <ruiyu.ni@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Dong Wei <Dong.Wei@arm.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Bartosz Szczepanek <bsz@semihalf.com>
>> ---
>>  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.

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ni, Ruiyu]

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".

Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Monday, September 11, 2017 2:47 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Dong Wei <Dong.Wei@arm.com>; Benjamin Herrenschmidt
> <benh@au1.ibm.com>; Andrew Fish <afish@apple.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes()
> returns Host address
> 
> 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 <ruiyu.ni@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Benjamin Herrenschmidt <benh@au1.ibm.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: Dong Wei <Dong.Wei@arm.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Bartosz Szczepanek <bsz@semihalf.com>
> > ---
> >  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?
> 
> I mean if a PCI driver author reads the UEFI 2.7 spec, the spec seems to give
> that impression.
> 
> (I'm sorry if I should have raised these questions last week -- I don't wish to
> block this patch. Please go ahead if I'm wrong.)
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ard Biesheuvel]

On 12 September 2017 at 06:01, Ni, Ruiyu <ruiyu.ni@intel.com> 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. 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.

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Laszlo Ersek]

On 09/12/17 08:44, Ard Biesheuvel wrote:
> On 12 September 2017 at 06:01, Ni, Ruiyu <ruiyu.ni@intel.com> 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

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ard Biesheuvel]

On 12 September 2017 at 01:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/12/17 08:44, Ard Biesheuvel wrote:
>> On 12 September 2017 at 06:01, Ni, Ruiyu <ruiyu.ni@intel.com> 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.

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Laszlo Ersek]

On 09/12/17 17:49, Ard Biesheuvel wrote:
> On 12 September 2017 at 01:40, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/12/17 08:44, Ard Biesheuvel wrote:
>>> On 12 September 2017 at 06:01, Ni, Ruiyu <ruiyu.ni@intel.com> 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)

I'd be totally OK with that...

> 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.

...as long as the USWG agreed to invert the sense of the fields in the
UEFI spec, based on which the GOPs should be updated.

In practice this would mean reverting
<https://mantis.uefi.org/mantis/view.php?id=1502>. By now the fix for
Mantis#1502 has been in three released versions of the spec (one of the
2.5 Errata, 2.6 and 2.7).

I'm fine both ways, as long as code and spec are consistent. From a
development perspective though, I think the spec is harder to change
than the code, no matter how ugly the code ends up.

Thanks!
Laszlo

Re: [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes() returns Host address

[Ni, Ruiyu]

I am going to hold the patch.
Unless some real issue is exposed using current PciBus driver, I will not make any change to current implementation.


Thanks/Ray

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@au1.ibm.com]
> Sent: Wednesday, September 13, 2017 7:38 AM
> To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong Wei
> <Dong.Wei@arm.com>; Andrew Fish <afish@apple.com>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: GetBarAttributes()
> returns Host address
> 
> On Tue, 2017-09-12 at 18:15 +0200, Laszlo Ersek wrote:
> > > 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)
> >
> > I'd be totally OK with that...
> 
> Which happens to be the approach I took in my original port looking back at
> my code.
> 
> > > 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.
> >
> > ...as long as the USWG agreed to invert the sense of the fields in the
> > UEFI spec, based on which the GOPs should be updated.
> 
> I think we had that conversation back then ;-) I remember a number of
> inconsistencies in some of the resource fields including existing cases of UEFI
> not quite following the ACPI spec.
> 
> However:
> 
> > In practice this would mean reverting
> > <https://urldefense.proofpoint.com/v2/url?u=https-3A__mantis.uefi.org_
> > mantis_view.php-3Fid-3D1502&d=DwICaQ&c=jf_iaSHvJObTbx-
> siA1ZOg&r=ilASlO
> >
> u_ee2uzSGEkp3JMw&m=Y8F47uHCO5ZLYtvPhj45KfJDaIOCvYcJVK7HG2h7PC
> M&s=klaAU
> > FJYXQhUuF3Ad3BNs3HfM_8TpbgsENeLHRlI-vc&e== >. By now the fix for
> > Mantis#1502 has been in three released versions of the spec (one of
> > the
> > 2.5 Errata, 2.6 and 2.7).
> >
> > I'm fine both ways, as long as code and spec are consistent. From a
> > development perspective though, I think the spec is harder to change
> > than the code, no matter how ugly the code ends up.
> 
> That's a bit annoying. We need to ensure nobody already implemented
> something based on the above and rleased it...
> 
> Note there's another problem in EDK implementation. The
> AddressTranslationOffset field is hijacked in GetProposedResources return
> code iirc (at least back then, this might have changed since).
> 
> I had this patch to the header (and corresponding implementation
> changes) in my tree in
> MdePkg/Include/Protocol/PciHostBridgeResourceAllocation.h:
> 
>  ///
> -/// A UINT64 value that contains the status of a PCI resource requested -///
> in the Configuration parameter returned by GetProposedResources() -///
> The legal values are EFI_RESOURCE_SATISFIED and
> EFI_RESOURCE_NOT_SATISFIED
> +/// If this bit is set, then the PCI Root Bridge doesn't support /// IO
> +space
>  ///
> -typedef UINT64 EFI_RESOURCE_ALLOCATION_STATUS;
> +#define EFI_PCI_HOST_BRIDGE_NO_IO_DECODE   4
> 
>  ///
> -/// The request of this resource type could be fulfilled.  Used in the -///
> Configuration parameter returned by GetProposedResources() to identify -
> /// a PCI resources request that can be satisfied.
> +/// The "Address Translation Offset" field of the ACPI descriptor ///
> +returned by GetProposedResources() can contain either of these ///
> +ranges of *unsigned* values:
> +///
> +///  EFI_RESOURCE_NONEXISTENT : The resource request cannot be
> +satisfied ///
> +///  EFI_RESOURCE_LESS        : The resource wasn't satisified but a small
> +///                             request could be
>  ///
> -#define EFI_RESOURCE_SATISFIED      0x0000000000000000ULL
> +///  Value < EFI_RESOURCE_OFFSET_LIMIT
> +///                           : An offset representing the translation from
> +///                             PCI addresses to CPU addresses
> +//
> +#define EFI_RESOURCE_NONEXISTENT  0xFFFFFFFFFFFFFFFFULL
> +#define EFI_RESOURCE_LESS         0xFFFFFFFFFFFFFFFEULL
> +#define EFI_RESOURCE_OFFSET_LIMIT 0xFFFFFFFFFFFFFFF0ULL
> +
> +// Helper for internal use of PciBusDxe
> +#define EFI_RESOURCE_SATISFIED    0
> 
>  ///
> -/// The request of this resource type could not be fulfilled for its -///
> absence in the host bridge resource pool.  Used in the Configuration
> parameter -/// returned by GetProposedResources() to identify a PCI
> resources request that -/// can not be satisfied.
> +/// The request of this resource type could be fulfilled.  Used in the
> +/// Configuration parameter returned by GetProposedResources() to
> +identify /// a PCI resources request that can be satisfied.
>  ///
> -#define EFI_RESOURCE_NOT_SATISFIED  0xFFFFFFFFFFFFFFFFULL
> +#define EFI_RESOURCE_IS_SATISFIED(Offset) \
> +  (((UINT64)Offset) < EFI_RESOURCE_OFFSET_LIMIT)
> 
> Along with making PciEnumerator.c use  EFI_RESOURCE_IS_SATISFIED()
> rather than comparing against EFI_RESOURCE_NOT_SATISFIED
> 
> Cheers,
> Ben.