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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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