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

[Tim Deegan <tim@xxxxxxx>]

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.

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

> I.e. if I
> was to go that route, we should then first formally (even if just
> verbally) tighten the requirements on these context load operations.

I have no objection to that; I think a comment in save.h would
suffice.

Tim.

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