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

[Philippe Mathieu-Daudé <philmd@xxxxxxxxxx>]

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