Re: [Qemu-devel] [PATCH v2] virtio-mmio: implement modern (v2) personality (virtio-1)

[Sergio Lopez]

Cornelia Huck <address@hidden> writes:

> On Thu, 29 Aug 2019 18:50:27 +0200
> Sergio Lopez <address@hidden> wrote:
>
>> Implement the modern (v2) personality, according to the VirtIO 1.0
>> specification.
>> 
>> Support for v2 among guests is not as widespread as it'd be
>> desirable. While the Linux driver has had it for a while, support is
>> missing, at least, from Tianocore EDK II, NetBSD and FreeBSD.
>> 
>> For this reason, the v2 personality is disabled, keeping the legacy
>> behavior as default. Machine types willing to use v2, can enable it
>> using MachineClass's compat_props.
>> 
>> Signed-off-by: Sergio Lopez <address@hidden>
>> ---
>> Changelog:
>> 
>> v2:
>>  - Switch from RFC to PATCH.
>>  - Avoid the modern vs. legacy dichotomy. Use legacy or non-legacy
>>    instead. (Andrea Bolognani, Cornelia Huck)
>>  - Include the register offset in the warning messages. (Stefan
>>    Hajnoczi)
>>  - Fix device endianness for the non-legacy mode. (Michael S. Tsirkin)
>>  - Honor the specs in VIRTIO_MMIO_QUEUE_READY. (Michael S. Tsirkin)
>> ---
>>  hw/virtio/virtio-mmio.c | 296 +++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 279 insertions(+), 17 deletions(-)
>> 
>
>> @@ -146,28 +163,51 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr 
>> offset, unsigned size)
>>      case VIRTIO_MMIO_MAGIC_VALUE:
>>          return VIRT_MAGIC;
>>      case VIRTIO_MMIO_VERSION:
>> -        return VIRT_VERSION;
>> +        if (proxy->legacy) {
>> +            return VIRT_VERSION_LEGACY;
>> +        } else {
>> +            return VIRT_VERSION;
>> +        }
>>      case VIRTIO_MMIO_DEVICE_ID:
>>          return vdev->device_id;
>>      case VIRTIO_MMIO_VENDOR_ID:
>>          return VIRT_VENDOR;
>>      case VIRTIO_MMIO_DEVICE_FEATURES:
>> -        if (proxy->host_features_sel) {
>> -            return 0;
>> -        }
>> -        return vdev->host_features;
>> +        return vdev->host_features >> (32 * proxy->host_features_sel);
>
> Hm... I think you want to return 0 for host_features_sel > 0 on legacy
> devices.

You're right, better play safe here.

> Also, there's VirtIODeviceClass->legacy_features, which probably should
> be masked out for non-legacy devices?

Good point.

>> @@ -229,17 +275,33 @@ static void virtio_mmio_write(void *opaque, hwaddr 
>> offset, uint64_t value,
>>      }
>>      switch (offset) {
>>      case VIRTIO_MMIO_DEVICE_FEATURES_SEL:
>> -        proxy->host_features_sel = value;
>> +        if (value) {
>> +            proxy->host_features_sel = 1;
>> +        } else {
>> +            proxy->host_features_sel = 0;
>> +        }
>>          break;
>>      case VIRTIO_MMIO_DRIVER_FEATURES:
>> -        if (!proxy->guest_features_sel) {
>> +        if (!proxy->legacy) {
>> +            proxy->guest_features[proxy->guest_features_sel] = value;
>> +        } else if (!proxy->guest_features_sel) {
>>              virtio_set_features(vdev, value);
>
> If the guest tries to set something !0 for guest_features_sel > 0 on a
> legacy device, should that be logged as a guest bug?

We already have some warnings for unexpected guest behavior, so I agree
we should add this one for completeness.

>>          }
>>          break;
>
> (...)
>
> Otherwise, looks good to me.

Thanks!
Sergio.

signature.asc
Description: PGP signature