On 12/06/18 09:27, Jan Beulich wrote:
>>>> On 08.06.18 at 20:48, <[email protected]> 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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
