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

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

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.
> 
> I had also reworded your commit message somewhat:
>     xen/arm: vcpu: Correctly release resources when a VCPU fails to initialize
>     
>     While I was adding new failing code at the end of the function, I noticed
>     that the vtimers are not freed which messes up all the timers and will crash
>     Xen quickly when the page s reused.
>     
>     Currently neither vcpu_vgic_init nor vcpu_vtimer_init fails, so we
>     are safe for now. With the new GICv3 code, the former function will be able
>     to fail. This will result in 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 any safeguard against removing a non-initialized timer.

The commit 6fedf29 "xen/arm: Drop event_mask in arch_vcpu" which makes
this patch compiles on both arm32 and arm64.

I think you can safely push this patch now :).

Thanks,

-- 
Julien Grall