|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 27/32] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
>>> On 02.10.15 at 17:48, <roger.pau@xxxxxxxxxx> wrote:
> @@ -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)
const ... *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");
> + return -EINVAL;
> + }
> +
> + limit = cs.limit * (cs.attr.fields.g ? PAGE_SIZE : 1);
> + if ( regs->eip > limit )
> + {
> + gprintk(XENLOG_ERR, "EIP address is outside of the CS limit\n");
> + return -EINVAL;
> + }
> +
> + if ( ds.attr.fields.dpl > cs.attr.fields.dpl )
Checks like this imo need to take into account cases where the effect
of a null selector loaded into the register is intended (in which case I
would assume DPL to not matter). Speaking of which - with all these
DPL checks done, what about non-code segments loaded into CS or
other illegal things? Question is whether the
hvm_set_segment_register() calls below could be made take care of
these instead of having to enumerate everything here.
> --- a/xen/common/compat/domain.c
> +++ b/xen/common/compat/domain.c
> @@ -10,6 +10,9 @@
> #include <xen/guest_access.h>
> #include <xen/hypercall.h>
> #include <compat/vcpu.h>
> +#ifdef CONFIG_X86
> +#include <compat/hvm/hvm_vcpu.h>
> +#endif
I'd avoid such #if-s in this file, since it's only x86 that uses compat
code right now.
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1207,11 +1207,35 @@ void unmap_vcpu_info(struct vcpu *v)
> put_page_and_type(mfn_to_page(mfn));
> }
>
> +static int default_initialize_vcpu(struct vcpu *v,
> + XEN_GUEST_HANDLE_PARAM(void) arg)
> +{
> + struct vcpu_guest_context *ctxt;
> + struct domain *d = v->domain;
> + int rc;
> +
> + if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> + 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;
> +}
> +
> long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> {
> struct domain *d = current->domain;
> struct vcpu *v;
> - struct vcpu_guest_context *ctxt;
> long rc = 0;
>
> if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
> @@ -1223,20 +1247,28 @@ long do_vcpu_op(int cmd, unsigned int vcpuid,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> if ( v->vcpu_info == &dummy_vcpu_info )
> return -EINVAL;
>
> - if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
> - return -ENOMEM;
> -
> - if ( copy_from_guest(ctxt, arg, 1) )
> +#if defined(CONFIG_X86)
Looks like you went from one extreme to the other: Now there's no
per-arch function anymore, and hence you need this ugly #ifdef-ery.
Why don't you add default_initialize_vcpu() as a non-static function,
to be called by or (on ARM) used as arch_initialize_vcpu()?
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -10,6 +10,7 @@
> #include <asm/mce.h>
> #include <public/vcpu.h>
> #include <public/hvm/hvm_info_table.h>
> +#include <public/hvm/hvm_vcpu.h>
Please avoid this in headers so widely included:
> @@ -599,6 +600,8 @@ static inline void free_vcpu_guest_context(struct
> vcpu_guest_context *vgc)
> vfree(vgc);
> }
>
> +int arch_set_info_hvm_guest(struct vcpu *v, vcpu_hvm_context_t *ctx);
If you replace the typedef name by the actual structure, and if you
forward declare the structure, you don't have any build issue, but
you will avoid almost every file including the new header.
> +struct vcpu_hvm_x86_32 {
> + uint32_t eax;
> + uint32_t ecx;
> + uint32_t edx;
> + uint32_t ebx;
> + uint32_t esp;
> + uint32_t ebp;
> + uint32_t esi;
> + uint32_t edi;
> + uint32_t eip;
> + uint32_t eflags;
> +
> + uint32_t cr0;
> + uint32_t cr3;
> + uint32_t cr4;
> +
> + uint32_t pad1;
> +
> + /*
> + * EFER should only be used to set the NXE bit (if required)
> + * when starting a vCPU in 32bit mode with paging enabled or
> + * to set the LME/LMA bits in order to start the vCPU in
> + * compatibility mode.
> + */
> + uint64_t efer;
> +
> + uint32_t cs_base;
> + uint32_t ds_base;
> + uint32_t ss_base;
> + uint32_t es_base;
> + uint32_t tr_base;
> + uint32_t cs_limit;
> + uint32_t ds_limit;
> + uint32_t ss_limit;
> + uint32_t es_limit;
> + uint32_t tr_limit;
> + uint16_t cs_ar;
> + uint16_t ds_ar;
> + uint16_t ss_ar;
> + uint16_t es_ar;
> + uint16_t tr_ar;
> +
> + uint16_t pad2[2];
[3] perhaps?
Also I don't think I've seen you check these padding fields to be zero,
implying that we wouldn't be able to assign meaning to them later on.
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -56,6 +56,9 @@
> ? grant_entry_header grant_table.h
> ? grant_entry_v2 grant_table.h
> ? gnttab_swap_grant_ref grant_table.h
> +? vcpu_hvm_context hvm/hvm_vcpu.h
> +? vcpu_hvm_x86_32 hvm/hvm_vcpu.h
> +? vcpu_hvm_x86_64 hvm/hvm_vcpu.h
Do you really need all three added here, instead of just the
top level one?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |