Re: [Qemu-devel] [SeaBIOS] [PATCH 07/18] virtio: add version 1.0 read/write macros

[Kevin O'Connor]

On Mon, Jun 29, 2015 at 10:53:29AM +0200, Gerd Hoffmann wrote:
> Add macros to read/write registers of virtio-1.0 regions.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  src/hw/virtio-pci.h | 76 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
> 
> diff --git a/src/hw/virtio-pci.h b/src/hw/virtio-pci.h
> index 893a7dd..e1d8b3e 100644
> --- a/src/hw/virtio-pci.h
> +++ b/src/hw/virtio-pci.h
> @@ -111,6 +111,82 @@ struct vp_device {
>      struct vp_cap common, notify, isr, device;
>  };
>  
> +#define vp_modern_read(_cap, _struct, _field, _var) {                   \
> +        u32 addr = _cap.addr;                                           \
> +        addr += offsetof(_struct, _field);                              \

Wouldn't this make more sense if the bulk of the code was in a
function?

That is, something like:

extern u32 _vp_modern_read(struct vp_cap *cap, u32 offset, u8 size);

#define vp_modern_read(_cap, _struct, _field, _var) {    \
  _var = _vp_modern_read(&_cap, offsetof(_struct, _field), sizeof((_struct 
*)0)->_field)

> +        if (_cap.is_io) {                                               \
> +            switch (sizeof(((_struct *)0)->_field)) {                   \
> +            case 8:                                                     \
> +                _var = inl(addr);                                       \
> +                _var |= (u64)inl(addr+4) << 32;                         \
> +                break;                                                  \

It didn't look like there were any 64bit fields defined, but maybe I
missed something.

BTW, last I checked, gcc didn't do a good job with code generation
when shifting 64bit values on a 32bit architecture.  Using unions (eg,
compiler.h:union u64_u32_u) seemed to produce better code.  (Though,
not worth bothering with if you want to use the same code in other
projects.)

-Kevin