Re: [PATCH] xen/xmalloc: XMEM_POOL_POISON improvements

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 20/12/2023 21:47, Andrew Cooper wrote:
> When in use, the spew:
>
>    (XEN) Assertion '!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, POISON_BYTE, 
> (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)' failed at common/xmalloc_tlsf.c:246
>
> is unweidly and likely meaningless to non-Xen developers.  Therefore:
>
>   * Switch to IS_ENABLED().  There's no need for full #ifdef-ary.
>   * Pull memchr_inv() out into the if(), and provide an error message which
>     clearly states that corruption has been found.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
With one remark:

Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> Observations from the debugging of:
>    https://github.com/Dasharo/dasharo-issues/issues/488
>
> v2:
>   * Switch to printk()+BUG()
I would suggest to add a sentence about the switch to printk() + BUG() 
in the commit message. Maybe:
"CONFIG_XMEM_POOL_POISON can be enabled even on production build. So we 
should switch from ASSERT() to BUG()."
Cheers,

--
Julien Grall