| Message ID | 20231114143816.71079-14-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/xen: Have most of Xen files become target-agnostic | expand |
Cc'ing Anton. On 14/11/23 15:38, 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. > > Per xen/include/public/hvm/ioreq.h header: > > 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. > > Masking the upper bits of 'data' to keep 'req->size' low bits > is irrelevant of the target word size. Remove the word size > check and always extract the relevant bits. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/xen/xen-hvm-common.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c > index bb3cfb200c..fb81bd8fbc 100644 > --- a/hw/xen/xen-hvm-common.c > +++ b/hw/xen/xen-hvm-common.c > @@ -1,5 +1,6 @@ > #include "qemu/osdep.h" > #include "qemu/units.h" > +#include "qemu/bitops.h" > #include "qapi/error.h" > #include "trace.h" > > @@ -426,9 +427,8 @@ 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; > + if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)) { > + req->data = extract64(req->data, 0, BITS_PER_BYTE * req->size); > } > > if (req->dir == IOREQ_WRITE)
On Tue, Nov 14, 2023 at 03:38:09PM +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. > > Per xen/include/public/hvm/ioreq.h header: > > 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 Looks like it could also be 8, 16, or 32 as well, depending on req->size. > - 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. > > Masking the upper bits of 'data' to keep 'req->size' low bits > is irrelevant of the target word size. Remove the word size > check and always extract the relevant bits. When building QEMU for Xen, we tend to build the target "i386-softmmu", which looks like to have target_ulong == uint32_t. So the `data` clamping would only apply to size 8 and 16. The clamping with target_ulong was introduce long time ago, here: https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=b4a663b87df3954557434a2d31bff7f6b2706ec1 and they were more IOREQ types. So my guess is it isn't relevant anymore, but extending the clamping to 32-bits request should be fine, when using qemu-system-i386 that is, as it is already be done if one use qemu-system-x86_64. So I think the patch is fine, and the tests I've ran so far worked fine. > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c index bb3cfb200c..fb81bd8fbc 100644 --- a/hw/xen/xen-hvm-common.c +++ b/hw/xen/xen-hvm-common.c @@ -1,5 +1,6 @@ #include "qemu/osdep.h" #include "qemu/units.h" +#include "qemu/bitops.h" #include "qapi/error.h" #include "trace.h" @@ -426,9 +427,8 @@ 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; + if (!req->data_is_ptr && (req->dir == IOREQ_WRITE)) { + req->data = extract64(req->data, 0, BITS_PER_BYTE * req->size); } if (req->dir == IOREQ_WRITE)
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. Per xen/include/public/hvm/ioreq.h header: 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. Masking the upper bits of 'data' to keep 'req->size' low bits is irrelevant of the target word size. Remove the word size check and always extract the relevant bits. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/xen/xen-hvm-common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)