[Qemu-devel] [PATCH] finally kill cpudef config section support

[Qemu-devel] [PATCH] finally kill cpudef config section support

[Eduardo Habkost]

The external CPU models were removed on QEMU 1.2, and the support for
the "cpudef" config sections was documented as deprecated, but the
actual removal of the config section was pending.

Now that QEMU 1.3 was released, we can finally kill the support for
cpudef config sections, and support only the built-in CPU models from
target-i386/cpu.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 qemu-config.c | 49 -------------------------------------------------
 1 file changed, 49 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 10d1ba4..aa78fb9 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -417,54 +417,6 @@ static QemuOptsList qemu_trace_opts = {
     },
 };
 
-static QemuOptsList qemu_cpudef_opts = {
-    .name = "cpudef",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpudef_opts.head),
-    .desc = {
-        {
-            .name = "name",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "level",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "vendor",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "family",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "model",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "stepping",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "feature_edx",      /* cpuid 0000_0001.edx */
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "feature_ecx",      /* cpuid 0000_0001.ecx */
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "extfeature_edx",   /* cpuid 8000_0001.edx */
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "extfeature_ecx",   /* cpuid 8000_0001.ecx */
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "xlevel",
-            .type = QEMU_OPT_NUMBER,
-        },{
-            .name = "model_id",
-            .type = QEMU_OPT_STRING,
-        },{
-            .name = "vendor_override",
-            .type = QEMU_OPT_NUMBER,
-        },
-        { /* end of list */ }
-    },
-};
-
 QemuOptsList qemu_spice_opts = {
     .name = "spice",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
@@ -700,7 +652,6 @@ static QemuOptsList *vm_config_groups[32] = {
     &qemu_rtc_opts,
     &qemu_global_opts,
     &qemu_mon_opts,
-    &qemu_cpudef_opts,
     &qemu_trace_opts,
     &qemu_option_rom_opts,
     &qemu_machine_opts,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Andreas Färber]

Am 04.12.2012 19:32, schrieb Eduardo Habkost:
> The external CPU models were removed on QEMU 1.2, and the support for
> the "cpudef" config sections was documented as deprecated, but the
> actual removal of the config section was pending.
> 
> Now that QEMU 1.3 was released, we can finally kill the support for
> cpudef config sections, and support only the built-in CPU models from
> target-i386/cpu.c.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

This looks okay so far, but I'm guessing this was based on another
branch of yours? If we apply this right now, we would also want to
remove the #ifdef'ed cpudef_init() or so function invoking the parsing, no?

Andreas

> ---
>  qemu-config.c | 49 -------------------------------------------------
>  1 file changed, 49 deletions(-)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index 10d1ba4..aa78fb9 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -417,54 +417,6 @@ static QemuOptsList qemu_trace_opts = {
>      },
>  };
>  
> -static QemuOptsList qemu_cpudef_opts = {
> -    .name = "cpudef",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpudef_opts.head),
> -    .desc = {
> -        {
> -            .name = "name",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "level",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "vendor",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "family",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "model",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "stepping",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "feature_edx",      /* cpuid 0000_0001.edx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "feature_ecx",      /* cpuid 0000_0001.ecx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "extfeature_edx",   /* cpuid 8000_0001.edx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "extfeature_ecx",   /* cpuid 8000_0001.ecx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "xlevel",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "model_id",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "vendor_override",
> -            .type = QEMU_OPT_NUMBER,
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  QemuOptsList qemu_spice_opts = {
>      .name = "spice",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
> @@ -700,7 +652,6 @@ static QemuOptsList *vm_config_groups[32] = {
>      &qemu_rtc_opts,
>      &qemu_global_opts,
>      &qemu_mon_opts,
> -    &qemu_cpudef_opts,
>      &qemu_trace_opts,
>      &qemu_option_rom_opts,
>      &qemu_machine_opts,
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Eduardo Habkost]

On Tue, Dec 04, 2012 at 07:41:47PM +0100, Andreas Färber wrote:
> Am 04.12.2012 19:32, schrieb Eduardo Habkost:
> > The external CPU models were removed on QEMU 1.2, and the support for
> > the "cpudef" config sections was documented as deprecated, but the
> > actual removal of the config section was pending.
> > 
> > Now that QEMU 1.3 was released, we can finally kill the support for
> > cpudef config sections, and support only the built-in CPU models from
> > target-i386/cpu.c.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> This looks okay so far, but I'm guessing this was based on another
> branch of yours? If we apply this right now, we would also want to
> remove the #ifdef'ed cpudef_init() or so function invoking the parsing, no?

Right now cpudef_init() calls cpudef_setup(), that initializes some
fields in the builtin CPU model list on x86, but doesn't use the cpudef
config section anymore[1].

cpudef_setup() and cpudef_init() will be killed later, when we kill the
builtin_x86_defs table and introduce CPU model subclasses.


[1] The code dealing with the cpudef section was removed on
    c04321b3685a0b06d737d04146a0f1f2c5950b39.
    So, in the end it would be safe to apply this patch before 1.3, as
    the code that actually used the cpudef CPU models was already
    removed.

> 
> Andreas
> 
> > ---
> >  qemu-config.c | 49 -------------------------------------------------
> >  1 file changed, 49 deletions(-)
> > 
> > diff --git a/qemu-config.c b/qemu-config.c
> > index 10d1ba4..aa78fb9 100644
> > --- a/qemu-config.c
> > +++ b/qemu-config.c
> > @@ -417,54 +417,6 @@ static QemuOptsList qemu_trace_opts = {
> >      },
> >  };
> >  
> > -static QemuOptsList qemu_cpudef_opts = {
> > -    .name = "cpudef",
> > -    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpudef_opts.head),
> > -    .desc = {
> > -        {
> > -            .name = "name",
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "level",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },{
> > -            .name = "vendor",
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "family",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },{
> > -            .name = "model",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },{
> > -            .name = "stepping",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },{
> > -            .name = "feature_edx",      /* cpuid 0000_0001.edx */
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "feature_ecx",      /* cpuid 0000_0001.ecx */
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "extfeature_edx",   /* cpuid 8000_0001.edx */
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "extfeature_ecx",   /* cpuid 8000_0001.ecx */
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "xlevel",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },{
> > -            .name = "model_id",
> > -            .type = QEMU_OPT_STRING,
> > -        },{
> > -            .name = "vendor_override",
> > -            .type = QEMU_OPT_NUMBER,
> > -        },
> > -        { /* end of list */ }
> > -    },
> > -};
> > -
> >  QemuOptsList qemu_spice_opts = {
> >      .name = "spice",
> >      .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
> > @@ -700,7 +652,6 @@ static QemuOptsList *vm_config_groups[32] = {
> >      &qemu_rtc_opts,
> >      &qemu_global_opts,
> >      &qemu_mon_opts,
> > -    &qemu_cpudef_opts,
> >      &qemu_trace_opts,
> >      &qemu_option_rom_opts,
> >      &qemu_machine_opts,
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Blue Swirl]

Thanks, applied.

On Tue, Dec 4, 2012 at 6:32 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The external CPU models were removed on QEMU 1.2, and the support for
> the "cpudef" config sections was documented as deprecated, but the
> actual removal of the config section was pending.
>
> Now that QEMU 1.3 was released, we can finally kill the support for
> cpudef config sections, and support only the built-in CPU models from
> target-i386/cpu.c.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  qemu-config.c | 49 -------------------------------------------------
>  1 file changed, 49 deletions(-)
>
> diff --git a/qemu-config.c b/qemu-config.c
> index 10d1ba4..aa78fb9 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -417,54 +417,6 @@ static QemuOptsList qemu_trace_opts = {
>      },
>  };
>
> -static QemuOptsList qemu_cpudef_opts = {
> -    .name = "cpudef",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpudef_opts.head),
> -    .desc = {
> -        {
> -            .name = "name",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "level",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "vendor",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "family",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "model",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "stepping",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "feature_edx",      /* cpuid 0000_0001.edx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "feature_ecx",      /* cpuid 0000_0001.ecx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "extfeature_edx",   /* cpuid 8000_0001.edx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "extfeature_ecx",   /* cpuid 8000_0001.ecx */
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "xlevel",
> -            .type = QEMU_OPT_NUMBER,
> -        },{
> -            .name = "model_id",
> -            .type = QEMU_OPT_STRING,
> -        },{
> -            .name = "vendor_override",
> -            .type = QEMU_OPT_NUMBER,
> -        },
> -        { /* end of list */ }
> -    },
> -};
> -
>  QemuOptsList qemu_spice_opts = {
>      .name = "spice",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_spice_opts.head),
> @@ -700,7 +652,6 @@ static QemuOptsList *vm_config_groups[32] = {
>      &qemu_rtc_opts,
>      &qemu_global_opts,
>      &qemu_mon_opts,
> -    &qemu_cpudef_opts,
>      &qemu_trace_opts,
>      &qemu_option_rom_opts,
>      &qemu_machine_opts,
> --
> 1.7.11.7
>
>

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Andreas Färber]

