[Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()

Peter Maydell posted 4 patches 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1486996099-15820-1-git-send-email-peter.maydell@linaro.org
Test checkpatch passed
Test docker passed
Test s390x passed
include/qom/cpu.h     | 17 +++++++++++++++++
hw/arm/integratorcp.c | 22 ++--------------------
hw/arm/virt.c         | 24 ++----------------------
qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
4 files changed, 47 insertions(+), 53 deletions(-)
[Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Peter Maydell 7 years, 1 month ago
This patchset adds a new function cpu_generic_new()
which is similar to cpu_generic_init() except that it
does not realize the created CPU object. This means that
board code can do a "new cpu; set QOM properties; realize"
sequence without having to do all the work of splitting
the CPU model string and calling parse_features by hand.

Patch 2 clarifies a TODO comment, hopefully correctly,
based on an email conversation I had with Eduardo a
little while back.

Patches 3 and 4 change the ARM boards which currently
call parse_features by hand to use the new function.


If there's consensus that this is the right general
direction to go in, then I think that some other
architectures could also make cleanups to use this:
 * cpu_s390x_create() is almost exactly this function,
   give or take some fine detail of error handling
 * ppc_cpu_parse_features is almost the same thing,
   except that it doesn't actually create the CPU object,
   it only calls parse_features
 * hw/i386/pc.c does a manual parse_features

I'm not strongly attached to this particular approach
(though it seems like a reasonable one, especially given
the proliferation of different arch-specific helpers
listed above and the bugs in boards which don't call
parse_features when they should), but I would like us to
figure out and document what the right way for a board
to create and configure its CPU objects is...


Michael Davidsaver (1):
  cpu: add cpu_generic_new()

Peter Maydell (3):
  cpu: Clarify TODO comment in cpu_generic_new()
  hw/arm/integrator: Use new cpu_generic_new()
  hw/arm/virt: Use new cpu_generic_new()

 include/qom/cpu.h     | 17 +++++++++++++++++
 hw/arm/integratorcp.c | 22 ++--------------------
 hw/arm/virt.c         | 24 ++----------------------
 qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
 4 files changed, 47 insertions(+), 53 deletions(-)

-- 
2.7.4


Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Peter Maydell 7 years, 1 month ago
On 13 February 2017 at 14:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.
>
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
>
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
>
>
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
>
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...

I know it's only been a few days, but I just wanted to
say that I'd appreciate it if we could manage relatively
quick review on this one, since I have a set of patches
pending that depend on this and which it would be nice
to get into 2.9 (softfreeze approaching rapidly).

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Eduardo Habkost 7 years, 1 month ago
On Thu, Feb 16, 2017 at 04:40:43PM +0000, Peter Maydell wrote:
> On 13 February 2017 at 14:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> >
> > Patch 2 clarifies a TODO comment, hopefully correctly,
> > based on an email conversation I had with Eduardo a
> > little while back.
> >
> > Patches 3 and 4 change the ARM boards which currently
> > call parse_features by hand to use the new function.
> >
> >
> > If there's consensus that this is the right general
> > direction to go in, then I think that some other
> > architectures could also make cleanups to use this:
> >  * cpu_s390x_create() is almost exactly this function,
> >    give or take some fine detail of error handling
> >  * ppc_cpu_parse_features is almost the same thing,
> >    except that it doesn't actually create the CPU object,
> >    it only calls parse_features
> >  * hw/i386/pc.c does a manual parse_features
> >
> > I'm not strongly attached to this particular approach
> > (though it seems like a reasonable one, especially given
> > the proliferation of different arch-specific helpers
> > listed above and the bugs in boards which don't call
> > parse_features when they should), but I would like us to
> > figure out and document what the right way for a board
> > to create and configure its CPU objects is...
> 
> I know it's only been a few days, but I just wanted to
> say that I'd appreciate it if we could manage relatively
> quick review on this one, since I have a set of patches
> pending that depend on this and which it would be nice
> to get into 2.9 (softfreeze approaching rapidly).

I was away from work for the past 3 weeks and I am catching up on
e-mail, right now. Is this proposal still up, or is this series
obsoleted by something else?

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Peter Maydell 7 years, 1 month ago
On 21 February 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Feb 16, 2017 at 04:40:43PM +0000, Peter Maydell wrote:
>> I know it's only been a few days, but I just wanted to
>> say that I'd appreciate it if we could manage relatively
>> quick review on this one, since I have a set of patches
>> pending that depend on this and which it would be nice
>> to get into 2.9 (softfreeze approaching rapidly).
>
> I was away from work for the past 3 weeks and I am catching up on
> e-mail, right now. Is this proposal still up, or is this series
> obsoleted by something else?

Given Igor's view that this isn't the way we want to go, I have
decoupled the other patches I mention above from this patchset
(making them do an in-line call to parse_features etc like
the virt.c code etc etc do at the moment). So there's no rush
and we can take the time to decide what the right approach is
(which we can then implement for QEMU 2.10, I expect).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Igor Mammedov 7 years, 1 month ago
On Mon, 13 Feb 2017 14:28:15 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.
> 
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
> 
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
> 
> 
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
> 
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...

series looks like a step back adding yet another way
to create CPU and makes code even more inconsistent,
instead of removing TODO item by doing proper generalization.
So I'm sort of object to it.

I'll just posted RFC which show idea how generalization of
cpu_model/features parsing should be implemented.

However I don't have cycles to complete it, only
virt-arm/spapr/pc are converted as example.
One who would pick the task up should complete it for
all boards to make code consistent.

> 
> 
> Michael Davidsaver (1):
>   cpu: add cpu_generic_new()
> 
> Peter Maydell (3):
>   cpu: Clarify TODO comment in cpu_generic_new()
>   hw/arm/integrator: Use new cpu_generic_new()
>   hw/arm/virt: Use new cpu_generic_new()
> 
>  include/qom/cpu.h     | 17 +++++++++++++++++
>  hw/arm/integratorcp.c | 22 ++--------------------
>  hw/arm/virt.c         | 24 ++----------------------
>  qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
>  4 files changed, 47 insertions(+), 53 deletions(-)
> 


Re: [Qemu-devel] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Eduardo Habkost 7 years, 1 month ago
On Fri, Feb 17, 2017 at 08:05:22PM +0100, Igor Mammedov wrote:
> On Mon, 13 Feb 2017 14:28:15 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> > 
> > Patch 2 clarifies a TODO comment, hopefully correctly,
> > based on an email conversation I had with Eduardo a
> > little while back.
> > 
> > Patches 3 and 4 change the ARM boards which currently
> > call parse_features by hand to use the new function.
> > 
> > 
> > If there's consensus that this is the right general
> > direction to go in, then I think that some other
> > architectures could also make cleanups to use this:
> >  * cpu_s390x_create() is almost exactly this function,
> >    give or take some fine detail of error handling
> >  * ppc_cpu_parse_features is almost the same thing,
> >    except that it doesn't actually create the CPU object,
> >    it only calls parse_features
> >  * hw/i386/pc.c does a manual parse_features
> > 
> > I'm not strongly attached to this particular approach
> > (though it seems like a reasonable one, especially given
> > the proliferation of different arch-specific helpers
> > listed above and the bugs in boards which don't call
> > parse_features when they should), but I would like us to
> > figure out and document what the right way for a board
> > to create and configure its CPU objects is...
> 
> series looks like a step back adding yet another way
> to create CPU and makes code even more inconsistent,
> instead of removing TODO item by doing proper generalization.
> So I'm sort of object to it.
> 
> I'll just posted RFC which show idea how generalization of
> cpu_model/features parsing should be implemented.
> 
> However I don't have cycles to complete it, only
> virt-arm/spapr/pc are converted as example.
> One who would pick the task up should complete it for
> all boards to make code consistent.

If conversion of the code will take a while and this wrapper
removes code duplication of the current boards, I believe this
series is stlil a step in the right direction, even if the plan
is to replace the wrapper one day. But I will take a look at your
RFC first, before giving my opinion on which approach to take.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Alex Bennée 6 years, 9 months ago
Peter Maydell <peter.maydell@linaro.org> writes:

> This patchset adds a new function cpu_generic_new()
> which is similar to cpu_generic_init() except that it
> does not realize the created CPU object. This means that
> board code can do a "new cpu; set QOM properties; realize"
> sequence without having to do all the work of splitting
> the CPU model string and calling parse_features by hand.


Just going through my review queue and I see this needs re-basing. Is
there going to be another rev or was a different approach suggested?

>
> Patch 2 clarifies a TODO comment, hopefully correctly,
> based on an email conversation I had with Eduardo a
> little while back.
>
> Patches 3 and 4 change the ARM boards which currently
> call parse_features by hand to use the new function.
>
>
> If there's consensus that this is the right general
> direction to go in, then I think that some other
> architectures could also make cleanups to use this:
>  * cpu_s390x_create() is almost exactly this function,
>    give or take some fine detail of error handling
>  * ppc_cpu_parse_features is almost the same thing,
>    except that it doesn't actually create the CPU object,
>    it only calls parse_features
>  * hw/i386/pc.c does a manual parse_features
>
> I'm not strongly attached to this particular approach
> (though it seems like a reasonable one, especially given
> the proliferation of different arch-specific helpers
> listed above and the bugs in boards which don't call
> parse_features when they should), but I would like us to
> figure out and document what the right way for a board
> to create and configure its CPU objects is...
>
>
> Michael Davidsaver (1):
>   cpu: add cpu_generic_new()
>
> Peter Maydell (3):
>   cpu: Clarify TODO comment in cpu_generic_new()
>   hw/arm/integrator: Use new cpu_generic_new()
>   hw/arm/virt: Use new cpu_generic_new()
>
>  include/qom/cpu.h     | 17 +++++++++++++++++
>  hw/arm/integratorcp.c | 22 ++--------------------
>  hw/arm/virt.c         | 24 ++----------------------
>  qom/cpu.c             | 37 ++++++++++++++++++++++++++-----------
>  4 files changed, 47 insertions(+), 53 deletions(-)


--
Alex Bennée

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Eduardo Habkost 6 years, 9 months ago
On Mon, Jun 26, 2017 at 02:28:13PM +0100, Alex Bennée wrote:
> 
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > This patchset adds a new function cpu_generic_new()
> > which is similar to cpu_generic_init() except that it
> > does not realize the created CPU object. This means that
> > board code can do a "new cpu; set QOM properties; realize"
> > sequence without having to do all the work of splitting
> > the CPU model string and calling parse_features by hand.
> 
> 
> Just going through my review queue and I see this needs re-basing. Is
> there going to be another rev or was a different approach suggested?

The right way to go is not clear.  We know we want to remove duplication
of CPU creation code, but probably we should first refactor the -cpu
parsing code, so parsing happens: 1) only once; 2) earlier in main(),
preferably before machine->init() runs; 3) inside generic code instead
of arch-specific code; 4) preferably using the QemuOpts parser instead
of the current strtok()-based custom parsers.

