Re: [Xen-devel] [PATCH v8 17/21] xen/x86: allow HVM guests to use hypercalls to bring up vCPUs

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.11.15 at 17:57, <roger.pau@xxxxxxxxxx> wrote:
> El 12/11/15 a les 17.57, Jan Beulich ha escrit:
>>>>> On 06.11.15 at 17:05, <roger.pau@xxxxxxxxxx> wrote:
>>> +    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.

Its presence having confused me means it may confuse other readers.

>> 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)?

Yes, exactly. Attributes being all zero makes a segment a null one
regardless of base or limit (if anything refusing non-zero base/limit
when attributes are zero as being inconsistent would be an option).

>>> +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.

I'd actually have expected that to just be a #define, but okay.

Jan


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