Re: [PATCH v3 2/6] xen/x86: Add initial x2APIC ID to the per-vLAPIC save area

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 30/05/2024 12:08, Andrew Cooper wrote:
> On 29/05/2024 3:32 pm, Alejandro Vallejo wrote:
>> diff --git a/xen/lib/x86/policy.c b/xen/lib/x86/policy.c
>> index f033d22785be..b70b22d55fcf 100644
>> --- a/xen/lib/x86/policy.c
>> +++ b/xen/lib/x86/policy.c
>> @@ -2,6 +2,17 @@
>>  
>>  #include <xen/lib/x86/cpu-policy.h>
>>  
>> +uint32_t x86_x2apic_id_from_vcpu_id(const struct cpu_policy *p, uint32_t id)
>> +{
>> +    /*
>> +     * TODO: Derive x2APIC ID from the topology information inside `p`
>> +     *       rather than from the vCPU ID alone. This bodge is a temporary
>> +     *       measure until all infra is in place to retrieve or derive the
>> +     *       initial x2APIC ID from migrated domains.
>> +     */
>> +    return id * 2;
>> +}
>> +
> 
> I'm afraid it's nonsensical to try and derive x2APIC ID from a
> policy+vcpu_id.

That's debatable, and we clearly have different views, however...

> 
> Take a step back, and think the data through.
> 
> A VM has:
> * A unique APIC_ID for each vCPU
> * Info in CPUID describing how to decompose the APIC_ID into topology
> 
> Right now, because this is all completely broken, we have:
> * Hardcoded APIC_ID = vCPU_ID * 2
> * Total nonsense in CPUID
> 
> 
> When constructing a VM, the toolstack (given suitable admin
> guidance/defaults) *must* choose both:
> * The APIC_ID themselves
> * The CPUID topo data to match
> 
> i.e. this series should be editing the toolstack's call to
> xc_domain_hvm_setcontext().
> 
> It's not, because AFAICT you're depending on the migration compatibility
> logic and inserting a new hardcoded assumption about symmetry of the layout.
> 
> 
> The data flows we need are:
> 
> (New) create:
> * Toolstack chooses both parts of topo information
> * Xen needs a default, which reasonably can be APIC_ID=vCPU_ID when the
> rest of the data flow has been cleaned up.  But this is needs to be
> explicit in vcpu_create() and without reference to the policy.
> 
> And to be clear, it's fine for now for the toolstack to choose a
> symmetric layout and pick appropriate APIC_IDs+CPUID for this, but it
> needs to be the toolstack making this decision, not Xen inventing state
> out of thin air based on the toolstack only giving half the information.
> 
> (New) migrate:
> * Data from the stream, exactly as presented
> 
> (Compat) migrate:
> * Synthesize the missing xapic_id field in LAPIC_REGs as APIC_ID=vCPU_ID
> * 2.
> 
> I'm pretty sure this will be a net reduction in complexity in this
> series.  It definitely reduces the Xen complexity.
> 
> ~Andrew

... I didn't know toolstack could send hvmcontexts during non-migrated
domain creation. That's neat!

I was going to defend my approach (because it does make sense), but
there's an extra benefit from yours you didn't seem to notice. With the
x2apicid in the migration stream (patches 1 and parts of 2) it's not
only possible to set the APIC ID from toolstack per vCPU with the
contexts, but it would also allow toolstack to be responsible to
preinitialize all the APICs in x2apic mode when any of them is 255 or more.

I'll try to do that soon-ish. I suspect the pain points are going to be
making it work nicely as well on 1vCPU systems with no APIC (are those
expected to work?).

I'm not looking forward to re-testing all of this again...

Cheers,
Alejandro