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

[Marcel Apfelbaum]

On 11/08/2015 07:10 PM, Michael S. Tsirkin wrote:
> On Thu, Oct 29, 2015 at 01:56:28PM +0200, 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.
> >
> > Devices plugged directly into the Root Complex as
> > Integrated Endpoints remain PCI.
> >
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
>
>
> Looks ok. Two comments:
>
> > ---
> > v2 -> v3:
> >   - Addressed Michael S. Tsirkin's comments:
> >     - enable pcie only for 2.5+ machines.
> >
> > v1 -> v2:
> >   - Addressed Michael S. Tsirkin's comments:
> >     - Added the minimum required capabilities for PCIe devices
> >     - Integrated Endpoints remain PCI
> >
> >   - Use pcie_endpoint_cap_init instead of manually creating the pcie capability.
> >
> >   - Regarding Gerd Hoffman's comments:
> >     - Creating virtio-pcie devices:
> >       For the moment I prefer to not duplicate the virtio definitions,
> >       at least until we don't have a consensus (Personally I don't like it)
> >     - Removing the IO bar:
> >       This would be my next patch on the "virtio to express" series, I plan
> >       to remove it only for "modern" devices.
> >
> > Thanks,
> > Marcel
> >
> >   hw/virtio/virtio-pci.c | 22 ++++++++++++++++++++++
> >   hw/virtio/virtio-pci.h |  2 ++
> >   include/hw/compat.h    | 46 +++++++++++++++++++++++++++++++++++++++++++++-
> >   3 files changed, 69 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index f55dd2b..a288d8b 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -1592,6 +1592,26 @@ 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_PCIE)
> > +        && !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN)
> > +        && pci_bus_is_express(pci_dev->bus)
> > +        && !pci_bus_is_root(pci_dev->bus)) {
> > +        int pos;
> > +
> > +        pci_dev->cap_present |= QEMU_PCI_CAP_EXPRESS;
> > +        pos = pcie_endpoint_cap_init(pci_dev, 0);
> > +        assert(pos > 0);
> > +
> > +        pos = pci_add_capability(pci_dev, PCI_CAP_ID_PM, 0, PCI_PM_SIZEOF);
> > +        assert(pos > 0);
> > +
> > +        /*
> > +         * Indicates that this function complies with revision 1.2 of the
> > +         * PCI Power Management Interface Specification.
> > +         */
> > +        pci_set_word(pci_dev->config + pos + PCI_PM_PMC, 0x3);
> > +    }
> > +
> >       virtio_pci_bus_new(&proxy->bus, sizeof(proxy->bus), proxy);
> >       if (k->realize) {
> >           k->realize(proxy, errp);
> > @@ -1622,6 +1642,8 @@ static Property virtio_pci_properties[] = {
> >                       VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT, false),
> >       DEFINE_PROP_BIT("disable-modern", VirtIOPCIProxy, flags,
> >                       VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT, true),
> > +    DEFINE_PROP_BIT("disable-pcie", VirtIOPCIProxy, flags,
> > +                    VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT, false),
>
> It's preferable to call this one x-disable-pcie - this
> is the convention to mark it as "not for user".

Sure, I'll change it.

> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> > diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> > index 801c23a..1a487fc 100644
> > --- a/hw/virtio/virtio-pci.h
> > +++ b/hw/virtio/virtio-pci.h
> > @@ -72,8 +72,10 @@ typedef struct VirtioBusClass VirtioPCIBusClass;
> >   /* virtio version flags */
> >   #define VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT 2
> >   #define VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT 3
> > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT 4
> >   #define VIRTIO_PCI_FLAG_DISABLE_LEGACY (1 << 
> > VIRTIO_PCI_FLAG_DISABLE_LEGACY_BIT)
> >   #define VIRTIO_PCI_FLAG_DISABLE_MODERN (1 << 
> > VIRTIO_PCI_FLAG_DISABLE_MODERN_BIT)
> > +#define VIRTIO_PCI_FLAG_DISABLE_PCIE (1 << VIRTIO_PCI_FLAG_DISABLE_PCIE_BIT)
> >
> >   typedef struct {
> >       MSIMessage msg;
> > diff --git a/include/hw/compat.h b/include/hw/compat.h
> > index 095de5d..0a08531 100644
> > --- a/include/hw/compat.h
> > +++ b/include/hw/compat.h
> > @@ -2,7 +2,51 @@
> >   #define HW_COMPAT_H
> >
> >   #define HW_COMPAT_2_4 \
> > -        /* empty */
> > +        {\
> > +            .driver   = "virtio-blk-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-scsi-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-net-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-input-host-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-keyboard-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-mouse-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-serial-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-tablet-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-gpu-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-balloon-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },{\
> > +            .driver   = "virtio-rng-pci",\
> > +            .property = "disable-pcie",\
> > +            .value    = "on",\
> > +        },
>
> Can't these properties be defined for type virtio-pci?
> If no - why not?

You are right, I changed this in V4:
https://patchwork.ozlabs.org/patch/538709/

Thanks,
Marcel

> >   #define HW_COMPAT_2_3 \
> >           {\
> > --
> > 2.1.0