Re: [PATCH v4] xen/x86: On x2APIC mode, derive LDR from APIC ID

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

On 27/11/2023 12:20, Andrew Cooper wrote:
> On 27/11/2023 12:08 pm, Alejandro Vallejo wrote:
>> On 24/11/2023 22:05, Andrew Cooper wrote:
>>>> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
>>>> index 5cb87f8649..cd4701c5a2 100644
>>>> --- a/xen/arch/x86/hvm/vlapic.c
>>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>>> @@ -1061,13 +1061,26 @@ static const struct hvm_mmio_ops
>>>> vlapic_mmio_ops = {
>>>>        .write = vlapic_mmio_write,
>>>>    };
>>>>    +static uint32_t x2apic_ldr_from_id(uint32_t id)
>>>> +{
>>>> +    return ((id & ~0xf) << 12) | (1 << (id & 0xf));
>>>> +}
>>>> +
>>>>    static void set_x2apic_id(struct vlapic *vlapic)
>>>>    {
>>>
>>> const struct vcpu *v = vlapic_vcpu(vlapic);
>>>
>>> seeing as you've got the expression used twice already.
>>>
>>> With that, the following logic is shorter too; you can get away with
>>> dropping the vcpu_id variable as v->vcpu_id is the more common form to
>>> use in Xen.
>>
>> Twice? I can see a vague raison-d'etre in lapic_load_fixup(), but
>> there's a single occurence here.
> 
> It's hidden in the vlapic_domain(), which is vlacpi_vcpu()->domain.
> 
>>
>>>
>>>>    We must preserve LDRs so new vCPUs use consistent
>>>> +         * derivations and existing guests, which may have already
>>>> read the
>>>> +         * LDR at the source host, aren't surprised when interrupts
>>>> stop
>>>> +         * working the way they did at the other end.
>>>>             */
>>>> -        if ( GET_xAPIC_ID(id) != vlapic_vcpu(vlapic)->vcpu_id * 2 ||
>>>> -             id != SET_xAPIC_ID(GET_xAPIC_ID(id)) )
>>>> -            printk(XENLOG_G_WARNING "%pv: bogus APIC ID %#x loaded\n",
>>>> -                   vlapic_vcpu(vlapic), id);
>>>> -        set_x2apic_id(vlapic);
>>>> -    }
>>>> -    else /* Undo an eventual earlier fixup. */
>>>> -    {
>>>> -        vlapic_set_reg(vlapic, APIC_ID, id);
>>>> -        vlapic_set_reg(vlapic, APIC_LDR, vlapic->loaded.ldr);
>>>> -    }
>>>> +        vlapic_domain(vlapic)->arch.hvm.bug_x2apic_ldr_vcpu_id = true;
>>>> +    else
>>>> +        printk(XENLOG_G_WARNING
>>>> +               "%pv: bogus x2APIC loaded id=%#x ldr=%#x (expected
>>>> ldr=%#x)\n",
>>>
>>> %pv: bogus loaded x2APIC ID %#x, LDR %#x, expected LDR %#x\n
>>>
>>> If you properly capitalise x2APIC, you should capitalise ID and LDR.
>>> The other changes are matter of taste, but make for a less cluttered log
>>> message IMO.
>>>
>>
>> "bogus x2APIC loaded" is meant to be a sentence followed by key-value
>> pairs. Uppercasing the keys is fine (albeit unnecessary, IMO), but you
>> choice of word order feels backwards.
> 
> The grammar is awkward either way.
> 
> How about "bogus x2APIC record: "
> 
> ?
> 
> That is much clearer I think.
> 
> ~Andrew

LGTM.

Do you want me to send a v5 with it all?

Cheers,
Alejandro

Cheers,
Alejandro