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

[Juergen Gross]

On 17/06/16 12:15, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel [mailto:address@hidden On Behalf Of
>> Juergen Gross
>> Sent: 17 June 2016 11:08
>> To: Paul Durrant; Jan Beulich
>> Cc: Anthony Perard; xen-devel; address@hidden; qemu-
>> address@hidden; address@hidden
>> 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:address@hidden
>>>> Sent: 17 June 2016 10:46
>>>> To: Paul Durrant; Jan Beulich
>>>> Cc: Anthony Perard; xen-devel; address@hidden; qemu-
>>>> address@hidden; address@hidden
>>>> 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:address@hidden On Behalf
>> Of
>>>> Jan
>>>>>> Beulich
>>>>>> Sent: 17 June 2016 10:26
>>>>>> To: Juergen Gross
>>>>>> Cc: Anthony Perard; xen-devel; address@hidden; qemu-
>>>>>> address@hidden; address@hidden
>>>>>> 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, <address@hidden> 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 <address@hidden>
>>>>>>> ---
>>>>>>> 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?


Juergen