Re: [PATCH v7 27/32] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

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

On 02/10/15 16:48, 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>
> Signed-off-by: Andrew Cooper <andrew.cooper3@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>
> ---
> Changes since v6:
>  - Add comments to clarify some initializations.
>  - Introduce a generic default_initialize_vcpu that's used to initialize a
>    ARM vCPU or a x86 PV vCPU.
>  - Move the undef of the SEG macro.
>  - Fix the size of the eflags register, it should be 32bits.
>  - Add a comment regarding the value of the 12-15 bits of the _ar fields.
>  - Remove the 16bit strucutre, the 32bit one can be used to start the cpu in
>    real mode.
>  - Add some sanity checks to the values passed in.
>  - Add paddings to vcpu_hvm_context so the layout on 32/64bits is the same.
>  - Add support for the compat version of VCPUOP_initialise.
>
> Changes since v5:
>  - Fix a coding style issue.
>  - Merge the code from wip-dmlite-v5-refactor by Andrew in order to reduce
>    bloat.
>  - Print the offending %cr3 in case of error when using shadow.
>  - Reduce the scope of local variables in arch_initialize_vcpu.
>  - s/current->domain/v->domain/g in arch_initialize_vcpu.
>  - Expand the comment in public/vcpu.h to document the usage of
>    vcpu_hvm_context for HVM guests.
>  - Add myself as the copyright holder for the public hvm_vcpu.h header.
>
> Changes since v4:
>  - Don't assume mode is 64B, add an explicit check.
>  - Don't set TF_kernel_mode, it is only needed for PV guests.
>  - Don't set CR0_ET unconditionally.
> ---
>  xen/arch/x86/domain.c             | 185 ++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c            |   8 ++
>  xen/common/compat/domain.c        |  71 +++++++++++----
>  xen/common/domain.c               |  56 +++++++++---
>  xen/include/Makefile              |   1 +
>  xen/include/asm-x86/domain.h      |   3 +
>  xen/include/public/hvm/hvm_vcpu.h | 144 +++++++++++++++++++++++++++++
>  xen/include/public/vcpu.h         |   6 +-
>  xen/include/xlat.lst              |   3 +
>  9 files changed, 448 insertions(+), 29 deletions(-)
>  create mode 100644 xen/include/public/hvm/hvm_vcpu.h
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index a3b1c9b..af5feea 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>
> @@ -1176,6 +1177,190 @@ int arch_set_info_guest(
>  #undef c
>  }
>  
> +/* Called by VCPUOP_initialise for HVM guests. */
> +int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx)
> +{
> +    struct cpu_user_regs *uregs = &v->arch.user_regs;
> +    struct segment_register cs, ds, ss, es, tr;
> +
> +    switch ( ctx->mode )
> +    {
> +    default:
> +        return -EINVAL;
> +
> +    case VCPU_HVM_MODE_32B:
> +    {
> +        const struct vcpu_hvm_x86_32 *regs = &ctx->cpu_regs.x86_32;
> +        uint32_t limit;
> +
> +#define SEG(s, r)                                                       \
> +    (struct segment_register){ .sel = 0, .base = (r)->s ## _base,       \
> +            .limit = (r)->s ## _limit, .attr.bytes = (r)->s ## _ar }
> +        cs = SEG(cs, regs);
> +        ds = SEG(ds, regs);
> +        ss = SEG(ss, regs);
> +        es = SEG(es, regs);
> +        tr = SEG(tr, regs);
> +#undef SEG
> +
> +        /* Basic sanity checks. */
> +        if ( cs.attr.fields.pad != 0 || ds.attr.fields.pad != 0 ||
> +             ss.attr.fields.pad != 0 || es.attr.fields.pad != 0 ||
> +             tr.attr.fields.pad != 0 )
> +        {
> +            gprintk(XENLOG_ERR, "Attribute bits 12-15 of the segments are not null\n");

I would use 'zero' as opposed to 'null' here.  There is nothing to do
with pointers here.

> +            return -EINVAL;
> +        }
> +
> +        limit = cs.limit * (cs.attr.fields.g ? PAGE_SIZE : 1);

This will overflow in the common case.  Calculation of the limit is a
little awkward.  I believe this should do:

limit = cs.limit
if ( cs.attr.fields.g )
    limit = (limit << 12) | 0xfff;

In the case that g is set and cs is a flat segment, limit should have
the value ~0U, rather than 0 which is what your calculation will achieve.

> +        if ( regs->eip > limit )
> +        {
> +            gprintk(XENLOG_ERR, "EIP address is outside of the CS limit\n");

In all cases, please print out the values, to make the error message
more helpful.

e.g. "EIP (%08x) outside CS limit (%08x)"

> +            return -EINVAL;
> +        }
> +
> +        if ( ds.attr.fields.dpl > cs.attr.fields.dpl )
> +        {
> +            gprintk(XENLOG_ERR, "DPL of DS is greater than DPL of CS\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( ss.attr.fields.dpl != cs.attr.fields.dpl )
> +        {
> +            gprintk(XENLOG_ERR, "DPL of SS is different than DPL of CS\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( es.attr.fields.dpl > cs.attr.fields.dpl )
> +        {
> +            gprintk(XENLOG_ERR, "DPL of ES is greater than DPL of CS\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( ((regs->efer & EFER_LMA) && !(regs->efer & EFER_LME)) ||
> +             ((regs->efer & EFER_LME) && !(regs->efer & EFER_LMA)) )

This simplifies to ( (!!(regs->efer & EFER_LMA)) ^ (!!(regs->efer &
EFER_LME)) )

> +        {
> +            gprintk(XENLOG_ERR, "EFER.LMA and EFER.LME must be both set\n");

And this should say "both the same", rather than both set.

Having said this, I still don't think it is sensible to require that LMA
is set, seeing as it is strictly a read-only bit in EFER.  I would
suggest keying on LME alone, and automatically ORing in LMA, which
matches the behaviour of hardware more closely.

> +            return -EINVAL;
> +        }
> +
> +        uregs->rax    = regs->eax;
> +        uregs->rcx    = regs->ecx;
> +        uregs->rdx    = regs->edx;
> +        uregs->rbx    = regs->ebx;
> +        uregs->rsp    = regs->esp;
> +        uregs->rbp    = regs->ebp;
> +        uregs->rsi    = regs->esi;
> +        uregs->rdi    = regs->edi;
> +        uregs->rip    = regs->eip;
> +        uregs->rflags = regs->eflags;
> +
> +        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
> +        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
> +        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
> +        v->arch.hvm_vcpu.guest_efer  = regs->efer;
> +    }
> +    break;
> +
> +    case VCPU_HVM_MODE_64B:
> +    {
> +        const struct vcpu_hvm_x86_64 *regs = &ctx->cpu_regs.x86_64;
> +
> +        /* Basic sanity checks. */
> +        if ( !is_canonical_address(regs->rip) )
> +        {
> +            gprintk(XENLOG_ERR, "RIP contains a non-canonical address\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( !(regs->cr0 & X86_CR0_PG) )
> +        {
> +            gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( !(regs->cr4 & X86_CR4_PAE) )
> +        {
> +            gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled\n");
> +            return -EINVAL;
> +        }
> +
> +        if ( (regs->efer & (EFER_LME | EFER_LMA)) != (EFER_LME | EFER_LMA) )
> +        {
> +            gprintk(XENLOG_ERR, "EFER doesn't have LME or LMA enabled\n");
> +            return -EINVAL;
> +        }
> +
> +        uregs->rax    = regs->rax;
> +        uregs->rcx    = regs->rcx;
> +        uregs->rdx    = regs->rdx;
> +        uregs->rbx    = regs->rbx;
> +        uregs->rsp    = regs->rsp;
> +        uregs->rbp    = regs->rbp;
> +        uregs->rsi    = regs->rsi;
> +        uregs->rdi    = regs->rdi;
> +        uregs->rip    = regs->rip;
> +        uregs->rflags = regs->rflags;
> +
> +        v->arch.hvm_vcpu.guest_cr[0] = regs->cr0;
> +        v->arch.hvm_vcpu.guest_cr[3] = regs->cr3;
> +        v->arch.hvm_vcpu.guest_cr[4] = regs->cr4;
> +        v->arch.hvm_vcpu.guest_efer  = regs->efer;
> +
> +#define SEG(b, l, a)                                                    \
> +    (struct segment_register){ .sel = 0, .base = (b), .limit = (l),     \
> +                               .attr.bytes = (a) }
> +        cs = SEG(0, ~0u, 0xa9b); /* 64bit code segment. */
> +        ds = ss = es = SEG(0, ~0u, 0xc93);
> +        tr = SEG(0, 0x67, 0x8b); /* 64bit TSS (busy). */
> +#undef SEG

I would be tempted to get rid of this macro entirely.  The other macro
was to hide all the regs-> references, but this is entirely from constants.

~Andrew

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