Re: [Xen-devel] [PATCH 06/11] VMX/altp2m: add code to support EPTP switching and #VE.

[Tim Deegan <tim@xxxxxxx>]

At 10:55 -0800 on 15 Jan (1421315724), Ed White wrote:
> On 01/15/2015 08:56 AM, Tim Deegan wrote:
> > Hi,
> > 
> > At 13:26 -0800 on 09 Jan (1420806396), Ed White wrote:
> >> @@ -2551,6 +2640,17 @@ static void vmx_vmexit_ud_intercept(struct 
> >> cpu_user_regs *regs)
> >>          hvm_inject_hw_exception(TRAP_invalid_op, 
> >> HVM_DELIVER_NO_ERROR_CODE);
> >>          break;
> >>      case X86EMUL_EXCEPTION:
> >> +        /* check for a VMFUNC that should be emulated */
> >> +        if ( !cpu_has_vmx_vmfunc && altp2mhvm_active(current->domain) &&
> >> +             ctxt.insn_buf_bytes >= 3 && ctxt.insn_buf[0] == 0x0f &&
> >> +             ctxt.insn_buf[1] == 0x01 && ctxt.insn_buf[2] == 0xd4 &&
> >> +             regs->eax == 0 &&
> >> +             p2m_switch_vcpu_altp2m_by_id(current, (uint16_t)regs->ecx) )
> >> +        {
> >> +            regs->eip += 3;
> >> +            return;
> >> +        }
> >> +
> > 
> > I think Andrew already pointed out that this needs to be done by
> > adding VMFUNC to the emulator itself with a callback.  Apart from
> > anything else that will DTRT with prefix bytes &c.
> > 
> >> +        if ( (uint16_t)idx != vcpu_altp2mhvm(v).p2midx )
> >> +        {
> >> +            cpumask_clear_cpu(v->vcpu_id, 
> >> p2m_get_altp2m(v)->dirty_cpumask);
> >> +            vcpu_altp2mhvm(v).p2midx = (uint16_t)idx;
> >> +            cpumask_set_cpu(v->vcpu_id, p2m_get_altp2m(v)->dirty_cpumask);
> > 
> > This looks wrong -- you need to do a TLB flush before you can remove
> > this CPU from the dirty_cpumask.
> > 
> 
> No, the whole point of multiple EPTP's is that you can switch between them
> without a flush. The EPTP is part of the TLB tag, and you want that entry
> to stay in the TLB because you're probably going to switch back and use
> it again.

That's actually what I was worried about...

> If you tear the whole table down you need a flush, but I think the
> existing EPT code handles that.  I only use the mask to make sure I
> don't tear down a table that is the current table for a vcpu.

and this is why I was confused.  The meaning of 'dirty_cpumask' in Xen
generally is 'all CPUs that might hold state derived from this',
i.e. all the CPUs you'd have to IPI if you wanted to be sure that a
mapping you removed from this table wasn't still cached.  IOW, this
could be used to mask down flush IPIs when p2m updates happen to this
table.

Looking at the code, the current (non-nested) HAP code uses the
_domain_'s dirty_cpumask for all flushes, so for altp2m this field is
not needed.

I'm not comfortable with it being reused for something
almost-but-not-quite like the usual semantics, though.  Can you please
use a simple counter for this instead?

Cheers,

Tim.

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