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
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~
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.
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... :)
© 2016 - 2026 Red Hat, Inc.