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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/05/18 04:47, Tian, Kevin wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
>> Sent: Tuesday, May 22, 2018 7:21 PM
>>
>> 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.
>>
>> Note these expectations beside the fields in arch_vmx_struct, and reorder
>> the
>> fields to avoid unnecessary padding.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> To preempt any questions about spinlocks, the use of the MSR lists in the
>> return-to-guest path causes checklock failures for plain spinlocks (despite 
>> it
>> technically being safe to live here), and the call to alloc_xenheap_page()
>> makes it impossible to use irqsave/restore variants, due to the nested
>> acquisition of the heap lock.
> I don't understand above words. How does it relate to the patch here?

It explains why I haven't/can't introduce a spinlock to protect access,
in case someone reviewing the code asks "why not introduce a spinlock".

>> @@ -1333,12 +1335,14 @@ struct vmx_msr_entry
>> *vmx_find_msr(uint32_t msr, enum vmx_msr_list_type type)
>>      return ((ent < end) && (ent->index == msr)) ? ent : NULL;
>>  }
>>
>> -int vmx_add_msr(uint32_t msr, enum vmx_msr_list_type type)
>> +int vmx_add_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 **ptr, *start = NULL, *ent, *end;
>>      unsigned int total;
>> +    int rc;
>> +
>> +    ASSERT(v == current || !vcpu_runnable(v));
>>
>>      switch ( type )
>>      {
>> @@ -1357,13 +1361,18 @@ int vmx_add_msr(uint32_t msr, enum
>> vmx_msr_list_type type)
>>          return -EINVAL;
>>      }
>>
>> +    vmx_vmcs_enter(v);
>> +
> why entering vmcs so early even before possible page allocation?

Because the next thing the allocation path does is write to the MSR
load/save list fields.

The alternative would be to have an else on this if(), and a second
vmcs_enter() after the memory allocation, but as these are two one-time
allocations in uncontended paths, I didn't consider the added complexity
worth it.

~Andrew

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