Re: [PATCH v2] xen/arm: vcpu: Correctly release resource when the VCPU failed to initialized

[Julien Grall <julien.grall@linaro.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 <julien.grall@linaro.org>
>>>>> 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