Am 08.12.2012 18:54, schrieb Blue Swirl:
> Thanks, applied.

As discussed this still leaves some cpudef cruft behind.

Eduardo, can you please send a follow-up to move the versioning code and
kills all cpudef_*(), now that the patch got prematurely applied?

Thanks,
Andreas


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Blue Swirl]

On Sat, Dec 8, 2012 at 6:02 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 08.12.2012 18:54, schrieb Blue Swirl:
>> Thanks, applied.
>
> As discussed this still leaves some cpudef cruft behind.

But Eduardo said that it will be removed later.

> Eduardo, can you please send a follow-up to move the versioning code and
> kills all cpudef_*(), now that the patch got prematurely applied?
>
> Thanks,
> Andreas
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Andreas Färber]

Am 08.12.2012 21:00, schrieb Blue Swirl:
> On Sat, Dec 8, 2012 at 6:02 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 08.12.2012 18:54, schrieb Blue Swirl:
>>> Thanks, applied.
>>
>> As discussed this still leaves some cpudef cruft behind.
> 
> But Eduardo said that it will be removed later.

I chose not to include it in the current qom-cpu pull while I was still
investigating how to resolve the conflict between the patch claiming to
"finally kill", not satisfactorily doing it but not yet having X86CPU
subclasses.

Actually I now found a very easy and un-intrusive way to clean that up,
series coming up!

What I am disappointed about here is the work duplication. The only user
of this cpudef is the x86 CPU, an area that I have been maintaining
since Anthony asked for help with his maintenance areas. You can argue
that qemu-config.c is not under my maintenance and that
target-i386/cpu.c is not properly documented as such in MAINTAINERS
(which I will fix, noticing this), but either way since my reply
indicates that I started review, I would have appreciated a reply
indicating you agree with Eduardo I should apply it as such or asking
whether you can apply it now rather than applying this patch without
anyone's Reviewed-by or Acked-by while my pull is still in flight.

Generally I expect committers to handle PULLs from maintainers and
PATCHes from unmaintained areas only. At times in lack of communication
those borders get blurred, leading to patches handled differently by two
persons, such as the Haswell patch showing up twice in the shortlog or
this patch getting applied while review comments are unresolved.

If you think I'm doing a terrible job as maintainer (hard freeze, review
times, my perfectionism...) and wish to handle x86 CPU patches yourself,
just say so openly and I'll happily invest my time in an area where the
effort is more appreciated.

What I would've liked was an idea raised at QEMU Summit of having a
staging tree similar to the trivial queue as sort-of a last-call to
point out typos or potential conflicts between trees before a patch
lands in qemu.git and either stays or must be reverted or followed up.
But this did not find a lot of support.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Blue Swirl]

On Sun, Dec 9, 2012 at 7:13 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 08.12.2012 21:00, schrieb Blue Swirl:
>> On Sat, Dec 8, 2012 at 6:02 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Am 08.12.2012 18:54, schrieb Blue Swirl:
>>>> Thanks, applied.
>>>
>>> As discussed this still leaves some cpudef cruft behind.
>>
>> But Eduardo said that it will be removed later.
>
> I chose not to include it in the current qom-cpu pull while I was still
> investigating how to resolve the conflict between the patch claiming to
> "finally kill", not satisfactorily doing it but not yet having X86CPU
> subclasses.
>
> Actually I now found a very easy and un-intrusive way to clean that up,
> series coming up!
>
> What I am disappointed about here is the work duplication. The only user
> of this cpudef is the x86 CPU, an area that I have been maintaining
> since Anthony asked for help with his maintenance areas. You can argue
> that qemu-config.c is not under my maintenance and that
> target-i386/cpu.c is not properly documented as such in MAINTAINERS
> (which I will fix, noticing this), but either way since my reply
> indicates that I started review, I would have appreciated a reply
> indicating you agree with Eduardo I should apply it as such or asking
> whether you can apply it now rather than applying this patch without
> anyone's Reviewed-by or Acked-by while my pull is still in flight.

Actually I checked that it didn't conflict with your pull (which I
applied) and also with your RFC series (which I didn't apply, of
course). So it looked OK to me.

> Generally I expect committers to handle PULLs from maintainers and
> PATCHes from unmaintained areas only. At times in lack of communication
> those borders get blurred, leading to patches handled differently by two
> persons, such as the Haswell patch showing up twice in the shortlog or
> this patch getting applied while review comments are unresolved.
>
> If you think I'm doing a terrible job as maintainer (hard freeze, review
> times, my perfectionism...) and wish to handle x86 CPU patches yourself,
> just say so openly and I'll happily invest my time in an area where the
> effort is more appreciated.

I think you are doing a great job, thanks for your efforts. I have no
interest for doing the QOM conversion myself.

> What I would've liked was an idea raised at QEMU Summit of having a
> staging tree similar to the trivial queue as sort-of a last-call to
> point out typos or potential conflicts between trees before a patch
> lands in qemu.git and either stays or must be reverted or followed up.
> But this did not find a lot of support.

I think it would be nice if you could take patches like this which are
close to your CPU patches and keep them in your tree, or just NAK
patches which conflict with other work. That kind of coordination
would avoid staging problems.

What's for example the plan for Igor's series, should they be applied
next or something else?

>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Andreas Färber]

Am 09.12.2012 21:46, schrieb Blue Swirl:
> On Sun, Dec 9, 2012 at 7:13 PM, Andreas Färber <afaerber@suse.de> wrote:
>> [...] I would have appreciated a reply
>> indicating you agree with Eduardo I should apply it as such or asking
>> whether you can apply it now rather than applying this patch without
>> anyone's Reviewed-by or Acked-by while my pull is still in flight.
> 
> Actually I checked that it didn't conflict with your pull (which I
> applied) and also with your RFC series (which I didn't apply, of
> course). So it looked OK to me.

Yes thanks, your merging the ioport pull was indeed appreciated
(although it does push the merge conflict to Gerd's earlier ACPI pull).
But actually I was referring to my qom-cpu pull. ;) It automatically
rebases, too, so no drama.

[snip]

Thanks for your kind words. I was not trying to push QOM authoring work
on you, just wondering about who applies which patches when. What I was
looking for is some constructive feedback on what I should or shouldn't
do to help clarify responsibilities:

> I think it would be nice if you could take patches like this which are
> close to your CPU patches and keep them in your tree, or just NAK
> patches which conflict with other work. That kind of coordination
> would avoid staging problems.

I take it that adding an Acked-by to patches and sending a PULL later
without sending a "Thanks applied" reply was not a good idea of mine,
possibly confusing Anthony's mailbox script.

Your response I'm interpreting as I should've replied that I'm
investigating the enhancement instead of waiting until I have results to
share of whether or not it can be done better in one go (and not as
being some formal MAINTAINERS file pattern documentation issue).

And my wait on the frozen qom-cpu branch was resolvable with a scripted
double-rebase first on qom-cpu, then on master, to simulate a qom-cpu
merge for the new follow-up series.

> What's for example the plan for Igor's series, should they be applied
> next or something else?

I've been discussing our next steps with both Igor and Eduardo (who have
been frequently rebasing onto each other, leading to some confusion on
my part) with no final conclusion. For me it depends how different
approaches work out patch-wise. On my x86 radar is this bulk:

* finish CPU-as-a-device: the latest series does not realize/init CPUs
unlike Anthony's earlier proposal, looks okay otherwise; will affect
properties series and is affected by decisions about how to implement
QOM realize
* VMState: can we reuse DeviceClass::vmsd or do we need our own? open
question from Juan's series: how to integrate machine.c VMState code?
* subclasses: me having investigated fast-tracking host-x86-cpu; results
negative, but starting to be untangled in new series.
* sh4/alpha as small test runs for the proposed subclass type name
scheme; issue: some targets like sh4 do case-insensitive name
comparisons and (unposted) sh4 proposal of CPU name field search does
not scale well; to-be-evaluated ideas beyond alpha v2: lower-casing in
addition to appending -<arch>-cpu prefix; do we need a CPUClass callback
for -cpu argument -> type name or is it a fully-custom per target decision?
* avoid and fix the following bug: qemu-system-arm -cpu tmp105
* X86CPU subclasses: me investigating (with interruptions) a slim
class_init-based conversion approach without waiting on all properties,
to restructure the x86_def_t-centric helper functions
* x86 CPU properties: there were still some naming issues in the last
version I reviewed, ordering under discussion
* HyperV properties: waiting on the previous properties series
* QOM realize: if we do realize at device-level and the CPU becomes a
device, it will benefit from the infrastructure but will need
Object->DeviceState signature changes; realize at device-level cannot
propagate recursively through non-device Container (e.g., "/machine",
"/machine/unassigned")
* lots of open issues: canonical paths for CPUs and their APIC etc.
children; more CPU_COMMON->CPUState field moves needed for x86
socket-based hotplug; linux-user cpu_copy() / cpu_clone_regs(); icount
field movements causing qtest failure; ...

Plus obviously my attempts to bring the remaining non-x86 targets on
par. Thanks again for your support in slowly crawling forward there.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Eduardo Habkost]

On Mon, Dec 10, 2012 at 01:13:06AM +0100, Andreas Färber wrote:
> Am 09.12.2012 21:46, schrieb Blue Swirl:
> > On Sun, Dec 9, 2012 at 7:13 PM, Andreas Färber <afaerber@suse.de> wrote:
[...]
> > What's for example the plan for Igor's series, should they be applied
> > next or something else?
> 
> I've been discussing our next steps with both Igor and Eduardo (who have
> been frequently rebasing onto each other, leading to some confusion on
> my part) with no final conclusion. For me it depends how different
> approaches work out patch-wise. On my x86 radar is this bulk:
> 
> * finish CPU-as-a-device: the latest series does not realize/init CPUs
> unlike Anthony's earlier proposal, looks okay otherwise; will affect
> properties series and is affected by decisions about how to implement
> QOM realize

This is holding the CPU properties work (that in turn is holding the CPU
subclasses work, that will probably be much simpler to do using the CPU
properties work as base).

If there are no problems with the series, I would be very happy If we
manage to get it in a pull request before new year. But I didn't get a
single comment as reply to the v10 series, yet. :(



> * VMState: can we reuse DeviceClass::vmsd or do we need our own? open
> question from Juan's series: how to integrate machine.c VMState code?

I haven't even considered how to address VMState. I hope other people
are looking into that.  :)


> * subclasses: me having investigated fast-tracking host-x86-cpu; results
> negative, but starting to be untangled in new series.

I think I will probably be able to submit a RFC that converts "host"
first (and in a very simple way), then convert the rest of CPU models.
But I will send it as RFC because I think the results will look better
after the CPU properties code is integrated.


> * sh4/alpha as small test runs for the proposed subclass type name
> scheme; issue: some targets like sh4 do case-insensitive name
> comparisons and (unposted) sh4 proposal of CPU name field search does
> not scale well; to-be-evaluated ideas beyond alpha v2: lower-casing in
> addition to appending -<arch>-cpu prefix; do we need a CPUClass callback
> for -cpu argument -> type name or is it a fully-custom per target decision?
> * avoid and fix the following bug: qemu-system-arm -cpu tmp105
> * X86CPU subclasses: me investigating (with interruptions) a slim
> class_init-based conversion approach without waiting on all properties,
> to restructure the x86_def_t-centric helper functions

I think we all converged to a class_init-based approach. We have your
RFC, and I have a series to do that as well. Anyway I think we could at
least convert the feature word fields into an array (series submitted by
me this week), to simplify the -cpu check/enforce and -cpu host code. I
plan to send a new x86-subclass RFC this week.


> * x86 CPU properties: there were still some naming issues in the last
> version I reviewed, ordering under discussion

Right now I am waiting for a new version of the series from Igor, and I
think it will be easier to implement X86CPU classes using it as base. (I
don't know if he plans to submit a new version soon)


> * HyperV properties: waiting on the previous properties series
> * QOM realize: if we do realize at device-level and the CPU becomes a
> device, it will benefit from the infrastructure but will need
> Object->DeviceState signature changes; realize at device-level cannot
> propagate recursively through non-device Container (e.g., "/machine",
> "/machine/unassigned")

I haven't looked at the QOM realize efforts, except by trying to make
cpu_x86_realize() independent and separate from the rest of the X86CPU
creation/initialization code.


> * lots of open issues: canonical paths for CPUs and their APIC etc.
> children; more CPU_COMMON->CPUState field moves needed for x86
> socket-based hotplug; linux-user cpu_copy() / cpu_clone_regs(); icount
> field movements causing qtest failure; ...

I consider the APIC-ID-related topology bug something we really need to
fix in 1.4, and that probably can't wait until we remodel everything.

Anyway, most of the work required in my experiments to fix the bug
involved the PC initialization code (because we need to enable
bug-compatible behavior on older machine-types), with small impact into
the internal X86CPU code. I guess that's good news: some time ago the
fix required lots of changes in the CPU initialization code, and now the
CPU create()+realize() interface looks good enough to allow a simple fix
to the problem without touching internal CPU code.


> 
> Plus obviously my attempts to bring the remaining non-x86 targets on
> par. Thanks again for your support in slowly crawling forward there.
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

Re: [Qemu-devel] [PATCH] finally kill cpudef config section support

[Eduardo Habkost]

On Sat, Dec 08, 2012 at 07:02:39PM +0100, Andreas Färber wrote:
> Am 08.12.2012 18:54, schrieb Blue Swirl:
> > Thanks, applied.
> 
> As discussed this still leaves some cpudef cruft behind.
> 
> Eduardo, can you please send a follow-up to move the versioning code and
> kills all cpudef_*(), now that the patch got prematurely applied?

Discussions about process aside, I don't see the relation between
removing the entry for the "cpudef" config section from qemu-option.c
(that's basically an unused variable, as no code used the config section
since QEMU 1.2), and kiling the cpudef_*() functions (that aside from
being named "cpudef_*", have nothing to do with the cpudef config
section). Both cleanups are completely independent from each other.

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu 0/4] target-i386: Finish killing cpudef support

[Andreas Färber]

Hello,

This mini-series follows up the removal of cpudef config section support.
It cleans up the list of x86 definitions previously parsed from the cpudefs
and rips out everything cpudef_*, making remaining x86_cpudef_setup() static.
This is done by calling our setup code from class_init - an alternative would
be type registration time.

