Re: [Xen-devel] [PATCH] x86/hvm: fix domain crash when CR3 has the noflush bit set

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 01/29/2018 02:58 PM, Jan Beulich wrote:
>>>> On 29.01.18 at 13:44, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 01/26/2018 12:27 PM, Jan Beulich wrote:
>>>>>> On 26.01.18 at 10:39, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -2324,6 +2324,9 @@ int hvm_set_cr3(unsigned long value, bool_t 
>>>> may_defer)
>>>>          }
>>>>      }
>>>>  
>>>> +    if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
>>>> +        value &= ((1ull << 63) - 1);
>>>
>>> This needs a proper X86_CR3_* constant defined.
>>>
>>> Did you investigate what it would take to actually _act_ upon the
>>> bit being set (besides clearing it for the value to be stored to be
>>> sane)? Afaict it would require handing a boolean through
>>> paging_update_cr3() -> ... -> {svm,vmx}_update_guest_cr() to
>>> allow suppressing the hvm_asid_flush_vcpu() etc there.
>>
>> I'm now propagating the bool to {svm,vmx}_update_guest_cr() in a
>> quick-test patch (attached), however my Windows 10 KPTI-enabled guest is
>> now BSODing with "IRQL NOT LESS OR EQUAL" soon after I attach to it with
>> xen-access.
>>
>> Clearly I'm touching code I'm not familiar with, so if this is something
>> worth pursuing any suggestions are appreciated. Otherwise, I can stick
>> to the original plan and just clear the noflush bit with no other
>> processing until things become clearer with the new code.
> 
> I think once you omit the unnecessary !! from
> 
>     bool flush = !!(value & X86_CR3_NOFLUSH);
> 
> you'll recognize yourself that you've got this inverted (besides
> the whole construct needing to sit in the PCID conditional). I
> didn't look further, because I think with such an issue things
> can't possibly work. I'd anyway suggest to make the new
> parameters "noflush", as the override is the exception, and
> flushing is the default.

Indeed, sorry for getting lost in all the negations. I'll modify the
patch to do the right thing there and give it more testing.


Thanks,
Razvan

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