|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v8 17/21] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs
El 12/11/15 a les 17.57, Jan Beulich ha escrit:
>>>> On 06.11.15 at 17:05, <roger.pau@xxxxxxxxxx> wrote:
>> @@ -1183,6 +1184,301 @@ int arch_set_info_guest(
>> #undef c
>> }
>>
>> +static inline int check_segment(struct segment_register reg,
>
> Passed by value?
Hm, right, will pass by reference in next version.
>
>> + enum x86_segment seg)
>> +{
>> +
>> + if ( reg.attr.fields.pad != 0 )
>> + {
>> + gprintk(XENLOG_ERR,
>> + "Attribute bits 12-15 of the segment are not zero\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( reg.sel == 0 && reg.base == 0 && reg.limit == 0 &&
>
> What's the sel check good for when your only caller only ever calls
> you with it being zero?
I don't mind removing the sel == 0 check but I don't think it hurts either.
> Looking at base or limit here doesn't seem
> right either.
I'm sorry but I'm not following you here, why is this not right? Would
you rather conclude that the user is trying to load a null segment by
just looking at the attributes field (and checking it's 0)?
>> + reg.attr.bytes == 0)
>> + {
>> + if ( seg == x86_seg_cs || seg == x86_seg_ss )
>> + {
>> + gprintk(XENLOG_ERR, "Null selector provided for CS or SS\n");
>> + return -EINVAL;
>> + }
>> + return 0;
>> + }
>> +
>> + if ( reg.attr.fields.p != 1 )
>
> This is effectively a boolean field - no need for "!= 1", should be
> just !.
Ack.
>> + {
>> + gprintk(XENLOG_ERR, "Non-present segment provided\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( seg == x86_seg_cs && !(reg.attr.fields.type & 0x8) )
>> + {
>> + gprintk(XENLOG_ERR, "CS is missing code type\n");
>> + return -EINVAL;
>> + }
>> +
>> + if ( seg != x86_seg_cs && !!(reg.attr.fields.type & 0x8) )
>
> No need for !! here. Also only SS is disallowed to get code segments
> loaded. For other selector registers they're okay when readable.
>
>> + {
>> + gprintk(XENLOG_ERR, "Non code segment with code type set\n");
>> + return -EINVAL;
>> + }
>
> SS must be writable.
OK, I've fixed both issues and also added a check to make sure the S
attribute bit is set for all segments except TR.
>> +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;
>> + int rc;
>> +
>> + if ( ctx->pad != 0 )
>> + {
>> + gprintk(XENLOG_ERR, "Padding field != 0\n");
>
> Please don't.
OK, I'm also going to remove the other gprintk below regarding non 0
padding.
>
>> + 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 )
>> + {
>> + gprintk(XENLOG_ERR, "Padding field != 0\n");
>> + 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;
>> +
>> + /* Basic sanity checks. */
>> + limit = cs.limit;
>> + if ( cs.attr.fields.g )
>> + limit = (limit << 12) | 0xfff;
>> + if ( regs->eip > limit )
>> + {
>> + gprintk(XENLOG_ERR, "EIP (%#08x) outside CS limit (%#08x)\n",
>> + regs->eip, limit);
>> + return -EINVAL;
>> + }
>> +
>> + if ( ds.attr.fields.p && ds.attr.fields.dpl > cs.attr.fields.dpl )
>> + {
>> + gprintk(XENLOG_ERR, "DS.DPL (%u) is greater than CS.DPL (%u)\n",
>> + ds.attr.fields.dpl, cs.attr.fields.dpl);
>
> Indentation.
Done (and fixed others above and below).
>
>> + return -EINVAL;
>> + }
>> +
>> + if ( ss.attr.fields.p && ss.attr.fields.dpl != cs.attr.fields.dpl )
>> + {
>> + gprintk(XENLOG_ERR, "SS.DPL (%u) is different than CS.DPL
>> (%u)\n",
>> + ss.attr.fields.dpl, cs.attr.fields.dpl);
>> + return -EINVAL;
>> + }
>
> I think this should go ahead of the DS one. Also I don't think
> either CS or SS would be okay with the present bit clear, even
> less so in 32-bit mode.
Yes, CS or SS cannot have the present bit clear, we already make sure of
that in the check_segment function above. This condition can indeed be
simplified.
>> +int arch_initialize_vcpu(struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>> +{
>> + struct domain *d = v->domain;
>> + int rc;
>> +
>> + if ( is_hvm_vcpu(v) )
>> + {
>> + struct vcpu_hvm_context ctxt;
>> +
>> + if ( copy_from_guest(&ctxt, arg, 1) )
>> + return -EFAULT;
>> +
>> + domain_lock(d);
>> + rc = v->is_initialised ? -EEXIST : arch_set_info_hvm_guest(v,
>> &ctxt);
>> + domain_unlock(d);
>> + }
>> + else
>> + {
>> + struct vcpu_guest_context *ctxt;
>> +
>> + 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);
>> + }
>
> This else branch looks suspiciously like the ARM variant, and iirc I
> had asked already on an earlier version to have this handled in
> common code (with ARM simply using the common function for its
> arch_initialize_vcpu()).
Done, I've created a default_initalize_vcpu that's shared between ARM
and x86 PV guests. The arch_initialize_vcpu implementation on ARM is
just a stub that calls default_initialize_vcpu.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |