target/m68k: MacOS supervisor/user mode switch fixes

[PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Mark Cave-Ayland 3 years, 4 months ago

There are already 32 feature bits in use, so change the size of the m68k
CPU features to uint64_t (allong with the associated m68k_feature()
functions) to allow up to 64 feature bits to be used.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/m68k/cpu.c | 4 ++--
 target/m68k/cpu.h | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index f681be3a2a..7b4797e2f1 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
 
 static void m68k_set_feature(CPUM68KState *env, int feature)
 {
-    env->features |= (1u << feature);
+    env->features |= (1ul << feature);
 }
 
 static void m68k_unset_feature(CPUM68KState *env, int feature)
 {
-    env->features &= (-1u - (1u << feature));
+    env->features &= (-1ul - (1ul << feature));
 }
 
 static void m68k_cpu_reset(DeviceState *dev)
diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index 67b6c12c28..d3384e5d98 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -154,7 +154,7 @@ typedef struct CPUArchState {
     struct {} end_reset_fields;
 
     /* Fields from here on are preserved across CPU reset. */
-    uint32_t features;
+    uint64_t features;
 } CPUM68KState;
 
 /*
@@ -539,9 +539,9 @@ enum m68k_features {
     M68K_FEATURE_TRAPCC,
 };
 
-static inline int m68k_feature(CPUM68KState *env, int feature)
+static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
 {
-    return (env->features & (1u << feature)) != 0;
+    return (env->features & (1ul << feature)) != 0;
 }
 
 void m68k_cpu_list(void);
-- 
2.30.2

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by BALATON Zoltan 3 years, 4 months ago

On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
> There are already 32 feature bits in use, so change the size of the m68k
> CPU features to uint64_t (allong with the associated m68k_feature()
> functions) to allow up to 64 feature bits to be used.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> target/m68k/cpu.c | 4 ++--
> target/m68k/cpu.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index f681be3a2a..7b4797e2f1 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>
> static void m68k_set_feature(CPUM68KState *env, int feature)
> {
> -    env->features |= (1u << feature);
> +    env->features |= (1ul << feature);
> }
>
> static void m68k_unset_feature(CPUM68KState *env, int feature)
> {
> -    env->features &= (-1u - (1u << feature));
> +    env->features &= (-1ul - (1ul << feature));

Should these be ull instead of ul?

Regards,
BALATON Zoltan

> }
>
> static void m68k_cpu_reset(DeviceState *dev)
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index 67b6c12c28..d3384e5d98 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>     struct {} end_reset_fields;
>
>     /* Fields from here on are preserved across CPU reset. */
> -    uint32_t features;
> +    uint64_t features;
> } CPUM68KState;
>
> /*
> @@ -539,9 +539,9 @@ enum m68k_features {
>     M68K_FEATURE_TRAPCC,
> };
>
> -static inline int m68k_feature(CPUM68KState *env, int feature)
> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> {
> -    return (env->features & (1u << feature)) != 0;
> +    return (env->features & (1ul << feature)) != 0;
> }
>
> void m68k_cpu_list(void);
>

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Mark Cave-Ayland 3 years, 4 months ago

On 17/09/2022 13:09, BALATON Zoltan wrote:

> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>> There are already 32 feature bits in use, so change the size of the m68k
>> CPU features to uint64_t (allong with the associated m68k_feature()
>> functions) to allow up to 64 feature bits to be used.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> target/m68k/cpu.c | 4 ++--
>> target/m68k/cpu.h | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index f681be3a2a..7b4797e2f1 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>
>> static void m68k_set_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features |= (1u << feature);
>> +    env->features |= (1ul << feature);
>> }
>>
>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features &= (-1u - (1u << feature));
>> +    env->features &= (-1ul - (1ul << feature));
> 
> Should these be ull instead of ul?

Indeed, it looks like Windows needs ULL in order to work correctly with uint64_t - I 
can easily fix that in v2.

>> }
>>
>> static void m68k_cpu_reset(DeviceState *dev)
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index 67b6c12c28..d3384e5d98 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>     struct {} end_reset_fields;
>>
>>     /* Fields from here on are preserved across CPU reset. */
>> -    uint32_t features;
>> +    uint64_t features;
>> } CPUM68KState;
>>
>> /*
>> @@ -539,9 +539,9 @@ enum m68k_features {
>>     M68K_FEATURE_TRAPCC,
>> };
>>
>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>> {
>> -    return (env->features & (1u << feature)) != 0;
>> +    return (env->features & (1ul << feature)) != 0;
>> }
>>
>> void m68k_cpu_list(void);


ATB,

Mark.

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Philippe Mathieu-Daudé via 3 years, 4 months ago

On 17/9/22 14:09, BALATON Zoltan wrote:
> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>> There are already 32 feature bits in use, so change the size of the m68k
>> CPU features to uint64_t (allong with the associated m68k_feature()
>> functions) to allow up to 64 feature bits to be used.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> target/m68k/cpu.c | 4 ++--
>> target/m68k/cpu.h | 6 +++---
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>> index f681be3a2a..7b4797e2f1 100644
>> --- a/target/m68k/cpu.c
>> +++ b/target/m68k/cpu.c
>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>
>> static void m68k_set_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features |= (1u << feature);
>> +    env->features |= (1ul << feature);

         env->features = deposit64(env->features, feature, 1, 1);

>> }
>>
>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>> {
>> -    env->features &= (-1u - (1u << feature));
>> +    env->features &= (-1ul - (1ul << feature));

         env->features = deposit64(env->features, feature, 1, 0);

> Should these be ull instead of ul?

Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.

>> }
>>
>> static void m68k_cpu_reset(DeviceState *dev)
>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>> index 67b6c12c28..d3384e5d98 100644
>> --- a/target/m68k/cpu.h
>> +++ b/target/m68k/cpu.h
>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>     struct {} end_reset_fields;
>>
>>     /* Fields from here on are preserved across CPU reset. */
>> -    uint32_t features;
>> +    uint64_t features;
>> } CPUM68KState;
>>
>> /*
>> @@ -539,9 +539,9 @@ enum m68k_features {
>>     M68K_FEATURE_TRAPCC,
>> };
>>
>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)

Why uint64_t? Can we simplify using a boolean?

>> {
>> -    return (env->features & (1u << feature)) != 0;
>> +    return (env->features & (1ul << feature)) != 0;

         return extract64(env->features, feature, 1) == 1;

>> }
>>
>> void m68k_cpu_list(void);
>>
>

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Mark Cave-Ayland 3 years, 4 months ago

On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:

> On 17/9/22 14:09, BALATON Zoltan wrote:
>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>> There are already 32 feature bits in use, so change the size of the m68k
>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>> functions) to allow up to 64 feature bits to be used.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> target/m68k/cpu.c | 4 ++--
>>> target/m68k/cpu.h | 6 +++---
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>> index f681be3a2a..7b4797e2f1 100644
>>> --- a/target/m68k/cpu.c
>>> +++ b/target/m68k/cpu.c
>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>
>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>> {
>>> -    env->features |= (1u << feature);
>>> +    env->features |= (1ul << feature);
> 
>          env->features = deposit64(env->features, feature, 1, 1);
> 
>>> }
>>>
>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>> {
>>> -    env->features &= (-1u - (1u << feature));
>>> +    env->features &= (-1ul - (1ul << feature));
> 
>          env->features = deposit64(env->features, feature, 1, 0);
> 
>> Should these be ull instead of ul?
> 
> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.

I must admit I find the deposit64() variants not particularly easy to read: if we're 
considering alterations rather than changing the constant suffix then I'd much rather 
go for:

     env->features |= (1ULL << feature);

and:

     env->features &= ~(1ULL << feature);

Laurent, what would be your preference?

>>> }
>>>
>>> static void m68k_cpu_reset(DeviceState *dev)
>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>> index 67b6c12c28..d3384e5d98 100644
>>> --- a/target/m68k/cpu.h
>>> +++ b/target/m68k/cpu.h
>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>     struct {} end_reset_fields;
>>>
>>>     /* Fields from here on are preserved across CPU reset. */
>>> -    uint32_t features;
>>> +    uint64_t features;
>>> } CPUM68KState;
>>>
>>> /*
>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>     M68K_FEATURE_TRAPCC,
>>> };
>>>
>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> 
> Why uint64_t? Can we simplify using a boolean?

I don't really feel strongly either way here. Again I'm happy to go with whatever 
Laurent would prefer as maintainer.

>>> {
>>> -    return (env->features & (1u << feature)) != 0;
>>> +    return (env->features & (1ul << feature)) != 0;
> 
>          return extract64(env->features, feature, 1) == 1;
> 
>>> }
>>>
>>> void m68k_cpu_list(void);


ATB,

Mark.

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Laurent Vivier 3 years, 4 months ago

Le 20/09/2022 à 18:30, Mark Cave-Ayland a écrit :
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
> 
>> On 17/9/22 14:09, BALATON Zoltan wrote:
>>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>>> There are already 32 feature bits in use, so change the size of the m68k
>>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>>> functions) to allow up to 64 feature bits to be used.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> target/m68k/cpu.c | 4 ++--
>>>> target/m68k/cpu.h | 6 +++---
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>>> index f681be3a2a..7b4797e2f1 100644
>>>> --- a/target/m68k/cpu.c
>>>> +++ b/target/m68k/cpu.c
>>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>>
>>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features |= (1u << feature);
>>>> +    env->features |= (1ul << feature);
>>
>>          env->features = deposit64(env->features, feature, 1, 1);
>>
>>>> }
>>>>
>>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features &= (-1u - (1u << feature));
>>>> +    env->features &= (-1ul - (1ul << feature));
>>
>>          env->features = deposit64(env->features, feature, 1, 0);
>>
>>> Should these be ull instead of ul?
>>
>> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
> 
> I must admit I find the deposit64() variants not particularly easy to read: if we're considering 
> alterations rather than changing the constant suffix then I'd much rather go for:
> 
>      env->features |= (1ULL << feature);
> 
> and:
> 
>      env->features &= ~(1ULL << feature);
> 
> Laurent, what would be your preference?

I have no preference, do as you prefer.

> 
>>>> }
>>>>
>>>> static void m68k_cpu_reset(DeviceState *dev)
>>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>>> index 67b6c12c28..d3384e5d98 100644
>>>> --- a/target/m68k/cpu.h
>>>> +++ b/target/m68k/cpu.h
>>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>>     struct {} end_reset_fields;
>>>>
>>>>     /* Fields from here on are preserved across CPU reset. */
>>>> -    uint32_t features;
>>>> +    uint64_t features;
>>>> } CPUM68KState;
>>>>
>>>> /*
>>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>>     M68K_FEATURE_TRAPCC,
>>>> };
>>>>
>>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>>
>> Why uint64_t? Can we simplify using a boolean?
> 
> I don't really feel strongly either way here. Again I'm happy to go with whatever Laurent would 
> prefer as maintainer.

A boolean seems more logic, I think.

Thanks,
Laurent

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by BALATON Zoltan 3 years, 4 months ago

On Tue, 20 Sep 2022, Mark Cave-Ayland wrote:
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
>
>> On 17/9/22 14:09, BALATON Zoltan wrote:
>>> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
>>>> There are already 32 feature bits in use, so change the size of the m68k
>>>> CPU features to uint64_t (allong with the associated m68k_feature()
>>>> functions) to allow up to 64 feature bits to be used.
>>>> 
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> target/m68k/cpu.c | 4 ++--
>>>> target/m68k/cpu.h | 6 +++---
>>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>> 
>>>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
>>>> index f681be3a2a..7b4797e2f1 100644
>>>> --- a/target/m68k/cpu.c
>>>> +++ b/target/m68k/cpu.c
>>>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
>>>> 
>>>> static void m68k_set_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features |= (1u << feature);
>>>> +    env->features |= (1ul << feature);
>>
>>          env->features = deposit64(env->features, feature, 1, 1);
>> 
>>>> }
>>>> 
>>>> static void m68k_unset_feature(CPUM68KState *env, int feature)
>>>> {
>>>> -    env->features &= (-1u - (1u << feature));
>>>> +    env->features &= (-1ul - (1ul << feature));
>>
>>          env->features = deposit64(env->features, feature, 1, 0);
>> 
>>> Should these be ull instead of ul?
>> 
>> Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
>
> I must admit I find the deposit64() variants not particularly easy to read:

I agree with that and also dislike the dposit/extract functions that would 
not bring much here. Maybe they are useful for multiple bits but for a 
single bit they just add overhead and obfuscation.

> if we're considering alterations rather than changing the constant suffix 
> then I'd much rather go for:
>
>    env->features |= (1ULL << feature);
>
> and:
>
>    env->features &= ~(1ULL << feature);

There's also a BIT_ULL macro which could be used but it's up to you, 
shifting 1ULL is also simple enough to read.

Regards,
BALATON Zoltan

> Laurent, what would be your preference?
>
>>>> }
>>>> 
>>>> static void m68k_cpu_reset(DeviceState *dev)
>>>> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
>>>> index 67b6c12c28..d3384e5d98 100644
>>>> --- a/target/m68k/cpu.h
>>>> +++ b/target/m68k/cpu.h
>>>> @@ -154,7 +154,7 @@ typedef struct CPUArchState {
>>>>     struct {} end_reset_fields;
>>>> 
>>>>     /* Fields from here on are preserved across CPU reset. */
>>>> -    uint32_t features;
>>>> +    uint64_t features;
>>>> } CPUM68KState;
>>>> 
>>>> /*
>>>> @@ -539,9 +539,9 @@ enum m68k_features {
>>>>     M68K_FEATURE_TRAPCC,
>>>> };
>>>> 
>>>> -static inline int m68k_feature(CPUM68KState *env, int feature)
>>>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
>> 
>> Why uint64_t? Can we simplify using a boolean?
>
> I don't really feel strongly either way here. Again I'm happy to go with 
> whatever Laurent would prefer as maintainer.
>
>>>> {
>>>> -    return (env->features & (1u << feature)) != 0;
>>>> +    return (env->features & (1ul << feature)) != 0;
>>
>>          return extract64(env->features, feature, 1) == 1;
>> 
>>>> }
>>>> 
>>>> void m68k_cpu_list(void);
>
>
> ATB,
>
> Mark.
>
>

Re: [PATCH 2/4] target/m68k: increase size of m68k CPU features from uint32_t to uint64_t

Posted by Philippe Mathieu-Daudé via 3 years, 4 months ago

On Tue, Sep 20, 2022 at 6:30 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> On 17/09/2022 23:27, Philippe Mathieu-Daudé via wrote:
> > On 17/9/22 14:09, BALATON Zoltan wrote:
> >> On Sat, 17 Sep 2022, Mark Cave-Ayland wrote:
> >>> There are already 32 feature bits in use, so change the size of the m68k
> >>> CPU features to uint64_t (allong with the associated m68k_feature()
> >>> functions) to allow up to 64 feature bits to be used.
> >>>
> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >>> ---
> >>> target/m68k/cpu.c | 4 ++--
> >>> target/m68k/cpu.h | 6 +++---
> >>> 2 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> >>> index f681be3a2a..7b4797e2f1 100644
> >>> --- a/target/m68k/cpu.c
> >>> +++ b/target/m68k/cpu.c
> >>> @@ -38,12 +38,12 @@ static bool m68k_cpu_has_work(CPUState *cs)
> >>>
> >>> static void m68k_set_feature(CPUM68KState *env, int feature)
> >>> {
> >>> -    env->features |= (1u << feature);
> >>> +    env->features |= (1ul << feature);
> >
> >          env->features = deposit64(env->features, feature, 1, 1);
> >
> >>> }
> >>>
> >>> static void m68k_unset_feature(CPUM68KState *env, int feature)
> >>> {
> >>> -    env->features &= (-1u - (1u << feature));
> >>> +    env->features &= (-1ul - (1ul << feature));
> >
> >          env->features = deposit64(env->features, feature, 1, 0);
> >
> >> Should these be ull instead of ul?
> >
> > Yes. Not needed if using the <qemu/bitops.h> extract/deposit API.
>
> I must admit I find the deposit64() variants not particularly easy to read: if we're
> considering alterations rather than changing the constant suffix then I'd much rather
> go for:
>
>      env->features |= (1ULL << feature);
>
> and:
>
>      env->features &= ~(1ULL << feature);
>
> Laurent, what would be your preference?

OK, no need to change then.

> >>> -static inline int m68k_feature(CPUM68KState *env, int feature)
> >>> +static inline uint64_t m68k_feature(CPUM68KState *env, int feature)
> >
> > Why uint64_t? Can we simplify using a boolean?
>
> I don't really feel strongly either way here. Again I'm happy to go with whatever
> Laurent would prefer as maintainer.

Preferably using boolean:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>