As a bonus I'm proposing to document my coordinating the x86 CPU rework in
the MAINTAINERS file.

This series is based on my qom-cpu-next branch (pending qom-cpu + latest master)
and is intended to be included in an upcoming qom-cpu pull, just to be clear. :)

Regards,
Andreas

Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>

Andreas Färber (4):
  target-i386: Inline -cpu host check into cpu_x86_register()
  target-i386: Drop redundant list of CPU definitions
  Really finally kill cpudef config section support
  MAINTAINERS: Include X86CPU in CPU maintenance area

 MAINTAINERS       |    1 +
 arch_init.c       |    7 -------
 arch_init.h       |    1 -
 bsd-user/main.c   |    3 ---
 linux-user/main.c |    3 ---
 target-i386/cpu.c |   54 ++++++++++++++++++++++++++---------------------------
 target-i386/cpu.h |    2 --
 vl.c              |    7 -------
 8 Dateien geändert, 27 Zeilen hinzugefügt(+), 51 Zeilen entfernt(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH qom-cpu 1/4] target-i386: Inline -cpu host check into cpu_x86_register()

[Andreas Färber]

Simplifies the upcoming cleanup of cpu_x86_find_by_name().

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.c |   12 +++++++-----
 1 Datei geändert, 7 Zeilen hinzugefügt(+), 5 Zeilen entfernt(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7be3ad8..a46faa2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1217,9 +1217,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
             break;
         }
     }
-    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
-        kvm_cpu_fill_host(x86_cpu_def);
-    } else if (!def) {
+    if (!def) {
         return -1;
     } else {
         memcpy(x86_cpu_def, def, sizeof(*def));
@@ -1505,8 +1503,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     name = model_pieces[0];
     features = model_pieces[1];
 
-    if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+    if (kvm_enabled() && strcmp(name, "host") == 0) {
+        kvm_cpu_fill_host(def);
+    } else {
+        if (cpu_x86_find_by_name(def, name) < 0) {
+            goto error;
+        }
     }
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 1/4] target-i386: Inline -cpu host check into cpu_x86_register()

[Eduardo Habkost]

On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote:
> Simplifies the upcoming cleanup of cpu_x86_find_by_name().

...by making cpu_x86_register() more complicated, and having CPU model
name lookup spread into different parts of the code.

The CPU model lookup is a bit complex because of the "host" exception,
but at least the complexity was hidden inside cpu_x86_find_by_name()
(making it very easy to replace that logic by a CPU subclass lookup,
later).

> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  target-i386/cpu.c |   12 +++++++-----
>  1 Datei geändert, 7 Zeilen hinzugefügt(+), 5 Zeilen entfernt(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7be3ad8..a46faa2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1217,9 +1217,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>              break;
>          }
>      }
> -    if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> -        kvm_cpu_fill_host(x86_cpu_def);
> -    } else if (!def) {
> +    if (!def) {
>          return -1;
>      } else {
>          memcpy(x86_cpu_def, def, sizeof(*def));
> @@ -1505,8 +1503,12 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
> -    if (cpu_x86_find_by_name(def, name) < 0) {
> -        goto error;
> +    if (kvm_enabled() && strcmp(name, "host") == 0) {
> +        kvm_cpu_fill_host(def);
> +    } else {
> +        if (cpu_x86_find_by_name(def, name) < 0) {
> +            goto error;
> +        }
>      }
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> -- 
> 1.7.10.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 1/4] target-i386: Inline -cpu host check into cpu_x86_register()

[Igor Mammedov]

On Mon, 10 Dec 2012 10:46:13 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote:
> > Simplifies the upcoming cleanup of cpu_x86_find_by_name().
> 
> ...by making cpu_x86_register() more complicated, and having CPU model
> name lookup spread into different parts of the code.
> 
> The CPU model lookup is a bit complex because of the "host" exception,
> but at least the complexity was hidden inside cpu_x86_find_by_name()
> (making it very easy to replace that logic by a CPU subclass lookup,
> later).
+1 

[...]
> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH qom-cpu 1/4] target-i386: Inline -cpu host check into cpu_x86_register()

[Andreas Färber]

Am 10.12.2012 19:55, schrieb Igor Mammedov:
> On Mon, 10 Dec 2012 10:46:13 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote:
>>> Simplifies the upcoming cleanup of cpu_x86_find_by_name().
>>
>> ...by making cpu_x86_register() more complicated, and having CPU model
>> name lookup spread into different parts of the code.
>>
>> The CPU model lookup is a bit complex because of the "host" exception,
>> but at least the complexity was hidden inside cpu_x86_find_by_name()
>> (making it very easy to replace that logic by a CPU subclass lookup,
>> later).

(Somehow I didn't get this message ... yet)

> +1

Could you guys re-review this in light of the subclasses patch? The
issue I was facing is that I did not see a reliable way to register the
host class depending on kvm_enabled(). Therefore ..._find_by_name()
becomes the class lookup whereas the host check remains a special check
in cpu_x86_init() [a faulty one I now see, ignoring "host-x86_64-cpu"].
cpu_x86_register() is problematic in that it gets the X86CPU served on a
silver plate, so it's too late to choose subclasses in that function.

Andreas

> 
> [...]
>>
>> -- 
>> Eduardo
>>
> 
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu 1/4] target-i386: Inline -cpu host check into cpu_x86_register()

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 12:21:10AM +0100, Andreas Färber wrote:
> Am 10.12.2012 19:55, schrieb Igor Mammedov:
> > On Mon, 10 Dec 2012 10:46:13 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> >> On Sun, Dec 09, 2012 at 08:45:50PM +0100, Andreas Färber wrote:
> >>> Simplifies the upcoming cleanup of cpu_x86_find_by_name().
> >>
> >> ...by making cpu_x86_register() more complicated, and having CPU model
> >> name lookup spread into different parts of the code.
> >>
> >> The CPU model lookup is a bit complex because of the "host" exception,
> >> but at least the complexity was hidden inside cpu_x86_find_by_name()
> >> (making it very easy to replace that logic by a CPU subclass lookup,
> >> later).
> 
> (Somehow I didn't get this message ... yet)
> 
> > +1
> 
> Could you guys re-review this in light of the subclasses patch? The
> issue I was facing is that I did not see a reliable way to register the
> host class depending on kvm_enabled(). Therefore ..._find_by_name()
> becomes the class lookup whereas the host check remains a special check
> in cpu_x86_init() [a faulty one I now see, ignoring "host-x86_64-cpu"].
> cpu_x86_register() is problematic in that it gets the X86CPU served on a
> silver plate, so it's too late to choose subclasses in that function.

I have a patch[1] to change that in my APIC-ID-topology work in progress
branch[2]. I can submit this (and maybe a few other code movements) as
PATCH tomorrow.

I have a branch for CPU subclasses using an approach similar to the one
you used, but that includes "host" as well[3]. But I was waiting until
the properties series is resubmitted, so I could use it as base and make
the subclass code simpler. I will take a deeper look at your RFC
tomorrow, and probably send additional comments.


[1] https://github.com/ehabkost/qemu-hacks/commit/850c5531c84839919d650b2dac6e98fa4c2529c4
[2] https://github.com/ehabkost/qemu-hacks/commits/work/apicid
[3] https://github.com/ehabkost/qemu-hacks/commits/work/cpu-model-classes

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu 2/4] target-i386: Drop redundant list of CPU definitions

[Andreas Färber]

Since we are no longer parsing cpudefs from config files, only the array
of built-in definitions remains.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.c |   37 ++++++++++++++-----------------------
 1 Datei geändert, 14 Zeilen hinzugefügt(+), 23 Zeilen entfernt(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a46faa2..e265317 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -330,11 +330,7 @@ typedef struct x86_def_t {
 #define TCG_SVM_FEATURES 0
 #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
 
-/* maintains list of cpu model definitions
- */
-static x86_def_t *x86_defs = {NULL};
-
-/* built-in cpu model definitions (deprecated)
+/* built-in cpu model definitions
  */
 static x86_def_t builtin_x86_defs[] = {
     {
@@ -1210,20 +1206,17 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
 
 static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
 {
-    x86_def_t *def;
+    int i;
 
-    for (def = x86_defs; def; def = def->next) {
-        if (name && !strcmp(name, def->name)) {
-            break;
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
+        x86_def_t *def = &builtin_x86_defs[i];
+        if (strcmp(name, def->name) == 0) {
+            memcpy(x86_cpu_def, def, sizeof(*def));
+            return 0;
         }
     }
-    if (!def) {
-        return -1;
-    } else {
-        memcpy(x86_cpu_def, def, sizeof(*def));
-    }
 
-    return 0;
+    return -1;
 }
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
@@ -1418,10 +1411,11 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
 /* generate CPU information. */
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    x86_def_t *def;
+    int i;
     char buf[256];
 
-    for (def = x86_defs; def; def = def->next) {
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
+        x86_def_t *def = &builtin_x86_defs[i];
         snprintf(buf, sizeof(buf), "%s", def->name);
         (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
     }
@@ -1442,14 +1436,14 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
-    x86_def_t *def;
+    int i;
 
-    for (def = x86_defs; def; def = def->next) {
+    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
         CpuDefinitionInfoList *entry;
         CpuDefinitionInfo *info;
 
         info = g_malloc0(sizeof(*info));
-        info->name = g_strdup(def->name);
+        info->name = g_strdup(builtin_x86_defs[i].name);
 
         entry = g_malloc0(sizeof(*entry));
         entry->value = info;
@@ -1598,7 +1592,6 @@ void x86_cpudef_setup(void)
 
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
         x86_def_t *def = &builtin_x86_defs[i];
-        def->next = x86_defs;
 
         /* Look for specific "cpudef" models that */
         /* have the QEMU version in .model_id */
@@ -1611,8 +1604,6 @@ void x86_cpudef_setup(void)
                 break;
             }
         }
-
-        x86_defs = def;
     }
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 2/4] target-i386: Drop redundant list of CPU definitions

[Eduardo Habkost]

On Sun, Dec 09, 2012 at 08:45:51PM +0100, Andreas Färber wrote:
> Since we are no longer parsing cpudefs from config files, only the array
> of built-in definitions remains.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

But this probably has to be redone if you remove patch 1/4.

I believe the code in cpu_x86_find_by_name() will get much simpler and
friendlier to change if we simply move the strcmp(name, "host") check
above the model list lookup (instead of moving it to
cpu_x86_register()).


> ---
>  target-i386/cpu.c |   37 ++++++++++++++-----------------------
>  1 Datei geändert, 14 Zeilen hinzugefügt(+), 23 Zeilen entfernt(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a46faa2..e265317 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -330,11 +330,7 @@ typedef struct x86_def_t {
>  #define TCG_SVM_FEATURES 0
>  #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
>  
> -/* maintains list of cpu model definitions
> - */
> -static x86_def_t *x86_defs = {NULL};
> -
> -/* built-in cpu model definitions (deprecated)
> +/* built-in cpu model definitions
>   */
>  static x86_def_t builtin_x86_defs[] = {
>      {
> @@ -1210,20 +1206,17 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>  
>  static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *name)
>  {
> -    x86_def_t *def;
> +    int i;
>  
> -    for (def = x86_defs; def; def = def->next) {
> -        if (name && !strcmp(name, def->name)) {
> -            break;
> +    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> +        x86_def_t *def = &builtin_x86_defs[i];
> +        if (strcmp(name, def->name) == 0) {
> +            memcpy(x86_cpu_def, def, sizeof(*def));
> +            return 0;
>          }
>      }
> -    if (!def) {
> -        return -1;
> -    } else {
> -        memcpy(x86_cpu_def, def, sizeof(*def));
> -    }
>  
> -    return 0;
> +    return -1;
>  }
>  
>  /* Parse "+feature,-feature,feature=foo" CPU feature string
> @@ -1418,10 +1411,11 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
>  /* generate CPU information. */
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    x86_def_t *def;
> +    int i;
>      char buf[256];
>  
> -    for (def = x86_defs; def; def = def->next) {
> +    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> +        x86_def_t *def = &builtin_x86_defs[i];
>          snprintf(buf, sizeof(buf), "%s", def->name);
>          (*cpu_fprintf)(f, "x86 %16s  %-48s\n", buf, def->model_id);
>      }
> @@ -1442,14 +1436,14 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
> -    x86_def_t *def;
> +    int i;
>  
> -    for (def = x86_defs; def; def = def->next) {
> +    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
>          CpuDefinitionInfoList *entry;
>          CpuDefinitionInfo *info;
>  
>          info = g_malloc0(sizeof(*info));
> -        info->name = g_strdup(def->name);
> +        info->name = g_strdup(builtin_x86_defs[i].name);
>  
>          entry = g_malloc0(sizeof(*entry));
>          entry->value = info;
> @@ -1598,7 +1592,6 @@ void x86_cpudef_setup(void)
>  
>      for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) {
>          x86_def_t *def = &builtin_x86_defs[i];
> -        def->next = x86_defs;
>  
>          /* Look for specific "cpudef" models that */
>          /* have the QEMU version in .model_id */
> @@ -1611,8 +1604,6 @@ void x86_cpudef_setup(void)
>                  break;
>              }
>          }
> -
> -        x86_defs = def;
>      }
>  }
>  
> -- 
> 1.7.10.4
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu 3/4] Really finally kill cpudef config section support

[Andreas Färber]

Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
config section support) removed the cpudef parsing support but left
cpudef_* hooks behind. Remove those.

Since TYPE_X86_CPU currently is the only CPU class and CPUs are
instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
its class_init to bootstrap the list of model definitions until we have
these as subclasses. Avoid code movements for x86_cpudef_setup() to not
interfere with the various approaches to drop it in favor of subclasses.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Blue Swirl <blauwirbel@gmail.com>
---
 arch_init.c       |    7 -------
 arch_init.h       |    1 -
 bsd-user/main.c   |    3 ---
 linux-user/main.c |    3 ---
 target-i386/cpu.c |    7 ++++++-
 target-i386/cpu.h |    2 --
 vl.c              |    7 -------
 7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)

diff --git a/arch_init.c b/arch_init.c
index e6effe8..bea1b9c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1124,13 +1124,6 @@ void do_smbios_option(const char *optarg)
 #endif
 }
 
-void cpudef_init(void)
-{
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file */
-#endif
-}
-
 int audio_available(void)
 {
 #ifdef HAS_AUDIO
diff --git a/arch_init.h b/arch_init.h
index 5fc780c..84a7f9a 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -27,7 +27,6 @@ extern const uint32_t arch_type;
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
-void cpudef_init(void);
 int audio_available(void);
 void audio_init(ISABus *isa_bus, PCIBus *pci_bus);
 int tcg_available(void);
diff --git a/bsd-user/main.c b/bsd-user/main.c
index 095ae8e..cf0a345 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -762,9 +762,6 @@ int main(int argc, char **argv)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     optind = 1;
     for(;;) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 25e35cd..e881fcf 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3426,9 +3426,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     cpu_model = NULL;
-#if defined(cpudef_setup)
-    cpudef_setup(); /* parse cpu definitions in target config file (TBD) */
-#endif
 
     /* init debug */
     cpu_set_log_filename(log_file);
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e265317..13de2e3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1414,6 +1414,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     int i;
     char buf[256];
 
+    /* Force creation of CPU class */
+    object_class_by_name(TYPE_X86_CPU);
+
     for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
         x86_def_t *def = &builtin_x86_defs[i];
         snprintf(buf, sizeof(buf), "%s", def->name);
@@ -1585,7 +1588,7 @@ void cpu_clear_apic_feature(CPUX86State *env)
 
 /* Initialize list of CPU models, filling some non-static fields if necessary
  */
-void x86_cpudef_setup(void)
+static void x86_cpudef_setup(void)
 {
     int i, j;
     static const char *model_with_versions[] = { "qemu32", "qemu64", "athlon" };
@@ -2138,6 +2141,8 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
     xcc->parent_reset = cc->reset;
     cc->reset = x86_cpu_reset;
+
+    x86_cpudef_setup();
 }
 
 static const TypeInfo x86_cpu_type_info = {
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..bd6aec4 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -865,7 +865,6 @@ typedef struct CPUX86State {
 X86CPU *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
-void x86_cpudef_setup(void);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
 
 int cpu_get_pic_interrupt(CPUX86State *s);
@@ -1047,7 +1046,6 @@ static inline CPUX86State *cpu_init(const char *cpu_model)
 #define cpu_gen_code cpu_x86_gen_code
 #define cpu_signal_handler cpu_x86_signal_handler
 #define cpu_list x86_cpu_list
-#define cpudef_setup	x86_cpudef_setup
 
 #define CPU_SAVE_VERSION 12
 
diff --git a/vl.c b/vl.c
index a3ab384..9d867c2 100644
--- a/vl.c
+++ b/vl.c
@@ -3560,13 +3560,6 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     }
 
-    /* Init CPU def lists, based on config
-     * - Must be called after all the qemu_read_config_file() calls
-     * - Must be called before list_cpus()
-     * - Must be called before machine->init()
-     */
-    cpudef_init();
-
     if (cpu_model && is_help_option(cpu_model)) {
         list_cpus(stdout, &fprintf, cpu_model);
         exit(0);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 3/4] Really finally kill cpudef config section support

[Eduardo Habkost]

On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
> config section support) removed the cpudef parsing support but left
> cpudef_* hooks behind. Remove those.

The cpudef_* functions have nothing to do with the cpudef config section
since QEMU 1.2, it is just about initializing CPU-definition-related
data structures, so the patch subject is a bit misleading.

But I agree with the changes done here, and patch looks good to me, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> 
> Since TYPE_X86_CPU currently is the only CPU class and CPUs are
> instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
> its class_init to bootstrap the list of model definitions until we have
> these as subclasses. Avoid code movements for x86_cpudef_setup() to not
> interfere with the various approaches to drop it in favor of subclasses.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> ---
>  arch_init.c       |    7 -------
>  arch_init.h       |    1 -
>  bsd-user/main.c   |    3 ---
>  linux-user/main.c |    3 ---
>  target-i386/cpu.c |    7 ++++++-
>  target-i386/cpu.h |    2 --
>  vl.c              |    7 -------
>  7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
> 
[...]

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 3/4] Really finally kill cpudef config section support

[Andreas Färber]

Am 10.12.2012 19:09, schrieb Eduardo Habkost:
> On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
>> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
>> config section support) removed the cpudef parsing support but left
>> cpudef_* hooks behind. Remove those.
> 
> The cpudef_* functions have nothing to do with the cpudef config section
> since QEMU 1.2, it is just about initializing CPU-definition-related
> data structures, so the patch subject is a bit misleading.

