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

[Liu, Jing2]

Hi Laszlo,
Sorry for late reply. I missed these mails because of wrong filter.
And thanks very much for comments. My reply as belows.

On 8/7/2018 8:19 PM, Laszlo Ersek wrote:
> On 08/07/18 09:04, Jing Liu wrote:
[...]
> > @@ -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:
This is really good and I'm only not sure if DEFINE_PROP_ like,
DEFINE_PROP_UINT32("bus-reserve", GenPCIERootPort,
res_reserve.bus_reserve, -1), is a right/good way?
I mean the third parameter "_f".

[...]
> - 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.)
I consider whether we need limit the bus_reserve of pci-pci bridge. For 
old codes, we could add "unlimited" numbers of devices (but less than 
than max) onto pci-pci bridge.

> (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?
Umm, I looked into the codes that doesn't have hotplug property?
So do we need add resource reserve for it?

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

sure, can add a pre-check. But I don't understand why we need that,
msi_uninit() will check msi_present()?

> >   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.
OK, will add the details.


Thanks very much.
Jing

> For example:
>
>    hw/pci: support resource reservation capability on "pci-bridge"
>
> Thanks
> Laszlo