[Qemu-devel] [PULL 00/11] X86 patches

[Qemu-devel] [PULL 00/11] X86 patches

[Eduardo Habkost]

The following changes since commit 3d30395f7fb3315e4ecf0de4e48790e1326bbd47:

  Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-20150218-1' into staging (2015-02-25 11:54:15 +0000)

are available in the git repository at:

  https://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to de13197a38cf45c990802661a057f64a05426cbc:

  target-i386: Move APIC ID compatibility code to pc.c (2015-02-25 15:00:07 -0300)

----------------------------------------------------------------
Those patches were reviewed some time ago, and Paolo suggested I submit them
through my own tree. So, here is my first x86 pull request. :)
----------------------------------------------------------------

Eduardo Habkost (11):
  target-i386: Simplify listflags() function
  target-i386: Eliminate unnecessary get_cpuid_vendor() function
  target-i386: Move topology.h to include/hw/i386
  target-i386: Rename cpu_x86_init() to cpu_x86_init_user()
  target-i386: Eliminate cpu_init() function
  target-i386: Simplify error handling on cpu_x86_init_user()
  target-i386: Move CPUX86State.cpuid_apic_id to X86CPU.apic_id
  linux-user: Check for cpu_init() errors
  target-i386: Set APIC ID using cpu_index on CONFIG_USER
  target-i386: Require APIC ID to be explicitly set before CPU realize
  target-i386: Move APIC ID compatibility code to pc.c

 hw/i386/pc.c                                |  35 +++++++
 {target-i386 => include/hw/i386}/topology.h |   6 +-
 linux-user/main.c                           |   9 +-
 target-i386/cpu-qom.h                       |   1 +
 target-i386/cpu.c                           | 148 ++++++++++------------------
 target-i386/cpu.h                           |  13 +--
 target-i386/kvm.c                           |   2 +-
 tests/Makefile                              |   2 -
 tests/test-x86-cpuid.c                      |   2 +-
 9 files changed, 104 insertions(+), 114 deletions(-)
 rename {target-i386 => include/hw/i386}/topology.h (97%)

-- 
2.1.0

[Qemu-devel] [PULL 01/11] target-i386: Simplify listflags() function

[Eduardo Habkost]

listflags() had lots of unnecessary complexity. Instead of printing to a
buffer that will be immediately printed, simply call the printing
function directly. Also, remove the fbits and flags arguments that were
always set to the same value. Also, there's no need to list the flags in
reverse order.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 42 ++++++++++++++----------------------------
 1 file changed, 14 insertions(+), 28 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3a9b32e..39d2fda 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1911,34 +1911,19 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     }
 }
 
-/* generate a composite string into buf of all cpuid names in featureset
- * selected by fbits.  indicate truncation at bufsize in the event of overflow.
- * if flags, suppress names undefined in featureset.
+/* Print all cpuid feature names in featureset
  */
-static void listflags(char *buf, int bufsize, uint32_t fbits,
-                      const char **featureset, uint32_t flags)
-{
-    const char **p = &featureset[31];
-    char *q, *b, bit;
-    int nc;
-
-    b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
-    *buf = '\0';
-    for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), --bit)
-        if (fbits & 1 << bit && (*p || !flags)) {
-            if (*p)
-                nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
-            else
-                nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", bit);
-            if (bufsize <= nc) {
-                if (b) {
-                    memcpy(b, "...", sizeof("..."));
-                }
-                return;
-            }
-            q += nc;
-            bufsize -= nc;
+static void listflags(FILE *f, fprintf_function print, const char **featureset)
+{
+    int bit;
+    bool first = true;
+
+    for (bit = 0; bit < 32; bit++) {
+        if (featureset[bit]) {
+            print(f, "%s%s", first ? "" : " ", featureset[bit]);
+            first = false;
         }
+    }
 }
 
 /* generate CPU information. */
@@ -1963,8 +1948,9 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
     for (i = 0; i < ARRAY_SIZE(feature_word_info); i++) {
         FeatureWordInfo *fw = &feature_word_info[i];
 
-        listflags(buf, sizeof(buf), (uint32_t)~0, fw->feat_names, 1);
-        (*cpu_fprintf)(f, "  %s\n", buf);
+        (*cpu_fprintf)(f, "  ");
+        listflags(f, cpu_fprintf, fw->feat_names);
+        (*cpu_fprintf)(f, "\n");
     }
 }
 
-- 
2.1.0

[Qemu-devel] [PULL 02/11] target-i386: Eliminate unnecessary get_cpuid_vendor() function

[Eduardo Habkost]

The function was used in only two places. In one of them, the function
made the code less readable by requiring temporary te[bcd]x variables.
In the other one we can simply inline the existing code.

Reviewed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 39d2fda..8c44e5b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2214,14 +2214,6 @@ void x86_cpudef_setup(void)
     }
 }
 
