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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 24/09/14 12:41, Jan Beulich wrote:
>>>> On 24.09.14 at 12:42, <andrew.cooper3@citrix.com> 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().

Making them WARN_ON()s would make it more obvious if underlying
assumption/implementations change which subsequently invalidate the checks.

>
>> 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.

Sorry - it is one which is expected to change in future development work.

> 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.

This check gets applied equally to migrations from the same version of
Xen as to those from older versions.  In this case I suppose the "id &&
vlapic->loaded.ldr" will short circuit the vcpu_id*2 check, in makes it
ok (assuming no backports).

>
>> 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.

Right, which again due to short circuiting ok given that we have never
supported x2apic with ids greater than 255.

~Andrew