My memory tells me they were specifically added for the config file
support... git-blame proves me wrong and shows they were added by Johan
Cooper and refactored by Blue, explaining his sudden cpudef patch
involvement. Sorry.

Do you have a proposal for a better text? My reasoning is we should
clean up before we forget about it and things stay behind.

Andreas

> 
> But I agree with the changes done here, and patch looks good to me, so:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
>>
>> Since TYPE_X86_CPU currently is the only CPU class and CPUs are
>> instantiated exclusively through QOM (i.e., cpu_x86_init()), we can use
>> its class_init to bootstrap the list of model definitions until we have
>> these as subclasses. Avoid code movements for x86_cpudef_setup() to not
>> interfere with the various approaches to drop it in favor of subclasses.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  arch_init.c       |    7 -------
>>  arch_init.h       |    1 -
>>  bsd-user/main.c   |    3 ---
>>  linux-user/main.c |    3 ---
>>  target-i386/cpu.c |    7 ++++++-
>>  target-i386/cpu.h |    2 --
>>  vl.c              |    7 -------
>>  7 Dateien geändert, 6 Zeilen hinzugefügt(+), 24 Zeilen entfernt(-)
>>
> [...]
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu 3/4] Really finally kill cpudef config section support

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 12:12:23AM +0100, Andreas Färber wrote:
> Am 10.12.2012 19:09, schrieb Eduardo Habkost:
> > On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
> >> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
> >> config section support) removed the cpudef parsing support but left
> >> cpudef_* hooks behind. Remove those.
> > 
> > The cpudef_* functions have nothing to do with the cpudef config section
> > since QEMU 1.2, it is just about initializing CPU-definition-related
> > data structures, so the patch subject is a bit misleading.
> 
> My memory tells me they were specifically added for the config file
> support... git-blame proves me wrong and shows they were added by Johan
> Cooper and refactored by Blue, explaining his sudden cpudef patch
> involvement. Sorry.
> 
> Do you have a proposal for a better text? My reasoning is we should
> clean up before we forget about it and things stay behind.

