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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.09.14 at 14:59, <tim@xxxxxxx> wrote:
> At 13:20 +0100 on 18 Sep (1411042805), Jan Beulich wrote:
>> >>> On 18.09.14 at 12:53, <tim@xxxxxxx> wrote:
>> > At 08:57 +0100 on 12 Sep (1410508628), Jan Beulich wrote:
>> >> >> @@ -1254,6 +1255,29 @@ HVM_REGISTER_SAVE_RESTORE(LAPIC, lapic_s
>> >> >>  HVM_REGISTER_SAVE_RESTORE(LAPIC_REGS, lapic_save_regs, 
>> >> >> lapic_load_regs,
>> >> >>                            1, HVMSR_PER_VCPU);
>> >> >>  
>> >> >> +void vlapic_domain_unpause(const struct domain *d)
>> >> >> +{
>> >> >> +    /*
>> >> >> +     * Following lapic_load_hidden()/lapic_load_regs() we may need to
>> >> >> +     * correct ID and LDR when they come from an old, broken 
>> >> >> hypervisor.
>> >> >> +     */
>> >> > 
>> >> > This seems like aweful overhead for the domain_{,un}pause() path.
>> >> 
>> >> It's the best I could think of (and domain un-pausing shouldn't be that
>> >> frequent an operation I would hope), since ...
>> >> 
>> >> >  Why can't it be fixed up once in lapic_load_{regs,hidden}(),
>> >> 
>> >> ... both of these would have to have got carried out and ...
>> > 
>> > You're already tracking whether either of them happens so you could
>> > track whether both have happened and do the fixup then.  Loading a VM
>> > with only the hidden state and not the register state is not something
>> > we care about (and in that case, by construction, LDR won't be 1).
>> 
>> I had considered that, but dropped the idea because the two pieces
>> are independent, and hence I don't think we should (implicitly) disallow
>> it.
> 
> We wouldn't be disallowing it; we'd just fail to magically fix vlapic
> state for that one case.  And since the fix assumes that it's dealing
> with a save record from an older Xen I think that's OK.  After all,
> the fix also implicitly disallows deliberately setting up a vcpu that
> looks like an old-fashioned HVM vcpu.
> 
> It would, I think, be reasonable to abandon the fix altogether and
> just let upgraded VMs continue with their bogus state -- after all it
> can't be that much of a problem for them or they'd have failed on the
> old box.

No, we can't drop it, because it doesn't fix only LDR. It also changes
the ID representation (since in particular the GET_xAPIC_ID() use
gets dropped from hvm_x2apic_msr_read()'s APIC_ID case), and this
one must be transparent to the guest.

>> Nor do I think we should (implicitly) disallow restoring the same
>> state more than once before resuming the guest (or vCPU).
> 
> True, but that's true whether we do the fix at unpause or beforehand.

No, it's not: If the first load resulted in x2APIC mode and the
second one not, we'd have done an adjustment already which we
would better not have done.

But I think I can see another alternative: Latch the most recently
loaded or live ID and LDR values into struct vlapic and restore
them into register state when hidden state got updated, applying
the fix immediately afterwards. Any write to those two registers
(or perhaps just any live register access) would then clear the
"loaded" state.

Jan


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