[Qemu-trivial] [PATCH] PPC: MMU compatibility check.

[Qemu-trivial] [PATCH] PPC: MMU compatibility check.

[Valentin Plotkin]

Hi everyone,

I looked at the "qemu-system-ppc -nographic -cpu G2leGP3 -M ppce500" on 
the BiteSizedTasks page. The segfault was caused by machine initialization 
code which expected a certain MMU model, checked, so unused SPR were read, 
returning zeros. bamboo and virtex machines are affected as well, but it 
doesn't always cause segfault, usually running into unmapped memory and 
failing somewhat more nicely.

I added the checks. It would be possible to add support for other MMU 
models, but I'm not sure if there is any point (would any guest OS support 
mutually exclusive CPU and machine)?

---
  hw/ppc/e500.c          | 5 +++++
  hw/ppc/ppc440_bamboo.c | 5 +++++
  hw/ppc/virtex_ml507.c  | 5 +++++
  3 files changed, 15 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index cf8b122..cd352f6 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -631,6 +631,11 @@ static uint64_t mmubooke_initial_mapsize(CPUPPCState 
*env)

  static void mmubooke_create_initial_mapping(CPUPPCState *env)
  {
+    if(env->mmu_model!=POWERPC_MMU_BOOKE206){
+        fprintf(stderr,"MMU model %i not supported by this machine.\n", 
env->mmu_model);
+        exit(-1);
+    }
+
      ppcmas_tlb_t *tlb = booke206_get_tlbm(env, 1, 0, 0);
      hwaddr size;
      int ps;
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 5c535b1..61e7028 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -124,6 +124,11 @@ static void 
mmubooke_create_initial_mapping(CPUPPCState *env,
                                       target_ulong va,
                                       hwaddr pa)
  {
+    if(env->mmu_model!=POWERPC_MMU_BOOKE){
+        fprintf(stderr,"MMU model %i not supported by this machine.\n", 
env->mmu_model);
+        exit(-1);
+    }
+
      ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];

      tlb->attr = 0;
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index b97d966..4041ff3 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -69,6 +69,11 @@ static void mmubooke_create_initial_mapping(CPUPPCState 
*env,
                                       target_ulong va,
                                       hwaddr pa)
  {
+    if(env->mmu_model!=POWERPC_MMU_BOOKE){
+        fprintf(stderr,"MMU model %i not supported by this machine.\n", 
env->mmu_model);
+        exit(-1);
+    }
+
      ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];

      tlb->attr = 0;
-- 
2.5.5

Valentin Plotkin
caliborn@sdf.org
SDF Public Access UNIX System - http://sdf.org

Re: [Qemu-trivial] [Qemu-ppc] [PATCH] PPC: MMU compatibility check.

[Thomas Huth]

On 24.01.2017 19:56, Valentin Plotkin wrote:
> 
> Hi everyone,
> 
> I looked at the "qemu-system-ppc -nographic -cpu G2leGP3 -M ppce500" on
> the BiteSizedTasks page. The segfault was caused by machine
> initialization code which expected a certain MMU model, checked, so
> unused SPR were read, returning zeros. bamboo and virtex machines are
> affected as well, but it doesn't always cause segfault, usually running
> into unmapped memory and failing somewhat more nicely.
> 
> I added the checks. It would be possible to add support for other MMU
> models, but I'm not sure if there is any point (would any guest OS
> support mutually exclusive CPU and machine)?

 Hi,

great to have a fix for this crash! I don't think it make much sense to
add support for other MMU models here, so the simple checks should be
good enough.
However, your new code obviously does not follow the QEMU coding style.
Could you please run your patch through scripts/checkpatch.pl and fix
all issues that it reports? And when you resubmit, please make sure to
copy the maintainers on CC: as well (scripts/get_maintainers.pl is your
friend here).

 Thanks,
  Thomas

Re: [Qemu-trivial] [Qemu-ppc] [PATCH] PPC: MMU compatibility check.

[Valentin Plotkin]

On Tue, 24 Jan 2017, Thomas Huth wrote:

> Date: Tue, 24 Jan 2017 21:32:44 +0100
> From: Thomas Huth <thuth@redhat.com>
> To: Valentin Plotkin <caliborn@SDF.ORG>, qemu-trivial@nongnu.org
> Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org
> Subject: Re: [Qemu-ppc] [PATCH] PPC: MMU compatibility check.
> 
> On 24.01.2017 19:56, Valentin Plotkin wrote:
>>
>> Hi everyone,
>>
>> I looked at the "qemu-system-ppc -nographic -cpu G2leGP3 -M ppce500" on
>> the BiteSizedTasks page. The segfault was caused by machine
>> initialization code which expected a certain MMU model, checked, so
>> unused SPR were read, returning zeros. bamboo and virtex machines are
>> affected as well, but it doesn't always cause segfault, usually running
>> into unmapped memory and failing somewhat more nicely.
>>
>> I added the checks. It would be possible to add support for other MMU
>> models, but I'm not sure if there is any point (would any guest OS
>> support mutually exclusive CPU and machine)?
>
> Hi,
>
> great to have a fix for this crash! I don't think it make much sense to
> add support for other MMU models here, so the simple checks should be
> good enough.
> However, your new code obviously does not follow the QEMU coding style.
> Could you please run your patch through scripts/checkpatch.pl and fix
> all issues that it reports? And when you resubmit, please make sure to
> copy the maintainers on CC: as well (scripts/get_maintainers.pl is your
> friend here).
>
> Thanks,
>  Thomas
>
>

Here is fengshuised version (at least I hope so). Thanks for guiding me.

Signed-off-by: Valentin Plotkin <caliborn@sdf.org>
---
  hw/ppc/e500.c          | 6 ++++++
  hw/ppc/ppc440_bamboo.c | 6 ++++++
  hw/ppc/virtex_ml507.c  | 6 ++++++
  3 files changed, 18 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index cf8b122..683d9a9 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -631,6 +631,12 @@ static uint64_t mmubooke_initial_mapsize(CPUPPCState 
*env)

  static void mmubooke_create_initial_mapping(CPUPPCState *env)
  {
+    if (env->mmu_model != POWERPC_MMU_BOOKE206) {
+        fprintf(stderr, "MMU model %i not supported by this machine.\n",
+            env->mmu_model);
+        exit(-1);
+    }
+
      ppcmas_tlb_t *tlb = booke206_get_tlbm(env, 1, 0, 0);
      hwaddr size;
      int ps;
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 5c535b1..793b758 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -124,6 +124,12 @@ static void 
mmubooke_create_initial_mapping(CPUPPCState *env,
                                       target_ulong va,
                                       hwaddr pa)
  {
+    if (env->mmu_model != POWERPC_MMU_BOOKE) {
+        fprintf(stderr, "MMU model %i not supported by this machine.\n",
+            env->mmu_model);
+        exit(-1);
+    }
+
      ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];

      tlb->attr = 0;
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index b97d966..c01415c 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -69,6 +69,12 @@ static void mmubooke_create_initial_mapping(CPUPPCState 
*env,
                                       target_ulong va,
                                       hwaddr pa)
  {
+    if (env->mmu_model != POWERPC_MMU_BOOKE) {
+        fprintf(stderr, "MMU model %i not supported by this machine.\n",
+            env->mmu_model);
+        exit(-1);
+    }
+
      ppcemb_tlb_t *tlb = &env->tlb.tlbe[0];

      tlb->attr = 0;
-- 
2.5.5

caliborn@sdf.org
SDF Public Access UNIX System - http://sdf.org

Re: [Qemu-trivial] [Qemu-ppc] [PATCH] PPC: MMU compatibility check.

[Thomas Huth]

On 24.01.2017 22:41, Valentin Plotkin wrote:
> On Tue, 24 Jan 2017, Thomas Huth wrote:
> 
>> Date: Tue, 24 Jan 2017 21:32:44 +0100
>> From: Thomas Huth <thuth@redhat.com>
>> To: Valentin Plotkin <caliborn@SDF.ORG>, qemu-trivial@nongnu.org
>> Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org
>> Subject: Re: [Qemu-ppc] [PATCH] PPC: MMU compatibility check.
>>
>> On 24.01.2017 19:56, Valentin Plotkin wrote:
>>>
>>> Hi everyone,
>>>
>>> I looked at the "qemu-system-ppc -nographic -cpu G2leGP3 -M ppce500" on
>>> the BiteSizedTasks page. The segfault was caused by machine
>>> initialization code which expected a certain MMU model, checked, so
>>> unused SPR were read, returning zeros. bamboo and virtex machines are
>>> affected as well, but it doesn't always cause segfault, usually running
>>> into unmapped memory and failing somewhat more nicely.
>>>
>>> I added the checks. It would be possible to add support for other MMU
>>> models, but I'm not sure if there is any point (would any guest OS
>>> support mutually exclusive CPU and machine)?
>>
>> Hi,
>>
>> great to have a fix for this crash! I don't think it make much sense to
>> add support for other MMU models here, so the simple checks should be
>> good enough.
>> However, your new code obviously does not follow the QEMU coding style.
>> Could you please run your patch through scripts/checkpatch.pl and fix
>> all issues that it reports? And when you resubmit, please make sure to
>> copy the maintainers on CC: as well (scripts/get_maintainers.pl is your
>> friend here).
> 
> Here is fengshuised version (at least I hope so). Thanks for guiding me.

Thanks, code looks better now!
Now to get your patch included, you've got to send it in a separate
mail, with only the patch description in it, i.e. not as a reply to
another mail with the history of the mail thread. Reason for this is
simply that the maintainers can pick up your patch with "git am" without
having to edit things manually. Please see also
http://qemu-project.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message
if you haven't read that page yet.

Ah, and two more things that came to my mind:

1) Please use "exit(1)" (or "exit(EXIT_FAILURE)") instead of "exit(-1)",
since that's the more common way to do that (since AFAIK the -1 will be
cast to 255 at the shell level, which is just ugly).

2) Maybe it would also be better to do the check right after the
cpu_ppc_init() in the machine's init function already, instead of doing
it in the mmubooke_create_initial_mapping() function? That way we could
be sure that wrong CPUs are blocked right from the start.

 Thanks,
  Thomas