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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.  Care also needs to be taken on the BSP where current
isn't valid either.

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

~Andrew

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