From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC 29/31] x86/pv: Provide custom cpumasks for PV domains Date: Fri, 22 Jan 2016 14:42:01 +0000 Message-ID: <56A23FB9.5010206@citrix.com> References: <1450301073-28191-1-git-send-email-andrew.cooper3@citrix.com> <1450301073-28191-30-git-send-email-andrew.cooper3@citrix.com> <56A20AF102000078000C9F14@prv-mh.provo.novell.com> <56A23BBB.6020108@citrix.com> <56A24BB202000078000CA263@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56A24BB202000078000CA263@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org On 22/01/16 14:33, Jan Beulich wrote: >>>> On 22.01.16 at 15:24, wrote: >> On 22/01/16 09:56, Jan Beulich wrote: >>>>>> On 16.12.15 at 22:24, 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