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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Juergen Gross [mailto:jgross@suse.com]
> Sent: 17 June 2016 11:40
> To: Paul Durrant; Jan Beulich
> Cc: Anthony Perard; xen-devel; sstabellini@kernel.org; qemu-
> devel@nongnu.org; kraxel@redhat.com
> 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@lists.xen.org] On Behalf Of
> >> Juergen Gross
> >> Sent: 17 June 2016 11:08
> >> To: Paul Durrant; Jan Beulich
> >> Cc: Anthony Perard; xen-devel; sstabellini@kernel.org; qemu-
> >> devel@nongnu.org; kraxel@redhat.com
> >> 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@suse.com]
> >>>> Sent: 17 June 2016 10:46
> >>>> To: Paul Durrant; Jan Beulich
> >>>> Cc: Anthony Perard; xen-devel; sstabellini@kernel.org; qemu-
> >>>> devel@nongnu.org; kraxel@redhat.com
> >>>> 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@lists.xen.org] On
> Behalf
> >> Of
> >>>> Jan
> >>>>>> Beulich
> >>>>>> Sent: 17 June 2016 10:26
> >>>>>> To: Juergen Gross
> >>>>>> Cc: Anthony Perard; xen-devel; sstabellini@kernel.org; qemu-
> >>>>>> devel@nongnu.org; kraxel@redhat.com
> >>>>>> 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@suse.com> 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@suse.com>
> >>>>>>> ---
> >>>>>>> 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.

  Paul

> 
> Juergen