On Thu, Nov 24, 2011 at 01:28:52PM +0100, Paolo Bonzini wrote:
> vdev->guest_features is not masking features that are not supported by
> the guest. Fix this by introducing a common wrapper to be used by all
> virtio bus implementations.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
Yes, while I can't point to a specific problem
I think it's a very good idea to avoid invalid guest
feature values.
And removing code duplication is good too.
A minor comment below
> ---
> hw/s390-virtio-bus.c | 5 +----
> hw/syborg_virtio.c | 4 +---
> hw/virtio-pci.c | 9 ++-------
> hw/virtio.c | 24 ++++++++++++++++++------
> hw/virtio.h | 1 +
> 5 files changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 0ce6406..c4b9a99 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -254,10 +254,7 @@ void s390_virtio_device_update_status(VirtIOS390Device
> *dev)
> /* Update guest supported feature bitmap */
>
> features = bswap32(ldl_be_phys(dev->feat_offs));
> - if (vdev->set_features) {
> - vdev->set_features(vdev, features);
> - }
> - vdev->guest_features = features;
> + virtio_set_features(vdev, features);
> }
>
> VirtIOS390Device *s390_virtio_bus_console(VirtIOS390Bus *bus)
> diff --git a/hw/syborg_virtio.c b/hw/syborg_virtio.c
> index 00c7be8..6de952c 100644
> --- a/hw/syborg_virtio.c
> +++ b/hw/syborg_virtio.c
> @@ -131,9 +131,7 @@ static void syborg_virtio_writel(void *opaque,
> target_phys_addr_t offset,
> }
> switch (offset >> 2) {
> case SYBORG_VIRTIO_GUEST_FEATURES:
> - if (vdev->set_features)
> - vdev->set_features(vdev, value);
> - vdev->guest_features = value;
> + virtio_set_features(vdev, value);
> break;
> case SYBORG_VIRTIO_QUEUE_BASE:
> if (value == 0)
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index ca5923c..64c6a94 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -285,14 +285,9 @@ static void virtio_ioport_write(void *opaque, uint32_t
> addr, uint32_t val)
> case VIRTIO_PCI_GUEST_FEATURES:
> /* Guest does not negotiate properly? We have to assume nothing. */
> if (val & (1 << VIRTIO_F_BAD_FEATURE)) {
> - if (vdev->bad_features)
> - val = proxy->host_features & vdev->bad_features(vdev);
> - else
> - val = 0;
> + val = vdev->bad_features ? vdev->bad_features(vdev) : 0;
> }
> - if (vdev->set_features)
> - vdev->set_features(vdev, val);
> - vdev->guest_features = val;
> + virtio_set_features(vdev, val);
> break;
> case VIRTIO_PCI_QUEUE_PFN:
> pa = (target_phys_addr_t)val << VIRTIO_PCI_QUEUE_ADDR_SHIFT;
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 7011b5b..81ecc40 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -763,12 +763,25 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f)
> }
> }
>
> +int virtio_set_features(VirtIODevice *vdev, uint32_t val)
> +{
> + uint32_t supported_features =
> + vdev->binding->get_features(vdev->binding_opaque);
> + bool bad = (val & ~supported_features) != 0;
> +
> + val &= supported_features;
> + if (vdev->set_features) {
> + vdev->set_features(vdev, val);
> + }
> + vdev->guest_features = val;
> + return bad ? -1 : 0;
> +}
> +
Most users ignore the return value anyway,
and virtio_load needs to look at supported features
mask for the diagnostic anyway. So let's use virtio_set_features
void.
As a nice side effect, virtio_load can do it's checks
before anything is set, only set features if it's ok.
> int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> {
> int num, i, ret;
> uint32_t features;
> - uint32_t supported_features =
> - vdev->binding->get_features(vdev->binding_opaque);
> + uint32_t supported_features;
>
> if (vdev->binding->load_config) {
> ret = vdev->binding->load_config(vdev->binding_opaque, f);
> @@ -780,14 +793,13 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
> qemu_get_8s(f, &vdev->isr);
> qemu_get_be16s(f, &vdev->queue_sel);
> qemu_get_be32s(f, &features);
> - if (features & ~supported_features) {
> +
> + if (virtio_set_features(vdev, features) < 0) {
> + supported_features =
> vdev->binding->get_features(vdev->binding_opaque);
> error_report("Features 0x%x unsupported. Allowed features: 0x%x",
> features, supported_features);
> return -1;
> }
> - if (vdev->set_features)
> - vdev->set_features(vdev, features);
> - vdev->guest_features = features;
> vdev->config_len = qemu_get_be32(f);
> qemu_get_buffer(f, vdev->config, vdev->config_len);
>
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 2d18209..25f5564 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -185,6 +185,7 @@ void virtio_queue_set_vector(VirtIODevice *vdev, int n,
> uint16_t vector);
> void virtio_set_status(VirtIODevice *vdev, uint8_t val);
> void virtio_reset(void *opaque);
> void virtio_update_irq(VirtIODevice *vdev);
> +int virtio_set_features(VirtIODevice *vdev, uint32_t val);
>
> void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding,
> void *opaque);
> --
> 1.7.7.1