-static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
-                             uint32_t *ecx, uint32_t *edx)
-{
-    *ebx = env->cpuid_vendor1;
-    *edx = env->cpuid_vendor2;
-    *ecx = env->cpuid_vendor3;
-}
-
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx)
@@ -2255,7 +2247,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     switch(index) {
     case 0:
         *eax = env->cpuid_level;
-        get_cpuid_vendor(env, ebx, ecx, edx);
+        *ebx = env->cpuid_vendor1;
+        *edx = env->cpuid_vendor2;
+        *ecx = env->cpuid_vendor3;
         break;
     case 1:
         *eax = env->cpuid_version;
@@ -2448,11 +2442,9 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
          * So dont set it here for Intel to make Linux guests happy.
          */
         if (cs->nr_cores * cs->nr_threads > 1) {
-            uint32_t tebx, tecx, tedx;
-            get_cpuid_vendor(env, &tebx, &tecx, &tedx);
-            if (tebx != CPUID_VENDOR_INTEL_1 ||
-                tedx != CPUID_VENDOR_INTEL_2 ||
-                tecx != CPUID_VENDOR_INTEL_3) {
+            if (env->cpuid_vendor1 != CPUID_VENDOR_INTEL_1 ||
+                env->cpuid_vendor2 != CPUID_VENDOR_INTEL_2 ||
+                env->cpuid_vendor3 != CPUID_VENDOR_INTEL_3) {
                 *ecx |= 1 << 1;    /* CmpLegacy bit */
             }
         }
-- 
2.1.0

[Qemu-devel] [PULL 03/11] target-i386: Move topology.h to include/hw/i386

[Eduardo Habkost]

This will allow the PC code to use the header, and lets us eliminate the
QEMU_INCLUDES hack inside tests/Makefile.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 {target-i386 => include/hw/i386}/topology.h | 6 +++---
 target-i386/cpu.c                           | 2 +-
 tests/Makefile                              | 2 --
 tests/test-x86-cpuid.c                      | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)
 rename {target-i386 => include/hw/i386}/topology.h (97%)

diff --git a/target-i386/topology.h b/include/hw/i386/topology.h
similarity index 97%
rename from target-i386/topology.h
rename to include/hw/i386/topology.h
index 07a6c5f..9c6f3a9 100644
--- a/target-i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -21,8 +21,8 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef TARGET_I386_TOPOLOGY_H
-#define TARGET_I386_TOPOLOGY_H
+#ifndef HW_I386_TOPOLOGY_H
+#define HW_I386_TOPOLOGY_H
 
 /* This file implements the APIC-ID-based CPU topology enumeration logic,
  * documented at the following document:
@@ -131,4 +131,4 @@ static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
     return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
 }
 
-#endif /* TARGET_I386_TOPOLOGY_H */
+#endif /* HW_I386_TOPOLOGY_H */
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8c44e5b..356f97e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,7 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
diff --git a/tests/Makefile b/tests/Makefile
index 307035c..7d4b96d 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -239,8 +239,6 @@ $(test-obj-y): QEMU_INCLUDES += -Itests
 QEMU_CFLAGS += -I$(SRC_PATH)/tests
 qom-core-obj = qom/object.o qom/qom-qobject.o qom/container.o
 
-tests/test-x86-cpuid.o: QEMU_INCLUDES += -I$(SRC_PATH)/target-i386
-
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
 tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c
index 8d9f96a..6cd20d4 100644
--- a/tests/test-x86-cpuid.c
+++ b/tests/test-x86-cpuid.c
@@ -24,7 +24,7 @@
 
 #include <glib.h>
 
-#include "topology.h"
+#include "hw/i386/topology.h"
 
 static void test_topo_bits(void)
 {
-- 
2.1.0

[Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Eduardo Habkost]

The function is used only for CONFIG_USER, so make its purpose clear.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 2 +-
 target-i386/cpu.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 356f97e..8f18556 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2135,7 +2135,7 @@ out:
     return cpu;
 }
 
-X86CPU *cpu_x86_init(const char *cpu_model)
+X86CPU *cpu_x86_init_user(const char *cpu_model)
 {
     Error *error = NULL;
     X86CPU *cpu;
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 478450c..41d7f0a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -982,7 +982,7 @@ typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init(const char *cpu_model);
+X86CPU *cpu_x86_init_user(const char *cpu_model);
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
@@ -1173,7 +1173,7 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 
 static inline CPUX86State *cpu_init(const char *cpu_model)
 {
-    X86CPU *cpu = cpu_x86_init(cpu_model);
+    X86CPU *cpu = cpu_x86_init_user(cpu_model);
     if (cpu == NULL) {
         return NULL;
     }
-- 
2.1.0

Re: [Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Andreas Färber]

Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> The function is used only for CONFIG_USER, so make its purpose clear.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 2 +-
>  target-i386/cpu.h | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Please don't. I happily got all architectures aligned that it's at least
cpu_something_init, and it only happens to be user-only for x86. It is
rather the legacy function that was used in both system and user.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Eduardo Habkost]

On Wed, Feb 25, 2015 at 11:06:55PM +0100, Andreas Färber wrote:
> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> > The function is used only for CONFIG_USER, so make its purpose clear.
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 2 +-
> >  target-i386/cpu.h | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> Please don't. I happily got all architectures aligned that it's at least
> cpu_something_init, and it only happens to be user-only for x86. It is
> rather the legacy function that was used in both system and user.

If that's a legacy function, what are the steps you plan to follow to
eliminate it? I would be glad to help eliminating legacy code.

Initialization of CPUs in *-user and *-softmmu is different in i386, so
we are going to have different code for both. How do you think I should
name the *-user-specific CPU init function in target-i386, then?

-- 
Eduardo

Re: [Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Andreas Färber]

Am 26.02.2015 um 16:59 schrieb Eduardo Habkost:
> On Wed, Feb 25, 2015 at 11:06:55PM +0100, Andreas Färber wrote:
>> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
>>> The function is used only for CONFIG_USER, so make its purpose clear.
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target-i386/cpu.c | 2 +-
>>>  target-i386/cpu.h | 4 ++--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> Please don't. I happily got all architectures aligned that it's at least
>> cpu_something_init, and it only happens to be user-only for x86. It is
>> rather the legacy function that was used in both system and user.
> 
> If that's a legacy function, what are the steps you plan to follow to
> eliminate it? I would be glad to help eliminating legacy code.
> 
> Initialization of CPUs in *-user and *-softmmu is different in i386, so
> we are going to have different code for both. How do you think I should
> name the *-user-specific CPU init function in target-i386, then?

I would prefer to leave its name as it is (unless we are renaming all,
which would probably be a waste of effort giving the next steps) and
simply not use it in PC code. If you want to enforce this, you could
#ifdef CONFIG_USER_ONLY it.

For some targets - as can be seen in your uc32 patch - there is already
a cpu_generic_init() that calls into the CPUClass hooks of the given CPU
type. I would like to call that from linux-user directly (or from a
lightweight wrapper to be shared between linux-user and bsd-user, I
assume we're going need some target-specific #ifdefs) and drop
cpu_init() in its current form. In particular I want to somehow move the
realized=true part out of it, which means either inlining it into dozens
of machines or finishing the recursive realization work so that we only
need one central realized=true for /machine.

My branch with the last realize_children code unfortunately ran into
conflicts with hotplug handler code and recursive un-realization and
hasn't been rebased in months. Paolo's comment had been that we would
need to assure by reordering (or at least prove it not-yet-necessary by
assertions) that the qdev-bus topology in addition to the QOM parents
have been realized before a given child device gets realized.

Additionally, /machine is its own type iirc, so will either need to
become a device or have its own "realized" property implementation
iterating over its direct, unassigned and peripheral children.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Eduardo Habkost]

On Fri, Feb 27, 2015 at 03:10:27PM +0100, Andreas Färber wrote:
> Am 26.02.2015 um 16:59 schrieb Eduardo Habkost:
> > On Wed, Feb 25, 2015 at 11:06:55PM +0100, Andreas Färber wrote:
> >> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> >>> The function is used only for CONFIG_USER, so make its purpose clear.
> >>>
> >>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> >>> ---
> >>>  target-i386/cpu.c | 2 +-
> >>>  target-i386/cpu.h | 4 ++--
> >>>  2 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> Please don't. I happily got all architectures aligned that it's at least
> >> cpu_something_init, and it only happens to be user-only for x86. It is
> >> rather the legacy function that was used in both system and user.
> > 
> > If that's a legacy function, what are the steps you plan to follow to
> > eliminate it? I would be glad to help eliminating legacy code.
> > 
> > Initialization of CPUs in *-user and *-softmmu is different in i386, so
> > we are going to have different code for both. How do you think I should
> > name the *-user-specific CPU init function in target-i386, then?
> 
> I would prefer to leave its name as it is (unless we are renaming all,
> which would probably be a waste of effort giving the next steps) and
> simply not use it in PC code. If you want to enforce this, you could
> #ifdef CONFIG_USER_ONLY it.
> 
> For some targets - as can be seen in your uc32 patch - there is already
> a cpu_generic_init() that calls into the CPUClass hooks of the given CPU
> type. I would like to call that from linux-user directly (or from a
> lightweight wrapper to be shared between linux-user and bsd-user, I
> assume we're going need some target-specific #ifdefs) and drop
> cpu_init() in its current form. In particular I want to somehow move the
> realized=true part out of it, which means either inlining it into dozens
> of machines or finishing the recursive realization work so that we only
> need one central realized=true for /machine.

Moving realized=true outside cpu_generic_init() would make lots of sense
for PC, as we already need to perform additional steps in the PC init
code before realizing the CPU (and I expect the list of PC-specific CPU
initialization steps to grow). And when we do this, cpu_x86_init()
(used by CONFIG_USER) and cpu_x86_create() (used by PC) can become the
same function.

But I don't see how to implement this without requiring changing machine
code case-by-case, as existing machine code may expect realized=true to
be set very early and break if you don't set it immediately after
cpu_*_init(). In other words, even if we implement recursive
realization, we may need to inline realize=true on all machines as an
intermediate (and automated) step, and then change machines to just rely
on recursive realization, one by one.

-- 
Eduardo

Re: [Qemu-devel] [PULL 04/11] target-i386: Rename cpu_x86_init() to cpu_x86_init_user()

[Andreas Färber]

Am 27.02.2015 um 20:01 schrieb Eduardo Habkost:
> On Fri, Feb 27, 2015 at 03:10:27PM +0100, Andreas Färber wrote:
>> Am 26.02.2015 um 16:59 schrieb Eduardo Habkost:
>>> On Wed, Feb 25, 2015 at 11:06:55PM +0100, Andreas Färber wrote:
>>>> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
>>>>> The function is used only for CONFIG_USER, so make its purpose clear.
>>>>>
>>>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>>>> ---
>>>>>  target-i386/cpu.c | 2 +-
>>>>>  target-i386/cpu.h | 4 ++--
>>>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> Please don't. I happily got all architectures aligned that it's at least
>>>> cpu_something_init, and it only happens to be user-only for x86. It is
>>>> rather the legacy function that was used in both system and user.
>>>
>>> If that's a legacy function, what are the steps you plan to follow to
>>> eliminate it? I would be glad to help eliminating legacy code.
>>>
>>> Initialization of CPUs in *-user and *-softmmu is different in i386, so
>>> we are going to have different code for both. How do you think I should
>>> name the *-user-specific CPU init function in target-i386, then?
>>
>> I would prefer to leave its name as it is (unless we are renaming all,
>> which would probably be a waste of effort giving the next steps) and
>> simply not use it in PC code. If you want to enforce this, you could
>> #ifdef CONFIG_USER_ONLY it.
>>
>> For some targets - as can be seen in your uc32 patch - there is already
>> a cpu_generic_init() that calls into the CPUClass hooks of the given CPU
>> type. I would like to call that from linux-user directly (or from a
>> lightweight wrapper to be shared between linux-user and bsd-user, I
>> assume we're going need some target-specific #ifdefs) and drop
>> cpu_init() in its current form. In particular I want to somehow move the
>> realized=true part out of it, which means either inlining it into dozens
>> of machines or finishing the recursive realization work so that we only
>> need one central realized=true for /machine.
> 
> Moving realized=true outside cpu_generic_init() would make lots of sense
> for PC, as we already need to perform additional steps in the PC init
> code before realizing the CPU (and I expect the list of PC-specific CPU
> initialization steps to grow). And when we do this, cpu_x86_init()
> (used by CONFIG_USER) and cpu_x86_create() (used by PC) can become the
> same function.
> 
> But I don't see how to implement this without requiring changing machine
> code case-by-case, as existing machine code may expect realized=true to
> be set very early and break if you don't set it immediately after
> cpu_*_init(). In other words, even if we implement recursive
> realization, we may need to inline realize=true on all machines as an
> intermediate (and automated) step, and then change machines to just rely
> on recursive realization, one by one.

Yes, that's why it's not done yet, it takes a lot of review and testing.
It is not going to be a pure Coccinelle patch. :) Doing that for the
inlining is an idea I had not yet considered, thanks.

On my qom-cpu-x86 branch I have already moved realized=true for the PC,
in order to make a CPU core object its parent.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 05/11] target-i386: Eliminate cpu_init() function

[Eduardo Habkost]

Instead of putting extra logic inside cpu.h, just do everything inside
cpu_x86_init_user().

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c |  6 +++---
 target-i386/cpu.h | 12 +++---------
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8f18556..aee4d3e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2135,7 +2135,7 @@ out:
     return cpu;
 }
 
-X86CPU *cpu_x86_init_user(const char *cpu_model)
+CPUX86State *cpu_x86_init_user(const char *cpu_model)
 {
     Error *error = NULL;
     X86CPU *cpu;
@@ -2153,10 +2153,10 @@ out:
         error_free(error);
         if (cpu != NULL) {
             object_unref(OBJECT(cpu));
-            cpu = NULL;
         }
+        return NULL;
     }
-    return cpu;
+    return &cpu->env;
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 41d7f0a..d5bd74e 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -982,7 +982,6 @@ typedef struct CPUX86State {
 
 #include "cpu-qom.h"
 
-X86CPU *cpu_x86_init_user(const char *cpu_model);
 X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
                        Error **errp);
 int cpu_x86_exec(CPUX86State *s);
@@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
 # define PHYS_ADDR_MASK 0xfffffffffLL
 # endif
 
-static inline CPUX86State *cpu_init(const char *cpu_model)
-{
-    X86CPU *cpu = cpu_x86_init_user(cpu_model);
-    if (cpu == NULL) {
-        return NULL;
-    }
-    return &cpu->env;
-}
+/* CPU creation function for *-user */
+CPUX86State *cpu_x86_init_user(const char *cpu_model);
+#define cpu_init cpu_x86_init_user
 
 #define cpu_exec cpu_x86_exec
 #define cpu_gen_code cpu_x86_gen_code
-- 
2.1.0

Re: [Qemu-devel] [PULL 05/11] target-i386: Eliminate cpu_init() function

[Andreas Färber]

Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> Instead of putting extra logic inside cpu.h, just do everything inside
> cpu_x86_init_user().
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c |  6 +++---
>  target-i386/cpu.h | 12 +++---------
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 8f18556..aee4d3e 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2135,7 +2135,7 @@ out:
>      return cpu;
>  }
>  
> -X86CPU *cpu_x86_init_user(const char *cpu_model)
> +CPUX86State *cpu_x86_init_user(const char *cpu_model)
>  {
>      Error *error = NULL;
>      X86CPU *cpu;
> @@ -2153,10 +2153,10 @@ out:
>          error_free(error);
>          if (cpu != NULL) {
>              object_unref(OBJECT(cpu));
> -            cpu = NULL;
>          }
> +        return NULL;
>      }
> -    return cpu;
> +    return &cpu->env;
>  }
>  
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 41d7f0a..d5bd74e 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -982,7 +982,6 @@ typedef struct CPUX86State {
>  
>  #include "cpu-qom.h"
>  
> -X86CPU *cpu_x86_init_user(const char *cpu_model);
>  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>                         Error **errp);
>  int cpu_x86_exec(CPUX86State *s);
> @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>  # define PHYS_ADDR_MASK 0xfffffffffLL
>  # endif
>  
> -static inline CPUX86State *cpu_init(const char *cpu_model)
> -{
> -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
> -    if (cpu == NULL) {
> -        return NULL;
> -    }
> -    return &cpu->env;
> -}
> +/* CPU creation function for *-user */
> +CPUX86State *cpu_x86_init_user(const char *cpu_model);
> +#define cpu_init cpu_x86_init_user

