On 09.03.2026 13:31, Julian Vetter wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -594,6 +594,35 @@ int vioapic_get_trigger_mode(const struct domain *d,
> unsigned int gsi)
> return vioapic->redirtbl[pin].fields.trig_mode;
> }
>
> +static int cf_check ioapic_check(const struct domain *d,
> hvm_domain_context_t *h)
> +{
> + const HVM_SAVE_TYPE(IOAPIC) *s;
> + unsigned int i;
> +
> + if ( !has_vioapic(d) )
> + return -ENODEV;
> +
> + s = hvm_get_entry(IOAPIC, h);
> + if ( !s )
> + return -ENODATA;
> +
> + for ( i = 0; i < ARRAY_SIZE(s->redirtbl); i++ )
> + {
> + const union vioapic_redir_entry *e = &s->redirtbl[i];
> +
> + /*
> + * Check to-be-loaded values are within valid range, for them to
> + * represent actually reachable state.
> + */
> + if ( e->fields.reserve ||
> + e->fields.reserved[0] || e->fields.reserved[1] ||
> + e->fields.reserved[2] || e->fields.reserved[3] )
> + return -EINVAL;
Are comment and code actually in sync? I can't spot anything preventing the
reserved fields to be set by a guest. Such setting would simply be ignored.
(And this is actually why I was asking you to add such a function: By adding
the checks you should have noticed that the fields can be non-zero if a guest
writes them this way. Which in turn may pose a problem for your extid
intentions.)
> + }
> +
> + return 0;
> +}
If it wasn't for the above, something like this may do for starters. Would
be nice if base_address, ioregsel, and id also had some sanity checking
applied.
However, does this build at all with the function unused? You lack ...
> static int cf_check ioapic_save(struct vcpu *v, hvm_domain_context_t *h)
> {
> const struct domain *d = v->domain;
... a hunk altering the HVM_REGISTER_SAVE_RESTORE() further down.
Jan
