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

[Juergen Gross <jgross@xxxxxxxx>]

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?


Juergen


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