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

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

[Woodhouse, David]

[-- Attachment #1: Type: text/plain, Size: 1639 bytes --]



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?

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.

Can we be explicit about what's expected to work here and what's not in scope?




Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.



[-- Attachment #2: Type: text/html, Size: 2480 bytes --]

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

[Philippe Mathieu-Daudé]

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.

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.

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

Regards,

Phil.

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3402 bytes --]

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?

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 #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

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

[Philippe Mathieu-Daudé]

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

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 3202 bytes --]

On Tue, 2023-11-14 at 08:58 +0100, Philippe Mathieu-Daudé wrote:
> > > 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 :) 

Structures like that *shouldn't* differ between targets, but the Xen
struct blkif_request does:

typedef blkif_vdev_t uint16_t;
struct blkif_request {
    uint8_t        operation;    /* BLKIF_OP_???                         */
    uint8_t        nr_segments;  /* number of segments                   */
    blkif_vdev_t   handle;       /* only for read/write requests         */
    uint64_t       id;           /* private guest value, echoed in resp  */
    blkif_sector_t sector_number;/* start sector idx on disk (r/w only)  */
    struct blkif_request_segment seg[BLKIF_MAX_SEGMENTS_PER_REQUEST];
};

This is why we end up with explicit versions for x86-32 and x86-64,
with the 'id' field aligned explicitly to 4 or 8 bytes. The 'native'
version when we build it in qemu will just use the *host* ABI to decide
how to align it.

> And if they were, they wouldn't compile as target agnostic.

The words "wouldn't compile" gives me fantasies of a compiler that
literally errors out, saying "you can't use that struct in arch-
agnostic code because the padding is different on different
architectures".

It isn't so. You're just a tease.

With the exception of the x86-{32,64} special case, the
BLKIF_PROTOCOL_NATIVE support ends up using the *host* ABI, regardless
of which target it was aimed at.

Which might even be OK; are there any other targets which align
uint64_t to 4 bytes, like i386 does? And certainly isn't *your* problem
anyway.

What I was thinking was that if we *do* need to do something to make
BLKIF_PROTOCOL_NATIVE actually differ between targets, maybe that would
mean we really *do* want to build this code separately for each target
rather than just once? 

I suppose *if* we fix it, we can fix it in a way that doesn't require
specific compilation. Like we already did for x86. I literally use
object_dynamic_cast(qdev_get_machine(), "x86-machine") there.

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

No, there is support in the *kernel* for Xen guests, which hasn't been
added to AArch64 KVM.


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

[PATCH-for-9.0 00/10] hw/xen: Have most of Xen files become target-agnostic

[Philippe Mathieu-Daudé]

Hi,

After discussing with Alex Bennée I realized most Xen code
should be target-agnostic. David Woodhouse confirmed that
last week, so I had a quick look and here is the result.

More work is required to be able to instanciate Xen HW in
an heterogeneous machine, but this doesn't make sense yet
until we can run multiple accelerators concurrently.

Only build-tested.

Regards,

Phil.

