Re: [Qemu-devel] [PATCH RFC] hw/virtio: Add PCIe capability to virtio devices

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 13, 2015 at 05:31:27PM +0300, Marcel Apfelbaum wrote:
> 
> 
> On Monday, October 12, 2015, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
>     On Mon, Oct 12, 2015 at 06:17:54PM +0300, Marcel Apfelbaum wrote:
>     > The virtio devices are converted to PCI-Express
>     > if they are plugged into a PCI-Express bus and
>     > the 'modern' protocol is enabled.
>     >
>     > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>     > ---
>     >
>     > This is an RFC because all it does it adds the PCIe capability and
>     nothing more.
> 
>     Express capability is easy.
>     But if you go over express space you will see that a bunch of
>     other capabilities are required, such as PM capability etc.
>     These might need more work.
> 
> 
> Sure, I'll look on the PCIe spec for the minimum requirements.
>  
> 
> 
>     > I post it early so I can get feedbacks on what is the best way to
>     continue it.
>     >
>     > Any comments would be appreciated,
>     > Thanks,
>     > Marcel
>     >
>     >  hw/virtio/virtio-pci.c | 11 +++++++++++
>     >  1 file changed, 11 insertions(+)
>     >
>     > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>     > index 6703806..f7c93d9 100644
>     > --- a/hw/virtio/virtio-pci.c
>     > +++ b/hw/virtio/virtio-pci.c
>     > @@ -1591,6 +1591,17 @@ static void virtio_pci_realize(PCIDevice *pci_dev,
>     Error **errp)
>     >
>     >      address_space_init(&proxy->modern_as, &proxy->modern_cfg,
>     "virtio-pci-cfg-as");
>     >
>     > +    if (!(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
>     > +        && pci_bus_is_express(pci_dev->bus)) {
> 
>     One point: we probably want to avoid doing this for integrated
>     devices on root bus. Does pci_bus_is_express return true there?
> 
> 
> Hmm, I'll check, but I think it does. Is this a must? 

It's probably a smart thing to do because you can't e.g.
hot-unplug them. So supporting PCI E capability for
integrated devices is probably more work than just making
the PCI devices (which is not spec compliant but very popular
so guests support this).

> 
> 
>     > +        int pos = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, 0,
>     > +                                     PCI_EXP_VER2_SIZEOF);
>     > +
>     > +        if (pos > 0) {
> 
>     We probably want to assert on pos < 0 instead.
>     That implies a code bug.
> 
> 
> 
> I was wondering what to do here, thanks for the idea!
> 
> Thanks,
> Marcel
> 
>  
> 
> 
>     > +            pci_dev->exp.exp_cap = pos;
>     > +            pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
>     > +        }
>     > +    }
>     > +
>     >      virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
>     >      if (k->realize) {
>     >          k->realize(proxy, errp);
>     > --
>     > 2.1.0
> 
>