The very purpose of this lightweight glue code in cpu.h was to let us
use X86CPU in cpu.c. It seems that you are needlessly reverting my change?

Andreas

>  
>  #define cpu_exec cpu_x86_exec
>  #define cpu_gen_code cpu_x86_gen_code
> 


-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 05/11] target-i386: Eliminate cpu_init() function

[Eduardo Habkost]

On Wed, Feb 25, 2015 at 11:08:56PM +0100, Andreas Färber wrote:
> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> > Instead of putting extra logic inside cpu.h, just do everything inside
> > cpu_x86_init_user().
> > 
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c |  6 +++---
> >  target-i386/cpu.h | 12 +++---------
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 8f18556..aee4d3e 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2135,7 +2135,7 @@ out:
> >      return cpu;
> >  }
> >  
> > -X86CPU *cpu_x86_init_user(const char *cpu_model)
> > +CPUX86State *cpu_x86_init_user(const char *cpu_model)
> >  {
> >      Error *error = NULL;
> >      X86CPU *cpu;
> > @@ -2153,10 +2153,10 @@ out:
> >          error_free(error);
> >          if (cpu != NULL) {
> >              object_unref(OBJECT(cpu));
> > -            cpu = NULL;
> >          }
> > +        return NULL;
> >      }
> > -    return cpu;
> > +    return &cpu->env;
> >  }
> >  
> >  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 41d7f0a..d5bd74e 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -982,7 +982,6 @@ typedef struct CPUX86State {
> >  
> >  #include "cpu-qom.h"
> >  
> > -X86CPU *cpu_x86_init_user(const char *cpu_model);
> >  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
> >                         Error **errp);
> >  int cpu_x86_exec(CPUX86State *s);
> > @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
> >  # define PHYS_ADDR_MASK 0xfffffffffLL
> >  # endif
> >  
> > -static inline CPUX86State *cpu_init(const char *cpu_model)
> > -{
> > -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
> > -    if (cpu == NULL) {
> > -        return NULL;
> > -    }
> > -    return &cpu->env;
> > -}
> > +/* CPU creation function for *-user */
> > +CPUX86State *cpu_x86_init_user(const char *cpu_model);
> > +#define cpu_init cpu_x86_init_user
> 
> The very purpose of this lightweight glue code in cpu.h was to let us
> use X86CPU in cpu.c. It seems that you are needlessly reverting my change?

