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
