| Message ID | 1435568020-8669-8-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <kraxel@redhat.com> > --- > 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
On Mo, 2015-06-29 at 09:02 -0400, Kevin O'Connor wrote: > 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 <kraxel@redhat.com> > > --- > > 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? The idea is that 'sizeof((_struct *)0)->_field)' evaluates to a compile-time constant, so gcc can optimize away the bulk of the #define. Also we don't have to use u64 for _var then (except when it actually is a 64bit value). But having 'vp_modern_read(..., var)' in the code instead of 'var = vp_modern_read(...)' isn't that nice indeed. > > + 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. The is a single one (number of sectors for virtio-blk disks). cheers, Gerd
On Mon, Jun 29, 2015 at 03:46:54PM +0200, Gerd Hoffmann wrote: > On Mo, 2015-06-29 at 09:02 -0400, Kevin O'Connor wrote: > > 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 <kraxel@redhat.com> > > > --- > > > 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? > > The idea is that 'sizeof((_struct *)0)->_field)' evaluates to a > compile-time constant, so gcc can optimize away the bulk of the #define. Would it matter though? Are any of these accesses performance sensitive? Also, if "_vp_modern_read()" was marked as inline wouldn't gcc ultimately produce the same thing? > Also we don't have to use u64 for _var then (except when it actually is > a 64bit value). > > But having 'vp_modern_read(..., var)' in the code instead of 'var = > vp_modern_read(...)' isn't that nice indeed. Hrmm. That's an interesting question - how well would gcc do if _vp_modern_read() was inline'd and returned a 64bit value that was then immediately truncated in the majority of cases. -Kevin
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); \ + if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = inl(addr); \ + _var |= (u64)inl(addr+4) << 32; \ + break; \ + case 4: \ + _var = inl(addr); \ + break; \ + case 2: \ + _var = inw(addr); \ + break; \ + case 1: \ + _var = inb(addr); \ + break; \ + default: \ + _var = 0; \ + } \ + } else { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 8: \ + _var = readl((void*)addr); \ + _var |= (u64)readl((void*)(addr+4)) << 32; \ + break; \ + case 4: \ + _var = readl((void*)addr); \ + break; \ + case 2: \ + _var = readw((void*)addr); \ + break; \ + case 1: \ + _var = readb((void*)addr); \ + break; \ + default: \ + _var = 0; \ + } \ + } \ + dprintf(9, "vp read %x (%d) -> 0x%x\n", \ + addr, sizeof(((_struct *)0)->_field), (u32)_var); \ + } + +#define vp_modern_write(_cap, _struct, _field, _var) { \ + u32 addr = _cap.addr; \ + addr += offsetof(_struct, _field); \ + dprintf(9, "vp write %x (%d) <- 0x%x\n", \ + addr, sizeof(((_struct *)0)->_field), (u32)_var); \ + if (_cap.is_io) { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 4: \ + outl(_var, addr); \ + break; \ + case 2: \ + outw(_var, addr); \ + break; \ + case 1: \ + outb(_var, addr); \ + break; \ + } \ + } else { \ + switch (sizeof(((_struct *)0)->_field)) { \ + case 4: \ + writel((void*)addr, _var); \ + break; \ + case 2: \ + writew((void*)addr, _var); \ + break; \ + case 1: \ + writeb((void*)addr, _var); \ + break; \ + } \ + } \ + } + static inline u32 vp_get_features(struct vp_device *vp) { return inl(GET_LOWFLAT(vp->ioaddr) + VIRTIO_PCI_HOST_FEATURES);
Add macros to read/write registers of virtio-1.0 regions. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- src/hw/virtio-pci.h | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)