Re: [Qemu-devel] How many msi-x vectors should be allocated for the virtio-serial device?

[Gal Hammer]

On 27/02/2013 16:03, Michael S. Tsirkin wrote:
> On Wed, Feb 27, 2013 at 03:46:25PM +0200, Gal Hammer wrote:
> > On 27/02/2013 15:28, Paolo Bonzini wrote:
> > > Il 27/02/2013 14:24, Gal Hammer ha scritto:
> > > > On 27/02/2013 14:35, Amit Shah wrote:
> > > > > On (Mon) 04 Feb 2013 [11:59:02], Michael S. Tsirkin wrote:
> > > > > > On Mon, Feb 04, 2013 at 10:42:32AM +0100, Paolo Bonzini wrote:
> > > > > > > Il 03/02/2013 13:11, Michael S. Tsirkin ha scritto:
> > > > > > > > > >    static Property virtio_serial_properties[] = {
> > > > > > > > > >        DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
> > > > > > > > > > VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> > > > > > > > > > -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
> > > > > > > > > > DEV_NVECTORS_UNSPECIFIED),
> > > > > > > > > > +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> > > > > > > > > >        DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> > > > > > > > > >        DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> > > > > > > > > >        DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy,
> > > > > > > > > > serial.max_virtserial_ports, 31),
> > > > > > > > > >
> > > > > > > > > > plus the backwards-compatibility stuff.
> > > > > > > > > >
> > > > > > > > > > Paolo
> > > > > > > > Makes sense, but the logic in virtio_serial_init_pci is
> > > > > > > > then dead code and should go away.
> > > > > > >
> > > > > > > It won't be dead code for the backwards-compatible machine types (that
> > > > > > > use DEV_NVECTORS_UNSPECIFIED).
> > > > > > >
> > > > > > > Paolo
> > > > > >
> > > > > > Good point. Ack. Want to post this with proper signature etc?
> > > > >
> > > > > Gal, do you want to submit a patch for this?
> > > > >
> > > > >          Amit
> > > >
> > > > Attached.
> > >
> > > You need to add backwards-compatibility properties in hw/pc.h.
> > >
> > > Paolo
> >
> > I've added the BC property. I hope it is in the right format.
> >
> > Thanks,
> >
> >      Gal.
>
> > >From db49a36ecc4de5744415357691140fb68df4fae2 Mon Sep 17 00:00:00 2001
> > From: Gal Hammer <address@hidden>
> > Date: Wed, 27 Feb 2013 15:15:31 +0200
> > Subject: [PATCH] Set virtio-serial device to have a default of 2 MSI vectors.
> >
> > The virtio-serial device is expected to use 2 MSI vectors, one for
> > control queue and a second shared for all queues.
> >
> > Signed-off-by: Gal Hammer <address@hidden>
> > ---
> >   hw/pc.h         | 4 ++++
> >   hw/virtio-pci.c | 5 ++++-
> >   2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pc.h b/hw/pc.h
> > index da1b102..d63d411 100644
> > --- a/hw/pc.h
> > +++ b/hw/pc.h
> > @@ -216,6 +216,10 @@ int e820_add_entry(uint64_t, uint64_t, uint32_t);
> >               .driver   = "virtio-blk-pci",\
> >               .property = "discard_granularity",\
> >               .value    = stringify(0),\
> > +       },{\
> > +            .driver   = "virtio-serial-pci",\
> > +            .property = "vectors",\
> > +            .value    = stringify(DEV_NVECTORS_UNSPECIFIED),\
> >         }
> >
> >   #endif
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index a869f53..ba56ab2 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -975,6 +975,9 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
> >       if (!vdev) {
> >           return -1;
> >       }
> > +
> > +    /* backwards-compatibility with machines that were created with
> > +       DEV_NVECTORS_UNSPECIFIED */
> >       vdev->nvectors = proxy->nvectors == DEV_NVECTORS_UNSPECIFIED
> >                                           ? proxy->serial.max_virtserial_ports 
> > + 1
> >                                           : proxy->nvectors;
> > @@ -1155,7 +1158,7 @@ static const TypeInfo virtio_net_info = {
> >
> >   static Property virtio_serial_properties[] = {
> >       DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, 
> > VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
> > -    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 
> > DEV_NVECTORS_UNSPECIFIED),
> > +    DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
> >       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
> >       DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> >       DEFINE_PROP_UINT32("max_ports", VirtIOPCIProxy, 
> > serial.max_virtserial_ports, 31),
> > --
> > 1.8.1.2
>
>
> Looks good but just making sure - you did test with compat machine type,
> yes?

After a small fix (attached), I've executed:

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci
vectors = 2

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-serial-pci -M pc-0.14
vectors = 32

Is this test is enough?

    Gal.

0001-Set-virtio-serial-device-to-have-a-default-of-2-MSI-.patch
Description: Text document