I am not sure I understand what you mean by "let us use X86CPU in
cpu.c". But I think the series can be redone without this change,
anyway.

In either case, what's your plan to eliminate this glue code from the
cpu.h files?

-- 
Eduardo

Re: [Qemu-devel] [PULL 05/11] target-i386: Eliminate cpu_init() function

[Andreas Färber]

Am 26.02.2015 um 17:03 schrieb Eduardo Habkost:
> On Wed, Feb 25, 2015 at 11:08:56PM +0100, Andreas Färber wrote:
>> Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
>>> Instead of putting extra logic inside cpu.h, just do everything inside
>>> cpu_x86_init_user().
>>>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target-i386/cpu.c |  6 +++---
>>>  target-i386/cpu.h | 12 +++---------
>>>  2 files changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 8f18556..aee4d3e 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -2135,7 +2135,7 @@ out:
>>>      return cpu;
>>>  }
>>>  
>>> -X86CPU *cpu_x86_init_user(const char *cpu_model)
>>> +CPUX86State *cpu_x86_init_user(const char *cpu_model)
>>>  {
>>>      Error *error = NULL;
>>>      X86CPU *cpu;
>>> @@ -2153,10 +2153,10 @@ out:
>>>          error_free(error);
>>>          if (cpu != NULL) {
>>>              object_unref(OBJECT(cpu));
>>> -            cpu = NULL;
>>>          }
>>> +        return NULL;
>>>      }
>>> -    return cpu;
>>> +    return &cpu->env;
>>>  }
>>>  
>>>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
>>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>>> index 41d7f0a..d5bd74e 100644
>>> --- a/target-i386/cpu.h
>>> +++ b/target-i386/cpu.h
>>> @@ -982,7 +982,6 @@ typedef struct CPUX86State {
>>>  
>>>  #include "cpu-qom.h"
>>>  
>>> -X86CPU *cpu_x86_init_user(const char *cpu_model);
>>>  X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge,
>>>                         Error **errp);
>>>  int cpu_x86_exec(CPUX86State *s);
>>> @@ -1171,14 +1170,9 @@ uint64_t cpu_get_tsc(CPUX86State *env);
>>>  # define PHYS_ADDR_MASK 0xfffffffffLL
>>>  # endif
>>>  
>>> -static inline CPUX86State *cpu_init(const char *cpu_model)
>>> -{
>>> -    X86CPU *cpu = cpu_x86_init_user(cpu_model);
>>> -    if (cpu == NULL) {
>>> -        return NULL;
>>> -    }
>>> -    return &cpu->env;
>>> -}
>>> +/* CPU creation function for *-user */
>>> +CPUX86State *cpu_x86_init_user(const char *cpu_model);
>>> +#define cpu_init cpu_x86_init_user
>>
>> The very purpose of this lightweight glue code in cpu.h was to let us
>> use X86CPU in cpu.c. It seems that you are needlessly reverting my change?
> 
> I am not sure I understand what you mean by "let us use X86CPU in
> cpu.c".

I mean, more and more code is supposed to use *CPU, not CPU*State. Like
your patch moving the APIC ID is adopting X86CPU, which is the right
direction. Replacing X86CPU with CPUX86State however feels wrong to me.

Since I changed cpu_x86_init() to return X86CPU, there will have been a
non-user caller at the time making use of X86CPU or CPUState. Just as in
some ARM use cases I assume that the current cpu_*_init() limitations
lead to its code getting inlined.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

[Qemu-devel] [PULL 06/11] target-i386: Simplify error handling on cpu_x86_init_user()

[Eduardo Habkost]

Isolate error handling path from the "if (error)" checks.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index aee4d3e..f6a7671 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2142,21 +2142,23 @@ CPUX86State *cpu_x86_init_user(const char *cpu_model)
 
     cpu = cpu_x86_create(cpu_model, NULL, &error);
     if (error) {
-        goto out;
+        goto error;
     }
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &error);
-
-out:
     if (error) {
-        error_report("%s", error_get_pretty(error));
-        error_free(error);
-        if (cpu != NULL) {
-            object_unref(OBJECT(cpu));
-        }
-        return NULL;
+        goto error;
     }
+
     return &cpu->env;
+
+error:
+    error_report("%s", error_get_pretty(error));
+    error_free(error);
+    if (cpu != NULL) {
+        object_unref(OBJECT(cpu));
+    }
+    return NULL;
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
-- 
2.1.0

[Qemu-devel] [PULL 07/11] target-i386: Move CPUX86State.cpuid_apic_id to X86CPU.apic_id

[Eduardo Habkost]

The field doesn't need to be inside CPUState, and it is not specific for
the CPUID instruction, so move and rename it.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h |  1 +
 target-i386/cpu.c     | 17 ++++++++---------
 target-i386/cpu.h     |  1 -
 target-i386/kvm.c     |  2 +-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index b557b61..4a6f48a 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,6 +93,7 @@ typedef struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
+    uint32_t apic_id;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f6a7671..e8cd7b3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1690,7 +1690,7 @@ static void x86_cpuid_get_apic_id(Object *obj, Visitor *v, void *opaque,
                                   const char *name, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
-    int64_t value = cpu->env.cpuid_apic_id;
+    int64_t value = cpu->apic_id;
 
     visit_type_int(v, &value, name, errp);
 }
