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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 27/04/2026 à 15:57, Julian Vetter a écrit :
> Register a check callback for the IOAPIC HVM save/restore entry,
> following the pattern established by vpic_check() for the virtual PIC.
> The function first verifies the target domain actually has a virtual
> IO-APIC, returning -ENODEV otherwise. It then validates individual
> fields of the saved state: the base_address must be non-zero (as 0 is
> never valid for the IO-APIC MMIO window), the APIC ID must fit within
> its 4-bit hardware field, and ioregsel must address a defined register.
> 
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v4:
> - Replaced the reserved-bit loop from v3 (iterating all redirection
>    table entries and rejecting any with non-zero reserved fields) with
>    targeted named-field validation (base_address != 0, APIC ID < 0xF, and
>    ioregsel addresses a defined register)
> - The extended-destination migration safety check (refusing to restore
>    IO-APIC state with ext_dest_id bits set on a domain that does not
>    advertise XEN_HVM_CPUID_EXT_DEST_ID) is added in patch 8, once the
>    flag exists
> ---
>   xen/arch/x86/hvm/vioapic.c | 28 +++++++++++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> index 7c725f9e47..43fb165f84 100644
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -594,6 +594,32 @@ 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;
> +
> +    if ( !has_vioapic(d) )
> +        return -ENODEV;
> +
> +    s = hvm_get_entry(IOAPIC, h);
> +    if ( !s )
> +        return -ENODATA;
> +
> +    /* base_address of 0 is never valid for the IO-APIC MMIO window. */
> +    if ( !s->base_address )
> +        return -EINVAL;
> +
> +    /* IO-APIC APIC ID is a 4-bit field. */
> +    if ( s->id > 0xf )
> +        return -EINVAL;
> +
> +    /* ioregsel must address a defined register. */
> +    if ( s->ioregsel > VIOAPIC_REG_RTE0 + (ARRAY_SIZE(s->redirtbl) - 1) * 2 
> + 1 )

you can rewrite it as

s->ioregsel >= VIOAPIC_REG_RTE0 + ARRAY_SIZE(s->redirtbl) * 2

> +        return -EINVAL;
> +
> +    return 0;
> +}
> +
>   static int cf_check ioapic_save(struct vcpu *v, hvm_domain_context_t *h)
>   {
>       const struct domain *d = v->domain;
> @@ -630,7 +656,7 @@ static int cf_check ioapic_load(struct domain *d, 
> hvm_domain_context_t *h)
>       return 0;
>   }
>   
> -HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, NULL, ioapic_load, 1,
> +HVM_REGISTER_SAVE_RESTORE(IOAPIC, ioapic_save, ioapic_check, ioapic_load, 1,
>                             HVMSR_PER_DOM);

You think you can relax some of the checks in ioapic_load as you moved 
some of them in ioapic_check.

>   
>   void vioapic_reset(struct domain *d)

Teddy


--
 | Vates

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech