From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized Date: Fri, 02 May 2014 16:36:52 +0100 Message-ID: <5363BB94.4060606@linaro.org> References: <1398885355-13200-1-git-send-email-julien.grall@linaro.org> <1399033506.32736.84.camel@kazak.uk.xensource.com> <5363A735.3020205@linaro.org> <5363A8F0.2050109@citrix.com> <1399043711.18944.6.camel@kazak.uk.xensource.com> <1399044437.18944.15.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WgFWC-0005nX-G0 for xen-devel@lists.xenproject.org; Fri, 02 May 2014 15:36:56 +0000 Received: by mail-ee0-f49.google.com with SMTP id e53so3266722eek.8 for ; Fri, 02 May 2014 08:36:54 -0700 (PDT) In-Reply-To: <1399044437.18944.15.camel@kazak.uk.xensource.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: Ian Campbell Cc: Vijay Kilari , Andrew Cooper , Vijaya Kumar K , tim@xen.org, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 05/02/2014 04:27 PM, Ian Campbell wrote: > On Fri, 2014-05-02 at 16:15 +0100, Ian Campbell wrote: >> On Fri, 2014-05-02 at 15:17 +0100, Andrew Cooper wrote: >>> On 02/05/14 15:09, Julien Grall wrote: >>>> On 05/02/2014 01:25 PM, Ian Campbell wrote: >>>>> On Wed, 2014-04-30 at 20:15 +0100, Julien Grall wrote: >>>>>> While I was adding new failing code at the end of the function, I've noticed >>>>>> that the vtimers are not freed which mess all the timers and will crash Xen >>>>>> quickly when the page will be reused. >>>>>> >>>>>> Currently neither vcpu_vgic_init nor vcpu_vtimer_init fail, so we >>>>>> are safe for now. With the new GICv3 code, the former function will be able >>>>>> to fail. This will result to a memory leak. >>>>>> >>>>>> Call vcpu_destroy if the initialization has failed. We also need to add a >>>>>> boolean to know if the vtimers are correctly setup as the timer common code >>>>>> doesn't have safe guard against removing non-initialized timer. >>>>>> >>>>>> Signed-off-by: Julien Grall >>>>> I was about to acked + apply but it failed to build on arm64 with: >>>>> >>>>> domain.c: In function 'alloc_vcpu_struct': >>>>> /local/scratch/ianc/devel/committer.git/xen/include/xen/lib.h:19:31: error: static assertion failed: "!(sizeof(*v) > PAGE_SIZE)" >>>>> #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) >>>>> ^ >>>>> domain.c:415:5: note: in expansion of macro 'BUILD_BUG_ON' >>>>> BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE); >>>>> ^ >>>>> struct arch_vcpu is apparently now too large. >>>> Hmmm... I'm not sure what is the best solution. Can: >>>> 1) Allocate 2 pages for the VCPU structure >>>> 2) Allocate vgic structure outside. >>>> >>>> Any opinions? >>>> >>>> Regards, >>>> >>> >>> 2) >>> >>> The reason structs vcpu/domain were reduced to this size to was avoid >>> needing multi-page allocations, which risk allocation failures on >>> systems with sufficiently fragmented memory. >> >> Ack. #2 (with s/vgic/anything suitably self contained/) is the answer. > > Was Vijay not moving the vgtic stuff out in one of the gicv3 patches? (CC him) IIRC, he only moves the private_irqs field. I think we should move the whole structure, to give more space for the future. This patch will be necessary for the GICv3 serie has vcpu_vgic_init will be able to fail (see patch #10). Ideally, for bisection purpose, it should be applied before the patch #10. Regards, -- Julien Grall