Re: [Xen-devel] [PATCH v2 8/9] x86/vmx: Support removing MSRs from the host/guest load/save lists

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/06/18 09:27, Jan Beulich wrote:
>>>> On 08.06.18 at 20:48, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -1452,6 +1452,74 @@ int vmx_add_msr(struct vcpu *v, uint32_t msr, 
>> uint64_t val,
>>      return rc;
>>  }
>>  
>> +int vmx_del_msr(struct vcpu *v, uint32_t msr, enum vmx_msr_list_type type)
>> +{
>> +    struct arch_vmx_struct *vmx = &v->arch.hvm_vmx;
>> +    struct vmx_msr_entry *start = NULL, *ent, *end;
>> +    unsigned int substart, subend, total;
>> +
>> +    ASSERT(v == current || !vcpu_runnable(v));
>> +
>> +    switch ( type )
>> +    {
>> +    case VMX_MSR_HOST:
>> +        start    = vmx->host_msr_area;
>> +        substart = 0;
>> +        subend   = vmx->host_msr_count;
>> +        total    = subend;
>> +        break;
>> +
>> +    case VMX_MSR_GUEST:
>> +        start    = vmx->msr_area;
>> +        substart = 0;
>> +        subend   = vmx->msr_save_count;
>> +        total    = vmx->msr_load_count;
>> +        break;
>> +
>> +    case VMX_MSR_GUEST_LOADONLY:
>> +        start    = vmx->msr_area;
>> +        substart = vmx->msr_save_count;
>> +        subend   = vmx->msr_load_count;
>> +        total    = subend;
>> +        break;
>> +
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +    }
>> +
>> +    if ( !start )
>> +        return -ESRCH;
> I'm pretty sure not all gcc versions we support are capable of recognizing
> that substart, subend, and total can't be used uninitialized due to this
> return path, without there also being a return in after default: - I'm not
> sure though whether adding that return or initializers might be the
> better approach towards addressing this. At least for substart an
> initializer (of zero) would allow dropping two other lines of code.

The oldest compiler I can easily put my hands on:

x86_64-linux-gcc (GCC) 4.1.2 20080704 (Red Hat 4.1.2-46)

is fine with this.

>
>> +    end = start + total;
>> +    ent = locate_msr_entry(start + substart, start + subend, msr);
>> +
>> +    if ( (ent == end) || (ent->index != msr) )
>> +        return -ESRCH;
>> +
>> +    memmove(ent, ent + 1, sizeof(*ent) * (end - ent));
> Aren't you running over the end of the array by 1 entry here?

ent == end is an error condition above.  By this point, ent is
guaranteed to be < end.

>
>> +    vmx_vmcs_enter(v);
>> +
>> +    switch ( type )
>> +    {
>> +    case VMX_MSR_HOST:
>> +        __vmwrite(VM_EXIT_MSR_LOAD_COUNT, vmx->host_msr_count--);
>> +        break;
>> +
>> +    case VMX_MSR_GUEST:
>> +        __vmwrite(VM_EXIT_MSR_STORE_COUNT, vmx->msr_save_count--);
>> +
>> +        /* Fallthrough */
>> +    case VMX_MSR_GUEST_LOADONLY:
>> +        __vmwrite(VM_ENTRY_MSR_LOAD_COUNT, vmx->msr_load_count--);
>> +        break;
>> +    }
> Don't you want pre-decrements in all of these?

Using pre-decrements would end up with the value in struct vcpu being
correct, but the value in the VMCS being one-too-large.

I could alternatively move the subtraction to an earlier statement to
avoid any pre/post confusion?

~Andrew

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