| Message ID | 6a631756a126e73390f95b9e86c69e3286c92f59.1702991909.git.mykyta_poturai@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] xen/dm: arm: Introudce arm_inject_msi DM op | expand |
On 19.12.2023 14:48, Mykyta Poturai wrote: > This patch adds the ability for the device emulator to inject MSI > interrupts into guests. This is done by adding a new DM op to the device > model library. > > It is not possible to reuse already existing inject_msi DM op, because > it does not have a devid parameter, which is required for translation of > MSIs to interrupt numbers on ARM. Yet then ... > @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) > break; > } > > + case XEN_DMOP_arm_inject_msi: > + { > + const struct xen_dm_op_arm_inject_msi *data = > + &op.u.arm_inject_msi; > + > + if ( d->arch.vgic.its == NULL ) > + { > + rc = -EINVAL; > + break; > + } > + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); > + break; > + > + } ... you're not using the addr field at all, which therefore could likely hold the devid data (encoded to really represent some form of address, or stored directly - much depends on what purpose the address serves on Arm for MSI). > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus { > }; > typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t; > > +#define XEN_DMOP_arm_inject_msi 21 > + > +struct xen_dm_op_arm_inject_msi { > + uint64_t addr; > + uint32_t data; > + uint32_t devid; > +}; Additionally xen_dm_op_inject_msi has a padding field, which is likely possible to use as well (perhaps by simply renaming it). Also note how the addr field there is using uint64_aligned_t. Jan
Hi, On 19/12/2023 13:48, Mykyta Poturai wrote: > This patch adds the ability for the device emulator to inject MSI > interrupts into guests. This is done by adding a new DM op to the device > model library. > > It is not possible to reuse already existing inject_msi DM op, because > it does not have a devid parameter, which is required for translation of > MSIs to interrupt numbers on ARM. From the code below, it is not 100% clear what is the devid. It seems to be the device ID from the ITS PoV. However, the ID space is per ITS and I don't think it would be a good idea to design the DM OP interface with just one ITS in mind. It is also not clear how QEMU would be able to know the device ID. So I think the operation should take an SBDF. > > This approach was successfully tested on a virtio-pci setup with QEMU > backend emulating block devices with following changes from the mainline > Xen: > > This branch was taken as a base for virtio-pci: > https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 > > With added new VGICv3 from here: > https://github.com/Deedone/xen/tree/new_vgic_v2 > (although it should also work with the current VGIC) > > And patches from this branch to allow for proper ITS emulation in guests: > https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 > > The main purpose of this RFC is to get some feedback on the addition of > the new DM op. Is it the right approach or should we somehow modify the > existing inject_msi DM op to be compatible with ARM? In general the DM op interface is stable. So modification needs to be done with care. > > Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com> > --- > tools/include/xendevicemodel.h | 4 ++++ > tools/libs/devicemodel/core.c | 19 +++++++++++++++++++ > tools/libs/devicemodel/libxendevicemodel.map | 5 +++++ > xen/arch/arm/dm.c | 15 +++++++++++++++ > xen/arch/arm/include/asm/new_vgic.h | 2 ++ > xen/arch/arm/vgic/vgic-its.c | 2 +- > xen/include/public/hvm/dm_op.h | 10 ++++++++++ > 7 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h > index 797e0c6b29..e28710a6a5 100644 > --- a/tools/include/xendevicemodel.h > +++ b/tools/include/xendevicemodel.h > @@ -236,6 +236,10 @@ int xendevicemodel_inject_msi( > xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, > uint32_t msi_data); > > +int xendevicemodel_arm_inject_msi( > + xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t devid, In your new proposal, msi_addr is not used. As Jan just suggested, can we use it? If not, can you explain why? > + uint32_t data); > + > /** > * This function enables tracking of changes in the VRAM area. > * > diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c > index 8e619eeb0a..d15ffa46fb 100644 > --- a/tools/libs/devicemodel/core.c > +++ b/tools/libs/devicemodel/core.c > @@ -448,6 +448,25 @@ int xendevicemodel_set_irq_level( > return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op)); > } > > +int xendevicemodel_arm_inject_msi( > + xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t msi_data, > + uint32_t devid) > +{ > + struct xen_dm_op op; > + struct xen_dm_op_arm_inject_msi *data; > + > + memset(&op, 0, sizeof(op)); > + > + op.op = XEN_DMOP_arm_inject_msi; > + data = &op.u.arm_inject_msi; > + > + data->addr = msi_addr; > + data->devid = devid; > + data->data = msi_data; > + > + return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op)); > +} > + > int xendevicemodel_set_pci_link_route( > xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq) > { > diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map > index f7f9e3d932..8dceba5056 100644 > --- a/tools/libs/devicemodel/libxendevicemodel.map > +++ b/tools/libs/devicemodel/libxendevicemodel.map > @@ -44,3 +44,8 @@ VERS_1.4 { > xendevicemodel_set_irq_level; > xendevicemodel_nr_vcpus; > } VERS_1.3; > + > +VERS_1.5 { > + global: > + xendevicemodel_arm_inject_msi; > +} VERS_1.4; > diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c > index 5569efa121..b42f11e569 100644 > --- a/xen/arch/arm/dm.c > +++ b/xen/arch/arm/dm.c > @@ -27,6 +27,7 @@ int dm_op(const struct dmop_args *op_args) > [XEN_DMOP_set_ioreq_server_state] = sizeof(struct xen_dm_op_set_ioreq_server_state), > [XEN_DMOP_destroy_ioreq_server] = sizeof(struct xen_dm_op_destroy_ioreq_server), > [XEN_DMOP_set_irq_level] = sizeof(struct xen_dm_op_set_irq_level), > + [XEN_DMOP_arm_inject_msi] = sizeof(struct xen_dm_op_arm_inject_msi), > [XEN_DMOP_nr_vcpus] = sizeof(struct xen_dm_op_nr_vcpus), > }; > > @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) > break; > } > > + case XEN_DMOP_arm_inject_msi: > + { > + const struct xen_dm_op_arm_inject_msi *data = > + &op.u.arm_inject_msi; > + > + if ( d->arch.vgic.its == NULL ) > + { > + rc = -EINVAL; > + break; > + } > + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); vgic_its_trigger_msi() returns a value. So surely we want to propagate (possibly after translating the value). > + break; > + > + } > case XEN_DMOP_nr_vcpus: > { > struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus; > diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h > index dfc434ab41..dedc294ce9 100644 > --- a/xen/arch/arm/include/asm/new_vgic.h > +++ b/xen/arch/arm/include/asm/new_vgic.h > @@ -275,6 +275,8 @@ int vgic_its_add_device(struct domain *d, struct vgic_its_device *its_dev); > void vgic_its_delete_device(struct domain *d, struct vgic_its_device *its_dev); > struct vgic_its_device* vgic_its_get_device(struct domain *d, paddr_t vdoorbell, > uint32_t vdevid); > +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, > + u32 devid, u32 eventid); > #else > static inline void vgic_enable_lpis(struct vcpu *vcpu) > { > diff --git a/xen/arch/arm/vgic/vgic-its.c b/xen/arch/arm/vgic/vgic-its.c > index fd5aaf4a70..706987d93a 100644 > --- a/xen/arch/arm/vgic/vgic-its.c > +++ b/xen/arch/arm/vgic/vgic-its.c > @@ -608,7 +608,7 @@ int vgic_its_inject_cached_translation(struct domain *d, struct vgic_its *its, u > * Returns 0 on success, a positive error value for any ITS mapping > * related errors and negative error values for generic errors. > */ > -static int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, > +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, > u32 devid, u32 eventid) > { > struct vgic_irq *irq = NULL; > diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h > index fa98551914..a7d72e4389 100644 > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus { > }; > typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t; > > +#define XEN_DMOP_arm_inject_msi 21 > + > +struct xen_dm_op_arm_inject_msi { > + uint64_t addr; > + uint32_t data; > + uint32_t devid; > +}; > +typedef struct xen_dm_op_arm_inject_msi xen_dm_op_arm_inject_msi_t; > + > struct xen_dm_op { > uint32_t op; > uint32_t pad; > @@ -463,6 +472,7 @@ struct xen_dm_op { > xen_dm_op_set_mem_type_t set_mem_type; > xen_dm_op_inject_event_t inject_event; > xen_dm_op_inject_msi_t inject_msi; > + xen_dm_op_arm_inject_msi_t arm_inject_msi; > xen_dm_op_map_mem_type_to_ioreq_server_t map_mem_type_to_ioreq_server; > xen_dm_op_remote_shutdown_t remote_shutdown; > xen_dm_op_relocate_memory_t relocate_memory; Cheers,
Hi Jan, On 19/12/2023 13:57, Jan Beulich wrote: > On 19.12.2023 14:48, Mykyta Poturai wrote: >> This patch adds the ability for the device emulator to inject MSI >> interrupts into guests. This is done by adding a new DM op to the device >> model library. >> >> It is not possible to reuse already existing inject_msi DM op, because >> it does not have a devid parameter, which is required for translation of >> MSIs to interrupt numbers on ARM. > > Yet then ... > >> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) >> break; >> } >> >> + case XEN_DMOP_arm_inject_msi: >> + { >> + const struct xen_dm_op_arm_inject_msi *data = >> + &op.u.arm_inject_msi; >> + >> + if ( d->arch.vgic.its == NULL ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); >> + break; >> + >> + } > > ... you're not using the addr field at all, which therefore could likely > hold the devid data (encoded to really represent some form of address, > or stored directly - much depends on what purpose the address serves on > Arm for MSI). For real HW, the address would point to an ITS doorbell. All access will be tagged by the HW with the DeviceID. This is then used with the data (an event ID) to look up the associated interrupt. I think for Xen on Arm, we want 'addr' to be the SBDF. This could then be used to find the associated vITS and device ID. Ideally, this is an interface that will be agnostic to the MSI controller (someone need to check if the model I suggested above works with the GICv2m). Cheers,
On 19.12.2023 15:12, Julien Grall wrote: > On 19/12/2023 13:57, Jan Beulich wrote: >> On 19.12.2023 14:48, Mykyta Poturai wrote: >>> This patch adds the ability for the device emulator to inject MSI >>> interrupts into guests. This is done by adding a new DM op to the device >>> model library. >>> >>> It is not possible to reuse already existing inject_msi DM op, because >>> it does not have a devid parameter, which is required for translation of >>> MSIs to interrupt numbers on ARM. >> >> Yet then ... >> >>> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) >>> break; >>> } >>> >>> + case XEN_DMOP_arm_inject_msi: >>> + { >>> + const struct xen_dm_op_arm_inject_msi *data = >>> + &op.u.arm_inject_msi; >>> + >>> + if ( d->arch.vgic.its == NULL ) >>> + { >>> + rc = -EINVAL; >>> + break; >>> + } >>> + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); >>> + break; >>> + >>> + } >> >> ... you're not using the addr field at all, which therefore could likely >> hold the devid data (encoded to really represent some form of address, >> or stored directly - much depends on what purpose the address serves on >> Arm for MSI). > > For real HW, the address would point to an ITS doorbell. All access will > be tagged by the HW with the DeviceID. This is then used with the data > (an event ID) to look up the associated interrupt. So no properties of the destination are encoded in the address (besides it being an ITS specific address of course)? > I think for Xen on Arm, we want 'addr' to be the SBDF. This could then > be used to find the associated vITS and device ID. FTAOD, the vSBDF you mean then? Jan
Following up with relevant QEMU patch link. https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/
On 19/12/2023 1:48 pm, Mykyta Poturai wrote: > This patch adds the ability for the device emulator to inject MSI > interrupts into guests. This is done by adding a new DM op to the device > model library. > > It is not possible to reuse already existing inject_msi DM op, because > it does not have a devid parameter, which is required for translation of > MSIs to interrupt numbers on ARM. Ok, so the original hypercall is broken. But the new hypercall isn't ARM specific. It's just better form of inject_msi, and needed for all architectures. So, name it DMOP_inject_msi2 and get rid of the ARM infix. > This approach was successfully tested on a virtio-pci setup with QEMU > backend emulating block devices with following changes from the mainline > Xen: > > This branch was taken as a base for virtio-pci: > https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 > > With added new VGICv3 from here: > https://github.com/Deedone/xen/tree/new_vgic_v2 > (although it should also work with the current VGIC) > > And patches from this branch to allow for proper ITS emulation in guests: > https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 > > The main purpose of this RFC is to get some feedback on the addition of > the new DM op. Is it the right approach or should we somehow modify the > existing inject_msi DM op to be compatible with ARM? The DM_OP ABI does allow you to extend the structure behind DMOP_inject_msi, as long as 0 is meaningful. However, the semantics of zero-extending are wrong in this case, because it would mean that users of DMOP_inject_msi on an updated Xen would be sending interrupts with an implicit source id of host bridge. So you need a new DMOP_inject_msi2 that has better semantics. > diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h > index fa98551914..a7d72e4389 100644 > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus { > }; > typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t; > > +#define XEN_DMOP_arm_inject_msi 21 > + > +struct xen_dm_op_arm_inject_msi { > + uint64_t addr; > + uint32_t data; > + uint32_t devid; uint32_t source_id; /* PCI SBDF */ Technically the PCI spec calls it the Requester ID, but Source ID is the more common name. It is important not to use "devid" because that implies it's only a 3 bit number, and it isn't. ~Andrew
Hi Jan, On 19/12/2023 14:17, Jan Beulich wrote: > On 19.12.2023 15:12, Julien Grall wrote: >> On 19/12/2023 13:57, Jan Beulich wrote: >>> On 19.12.2023 14:48, Mykyta Poturai wrote: >>>> This patch adds the ability for the device emulator to inject MSI >>>> interrupts into guests. This is done by adding a new DM op to the device >>>> model library. >>>> >>>> It is not possible to reuse already existing inject_msi DM op, because >>>> it does not have a devid parameter, which is required for translation of >>>> MSIs to interrupt numbers on ARM. >>> >>> Yet then ... >>> >>>> @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) >>>> break; >>>> } >>>> >>>> + case XEN_DMOP_arm_inject_msi: >>>> + { >>>> + const struct xen_dm_op_arm_inject_msi *data = >>>> + &op.u.arm_inject_msi; >>>> + >>>> + if ( d->arch.vgic.its == NULL ) >>>> + { >>>> + rc = -EINVAL; >>>> + break; >>>> + } >>>> + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); >>>> + break; >>>> + >>>> + } >>> >>> ... you're not using the addr field at all, which therefore could likely >>> hold the devid data (encoded to really represent some form of address, >>> or stored directly - much depends on what purpose the address serves on >>> Arm for MSI). >> >> For real HW, the address would point to an ITS doorbell. All access will >> be tagged by the HW with the DeviceID. This is then used with the data >> (an event ID) to look up the associated interrupt. > > So no properties of the destination are encoded in the address (besides > it being an ITS specific address of course)? From my understanding, for the ITS yes. This could be different for other MSI controller. > >> I think for Xen on Arm, we want 'addr' to be the SBDF. This could then >> be used to find the associated vITS and device ID. > > FTAOD, the vSBDF you mean then? Indeed. Cheers,
On 19.12.2023 15:19, Andrew Cooper wrote: > On 19/12/2023 1:48 pm, Mykyta Poturai wrote: >> This patch adds the ability for the device emulator to inject MSI >> interrupts into guests. This is done by adding a new DM op to the device >> model library. >> >> It is not possible to reuse already existing inject_msi DM op, because >> it does not have a devid parameter, which is required for translation of >> MSIs to interrupt numbers on ARM. > > Ok, so the original hypercall is broken. > > But the new hypercall isn't ARM specific. It's just better form of > inject_msi, and needed for all architectures. > > So, name it DMOP_inject_msi2 and get rid of the ARM infix. > >> This approach was successfully tested on a virtio-pci setup with QEMU >> backend emulating block devices with following changes from the mainline >> Xen: >> >> This branch was taken as a base for virtio-pci: >> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 >> >> With added new VGICv3 from here: >> https://github.com/Deedone/xen/tree/new_vgic_v2 >> (although it should also work with the current VGIC) >> >> And patches from this branch to allow for proper ITS emulation in guests: >> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 >> >> The main purpose of this RFC is to get some feedback on the addition of >> the new DM op. Is it the right approach or should we somehow modify the >> existing inject_msi DM op to be compatible with ARM? > > The DM_OP ABI does allow you to extend the structure behind > DMOP_inject_msi, as long as 0 is meaningful. > > However, the semantics of zero-extending are wrong in this case, because > it would mean that users of DMOP_inject_msi on an updated Xen would be > sending interrupts with an implicit source id of host bridge. > > So you need a new DMOP_inject_msi2 that has better semantics. As said in another reply, the existing structure has a 32-bit padding field, which could be used here. In the handler it's properly being checked to be zero right now; whether that would want to remain this way, or whether we'd expect source ID to also be passed on x86 I don't know (yet). Jan
Hi, On 19/12/2023 14:18, Mykyta Poturai wrote: > Following up with relevant QEMU patch link. > > https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/ I don't seem to have the patch in my inbox. I guess you didn't CC xen-devel? Anyway, I will reply here. I think this is a mistake for QEMU to assume that Xen will expose a GICv3 ITS to the guest (we may decide to implement another MSI controller). But QEMU should really not need to implement a full ITS. What it needs is a way to forward the MSI to Xen. That's it. Stefano, do you have any suggestion how to do this in QEMU? Cheers,
On 19/12/2023 2:25 pm, Jan Beulich wrote: > On 19.12.2023 15:19, Andrew Cooper wrote: >> On 19/12/2023 1:48 pm, Mykyta Poturai wrote: >>> This patch adds the ability for the device emulator to inject MSI >>> interrupts into guests. This is done by adding a new DM op to the device >>> model library. >>> >>> It is not possible to reuse already existing inject_msi DM op, because >>> it does not have a devid parameter, which is required for translation of >>> MSIs to interrupt numbers on ARM. >> Ok, so the original hypercall is broken. >> >> But the new hypercall isn't ARM specific. It's just better form of >> inject_msi, and needed for all architectures. >> >> So, name it DMOP_inject_msi2 and get rid of the ARM infix. >> >>> This approach was successfully tested on a virtio-pci setup with QEMU >>> backend emulating block devices with following changes from the mainline >>> Xen: >>> >>> This branch was taken as a base for virtio-pci: >>> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 >>> >>> With added new VGICv3 from here: >>> https://github.com/Deedone/xen/tree/new_vgic_v2 >>> (although it should also work with the current VGIC) >>> >>> And patches from this branch to allow for proper ITS emulation in guests: >>> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 >>> >>> The main purpose of this RFC is to get some feedback on the addition of >>> the new DM op. Is it the right approach or should we somehow modify the >>> existing inject_msi DM op to be compatible with ARM? >> The DM_OP ABI does allow you to extend the structure behind >> DMOP_inject_msi, as long as 0 is meaningful. >> >> However, the semantics of zero-extending are wrong in this case, because >> it would mean that users of DMOP_inject_msi on an updated Xen would be >> sending interrupts with an implicit source id of host bridge. >> >> So you need a new DMOP_inject_msi2 that has better semantics. > As said in another reply, the existing structure has a 32-bit padding > field, which could be used here. In the handler it's properly being > checked to be zero right now; It's still not safe to reuse this zero for a source ID semantic behind the back of older userspace. > whether that would want to remain this > way, or whether we'd expect source ID to also be passed on x86 I don't > know (yet). We do need the source ID in x86, as soon as the guest has vIOMMU for any reason. It's a design error that it wasn't added originally, but I suppose you can say the same of x86 platforms in general, having to retrofit an OS-visible Source ID to HPETs/IO-APICs to make them compatible with IOMMUs. ~Andrew
On 19.12.2023 15:33, Andrew Cooper wrote: > On 19/12/2023 2:25 pm, Jan Beulich wrote: >> On 19.12.2023 15:19, Andrew Cooper wrote: >>> On 19/12/2023 1:48 pm, Mykyta Poturai wrote: >>>> This patch adds the ability for the device emulator to inject MSI >>>> interrupts into guests. This is done by adding a new DM op to the device >>>> model library. >>>> >>>> It is not possible to reuse already existing inject_msi DM op, because >>>> it does not have a devid parameter, which is required for translation of >>>> MSIs to interrupt numbers on ARM. >>> Ok, so the original hypercall is broken. >>> >>> But the new hypercall isn't ARM specific. It's just better form of >>> inject_msi, and needed for all architectures. >>> >>> So, name it DMOP_inject_msi2 and get rid of the ARM infix. >>> >>>> This approach was successfully tested on a virtio-pci setup with QEMU >>>> backend emulating block devices with following changes from the mainline >>>> Xen: >>>> >>>> This branch was taken as a base for virtio-pci: >>>> https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 >>>> >>>> With added new VGICv3 from here: >>>> https://github.com/Deedone/xen/tree/new_vgic_v2 >>>> (although it should also work with the current VGIC) >>>> >>>> And patches from this branch to allow for proper ITS emulation in guests: >>>> https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 >>>> >>>> The main purpose of this RFC is to get some feedback on the addition of >>>> the new DM op. Is it the right approach or should we somehow modify the >>>> existing inject_msi DM op to be compatible with ARM? >>> The DM_OP ABI does allow you to extend the structure behind >>> DMOP_inject_msi, as long as 0 is meaningful. >>> >>> However, the semantics of zero-extending are wrong in this case, because >>> it would mean that users of DMOP_inject_msi on an updated Xen would be >>> sending interrupts with an implicit source id of host bridge. >>> >>> So you need a new DMOP_inject_msi2 that has better semantics. >> As said in another reply, the existing structure has a 32-bit padding >> field, which could be used here. In the handler it's properly being >> checked to be zero right now; > > It's still not safe to reuse this zero for a source ID semantic behind > the back of older userspace. As long as we simply ignore that field's value, I don't see anything wrong there (not very different from Arm ignoring the address, as the intent looks to be). And ... >> whether that would want to remain this >> way, or whether we'd expect source ID to also be passed on x86 I don't >> know (yet). > > We do need the source ID in x86, as soon as the guest has vIOMMU for any > reason. ... I wonder whether I'll still be around when Xen actually gets there. Jan > It's a design error that it wasn't added originally, but I suppose you > can say the same of x86 platforms in general, having to retrofit an > OS-visible Source ID to HPETs/IO-APICs to make them compatible with IOMMUs. > > ~Andrew >
On Tue, 19 Dec 2023, Julien Grall wrote: > Hi, > > On 19/12/2023 14:18, Mykyta Poturai wrote: > > Following up with relevant QEMU patch link. > > > > https://patchwork.kernel.org/project/qemu-devel/patch/c7a180a5874f036c246fc39f921eefafecbc8c76.1702994649.git.mykyta_poturai@epam.com/ > > I don't seem to have the patch in my inbox. I guess you didn't CC xen-devel? > > Anyway, I will reply here. I think this is a mistake for QEMU to assume that > Xen will expose a GICv3 ITS to the guest (we may decide to implement another > MSI controller). > > But QEMU should really not need to implement a full ITS. What it needs is a > way to forward the MSI to Xen. That's it. I fully agree with Julien > Stefano, do you have any suggestion how to do this in QEMU? Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM
Hi, On 20.12.23 02:42, Stefano Stabellini wrote: > On Tue, 19 Dec 2023, Julien Grall wrote: >> >> But QEMU should really not need to implement a full ITS. What it needs is a >> way to forward the MSI to Xen. That's it. > > I fully agree with Julien > > >> Stefano, do you have any suggestion how to do this in QEMU? > > Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM It is exactly like xen_apic.c. All this implementation does is getting the MSI messages and forwarding them to Xen using the DM op from this patch.
Hi, On 20/12/2023 09:13, Mykyta Poturai wrote: > > Hi, > > On 20.12.23 02:42, Stefano Stabellini wrote: >> On Tue, 19 Dec 2023, Julien Grall wrote: >>> >>> But QEMU should really not need to implement a full ITS. What it needs is a >>> way to forward the MSI to Xen. That's it. >> >> I fully agree with Julien >> >> >>> Stefano, do you have any suggestion how to do this in QEMU? >> >> Yes, we just need something like hw/i386/xen/xen_apic.c but for ARM > > It is exactly like xen_apic.c. All this implementation does is getting > the MSI messages and forwarding them to Xen using the DM op from this patch. If this implementation is only getting the MSI messages and forwarding them, then why is it built on top of the vGICv3 ITS structure? Shouldn't instead be something that is agnostic to the interrupt controller (and even possibly arch agnostic)? The point here is in Xen context, QEMU doesn't know which interrupt controller will be exposed to the guest. QEMU is just used for emulating devices and none of this should really be architecture specific. Instead, it should be a Xen specific way to inject MSI/IRQ. Cheers,
diff --git a/tools/include/xendevicemodel.h b/tools/include/xendevicemodel.h index 797e0c6b29..e28710a6a5 100644 --- a/tools/include/xendevicemodel.h +++ b/tools/include/xendevicemodel.h @@ -236,6 +236,10 @@ int xendevicemodel_inject_msi( xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t msi_data); +int xendevicemodel_arm_inject_msi( + xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t devid, + uint32_t data); + /** * This function enables tracking of changes in the VRAM area. * diff --git a/tools/libs/devicemodel/core.c b/tools/libs/devicemodel/core.c index 8e619eeb0a..d15ffa46fb 100644 --- a/tools/libs/devicemodel/core.c +++ b/tools/libs/devicemodel/core.c @@ -448,6 +448,25 @@ int xendevicemodel_set_irq_level( return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op)); } +int xendevicemodel_arm_inject_msi( + xendevicemodel_handle *dmod, domid_t domid, uint64_t msi_addr, uint32_t msi_data, + uint32_t devid) +{ + struct xen_dm_op op; + struct xen_dm_op_arm_inject_msi *data; + + memset(&op, 0, sizeof(op)); + + op.op = XEN_DMOP_arm_inject_msi; + data = &op.u.arm_inject_msi; + + data->addr = msi_addr; + data->devid = devid; + data->data = msi_data; + + return xendevicemodel_op(dmod, domid, 1, &op, sizeof(op)); +} + int xendevicemodel_set_pci_link_route( xendevicemodel_handle *dmod, domid_t domid, uint8_t link, uint8_t irq) { diff --git a/tools/libs/devicemodel/libxendevicemodel.map b/tools/libs/devicemodel/libxendevicemodel.map index f7f9e3d932..8dceba5056 100644 --- a/tools/libs/devicemodel/libxendevicemodel.map +++ b/tools/libs/devicemodel/libxendevicemodel.map @@ -44,3 +44,8 @@ VERS_1.4 { xendevicemodel_set_irq_level; xendevicemodel_nr_vcpus; } VERS_1.3; + +VERS_1.5 { + global: + xendevicemodel_arm_inject_msi; +} VERS_1.4; diff --git a/xen/arch/arm/dm.c b/xen/arch/arm/dm.c index 5569efa121..b42f11e569 100644 --- a/xen/arch/arm/dm.c +++ b/xen/arch/arm/dm.c @@ -27,6 +27,7 @@ int dm_op(const struct dmop_args *op_args) [XEN_DMOP_set_ioreq_server_state] = sizeof(struct xen_dm_op_set_ioreq_server_state), [XEN_DMOP_destroy_ioreq_server] = sizeof(struct xen_dm_op_destroy_ioreq_server), [XEN_DMOP_set_irq_level] = sizeof(struct xen_dm_op_set_irq_level), + [XEN_DMOP_arm_inject_msi] = sizeof(struct xen_dm_op_arm_inject_msi), [XEN_DMOP_nr_vcpus] = sizeof(struct xen_dm_op_nr_vcpus), }; @@ -112,6 +113,20 @@ int dm_op(const struct dmop_args *op_args) break; } + case XEN_DMOP_arm_inject_msi: + { + const struct xen_dm_op_arm_inject_msi *data = + &op.u.arm_inject_msi; + + if ( d->arch.vgic.its == NULL ) + { + rc = -EINVAL; + break; + } + vgic_its_trigger_msi(d, d->arch.vgic.its, data->devid, data->data); + break; + + } case XEN_DMOP_nr_vcpus: { struct xen_dm_op_nr_vcpus *data = &op.u.nr_vcpus; diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h index dfc434ab41..dedc294ce9 100644 --- a/xen/arch/arm/include/asm/new_vgic.h +++ b/xen/arch/arm/include/asm/new_vgic.h @@ -275,6 +275,8 @@ int vgic_its_add_device(struct domain *d, struct vgic_its_device *its_dev); void vgic_its_delete_device(struct domain *d, struct vgic_its_device *its_dev); struct vgic_its_device* vgic_its_get_device(struct domain *d, paddr_t vdoorbell, uint32_t vdevid); +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, + u32 devid, u32 eventid); #else static inline void vgic_enable_lpis(struct vcpu *vcpu) { diff --git a/xen/arch/arm/vgic/vgic-its.c b/xen/arch/arm/vgic/vgic-its.c index fd5aaf4a70..706987d93a 100644 --- a/xen/arch/arm/vgic/vgic-its.c +++ b/xen/arch/arm/vgic/vgic-its.c @@ -608,7 +608,7 @@ int vgic_its_inject_cached_translation(struct domain *d, struct vgic_its *its, u * Returns 0 on success, a positive error value for any ITS mapping * related errors and negative error values for generic errors. */ -static int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, +int vgic_its_trigger_msi(struct domain *d, struct vgic_its *its, u32 devid, u32 eventid) { struct vgic_irq *irq = NULL; diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index fa98551914..a7d72e4389 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -444,6 +444,15 @@ struct xen_dm_op_nr_vcpus { }; typedef struct xen_dm_op_nr_vcpus xen_dm_op_nr_vcpus_t; +#define XEN_DMOP_arm_inject_msi 21 + +struct xen_dm_op_arm_inject_msi { + uint64_t addr; + uint32_t data; + uint32_t devid; +}; +typedef struct xen_dm_op_arm_inject_msi xen_dm_op_arm_inject_msi_t; + struct xen_dm_op { uint32_t op; uint32_t pad; @@ -463,6 +472,7 @@ struct xen_dm_op { xen_dm_op_set_mem_type_t set_mem_type; xen_dm_op_inject_event_t inject_event; xen_dm_op_inject_msi_t inject_msi; + xen_dm_op_arm_inject_msi_t arm_inject_msi; xen_dm_op_map_mem_type_to_ioreq_server_t map_mem_type_to_ioreq_server; xen_dm_op_remote_shutdown_t remote_shutdown; xen_dm_op_relocate_memory_t relocate_memory;
This patch adds the ability for the device emulator to inject MSI interrupts into guests. This is done by adding a new DM op to the device model library. It is not possible to reuse already existing inject_msi DM op, because it does not have a devid parameter, which is required for translation of MSIs to interrupt numbers on ARM. This approach was successfully tested on a virtio-pci setup with QEMU backend emulating block devices with following changes from the mainline Xen: This branch was taken as a base for virtio-pci: https://github.com/xen-troops/xen/tree/xen-4.18-xt0.2 With added new VGICv3 from here: https://github.com/Deedone/xen/tree/new_vgic_v2 (although it should also work with the current VGIC) And patches from this branch to allow for proper ITS emulation in guests: https://github.com/stewdk/xen/commits/pcie-passthrough-arm-vpci-v11 The main purpose of this RFC is to get some feedback on the addition of the new DM op. Is it the right approach or should we somehow modify the existing inject_msi DM op to be compatible with ARM? Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com> --- tools/include/xendevicemodel.h | 4 ++++ tools/libs/devicemodel/core.c | 19 +++++++++++++++++++ tools/libs/devicemodel/libxendevicemodel.map | 5 +++++ xen/arch/arm/dm.c | 15 +++++++++++++++ xen/arch/arm/include/asm/new_vgic.h | 2 ++ xen/arch/arm/vgic/vgic-its.c | 2 +- xen/include/public/hvm/dm_op.h | 10 ++++++++++ 7 files changed, 56 insertions(+), 1 deletion(-)