Re: [Xen-devel] [PATCH] nvmx: implement support for MSR bitmaps

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2020 13:31, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -128,6 +128,16 @@ int nvmx_vcpu_initialise(struct vcpu *v)
>          unmap_domain_page(vw);
>      }
>  
> +    if ( cpu_has_vmx_msr_bitmap )
> +    {
> +        nvmx->msr_merged = alloc_domheap_page(NULL, 0);

Despite this matching other code in the same file, it's not really
what you want, I think. Instead please consider using

        nvmx->msr_merged = alloc_domheap_page(d, MEMF_no_owner);

to honor d's NUMA properties.

> @@ -182,6 +192,11 @@ void nvmx_vcpu_destroy(struct vcpu *v)
>          free_domheap_page(v->arch.hvm.vmx.vmwrite_bitmap);
>          v->arch.hvm.vmx.vmwrite_bitmap = NULL;
>      }
> +    if ( nvmx->msr_merged )
> +    {
> +        free_domheap_page(nvmx->msr_merged);
> +        nvmx->msr_merged = NULL;

Hmm, I'm puzzled that we have FREE_XENHEAP_PAGE(), but no
FREE_DOMHEAP_PAGE().

> @@ -548,6 +563,50 @@ unsigned long *_shadow_io_bitmap(struct vcpu *v)
>      return nestedhvm_vcpu_iomap_get(port80, portED);
>  }
>  
> +static void update_msrbitmap(struct vcpu *v)
> +{
> +    struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    struct vmx_msr_bitmap *msr_bitmap;
> +    unsigned int msr;
> +
> +    ASSERT(__n2_exec_control(v) & CPU_BASED_ACTIVATE_MSR_BITMAP);
> +
> +    if ( !nvmx->msrbitmap )
> +        return;
> +
> +    msr_bitmap = __map_domain_page(nvmx->msr_merged);
> +
> +    bitmap_or(msr_bitmap->read_low, nvmx->msrbitmap->read_low,
> +              v->arch.hvm.vmx.msr_bitmap->read_low,
> +              sizeof(msr_bitmap->read_low) * 8);
> +    bitmap_or(msr_bitmap->read_high, nvmx->msrbitmap->read_high,
> +              v->arch.hvm.vmx.msr_bitmap->read_high,
> +              sizeof(msr_bitmap->read_high) * 8);
> +    bitmap_or(msr_bitmap->write_low, nvmx->msrbitmap->write_low,
> +              v->arch.hvm.vmx.msr_bitmap->write_low,
> +              sizeof(msr_bitmap->write_low) * 8);
> +    bitmap_or(msr_bitmap->write_high, nvmx->msrbitmap->write_high,
> +              v->arch.hvm.vmx.msr_bitmap->write_high,
> +              sizeof(msr_bitmap->write_high) * 8);
> +
> +    /*
> +     * Nested VMX doesn't support any x2APIC hardware virtualization, so
> +     * make sure all the x2APIC MSRs are trapped.
> +     */
> +    ASSERT(!(__n2_secondary_exec_control(v) &
> +             (SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
> +              SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY)) );
> +    for ( msr = MSR_X2APIC_FIRST; msr <= MSR_X2APIC_FIRST + 0xff; msr++ )
> +    {
> +        set_bit(msr, msr_bitmap->read_low);
> +        set_bit(msr, msr_bitmap->write_low);

Surely __set_bit() will suffice, if all the bitmap_or() above are
fine? Of course ultimately we will want to have something like
bitmap_fill_range() for purposes like this one.

> @@ -558,10 +617,15 @@ void nvmx_update_exec_control(struct vcpu *v, u32 
> host_cntrl)
>      shadow_cntrl = __n2_exec_control(v);
>      pio_cntrl &= shadow_cntrl;
>      /* Enforce the removed features */
> -    shadow_cntrl &= ~(CPU_BASED_ACTIVATE_MSR_BITMAP
> -                      | CPU_BASED_ACTIVATE_IO_BITMAP
> +    shadow_cntrl &= ~(CPU_BASED_ACTIVATE_IO_BITMAP
>                        | CPU_BASED_UNCOND_IO_EXITING);
> -    shadow_cntrl |= host_cntrl;
> +    /*
> +     * Do NOT enforce the MSR bitmap currently used by L1, as certain 
> hardware
> +     * virtualization features require specific MSR bitmap settings, but 
> without
> +     * using such features the bitmap could be leaking through unwanted MSR
> +     * accesses.

Perhaps "..., but without the guest also using these same features
..."? And then - why would a similar argument not apply to the I/O
bitmap as well?

> @@ -584,6 +648,9 @@ void nvmx_update_exec_control(struct vcpu *v, u32 
> host_cntrl)
>          __vmwrite(IO_BITMAP_B, virt_to_maddr(bitmap) + PAGE_SIZE);
>      }
>  
> +    if ( shadow_cntrl & CPU_BASED_ACTIVATE_MSR_BITMAP )
> +        update_msrbitmap(v);

In the function you assert the bit to be set in the vVMCS, but ...

>      /* TODO: change L0 intr window to MTF or NMI window */
>      __vmwrite(CPU_BASED_VM_EXEC_CONTROL, shadow_cntrl);

... it gets written only here.

> @@ -1351,6 +1418,9 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
>      vmx_update_secondary_exec_control(v);
>      vmx_update_exception_bitmap(v);
>  
> +    if ( v->arch.hvm.vmx.exec_control & CPU_BASED_ACTIVATE_MSR_BITMAP )
> +        __vmwrite(MSR_BITMAP, virt_to_maddr(v->arch.hvm.vmx.msr_bitmap));
> +
>      load_vvmcs_host_state(v);

Wouldn't the addition better move into this function?

> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> @@ -37,7 +37,8 @@ struct nestedvmx {
>       */
>      paddr_t    vmxon_region_pa;
>      void       *iobitmap[2];         /* map (va) of L1 guest I/O bitmap */
> -    void       *msrbitmap;           /* map (va) of L1 guest MSR bitmap */
> +    struct vmx_msr_bitmap *msrbitmap;        /* map (va) of L1 guest MSR 
> bitmap */
> +    struct page_info *msr_merged;       /* merged L1 and L1 guest MSR bitmap 
> */

Either you convert the tab to spaces at least on the line you
change, or you use a tab (or two) as well on the line you add.

Jan

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