Re: [Qemu-devel] [RFC 2/5] virtio: add child alias properties for virtio-blk

[Stefan Hajnoczi]

On Wed, May 14, 2014 at 02:04:25PM +0000, Peter Crosthwaite wrote:
> On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi <address@hidden> wrote:
> > Now that qdev child alias properties exist, define aliases for
> > virtio-blk properties.  The aliases will replace the duplicated
> > properties that current live in virtio-blk-pci, virtio-blk-ccw, and
> > friends.
> >
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > ---
> >  include/hw/block/block.h       | 14 ++++++++++++++
> >  include/hw/virtio/virtio-blk.h | 17 +++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> > index 7c3d6c8..702f1eb 100644
> > --- a/include/hw/block/block.h
> > +++ b/include/hw/block/block.h
> > @@ -52,11 +52,25 @@ static inline unsigned int 
> > get_physical_block_exp(BlockConf *conf)
> >      DEFINE_PROP_UINT32("discard_granularity", _state, \
> >                         _conf.discard_granularity, -1)
> >
> > +#define DEFINE_BLOCK_CHILD_ALIASES(_state, _field)                      \
> > +    DEFINE_PROP_CHILD_ALIAS("drive", _state, _field),                   \
> > +    DEFINE_PROP_CHILD_ALIAS("logical_block_size", _state, _field),      \
> > +    DEFINE_PROP_CHILD_ALIAS("physical_block_size", _state, _field),     \
> > +    DEFINE_PROP_CHILD_ALIAS("min_io_size", _state, _field),             \
> > +    DEFINE_PROP_CHILD_ALIAS("opt_io_size", _state, _field),             \
> > +    DEFINE_PROP_CHILD_ALIAS("bootindex", _state, _field),               \
> > +    DEFINE_PROP_CHILD_ALIAS("discard_granularity", _state, _field)
> > +
> >  #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
> >      DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
> >      DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
> >      DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
> >
> > +#define DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field)      \
> > +    DEFINE_PROP_CHILD_ALIAS("cyls", _state, _field),        \
> > +    DEFINE_PROP_CHILD_ALIAS("heads", _state, _field),       \
> > +    DEFINE_PROP_CHILD_ALIAS("secs", _state, _field)
> > +
> >  /* Configuration helpers */
> >
> >  void blkconf_serial(BlockConf *conf, char **serial);
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index e4c41ff..5095ff4 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -153,6 +153,23 @@ typedef struct VirtIOBlock {
> >          DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread)
> >  #endif /* __linux__ */
> >
> > +#ifdef __linux__
> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                    
> >    \
> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                        
> >    \
> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                    
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                 
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),             
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),                   
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> > +#else
> > +#define DEFINE_VIRTIO_BLK_CHILD_ALIASES(_state, _field)                    
> >    \
> > +        DEFINE_BLOCK_CHILD_ALIASES(_state, _field),                        
> >    \
> > +        DEFINE_BLOCK_CHS_CHILD_ALIASES(_state, _field),                    
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("serial", _state, _field),                 
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("config-wce", _state, _field),             
> >    \
> > +        DEFINE_PROP_CHILD_ALIAS("x-iothread", _state, _field)
> > +#endif /* __linux__ */
> 
> Can the duplication be avoided with:
> 
> #ifdef __linux__
> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX \
>         DEFINE_PROP_CHILD_ALIAS("scsi", _state, _field),
> #else
> #define DEFINE_VIRTIO_BLK_CHILD_ALIASES_LINUX
> #endif
> 
> ?

Absolutely, but I want each patch to do only one thing.

The "duplication" I referred to in the commit description is something
else: for virtio we create two sets of qdev properties with the same
name.  The first set is in the parent object and the second set is in
the child.  The actual implementation of this in hw/virtio/virtio-pci.c
varies between device types: in some cases we really have the property
values duplicated, in other cases we share the memory between parent and
child.  It's nuts and I want to eliminate that.

The __linux__ ifdef duplication is a minor thing that doesn't worry me.
It could be fixed in a trivial patch.

Stefan