Re: [Qemu-devel] [PATCH] virtio: fix vring->inuse recalc after migr

[Halil Pasic <pasic@linux.vnet.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]



On 12/16/2016 11:25 AM, Stefan Hajnoczi wrote:
> On Thu, Dec 15, 2016 at 04:43:30PM +0100, Halil Pasic wrote:
>> Correct recalculation of vring->inuse after migration for
>> the corner case where the avail_idx has already wrapped
>> but used_idx not yet.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Fixes: bccdef6b ("virtio: recalculate vq->inuse after migration")
>> CC: qemu-stable@nongnu.org
>> ---
>>
>> I think we could also change the type of inuse to uint16_t.
>> Would this be considered a good idea?
> 
> VRing.num is unsigned int.  I would use the same type as num since this
> is a size, not an index.
> 

Thanks. I asked myself the question why are VRing.num and inuse (at
least theoretically arch depended width but at least 16 bit) int while
the indexes of the available and used rings uint16_t. Only thing I can
think of is some sort of optimization, because the only difference I can
see is that the VRing.num is communicated via the transport while the
indexes are a part (and/or corresponding to) of the ring layout (that is
the shared memory virtio structures and have a fixed width).

Now I'm kind of in doubt: I think having a signed has the benefit of a
negative value being more obviously wrong size (for a human looking at
it) that a to large positive, but purely theoretically the maximum value
of inuse is not guaranteed to fit int (as in C99 MIN_INT is 2^15 - 1).

What is your opinion? Should we keep 'inuse' as is, or should I change
it to unsigned with v2 (or prepare a separate patch)?


>> ---
>>  hw/virtio/virtio.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 1af2de2..089c6f6 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1855,9 +1855,12 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>>              /*
>>               * Some devices migrate VirtQueueElements that have been popped
>>               * from the avail ring but not yet returned to the used ring.
>> +             * Cast to uint16_t is OK because max ring size is 0x8000. Thus
>> +             * no the size of largest array indexable by an integral type
>> +             * can not be represented by the same type problem.
> 
> I think it would be safe up to max ring size UINT16_MAX - 1 (we need
> that extra value to distinguish between empty and full avail rings)?
>

Yeah.
 
> The last sentence is hard to understand due to the double negative.  I
> think you are saying "Since max ring size < UINT16_MAX it's safe to use
> uint16_t to represent the size of the ring".
> 

You are not the first one complaining, so the sentence is definitively
bad. What disturbs me regarding your formulation is that we do not use
uint16_t to represent neither the ring size nor inuse.

How about "Since max ring size < UINT16_MAX it's safe to use modulo
UINT16_MAX + 1 subtraction."?

Cheers,
Halil

>>               */
>> -            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
>> -                                vdev->vq[i].used_idx;
>> +            vdev->vq[i].inuse = (uint16_t)(vdev->vq[i].last_avail_idx -
>> +                                vdev->vq[i].used_idx);
>>              if (vdev->vq[i].inuse > vdev->vq[i].vring.num) {
>>                  error_report("VQ %d size 0x%x < last_avail_idx 0x%x - "
>>                               "used_idx 0x%x",
>> -- 
>> 2.8.4
>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]