qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH] hw/pci: add pci capability to pci-pci bridge
Date: Tue, 7 Aug 2018 14:19:41 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 08/07/18 09:04, Jing Liu wrote:
> Add hint to firmware (e.g. SeaBIOS) to reserve addtional
> IO/MEM/PREF spaces for legacy pci-pci bridge, to enable
> some pci devices hotplugging whose IO/MEM/PREF spaces
> requests are larger than the ones in pci-pci bridge set
> by firmware.
> 
> Signed-off-by: Jing Liu <address@hidden>
> ---
>  hw/pci-bridge/pci_bridge_dev.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index b2d861d..8e9afbd 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -46,6 +46,12 @@ struct PCIBridgeDev {
>      uint32_t flags;
>  
>      OnOffAuto msi;
> +
> +    /* additional resources to reserve on firmware init */
> +    uint64_t io_reserve;
> +    uint64_t mem_reserve;
> +    uint64_t pref32_reserve;
> +    uint64_t pref64_reserve;
>  };
>  typedef struct PCIBridgeDev PCIBridgeDev;

(1) Please evaluate the following idea:

- Factor the "bus_reserve", "io_reserve", "mem_reserve",
"pref32_reserve" and "pref64_reserve" fiels of the "GenPCIERootPort"
structure out to another structure.

- I think this new structure type should be defined in
"include/hw/pci/pci_bridge.h", just before the declaration of the
pci_bridge_qemu_reserve_cap_init() function.

- Note that the PCIBridgeQemuCap structure should *not* be modified
(i.e. it should not be rebased to the new sub-structure) -- the fields
are not identical!

- The GenPCIERootPort structure should embed this new sub-structure; the
field name could be "res_reserve".

- The parameter list of pci_bridge_qemu_reserve_cap_init() should be
updated to take a pointer to a constant sub-structure.

- gen_rp_realize() can simply pass &grp->res_reserve.

- gen_rp_props should be updated accordingly. The property names should
stay the same, but they should reference fields of the "res_reserve" field.

(2) Then, in a separate patch, you can add another "res_reserve" field
to "PCIBridgeDev". (As a consequence, "bus_reserve" will also be added,
which I think is the right thing to do.)

(3) There is a class that is derived from TYPE_PCI_BRIDGE_DEV:
TYPE_PCI_BRIDGE_SEAT_DEV. Is it correct to add the new properties /
fields to the latter as well?

>  
> @@ -95,6 +101,13 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error 
> **errp)
>          error_free(local_err);
>      }
>  
> +    err = pci_bridge_qemu_reserve_cap_init(dev, 0, 0,
> +          bridge_dev->io_reserve, bridge_dev->mem_reserve,
> +          bridge_dev->pref32_reserve, bridge_dev->pref64_reserve, errp);
> +    if (err) {
> +        goto cap_error;
> +    }
> +
>      if (shpc_present(dev)) {
>          /* TODO: spec recommends using 64 bit prefetcheable BAR.
>           * Check whether that works well. */
> @@ -103,6 +116,8 @@ static void pci_bridge_dev_realize(PCIDevice *dev, Error 
> **errp)
>      }
>      return;
>  
> +cap_error:
> +    msi_uninit(dev);

(4) This error handler doesn't look entirely correct; we can reach it
without having initialized MSI. (MSI setup is conditional; and even if
we attempt it, it is permitted to fail with "msi=auto".) Therefore,
msi_uninit() should be wrapped into "if (msi_present())", same as you
see in pci_bridge_dev_exitfn().

>  msi_error:
>      slotid_cap_cleanup(dev);
>  slotid_error:
> @@ -162,6 +177,11 @@ static Property pci_bridge_dev_properties[] = {
>                              ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags,
>                      PCI_BRIDGE_DEV_F_SHPC_REQ, true),
> +    DEFINE_PROP_SIZE("io-reserve", PCIBridgeDev, io_reserve, -1),
> +    DEFINE_PROP_SIZE("mem-reserve", PCIBridgeDev, mem_reserve, -1),
> +    DEFINE_PROP_SIZE("pref32-reserve", PCIBridgeDev, pref32_reserve, -1),
> +    DEFINE_PROP_SIZE("pref64-reserve", PCIBridgeDev, pref64_reserve, -1),
> +
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> 

(5) Similarly to (2), this property list should cover "bus-reserve" too.
If we are exposing the same capability in PCI config space, then I think
we should let the user control all fields of it as well.

(6) Please clarify the capability ID in the subject line of the patch.
For example:

  hw/pci: support resource reservation capability on "pci-bridge"

Thanks
Laszlo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]