Re: [PATCH-for-9.0 04/10] hw/xen: Factor xen_arch_align_ioreq_data() out of handle_ioreq()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 13/11/23 18:11, David Woodhouse wrote:
> On Mon, 2023-11-13 at 17:09 +0100, Philippe Mathieu-Daudé wrote:
>> On 13/11/23 16:58, Woodhouse, David wrote:
>>> On 13 Nov 2023 10:22, Philippe Mathieu-Daudé <philmd@linaro.org>
>>> wrote:
>>>
>>>      Per commit f17068c1c7 ("xen-hvm: reorganize xen-hvm and move
>>> common
>>>      function to xen-hvm-common"), handle_ioreq() is expected to be
>>>      target-agnostic. However it uses 'target_ulong', which is a
>>> target
>>>      specific definition.
>>>
>>>      In order to compile this file once for all targets, factor the
>>>      target-specific code out of handle_ioreq() as a per-target
>>> handler
>>>      called xen_arch_align_ioreq_data().
>>>
>>>      Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>      ---
>>>      Should we have a 'unsigned qemu_target_long_bits();' helper
>>>      such qemu_target_page_foo() API and target_words_bigendian()?
>>>
>>>
>>> It can be more fun than that though. What about
>>> qemu_target_alignof_uint64() for example, which differs between
>>> i386 and
>>> x86_64 and causes even structs with *explicitly* sized fields to
>>> differ
>>> because of padding.
>>>
>>> I'd *love* to see this series as a step towards my fantasy of being
>>> able
>>> to support Xen under TCG. After all, without that what's the point
>>> in
>>> being target-agnostic?
>>
>> Another win is we are building all these files once instead of one
>> for
>> each i386/x86_64/aarch64 targets, so we save CI time and Amazon
>> trees.
>>
>>> However, I am mildly concerned that some of these files are
>>> accidentally
>>> using the host ELF ABI, perhaps with explicit management of 32-bit
>>> compatibility, and the target-agnosticity is purely an illusion?
>>>
>>> See the "protocol" handling and the three ABIs for the ring in
>>> xen-block, for example.
>>
>> If so I'd expect build failures or violent runtime assertions.
> 
> Heh, mostly the guest just crashes in the cases I've seen so far.
> 
> See commit a1c1082908d ("hw/xen: use correct default protocol for xen-
> block on x86").
> 
>> Reviewing quickly hw/block/dataplane/xen-block.c, this code doesn't
>> seem target specific at all IMHO. Otherwise I'd really expect it to
>> fail compiling. But I don't know much about Xen, so I'll let block &
>> xen experts to have a look.
> 
> Where it checks dataplane->protocol and does different things for
> BLKIF_PROTOCOL_NATIVE/BLKIF_PROTOCOL_X86_32/BLKIF_PROTOCOL_X86_64, the
> *structures* it uses are intended to be using the correct ABI. I think
> the structs for BLKIF_PROTOCOL_NATIVE may actually be *different*
> according to the target, in theory?

OK I see what you mean, blkif_back_rings_t union in hw/block/xen_blkif.h

These structures shouldn't differ between targets, this is the point of
an ABI :) And if they were, they wouldn't compile as target agnostic.

> I don't know that they are *correct* right now, if the host is
> different from the target. But that's just a bug (that only matters if
> we ever want to support Xen-compatible guests using TCG).
> 
>>> Can we be explicit about what's expected to work here and what's
>>> not in scope?
>>
>> What do you mean? Everything is expected to work like without this
>> series applied :)
> 
> I think that if we ever do support Xen-compatible guests using TCG,
> we'll have to fix that bug and use the right target-specific
> structures... and then perhaps we'll want the affected files to
> actually become target-specfic again?
> 
> I think this series makes it look like target-agnostic support *should*
> work... but it doesn't really?

For testing we have:

aarch64: tests/avocado/boot_xen.py
x86_64: tests/avocado/kvm_xen_guest.py

No combination with i386 is tested,
Xen within aarch64 KVM is not tested (not sure it works).