Re: [Xen-devel] [PATCH v4 2/4] x86/HVM: fix ID handling of x2APIC emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.09.14 at 12:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/09/14 16:19, Jan Beulich wrote:
>> For full context, here's the full intended function again:
>>
>> +static void lapic_load_fixup(struct vlapic *vlapic)
>> +{
>> +    uint32_t id = vlapic->loaded.id;
>> +
>> +    if ( vlapic_x2apic_mode(vlapic) && id && vlapic->loaded.ldr == 1 &&
>> +         /* Further checks are optional: ID != 0 contradicts LDR == 1. */
>> +         GET_xAPIC_ID(id) == vlapic_vcpu(vlapic)->vcpu_id * 2 &&
>> +         id == SET_xAPIC_ID(GET_xAPIC_ID(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);
>> +    }
>> +}
> 
> How about dropping the optional checks, as "id && vlapic->loaded.ldr ==
> 1" covers the broken hypervisor case?

I'd like to keep them for a while - after all that's why I added the
comment saying they're optional. The moment they start conflicting
with something else, they could be dropped.

The alternative would be to make them WARN_ON()s inside the if().

> The "id = vcpu_id * 2" is a broken assumption which I do need to fix as
> part of the cpuid infrastructure improvements, which would then break
> this check for a broken Xen.

For one it's not a broken assumption imo: The APIC ID gets set up
this way. And then I don't see why altering the APIC ID setting
would break this check here: If altering how the ID gets established
would get backported, I'd surely expect the change to the ID
handling here to also be.

> Furthermore, vlapic_x2apic_mode(vlapic) contradicts the use of
> {GET,SET}_xAPIC_ID().

So it does, but intentionally. Remember - we're checking whether some
fixup to what came in is necessary, and part of the brokenness was
that the ID was left set in a legacy APIC manner.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel