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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.09.14 at 16:30, <andrew.cooper3@xxxxxxxxxx> 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);
+    }
+}

Jan


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