[v6] x86: Expose consistent topology to guests

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

Posted by Alejandro Vallejo 1 month, 3 weeks ago

If toolstack were to upload LAPIC contexts as part of domain creation it
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@cloud.com>
---
 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.
+         */
+        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);
 
-- 
2.46.0

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

Posted by Jan Beulich 1 month, 1 week ago

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@cloud.com>
> ---
>  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);
>

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

Posted by Alejandro Vallejo 1 month, 1 week ago

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.

>
> > 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@cloud.com>
> > ---
> >  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

I assume the "never really happens" is about the same as the previous
paragraph? If so, the same answer applies.

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.

>
> > +        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);
> >  

Cheers,
Alejandro

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

Posted by Jan Beulich 1 month, 1 week ago

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@cloud.com>
>>> ---
>>>  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