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

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

> -----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?

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

  Paul

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