Re: [PATCH v4 26/31] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/08/15 11:18, Roger Pau Monne wrote:
> Allow the usage of the VCPUOP_initialise, VCPUOP_up, VCPUOP_down and
> VCPUOP_is_up hypercalls from HVM guests.
>
> This patch introduces a new structure (vcpu_hvm_context) that should be used
> in conjuction with the VCPUOP_initialise hypercall in order to initialize
> vCPUs for HVM guests.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
> ---
>  xen/arch/arm/domain.c             |  24 ++++++
>  xen/arch/x86/domain.c             | 156 +++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c            |   8 ++
>  xen/common/domain.c               |  16 +---
>  xen/include/public/hvm/hvm_vcpu.h | 168 ++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/domain.h          |   2 +
>  6 files changed, 359 insertions(+), 15 deletions(-)
>  create mode 100644 xen/include/public/hvm/hvm_vcpu.h
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index b2bfc7d..b20035d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -752,6 +752,30 @@ int arch_set_info_guest(
>      return 0;
>  }
>  
> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct vcpu_guest_context *ctxt;
> +    struct domain *d = current->domain;
> +    int rc;
> +
> +    if ( (ctxt = alloc_vcpu_guest_context()) == NULL )

I have a patch to drop this {alloc,free}_vcpu_guest_context()
infrastructure in 4.7

It was done to avoid issuing order 1 domheap allocations and spuriously
failing because of memory fragmentation, but vmalloc() is now a strictly
better option.

> +        return -ENOMEM;
> +
> +    if ( copy_from_guest(ctxt, arg, 1) )
> +    {
> +        free_vcpu_guest_context(ctxt);
> +        return -EFAULT;
> +    }
> +
> +    domain_lock(d);
> +    rc = v->is_initialised ? -EEXIST : arch_set_info_guest(v, ctxt);
> +    domain_unlock(d);
> +
> +    free_vcpu_guest_context(ctxt);
> +
> +    return rc;
> +}
> +
>  int arch_vcpu_reset(struct vcpu *v)
>  {
>      vcpu_end_shutdown_deferral(v);
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 432fe43..4a7f8d9 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -37,6 +37,7 @@
>  #include <xen/wait.h>
>  #include <xen/guest_access.h>
>  #include <public/sysctl.h>
> +#include <public/hvm/hvm_vcpu.h>
>  #include <asm/regs.h>
>  #include <asm/mc146818rtc.h>
>  #include <asm/system.h>
> @@ -1135,6 +1136,161 @@ int arch_set_info_guest(
>  #undef c
>  }
>  
> +/* Called by VCPUOP_initialise for HVM guests. */
> +static int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx)
> +{
> +    struct segment_register seg;
> +
> +#define get_context_seg(ctx, seg, f)                                        \
> +    (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.seg##_##f :   \
> +    (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.seg##_##f :   \
> +    (ctx)->cpu_regs.x86_64.seg##_##f

I would be tempted to remove _context from the name, capitalise them to
make them stick out as macros, and turn them into function style macros
({ }) to avoid risk of multiple expansion.

Also you need a sanity check on mode, rather than assuming 64bit.

> +
> +#define get_context_gpr(ctx, gpr)                                           \
> +    (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.gpr :         \
> +    (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.e##gpr :      \
> +    (ctx)->cpu_regs.x86_64.r##gpr
> +
> +#define get_context_field(ctx, field)                                       \
> +    (ctx)->mode == VCPU_HVM_MODE_16B ? (ctx)->cpu_regs.x86_16.field :       \
> +    (ctx)->mode == VCPU_HVM_MODE_32B ? (ctx)->cpu_regs.x86_32.field :       \
> +    (ctx)->cpu_regs.x86_64.field
> +
> +    memset(&seg, 0, sizeof(seg));
> +
> +    if ( !paging_mode_hap(v->domain) )
> +        v->arch.guest_table = pagetable_null();
> +
> +    v->arch.user_regs.rax = get_context_gpr(ctx, ax);
> +    v->arch.user_regs.rcx = get_context_gpr(ctx, cx);
> +    v->arch.user_regs.rdx = get_context_gpr(ctx, dx);
> +    v->arch.user_regs.rbx = get_context_gpr(ctx, bx);
> +    v->arch.user_regs.rsp = get_context_gpr(ctx, sp);
> +    v->arch.user_regs.rbp = get_context_gpr(ctx, bp);
> +    v->arch.user_regs.rsi = get_context_gpr(ctx, si);
> +    v->arch.user_regs.rdi = get_context_gpr(ctx, di);
> +    v->arch.user_regs.rip = get_context_gpr(ctx, ip);
> +    v->arch.user_regs.rflags = get_context_gpr(ctx, flags);
> +
> +    v->arch.hvm_vcpu.guest_cr[0] = get_context_field(ctx, cr0) | X86_CR0_ET;

I am not sure whether ET is worth doing here.  It is stuck high and
ignores writes on any process Xen currently functions on.

> +    hvm_update_guest_cr(v, 0);
> +    v->arch.hvm_vcpu.guest_cr[4] = get_context_field(ctx, cr4);
> +    hvm_update_guest_cr(v, 4);
> +
> +    switch ( ctx->mode )
> +    {
> +    case VCPU_HVM_MODE_32B:
> +        v->arch.hvm_vcpu.guest_efer = ctx->cpu_regs.x86_32.efer;
> +        hvm_update_guest_efer(v);
> +        v->arch.hvm_vcpu.guest_cr[3] = ctx->cpu_regs.x86_32.cr3;
> +        hvm_update_guest_cr(v, 3);
> +        break;

Newline here please.

> +    case VCPU_HVM_MODE_64B:
> +        v->arch.user_regs.r8 = ctx->cpu_regs.x86_64.r8;
> +        v->arch.user_regs.r9 = ctx->cpu_regs.x86_64.r9;
> +        v->arch.user_regs.r10 = ctx->cpu_regs.x86_64.r10;
> +        v->arch.user_regs.r11 = ctx->cpu_regs.x86_64.r11;
> +        v->arch.user_regs.r12 = ctx->cpu_regs.x86_64.r12;
> +        v->arch.user_regs.r13 = ctx->cpu_regs.x86_64.r13;
> +        v->arch.user_regs.r14 = ctx->cpu_regs.x86_64.r14;
> +        v->arch.user_regs.r15 = ctx->cpu_regs.x86_64.r15;
> +        v->arch.hvm_vcpu.guest_efer = ctx->cpu_regs.x86_64.efer;
> +        hvm_update_guest_efer(v);
> +        v->arch.hvm_vcpu.guest_cr[3] = ctx->cpu_regs.x86_64.cr3;
> +        hvm_update_guest_cr(v, 3);
> +        break;
> +    default:
> +        break;

Drop the default case if it is going to look like this.

> +    }
> +
> +    if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) )
> +    {
> +        /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
> +        struct page_info *page = get_page_from_gfn(v->domain,
> +                                 v->arch.hvm_vcpu.guest_cr[3] >> PAGE_SHIFT,
> +                                 NULL, P2M_ALLOC);
> +        if ( !page )
> +        {
> +            gdprintk(XENLOG_ERR, "Invalid CR3\n");
> +            domain_crash(v->domain);
> +            return -EINVAL;
> +        }
> +
> +        v->arch.guest_table = pagetable_from_page(page);
> +    }
> +
> +    seg.base = get_context_seg(ctx, cs, base);
> +    seg.limit = get_context_seg(ctx, cs, limit);
> +    seg.attr.bytes = get_context_seg(ctx, cs, ar);
> +    hvm_set_segment_register(v, x86_seg_cs, &seg);
> +    seg.base = get_context_seg(ctx, ds, base);
> +    seg.limit = get_context_seg(ctx, ds, limit);
> +    seg.attr.bytes = get_context_seg(ctx, ds, ar);
> +    hvm_set_segment_register(v, x86_seg_ds, &seg);
> +    seg.base = get_context_seg(ctx, ss, base);
> +    seg.limit = get_context_seg(ctx, ss, limit);
> +    seg.attr.bytes = get_context_seg(ctx, ss, ar);
> +    hvm_set_segment_register(v, x86_seg_ss, &seg);
> +    seg.base = get_context_seg(ctx, tr, base);
> +    seg.limit = get_context_seg(ctx, tr, limit);
> +    seg.attr.bytes = get_context_seg(ctx, tr, ar);
> +    hvm_set_segment_register(v, x86_seg_tr, &seg);

This block would be easier to read as 4 blocks, one for each segment,
and if the " = " in the middle were lined up.

> +
> +    /* Sync AP's TSC with BSP's. */
> +    v->arch.hvm_vcpu.cache_tsc_offset =
> +        v->domain->vcpu[0]->arch.hvm_vcpu.cache_tsc_offset;
> +    hvm_funcs.set_tsc_offset(v, v->arch.hvm_vcpu.cache_tsc_offset,
> +                             v->domain->arch.hvm_domain.sync_tsc);
> +
> +    v->arch.hvm_vcpu.msr_tsc_adjust = 0;
> +
> +    paging_update_paging_modes(v);
> +
> +    v->arch.flags |= TF_kernel_mode;

TF_kernel_mode only applies to PV guests.  HVM guests determine kernel
vs user mode based on cpl.

> +    v->is_initialised = 1;
> +    set_bit(_VPF_down, &v->pause_flags);
> +
> +    return 0;
> +#undef get_context_field
> +#undef get_context_gpr
> +#undef get_context_seg
> +}
> +
> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> +    struct vcpu_guest_context *ctxt;
> +    struct vcpu_hvm_context hvm_ctx;
> +    struct domain *d = current->domain;
> +    int rc;
> +
> +    if ( is_hvm_vcpu(v) )
> +    {
> +        if ( copy_from_guest(&hvm_ctx, arg, 1) )
> +            return -EFAULT;
> +
> +        domain_lock(d);
> +        rc = v->is_initialised ? -EEXIST : arch_set_info_hvm_guest(v, &hvm_ctx);
> +        domain_unlock(d);
> +    } else {

Style.

Otherwise, this looks rather neat and surprisingly non-invasive.

~Andrew

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