Re: [Xen-devel] [PATCH v2 1/4] x86/EPT: force re-evaluation of memory type as necessary

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.14 at 14:30, <tim@xxxxxxx> wrote:
> At 12:24 +0100 on 03 Apr (1396524290), Jan Beulich wrote:
>> >>> On 03.04.14 at 12:18, <tim@xxxxxxx> wrote:
>> > Hi, 
>> > 
>> > This version looks correct to me, but this tristate: 
>> > 
>> > At 13:17 +0000 on 28 Mar (1396009028), Jan Beulich wrote:
>> >> +    p2m_lock(p2m);
>> >> +
>> >> +    okay = -curr->arch.hvm_vmx.ept_spurious_misconfig;
>> > 
>> > [...]
>> > 
>> >> +    if ( okay > 0 )
>> >> +    {
>> >> +        struct vcpu *v;
>> >> +
>> >> +        for_each_vcpu ( curr->domain, v )
>> >> +            v->arch.hvm_vmx.ept_spurious_misconfig = 1;
>> >> +    }
>> >> +    curr->arch.hvm_vmx.ept_spurious_misconfig = 0;
>> >> +    ept_sync_domain(p2m);
>> >> +    p2m_unlock(p2m);
>> >> +
>> >> +    return !!okay;
>> > 
>> > seems a bit baroque.  Can we go for 'bool_t okay = 0' at the top and then
>> > 
>> >     if ( okay )
>> >     {
>> >         struct vcpu *v;
>> > 
>> >         for_each_vcpu ( curr->domain, v )
>> >             v->arch.hvm_vmx.ept_spurious_misconfig = 1;
>> >     }
>> >     else
>> >         okay = curr->arch.hvm_vmx.ept_spurious_misconfig; 
>> >     curr->arch.hvm_vmx.ept_spurious_misconfig = 0;
>> >     ept_sync_domain(p2m);
>> >     p2m_unlock(p2m);
>> > 
>> >     return okay;
>> > 
>> > instead?
>> 
>> Actually, trying this I find that this conflicts with the added use in
>> patch 2, so I'd prefer to keep it the way it is.
> 
> Oh.  On closer inspection, in patch 2, when you split a superpage you
> set okay = 0 and break, which should cause this whole function to
> return 0, killing the guest.  Shouldn't that be a 'continue'?

But that's only when the splitting fails, and in that case I have no
choice but killing the guest.

Jan

> And I'm not sure that setting okay = 0 is the right thing to do
> anyway.  I guess what you're looking for is to avoid a false negative
> if you split the superpage and then don't clear an invalid EMT in the
> new leaf; but you might have cleared an invalid EMT at a higher
> level, in which case okay should not be touched; 'continue' will DTRT.
> 
> Or maybe I'm confused.  Time for lunch and I'll stare at it again. :)
> 
> Tim.




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