[4/7,s390] reset avail and used index on reboot

From: Jens Freimann <jfrei@linux.vnet.ibm.com>

reset the guest vring avail/used idx fields, otherwise it's possible
that old values remain in memory which would cause a reboot to fail
with a "Guest moved used index" message

Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio-bus.c |    7 +++++++
 hw/s390-virtio-bus.h |    2 ++
 2 files changed, 9 insertions(+)

On 23.04.2012, at 11:52, Christian Borntraeger wrote:

> From: Jens Freimann <jfrei@linux.vnet.ibm.com>
> 
> reset the guest vring avail/used idx fields, otherwise it's possible
> that old values remain in memory which would cause a reboot to fail
> with a "Guest moved used index" message
> 
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390-virtio-bus.c |    7 +++++++
> hw/s390-virtio-bus.h |    2 ++
> 2 files changed, 9 insertions(+)
> 
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 74419b3..084bac1 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -225,6 +225,7 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
> {
>     VirtIOS390Bus *bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
>     ram_addr_t cur_offs;
> +    target_phys_addr_t idx_addr;
>     uint8_t num_vq;
>     int i;
> 
> @@ -250,6 +251,12 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>         vring = s390_virtio_next_ring(bus);
>         virtio_queue_set_addr(dev->vdev, i, vring);
>         virtio_queue_set_vector(dev->vdev, i, i);
> +        idx_addr = virtio_queue_get_avail_addr(dev->vdev, i) +
> +                     VIRTIO_VRING_AVAIL_IDX_OFFS;
> +        stw_phys(idx_addr, 0);
> +        idx_addr = virtio_queue_get_used_addr(dev->vdev, i) +
> +                     VIRTIO_VRING_USED_IDX_OFFS;
> +        stw_phys(idx_addr, 0);

Are you sure this is correct to do in the sync callback? The idea of "sync" was to have a callback that can be called every time config information changes.

That could even be something as simple as a byte changing in the virtio device specific config space. But because we don't get callbacks when the guest accesses them, we need to synchronize it with real memory.


Alex

>         stq_be_phys(vq + VIRTIO_VQCONFIG_OFFS_ADDRESS, vring);
>         stw_be_phys(vq + VIRTIO_VQCONFIG_OFFS_NUM, virtio_queue_get_num(dev->vdev, i));
>     }
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 0e60bc0..8deec1e 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -34,6 +34,8 @@
> #define VIRTIO_VQCONFIG_LEN		24
> 
> #define VIRTIO_RING_LEN			(TARGET_PAGE_SIZE * 3)
> +#define VIRTIO_VRING_AVAIL_IDX_OFFS 2
> +#define VIRTIO_VRING_USED_IDX_OFFS 2
> #define S390_DEVICE_PAGES		512
> 
> #define VIRTIO_PARAM_MASK               0xff
> -- 
> 1.7.9.6
>

>> @@ -250,6 +251,12 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>>         vring = s390_virtio_next_ring(bus);
>>         virtio_queue_set_addr(dev->vdev, i, vring);
>>         virtio_queue_set_vector(dev->vdev, i, i);
>> +        idx_addr = virtio_queue_get_avail_addr(dev->vdev, i) +
>> +                     VIRTIO_VRING_AVAIL_IDX_OFFS;
>> +        stw_phys(idx_addr, 0);
>> +        idx_addr = virtio_queue_get_used_addr(dev->vdev, i) +
>> +                     VIRTIO_VRING_USED_IDX_OFFS;
>> +        stw_phys(idx_addr, 0);
> 
> Are you sure this is correct to do in the sync callback? The idea of "sync" was to have a callback that can be called every time config information changes.
> 
> That could even be something as simple as a byte changing in the virtio device specific config space. But because we don't get callbacks when the guest accesses them, we need to synchronize it with real memory.

Hmm, currently it is only called on init and virtio reset hypercall. Are you talking about
an intended user which is not yet finished?

Christian

On Thu, Apr 26, 2012 at 03:16:00PM +0200, Alexander Graf wrote:
> 
> On 23.04.2012, at 11:52, Christian Borntraeger wrote:
> 
> > From: Jens Freimann <jfrei@linux.vnet.ibm.com>
> > 
> > reset the guest vring avail/used idx fields, otherwise it's possible
> > that old values remain in memory which would cause a reboot to fail
> > with a "Guest moved used index" message
> > 
> > Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> > hw/s390-virtio-bus.c |    7 +++++++
> > hw/s390-virtio-bus.h |    2 ++
> > 2 files changed, 9 insertions(+)
> > 
> > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> > index 74419b3..084bac1 100644
> > --- a/hw/s390-virtio-bus.c
> > +++ b/hw/s390-virtio-bus.c
> > @@ -225,6 +225,7 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
> > {
> >     VirtIOS390Bus *bus = DO_UPCAST(VirtIOS390Bus, bus, dev->qdev.parent_bus);
> >     ram_addr_t cur_offs;
> > +    target_phys_addr_t idx_addr;
> >     uint8_t num_vq;
> >     int i;
> > 
> > @@ -250,6 +251,12 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
> >         vring = s390_virtio_next_ring(bus);
> >         virtio_queue_set_addr(dev->vdev, i, vring);
> >         virtio_queue_set_vector(dev->vdev, i, i);
> > +        idx_addr = virtio_queue_get_avail_addr(dev->vdev, i) +
> > +                     VIRTIO_VRING_AVAIL_IDX_OFFS;
> > +        stw_phys(idx_addr, 0);
> > +        idx_addr = virtio_queue_get_used_addr(dev->vdev, i) +
> > +                     VIRTIO_VRING_USED_IDX_OFFS;
> > +        stw_phys(idx_addr, 0);
> 
> Are you sure this is correct to do in the sync callback? The idea of "sync" was to have a callback that can be called every time config information changes.
> 
> That could even be something as simple as a byte changing in the virtio device specific config space. But because we don't get callbacks when the guest accesses them, we need to synchronize it with real memory.
 
It seemed to be the best place to do this, as right now it's called only in 
device init and the VIRTIO_RESET hypercall. If you have plans to change this
I guess we could do it in a reset handler as well. 

regards,
Jens

On 26.04.2012, at 15:56, Christian Borntraeger wrote:

>>> @@ -250,6 +251,12 @@ void s390_virtio_device_sync(VirtIOS390Device *dev)
>>>        vring = s390_virtio_next_ring(bus);
>>>        virtio_queue_set_addr(dev->vdev, i, vring);
>>>        virtio_queue_set_vector(dev->vdev, i, i);
>>> +        idx_addr = virtio_queue_get_avail_addr(dev->vdev, i) +
>>> +                     VIRTIO_VRING_AVAIL_IDX_OFFS;
>>> +        stw_phys(idx_addr, 0);
>>> +        idx_addr = virtio_queue_get_used_addr(dev->vdev, i) +
>>> +                     VIRTIO_VRING_USED_IDX_OFFS;
>>> +        stw_phys(idx_addr, 0);
>> 
>> Are you sure this is correct to do in the sync callback? The idea of "sync" was to have a callback that can be called every time config information changes.
>> 
>> That could even be something as simple as a byte changing in the virtio device specific config space. But because we don't get callbacks when the guest accesses them, we need to synchronize it with real memory.
> 
> Hmm, currently it is only called on init and virtio reset hypercall. Are you talking about
> an intended user which is not yet finished?

It's about the intended idea behind the "sync", yeah. So far, all virtio devices are ok with a simple workflow, where they never touch the config space again once they're initialized. But we can't guarantee that this will always be the case.

How about resetting the queues only on reset?


Alex