[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxx>
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@xxxxxxxxxx>
     ---
     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).



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.