@@ -1723,11 +1723,11 @@ static void x86_cpuid_set_apic_id(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    if ((value != cpu->env.cpuid_apic_id) && cpu_exists(value)) {
+    if ((value != cpu->apic_id) && cpu_exists(value)) {
         error_setg(errp, "CPU with APIC ID %" PRIi64 " exists", value);
         return;
     }
-    cpu->env.cpuid_apic_id = value;
+    cpu->apic_id = value;
 }
 
 /* Generic getter for "feature-words" and "filtered-features" properties */
@@ -2255,7 +2255,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 1:
         *eax = env->cpuid_version;
-        *ebx = (env->cpuid_apic_id << 24) | 8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
+        *ebx = (cpu->apic_id << 24) |
+               8 << 8; /* CLFLUSH size in quad words, Linux wants it. */
         *ecx = env->features[FEAT_1_ECX];
         *edx = env->features[FEAT_1_EDX];
         if (cs->nr_cores * cs->nr_threads > 1) {
@@ -2702,7 +2703,6 @@ static void mce_init(X86CPU *cpu)
 #ifndef CONFIG_USER_ONLY
 static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 {
-    CPUX86State *env = &cpu->env;
     DeviceState *dev = DEVICE(cpu);
     APICCommonState *apic;
     const char *apic_type = "apic";
@@ -2721,7 +2721,7 @@ static void x86_cpu_apic_create(X86CPU *cpu, Error **errp)
 
     object_property_add_child(OBJECT(cpu), "apic",
                               OBJECT(cpu->apic_state), NULL);
-    qdev_prop_set_uint8(cpu->apic_state, "id", env->cpuid_apic_id);
+    qdev_prop_set_uint8(cpu->apic_state, "id", cpu->apic_id);
     /* TODO: convert to link<> */
     apic = APIC_COMMON(cpu->apic_state);
     apic->cpu = cpu;
@@ -2904,7 +2904,7 @@ static void x86_cpu_initfn(Object *obj)
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-    env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
+    cpu->apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
@@ -2918,9 +2918,8 @@ static void x86_cpu_initfn(Object *obj)
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
-    CPUX86State *env = &cpu->env;
 
-    return env->cpuid_apic_id;
+    return cpu->apic_id;
 }
 
 static bool x86_cpu_get_paging_enabled(const CPUState *cs)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index d5bd74e..f0e6ca4 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -944,7 +944,6 @@ typedef struct CPUX86State {
     uint32_t cpuid_version;
     FeatureWordArray features;
     uint32_t cpuid_model[12];
-    uint32_t cpuid_apic_id;
 
     /* MTRRs */
     uint64_t mtrr_fixed[11];
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 40d6a14..27fe2be 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -430,7 +430,7 @@ static void cpu_update_state(void *opaque, int running, RunState state)
 unsigned long kvm_arch_vcpu_id(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
-    return cpu->env.cpuid_apic_id;
+    return cpu->apic_id;
 }
 
 #ifndef KVM_CPUID_SIGNATURE_NEXT
-- 
2.1.0

[Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

This was the only caller of cpu_init() that was not checking for NULL
yet.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 linux-user/main.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index d92702a..111c1ff 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3453,10 +3453,17 @@ CPUArchState *cpu_copy(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
     CPUArchState *new_env = cpu_init(cpu_model);
-    CPUState *new_cpu = ENV_GET_CPU(new_env);
+    CPUState *new_cpu;
     CPUBreakpoint *bp;
     CPUWatchpoint *wp;
 
+    if (!new_env) {
+        fprintf(stderr, "cpu_copy: Failed to create new CPU\n");
+        exit(1);
+    }
+
+    new_cpu = ENV_GET_CPU(new_env);
+
     /* Reset non arch specific state */
     cpu_reset(new_cpu);
 
-- 
2.1.0

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Andreas Färber]

Am 25.02.2015 um 20:58 schrieb Eduardo Habkost:
> This was the only caller of cpu_init() that was not checking for NULL
> yet.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  linux-user/main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index d92702a..111c1ff 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3453,10 +3453,17 @@ CPUArchState *cpu_copy(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUArchState *new_env = cpu_init(cpu_model);
> -    CPUState *new_cpu = ENV_GET_CPU(new_env);
> +    CPUState *new_cpu;
>      CPUBreakpoint *bp;
>      CPUWatchpoint *wp;
>  
> +    if (!new_env) {
> +        fprintf(stderr, "cpu_copy: Failed to create new CPU\n");
> +        exit(1);
> +    }
> +
> +    new_cpu = ENV_GET_CPU(new_env);
> +
>      /* Reset non arch specific state */
>      cpu_reset(new_cpu);
>  

This one seems a genuine bug fix independent of your remodeling.

I would've expected that to go through Riku's/Peter's or my qom-cpu
tree, with Cc: qemu-stable@nongnu.org.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Peter Maydell]

On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> This was the only caller of cpu_init() that was not checking for NULL
> yet.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  linux-user/main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index d92702a..111c1ff 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3453,10 +3453,17 @@ CPUArchState *cpu_copy(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      CPUArchState *new_env = cpu_init(cpu_model);
> -    CPUState *new_cpu = ENV_GET_CPU(new_env);
> +    CPUState *new_cpu;
>      CPUBreakpoint *bp;
>      CPUWatchpoint *wp;
>
> +    if (!new_env) {
> +        fprintf(stderr, "cpu_copy: Failed to create new CPU\n");
> +        exit(1);
> +    }

Rather than bailing out here, it would be better to
propagate the failure out to the caller, which can
then fail the fork syscall it's trying to emulate
by returning a suitable error code to the guest.

-- PMM

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

On Thu, Feb 26, 2015 at 08:27:12AM +0900, Peter Maydell wrote:
> On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > This was the only caller of cpu_init() that was not checking for NULL
> > yet.
> >
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  linux-user/main.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/linux-user/main.c b/linux-user/main.c
> > index d92702a..111c1ff 100644
> > --- a/linux-user/main.c
> > +++ b/linux-user/main.c
> > @@ -3453,10 +3453,17 @@ CPUArchState *cpu_copy(CPUArchState *env)
> >  {
> >      CPUState *cpu = ENV_GET_CPU(env);
> >      CPUArchState *new_env = cpu_init(cpu_model);
> > -    CPUState *new_cpu = ENV_GET_CPU(new_env);
> > +    CPUState *new_cpu;
> >      CPUBreakpoint *bp;
> >      CPUWatchpoint *wp;
> >
> > +    if (!new_env) {
> > +        fprintf(stderr, "cpu_copy: Failed to create new CPU\n");
> > +        exit(1);
> > +    }
> 
> Rather than bailing out here, it would be better to
> propagate the failure out to the caller, which can
> then fail the fork syscall it's trying to emulate
> by returning a suitable error code to the guest.

Makes sense to me, but I would prefer that to be done by somebody
faimilar with the *-user code as a follow-up. My intention was to simply
fix the bug so that QEMU won't crash.

-- 
Eduardo

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Peter Maydell]

On 27 February 2015 at 01:06, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Feb 26, 2015 at 08:27:12AM +0900, Peter Maydell wrote:
>> Rather than bailing out here, it would be better to
>> propagate the failure out to the caller, which can
>> then fail the fork syscall it's trying to emulate
>> by returning a suitable error code to the guest.
>
> Makes sense to me, but I would prefer that to be done by somebody
> faimilar with the *-user code as a follow-up. My intention was to simply
> fix the bug so that QEMU won't crash.

When can it actually fail? Have you seen it do this in practice?
(Note that this is only called from fork, so we know that
cpu_model must be valid, or we would have failed on startup
trying to create the CPU for the initial thread.)

-- PMM

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

On Fri, Feb 27, 2015 at 07:44:29AM +0900, Peter Maydell wrote:
> On 27 February 2015 at 01:06, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Feb 26, 2015 at 08:27:12AM +0900, Peter Maydell wrote:
> >> Rather than bailing out here, it would be better to
> >> propagate the failure out to the caller, which can
> >> then fail the fork syscall it's trying to emulate
> >> by returning a suitable error code to the guest.
> >
> > Makes sense to me, but I would prefer that to be done by somebody
> > faimilar with the *-user code as a follow-up. My intention was to simply
> > fix the bug so that QEMU won't crash.
> 
> When can it actually fail? Have you seen it do this in practice?
> (Note that this is only called from fork, so we know that
> cpu_model must be valid, or we would have failed on startup
> trying to create the CPU for the initial thread.)

I have never seen it in practice, but in x86 it will fail if we try to
create more than 254 VCPUs.

-- 
Eduardo

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Peter Maydell]

On 27 February 2015 at 07:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Feb 27, 2015 at 07:44:29AM +0900, Peter Maydell wrote:
>> When can it actually fail? Have you seen it do this in practice?
>> (Note that this is only called from fork, so we know that
>> cpu_model must be valid, or we would have failed on startup
>> trying to create the CPU for the initial thread.)
>
> I have never seen it in practice, but in x86 it will fail if we try to
> create more than 254 VCPUs.

That's bad -- it's not hard to imagine a program with 256
threads. Why does x86 have this limitation and can we fix it?
That seems like the better course than this patch.

-- PMM

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

On Fri, Feb 27, 2015 at 08:00:25AM +0900, Peter Maydell wrote:
> On 27 February 2015 at 07:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Fri, Feb 27, 2015 at 07:44:29AM +0900, Peter Maydell wrote:
> >> When can it actually fail? Have you seen it do this in practice?
> >> (Note that this is only called from fork, so we know that
> >> cpu_model must be valid, or we would have failed on startup
> >> trying to create the CPU for the initial thread.)
> >
> > I have never seen it in practice, but in x86 it will fail if we try to
> > create more than 254 VCPUs.
> 
> That's bad -- it's not hard to imagine a program with 256
> threads. Why does x86 have this limitation and can we fix it?
> That seems like the better course than this patch.

I don't think *-user or any x86-specific code has any hard requirement
of having all VCPUs with different APIC IDs. But right now we set APIC
ID = cpu_index for every VCPU, so the current code has this limitation.

I would be glad to send a patch adding an assert() instead of an error
message, but I don't think I will be able to audit every single
cpu_init() function soon (to ensure they really never fail).

So, while we don't audit all cpu_init() functions, do you prefer to live
with an error message, or an assert() that may or may not be triggered
by existing code?

-- 
Eduardo

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Peter Maydell]

On 27 February 2015 at 08:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Feb 27, 2015 at 08:00:25AM +0900, Peter Maydell wrote:
>> On 27 February 2015 at 07:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > I have never seen it in practice, but in x86 it will fail if we try to
>> > create more than 254 VCPUs.
>>
>> That's bad -- it's not hard to imagine a program with 256
>> threads. Why does x86 have this limitation and can we fix it?
>> That seems like the better course than this patch.
>
> I don't think *-user or any x86-specific code has any hard requirement
> of having all VCPUs with different APIC IDs. But right now we set APIC
> ID = cpu_index for every VCPU, so the current code has this limitation.

But the -user code doesn't have an APIC at all, so it shouldn't
have to live with this limitation.

> I would be glad to send a patch adding an assert() instead of an error
> message, but I don't think I will be able to audit every single
> cpu_init() function soon (to ensure they really never fail).
>
> So, while we don't audit all cpu_init() functions, do you prefer to live
> with an error message, or an assert() that may or may not be triggered
> by existing code?

I would prefer:
 (1) that we fix the error handling properly so you return an error
     indication from cpu_copy() and we propagate it back to the guest
 (2) that we fix x86-64 so it can create more than 254 CPUs for usermode

I would rather not paper over this by adding a "let qemu assert or
fatally exit on a recoverable error condition" code path we'd just
have to revert when we fixed (1) properly.

(2) is not really important to deal with right now, since we don't
actually support multiple threads yet. But it would be nice eventually.

-- PMM

Re: [Qemu-devel] [PULL 08/11] linux-user: Check for cpu_init() errors

[Eduardo Habkost]

On Fri, Feb 27, 2015 at 08:23:12AM +0900, Peter Maydell wrote:
> On 27 February 2015 at 08:11, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Fri, Feb 27, 2015 at 08:00:25AM +0900, Peter Maydell wrote:
> >> On 27 February 2015 at 07:51, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> > I have never seen it in practice, but in x86 it will fail if we try to
> >> > create more than 254 VCPUs.
> >>
> >> That's bad -- it's not hard to imagine a program with 256
> >> threads. Why does x86 have this limitation and can we fix it?
> >> That seems like the better course than this patch.
> >
> > I don't think *-user or any x86-specific code has any hard requirement
> > of having all VCPUs with different APIC IDs. But right now we set APIC
> > ID = cpu_index for every VCPU, so the current code has this limitation.
> 
> But the -user code doesn't have an APIC at all, so it shouldn't
> have to live with this limitation.

Yes. I don't know yet if we can safely set apic_id=0 on all usermode
VCPUs (maybe some existing code will break if we do that), but we should
be able to do it eventually.

> 
> > I would be glad to send a patch adding an assert() instead of an error
> > message, but I don't think I will be able to audit every single
> > cpu_init() function soon (to ensure they really never fail).
> >
> > So, while we don't audit all cpu_init() functions, do you prefer to live
> > with an error message, or an assert() that may or may not be triggered
> > by existing code?
> 
> I would prefer:
>  (1) that we fix the error handling properly so you return an error
>      indication from cpu_copy() and we propagate it back to the guest
>  (2) that we fix x86-64 so it can create more than 254 CPUs for usermode
> 
> I would rather not paper over this by adding a "let qemu assert or
> fatally exit on a recoverable error condition" code path we'd just
> have to revert when we fixed (1) properly.
> 
> (2) is not really important to deal with right now, since we don't
> actually support multiple threads yet. But it would be nice eventually.

So, I have taken a look at the cpu_init() implementations, and all of
them seem to never fail unless an invalid CPU model string is used,
except for i386.

So it looks like there's no compelling reason to write extra code for
(1) right now if we fix (2). On the other hand, we will always have the
possibility of somebody introducing additional cpu_init() error
conditions in some architecture in the future (that's why I suggested
adding an assert()).

Personally, I don't mind either way. I will just leave this for the
developers interested in linux-user.

-- 
Eduardo

[Qemu-devel] [PULL 09/11] target-i386: Set APIC ID using cpu_index on CONFIG_USER

[Eduardo Habkost]

The PC CPU initialization code already sets apic-id based on the CPU
topology, and CONFIG_USER doesn't need the topology-based APIC ID
calculation code.

Make CONFIG_USER set apic-id before realizing the CPU (just like PC
already does), so we can simplify x86_cpu_initfn later. As there is no
CPU topology configuration in CONFIG_USER, just use cpu_index as the
APIC ID.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e8cd7b3..3e0c39c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2145,6 +2145,12 @@ CPUX86State *cpu_x86_init_user(const char *cpu_model)
         goto error;
     }
 
+    object_property_set_int(OBJECT(cpu), CPU(cpu)->cpu_index, "apic-id",
+                            &error);
+    if (error) {
+        goto error;
+    }
+
     object_property_set_bool(OBJECT(cpu), true, "realized", &error);
     if (error) {
         goto error;
-- 
2.1.0

[Qemu-devel] [PULL 10/11] target-i386: Require APIC ID to be explicitly set before CPU realize

[Eduardo Habkost]

Instead of setting APIC ID automatically when creating a X86CPU, require
the property to be set before realizing the object (which all callers of
cpu_x86_create() already do).

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu-qom.h | 2 +-
 target-i386/cpu.c     | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
index 4a6f48a..31a0c1e 100644
--- a/target-i386/cpu-qom.h
+++ b/target-i386/cpu-qom.h
@@ -93,7 +93,7 @@ typedef struct X86CPU {
     bool expose_kvm;
     bool migratable;
     bool host_features;
-    uint32_t apic_id;
+    int64_t apic_id;
 
     /* if true the CPUID code directly forward host cache leaves to the guest */
     bool cache_info_passthrough;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3e0c39c..54422f3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2767,6 +2767,11 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
     static bool ht_warned;
 
+    if (cpu->apic_id < 0) {
+        error_setg(errp, "apic-id property was not initialized properly");
+        return;
+    }
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }
@@ -2910,7 +2915,7 @@ static void x86_cpu_initfn(Object *obj)
                         NULL, NULL, (void *)cpu->filtered_features, NULL);
 
     cpu->hyperv_spinlock_attempts = HYPERV_SPINLOCK_NEVER_RETRY;
-    cpu->apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
+    cpu->apic_id = -1;
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
-- 
2.1.0

[Qemu-devel] [PULL 11/11] target-i386: Move APIC ID compatibility code to pc.c

[Eduardo Habkost]

The APIC ID compatibility code is required only for PC, and now that
x86_cpu_initfn() doesn't use x86_cpu_apic_id_from_index() anymore, that
code can be moved to pc.c.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc.c      | 35 +++++++++++++++++++++++++++++++++++
 target-i386/cpu.c | 34 ----------------------------------
 2 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c7af6aa..2519297 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -25,6 +25,8 @@
 #include "hw/i386/pc.h"
 #include "hw/char/serial.h"
 #include "hw/i386/apic.h"
+#include "hw/i386/topology.h"
+#include "sysemu/cpus.h"
 #include "hw/block/fdc.h"
 #include "hw/ide.h"
 #include "hw/pci/pci.h"
@@ -628,6 +630,39 @@ bool e820_get_entry(int idx, uint32_t type, uint64_t *address, uint64_t *length)
     return false;
 }
 
+/* Enables contiguous-apic-ID mode, for compatibility */
+static bool compat_apic_id_mode;
+
+void enable_compat_apic_id_mode(void)
+{
+    compat_apic_id_mode = true;
+}
+
+/* Calculates initial APIC ID for a specific CPU index
+ *
+ * Currently we need to be able to calculate the APIC ID from the CPU index
+ * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
+ * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
+ * all CPUs up to max_cpus.
+ */
+uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
+{
+    uint32_t correct_id;
+    static bool warned;
+
+    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
+    if (compat_apic_id_mode) {
+        if (cpu_index != correct_id && !warned) {
+            error_report("APIC IDs set in compatibility mode, "
+                         "CPU topology won't match the configuration");
+            warned = true;
+        }
+        return cpu_index;
+    } else {
+        return correct_id;
+    }
+}
+
 /* Calculates the limit to CPU APIC ID values
  *
  * This function returns the limit for the APIC ID value, so that all
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 54422f3..70fd6db 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -25,7 +25,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/cpus.h"
 #include "kvm_i386.h"
-#include "hw/i386/topology.h"
 
 #include "qemu/option.h"
 #include "qemu/config-file.h"
@@ -2836,39 +2835,6 @@ out:
     }
 }
 
-/* Enables contiguous-apic-ID mode, for compatibility */
-static bool compat_apic_id_mode;
-
-void enable_compat_apic_id_mode(void)
-{
-    compat_apic_id_mode = true;
-}
-
-/* Calculates initial APIC ID for a specific CPU index
- *
- * Currently we need to be able to calculate the APIC ID from the CPU index
- * alone (without requiring a CPU object), as the QEMU<->Seabios interfaces have
- * no concept of "CPU index", and the NUMA tables on fw_cfg need the APIC ID of
- * all CPUs up to max_cpus.
- */
-uint32_t x86_cpu_apic_id_from_index(unsigned int cpu_index)
-{
-    uint32_t correct_id;
-    static bool warned;
-
-    correct_id = x86_apicid_from_cpu_idx(smp_cores, smp_threads, cpu_index);
-    if (compat_apic_id_mode) {
-        if (cpu_index != correct_id && !warned) {
-            error_report("APIC IDs set in compatibility mode, "
-                         "CPU topology won't match the configuration");
-            warned = true;
-        }
-        return cpu_index;
-    } else {
-        return correct_id;
-    }
-}
-
 static void x86_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
-- 
2.1.0

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Peter Maydell]

