Re: [Qemu-devel] [PATCH v6 2/8] virtio-blk: add the virtio-blk device.

[Cornelia Huck]

On Thu, 14 Mar 2013 09:37:54 +0100
KONRAD Frédéric <address@hidden> wrote:

> On 14/03/2013 08:25, Cornelia Huck wrote:
> > On Wed, 13 Mar 2013 16:32:31 +0100
> > KONRAD Frédéric <address@hidden> wrote:
> >
> >> On 13/03/2013 09:24, KONRAD Frédéric wrote:
> >>> On 12/03/2013 17:31, Cornelia Huck wrote:
> >>>> On Tue, 12 Mar 2013 16:22:22 +0100
> >>>> KONRAD Frédéric <address@hidden> wrote:
> >>>>
> >>>>> On 12/03/2013 16:12, Peter Maydell wrote:
> >>>>>> On 12 March 2013 15:08, KONRAD Frédéric <address@hidden>
> >>>>>> wrote:
> >>>>>>> On 12/03/2013 15:42, Peter Maydell wrote:
> >>>>>>>> Yes, I see your problem there, but DEFINE_VIRTIO_BLK_SCSI_PROPERTY
> >>>>>>>> and DEFINE_DATA_PLANE_PROPERTIES are just convenience macros, not
> >>>>>>>> ones that are expected to be used by other code, right? So you can
> >>>>>>>> define them with commas (and name them something so it's obvious
> >>>>>>>> they're not intended for wider use as property array elements),
> >>>>>>>> and then just make sure your public-facing
> >>>>>>>> DEFINE_VIRTIO_BLK_PROPERTIES
> >>>>>>>> doesn't end with a comma. (You can do that by putting the macros
> >>>>>>>> that expand to maybe-comma-or-not at the front, not the end.)
> >>>>>>>>
> >>>>>>>> -- PMM
> >>>>>>> ok, I can put a comment which say not to use them?
> >>>>>> And suitable macro names (ie not ones which look like all
> >>>>>> the other DEFINE_FOO_PROPERTIES ones). Alternatively since the
> >>>>>> macro's only used once as far as I can see, you could just not
> >>>>>> bother to abstract it out. The virtio-ccw blk properties still
> >>>>>> just have inline #ifdefs for the scsi prop for instance.
> >>>>>>
> >>>>>> -- PMM
> >>>>> The macro is used for virtio-blk device and virtio-blk-pci.
> >>>>> s390x devices don't use the same properties.
> >>>>>
> >>>> Looking at the s390 devices, the difference seems to be the following:
> >>>>
> >>>> - CHS - missing on virtio-ccw, I'll do a patch.
> >>>> - config_wce - missing on s390-virtio and virtio-ccw, should probably
> >>>>     be added.
> >>>> - x-data-plane - we plan to add this eventually to virtio-ccw, but not
> >>>>     to s390-virtio. Could that be split out from the generic properties?
> >>>>
> >>> ok, so what I can do is:
> >>>
> >>> - split up x-data-plane property (so it will be only in virtio-pci.c).
> >>> - fix this comma thing.
> >>>
> >>> Then when you put these two missing properties you can just replace
> >>> all of them
> >>>   with the macro.
> >>>
> >>> Is that ok for everybody? Peter? Stefan?
> >>>
> >> Any other suggestion?
> > I currently have the following two patches sitting in my pending queue
> > (git://github.com/cohuck/qemu virtio-ccw-pending); I'll probably submit
> > them once my current pull request is through.
> >
> > On top of this, s390-virtio and virtio-ccw could use the generic macro
> > for the virtio-blk properties from the start if x-data-plane is split
> > out (I can add it to virtio-ccw once we support it).
> >
> Ok good,
> 
> And what about config-wce property for virtio-blk-s390x? It is missing too.

See the second patch :)