[v1] QOMify MIPS cpu

[Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Philippe Mathieu-Daudé 6 years, 8 months ago

Hi,

While working with the mips codebase I had to QOMify it.

I then read Igor's series "complete cpu QOMification" [1] and after some IRC
chat I suggested Igor to rebase his series on mine to avoid code moving
forward then back.

Since most of Igor's series is reviewed I'm posting this a week before 2.11.

I'm not sure about the TypeInfo.abstract change so it is RFC.

Also I couldn't test it with KVM.

Regards,

Phil.

[1]: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04414.html

Igor Mammedov (2):
  mips: MIPSCPU model subclasses
  mips: replace cpu_mips_init() with cpu_generic_init()

Philippe Mathieu-Daudé (6):
  mips: move hw/mips/cputimer.c to target/mips/
  mips: introduce internal.h and cleanup cpu.h
  mips: split cpu_mips_realize_env() out of cpu_mips_init()
  mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
  mips: now than MIPSCPU is QOMified, mark it abstract
  mips: update mips_cpu_list() to use object_class_get_list()

 target/mips/cpu-qom.h                         |   1 +
 target/mips/cpu.h                             | 357 +---------------------
 target/mips/internal.h                        | 422 ++++++++++++++++++++++++++
 hw/mips/cps.c                                 |   2 +-
 hw/mips/mips_fulong2e.c                       |   2 +-
 hw/mips/mips_jazz.c                           |   2 +-
 hw/mips/mips_malta.c                          |   2 +-
 hw/mips/mips_mipssim.c                        |   2 +-
 hw/mips/mips_r4k.c                            |   2 +-
 hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
 target/mips/cpu.c                             |  57 +++-
 target/mips/gdbstub.c                         |   1 +
 target/mips/helper.c                          |  47 +++
 target/mips/kvm.c                             |   1 +
 target/mips/machine.c                         |   1 +
 target/mips/msa_helper.c                      |   1 +
 target/mips/op_helper.c                       |   1 +
 target/mips/translate.c                       |  23 +-
 target/mips/translate_init.c                  |  68 +----
 hw/mips/Makefile.objs                         |   2 +-
 target/mips/Makefile.objs                     |   2 +-
 21 files changed, 549 insertions(+), 449 deletions(-)
 create mode 100644 target/mips/internal.h
 rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)

-- 
2.14.1

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by no-reply@patchew.org 6 years, 8 months ago

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20170817043102.6322-1-f4bug@amsat.org
Subject: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
1557a1941b mips: update mips_cpu_list() to use object_class_get_list()
fff05e4786 mips: replace cpu_mips_init() with cpu_generic_init()
78afa559f4 !fixup mips: now than MIPSCPU is QOMified, mark it abstract
ff2926570c mips: MIPSCPU model subclasses
36f62a8a0b mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
825094780c mips: split cpu_mips_realize_env() out of cpu_mips_init()
7339de695e mips: introduce internal.h and cleanup cpu.h
011150634e mips: move hw/mips/cputimer.c to target/mips/

=== OUTPUT BEGIN ===
Checking PATCH 1/8: mips: move hw/mips/cputimer.c to target/mips/...
Checking PATCH 2/8: mips: introduce internal.h and cleanup cpu.h...
ERROR: space prohibited after that '&' (ctx:WxW)
#725: FILE: target/mips/internal.h:230:
+    if ((env->CP0_VPControl >> CP0VPCtl_DIS) & 1) {
                                              ^

ERROR: space prohibited after that '&' (ctx:WxW)
#733: FILE: target/mips/internal.h:238:
+            ((other_cpu->env.CP0_VPControl >> CP0VPCtl_DIS) & 1)) {
                                                             ^

ERROR: space prohibited after that '&' (ctx:WxW)
#753: FILE: target/mips/internal.h:258:
+        env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
                                                       ^

total: 3 errors, 0 warnings, 842 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/8: mips: split cpu_mips_realize_env() out of cpu_mips_init()...
Checking PATCH 4/8: mips: call cpu_mips_realize_env() from mips_cpu_realizefn()...
Checking PATCH 5/8: mips: MIPSCPU model subclasses...
Checking PATCH 6/8: !fixup mips: now than MIPSCPU is QOMified, mark it abstract...
Checking PATCH 7/8: mips: replace cpu_mips_init() with cpu_generic_init()...
Checking PATCH 8/8: mips: update mips_cpu_list() to use object_class_get_list()...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Philippe Mathieu-Daudé 6 years, 8 months ago

On 08/17/2017 01:54 AM, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 20170817043102.6322-1-f4bug@amsat.org
> Subject: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu
> 
[...]
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> 1557a1941b mips: update mips_cpu_list() to use object_class_get_list()
> fff05e4786 mips: replace cpu_mips_init() with cpu_generic_init()
> 78afa559f4 !fixup mips: now than MIPSCPU is QOMified, mark it abstract
> ff2926570c mips: MIPSCPU model subclasses
> 36f62a8a0b mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
> 825094780c mips: split cpu_mips_realize_env() out of cpu_mips_init()
> 7339de695e mips: introduce internal.h and cleanup cpu.h
> 011150634e mips: move hw/mips/cputimer.c to target/mips/
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/8: mips: move hw/mips/cputimer.c to target/mips/...
> Checking PATCH 2/8: mips: introduce internal.h and cleanup cpu.h...
> ERROR: space prohibited after that '&' (ctx:WxW)
> #725: FILE: target/mips/internal.h:230:
> +    if ((env->CP0_VPControl >> CP0VPCtl_DIS) & 1) {
>                                                ^
> 

I doubt CHECKPATCH is correct here, removing the space it'd look like 
handling pointer address...

> ERROR: space prohibited after that '&' (ctx:WxW)
> #733: FILE: target/mips/internal.h:238:
> +            ((other_cpu->env.CP0_VPControl >> CP0VPCtl_DIS) & 1)) {
>                                                               ^
> 
> ERROR: space prohibited after that '&' (ctx:WxW)
> #753: FILE: target/mips/internal.h:258:
> +        env->hflags |= (env->CP0_Status >> CP0St_KSU) & MIPS_HFLAG_KSU;
>                                                         ^
> 
> total: 3 errors, 0 warnings, 842 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/8: mips: split cpu_mips_realize_env() out of cpu_mips_init()...
> Checking PATCH 4/8: mips: call cpu_mips_realize_env() from mips_cpu_realizefn()...
> Checking PATCH 5/8: mips: MIPSCPU model subclasses...
> Checking PATCH 6/8: !fixup mips: now than MIPSCPU is QOMified, mark it abstract...
> Checking PATCH 7/8: mips: replace cpu_mips_init() with cpu_generic_init()...
> Checking PATCH 8/8: mips: update mips_cpu_list() to use object_class_get_list()...
> === OUTPUT END ===

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Igor Mammedov 6 years, 8 months ago

On Thu, 17 Aug 2017 01:30:54 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi,
> 
> While working with the mips codebase I had to QOMify it.
> 
> I then read Igor's series "complete cpu QOMification" [1] and after some IRC
> chat I suggested Igor to rebase his series on mine to avoid code moving
> forward then back.
> 
> Since most of Igor's series is reviewed I'm posting this a week before 2.11.
> 
> I'm not sure about the TypeInfo.abstract change so it is RFC.
> 
> Also I couldn't test it with KVM.
Tested in TCG mode (boots debian mips/mips64 kernel with different cpu types),
and new CPU leaf types show up on QOM tree as expected (QOMifycation is done as expected)
and '-cpu help' also works as expected,
so with checkpatch issues fixed you may add to patches my

Tested-by: Igor Mammedov <imammedo@redhat.com>



> 
> Regards,
> 
> Phil.
> 
> [1]: http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg04414.html
> 
> Igor Mammedov (2):
>   mips: MIPSCPU model subclasses
>   mips: replace cpu_mips_init() with cpu_generic_init()
> 
> Philippe Mathieu-Daudé (6):
>   mips: move hw/mips/cputimer.c to target/mips/
>   mips: introduce internal.h and cleanup cpu.h
>   mips: split cpu_mips_realize_env() out of cpu_mips_init()
>   mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
>   mips: now than MIPSCPU is QOMified, mark it abstract
>   mips: update mips_cpu_list() to use object_class_get_list()
> 
>  target/mips/cpu-qom.h                         |   1 +
>  target/mips/cpu.h                             | 357 +---------------------
>  target/mips/internal.h                        | 422 ++++++++++++++++++++++++++
>  hw/mips/cps.c                                 |   2 +-
>  hw/mips/mips_fulong2e.c                       |   2 +-
>  hw/mips/mips_jazz.c                           |   2 +-
>  hw/mips/mips_malta.c                          |   2 +-
>  hw/mips/mips_mipssim.c                        |   2 +-
>  hw/mips/mips_r4k.c                            |   2 +-
>  hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
>  target/mips/cpu.c                             |  57 +++-
>  target/mips/gdbstub.c                         |   1 +
>  target/mips/helper.c                          |  47 +++
>  target/mips/kvm.c                             |   1 +
>  target/mips/machine.c                         |   1 +
>  target/mips/msa_helper.c                      |   1 +
>  target/mips/op_helper.c                       |   1 +
>  target/mips/translate.c                       |  23 +-
>  target/mips/translate_init.c                  |  68 +----
>  hw/mips/Makefile.objs                         |   2 +-
>  target/mips/Makefile.objs                     |   2 +-
>  21 files changed, 549 insertions(+), 449 deletions(-)
>  create mode 100644 target/mips/internal.h
>  rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)
>

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Philippe Mathieu-Daudé 6 years, 8 months ago

On 08/17/2017 12:22 PM, Igor Mammedov wrote:
> On Thu, 17 Aug 2017 01:30:54 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
[...]
>> Also I couldn't test it with KVM.
> Tested in TCG mode (boots debian mips/mips64 kernel with different cpu types),
> and new CPU leaf types show up on QOM tree as expected (QOMifycation is done as expected)

you mean the "info qom-tree" output?

> and '-cpu help' also works as expected,
> so with checkpatch issues fixed you may add to patches my
> 
> Tested-by: Igor Mammedov <imammedo@redhat.com>

ok thanks for the testing!

I'll wait to see if there is some KVM feedback from imgtec folks before 
spaming a v2.

>> Igor Mammedov (2):
>>    mips: MIPSCPU model subclasses
>>    mips: replace cpu_mips_init() with cpu_generic_init()
>>
>> Philippe Mathieu-Daudé (6):
>>    mips: move hw/mips/cputimer.c to target/mips/
>>    mips: introduce internal.h and cleanup cpu.h
>>    mips: split cpu_mips_realize_env() out of cpu_mips_init()
>>    mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
>>    mips: now than MIPSCPU is QOMified, mark it abstract
>>    mips: update mips_cpu_list() to use object_class_get_list()
>>
>>   target/mips/cpu-qom.h                         |   1 +
>>   target/mips/cpu.h                             | 357 +---------------------
>>   target/mips/internal.h                        | 422 ++++++++++++++++++++++++++
>>   hw/mips/cps.c                                 |   2 +-
>>   hw/mips/mips_fulong2e.c                       |   2 +-
>>   hw/mips/mips_jazz.c                           |   2 +-
>>   hw/mips/mips_malta.c                          |   2 +-
>>   hw/mips/mips_mipssim.c                        |   2 +-
>>   hw/mips/mips_r4k.c                            |   2 +-
>>   hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
>>   target/mips/cpu.c                             |  57 +++-
>>   target/mips/gdbstub.c                         |   1 +
>>   target/mips/helper.c                          |  47 +++
>>   target/mips/kvm.c                             |   1 +
>>   target/mips/machine.c                         |   1 +
>>   target/mips/msa_helper.c                      |   1 +
>>   target/mips/op_helper.c                       |   1 +
>>   target/mips/translate.c                       |  23 +-
>>   target/mips/translate_init.c                  |  68 +----
>>   hw/mips/Makefile.objs                         |   2 +-
>>   target/mips/Makefile.objs                     |   2 +-
>>   21 files changed, 549 insertions(+), 449 deletions(-)
>>   create mode 100644 target/mips/internal.h
>>   rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Igor Mammedov 6 years, 8 months ago

On Fri, 18 Aug 2017 17:08:29 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 08/17/2017 12:22 PM, Igor Mammedov wrote:
> > On Thu, 17 Aug 2017 01:30:54 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> [...]
> >> Also I couldn't test it with KVM.  
> > Tested in TCG mode (boots debian mips/mips64 kernel with different cpu types),
> > and new CPU leaf types show up on QOM tree as expected (QOMifycation is done as expected)  
> 
> you mean the "info qom-tree" output?
I've used scripts/qmp/qom-* scripts for testing which use QMP,
but QOM tree that both use is the same so there shouldn't be any difference.


> 
> > and '-cpu help' also works as expected,
> > so with checkpatch issues fixed you may add to patches my
> > 
> > Tested-by: Igor Mammedov <imammedo@redhat.com>  
> 
> ok thanks for the testing!
> 
> I'll wait to see if there is some KVM feedback from imgtec folks before 
> spaming a v2.
> 
> >> Igor Mammedov (2):
> >>    mips: MIPSCPU model subclasses
> >>    mips: replace cpu_mips_init() with cpu_generic_init()
> >>
> >> Philippe Mathieu-Daudé (6):
> >>    mips: move hw/mips/cputimer.c to target/mips/
> >>    mips: introduce internal.h and cleanup cpu.h
> >>    mips: split cpu_mips_realize_env() out of cpu_mips_init()
> >>    mips: call cpu_mips_realize_env() from mips_cpu_realizefn()
> >>    mips: now than MIPSCPU is QOMified, mark it abstract
> >>    mips: update mips_cpu_list() to use object_class_get_list()
> >>
> >>   target/mips/cpu-qom.h                         |   1 +
> >>   target/mips/cpu.h                             | 357 +---------------------
> >>   target/mips/internal.h                        | 422 ++++++++++++++++++++++++++
> >>   hw/mips/cps.c                                 |   2 +-
> >>   hw/mips/mips_fulong2e.c                       |   2 +-
> >>   hw/mips/mips_jazz.c                           |   2 +-
> >>   hw/mips/mips_malta.c                          |   2 +-
> >>   hw/mips/mips_mipssim.c                        |   2 +-
> >>   hw/mips/mips_r4k.c                            |   2 +-
> >>   hw/mips/cputimer.c => target/mips/cp0_timer.c |   2 +-
> >>   target/mips/cpu.c                             |  57 +++-
> >>   target/mips/gdbstub.c                         |   1 +
> >>   target/mips/helper.c                          |  47 +++
> >>   target/mips/kvm.c                             |   1 +
> >>   target/mips/machine.c                         |   1 +
> >>   target/mips/msa_helper.c                      |   1 +
> >>   target/mips/op_helper.c                       |   1 +
> >>   target/mips/translate.c                       |  23 +-
> >>   target/mips/translate_init.c                  |  68 +----
> >>   hw/mips/Makefile.objs                         |   2 +-
> >>   target/mips/Makefile.objs                     |   2 +-
> >>   21 files changed, 549 insertions(+), 449 deletions(-)
> >>   create mode 100644 target/mips/internal.h
> >>   rename hw/mips/cputimer.c => target/mips/cp0_timer.c (99%)  
>

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Igor Mammedov 6 years, 7 months ago

On Fri, 18 Aug 2017 17:08:29 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 08/17/2017 12:22 PM, Igor Mammedov wrote:
> > On Thu, 17 Aug 2017 01:30:54 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> [...]
> >> Also I couldn't test it with KVM.  
> > Tested in TCG mode (boots debian mips/mips64 kernel with different cpu types),
> > and new CPU leaf types show up on QOM tree as expected (QOMifycation is done as expected)  
> 
> you mean the "info qom-tree" output?
> 
> > and '-cpu help' also works as expected,
> > so with checkpatch issues fixed you may add to patches my
> > 
> > Tested-by: Igor Mammedov <imammedo@redhat.com>  
> 
> ok thanks for the testing!
> 
> I'll wait to see if there is some KVM feedback from imgtec folks before 
> spaming a v2.

Philippe,

Are you going to respin series soon?

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

Hi Igor,

On 08/30/2017 10:50 AM, Igor Mammedov wrote:
>> [...]
>>>> Also I couldn't test it with KVM.
[..]
>>
>> I'll wait to see if there is some KVM feedback from imgtec folks before
>> spaming a v2.
> 
> Philippe,
> 
> Are you going to respin series soon?

I'm waiting 2.11 officially announced open to send it which should be 
tomorrow, in the meaning time it is available in my git repository at:

   git://github.com/philmd/qemu.git tags/mips-qomify-20170830

Regards,

Phil.

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by James Hogan 6 years, 7 months ago

Hi Philippe,

On Wed, Aug 30, 2017 at 11:41:38AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Igor,
> 
> On 08/30/2017 10:50 AM, Igor Mammedov wrote:
> >> [...]
> >>>> Also I couldn't test it with KVM.
> [..]
> >>
> >> I'll wait to see if there is some KVM feedback from imgtec folks before
> >> spaming a v2.

Sorry for the delay,

> > 
> > Philippe,
> > 
> > Are you going to respin series soon?
> 
> I'm waiting 2.11 officially announced open to send it which should be 
> tomorrow, in the meaning time it is available in my git repository at:
> 
>    git://github.com/philmd/qemu.git tags/mips-qomify-20170830

A sanity check of your branch doesn't reveal any obvious regressions
when booting a Malta KVM guest kernel with KVM.

Cheers
James

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by Philippe Mathieu-Daudé 6 years, 7 months ago

Hi James,

On 08/30/2017 03:19 PM, James Hogan wrote:
[...]
>>     git://github.com/philmd/qemu.git tags/mips-qomify-20170830
> 
> A sanity check of your branch doesn't reveal any obvious regressions
> when booting a Malta KVM guest kernel with KVM.

Thank you for your time to test this!

Does that mean I can add your Tested-by: to the series?

Regards,

Phil.

Re: [Qemu-devel] [PATCH 0/8] QOMify MIPS cpu

Posted by James Hogan 6 years, 7 months ago

On Wed, Aug 30, 2017 at 04:52:04PM -0300, Philippe Mathieu-Daudé wrote:
> Hi James,
> 
> On 08/30/2017 03:19 PM, James Hogan wrote:
> [...]
> >>     git://github.com/philmd/qemu.git tags/mips-qomify-20170830
> > 
> > A sanity check of your branch doesn't reveal any obvious regressions
> > when booting a Malta KVM guest kernel with KVM.
> 
> Thank you for your time to test this!
> 
> Does that mean I can add your Tested-by: to the series?

Sure,

Cheers
James