Re: [PATCH v18 2/2] xen/arm: check read handler behavior

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 3/30/25 18:08, Julien Grall wrote:
> Hi Steward,
> 
> On 25/03/2025 17:27, Stewart Hildebrand wrote:
>> We expect mmio read handlers to leave the bits above the access size
>> zeroed. Add an ASSERT to check this aspect of read handler behavior.
>>
>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> 
> With one question below:
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks!

>> ---
>> v17->v18:
>> * no change
>>
>> v16->v17:
>> * new patch
>>
>> See 
>> https://lore.kernel.org/xen-devel/bc6660ef-59f1-4514-9792-067d987e3fbc@xxxxxxx/
>>
>> Also see 7db7bd0f319f ("arm/vpci: honor access size when returning an error")
>>
>> Also see xen/arch/arm/ioreq.c:handle_ioserv()
>> ---
>>   xen/arch/arm/io.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 653428e16c1f..68b5dca70026 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -37,6 +37,8 @@ static enum io_state handle_read(const struct mmio_handler 
>> *handler,
>>       if ( !handler->ops->read(v, info, &r, handler->priv) )
>>           return IO_ABORT;
>>   +    ASSERT((r & ~GENMASK_ULL((1U << info->dabt.size) * 8 - 1, 0)) == 0);
> 
> OOI, I was expecing GENMASK to be sufficient because "r" is effectively an 
> "unsigned long". So any reason to use GENMASK_ULL?

Only reason was that I took inspiration from Roger's suggestion at [1].
However, I agree that GENMASK is indeed sufficient. Feel free to fix up
on commit. Lastly, this is OK to commit out of order as there is no
dependency on the first patch.

[1] https://lore.kernel.org/xen-devel/Zk72jPtd9iXhChbc@macbook/