search for: device_feature_select

Anthony Liguori <aliguori at us.ibm.com> writes: > "Michael S. Tsirkin" <mst at 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 q...

Anthony Liguori <aliguori at us.ibm.com> writes: > "Michael S. Tsirkin" <mst at 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 q...

On Wed, May 29, 2013 at 02:01:18PM +0930, Rusty Russell wrote: > Anthony Liguori <aliguori at us.ibm.com> writes: > > "Michael S. Tsirkin" <mst at 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 readab...

...unsigned size) > > +{ > > + VirtIOPCIProxy *proxy = opaque; > > + VirtIODevice *vdev = proxy->vdev; > > + > > + uint64_t low = 0xffffffffull; > > + > > + switch (addr) { > > + 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. Any good reason not to use offsetof? I see about 138 examples in qemu. Anyway, that's the way Rusty wrote it in the kernel header - I started with defines. If you c...

Paolo Bonzini <pbonzini at redhat.com> writes: > Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto: >>>> > > + >>>> > > + switch (addr) { >>>> > > + 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. >> Any good reason not to use offsetof? > > I'm not sure it's portable to use it in case labels. IIRC, the...

...d(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; Asserting is definitely the wrong thing here, since the guest can trigger it. If you really want to use offsetof like this you're going to need to decorate the structs with...

..._config_common_read(void *opaque, hwaddr addr, + unsigned size) +{ + VirtIOPCIProxy *proxy = opaque; + VirtIODevice *vdev = proxy->vdev; + + uint64_t low = 0xffffffffull; + + switch (addr) { + case offsetof(struct virtio_pci_common_cfg, device_feature_select): + return proxy->device_feature_select; + case offsetof(struct virtio_pci_common_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): +...

..._config_common_read(void *opaque, hwaddr addr, + unsigned size) +{ + VirtIOPCIProxy *proxy = opaque; + VirtIODevice *vdev = proxy->vdev; + + uint64_t low = 0xffffffffull; + + switch (addr) { + case offsetof(struct virtio_pci_common_cfg, device_feature_select): + return proxy->device_feature_select; + case offsetof(struct virtio_pci_common_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): +...

...; > 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; > > Asserting is definitely the wrong thing here, since the > guest can trigger it. So? It's a driver bug. It can reset or crash guest with the same effe...

..., > + unsigned size) > +{ > + VirtIOPCIProxy *proxy = opaque; > + VirtIODevice *vdev = proxy->vdev; > + > + uint64_t low = 0xffffffffull; > + > + switch (addr) { > + 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. >From a QEMU pov, take a look at: https://github.com/aliguori/qemu/commit/587c35c1a3fe90f6af0f97927047ef4d3182a659 And: https://github.com/aliguori/qemu/commit/01ba80a23cf2eb1e15...

Rusty Russell <rusty at rustcorp.com.au> writes: > Anthony Liguori <aliguori at us.ibm.com> writes: >> "Michael S. Tsirkin" <mst at 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. I think beauty is in the eye of the beholder here... Pretty much every device we have has a switch statement like this. Consisten...

...features */ +u64 ifcvf_get_dev_features(struct ifcvf_hw *hw) { return hw->dev_features; } +u64 ifcvf_get_driver_features(struct ifcvf_hw *hw) +{ + struct virtio_pci_common_cfg __iomem *cfg = hw->common_cfg; + u32 features_lo, features_hi; + u64 features; + + vp_iowrite32(0, &cfg->device_feature_select); + features_lo = vp_ioread32(&cfg->guest_feature); + + vp_iowrite32(1, &cfg->device_feature_select); + features_hi = vp_ioread32(&cfg->guest_feature); + + features = ((u64)features_hi << 32) | features_lo; + + return features; +} + int ifcvf_verify_min_features(struct i...

...at us.ibm.com> writes: > Rusty Russell <rusty at rustcorp.com.au> writes: > >> Anthony Liguori <aliguori at us.ibm.com> writes: >>> "Michael S. Tsirkin" <mst at 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. > > I think beauty is in the eye of the beholder here... > > Pretty much every device we have has a...

...at us.ibm.com> writes: > Rusty Russell <rusty at rustcorp.com.au> writes: > >> Anthony Liguori <aliguori at us.ibm.com> writes: >>> "Michael S. Tsirkin" <mst at 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. > > I think beauty is in the eye of the beholder here... > > Pretty much every device we have has a...

The spec is very clear on this: 4.1.3.1 Driver Requirements: PCI Device Layout The driver MUST access each field using the ?natural? access method, i.e. 32-bit accesses for 32-bit fields, 16-bit accesses for 16-bit fields and 8-bit accesses for 8-bit fields. Add type-safe wrappers to prevent access with incorrect width. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> ---

The spec is very clear on this: 4.1.3.1 Driver Requirements: PCI Device Layout The driver MUST access each field using the ?natural? access method, i.e. 32-bit accesses for 32-bit fields, 16-bit accesses for 16-bit fields and 8-bit accesses for 8-bit fields. Add type-safe wrappers to prevent access with incorrect width. Signed-off-by: Michael S. Tsirkin <mst at redhat.com> ---

...;> > +{ >> > + VirtIOPCIProxy *proxy = opaque; >> > + VirtIODevice *vdev = proxy->vdev; >> > + >> > + uint64_t low = 0xffffffffull; >> > + >> > + switch (addr) { >> > + 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. > > Any good reason not to use offsetof? > I see about 138 examples in qemu. There are exactly zero: $ find . -name "*.c" -exec grep -l &...

Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto: >>> > > + >>> > > + switch (addr) { >>> > > + 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. > Any good reason not to use offsetof? I'm not sure it's portable to use it in case labels. IIRC, the definition ((int)&((...

...y Russell ha scritto: > Paolo Bonzini <pbonzini at redhat.com> writes: >> Il 28/05/2013 19:32, Michael S. Tsirkin ha scritto: >>>>>>> + >>>>>>> + switch (addr) { >>>>>>> + 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. >>> Any good reason not to use offsetof? >> >> I'm not sure it's portable to use it in case...

...CI_CAP_ID_VNDR */ + __u8 cap_next; /* Generic PCI field: next ptr. */ + __u8 cfg_type; /* One of the VIRTIO_PCI_CAP_*_CFG. */ + __u8 bar; /* Where to find it. */ __le32 offset; /* Offset within bar. */ __le32 length; /* Length. */ }; @@ -144,7 +146,7 @@ struct virtio_pci_common_cfg { __le32 device_feature_select; /* read-write */ __le32 device_feature; /* read-only */ __le32 guest_feature_select; /* read-write */ - __le32 guest_feature; /* read-only */ + __le32 guest_feature; /* read-write */ __le16 msix_config; /* read-write */ __le16 num_queues; /* read-only */ __u8 device_status; /* read...