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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 02/07/2018 07:01 PM, Jan Beulich wrote:
>>>> On 02.02.18 at 09:14, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> @@ -2313,6 +2314,12 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
>>          }
>>      }
>>  
>> +    if ( hvm_pcid_enabled(v) ) /* Clear the noflush bit. */
>> +    {
>> +        noflush = !!(value & X86_CR3_NOFLUSH);
> 
> Pointless !!.

I'll change it.

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -34,6 +34,9 @@ extern bool_t opt_hvm_fep;
>>  #define opt_hvm_fep 0
>>  #endif
>>  
>> +#define X86_CR3_NOFLUSH (1ull << 63)
> 
> This belongs in x86-defs.h

I'll move it.

>> +#define X86_CR3_NOFLUSH_DISABLE_MASK (X86_CR3_NOFLUSH - 1)
> 
> This shouldn't be needed (use ~X86_CR3_NOFLUSH instead).

Agreed.

>> @@ -322,9 +325,10 @@ hvm_update_host_cr3(struct vcpu *v)
>>          hvm_funcs.update_host_cr3(v);
>>  }
>>  
>> -static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr)
>> +static inline void hvm_update_guest_cr(struct vcpu *v, unsigned int cr,
>> +                                       bool noflush)
>>  {
>> -    hvm_funcs.update_guest_cr(v, cr);
>> +    hvm_funcs.update_guest_cr(v, cr, noflush);
>>  }
> 
> Instead of altering this function (and a lot of callers), how about
> introducing
> 
> static inline void hvm_update_guest_cr3(struct vcpu *v, bool noflush)
> {
>     hvm_funcs.update_guest_cr(v, 3, noflush);
> }
> 
> ? I'm also not convinced of the update_guest_cr() hook taking a
> bool which is meaningless for all other CRs. Perhaps a more general
> flags parameter would be better, with CR-specific meaning (you'd
> then e.g. introduce HVM_UPDATE_GUEST_CR3_NO_FLUSH).

Very true. I'll change the patch.


Thanks for the review,
Razvan

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