On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The following changes since commit 3d30395f7fb3315e4ecf0de4e48790e1326bbd47:
>
>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-20150218-1' into staging (2015-02-25 11:54:15 +0000)
>
> are available in the git repository at:
>
>   https://github.com/ehabkost/qemu.git tags/x86-pull-request
>
> for you to fetch changes up to de13197a38cf45c990802661a057f64a05426cbc:
>
>   target-i386: Move APIC ID compatibility code to pc.c (2015-02-25 15:00:07 -0300)
>
> ----------------------------------------------------------------
> Those patches were reviewed some time ago, and Paolo suggested I submit them
> through my own tree. So, here is my first x86 pull request. :)
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Andreas Färber]

Am 02.03.2015 um 16:19 schrieb Peter Maydell:
> On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> The following changes since commit 3d30395f7fb3315e4ecf0de4e48790e1326bbd47:
>>
>>   Merge remote-tracking branch 'remotes/kraxel/tags/pull-usb-20150218-1' into staging (2015-02-25 11:54:15 +0000)
>>
>> are available in the git repository at:
>>
>>   https://github.com/ehabkost/qemu.git tags/x86-pull-request
>>
>> for you to fetch changes up to de13197a38cf45c990802661a057f64a05426cbc:
>>
>>   target-i386: Move APIC ID compatibility code to pc.c (2015-02-25 15:00:07 -0300)
>>
>> ----------------------------------------------------------------
>> Those patches were reviewed some time ago, and Paolo suggested I submit them
>> through my own tree. So, here is my first x86 pull request. :)
>> ----------------------------------------------------------------
> 
> Applied, thanks.

Why? You yourself had objections against 08/11, no? And replacement
series are already on the list.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Peter Maydell]

On 3 March 2015 at 00:26, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.03.2015 um 16:19 schrieb Peter Maydell:
>> On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> ----------------------------------------------------------------
>>> Those patches were reviewed some time ago, and Paolo suggested I submit them
>>> through my own tree. So, here is my first x86 pull request. :)
>>> ----------------------------------------------------------------
>>
>> Applied, thanks.
>
> Why? You yourself had objections against 08/11, no? And replacement
> series are already on the list.

Because nobody followed up to this cover letter to say "don't apply this".
I process pullreqs in first-in-first-out order and I rely on
submitters (or others) letting me know if there's a reason not to
apply something, and on people not submitting pullreqs including
patches which have got negative review on list :-(

-- PMM

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Andreas Färber]

Am 02.03.2015 um 16:30 schrieb Peter Maydell:
> On 3 March 2015 at 00:26, Andreas Färber <afaerber@suse.de> wrote:
>> Am 02.03.2015 um 16:19 schrieb Peter Maydell:
>>> On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>> ----------------------------------------------------------------
>>>> Those patches were reviewed some time ago, and Paolo suggested I submit them
>>>> through my own tree. So, here is my first x86 pull request. :)
>>>> ----------------------------------------------------------------
>>>
>>> Applied, thanks.
>>
>> Why? You yourself had objections against 08/11, no? And replacement
>> series are already on the list.
> 
> Because nobody followed up to this cover letter to say "don't apply this".

