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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 22/01/16 14:33, Jan Beulich wrote:
>>>> On 22.01.16 at 15:24, <andrew.cooper3@citrix.com> wrote:
>> On 22/01/16 09:56, Jan Beulich wrote:
>>>>>> On 16.12.15 at 22:24, <andrew.cooper3@citrix.com> 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.

The point of my second sentence is that this would be a latent bug if
someone introduced another failure path, which is why I will fix it.

~Andrew