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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jan 08, 2020 at 01:55:42PM +0100, Jan Beulich wrote:
> 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.

Right.

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

Oh sure, and I already looked for a bitmap_fill_range when coding this
:).

> > @@ -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
> ..."? 

Ack

> And then - why would a similar argument not apply to the I/O
> bitmap as well?

Well, there seems to be some handling of IO bitmaps, see
nvmx_update_exec_control (note I'm not claiming such handling is
correct).

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

No, I assert the bit in the vvmcs as returned by __n2_exec_control,
which is not what gets written to the actual vmcs below.

> 
> >      /* 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?

I don't have a strong opinion TBH, it's all quite fuzzy as to which
function loads what IMO.

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

Will just use a tab instead.

Thanks, Roger.

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