That's pretty much what I replied to 04/11, and I expected you to see
that, in particular since you were on CC and chimed in. :/

I had some of Eduardo's alternative patches queued already and will look
into fixing this mess...

> I process pullreqs in first-in-first-out order and I rely on
> submitters (or others) letting me know if there's a reason not to
> apply something, and on people not submitting pullreqs including
> patches which have got negative review on list :-(

In this case it was Eduardo's first pull request, with overlap between
qom-cpu and target-i386 responsibilities and Paolo having given an Rb
for a full APIC movement series rather than the individual patches I
pointed out. That requires a bit more review.

Eduardo, I also notice that your tag luckily does not match the above
description in your cover letter. That section is supposed to be filled
in by git-request-pull from the tag, not hand-edited, and should be a
summary of what changes the pull includes, not who reviewed it. You can
place any additional comments above the generated template.

Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Peter Maydell]

On 3 March 2015 at 01:18, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.03.2015 um 16:30 schrieb Peter Maydell:
>> Because nobody followed up to this cover letter to say "don't apply this".
>
> That's pretty much what I replied to 04/11, and I expected you to see
> that, in particular since you were on CC and chimed in. :/

I do mean literally "to the cover letter" there, since gmail doesn't
thread emails :-( Since I'm just doing this pullreq processing in
odd moments of free time at the moment I'm less likely to remember
bits of context like that.

> I had some of Eduardo's alternative patches queued already and will look
> into fixing this mess...

I can just revert the whole set if you like, since I haven't applied
anything else on top yet.

>> I process pullreqs in first-in-first-out order and I rely on
>> submitters (or others) letting me know if there's a reason not to
>> apply something, and on people not submitting pullreqs including
>> patches which have got negative review on list :-(
>
> In this case it was Eduardo's first pull request, with overlap between
> qom-cpu and target-i386 responsibilities and Paolo having given an Rb
> for a full APIC movement series rather than the individual patches I
> pointed out. That requires a bit more review.

Yes, you're right in retrospect.

-- PMM

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Andreas Färber]

Am 02.03.2015 um 17:22 schrieb Peter Maydell:
> On 3 March 2015 at 01:18, Andreas Färber <afaerber@suse.de> wrote:
>> I had some of Eduardo's alternative patches queued already and will look
>> into fixing this mess...
> 
> I can just revert the whole set if you like, since I haven't applied
> anything else on top yet.

On second thoughts after trying to revert individual bits, please do
revert the full pull if still possible.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Peter Maydell]

On 3 March 2015 at 02:10, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.03.2015 um 17:22 schrieb Peter Maydell:
>> On 3 March 2015 at 01:18, Andreas Färber <afaerber@suse.de> wrote:
>>> I had some of Eduardo's alternative patches queued already and will look
>>> into fixing this mess...
>>
>> I can just revert the whole set if you like, since I haven't applied
>> anything else on top yet.
>
> On second thoughts after trying to revert individual bits, please do
> revert the full pull if still possible.

I've now done this.

IMPORTANT NOTE: if resubmitting a pull request which includes some
of these patches, you need to make sure that it's been rebased
on the revert-commit (0856579) or later, or otherwise ensure that
all the commits in the pullreq are different (different commit hashes)
from those in the reverted pullreq. Otherwise the merge will not
apply them because in git's view of history those commit hashes are
already present in master. (For a fuller explanation, see
https://www.kernel.org/pub/software/scm/git/docs/howto/revert-a-faulty-merge.txt)

-- PMM

Re: [Qemu-devel] [PULL 00/11] X86 patches

[Eduardo Habkost]

On Mon, Mar 02, 2015 at 05:18:01PM +0100, Andreas Färber wrote:
> Am 02.03.2015 um 16:30 schrieb Peter Maydell:
> > On 3 March 2015 at 00:26, Andreas Färber <afaerber@suse.de> wrote:
> >> Am 02.03.2015 um 16:19 schrieb Peter Maydell:
> >>> On 26 February 2015 at 04:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>> ----------------------------------------------------------------
> >>>> Those patches were reviewed some time ago, and Paolo suggested I submit them
> >>>> through my own tree. So, here is my first x86 pull request. :)
> >>>> ----------------------------------------------------------------
> >>>
> >>> Applied, thanks.
> >>
> >> Why? You yourself had objections against 08/11, no? And replacement
> >> series are already on the list.
> > 
> > Because nobody followed up to this cover letter to say "don't apply this".
> 
> That's pretty much what I replied to 04/11, and I expected you to see
> that, in particular since you were on CC and chimed in. :/
> 
> I had some of Eduardo's alternative patches queued already and will look
> into fixing this mess...

I assumed the pull request were already going to be ignored considering
all the replies. I didn't know an additional "please don't apply this"
request was necessary, sorry. :(

> 
> > I process pullreqs in first-in-first-out order and I rely on
> > submitters (or others) letting me know if there's a reason not to
> > apply something, and on people not submitting pullreqs including
> > patches which have got negative review on list :-(
> 
> In this case it was Eduardo's first pull request, with overlap between
> qom-cpu and target-i386 responsibilities and Paolo having given an Rb
> for a full APIC movement series rather than the individual patches I
> pointed out. That requires a bit more review.
> 
> Eduardo, I also notice that your tag luckily does not match the above
> description in your cover letter. That section is supposed to be filled
> in by git-request-pull from the tag, not hand-edited, and should be a
> summary of what changes the pull includes, not who reviewed it. You can
> place any additional comments above the generated template.

Yeah, I edited that text in the e-mail message only, not to the tag
description. It looks like I chose the wrong spot in the e-mail message
to add my notes and I made it look like it was the tag description.
Sorry again.

-- 
Eduardo