Re: [Qemu-devel] [PATCH 2/2] hw/pcie: disable IO port fwd by default for pcie-root-port

[Laszlo Ersek]

On 09/20/17 13:16, Marcel Apfelbaum wrote:
> On 20/09/2017 14:01, Laszlo Ersek wrote:
>> On 09/20/17 09:42, Marcel Apfelbaum wrote:

>>> The implementation issue might be that Guest Firmware would
>>> take the alignment as default value for an empty root port.
>>> (maybe take it as a bug and "solve" it?)
>>
>> Hm, I don't get this. What do you mean?
>>
> 
> Empty PCI Express Root Port with io_reserve = 0 should end with
> no IO ports range, right?

Yes.

> But there is some code in SeaBIOS that "aligns" the IO range to 4K,
> so we may end up with a 4K range even if io_reserve=0.

Ah.

OK, if this makes a difference, please report your findings :)

>>>> (d) pci_bridge_qemu_reserve_cap_init() should be updated accordingly
>>>> (i.e., a conflict exists if both mem_pref_32_reserve and
>>>> mem_pref_64_reserve are *positive*),
>>>>
>>>
>>> I thought we have this check already.
>>
>> We have a check like this, but it doesn't use the exact condition that
>> I'm suggesting above (emphasis on *positive*). We now have
>>
>>      if (mem_pref_32_reserve != (uint32_t)-1 &&
>>          mem_pref_64_reserve != (uint64_t)-1) {
>>          error_setg(errp,
>>
>> but after introducing the zero value, this is going to be too strict.
>> Consider all the possible combinations:
>>
>> * (-1; -1): default fw behavior, check is OK
>>
>> * (-1;  0): fw sticks with the default for the first component, picks
>>              "no reservation" for the second component, check is OK
>>
>> * (-1; >0): fw can treat this identically to ( 0; >0) -- see below. This
>>              is justified because, while the first component says "use
>>              the default", the entire situation of having such a
>>              capability is new, so the firmware can introduce new ways
>>              for handling it. The check passes, which is good.
>>
>> * ( 0;  0): check reports error, but the firmware can handle this case
>>              fine (no reservation for either component)
>>
>> * ( 0; >0): check reports error, but the fw can handle this (no
>>              reservation for the first component, specific reservation
>>              for the second)
>>
>> * (>0; >0): conflict, check is OK
>>
>> (I'm skipping the description of the inverted orderings, such as (0;-1),
>> they behave similarly.)
>>
>> So we need to accept 0 as "valid" for either component:
>>
>>      if (mem_pref_32_reserve > 0 &&
>>          mem_pref_32_reserve < (uint32_t)-1 &&
>>          mem_pref_64_reserve > 0 &&
>>          mem_pref_64_reserve < (uint64_t)-1) {
>>          error_setg(errp,
>>
> 
> Got it, you are right. How did we end up with something
> complicated again :( ?
> (complicated = needs documentation and not straight forward)

Haha, are you serious? :)

*Nothing* is simple in QEMU; nothing at all. :) In general, software is
complex because of feature requests, and QEMU caters to a million
different (and conflicting) use cases, so you get complexity.

(I know this is trivial, but it sure felt good to write down! :) )

>>>> Second, when determining the reservations in OVMF, I would like to look
>>>> only at the capability fields, and not do a read-write-read-write
>>>> quadruplet to the IO base/limit registers. Do you agree?
>>>>
>>>
>>> Well, as stated before, the semantics for the hint is
>>> max(hint, <sum of all IO/MEM ranges for devices behind the bridge>).
>>> If you can follow it, that would be OK.
>>
>> In OVMF there are two separate questions:
>> - how to report the requested reservations,
>> - how to act upon the reported values.
>>
>> The first question pertains to "platform code", i.e., what I'm going to
>> implement under OvmfPkg. The second question pertains to "universal
>> code" (under "MdeModulePkg/Bus/Pci/PciBusDxe"), which is a lot harder to
>> modify, because it is shipped on a wide range of physical platforms too.
>>
>> Again, my understanding is that PciBusDxe implements the "max" semantics
>> that you describe (pertaining to the 2nd question).
>>
>> My interest is in figuring out the 1st question now, that is, where I
>> should take the information from that goes into the "reservation
>> description" that PciBusDxe understands. My preference would be to look
>> only at the PCIE port in question (i.e. no other PCI devices at all),
>> and only at said capability in the config space of the PCIE port.
>>
> 
> Sounds OK.

Thank you! Looks like I can start writing code for OVMF when I find the
time.

Thanks!
Laszlo