| Message ID | 20231113152114.47916-5-philmd@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/xen: Have most of Xen files become target-agnostic | expand |
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... :)
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.
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,
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(-)