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

[Ed White <edmund.h.white@intel.com>]

On 01/16/2015 09:50 AM, Tim Deegan wrote:
> 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?

Will do. Since the mask is already there and not needed for anything
else in the non-nested case, I thought it was useful in case there
was a future need to know which vcpu's were using a given alt p2m,
but there is no such need currently.

Ed