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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2024 19:00, Alejandro Vallejo wrote:
> On 25/03/2024 16:45, Jan Beulich wrote:
>> On 09.01.2024 16:38, Alejandro Vallejo wrote:
>>> @@ -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.

Yes, and even in your reply you make the same assumption without further
explanation. It does not go without saying that vCPU 0 necessarily has
APIC ID 0. On bare metal that may be what one can typically observe, but
I'm unaware of any architectural guarantees to this effect.

>>> --- 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.
> 
> Huh. I was sure I zeroed that on vlapic_init(), but it must've been on a
> previous discarded series. Good catch.

No, explicit zeroing isn't needed, simply because all of struct vcpu
starts out zeroed.

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

It's really only this that I meant. Recall we now even have a dedicated
checking hook, running ahead of any state loading.

Jan