Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for 32/64 word size mix

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 20 Jun 2016, Juergen Gross wrote:
> On 17/06/16 18:10, Stefano Stabellini wrote:
> > On Fri, 17 Jun 2016, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Juergen Gross [mailto:jgross@xxxxxxxx]
> >>> Sent: 17 June 2016 11:40
> >>> To: Paul Durrant; Jan Beulich
> >>> Cc: Anthony Perard; xen-devel; sstabellini@xxxxxxxxxx; qemu-
> >>> devel@xxxxxxxxxx; kraxel@xxxxxxxxxx
> >>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> >>> 32/64 word size mix
> >>>
> >>> On 17/06/16 12:15, Paul Durrant wrote:
> >>>>> -----Original Message-----
> >>>>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of
> >>>>> Juergen Gross
> >>>>> Sent: 17 June 2016 11:08
> >>>>> To: Paul Durrant; Jan Beulich
> >>>>> Cc: Anthony Perard; xen-devel; sstabellini@xxxxxxxxxx; qemu-
> >>>>> devel@xxxxxxxxxx; kraxel@xxxxxxxxxx
> >>>>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD for
> >>>>> 32/64 word size mix
> >>>>>
> >>>>> On 17/06/16 11:50, Paul Durrant wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Juergen Gross [mailto:jgross@xxxxxxxx]
> >>>>>>> Sent: 17 June 2016 10:46
> >>>>>>> To: Paul Durrant; Jan Beulich
> >>>>>>> Cc: Anthony Perard; xen-devel; sstabellini@xxxxxxxxxx; qemu-
> >>>>>>> devel@xxxxxxxxxx; kraxel@xxxxxxxxxx
> >>>>>>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk BLKIF_OP_DISCARD
> >>> for
> >>>>>>> 32/64 word size mix
> >>>>>>>
> >>>>>>> On 17/06/16 11:37, Paul Durrant wrote:
> >>>>>>>>> -----Original Message-----
> >>>>>>>>> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On
> >>> Behalf
> >>>>> Of
> >>>>>>> Jan
> >>>>>>>>> Beulich
> >>>>>>>>> Sent: 17 June 2016 10:26
> >>>>>>>>> To: Juergen Gross
> >>>>>>>>> Cc: Anthony Perard; xen-devel; sstabellini@xxxxxxxxxx; qemu-
> >>>>>>>>> devel@xxxxxxxxxx; kraxel@xxxxxxxxxx
> >>>>>>>>> Subject: Re: [Xen-devel] [PATCH v2] xen: fix qdisk
> >>> BLKIF_OP_DISCARD
> >>>>> for
> >>>>>>>>> 32/64 word size mix
> >>>>>>>>>
> >>>>>>>>>>>> On 17.06.16 at 11:14, <JGross@xxxxxxxx> wrote:
> >>>>>>>>>> In case the word size of the domU and qemu running the qdisk
> >>>>> backend
> >>>>>>>>>> differ BLKIF_OP_DISCARD will not work reliably, as the request
> >>>>>>>>>> structure in the ring have different layouts for different word 
> >>>>>>>>>> size.
> >>>>>>>>>>
> >>>>>>>>>> Correct this by copying the request structure in case of different
> >>>>>>>>>> word size element by element in the BLKIF_OP_DISCARD case, too.
> >>>>>>>>>>
> >>>>>>>>>> The easiest way to achieve this is to resync hw/block/xen_blkif.h
> >>> with
> >>>>>>>>>> its original source from the Linux kernel.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> >>>>>>>>>> ---
> >>>>>>>>>> V2: resync with Linux kernel version of hw/block/xen_blkif.h as
> >>>>>>>>>>     suggested by Paul Durrant
> >>>>>>>>>
> >>>>>>>>> Oh, I didn't realize he suggested syncing with the Linux variant.
> >>>>>>>>> Why not with the canonical one? I have to admit that I particularly
> >>>>>>>>> dislike Linux'es strange union-izng, mainly because of it requiring
> >>>>>>>>> this myriad of __attribute__((__packed__)).
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Yes, it's truly grotesque and such things should be blown away with
> >>>>>>> extreme prejudice.
> >>>>>>>
> >>>>>>> Sorry, I'm confused now.
> >>>>>>>
> >>>>>>> Do you still mandate for the resync or not?
> >>>>>>>
> >>>>>>> Resyncing with elimination of all the __packed__ stuff seems not to be
> >>>>>>> a proper alternative as this would require a major rework.
> >>>>>>
> >>>>>> Why? Replacing the existing horribleness with the canonical header
> >>> (fixed
> >>>>> for style) might mean a large diff but it should be functionally the 
> >>>>> same or
> >>>>> something has gone very seriously wrong. If the extra part you need is
> >>> not in
> >>>>> the canonical header then adding this as a second patch seems like a
> >>>>> reasonable plan.
> >>>>>
> >>>>> I think you don't realize that qemu is built using the public headers
> >>>>> from the Xen build environment. So there is no way to resync with the
> >>>>> canonical header as this isn't part of the qemu tree.
> >>>>>
> >>>>
> >>>> Now I'm confused... you're posting a patch to hw/block/xen_blkif.h. 
> >>>> That's
> >>> in the QEMU source, right? That's not a Xen public header but is a Linux
> >>> mangled variant of a Xen public header. So, actually, I guess the 
> >>> question is
> >>> why can't this header just go away and QEMU use the canonical header
> >>> directly from Xen?
> >>>
> >>> No, hw/block/xen_blkif.h is based on the Linux header
> >>> drivers/block/xen-blkback/common.h which is an add-on header to the
> >>> canonical-based Linux header include/xen/interface/io/blkif.h
> >>>
> >>>>> The header in question is originating from the Linux one which is an
> >>>>> add-on of the canonical header containing the explicit 32- and 64-bit
> >>>>> variants of the xenbus protocol and the conversion routines between
> >>>>> those.
> >>>>>
> >>>>> It would be possible to add these parts to the canonical header, but
> >>>>> do we really want that?
> >>>>>
> >>>>
> >>>> No, we shouldn't be taking Linux brokenness into the canonical header.
> >>>
> >>> Okay, so then back to the first approach using hw/block/xen_blkif.h as
> >>> today and adapting the style first and then doing the necessary code
> >>> correction?
> >>>
> >>
> >> I guess re-syncing with the Linux header as in your v2 patch is the least 
> >> worst option then.
> > 
> > As the Linux header is by no means canonical or special, I prefer patch
> > #1 (with the indentation fixed).
> 
> Could you please specify in which way the indentation should be fixed?
> Should I keep the current indentation of the file via tabs or should I
> prepend a patch to change the whole file to qemu style?

Please prepend a patch to fix indentation to match QEMU's style.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel