Re: [PATCH v6 04/11] xen/x86: Add supporting code for uploading LAPIC contexts during domain create

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2024 14:38, Alejandro Vallejo wrote:
> If toolstack were to upload LAPIC contexts as part of domain creation it

If it were to - yes. But it doesn't, an peeking ahead in the series I also
couldn't spot this changing. Hence I don#t think I see why this change
would be needed, and why ...

> would encounter a problem were the architectural state does not reflect
> the APIC ID in the hidden state. This patch ensures updates to the
> hidden state trigger an update in the architectural registers so the
> APIC ID in both is consistent.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/vlapic.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 02570f9dd63a..a8183c3023da 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -1640,7 +1640,27 @@ static int cf_check lapic_load_hidden(struct domain 
> *d, hvm_domain_context_t *h)
>  
>      s->loaded.hw = 1;
>      if ( s->loaded.regs )
> +    {
> +        /*
> +         * We already processed architectural regs in lapic_load_regs(), so
> +         * this must be a migration. Fix up inconsistencies from any older 
> Xen.
> +         */
>          lapic_load_fixup(s);
> +    }
> +    else
> +    {
> +        /*
> +         * We haven't seen architectural regs so this could be a migration 
> or a
> +         * plain domain create. In the domain create case it's fine to modify
> +         * the architectural state to align it to the APIC ID that was just
> +         * uploaded and in the migrate case it doesn't matter because the
> +         * architectural state will be replaced by the LAPIC_REGS ctx later 
> on.
> +         */

... a comment would need to mention a case that never really happens, thus
only risking to cause confusion.

Jan

> +        if ( vlapic_x2apic_mode(s) )
> +            set_x2apic_id(s);
> +        else
> +            vlapic_set_reg(s, APIC_ID, SET_xAPIC_ID(s->hw.x2apic_id));
> +    }
>  
>      hvm_update_vlapic_mode(v);
>