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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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).

If Juergen finds a way to make the appropriate changes to the canonical
header in Xen, afterwards we could consider re-importing it in QEMU. But
as it stands I am not going to ask him to do that in order to get a
simple patch like 1466071320-10964-1-git-send-email-jgross@xxxxxxxx into
QEMU.

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