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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

Hi,

On 25/03/2024 16:45, Jan Beulich wrote:
> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -288,7 +288,10 @@ void update_guest_memory_policy(struct vcpu *v,
>>  static void cpu_policy_updated(struct vcpu *v)
>>  {
>>      if ( is_hvm_vcpu(v) )
>> +    {
>>          hvm_cpuid_policy_changed(v);
>> +        vlapic_cpu_policy_changed(v);
>> +    }
>>  }
> 
> This is a layering violation imo; hvm_cpuid_policy_changed() wants
> to call vlapic_cpu_policy_changed().

Sure.

> 
>> @@ -1083,6 +1083,22 @@ static void set_x2apic_id(struct vlapic *vlapic)
>>      vlapic_set_reg(vlapic, APIC_LDR, apic_ldr);
>>  }
>>  
>> +void vlapic_cpu_policy_changed(struct vcpu *v)
>> +{
>> +    struct vlapic *vlapic = vcpu_vlapic(v);
>> +    struct cpu_policy *cp = v->domain->arch.cpu_policy;
> 
> const please

Ack

> 
>> @@ -1514,6 +1530,13 @@ static void lapic_load_fixup(struct vlapic *vlapic)
>>      const struct vcpu *v = vlapic_vcpu(vlapic);
>>      uint32_t good_ldr = x2apic_ldr_from_id(vlapic->loaded.id);
>>  
>> +    /*
>> +     * Guest with hardcoded assumptions about x2apic_id <-> vcpu_id
>> +     * mappings. Recreate the mapping it used to have in old host.
>> +     */
>> +    if ( !vlapic->hw.x2apic_id )
>> +        vlapic->hw.x2apic_id = v->vcpu_id * 2;
> 
> This looks to depend upon it only ever being vCPU which may get a (new
> style) APIC ID of 0. I think such at least wants mentioning in the
> comment.

I don't quite follow you, I'm afraid. There is an implicit control flow
assumption that I can extract into a comment (I assume you were going
for that angle?). The implicit assumption that "vCPU0 always has
APIC_ID=0", which makes vCPU0 go through that path even when no
corrections are necessary. It's benign because it resolves to APIC_ID 0.

Is that what you meant? If so, I'll add it to v2.

> 
>> --- a/xen/include/public/arch-x86/hvm/save.h
>> +++ b/xen/include/public/arch-x86/hvm/save.h
>> @@ -394,6 +394,8 @@ struct hvm_hw_lapic {
>>      uint32_t             disabled; /* VLAPIC_xx_DISABLED */
>>      uint32_t             timer_divisor;
>>      uint64_t             tdt_msr;
>> +    uint32_t             x2apic_id;
>> +    uint32_t             rsvd_zero;
>>  };
> 
> I can't spot any checking of this last field indeed being zero.
> 
> Jan

Huh. I was sure I zeroed that on vlapic_init(), but it must've been on a
previous discarded series. Good catch.

Do we also want a check on migrate so a migration from a future Xen in
which it's not zero fails?

Cheers,
Alejandro