Re: [PATCH v3 1/7] x86/vioapic: Add ioapic_check() to validate IO-APIC state before restore

[Jan Beulich <jbeulich@xxxxxxxx>]

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