Re: [Xen-devel] [PATCH RFC 29/31] x86/pv: Provide custom cpumasks for PV domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.01.16 at 15:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 22/01/16 14:33, Jan Beulich wrote:
>>>>> On 22.01.16 at 15:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 22/01/16 09:56, Jan Beulich wrote:
>>>>>>> On 16.12.15 at 22:24, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> --- a/xen/arch/x86/cpu/amd.c
>>>>> +++ b/xen/arch/x86/cpu/amd.c
>>>>> @@ -203,7 +203,9 @@ static void __init noinline probe_masking_msrs(void)
>>>>>  void amd_ctxt_switch_levelling(const struct domain *nextd)
>>>>>  {
>>>>>   struct cpumasks *these_masks = &this_cpu(cpumasks);
>>>>> - const struct cpumasks *masks = &cpumask_defaults;
>>>>> + const struct cpumasks *masks =
>>>>> +            (nextd && is_pv_domain(nextd) && nextd->arch.pv_domain.masks)
>>>>> +            ? nextd->arch.pv_domain.masks : &cpumask_defaults;
>>>> Can nextd really ever be NULL here?
>>> Yes, when using this function to set the defaults in the first place
>>> during AP bringup.
>> Ah, I then didn't spot this second use.
>>
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -578,6 +578,12 @@ int arch_domain_create(struct domain *d, unsigned 
>>>>> int 
> domcr_flags,
>>>>>              goto fail;
>>>>>          clear_page(d->arch.pv_domain.gdt_ldt_l1tab);
>>>>>  
>>>>> +        d->arch.pv_domain.masks = xmalloc(struct cpumasks);
>>>>> +        if ( !d->arch.pv_domain.masks )
>>>>> +            goto fail;
>>>>> +        memcpy(d->arch.pv_domain.masks, &cpumask_defaults,
>>>>> +               sizeof(*d->arch.pv_domain.masks));
>>>> Structure assignment, to make the thing type safe?
>>>>
>>>> Also there's a change missing to the cleanup code after the "fail"
>>>> label.
>>> What change are you thinking of?  I suppose an xfree() wouldn't go amis,
>>> to prevent a problem for whomever introduces a new failure path, but I
>>> don't see a bug in the code as-is.
>> I don't understand this second sentence. It's the missing addition
>> of a matching xfree() that my comment was about.
> 
> All "goto fails;" are visible in this context.  As the code currently
> stands, there is not a failure path where the allocation isn't freed.

There are numerous "goto fail;" further down in the function afaics.
Are we looking at the same piece of code?

Jan


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