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

[David Woodhouse <dwmw2@xxxxxxxxxxxxx>]

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?

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?

Attachment: smime.p7s
Description: S/MIME cryptographic signature