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

[Marcel Apfelbaum <marcel@redhat.com>]

On 20/09/2017 14:01, Laszlo Ersek wrote:
> On 09/20/17 09:42, Marcel Apfelbaum wrote:
>> On 20/09/2017 1:15, Laszlo Ersek wrote:
> 
>>> It seems that we now have "grp->io_reserve" (a numeric quantity, not a
>>> boolean), from property "io-reserve". The default value is -1.
>>>
>>> According to the documentation added in c1800a162765 ("docs: update
>>> documentation considering PCIE-PCI bridge", 2017-08-18), the value -1
>>> seems to imply, "If any reservation field is -1 then this kind of
>>> reservation is not needed and must be ignored by firmware".
>>>
>>
>> This was decided after long long upstream discussions with
>> different maintainers.
> 
> I know :) I participated in the review, and the documentation patch
> noted above carries my R-b.
> 
> I didn't mean that the -1 default was wrong, only that we might have to
> polish the definition / semantics a bit.
> 
>>> I think we'll need to refine the definition. Once OVMF starts processing
>>> this capability, the behavior should be compatible with earlier
>>> behavior. Assume that a user sets only "mem-reserve" to something
>>> different from -1, and thus the capability appears. When OVMF sees the
>>> capability, it should be able to tell apart:
>>>
>>> - no particular hint about IO space, so continue doing whatever has been
>>> done all this time (default IO space reservation),
>>> - do not request IO space reservation at all, > - a given (positive)
>>> size of IO space should be reserved.
>>>
>>> So I think:
>>>
>>> (a) the above read-only mask setting should be done based on
>>>
>>>     (grp->io_reserve == 0)
>>>
>>> and the "enable-io-fwd" property should be unnecessary,
>>>
>>
>> I really want this to be the case,  but I need to check the
>> implementation first. The only concern is the semantics.
>>
>> (grp->io_reserve == 0) hints the firmware to take
>> max(hint, <default-value>), since we don't want to reserve less
>> than the IO range needed by existing devices behind the bridge.
> 
> I agree.
> 
> In OVMF, I would translate (io_reserve == 0) into *not* returning any
> particular IO reservation hint to the generic PciBusDxe driver, for the
> PCI-E port in question. In turn PciBusDxe follows the above "max"
> semantics, as far as I remember; in other words, setting io_reserve to
> zero wouldn't break existing IO requirements.
> 
> Here's a table:
> 
> - io_reserve == (-1) --> OVMF returns an IO reservation of 512 bytes
>    (which in practice gets rounded up to 4KB). If more IO is needed by
>    devices behind the bridge, then that larger value is used. This is the
>    current behavior and we should keep it.
> 
> - io_reserve == 0 --> OVMF returns no IO reservation at all. If any IO
>    is needed by devices behind the bridge, then that value is used.
>    Otherwise, no IO is allocated.
> 
> - io_reserve > 0 --> OVMF returns this value to PciBusDxe. PciBusDxe
>    might round it up. If more IO is needed by devices behind the bridge,
>    then that larger value is used.
> 
>> 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?

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.


>>
>>> (b) the "io-reserve" property should be set to 0 for 2.11 machine types
>>> and onward, and to -1 for 2.10 and earlier (for compatibility),
>>>
>>
>> Michael asked us to wait a little before changing the default,
>> you can ask him for more info :)
> 
> Sure, I'm totally fine with delaying the change to the default value.
> 
>>
>>> (c) the documentation in "docs/pcie_pci_bridge.txt" should be updated to
>>> say:
>>> * (-1) --> default firmware behavior (unspecified)
>>> *   0  --> do not reserve
>>> *  >0  --> specific reservation requested
>>>
>>
>> Agreed, once I re-check SeaBIOS to confirm behavior.
> 
> BTW, if the OVMF -- more precisely, PciBusDxe -- and SeaBIOS behaviors
> turn out to differ significantly in the handling of the values (although
> I don't expect it, see above -- last time I looked, the interpretations
> were similar), I think it's a possibility (although not optimal) to say
> that the interpretation of these values is firmware specific.
> 
> We won't know for certain until we write the code and test both firmwares.
> 
>>
>>> (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)

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

Thanks,
Marcel

>> Getting back to this patch, setting io-reserve to 0
>> would require also:
>>
>>     +      pci_word_test_and_clear_mask(pci_dev->wmask + PCI_COMMAND,
>>     +                                     PCI_COMMAND_IO);
>>     +      pci_dev->wmask[PCI_IO_BASE] = 0;
>>     +      pci_dev->wmask[PCI_IO_LIMIT] = 0;
>>
>> otherwise the Guest OS would not behave, just be aware of it.
> 
> Absolutely, no doubt about this.
> 
> Thanks,
> Laszlo
> 
> 
>>>>    static const VMStateDescription vmstate_rp_dev = {
>>>>        .name = "pcie-root-port",
>>>>        .version_id = 1,
>>>> @@ -78,6 +111,7 @@ static const VMStateDescription vmstate_rp_dev = {
>>>>      static Property gen_rp_props[] = {
>>>>        DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort,
>>>> migrate_msix, true),
>>>> +    DEFINE_PROP_BOOL("enable-io-fwd", GenPCIERootPort,
>>>> enable_io_fwd, false),
>>>>        DEFINE_PROP_END_OF_LIST()
>>>>    };
>>>>    @@ -86,6 +120,7 @@ static void gen_rp_dev_class_init(ObjectClass
>>>> *klass, void *data)
>>>>        DeviceClass *dc = DEVICE_CLASS(klass);
>>>>        PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>>>>        PCIERootPortClass *rpc = PCIE_ROOT_PORT_CLASS(klass);
>>>> +    GenPCIERootPortClass *grpc = GEN_PCIE_ROOT_PORT_CLASS(klass);
>>>>          k->vendor_id = PCI_VENDOR_ID_REDHAT;
>>>>        k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_RP;
>>>> @@ -96,6 +131,8 @@ static void gen_rp_dev_class_init(ObjectClass
>>>> *klass, void *data)
>>>>        rpc->interrupts_init = gen_rp_interrupts_init;
>>>>        rpc->interrupts_uninit = gen_rp_interrupts_uninit;
>>>>        rpc->aer_offset = GEN_PCIE_ROOT_PORT_AER_OFFSET;
>>>> +    grpc->parent_realize = dc->realize;
>>>> +    dc->realize = gen_rp_realize;
>>>>    }
>>>>      static const TypeInfo gen_rp_dev_info = {
>>>> @@ -103,6 +140,7 @@ static const TypeInfo gen_rp_dev_info = {
>>>>        .parent        = TYPE_PCIE_ROOT_PORT,
>>>>        .instance_size = sizeof(GenPCIERootPort),
>>>>        .class_init    = gen_rp_dev_class_init,
>>>> +    .class_size    = sizeof(GenPCIERootPortClass),
>>>>    };
>>>>       static void gen_rp_register_types(void)
>>>> diff --git a/include/hw/compat.h b/include/hw/compat.h
>>>> index 3e101f8f67..843bf4a3a5 100644
>>>> --- a/include/hw/compat.h
>>>> +++ b/include/hw/compat.h
>>>> @@ -2,7 +2,11 @@
>>>>    #define HW_COMPAT_H
>>>>      #define HW_COMPAT_2_10 \
>>>> -    /* empty */
>>>> +    {\
>>>> +        .driver   = "pcie-root-port",\
>>>> +        .property = "enable-io-fwd",\
>>>> +        .value    = "true",\
>>>> +    },
>>>>      #define HW_COMPAT_2_9 \
>>>>        {\
>>>>
>>>
>>
>