From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41146) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkaFc-0000d0-MT for qemu-devel@nongnu.org; Wed, 14 May 2014 10:33:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkaFR-0007M1-V0 for qemu-devel@nongnu.org; Wed, 14 May 2014 10:33:44 -0400 Received: from mail-wi0-x22a.google.com ([2a00:1450:400c:c05::22a]:37179) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkaFR-0007Lp-L6 for qemu-devel@nongnu.org; Wed, 14 May 2014 10:33:33 -0400 Received: by mail-wi0-f170.google.com with SMTP id bs8so8819354wib.3 for ; Wed, 14 May 2014 07:33:32 -0700 (PDT) Date: Wed, 14 May 2014 16:33:30 +0200 From: Stefan Hajnoczi Message-ID: <20140514143330.GA23702@stefanha-thinkpad.redhat.com> References: <1395244138-8834-1-git-send-email-stefanha@redhat.com> <1395244138-8834-3-git-send-email-stefanha@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC 2/5] virtio: add child alias properties for virtio-blk List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , Andreas Faerber , "qemu-devel@nongnu.org Developers" , Stefan Hajnoczi , =?iso-8859-1?Q?Fr=E9deric?= Konrad On Wed, May 14, 2014 at 02:04:25PM +0000, Peter Crosthwaite wrote: > On Wed, Mar 19, 2014 at 3:48 PM, Stefan Hajnoczi 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 > > --- > > 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