[Qemu-devel] virtio 1 issues

[Qemu-devel] virtio 1 issues

[Michael S. Tsirkin]

Going over the code, I found a couple of issues.
Recording them here since I won't be able to work
on them until after the forum.


1. I realized we don't save/restore
serialize the following fields in virtio pci:

    uint32_t dfselect;
    uint32_t gfselect;
    uint32_t guest_features[2];
    struct {
        uint16_t num;
        bool enabled;
        uint32_t desc[2];
        uint32_t avail[2];
        uint32_t used[2];
    } vqs[VIRTIO_QUEUE_MAX];

This isn't an issue unless VM is migrated during driver initialization.

2. ring resizing is broken - it actually has a comment:
        /* TODO: need a way to put num back on reset. */
guest that runs out of memory might down-size the ring.
Afterwards, ring size won't come back up after reset - not nice.



Given that the modern layout is disabled by default,
I don't think these are release blockers.

-- 
MST

Re: [Qemu-devel] virtio 1 issues

[Jason Wang]

On 08/07/2015 06:31 AM, Michael S. Tsirkin wrote:
> Going over the code, I found a couple of issues.
> Recording them here since I won't be able to work
> on them until after the forum.
>
>
> 1. I realized we don't save/restore
> serialize the following fields in virtio pci:
>
>     uint32_t dfselect;
>     uint32_t gfselect;
>     uint32_t guest_features[2];
>     struct {
>         uint16_t num;
>         bool enabled;
>         uint32_t desc[2];
>         uint32_t avail[2];
>         uint32_t used[2];

desc, avail, used could be inferred from vring.desc, vring.avail and
vring.used
>     } vqs[VIRTIO_QUEUE_MAX];
>
> This isn't an issue unless VM is migrated during driver initialization.
>
> 2. ring resizing is broken - it actually has a comment:
>         /* TODO: need a way to put num back on reset. */
> guest that runs out of memory might down-size the ring.
> Afterwards, ring size won't come back up after reset - not nice.
>

Then we need a new field to keep track the original queue size and
migrate this.

>
> Given that the modern layout is disabled by default,
> I don't think these are release blockers.
>

Yes and looks like we need a new subsection and transport specific
callbacks to fix above?

Re: [Qemu-devel] virtio 1 issues

[Cornelia Huck]

On Fri, 07 Aug 2015 13:07:35 +0800
Jason Wang <jasowang@redhat.com> wrote:

> > 2. ring resizing is broken - it actually has a comment:
> >         /* TODO: need a way to put num back on reset. */
> > guest that runs out of memory might down-size the ring.
> > Afterwards, ring size won't come back up after reset - not nice.
> >
> 
> Then we need a new field to keep track the original queue size and
> migrate this.

virtio-ccw neatly sidesteps it by not implementing ring size changes at
all :)

But it isn't really all good, see

/* TODO: Add interface to handle vring.num changing */

We either need to implement this or fence off changing the ring size, I
guess.

> 
> >
> > Given that the modern layout is disabled by default,
> > I don't think these are release blockers.
> >
> 
> Yes and looks like we need a new subsection and transport specific
> callbacks to fix above?

Agreed, it's not urgent as virtio-1 is not yet the default (and not
even enabled for ccw).

I'm not sure we need transport-specific callbacks. Shouldn't the core
be able to track initial and actual size (and migrate in an optional
subsection), as long as the transports use defined interfaces to
interact with it?

Re: [Qemu-devel] virtio 1 issues

[Jason Wang]

On 08/07/2015 06:49 PM, Cornelia Huck wrote:
> On Fri, 07 Aug 2015 13:07:35 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>>> 2. ring resizing is broken - it actually has a comment:
>>>         /* TODO: need a way to put num back on reset. */
>>> guest that runs out of memory might down-size the ring.
>>> Afterwards, ring size won't come back up after reset - not nice.
>>>
>> Then we need a new field to keep track the original queue size and
>> migrate this.
> virtio-ccw neatly sidesteps it by not implementing ring size changes at
> all :)
>
> But it isn't really all good, see
>
> /* TODO: Add interface to handle vring.num changing */
>
> We either need to implement this or fence off changing the ring size, I
> guess.

Yes.

>>> Given that the modern layout is disabled by default,
>>> I don't think these are release blockers.
>>>
>> Yes and looks like we need a new subsection and transport specific
>> callbacks to fix above?
> Agreed, it's not urgent as virtio-1 is not yet the default (and not
> even enabled for ccw).
>
> I'm not sure we need transport-specific callbacks. Shouldn't the core
> be able to track initial and actual size (and migrate in an optional
> subsection), as long as the transports use defined interfaces to
> interact with it?
>

For size, I agree it should be tracked by core. But we need also track
transport specific data like dfselect/gfselect for pci.

Re: [Qemu-devel] virtio 1 issues

[Cornelia Huck]

On Mon, 10 Aug 2015 14:22:07 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 
> 
> On 08/07/2015 06:49 PM, Cornelia Huck wrote:
> > On Fri, 07 Aug 2015 13:07:35 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> >
> >>> 2. ring resizing is broken - it actually has a comment:
> >>>         /* TODO: need a way to put num back on reset. */
> >>> guest that runs out of memory might down-size the ring.
> >>> Afterwards, ring size won't come back up after reset - not nice.
> >>>
> >> Then we need a new field to keep track the original queue size and
> >> migrate this.
> > virtio-ccw neatly sidesteps it by not implementing ring size changes at
> > all :)
> >
> > But it isn't really all good, see
> >
> > /* TODO: Add interface to handle vring.num changing */
> >
> > We either need to implement this or fence off changing the ring size, I
> > guess.
> 
> Yes.
> 
> >>> Given that the modern layout is disabled by default,
> >>> I don't think these are release blockers.
> >>>
> >> Yes and looks like we need a new subsection and transport specific
> >> callbacks to fix above?
> > Agreed, it's not urgent as virtio-1 is not yet the default (and not
> > even enabled for ccw).
> >
> > I'm not sure we need transport-specific callbacks. Shouldn't the core
> > be able to track initial and actual size (and migrate in an optional
> > subsection), as long as the transports use defined interfaces to
> > interact with it?
> >
> 
> For size, I agree it should be tracked by core. But we need also track
> transport specific data like dfselect/gfselect for pci.

Yup. I'll leave pci data to the pci folks :) I have a patch for
handling the size; I'll post it after I've tested it a bit.