| Message ID | 20220216103026.11533-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: extended destination ID support | expand |
On 16/02/2022 10:30, Roger Pau Monne wrote: > Introduce a new arch specific field to report whether an emulator > supports the Extended Destination ID field, so that the hypervisor can > refrain from exposing the feature if one of the emulators doesn't > support it. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Changes since v1: > - New in this version. > --- > RFC: I find this kind of clumsy. In fact fully emulated devices > should already support Extended Destination ID without any > modifications, because XEN_DMOP_inject_msi gets passed the address and > data fields, so the hypervisor extracts the extended destination ID > from there. > > PCI passthrough devices however use xc_domain_update_msi_irq and that > has leaked the gflags parameter in the API, even worse the position > of the flags are hardcoded in QEMU. > > Should the clearing of ext_dest_id be limited to the domain using an > IOMMU? > > RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is > aware the domain must use ext_dest_id? (implies device model knows > APIC ID = CPU ID * 2) There is still only a single sync ioreq server page, so 128 vCPUs is the max possible. Paul
On Wed, Feb 16, 2022 at 10:53:58AM +0000, Durrant, Paul wrote: > On 16/02/2022 10:30, Roger Pau Monne wrote: > > Introduce a new arch specific field to report whether an emulator > > supports the Extended Destination ID field, so that the hypervisor can > > refrain from exposing the feature if one of the emulators doesn't > > support it. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > Changes since v1: > > - New in this version. > > --- > > RFC: I find this kind of clumsy. In fact fully emulated devices > > should already support Extended Destination ID without any > > modifications, because XEN_DMOP_inject_msi gets passed the address and > > data fields, so the hypervisor extracts the extended destination ID > > from there. > > > > PCI passthrough devices however use xc_domain_update_msi_irq and that > > has leaked the gflags parameter in the API, even worse the position > > of the flags are hardcoded in QEMU. > > > > Should the clearing of ext_dest_id be limited to the domain using an > > IOMMU? > > > > RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is > > aware the domain must use ext_dest_id? (implies device model knows > > APIC ID = CPU ID * 2) > > There is still only a single sync ioreq server page, so 128 vCPUs is the max > possible. Right - so device models wanting to support > 128 vCPUs will already need to be modified, and hence we could assume that any HVM guests with > 128 vCPUs is using a device model capable of handling extended destination ID? Thanks, Roger.
On 16/02/2022 11:32, Roger Pau Monné wrote: > On Wed, Feb 16, 2022 at 10:53:58AM +0000, Durrant, Paul wrote: >> On 16/02/2022 10:30, Roger Pau Monne wrote: >>> Introduce a new arch specific field to report whether an emulator >>> supports the Extended Destination ID field, so that the hypervisor can >>> refrain from exposing the feature if one of the emulators doesn't >>> support it. >>> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>> --- >>> Changes since v1: >>> - New in this version. >>> --- >>> RFC: I find this kind of clumsy. In fact fully emulated devices >>> should already support Extended Destination ID without any >>> modifications, because XEN_DMOP_inject_msi gets passed the address and >>> data fields, so the hypervisor extracts the extended destination ID >>> from there. >>> >>> PCI passthrough devices however use xc_domain_update_msi_irq and that >>> has leaked the gflags parameter in the API, even worse the position >>> of the flags are hardcoded in QEMU. >>> >>> Should the clearing of ext_dest_id be limited to the domain using an >>> IOMMU? >>> >>> RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is >>> aware the domain must use ext_dest_id? (implies device model knows >>> APIC ID = CPU ID * 2) >> >> There is still only a single sync ioreq server page, so 128 vCPUs is the max >> possible. > > Right - so device models wanting to support > 128 vCPUs will already > need to be modified, and hence we could assume that any HVM guests > with > 128 vCPUs is using a device model capable of handling extended > destination ID? > Yes, exactly. Paul
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 308650b400..7d56d022c8 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -185,6 +185,11 @@ void arch_ioreq_domain_init(struct domain *d) { } +void arch_ioreq_server_create(struct domain *d, int bufioreq_handling, + ioservid_t *id, unsigned int arch_flags) +{ +} + /* * Local variables: * mode: C diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c index 02ad9db565..3276f0360d 100644 --- a/xen/arch/x86/hvm/ioreq.c +++ b/xen/arch/x86/hvm/ioreq.c @@ -336,6 +336,13 @@ void arch_ioreq_domain_init(struct domain *d) register_portio_handler(d, 0xcf8, 4, hvm_access_cf8); } +void arch_ioreq_server_create(struct domain *d, int bufioreq_handling, + ioservid_t *id, unsigned int arch_flags) +{ + if ( !(arch_flags & X86_SUPPORTS_EXT_DEST_ID) ) + d->arch.ext_dest_id = false; +} + /* * Local variables: * mode: C diff --git a/xen/common/ioreq.c b/xen/common/ioreq.c index 689d256544..d4d5c653c7 100644 --- a/xen/common/ioreq.c +++ b/xen/common/ioreq.c @@ -636,7 +636,7 @@ static void ioreq_server_deinit(struct ioreq_server *s) } static int ioreq_server_create(struct domain *d, int bufioreq_handling, - ioservid_t *id) + ioservid_t *id, unsigned int arch_flags) { struct ioreq_server *s; unsigned int i; @@ -681,6 +681,8 @@ static int ioreq_server_create(struct domain *d, int bufioreq_handling, if ( id ) *id = i; + arch_ioreq_server_create(d, bufioreq_handling, id, arch_flags); + spin_unlock_recursive(&d->ioreq_server.lock); domain_unpause(d); @@ -1340,11 +1342,11 @@ int ioreq_server_dm_op(struct xen_dm_op *op, struct domain *d, bool *const_op) *const_op = false; rc = -EINVAL; - if ( data->pad[0] || data->pad[1] || data->pad[2] ) + if ( data->pad[0] || data->pad[1] ) break; rc = ioreq_server_create(d, data->handle_bufioreq, - &data->id); + &data->id, data->arch_flags); break; } diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index fa3f083fed..c6c575328b 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -67,7 +67,11 @@ typedef uint16_t ioservid_t; struct xen_dm_op_create_ioreq_server { /* IN - should server handle buffered ioreqs */ uint8_t handle_bufioreq; - uint8_t pad[3]; + +/* Signals Xen the emulator supports the Extended Destination ID field. */ +#define X86_SUPPORTS_EXT_DEST_ID (1u << 0) + uint8_t arch_flags; + uint8_t pad[2]; /* OUT - server id */ ioservid_t id; }; diff --git a/xen/include/xen/ioreq.h b/xen/include/xen/ioreq.h index a26614d331..f4566a1254 100644 --- a/xen/include/xen/ioreq.h +++ b/xen/include/xen/ioreq.h @@ -127,6 +127,8 @@ bool arch_ioreq_server_destroy_all(struct domain *d); bool arch_ioreq_server_get_type_addr(const struct domain *d, const ioreq_t *p, uint8_t *type, uint64_t *addr); void arch_ioreq_domain_init(struct domain *d); +void arch_ioreq_server_create(struct domain *d, int bufioreq_handling, + ioservid_t *id, unsigned int arch_flags); #endif /* __XEN_IOREQ_H__ */
Introduce a new arch specific field to report whether an emulator supports the Extended Destination ID field, so that the hypervisor can refrain from exposing the feature if one of the emulators doesn't support it. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- RFC: I find this kind of clumsy. In fact fully emulated devices should already support Extended Destination ID without any modifications, because XEN_DMOP_inject_msi gets passed the address and data fields, so the hypervisor extracts the extended destination ID from there. PCI passthrough devices however use xc_domain_update_msi_irq and that has leaked the gflags parameter in the API, even worse the position of the flags are hardcoded in QEMU. Should the clearing of ext_dest_id be limited to the domain using an IOMMU? RFC: Only enable ext_dest_id if max_cpu > 128? So the device model is aware the domain must use ext_dest_id? (implies device model knows APIC ID = CPU ID * 2) --- xen/arch/arm/ioreq.c | 5 +++++ xen/arch/x86/hvm/ioreq.c | 7 +++++++ xen/common/ioreq.c | 8 +++++--- xen/include/public/hvm/dm_op.h | 6 +++++- xen/include/xen/ioreq.h | 2 ++ 5 files changed, 24 insertions(+), 4 deletions(-)