Re: [Xen-devel] [PATCH V3] x86/altp2m: Fix crash with INVALID_ALTP2M EPTP index

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.06.18 at 12:18, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 06/27/2018 12:46 PM, Jan Beulich wrote:
>>>>> On 26.06.18 at 16:21, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> When SECONDARY_EXEC_ENABLE_VIRT_EXCEPTIONS is set,
>>> vmx_vcpu_update_eptp() __vmwrites() EPTP_INDEX in
>>> altp2m_vcpu_destroy(). This means that when disabling altp2m on a
>>> domain after xc_altp2m_set_vcpu_enable_notify() has been
>>> successfully called, EPTP_INDEX ends up being stored as
>>> INVALID_ALTP2M. This makes it possible for vmx_vmexit_handler()
>>> to __vmread() the stale value after a subsequent call to
>>> xc_altp2m_set_vcpu_enable_notify(), and BUG_ON(idx >= MAX_ALTP2M).
>> 
>> I'm fine with the code change now, but I think this 3rd approach
>> of addressing the issue needs the description to be changed.
>> Already on v2 it wouldn't have become clear to me what the
>> issue was from just reading the description. In particular you now
>> want to point out why the change is correct / necessary also for
>> the other invocation of altp2m_vcpu_update_vmfunc_ve(). It
>> would also be helpful to have a statement on why other
>> altp2m_vcpu_update_p2m() invocations don't need to be
>> prefixed (now: replaced) by altp2m_vcpu_update_vmfunc_ve().
>> In the end it might well be that folding the two hooks into one is
>> the best course of action.
> 
> I'll do my best to make the description more readable. As for folding
> the two hooks into one (I assume you mean having a single function, such
> as, e.g. altp2m_vcpu_update_ve_and_p2m() and removing the other two), it
> looks like vmx_vcpu_update_vmfunc_ve() does a few things that would be
> unnnecessary (not optimal) in the general case. For example it calls
> __vmwrite(VM_FUNCTION_CONTROL, VMX_VMFUNC_EPTP_SWITCHING);, which
> shouldn't necessarily happen at the callsites of
> altp2m_vcpu_update_p2m(v) in p2m.c (in p2m_switch_vcpu_altp2m_by_id()
> and p2m_switch_domain_altp2m_by_id()). So from that point of view, it
> may be worth to keep both altp2m_vcpu_update_p2m() and
> altp2m_vcpu_update_vmfunc_ve() (the latter still always needing to call
> the former to do its job properly).
> 
> It's possible that I've misunderstood your comment here though.

I think you've understood me right; what you say makes sense at the
first glance. Please summarize this in the commit message, so that
further questions (perhaps also by others) can be avoided.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel