Re: [XEN PATCH] xen/arm: address violation of MISRA C Rule 10.1

[Demi Marie Obenour <demiobenour@xxxxxxxxx>]

On 7/10/25 20:15, Stefano Stabellini wrote:
> On Thu, 10 Jul 2025, Dmytro Prokopchuk1 wrote:
>> Rule 10.1: Operands shall not be of an
>> inappropriate essential type
>>
>> The following are non-compliant:
>> - unary minus on unsigned type;
>> - boolean used as a numeric value.
>>
>> Replace unary '-' operator with multiplying by '-1L' or '-1LL'.
>> Replace numeric constant '-1UL' with '~0UL'.
>> Replace numeric constant '-1ULL' with '~0ULL'.
>> Cast boolean to numeric value.
>>
>> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>
>> ---
>>  xen/arch/arm/gic-vgic.c               | 2 +-
>>  xen/common/memory.c                   | 2 +-
>>  xen/common/page_alloc.c               | 6 +++---
>>  xen/common/time.c                     | 2 +-
>>  xen/drivers/passthrough/arm/smmu-v3.c | 2 +-
>>  xen/lib/strtol.c                      | 2 +-
>>  xen/lib/strtoll.c                     | 2 +-
>>  7 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
>> index ea48c5375a..a35f33c5f2 100644
>> --- a/xen/arch/arm/gic-vgic.c
>> +++ b/xen/arch/arm/gic-vgic.c
>> @@ -17,7 +17,7 @@
>>  #include <asm/vgic.h>
>>  
>>  #define lr_all_full()                                           \
>> -    (this_cpu(lr_mask) == (-1ULL >> (64 - gic_get_nr_lrs())))
>> +    (this_cpu(lr_mask) == (~0ULL >> (64 - gic_get_nr_lrs())))
> 
> I understand this change, I think it is good
> 
> 
> 
>>  #undef GIC_DEBUG
>>  
>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>> index 46620ed825..96086704cb 100644
>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -773,7 +773,7 @@ static long 
>> memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>>  
>>                  nrspin_lock(&d->page_alloc_lock);
>>                  drop_dom_ref = (dec_count &&
>> -                                !domain_adjust_tot_pages(d, -dec_count));
>> +                                !domain_adjust_tot_pages(d, (-1L) * 
>> dec_count));
> 
> ...but I don't understand this? It looks like it would also break 10.1
> and/or 10.3 as well?
> 
> I would rather use casts if needed, which wouldn't change the behavior
> but would highlight the unsigned->signed conversion, making it more
> visible:
> 
>     !domain_adjust_tot_pages(d, -(int)dec_count));

-INT_MIN is undefined behavior, whereas -(unsigned)INT_MIN should be
well-defined.  I'm not sure if that matters here, though.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Attachment: OpenPGP_0xB288B55FFF9C22C1.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature