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 <mar...@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 \
> {\
>
