Re: [Xen-devel] [PATCH v4 4/4] nested vmx: enable VMCS shadowing feature

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.13 at 13:00, Dongxiao Xu <dongxiao.xu@xxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -48,6 +48,47 @@ int nvmx_vcpu_initialise(struct vcpu *v)
>      if ( !nvcpu->vvmcx_buf )
>          gdprintk(XENLOG_WARNING, "nest: allocating exchanging buffer 
> failed\n");
>  
> +    /* non-root VMREAD/VMWRITE bitmap. */
> +    if ( cpu_has_vmx_vmcs_shadowing )
> +    {
> +        struct page_info *vmread_bitmap, *vmwrite_bitmap;
> +        void *vr, *vw;
> +
> +        vmread_bitmap = alloc_domheap_page(NULL, 0);
> +        if ( !vmread_bitmap )
> +        {
> +            gdprintk(XENLOG_ERR, "nest: allocation for vmread bitmap 
> failed\n");
> +            goto out1;
> +        }
> +        v->arch.hvm_vmx.vmread_bitmap = page_to_mfn(vmread_bitmap);

Considering that you never really use the MFNs, why do you store
them instead of the struct page_info *?

> +
> +        vmwrite_bitmap = alloc_domheap_page(NULL, 0);
> +        if ( !vmwrite_bitmap )
> +        {
> +            gdprintk(XENLOG_ERR, "nest: allocation for vmwrite bitmap 
> failed\n");
> +            goto out2;
> +        }
> +        v->arch.hvm_vmx.vmwrite_bitmap = page_to_mfn(vmwrite_bitmap);
> +
> +        vr = map_domain_page(v->arch.hvm_vmx.vmread_bitmap);
> +        vw = map_domain_page(v->arch.hvm_vmx.vmwrite_bitmap);
> +
> +        clear_page(vr);
> +        clear_page(vw);
> +
> +        /* 
> +         * For the following 4 encodings, we need to handle them in VMM.
> +         * Let them vmexit as usual.
> +         */
> +        set_bit(IO_BITMAP_A, (u64 *)vw);
> +        set_bit(IO_BITMAP_A_HIGH, (u64 *)vw);
> +        set_bit(IO_BITMAP_B, (u64 *)vw);
> +        set_bit(IO_BITMAP_B_HIGH, (u64 *)vw);

Ugly casts - neither do you really need u64 here (unsigned int
or unsigned long would suffice), nor is there any point in having
these (dangerous) explicit casts. Just have the variables have
the right type, map_domain_page() returning void * allows you
to do so.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel