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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> +    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?

> +    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?

Jan



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