[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC 29/31] x86/pv: Provide custom cpumasks for PV domains
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. 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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |