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

[Tim Deegan <tim@xxxxxxx>]

At 13:39 +0100 on 03 Apr (1396528786), Jan Beulich wrote:
> >>> 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.

Right, yes, I was misreading patch 2; sorry for the noise.  And on the
grounds that the tristate is going away anyway,

Reviewed-by: Tim Deegan <tim@xxxxxxx>

for this patch as it stands.

Cheers,

Tim.

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