Philippe Mathieu-Daudé (10):
  sysemu/xen: Forbid using Xen headers in user emulation
  hw/xen/xen_arch_hvm: Rename prototypes using 'xen_arch_' prefix
  hw/xen: Merge 'hw/xen/arch_hvm.h' in 'hw/xen/xen-hvm-common.h'
  hw/xen: Factor xen_arch_align_ioreq_data() out of handle_ioreq()
  hw/xen: Use target-agnostic qemu_target_page_bits()
  hw/xen: Reduce inclusion of 'cpu.h' to target-specific sources
  sysemu/xen-mapcache: Check Xen availability with
    CONFIG_XEN_IS_POSSIBLE
  system/physmem: Only include 'hw/xen/xen.h' when Xen is available
  hw/xen: Extract 'xen_igd.h' from 'xen_pt.h'
  hw/xen: Have most of Xen files become target-agnostic

 hw/xen/xen_pt.h                 | 14 --------------
 include/hw/arm/xen_arch_hvm.h   |  9 ---------
 include/hw/i386/xen_arch_hvm.h  | 11 -----------
 include/hw/xen/arch_hvm.h       |  5 -----
 include/hw/xen/xen-hvm-common.h |  8 +++++++-
 include/hw/xen/xen_igd.h        | 23 +++++++++++++++++++++++
 include/sysemu/xen-mapcache.h   |  3 ++-
 include/sysemu/xen.h            |  8 ++++----
 accel/xen/xen-all.c             |  1 +
 hw/arm/xen_arm.c                | 14 +++++++++++---
 hw/i386/pc_piix.c               |  1 +
 hw/i386/xen/xen-hvm.c           | 16 ++++++++++++----
 hw/xen/xen-hvm-common.c         | 16 +++++++---------
 hw/xen/xen_pt.c                 |  3 ++-
 hw/xen/xen_pt_config_init.c     |  3 ++-
 hw/xen/xen_pt_graphics.c        |  3 ++-
 hw/xen/xen_pt_stub.c            |  2 +-
 system/physmem.c                |  5 ++++-
 accel/xen/meson.build           |  2 +-
 hw/block/dataplane/meson.build  |  2 +-
 hw/xen/meson.build              | 13 ++++---------
 21 files changed, 85 insertions(+), 77 deletions(-)
 delete mode 100644 include/hw/arm/xen_arch_hvm.h
 delete mode 100644 include/hw/i386/xen_arch_hvm.h
 delete mode 100644 include/hw/xen/arch_hvm.h
 create mode 100644 include/hw/xen/xen_igd.h

-- 
2.41.0

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

[Philippe Mathieu-Daudé]

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()?
---
 include/hw/xen/xen-hvm-common.h | 1 +
 hw/arm/xen_arm.c                | 8 ++++++++
 hw/i386/xen/xen-hvm.c           | 8 ++++++++
 hw/xen/xen-hvm-common.c         | 5 +----
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/include/hw/xen/xen-hvm-common.h b/include/hw/xen/xen-hvm-common.h
index 27e938d268..734bfa3183 100644
--- a/include/hw/xen/xen-hvm-common.h
+++ b/include/hw/xen/xen-hvm-common.h
@@ -97,6 +97,7 @@ void xen_register_ioreq(XenIOState *state, unsigned int max_cpus,
 
 void cpu_ioreq_pio(ioreq_t *req);
 
+void xen_arch_align_ioreq_data(ioreq_t *req);
 void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req);
 void xen_arch_set_memory(XenIOState *state,
                          MemoryRegionSection *section,
diff --git a/hw/arm/xen_arm.c b/hw/arm/xen_arm.c
index 6a1d7719e9..c646fd70d0 100644
--- a/hw/arm/xen_arm.c
+++ b/hw/arm/xen_arm.c
@@ -128,6 +128,14 @@ static void xen_init_ram(MachineState *machine)
     }
 }
 
+void xen_arch_align_ioreq_data(ioreq_t *req)
+{
+    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)
+            && (req->size < sizeof(target_ulong))) {
+        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
+    }
+}
+
 void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req)
 {
     hw_error("Invalid ioreq type 0x%x\n", req->type);
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f8a195270a..aff5c5b81d 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -699,6 +699,14 @@ void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
     }
 }
 
+void xen_arch_align_ioreq_data(ioreq_t *req)
+{
+    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)
+            && (req->size < sizeof(target_ulong))) {
+        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
+    }
+}
+
 void xen_arch_handle_ioreq(XenIOState *state, ioreq_t *req)
 {
     switch (req->type) {
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
index c028c1b541..03f9417e7e 100644
--- a/hw/xen/xen-hvm-common.c
+++ b/hw/xen/xen-hvm-common.c
@@ -426,10 +426,7 @@ static void handle_ioreq(XenIOState *state, ioreq_t *req)
     trace_handle_ioreq(req, req->type, req->dir, req->df, req->data_is_ptr,
                        req->addr, req->data, req->count, req->size);
 
-    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
-            (req->size < sizeof (target_ulong))) {
-        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
-    }
+    xen_arch_align_ioreq_data(req);
 
     if (req->dir == IOREQ_WRITE)
         trace_handle_ioreq_write(req, req->type, req->df, req->data_is_ptr,
-- 
2.41.0

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

On Mon, 2023-11-13 at 16:21 +0100, Philippe Mathieu-Daudé 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>

I prefer commits like this to explicitly state 'No function change
intended', and on that basis:

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

But...



> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -699,6 +699,14 @@ void xen_arch_set_memory(XenIOState *state, MemoryRegionSection *section,
>      }
>  }
>  
> +void xen_arch_align_ioreq_data(ioreq_t *req)
> +{
> +    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)
> +            && (req->size < sizeof(target_ulong))) {
> +        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
> +    }
> +}
> +

