Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Cédric Le Goater]

Hello Vitaly

On 1/21/22 01:02, Vitaly Cheptsov wrote:
> Hello,
> 
> PowerPC e500mc defines MSR bit 35 differently from most other PowerPC variants. In particular, for e500mc this is GS (Guest Supervisor) bit[1], while for others it is NOEXEC GUARD bit[2].
> QEMU ignores this architectural difference when handling the exceptions of attempting to run not executable code on e500mc, and mistakenly sets the GS bit[3][4].
> Setting this bit eventually leads to crashes, because although QEMU does not support Guest Supervisor mode on e500mc, it still requires it to be disabled[5].

This looks correct and even fixing an issue that Mario reported
on the TCG e6500 CPU with a kernel + KVM compiled in :

   https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/

KVM has some issues also with the e6500 but that's another problem
I think.


What is your environment ? Which QEMU machine ? Can you provide a
command line ?

Could you please resend the patch in a non attached way ?  See :

   https://www.qemu.org/docs/master/devel/submitting-a-patch.html

and copy qemu-devel.

Thanks,

C.


> Best regards,
> Vitaly
> 
> [1] https://www.nxp.com/docs/en/reference-manual/E500MCRM.pdf, 2.7.1 MSR
> [2] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0, 6.5.5 Instruction Storage Interrupt
> [3] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_common.c#L1426
> [4] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/excp_helper.c#L414-L416
> [5] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_helper.c#L1078-L1080
> 

Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 2805 bytes --]

Hi Cédric,

> This looks correct and even fixing an issue that Mario reported
> on the TCG e6500 CPU with a kernel + KVM compiled in :
> 
>  https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
> 
> KVM has some issues also with the e6500 but that's another problem
> I think.

Glad to hear that. Could you schedule the inclusion of the patch in 6.2.1 or 6.3 please?

> What is your environment ? Which QEMU machine ? Can you provide a
> command line ?

We have an in-house RTOS at ISP RAS, which we use to run some environmental tests on QEMU. The target hardware in this particular example is a QorIQ P3041-based board. The command line approximately looks like this:
qemu-system-ppc -cpu e500mc -M ppce500 -m 128M -icount 1 -kernel /path/to/kernel.elf -serial tcp::1111,server,nodelay

> Could you please resend the patch in a non attached way ?  See :
> 
>  https://www.qemu.org/docs/master/devel/submitting-a-patch.html
> 
> and copy qemu-devel.

Yes, sure. Have just done that.

Best regards,
Vitaly

> On 21 Jan 2022, at 11:17, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello Vitaly
> 
> On 1/21/22 01:02, Vitaly Cheptsov wrote:
>> Hello,
>> PowerPC e500mc defines MSR bit 35 differently from most other PowerPC variants. In particular, for e500mc this is GS (Guest Supervisor) bit[1], while for others it is NOEXEC GUARD bit[2].
>> QEMU ignores this architectural difference when handling the exceptions of attempting to run not executable code on e500mc, and mistakenly sets the GS bit[3][4].
>> Setting this bit eventually leads to crashes, because although QEMU does not support Guest Supervisor mode on e500mc, it still requires it to be disabled[5].
> 
> This looks correct and even fixing an issue that Mario reported
> on the TCG e6500 CPU with a kernel + KVM compiled in :
> 
>  https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
> 
> KVM has some issues also with the e6500 but that's another problem
> I think.
> 
> 
> What is your environment ? Which QEMU machine ? Can you provide a
> command line ?
> 
> Could you please resend the patch in a non attached way ?  See :
> 
>  https://www.qemu.org/docs/master/devel/submitting-a-patch.html
> 
> and copy qemu-devel.
> 
> Thanks,
> 
> C.
> 
> 
>> Best regards,
>> Vitaly
>> [1] https://www.nxp.com/docs/en/reference-manual/E500MCRM.pdf, 2.7.1 MSR
>> [2] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0, 6.5.5 Instruction Storage Interrupt
>> [3] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_common.c#L1426
>> [4] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/excp_helper.c#L414-L416
>> [5] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_helper.c#L1078-L1080
> 


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Cédric Le Goater]

Hello Vitaly,

On 1/21/22 10:33, Vitaly Cheptsov wrote:
> Hi Cédric,
> 
>> This looks correct and even fixing an issue that Mario reported
>> on the TCG e6500 CPU with a kernel + KVM compiled in :
>>
>>   https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
>>
>> KVM has some issues also with the e6500 but that's another problem
>> I think.
> 
> Glad to hear that. Could you schedule the inclusion of the patch in 6.2.1 or 6.3 please?

7.0 it should be.

>> What is your environment ? Which QEMU machine ? Can you provide a
>> command line ?
> 
> We have an in-house RTOS at ISP RAS, which we use to run some environmental tests on QEMU.

not a Linux. Diversity is good for the models.

> The target hardware in this particular example is a QorIQ P3041-based board. 

and the default ppce500 machine has enough devices for the purpose ?

Thanks,

C.

> The command line approximately looks like this:
> qemu-system-ppc -cpu e500mc -M ppce500 -m 128M -icount 1 -kernel /path/to/kernel.elf -serial tcp::1111,server,nodelay
> 
>> Could you please resend the patch in a non attached way ?  See :
>>
>>   https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>>
>> and copy qemu-devel.
> 
> Yes, sure. Have just done that.
> 
> Best regards,
> Vitaly
> 
>> On 21 Jan 2022, at 11:17, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello Vitaly
>>
>> On 1/21/22 01:02, Vitaly Cheptsov wrote:
>>> Hello,
>>> PowerPC e500mc defines MSR bit 35 differently from most other PowerPC variants. In particular, for e500mc this is GS (Guest Supervisor) bit[1], while for others it is NOEXEC GUARD bit[2].
>>> QEMU ignores this architectural difference when handling the exceptions of attempting to run not executable code on e500mc, and mistakenly sets the GS bit[3][4].
>>> Setting this bit eventually leads to crashes, because although QEMU does not support Guest Supervisor mode on e500mc, it still requires it to be disabled[5].
>>
>> This looks correct and even fixing an issue that Mario reported
>> on the TCG e6500 CPU with a kernel + KVM compiled in :
>>
>>   https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
>>
>> KVM has some issues also with the e6500 but that's another problem
>> I think.
>>
>>
>> What is your environment ? Which QEMU machine ? Can you provide a
>> command line ?
>>
>> Could you please resend the patch in a non attached way ?  See :
>>
>>   https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>>
>> and copy qemu-devel.
>>
>> Thanks,
>>
>> C.
>>
>>
>>> Best regards,
>>> Vitaly
>>> [1] https://www.nxp.com/docs/en/reference-manual/E500MCRM.pdf, 2.7.1 MSR
>>> [2] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0, 6.5.5 Instruction Storage Interrupt
>>> [3] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_common.c#L1426
>>> [4] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/excp_helper.c#L414-L416
>>> [5] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_helper.c#L1078-L1080
>>
> 

Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Vitaly Cheptsov]

[-- Attachment #1: Type: text/plain, Size: 3586 bytes --]

Hi Cédric,

> and the default ppce500 machine has enough devices for the purpose ?

We cannot test much without a predictable timer emulation on QEMU, sometimes we have fairly random freezes, but otherwise the basics work ok. I will let you know in case we find something more or less reproducible.

Best,
Vitaly

> On 21 Jan 2022, at 20:33, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Hello Vitaly,
> 
> On 1/21/22 10:33, Vitaly Cheptsov wrote:
>> Hi Cédric,
>>> This looks correct and even fixing an issue that Mario reported
>>> on the TCG e6500 CPU with a kernel + KVM compiled in :
>>> 
>>>  https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
>>> 
>>> KVM has some issues also with the e6500 but that's another problem
>>> I think.
>> Glad to hear that. Could you schedule the inclusion of the patch in 6.2.1 or 6.3 please?
> 
> 7.0 it should be.
> 
>>> What is your environment ? Which QEMU machine ? Can you provide a
>>> command line ?
>> We have an in-house RTOS at ISP RAS, which we use to run some environmental tests on QEMU.
> 
> not a Linux. Diversity is good for the models.
> 
>> The target hardware in this particular example is a QorIQ P3041-based board.
> 
> and the default ppce500 machine has enough devices for the purpose ?
> 
> Thanks,
> 
> C.
> 
>> The command line approximately looks like this:
>> qemu-system-ppc -cpu e500mc -M ppce500 -m 128M -icount 1 -kernel /path/to/kernel.elf -serial tcp::1111,server,nodelay
>>> Could you please resend the patch in a non attached way ?  See :
>>> 
>>>  https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>>> 
>>> and copy qemu-devel.
>> Yes, sure. Have just done that.
>> Best regards,
>> Vitaly
>>> On 21 Jan 2022, at 11:17, Cédric Le Goater <clg@kaod.org> wrote:
>>> 
>>> Hello Vitaly
>>> 
>>> On 1/21/22 01:02, Vitaly Cheptsov wrote:
>>>> Hello,
>>>> PowerPC e500mc defines MSR bit 35 differently from most other PowerPC variants. In particular, for e500mc this is GS (Guest Supervisor) bit[1], while for others it is NOEXEC GUARD bit[2].
>>>> QEMU ignores this architectural difference when handling the exceptions of attempting to run not executable code on e500mc, and mistakenly sets the GS bit[3][4].
>>>> Setting this bit eventually leads to crashes, because although QEMU does not support Guest Supervisor mode on e500mc, it still requires it to be disabled[5].
>>> 
>>> This looks correct and even fixing an issue that Mario reported
>>> on the TCG e6500 CPU with a kernel + KVM compiled in :
>>> 
>>>  https://lore.kernel.org/all/R5JFVM$911E343FF81933B99D53FD0992D8848F@locati.it/
>>> 
>>> KVM has some issues also with the e6500 but that's another problem
>>> I think.
>>> 
>>> 
>>> What is your environment ? Which QEMU machine ? Can you provide a
>>> command line ?
>>> 
>>> Could you please resend the patch in a non attached way ?  See :
>>> 
>>>  https://www.qemu.org/docs/master/devel/submitting-a-patch.html
>>> 
>>> and copy qemu-devel.
>>> 
>>> Thanks,
>>> 
>>> C.
>>> 
>>> 
>>>> Best regards,
>>>> Vitaly
>>>> [1] https://www.nxp.com/docs/en/reference-manual/E500MCRM.pdf, 2.7.1 MSR
>>>> [2] https://openpowerfoundation.org/?resource_lib=power-isa-version-3-0, 6.5.5 Instruction Storage Interrupt
>>>> [3] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_common.c#L1426
>>>> [4] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/excp_helper.c#L414-L416
>>>> [5] https://github.com/qemu/qemu/blob/v6.2.0/target/ppc/mmu_helper.c#L1078-L1080
>>> 
> 


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Cédric Le Goater]

On 1/21/22 10:31, Vitaly Cheptsov wrote:
> Book-E architecture does not set the error code in 31:27 bits
> of SRR1, but instead uses these bits for custom fields such
> as GS (Guest Supervisor).
> 
> Wrongly setting these fields will result in QEMU crashes
> when attempting to execute not executable code due to the attempts
> to use Guest Supervisor mode.
> 
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Greg Kurz <groug@kaod.org>
> Cc: qemu-ppc@nongnu.org
> Cc: qemu-devel@nongnu.org
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   target/ppc/mmu_common.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
> index 91270c1f17..6512ee031c 100644
> --- a/target/ppc/mmu_common.c
> +++ b/target/ppc/mmu_common.c
> @@ -1367,22 +1367,34 @@ static bool ppc_jumbo_xlate(PowerPCCPU *cpu, vaddr eaddr,
>               case -2:
>                   /* Access rights violation */
>                   cs->exception_index = POWERPC_EXCP_ISI;
> -                env->error_code = 0x08000000;
> +                if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
> +                    (env->mmu_model == POWERPC_MMU_BOOKE206)) {
> +                    env->error_code = 0;
> +                } else {
> +                    env->error_code = 0x08000000;
> +                }
>                   break;
>               case -3:
>                   /* No execute protection violation */
>                   if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
>                       (env->mmu_model == POWERPC_MMU_BOOKE206)) {
>                       env->spr[SPR_BOOKE_ESR] = 0x00000000;
> +                    env->error_code = 0;
> +                } else {
> +                    env->error_code = 0x10000000;
>                   }
>                   cs->exception_index = POWERPC_EXCP_ISI;
> -                env->error_code = 0x10000000;
>                   break;
>               case -4:
>                   /* Direct store exception */
>                   /* No code fetch is allowed in direct-store areas */
>                   cs->exception_index = POWERPC_EXCP_ISI;
> -                env->error_code = 0x10000000;
> +                if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
> +                    (env->mmu_model == POWERPC_MMU_BOOKE206)) {
> +                    env->error_code = 0;
> +                } else {
> +                    env->error_code = 0x10000000;
> +                }
>                   break;
>               }
>           } else {
> 

[PATCH] target/ppc/mmu_common: Fix SRR1/MSR error code on Book-E

[Vitaly Cheptsov]

Book-E architecture does not set the error code in 31:27 bits
of SRR1, but instead uses these bits for custom fields such
as GS (Guest Supervisor).

Wrongly setting these fields will result in QEMU crashes
when attempting to execute not executable code due to the attempts
to use Guest Supervisor mode.

Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Greg Kurz <groug@kaod.org>
Cc: qemu-ppc@nongnu.org
Cc: qemu-devel@nongnu.org
Cc: qemu-stable@nongnu.org
Signed-off-by: Vitaly Cheptsov <cheptsov@ispras.ru>
---
 target/ppc/mmu_common.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/target/ppc/mmu_common.c b/target/ppc/mmu_common.c
index 91270c1f17..6512ee031c 100644
--- a/target/ppc/mmu_common.c
+++ b/target/ppc/mmu_common.c
@@ -1367,22 +1367,34 @@ static bool ppc_jumbo_xlate(PowerPCCPU *cpu, vaddr eaddr,
             case -2:
                 /* Access rights violation */
                 cs->exception_index = POWERPC_EXCP_ISI;
-                env->error_code = 0x08000000;
+                if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
+                    (env->mmu_model == POWERPC_MMU_BOOKE206)) {
+                    env->error_code = 0;
+                } else {
+                    env->error_code = 0x08000000;
+                }
                 break;
             case -3:
                 /* No execute protection violation */
                 if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
                     (env->mmu_model == POWERPC_MMU_BOOKE206)) {
                     env->spr[SPR_BOOKE_ESR] = 0x00000000;
+                    env->error_code = 0;
+                } else {
+                    env->error_code = 0x10000000;
                 }
                 cs->exception_index = POWERPC_EXCP_ISI;
-                env->error_code = 0x10000000;
                 break;
             case -4:
                 /* Direct store exception */
                 /* No code fetch is allowed in direct-store areas */
                 cs->exception_index = POWERPC_EXCP_ISI;
-                env->error_code = 0x10000000;
+                if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
+                    (env->mmu_model == POWERPC_MMU_BOOKE206)) {
+                    env->error_code = 0;
+                } else {
+                    env->error_code = 0x10000000;
+                }
                 break;
             }
         } else {
-- 
2.32.0 (Apple Git-132)