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
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
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
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
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(-) >
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
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
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
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.
© 2016 - 2024 Red Hat, Inc.