Cornelia Huck <[email protected]> writes: > On Thu, 29 Aug 2019 18:50:27 +0200 > Sergio Lopez <[email protected]> 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 <[email protected]> >> --- >> 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