After the parsing code mess is sorted out, writing a generic CPU
creation wrapper will probably be easier (and safer).

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/4] cpu: Implement cpu_generic_new()
Posted by Igor Mammedov 6 years, 9 months ago
On Mon, 26 Jun 2017 23:33:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 26, 2017 at 02:28:13PM +0100, Alex Bennée wrote:
> > 
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >   
> > > This patchset adds a new function cpu_generic_new()
> > > which is similar to cpu_generic_init() except that it
> > > does not realize the created CPU object. This means that
> > > board code can do a "new cpu; set QOM properties; realize"
> > > sequence without having to do all the work of splitting
> > > the CPU model string and calling parse_features by hand.  
> > 
> > 
> > Just going through my review queue and I see this needs re-basing. Is
> > there going to be another rev or was a different approach suggested?  
> 
> The right way to go is not clear.  We know we want to remove duplication
> of CPU creation code, but probably we should first refactor the -cpu
> parsing code, so parsing happens: 1) only once; 2) earlier in main(),
> preferably before machine->init() runs; 3) inside generic code instead
> of arch-specific code; 4) preferably using the QemuOpts parser instead
> of the current strtok()-based custom parsers.
> 
> After the parsing code mess is sorted out, writing a generic CPU
> creation wrapper will probably be easier (and safer).

Also there is legacy cpu features parsing/handling in sparc target,
so we might need to clean it up and convert to property based features
(as have been done for i386) before making generic cpu creation.