[v2] system/physmem: Enhance the Address Space API

[PATCH v2 2/6] target/arm: Add a _MAX sentinel to ARMASIdx enum

Posted by Gustavo Romero 1 month, 3 weeks ago

Add a sentinel to the ARMASIdx enum so it can be used when the total
number of address spaces is required.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/cpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 39f2b2e54d..00f5af0fcd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2336,6 +2336,7 @@ typedef enum ARMASIdx {
     ARMASIdx_S = 1,
     ARMASIdx_TagNS = 2,
     ARMASIdx_TagS = 3,
+    ARMASIdx_MAX
 } ARMASIdx;
 
 static inline ARMMMUIdx arm_space_to_phys(ARMSecuritySpace space)
-- 
2.34.1

Re: [PATCH v2 2/6] target/arm: Add a _MAX sentinel to ARMASIdx enum

Posted by Philippe Mathieu-Daudé 1 month, 3 weeks ago

Hi Gustavo,

On 17/12/25 22:20, Gustavo Romero wrote:
> Add a sentinel to the ARMASIdx enum so it can be used when the total
> number of address spaces is required.
> 
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>   target/arm/cpu.h | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 39f2b2e54d..00f5af0fcd 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2336,6 +2336,7 @@ typedef enum ARMASIdx {
>       ARMASIdx_S = 1,
>       ARMASIdx_TagNS = 2,
>       ARMASIdx_TagS = 3,
> +    ARMASIdx_MAX

The problem with including this in the enum is this confuses static
analyzers:

warning: enumeration value 'ARMASIdx_MAX' not handled in switch [-Wswitch]

To avoid that we /define/ it manually instead:

#define ARMASIdx_MAX 4

>   } ARMASIdx;

Usually the definition is within the enum declaration, and we name
it ${enum}_COUNT:

     typedef enum ARMASIdx {
         ARMASIdx_NS = 0,
         ARMASIdx_S = 1,
         ARMASIdx_TagNS = 2,
         ARMASIdx_TagS = 3,
     #define ARMASIdx_COUNT 4
     } ARMASIdx;

Unfortunately this didn't work well with QAPI, so we could never enable
-Wswitch globally:
https://lore.kernel.org/qemu-devel/20230315112811.22355-1-philmd@linaro.org/

Today I'm not sure what is the best style anymore, so just take
my comments are historical 2 cents.

Regards,

Phil.

Re: [PATCH v2 2/6] target/arm: Add a _MAX sentinel to ARMASIdx enum

Posted by Manos Pitsidianakis 1 month, 3 weeks ago

On Thu, Dec 18, 2025 at 9:21 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Gustavo,
>
> On 17/12/25 22:20, Gustavo Romero wrote:
> > Add a sentinel to the ARMASIdx enum so it can be used when the total
> > number of address spaces is required.
> >
> > Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> > ---
> >   target/arm/cpu.h | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 39f2b2e54d..00f5af0fcd 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -2336,6 +2336,7 @@ typedef enum ARMASIdx {
> >       ARMASIdx_S = 1,
> >       ARMASIdx_TagNS = 2,
> >       ARMASIdx_TagS = 3,
> > +    ARMASIdx_MAX
>

My two cents:

Here, "ARMASIdx_MAX" should be equal to the last variant, 3. So to get
the total count, it would be ARMASIdx_MAX + 1.
And it should be called "ARMASIdx_COUNT". Max is the last variant.

> The problem with including this in the enum is this confuses static
> analyzers:
>
> warning: enumeration value 'ARMASIdx_MAX' not handled in switch [-Wswitch]
>

If we add a "MAX" instead of "COUNT" variant, then the value of the
MAX variant would be handled in switch cases.  The following
definition does not emit a warning with -Wswitch:

typedef enum ARMASIdx {
  ARMASIdx_NS = 0,
  ARMASIdx_S = 1,
  ARMASIdx_TagNS = 2,
  ARMASIdx_TagS = 3,
  ARMASIdx_MAX = ARMASIdx_TagS,
#define ARMASIdx_COUNT (ARMASIdx_MAX + 1)
} ARMASIdx;

int main() {
  ARMASIdx t = ARMASIdx_S;
  switch (t) {
  case ARMASIdx_NS:
    break;
  case ARMASIdx_S:
    break;
  case ARMASIdx_TagNS:
    break;
  case ARMASIdx_TagS:
    break;
  }
  printf("Last = %d Count = %d\n", ARMASIdx_MAX, ARMASIdx_COUNT);
}

Outputs:

"Last = 3 Count = 4"









> To avoid that we /define/ it manually instead:
>
> #define ARMASIdx_MAX 4
>
> >   } ARMASIdx;
>
> Usually the definition is within the enum declaration, and we name
> it ${enum}_COUNT:
>
>      typedef enum ARMASIdx {
>          ARMASIdx_NS = 0,
>          ARMASIdx_S = 1,
>          ARMASIdx_TagNS = 2,
>          ARMASIdx_TagS = 3,
>      #define ARMASIdx_COUNT 4
>      } ARMASIdx;
>
> Unfortunately this didn't work well with QAPI, so we could never enable
> -Wswitch globally:
> https://lore.kernel.org/qemu-devel/20230315112811.22355-1-philmd@linaro.org/
>
> Today I'm not sure what is the best style anymore, so just take
> my comments are historical 2 cents.
>
> Regards,
>
> Phil.
>

Re: [PATCH v2 2/6] target/arm: Add a _MAX sentinel to ARMASIdx enum

Posted by Gustavo Romero 1 month, 3 weeks ago

Hi Phil and Manos,

On 12/18/25 10:00, Manos Pitsidianakis wrote:
> On Thu, Dec 18, 2025 at 9:21 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Hi Gustavo,
>>
>> On 17/12/25 22:20, Gustavo Romero wrote:
>>> Add a sentinel to the ARMASIdx enum so it can be used when the total
>>> number of address spaces is required.
>>>
>>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>>> ---
>>>    target/arm/cpu.h | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 39f2b2e54d..00f5af0fcd 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -2336,6 +2336,7 @@ typedef enum ARMASIdx {
>>>        ARMASIdx_S = 1,
>>>        ARMASIdx_TagNS = 2,
>>>        ARMASIdx_TagS = 3,
>>> +    ARMASIdx_MAX
>>
> 
> My two cents:
> 
> Here, "ARMASIdx_MAX" should be equal to the last variant, 3. So to get
> the total count, it would be ARMASIdx_MAX + 1.
> And it should be called "ARMASIdx_COUNT". Max is the last variant.

That makes total sense to me. Thanks, I'll update it in v3.


>> The problem with including this in the enum is this confuses static
>> analyzers:
>>
>> warning: enumeration value 'ARMASIdx_MAX' not handled in switch [-Wswitch]
>>

hmm I guess I missed it because my test pipeline didn't check for it?

I've used QEMU_CI=2 to test the series. But I guess one needs access
to a pipeline with Coverity enabled? How can we test for warnings like
it in the CI?


> If we add a "MAX" instead of "COUNT" variant, then the value of the
> MAX variant would be handled in switch cases.  The following
> definition does not emit a warning with -Wswitch:
> 
> typedef enum ARMASIdx {
>    ARMASIdx_NS = 0,
>    ARMASIdx_S = 1,
>    ARMASIdx_TagNS = 2,
>    ARMASIdx_TagS = 3,
>    ARMASIdx_MAX = ARMASIdx_TagS,
> #define ARMASIdx_COUNT (ARMASIdx_MAX + 1)
> } ARMASIdx;
> 
> int main() {
>    ARMASIdx t = ARMASIdx_S;
>    switch (t) {
>    case ARMASIdx_NS:
>      break;
>    case ARMASIdx_S:
>      break;
>    case ARMASIdx_TagNS:
>      break;
>    case ARMASIdx_TagS:
>      break;
>    }
>    printf("Last = %d Count = %d\n", ARMASIdx_MAX, ARMASIdx_COUNT);
> }
> 
> Outputs:
> 
> "Last = 3 Count = 4"

Nice :)


Cheers,
Gustavo

> 
> 
> 
> 
> 
> 
> 
> 
>> To avoid that we /define/ it manually instead:
>>
>> #define ARMASIdx_MAX 4
>>
>>>    } ARMASIdx;
>>
>> Usually the definition is within the enum declaration, and we name
>> it ${enum}_COUNT:
>>
>>       typedef enum ARMASIdx {
>>           ARMASIdx_NS = 0,
>>           ARMASIdx_S = 1,
>>           ARMASIdx_TagNS = 2,
>>           ARMASIdx_TagS = 3,
>>       #define ARMASIdx_COUNT 4
>>       } ARMASIdx;
>>
>> Unfortunately this didn't work well with QAPI, so we could never enable
>> -Wswitch globally:
>> https://lore.kernel.org/qemu-devel/20230315112811.22355-1-philmd@linaro.org/
>>
>> Today I'm not sure what is the best style anymore, so just take
>> my comments are historical 2 cents.
>>
>> Regards,
>>
>> Phil.
>>