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

[Marcel Apfelbaum <marcel@redhat.com>]

Hi Laszlo,

On 20/09/2017 1:15, Laszlo Ersek wrote:
> Hi Marcel,
> 
> On 09/06/17 16:26, Marcel Apfelbaum wrote:
>> For most cases the devices attached to PCIe Root Ports
>> do not need IO ports range, add an 'enable-io-fwd' property
>> making it false by default, but keeping it true for older machines.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/pci-bridge/gen_pcie_root_port.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   include/hw/compat.h                |  6 +++++-
>>   2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
>> index cb694d6da5..cbdeb73e2c 100644
>> --- a/hw/pci-bridge/gen_pcie_root_port.c
>> +++ b/hw/pci-bridge/gen_pcie_root_port.c
>> @@ -20,12 +20,26 @@
>>   #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
>>   #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
>>   
>> +#define GEN_PCIE_ROOT_PORT(obj) \
>> +     OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
>> +#define GEN_PCIE_ROOT_PORT_CLASS(klass) \
>> +     OBJECT_CLASS_CHECK(GenPCIERootPortClass, (klass), TYPE_GEN_PCIE_ROOT_PORT)
>> +#define GEN_PCIE_ROOT_PORT_GET_CLASS(obj) \
>> +     OBJECT_GET_CLASS(GenPCIERootPortClass, (obj), TYPE_GEN_PCIE_ROOT_PORT)
>> +
>> +typedef struct GenPCIERootPortClass {
>> +    PCIERootPortClass parent_class;
>> +
>> +    DeviceRealize parent_realize;
>> +} GenPCIERootPortClass;
>> +
>>   typedef struct GenPCIERootPort {
>>       /*< private >*/
>>       PCIESlot parent_obj;
>>       /*< public >*/
>>   
>>       bool migrate_msix;
>> +    bool enable_io_fwd;
>>   } GenPCIERootPort;
>>   
>>   static uint8_t gen_rp_aer_vector(const PCIDevice *d)
>> @@ -60,6 +74,25 @@ static bool gen_rp_test_migrate_msix(void *opaque, int version_id)
>>       return rp->migrate_msix;
>>   }
>>   
>> +static void gen_rp_realize(DeviceState *d, Error **errp)
>> +{
>> +    GenPCIERootPortClass *grpc = GEN_PCIE_ROOT_PORT_GET_CLASS(d);
>> +    GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
>> +    PCIDevice *pci_dev = PCI_DEVICE(d);
>> +
>> +    grpc->parent_realize(DEVICE(d), errp);
>> +    if (*errp) {
>> +        return;
>> +    }
>> +
>> +    if (!grp->enable_io_fwd) {
>> +        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;
>> +    }
>> +}
>> +
> 

Thanks for the review!

> This function exists now, but with different implementation, from
> Aleksandr's commit 226263fb5cda ("hw/pci: add QEMU-specific PCI
> capability to the Generic PCI Express Root Port", 2017-08-18).
> 

Right, the patches touch the same areas.

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

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?)

> (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 :)

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

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

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


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.


Thanks,
Marcel

> 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 \
>>       {\
>>
>