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

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

El 27/11/15 a les 9.00, Jan Beulich ha escrit:
>>>> 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).

Thanks for the feedback, I'm also wondering whether I should call
hvm_cr4_guest_reserved_bits and hvm_efer_valid like it's done in
hvm_load_cpu_ctxt. Currently we don't perform any of the EFER/CR4 checks
in order to make sure that what the user enables is actually allowed.
What do you think?

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

I wanted to do it as a define, like:

#define arch_initialize_vcpu default_initalize_vcpu

Inside of arm/domain.h, but that's included before the common domain.h,
so the prototype of qrch_initialize_vcpu gets replaced to
defaul_initialize_vcpu, and then the compiler complains about duplicate
prototypes. I could shuffle them a bit in order to fix it, but I think
the stub in arm/domain.c is clearer, and the compiler should optimize it
anyway.

Roger.


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