Re: [Xen-devel] [PATCH 4/9] x86/vmx: Support remote access to the MSR lists

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/05/18 12:50, Roger Pau Monné wrote:
> On Tue, May 22, 2018 at 12:20:41PM +0100, Andrew Cooper wrote:
>> At the moment, all modifications of the MSR lists are in current context.
>> However, future changes may need to put MSR_EFER into the lists from domctl
>> hypercall context.
>>
>> Plumb a struct vcpu parameter down through the infrastructure, and use
>> vmx_vmcs_{enter,exit}() for safe access to the VMCS in vmx_add_msr().  Use
>> assertions to ensure that access is either in current context, or while the
>> vcpu is paused.
>>
>> For now it is safe to require that remote accesses are under the domctl lock.
>> This will remain safe if/when the global domctl lock becomes per-domain.
> I'm not sure I see the point of this sentence. From my reading of the
> above test accesses will always be safe regardless of whether the
> domctl lock is global or per-domain?

Its attempting to justify why the domctl lock is ok to use here, but I
can drop the paragraph if people think it isn't helpful.

>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index e4acdc1..8bf54c4 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1301,13 +1301,15 @@ static struct vmx_msr_entry *locate_msr_entry(
>>      return start;
>>  }
>>  
>> -struct vmx_msr_entry *vmx_find_msr(uint32_t msr, enum vmx_msr_list_type 
>> type)
>> +struct vmx_msr_entry *vmx_find_msr(struct vcpu *v, uint32_t msr,
>> +                                   enum vmx_msr_list_type type)
>>  {
>> -    struct vcpu *curr = current;
>> -    struct arch_vmx_struct *arch_vmx = &curr->arch.hvm_vmx;
>> +    struct arch_vmx_struct *arch_vmx = &v->arch.hvm_vmx;
>>      struct vmx_msr_entry *start = NULL, *ent, *end;
>>      unsigned int total;
>>  
>> +    ASSERT(v == current || !vcpu_runnable(v));
> I would rather do:
>
> if ( v != current && vcpu_runnable(v) )
> {
>     ASSERT_UNREACHABLE();
>     return NULL;
> }

I'm not so certain.  Failing the assertion doesn't make the later code
unsafe to execute.

Even for the mutable operations, all that would happen if the assertion
failed would be corruption to the guest's view of its MSR state.

~Andrew

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