Something like "kill the cpudef_setup() hook that we don't need anymore
[because only x86 uses it, and we can simply use class_init to
initialize data structures]" sounds good enough to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 3/4] Really finally kill cpudef config section support

[Wenchao Xia]

于 2012-12-11 7:53, Eduardo Habkost 写道:
> On Tue, Dec 11, 2012 at 12:12:23AM +0100, Andreas Färber wrote:
>> Am 10.12.2012 19:09, schrieb Eduardo Habkost:
>>> On Sun, Dec 09, 2012 at 08:45:52PM +0100, Andreas Färber wrote:
>>>> Commit 511c68d3af626cb0a39034cb77e7ac64d3a26c0c (finally kill cpudef
>>>> config section support) removed the cpudef parsing support but left
>>>> cpudef_* hooks behind. Remove those.
>>>
>>> The cpudef_* functions have nothing to do with the cpudef config section
>>> since QEMU 1.2, it is just about initializing CPU-definition-related
>>> data structures, so the patch subject is a bit misleading.
>>
>> My memory tells me they were specifically added for the config file
>> support... git-blame proves me wrong and shows they were added by Johan
>> Cooper and refactored by Blue, explaining his sudden cpudef patch
>> involvement. Sorry.
>>
>> Do you have a proposal for a better text? My reasoning is we should
>> clean up before we forget about it and things stay behind.
>
> Something like "kill the cpudef_setup() hook that we don't need anymore
> [because only x86 uses it, and we can simply use class_init to
> initialize data structures]" sounds good enough to me.
>
Hi, Eduardo
   I think we need uninstall rules which should delete the def files,
otherwise we met error:
qemu-system-x86_64:/usr/local/etc/qemu/target-x86_64.conf:3: There is no 
option group 'cpudef'

-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH qom-cpu 4/4] MAINTAINERS: Include X86CPU in CPU maintenance area

[Andreas Färber]

Document that the x86 CPU refactorings are going through the qom-cpu
tree. This does not contradict the established practice that patches
adding KVM features to the x86 CPU go through the qemu-kvm tree.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Marcello Tosatti <mtosatti@redhat.com>
---
 MAINTAINERS |    1 +
 1 Datei geändert, 1 Zeile hinzugefügt(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2ede20d..61d5a4b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -599,6 +599,7 @@ M: Andreas Färber <afaerber@suse.de>
 S: Supported
 F: qom/cpu.c
 F: include/qemu/cpu.h
+F: target-i386/cpu.c
 
 Device Tree
 M: Peter Crosthwaite <peter.crosthwaite@petalogix.com>
-- 
1.7.10.4