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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 02/15/2018 06:50 PM, Jan Beulich wrote:
>>>> On 09.02.18 at 12:01, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> @@ -563,13 +563,19 @@ void svm_update_guest_cr(struct vcpu *v, unsigned int 
>> cr)
>>      case 3:
>>          vmcb_set_cr3(vmcb, v->arch.hvm_vcpu.hw_cr[3]);
>>          if ( !nestedhvm_enabled(v->domain) )
>> -            hvm_asid_flush_vcpu(v);
>> +        {
>> +            if ( !(flags & HVM_UPDATE_GUEST_CR3_NO_FLUSH) )
>> +                hvm_asid_flush_vcpu(v);
>> +        }
>>          else if ( nestedhvm_vmswitch_in_progress(v) )
>>              ; /* CR3 switches during VMRUN/VMEXIT do not flush the TLB. */
>>          else
>> -            hvm_asid_flush_vcpu_asid(
>> -                nestedhvm_vcpu_in_guestmode(v)
>> -                ? &vcpu_nestedhvm(v).nv_n2asid : &v->arch.hvm_vcpu.n1asid);
>> +        {
>> +            if ( !(flags & HVM_UPDATE_GUEST_CR3_NO_FLUSH) )
> 
> Any reason you didn't make this an "else if()", reducing code churn?

I just thought it reads easier, but I don't feel strongly about it.
I'll change it.

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -80,6 +80,9 @@ enum hvm_intblk {
>>  #define HVM_EVENT_VECTOR_UNSET    (-1)
>>  #define HVM_EVENT_VECTOR_UPDATING (-2)
>>  
>> +/* update_guest_cr() flags. */
>> +#define HVM_UPDATE_GUEST_CR3_NO_FLUSH 0x00000001
> 
> I'd prefer if the naming was consistent with X86_CR3_NOFLUSH
> (i.e. have or don't have an underscore between NO and FLUSH in
> both cases).

Of course. I'll change it.

>> --- a/xen/include/asm-x86/x86-defns.h
>> +++ b/xen/include/asm-x86/x86-defns.h
>> @@ -43,6 +43,11 @@
>>  #define X86_CR0_PG              0x80000000 /* Paging                   (RW) 
>> */
>>  
>>  /*
>> + * Intel CPU flags in CR3
>> + */
>> +#define X86_CR3_NOFLUSH    0x8000000000000000
> 
> Please add the ULL suffix, so the insn emulator could eventually
> use this without breaking the 32-bit test harness build.

Will change it (to _AC(...)) (as suggested by both you and Andrew).


Thanks,
Razvan

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