|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v9 5/9] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
>>> On 27.11.15 at 14:43, <roger.pau@xxxxxxxxxx> wrote:
> +int arch_set_info_hvm_guest(struct vcpu *v, const vcpu_hvm_context_t *ctx)
> +{
> + struct cpu_user_regs *uregs = &v->arch.user_regs;
> + struct segment_register cs, ds, ss, es, tr;
> + const char *errstr;
> + int rc;
> +
> + if ( ctx->pad != 0 )
> + return -EINVAL;
> +
> + 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;
> +
> + if ( ctx->cpu_regs.x86_32.pad1 != 0 ||
> + ctx->cpu_regs.x86_32.pad2[0] != 0 ||
> + ctx->cpu_regs.x86_32.pad2[1] != 0 ||
> + ctx->cpu_regs.x86_32.pad2[2] != 0 )
> + return -EINVAL;
> +
> +#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
> +
> + rc = check_segment(&cs, x86_seg_cs);
> + rc |= check_segment(&ds, x86_seg_ds);
> + rc |= check_segment(&ss, x86_seg_ss);
> + rc |= check_segment(&es, x86_seg_es);
> + rc |= check_segment(&tr, x86_seg_tr);
> + if ( rc != 0 )
> + return rc;
This could be further simplified: The check_segment() invocation
could move into the macro, together with the assignments above.
And the .sel initializer is pointless. The former I'd leave to you,
but I'd like to see pointless things dropped to help readability.
> + 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
> (%#lx)\n",
> + regs->rip);
> + return -EINVAL;
> + }
> +
> + if ( !(regs->cr0 & X86_CR0_PG) )
> + {
> + gprintk(XENLOG_ERR, "CR0 doesn't have paging enabled
> (%#016lx)\n",
> + regs->cr0);
> + return -EINVAL;
> + }
> +
> + if ( !(regs->cr4 & X86_CR4_PAE) )
> + {
> + gprintk(XENLOG_ERR, "CR4 doesn't have PAE enabled (%#016lx)\n",
> + regs->cr4);
> + return -EINVAL;
> + }
> +
> + if ( !(regs->efer & EFER_LME) )
> + {
> + gprintk(XENLOG_ERR, "EFER doesn't have LME enabled (%#016lx)\n",
> + regs->efer);
> + 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
Similarly here: .sel and .base initializers are pointless.
> + }
> + break;
> +
> + }
> +
> + if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
> + v->arch.hvm_vcpu.guest_efer |= EFER_LMA;
> +
> + if ( v->arch.hvm_vcpu.guest_cr[4] & hvm_cr4_guest_reserved_bits(v, 1) )
> + {
> + gprintk(XENLOG_ERR, "Bad CR4 value: %#016lx\n",
> + v->arch.hvm_vcpu.guest_cr[4]);
> + return -EINVAL;
> + }
> +
> + errstr = hvm_efer_valid(v, v->arch.hvm_vcpu.guest_efer,
> + MASK_EXTR(v->arch.hvm_vcpu.guest_cr[0],
> + X86_CR0_PG));
Passing a non-negative value as the third argument here means
"use host CPUID", which I don't think is what we want here (and the
SCE check would be broken that way too).
> @@ -5130,6 +5129,10 @@ static long hvm_vcpu_op(
> case VCPUOP_stop_singleshot_timer:
> case VCPUOP_register_vcpu_info:
> case VCPUOP_register_vcpu_time_memory_area:
> + case VCPUOP_initialise:
> + case VCPUOP_up:
> + case VCPUOP_down:
> + case VCPUOP_is_up:
> rc = do_vcpu_op(cmd, vcpuid, arg);
> break;
> default:
> @@ -5188,6 +5191,10 @@ static long hvm_vcpu_op_compat32(
> case VCPUOP_stop_singleshot_timer:
> case VCPUOP_register_vcpu_info:
> case VCPUOP_register_vcpu_time_memory_area:
> + case VCPUOP_initialise:
> + case VCPUOP_up:
> + case VCPUOP_down:
> + case VCPUOP_is_up:
I vaguely recall possibly having commented on this before: This
leaves just VCPUOP_send_nmi and VCPUOP_get_physid not
allowed through, neither of which ought to be a problem to allow.
Therefore I'd rather see the wrappers go away than the switch()es
to get extended.
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -64,6 +64,8 @@ int arch_domain_soft_reset(struct domain *d);
> int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
> void arch_get_info_guest(struct vcpu *, vcpu_guest_context_u);
>
> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
> +
> int domain_relinquish_resources(struct domain *d);
>
> void dump_pageframe_info(struct domain *d);
> @@ -103,4 +105,6 @@ struct vnuma_info {
>
> void vnuma_destroy(struct vnuma_info *vnuma);
>
> +int default_initalize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg);
Any reason the two can't sit next to each other?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |