On Wed, May 29, 2013 at 02:01:18PM +0930, Rusty Russell wrote:
> Anthony Liguori <aligu...@us.ibm.com> writes:
> > "Michael S. Tsirkin" <m...@redhat.com> writes:
> >> + case offsetof(struct virtio_pci_common_cfg, device_feature_select):
> >> + return proxy->device_feature_select;
> >
> > Oh dear no... Please use defines like the rest of QEMU.
>
> It is pretty ugly.
>
> Yet the structure definitions are descriptive, capturing layout, size
> and endianness in natural a format readable by any C programmer.
>
> So AFAICT the question is, do we put the required
>
> #define VIRTIO_PCI_CFG_FEATURE_SEL \
> (offsetof(struct virtio_pci_common_cfg, device_feature_select))
>
> etc. in the kernel headers or qemu?
I forgot that we need to validate size (different fields
have different sizes so memory core does not validate
for us).
And that is much cleaner if we use offsetof directly:
You can see that you are checking the correct
size matching the offset, at a glance.
--->
virtio: new layout: add offset validation
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index f4db224..fd09ea7 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -467,51 +467,70 @@ static uint64_t virtio_pci_config_common_read(void
*opaque, hwaddr addr,
{
VirtIOPCIProxy *proxy = opaque;
VirtIODevice *vdev = proxy->vdev;
+ struct virtio_pci_common_cfg cfg;
uint64_t low = 0xffffffffull;
switch (addr) {
case offsetof(struct virtio_pci_common_cfg, device_feature_select):
+ assert(size == sizeof cfg.device_feature_select);
return proxy->device_feature_select;
case offsetof(struct virtio_pci_common_cfg, device_feature):
+ assert(size == sizeof cfg.device_feature);
/* TODO: 64-bit features */
return proxy->device_feature_select ? 0 : proxy->host_features;
case offsetof(struct virtio_pci_common_cfg, guest_feature_select):
+ assert(size == sizeof cfg.guest_feature_select);
return proxy->guest_feature_select;
case offsetof(struct virtio_pci_common_cfg, guest_feature):
+ assert(size == sizeof cfg.guest_feature);
/* TODO: 64-bit features */
return proxy->guest_feature_select ? 0 : vdev->guest_features;
case offsetof(struct virtio_pci_common_cfg, msix_config):
+ assert(size == sizeof cfg.msix_config);
return vdev->config_vector;
case offsetof(struct virtio_pci_common_cfg, num_queues):
+ assert(size == sizeof cfg.num_queues);
/* TODO: more exact limit? */
return VIRTIO_PCI_QUEUE_MAX;
case offsetof(struct virtio_pci_common_cfg, device_status):
+ assert(size == sizeof cfg.device_status);
return vdev->status;
/* About a specific virtqueue. */
case offsetof(struct virtio_pci_common_cfg, queue_select):
+ assert(size == sizeof cfg.queue_select);
return vdev->queue_sel;
case offsetof(struct virtio_pci_common_cfg, queue_size):
+ assert(size == sizeof cfg.queue_size);
return virtio_queue_get_num(vdev, vdev->queue_sel);
case offsetof(struct virtio_pci_common_cfg, queue_msix_vector):
+ assert(size == sizeof cfg.queue_msix_vector);
return virtio_queue_vector(vdev, vdev->queue_sel);
case offsetof(struct virtio_pci_common_cfg, queue_enable):
+ assert(size == sizeof cfg.queue_enable);
/* TODO */
return 0;
case offsetof(struct virtio_pci_common_cfg, queue_notify_off):
+ assert(size == sizeof cfg.queue_notify_off);
return vdev->queue_sel;
case offsetof(struct virtio_pci_common_cfg, queue_desc):
+ assert(size == 4);
return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) & low;
case offsetof(struct virtio_pci_common_cfg, queue_desc) + 4:
+ assert(size == 4);
return virtio_queue_get_desc_addr(vdev, vdev->queue_sel) >> 32;
case offsetof(struct virtio_pci_common_cfg, queue_avail):
+ assert(size == 4);
return virtio_queue_get_avail_addr(vdev, vdev->queue_sel) & low;
case offsetof(struct virtio_pci_common_cfg, queue_avail) + 4:
+ assert(size == 4);
return virtio_queue_get_avail_addr(vdev, vdev->queue_sel) >> 32;
case offsetof(struct virtio_pci_common_cfg, queue_used):
+ assert(size == 4);
return virtio_queue_get_used_addr(vdev, vdev->queue_sel) & low;
case offsetof(struct virtio_pci_common_cfg, queue_used) + 4:
+ assert(size == 4);
return virtio_queue_get_used_addr(vdev, vdev->queue_sel) >> 32;
default:
return 0;
@@ -523,76 +542,95 @@ static void virtio_pci_config_common_write(void *opaque,
hwaddr addr,
{
VirtIOPCIProxy *proxy = opaque;
VirtIODevice *vdev = proxy->vdev;
+ struct virtio_pci_common_cfg cfg;
uint64_t low = 0xffffffffull;
uint64_t high = ~low;
switch (addr) {
case offsetof(struct virtio_pci_common_cfg, device_feature_select):
+ assert(size == sizeof cfg.device_feature_select);
proxy->device_feature_select = val;
break;
case offsetof(struct virtio_pci_common_cfg, device_feature):
+ assert(size == sizeof cfg.device_feature);
break;
case offsetof(struct virtio_pci_common_cfg, guest_feature_select):
+ assert(size == sizeof cfg.guest_feature_select);
proxy->guest_feature_select = val;
break;
case offsetof(struct virtio_pci_common_cfg, guest_feature):
+ assert(size == sizeof cfg.guest_feature);
/* TODO: 64-bit features */
if (!proxy->guest_feature_select) {
virtio_set_features(vdev, val);
}
break;
case offsetof(struct virtio_pci_common_cfg, msix_config):
+ assert(size == sizeof cfg.msix_config);
vdev->config_vector = val;
break;
case offsetof(struct virtio_pci_common_cfg, num_queues):
+ assert(size == sizeof cfg.num_queues);
break;
case offsetof(struct virtio_pci_common_cfg, device_status):
+ assert(size == sizeof cfg.device_status);
virtio_pci_set_status(proxy, val);
break;
/* About a specific virtqueue. */
case offsetof(struct virtio_pci_common_cfg, queue_select):
+ assert(size == sizeof cfg.queue_select);
assert(val < VIRTIO_PCI_QUEUE_MAX);
vdev->queue_sel = val;
break;
case offsetof(struct virtio_pci_common_cfg, queue_size):
+ assert(size == sizeof cfg.queue_size);
assert(val && val < 0x8ffff && !(val & (val - 1)));
virtio_queue_set_num(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_msix_vector):
+ assert(size == sizeof cfg.queue_msix_vector);
virtio_queue_set_vector(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_enable):
+ assert(size == sizeof cfg.queue_enable);
/* TODO */
break;
case offsetof(struct virtio_pci_common_cfg, queue_notify_off):
+ assert(size == sizeof cfg.queue_notify_off);
break;
case offsetof(struct virtio_pci_common_cfg, queue_desc):
+ assert(size == 4);
val &= low;
val |= virtio_queue_get_desc_addr(vdev, vdev->queue_sel) & high;
virtio_queue_set_desc_addr(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_desc) + 4:
+ assert(size == 4);
val = val << 32;
val |= virtio_queue_get_desc_addr(vdev, vdev->queue_sel) & low;
virtio_queue_set_desc_addr(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_avail):
+ assert(size == 4);
val &= low;
val |= virtio_queue_get_avail_addr(vdev, vdev->queue_sel) & high;
virtio_queue_set_avail_addr(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_avail) + 4:
+ assert(size == 4);
val = val << 32;
val |= virtio_queue_get_avail_addr(vdev, vdev->queue_sel) & low;
virtio_queue_set_avail_addr(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_used):
+ assert(size == 4);
val &= low;
val |= virtio_queue_get_used_addr(vdev, vdev->queue_sel) & high;
virtio_queue_set_used_addr(vdev, vdev->queue_sel, val);
break;
case offsetof(struct virtio_pci_common_cfg, queue_used) + 4:
+ assert(size == 4);
val = val << 32;
val |= virtio_queue_get_used_addr(vdev, vdev->queue_sel) & low;
virtio_queue_set_used_addr(vdev, vdev->queue_sel, val);
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
