Re: [SeaBIOS] [PATCH v3 4/5] hw/pci: add QEMU-specific PCI capability to Generic PCI Express Root Port

[Alexander Bezzubikov]

2017-07-31 14:43 GMT+03:00 Marcel Apfelbaum :
> On 29/07/2017 2:37, Aleksandr Bezzubikov wrote:
> > From: Aleksandr Bezzubikov
> >
> > To enable hotplugging of a newly created pcie-pci-bridge,
> > we need to tell firmware (SeaBIOS in this case)
>
> Not only SeaBIOS, also OVMF - so all guest firmware

But it can mislead some users - it will look like
we've already implemented this for SeaBIOS and OVMF both,
but we haven't.
> to reserve
> > additional buses for pcie-root-port, that allows us to
> > hotplug pcie-pci-bridge into this root port.
> > The number of buses to reserve is provided to the device via a
> > corresponding
> > property, and to the firmware via new PCI capability.
> > The property's default value is 0 to keep default behavior unchanged.
> > Signed-off-by: Aleksandr Bezzubikov
> >
> > hw/pci-bridge/gen_pcie_root_port.c | 23 +++++++++++++++++++++++
> >   hw/pci-bridge/pcie_root_port.c     |  2 +-
> >   include/hw/pci/pcie_port.h         |  2 ++
> >   3 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c
> > b/hw/pci-bridge/gen_pcie_root_port.c
> > index cb694d6..da3caa1 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -16,6 +16,8 @@
> >   #include "hw/pci/pcie_port.h"
> >     #define TYPE_GEN_PCIE_ROOT_PORT                "pcie-root-port"
> > +#define GEN_PCIE_ROOT_PORT(obj) \
> >    OBJECT_CHECK(GenPCIERootPort, (obj), TYPE_GEN_PCIE_ROOT_PORT)
> > #define GEN_PCIE_ROOT_PORT_AER_OFFSET           0x100
> > #define GEN_PCIE_ROOT_PORT_MSIX_NR_VECTOR       1
> >
> > @@ -26,6 +28,9 @@ typedef struct GenPCIERootPort {
> >       /*< public >*/
> >         bool migrate_msix;
> > /* additional buses to reserve on firmware init */
> > uint8_t bus_reserve;
> > } GenPCIERootPort;
> > static uint8_t gen_rp_aer_vector(const PCIDevice *d)
> >
> > @@ -60,6 +65,21 @@ static bool gen_rp_test_migrate_msix(void *opaque, int
> > version_id)
> >       return rp->migrate_msix;
> >   }
> >   +static void gen_rp_realize(PCIDevice *d, Error **errp)
> > +{
> > rp_realize(d, errp);
> > PCIESlot *s = PCIE_SLOT(d);
> > GenPCIERootPort *grp = GEN_PCIE_ROOT_PORT(d);
> > int rc = pci_bridge_qemu_cap_init(d, 0, grp->bus_reserve, 0, 0, 0,
> >
> > errp);
> > if (rc < 0) {
> >    pcie_chassis_del_slot(s);
> >    pcie_cap_exit(d);
> >    gen_rp_interrupts_uninit(d);
> >    pci_bridge_exitfn(d);
> > }
> >
> > +}
> > static const VMStateDescription vmstate_rp_dev = {
> >     .name = "pcie-root-port",
> >     .version_id = 1,
> >
> > @@ -78,6 +98,7 @@ static const VMStateDescription vmstate_rp_dev = {
> >     static Property gen_rp_props[] = {
> >       DEFINE_PROP_BOOL("x-migrate-msix", GenPCIERootPort, migrate_msix,
> > true),
> > DEFINE_PROP_UINT8("bus-reserve", GenPCIERootPort, bus_reserve, 0),
> >  DEFINE_PROP_END_OF_LIST()
> > };
> > @@ -89,6 +110,8 @@ static void gen_rp_dev_class_init(ObjectClass *klass,
> >
> > void *data)
> >         k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >       k->device_id = PCI_DEVICE_ID_REDHAT_PCIE_RP;
> > k->realize = gen_rp_realize;
> > dc->desc = "PCI Express Root Port";
> > dc->vmsd = &vmstate_rp_dev;
> > dc->props = gen_rp_props;
> >
> > diff --git a/hw/pci-bridge/pcie_root_port.c
> > b/hw/pci-bridge/pcie_root_port.c
> > index 4d588cb..2f3bcb1 100644
> > --- a/hw/pci-bridge/pcie_root_port.c
> > +++ b/hw/pci-bridge/pcie_root_port.c
> > @@ -52,7 +52,7 @@ static void rp_reset(DeviceState *qdev)
> >       pci_bridge_disable_base_limit(d);
> >   }
> >   -static void rp_realize(PCIDevice *d, Error **errp)
> > +void rp_realize(PCIDevice *d, Error **errp)
> >   {
> >       PCIEPort *p = PCIE_PORT(d);
> >       PCIESlot *s = PCIE_SLOT(d);
> > diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> > index 1333266..febd96a 100644
> > --- a/include/hw/pci/pcie_port.h
> > +++ b/include/hw/pci/pcie_port.h
> > @@ -63,6 +63,8 @@ void pcie_chassis_del_slot(PCIESlot *s);
> >   #define PCIE_ROOT_PORT_GET_CLASS(obj) \
> >        OBJECT_GET_CLASS(PCIERootPortClass, (obj), TYPE_PCIE_ROOT_PORT)
> >   +void rp_realize(PCIDevice *d, Error **errp);
>
> This is not how QEMU re-uses parent's realize function.
> You can grep for "parent_realize" in the project, it
> goes something like this:
> You add  "DeviceRealize parent_realize" to GenPCIERootPort class.
> In class_init you save parent's realize and replace it with
> your own:
>   grpc->parent_realize = dc->realize;
>   dc->realize = gen_rp_realize;
> In gen_rp_realize call first parent_realize:
>   rpc->parent_realize(dev, errp);
> your code here -
> if (err)
>       rpc-> exit()

OK, this is really much more pretty.
> Thanks,
> Marcel
> > typedef struct PCIERootPortClass {
> >     PCIDeviceClass parent_class;