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

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

[Marcel Apfelbaum]

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. Keep the property true for older
machines.

Add QEMU 2.11 machine if nobody did it yet.

Note that the series may collide with Alexandr's pcie-pci-bridge
series.

Marcel Apfelbaum (2):
  pc: add 2.11 machine types
  hw/pcie: disable IO port fwd by default for pcie-root-port

 hw/i386/pc_piix.c                  | 15 ++++++++++++---
 hw/i386/pc_q35.c                   | 13 +++++++++++--
 hw/pci-bridge/gen_pcie_root_port.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/hw/compat.h                |  6 +++++-
 include/hw/i386/pc.h               |  3 +++
 5 files changed, 69 insertions(+), 6 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/2] pc: add 2.11 machine types

[Marcel Apfelbaum]

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 include/hw/i386/pc.h |  3 +++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 46dfd2c954..b03cc047c3 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -436,21 +436,30 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->default_display = "std";
 }
 
-static void pc_i440fx_2_10_machine_options(MachineClass *m)
+static void pc_i440fx_2_11_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v2_11, "pc-i440fx-2.11", NULL,
+                      pc_i440fx_2_11_machine_options);
+
+static void pc_i440fx_2_10_machine_options(MachineClass *m)
+{
+    pc_i440fx_2_11_machine_options(m);
+    m->is_default = 0;
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
+}
+
 DEFINE_I440FX_MACHINE(v2_10, "pc-i440fx-2.10", NULL,
                       pc_i440fx_2_10_machine_options);
 
 static void pc_i440fx_2_9_machine_options(MachineClass *m)
 {
     pc_i440fx_2_10_machine_options(m);
-    m->is_default = 0;
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
     m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
 }
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 169a214d50..c1cba584d1 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -302,10 +302,20 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_2_10_machine_options(MachineClass *m)
+static void pc_q35_2_11_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
+}
+
+DEFINE_Q35_MACHINE(v2_11, "pc-q35-2.11", NULL,
+                   pc_q35_2_11_machine_options);
+
+static void pc_q35_2_10_machine_options(MachineClass *m)
+{
+    pc_q35_2_11_machine_options(m);
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_10);
     m->numa_auto_assign_ram = numa_legacy_auto_assign_ram;
 }
 
@@ -315,7 +325,6 @@ DEFINE_Q35_MACHINE(v2_10, "pc-q35-2.10", NULL,
 static void pc_q35_2_9_machine_options(MachineClass *m)
 {
     pc_q35_2_10_machine_options(m);
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_9);
 }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index d80859bfad..8226904524 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -369,6 +369,9 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
 int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
+#define PC_COMPAT_2_10 \
+    HW_COMPAT_2_10 \
+
 #define PC_COMPAT_2_9 \
     HW_COMPAT_2_9 \
     {\
-- 
2.13.5

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

[Marcel Apfelbaum]

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;
+    }
+}
+
 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 \
     {\
-- 
2.13.5

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

[Eduardo Habkost]

On Wed, Sep 06, 2017 at 05:26:58PM +0300, 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>
[...]
> @@ -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),

There's no "x-" prefix, so I guess we really need to let the user
or management software to set enable-io-fwd=on explicitly on some
cases?

-- 
Eduardo

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

[Marcel Apfelbaum]

On 06/09/2017 17:49, Eduardo Habkost wrote:
> On Wed, Sep 06, 2017 at 05:26:58PM +0300, 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>
> [...]
>> @@ -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),
> 

Hi Eduardo,

> There's no "x-" prefix, so I guess we really need to let the user
> or management software to set enable-io-fwd=on explicitly on some
> cases?
> 

Definitely. Specifically 2 cases:
  - attach PCIe-PCI bridge to start a legacy PCI hierarchy.
  - assign a device requiring IO ports range.

Thanks,
Marcel

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

[Laszlo Ersek]

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;
> +    }
> +}
> +

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

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

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,

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

(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

(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*),


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?

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

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

[Marcel Apfelbaum]

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

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

[Laszlo Ersek]

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?

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

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

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

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

[Marcel Apfelbaum]

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

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

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

[Marcel Apfelbaum]

Hi Laszlo,

On 20/09/2017 14:01, Laszlo Ersek wrote:
>   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,
> 

SeaBIOS  will not allow (0,0) values for pref32 and pref64 fields.
More than that, 0 values are not being taken in consideration.

We may say is a bug and fix it. But taking a step back, we need the
hints to reserve *more* mem range, I am not sure 0 values are 
interesting for mem reservation. Also is also only a hint,
the firmware should decide.

Do we have a concrete scenario where would we want to use (0, non-zero)
or (non-zero, 0) ? (ask for 32/64 prefetch)

Our current semantic is: "we give a hint to the Guest Firmware,
we don't decide. However, giving hints to both pref32 and pref64
is wrong".


Not related: Using 0 value to disable IO works just fine!
No need for disable-IO-fwd property :)

Thanks,
Marcel

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

[Laszlo Ersek]

On 09/27/17 12:06, Marcel Apfelbaum wrote:
> Hi Laszlo,
> 
> On 20/09/2017 14:01, Laszlo Ersek wrote:
>>   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,
>>
> 
> SeaBIOS  will not allow (0,0) values for pref32 and pref64 fields.
> More than that, 0 values are not being taken in consideration.
> 
> We may say is a bug and fix it. But taking a step back, we need the
> hints to reserve *more* mem range, I am not sure 0 values are
> interesting for mem reservation. Also is also only a hint,
> the firmware should decide.
> 
> Do we have a concrete scenario where would we want to use (0, non-zero)
> or (non-zero, 0) ? (ask for 32/64 prefetch)
> 
> Our current semantic is: "we give a hint to the Guest Firmware,
> we don't decide. However, giving hints to both pref32 and pref64
> is wrong".

After having written the OVMF code for this, my understanding is
clearer, and I agree that, for *prefetchable*, distinguishing -1 from 0
is not necessary (-1 means "firmware default (unspecified)", and 0 means
"do not reserve"; and for prefetchable, the two things mean the same in
OVMF). So the current QEMU code should be fine.

> Not related: Using 0 value to disable IO works just fine!
> No need for disable-IO-fwd property :)

Great!

Thanks!
Laszlo