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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/01/16 14:48, Jan Beulich wrote:
>>>> 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?

Ah, it has changed between 4.6 and staging, but not sufficiently for
this code to be right on 4.6.  Sorry for the noise.

~Andrew

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