If a 64-bit Xen host is running a 32-bit guest, what is target_ulong,
and what is the actual alignment? I think we are actually communicating
with the 64-bit Xen and it's 64 bits, although the *guest* is 32?

I guess the only time when this would matter is when using
qemu-system-i386 as the device model on 64-bit Xen? And that's not
going to work for various reasons including this?

(I should clarify that I'm not objecting to your patch series, but I
just to understand just what the situation is, before you make it
*look* saner than it is... :)

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

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

[Richard Henderson]

On 11/13/23 07:21, Philippe Mathieu-Daudé wrote:
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index c028c1b541..03f9417e7e 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -426,10 +426,7 @@ static void handle_ioreq(XenIOState *state, ioreq_t *req)
>       trace_handle_ioreq(req, req->type, req->dir, req->df, req->data_is_ptr,
>                          req->addr, req->data, req->count, req->size);
>   
> -    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
> -            (req->size < sizeof (target_ulong))) {
> -        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
> -    }


I suspect this should never have been using target_ulong at all: req->data is uint64_t.


r~

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

[Philippe Mathieu-Daudé]

On 13/11/23 19:16, Richard Henderson wrote:
> On 11/13/23 07:21, Philippe Mathieu-Daudé wrote:
>> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
>> index c028c1b541..03f9417e7e 100644
>> --- a/hw/xen/xen-hvm-common.c
>> +++ b/hw/xen/xen-hvm-common.c
>> @@ -426,10 +426,7 @@ static void handle_ioreq(XenIOState *state, 
>> ioreq_t *req)
>>       trace_handle_ioreq(req, req->type, req->dir, req->df, 
>> req->data_is_ptr,
>>                          req->addr, req->data, req->count, req->size);
>> -    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
>> -            (req->size < sizeof (target_ulong))) {
>> -        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
>> -    }
> 
> 
> I suspect this should never have been using target_ulong at all: 
> req->data is uint64_t.

This could replace it:

-- >8 --
-    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE) &&
-            (req->size < sizeof (target_ulong))) {
-        req->data &= ((target_ulong) 1 << (8 * req->size)) - 1;
+    if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)) {
+        req->data = extract64(req->data, 0, BITS_PER_BYTE * req->size);
      }
---

Some notes while looking at this.

Per xen/include/public/hvm/ioreq.h header:

#define IOREQ_TYPE_PIO          0 /* pio */
#define IOREQ_TYPE_COPY         1 /* mmio ops */
#define IOREQ_TYPE_PCI_CONFIG   2
#define IOREQ_TYPE_VMWARE_PORT  3
#define IOREQ_TYPE_TIMEOFFSET   7
#define IOREQ_TYPE_INVALIDATE   8 /* mapcache */

   struct ioreq {
     uint64_t addr;          /* physical address */
     uint64_t data;          /* data (or paddr of data) */
     uint32_t count;         /* for rep prefixes */
     uint32_t size;          /* size in bytes */
     uint32_t vp_eport;      /* evtchn for notifications to/from device 
model */
     uint16_t _pad0;
     uint8_t state:4;
     uint8_t data_is_ptr:1;  /* if 1, data above is the guest paddr
                              * of the real data to use. */
     uint8_t dir:1;          /* 1=read, 0=write */
     uint8_t df:1;
     uint8_t _pad1:1;
     uint8_t type;           /* I/O type */
   };
   typedef struct ioreq ioreq_t;

If 'data' is not a pointer, it is a u64.

- In PIO / VMWARE_PORT modes, only 32-bit are used.

- In MMIO COPY mode, memory is accessed by chunks of 64-bit

- In PCI_CONFIG mode, access is u8 or u16 or u32.

- None of TIMEOFFSET / INVALIDATE use 'req'.

- Fallback is only used in x86 for VMWARE_PORT.

--

Regards,

Phil.