|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 8/9] hvm/ioreq: Negotiate extended destination ID support per ioreq server
On 8/19/26 16:38, Jan Beulich wrote:
> On 27.04.2026 15:54, Julian Vetter wrote:
>> ---
>> Changes in v4:
>> - As suggested by Roger, replaced XEN_DMOP_enable_ext_dest_id (v3 patch
>> 6), a separate DM op the device model had to call before starting
>> vCPUs, with a flags byte repurposed from the existing pad[3] field of
>> xen_dm_op_create_ioreq_server
>
> IOW the presence of XEN_DMOP_enable_ext_dest_id in the earlier patch is
> entirely stale?
>
>> - New XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID flag (bit 0) lets each ioreq
>> server signal support at registration time
>> - As suggested by Roger level the feature across all ioreq servers.
>> XEN_HVM_CPUID_EXT_DEST_ID is only advertised when every server
>> registered before arch_domain_creation_finished() sets the flag. A
>> single server without the flag suppresses the feature for the whole
>> domain!
>> - Lock the levelled result at domain creation time and enforce it for
>> servers registered afterwards, preventing a late opt-out from breaking
>> guests that already see the feature in CPUID
>> - Persist the locked flag via HVM_SAVE_TYPE(EXT_DEST_ID) so that live
>> migration preserves the guest-visible CPUID bit independently of when
>> the device model registers its ioreq servers on the destination host
>
> So a new save record for a single bit. That doesn't look very efficient
> to me.
>
>> @@ -1106,7 +1107,16 @@ int arch_domain_soft_reset(struct domain *d)
>> void arch_domain_creation_finished(struct domain *d)
>> {
>> if ( is_hvm_domain(d) )
>> + {
>> + /*
>> + * Lock the extended destination ID state. OR preserves any value
>> + * already restored from an HVM save record (migration path). For a
>> + * fresh domain, ext_dest_id starts false and the dynamic check
>> + * supplies the levelled result across all registered ioreq servers.
>> + */
>> + d->arch.hvm.ext_dest_id |= hvm_ext_dest_id_enabled(d);
>
> For an unaware guest, after migration it'll suddenly get the flag set
> if all servers are capable. That can't be right. It looks pretty much
> unavoidable for the field to become tristate (unset / false / true).
Thank you Jan. You're right true/false is not enough, but there's one
migration case left where even a tristate doesn't give a clear answer, I
believe and I'd like your opinion on that.
Scenario: a domain is migrated (or saved/restored) from a Xen that
predates this series, onto a new Xen where every registered ioreq server
has XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID set.
So, the incoming stream would not carry a EXT_DEST_ID record, so
ext_dest_id_load() never runs and the field would still be
EXT_DEST_ID_UNSET when arch_domain_creation_finished() runs on the
destination. The latch then takes the "fresh domain" path and recomputes
the levelled value, which here comes out ENABLED.
From that point Xen would interpret the extended destination ID bits
for this guest. But before the guest ran under a Xen that never
advertised XEN_HVM_CPUID_EXT_DEST_ID, so it never used those reserved
bits deliberately, but might have written garbage into them accidentaly.
I see two ways to handle this:
1. Accept it. Document that migrating in from a pre-feature Xen onto an
all-opted-in host may turn the feature on, and might now treat non-zero
reserved bits as extended destionation ID bits.
2. Distinguish "fresh domain" from "restored without the record" and
force the latter to DISABLED. A feature-aware guest then picks the
feature up on its next reboot on the new host, which matches how every
other creation-time-levelled property behaves.
The stream is parsed by Xen (the toolstack hands the HVM-context blob to
XEN_DOMCTL_sethvmcontext -> hvm_load()), so this stays entirely in the
hypervisor: add a 'bool context_loaded' to 'struct hvm_domain', set it
in the hvm_load(), and in the latch do
if ( d->arch.hvm.ext_dest_id == EXT_DEST_ID_UNSET )
d->arch.hvm.ext_dest_id =
(!d->arch.hvm.context_loaded && hvm_ext_dest_id_enabled(d))
? EXT_DEST_ID_ENABLED : EXT_DEST_ID_DISABLED;
This would mean one new bool in 'struct hvm_domain' which covers both
live migration and xl restore of an old image. What do you think? Would
this be acceptable?
>
>> hvm_domain_creation_finished(d);
>
> Blank line please between what you add and what was already there.
>
>> @@ -325,6 +326,42 @@ void arch_ioreq_domain_init(struct domain *d)
>> register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
>> }
>>
>> +int arch_ioreq_server_create_check(const struct domain *d, uint8_t flags)
>
> Bogus use of a fixed-width type again.
>
>> +{
>> + if ( !is_hvm_domain(d) || !d->creation_finished )
>> + return 0;
>
> Why the HVM check? ioreq_server_dm_op(), the sole caller, will only ever
> be called for HVM domains (as per the check near the top of dm_op()).
>
>> + if ( d->arch.hvm.ext_dest_id &&
>> + !(flags & XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) )
>> + return -EPERM;
>> +
>> + return 0;
>> +}
>> +
>> +static int cf_check ext_dest_id_save(struct vcpu *v, hvm_domain_context_t
>> *h)
>> +{
>> + struct hvm_hw_ext_dest_id s = {
>> + .enabled = v->domain->arch.hvm.ext_dest_id,
>> + };
>> +
>> + return hvm_save_entry(EXT_DEST_ID, 0, h, &s);
>> +}
>> +
>> +static int cf_check ext_dest_id_load(struct domain *d, hvm_domain_context_t
>> *h)
>> +{
>> + struct hvm_hw_ext_dest_id s;
>> +
>> + if ( hvm_load_entry(EXT_DEST_ID, h, &s) )
>> + return -EINVAL;
>> +
>> + d->arch.hvm.ext_dest_id = s.enabled;
>
> Afaict this can load arbitrary values other than 0 or 1. In fact ...
>
>> + return 0;
>> +}
>> +
>> +HVM_REGISTER_SAVE_RESTORE(EXT_DEST_ID, ext_dest_id_save, NULL,
>> + ext_dest_id_load, 1, HVMSR_PER_DOM);
>
> ... I think there's ext_dest_id_check() missing.
>
>> --- a/xen/arch/x86/hvm/vioapic.c
>> +++ b/xen/arch/x86/hvm/vioapic.c
>> @@ -24,6 +24,7 @@
>> * Ported to xen by using virtual IRQ line.
>> */
>>
>> +#include <xen/ioreq.h>
>> #include <xen/types.h>
>> #include <xen/mm.h>
>> #include <xen/xmalloc.h>
>
> Is this hunk stale?
>
>> @@ -597,6 +598,7 @@ int vioapic_get_trigger_mode(const struct domain *d,
>> unsigned int gsi)
>> static int cf_check ioapic_check(const struct domain *d,
>> hvm_domain_context_t *h)
>> {
>> const HVM_SAVE_TYPE(IOAPIC) *s;
>> + unsigned int i;
>
> Better ...
>
>> @@ -617,6 +619,24 @@ static int cf_check ioapic_check(const struct domain
>> *d, hvm_domain_context_t *h
>> if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) *
>> 2 + 1 )
>> return -EINVAL;
>>
>> + /*
>> + * If any RTE uses extended destination ID bits, the EXT_DEST_ID save
>> + * record must have been loaded first (restoring
>> d->arch.hvm.ext_dest_id).
>> + * The ioreq server re-registration by the DM happens later, so use the
>> + * domain-level locked flag rather than the per-server dynamic check.
>> + */
>> + for ( i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
>
> ...
>
> for ( unsigned int i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
>
>> + {
>> + if ( s->redirtbl[i].fields.ext_dest_id && !d->arch.hvm.ext_dest_id )
>> + {
>> + printk(XENLOG_G_ERR "HVM restore: %pd IO-APIC RTE %u has "
>> + "extended destination ID bits set but "
>> + "EXT_DEST_ID is not enabled\n",
>> + d, i);
>
> No, this is the wrong way round. As long as we permit guests to put
> non-zero in these bits, we can't demand the bits to be zero here. You
> need to avoid interpreting them as extended-ID when the feature isn't
> enabled for a guest.
>
>> --- a/xen/common/ioreq.c
>> +++ b/xen/common/ioreq.c
>> @@ -641,7 +641,7 @@ static void ioreq_server_deinit(struct ioreq_server *s)
>> }
>>
>> static int ioreq_server_create(struct domain *d, int bufioreq_handling,
>> - ioservid_t *id)
>> + uint8_t flags, ioservid_t *id)
>
> Inappropriate use of a fixed-width type again.
>
>> @@ -1350,11 +1352,16 @@ 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->flags & ~XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID ||
>
> Parentheses please around bitwise logic being operands to boolean logic.
>
>> + data->pad[0] || data->pad[1] )
>> + break;
>> +
>> + rc = arch_ioreq_server_create_check(d, data->flags);
>
> It's a little odd to have an arch hook here, yet at the same time an
> x86-specific check a few lines up (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID
> really is meaningless on non-x86, and should hence either be constrained
> to x86 [with the bit position reusable for something else on other
> architectures], or be properly rejected on non-x86).
>
>> @@ -455,6 +456,18 @@ int pt_irq_create_bind(
>> uint64_t msi_addr;
>> uint32_t msi_data;
>>
>> + /*
>> + * Refuse the old MSI bind path when extended destination IDs are
>> + * in use. The caller must use XEN_DMOP_bind_pt_msi_irq instead,
>> + * which passes the raw MSI address so Xen can decode the extended
>> + * bits. This old path only carries an 8-bit destination ID and
>> + * would silently misroute interrupts to vCPUs with APIC IDs > 255.
>
> "to" looks ambiguous to me here. Maybe better "targeted at"? It's also >= 255,
> I think.
>
>> + */
>> + if ( hvm_ext_dest_id_enabled(d) )
>> + {
>> + return -EPERM;
>> + }
>
> No need for curly braces here. Further I think -EPERM isn't a good choice, as
> that's what xsm_default_action() returns. -EOPNOTSUPP may be an option, or
> some other, more "exotic" indicator.
>
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -627,12 +627,27 @@ struct hvm_msr {
>>
>> #define CPU_MSR_CODE 20
>>
>> +/*
>> + * HVM_SAVE_TYPE(EXT_DEST_ID): domain-level extended MSI destination ID
>> state.
>
> Why MSI when the vIO-APIC uses it as well?
>
>> + * Records whether the extended destination ID feature was enabled for this
>> + * domain at the time guest vCPUs were started. This allows migration to
>> + * preserve the setting across hosts without relying on the device model to
>> + * re-register its ioreq servers before the guest's first CPUID query.
>> + */
>
> The guest's first CPUID query surely is going to happen after at least one DM
> has registered a server? (For HVM, that is. No server may ever be registered
> for PVH, aiui.) It's not quite clear to me why this connection to CPUID
> queries is being made here. The flag is necessary at server registration time,
> as ones not supporting the feature need to be rejected.
>
>> --- a/xen/include/public/hvm/dm_op.h
>> +++ b/xen/include/public/hvm/dm_op.h
>> @@ -39,18 +39,28 @@ typedef uint16_t ioservid_t;
>> * XEN_DMOP_create_ioreq_server: Instantiate a new IOREQ Server for a
>> * secondary emulator.
>> *
>> - * The <id> handed back is unique for target domain. The valur of
>> + * The <id> handed back is unique for target domain. The value of
>> * <handle_bufioreq> should be one of HVM_IOREQSRV_BUFIOREQ_* defined in
>> - * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
>> + * hvm_op.h. If the value is HVM_IOREQSRV_BUFIOREQ_OFF then the buffered
>> * ioreq ring will not be allocated and hence all emulation requests to
>> * this server will be synchronous.
>> + *
>> + * If <flags> contains XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID, the server will
>> + * use XEN_DMOP_bind_pt_msi_irq for all passthrough MSI bindings, passing
>> + * raw MSI address/data fields so Xen can decode extended destination ID
>> + * bits. Once any server sets this flag, Xen will advertise
>> + * XEN_HVM_CPUID_EXT_DEST_ID to the guest. Must be set before the guest
>> + * vCPUs are started.
>
> I don't understand the last sentence. Is it perhaps stale from how things
> were earlier? There's nothing to "set" here.
>
>> --- a/xen/include/xen/ioreq.h
>> +++ b/xen/include/xen/ioreq.h
>> @@ -54,9 +54,35 @@ struct ioreq_server {
>> evtchn_port_t bufioreq_evtchn;
>> struct rangeset *range[NR_IO_RANGE_TYPES];
>> bool enabled;
>> + bool ext_dest_id;
>> uint8_t bufioreq_handling;
>> };
>>
>> +/*
>> + * Return true if every registered ioreq server has opted in to extended
>> + * destination IDs (XEN_DMOP_IOREQ_SERVER_EXT_DEST_ID) and at least one
>> + * server exists.
>
> Why is one server existing relevant?
>
>> A single server without the flag is enough to suppress
>> + * XEN_HVM_CPUID_EXT_DEST_ID, preventing misrouted interrupts.
>> + */
>> +static inline bool hvm_ext_dest_id_enabled(const struct domain *d)
>> +{
>> + unsigned int i;
>> + bool found = false;
>> +
>> + for ( i = 0; i < MAX_NR_IOREQ_SERVERS; i++ )
>
> Please use ARRAY_SIZE() in such cases.
>
>> + {
>> + const struct ioreq_server *s = d->ioreq_server.server[i];
>> +
>> + if ( !s )
>> + continue;
>> + if ( !s->ext_dest_id )
>
> As there's no locking here, and as the comment ahead of the function also
> doesn't mention any locking requirements: What guarantees s to still be
> valid to deref here? Furthermore, what guarantees the result of this
> function to not be stale by the time the caller looks at it? (Some of
> this may be easier if this wasn't an inline function in a globally
> visible header.)
>
> Jan
>
--
Julian Vetter | Vates Hypervisor & Kernel Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |