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

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

On 22/09/14 16:19, Jan Beulich wrote:
>>>> On 22.09.14 at 16:30, <andrew.cooper3@citrix.com> wrote:
>> On 18/09/14 15:44, Jan Beulich wrote:
>>> @@ -656,6 +650,8 @@ static int vlapic_reg_write(struct vcpu 
>>>      struct vlapic *vlapic = vcpu_vlapic(v);
>>>      int rc = X86EMUL_OKAY;
>>>  
>>> +    memset(&vlapic->loaded, 0, sizeof(vlapic->loaded));
>> This is slightly expensive to do for each register write.  Why does this
>> clobbering need to be here?  Could it not be....
> No, clearing this state can't be done in the load functions themselves
> (or else we wouldn't need the state in the first place. This, however,
> is more obvious with the follow-up addition I mailed in reply to the
> patch here, adding an "else" path to lapic_load_fixup(). That code
> wouldn't work as intended if we cleared the state early.
>
> If writing 12 bytes to memory would really turn out expensive, we
> may end up having to limit the clearing to certain register writes,
> but I was really instead considering to also do the clearing on
> register reads.
>
>>> +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);
>> ... in here, where we have positively identified whether fixup is needed
>> or not.
> 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?

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.

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

~Andrew