Cornelia Huck <coh...@redhat.com> writes: > On Thu, 29 Aug 2019 18:50:27 +0200 > Sergio Lopez <s...@redhat.com> 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 <s...@redhat.com> >> --- >> 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