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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.10.2024 18:44, Alejandro Vallejo wrote:
> On Wed Oct 9, 2024 at 2:28 PM BST, Jan Beulich wrote:
>> 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 ...
> 
> Patch 10 does. It's the means by which (in a rather roundabout way)
> toolstack overrides vlapic->hw.x2apic_id.

Oh, indeed - I managed to not spot this. I think you want to either re-word
the description here to make clear there's actually a plan to do what is
being said as purely hypothetical, or simply fold the patches.

>>> 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.
> 
> I assume the "never really happens" is about the same as the previous
> paragraph? If so, the same answer applies.

Yes.

> About the lack of ordering in the migrate stream the code already makes no
> assumptions as to which HVM context blob might appear first in the vLAPIC 
> area.
> 
> I'm not sure why, but I assumed it may be different on older Xen.

I agree with being flexible here; I'm not aware of anything in the migration 
spec
(and certainly not in the unwritten v1 one) mandating particular ordering.

Jan