[Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Alexey Kardashevskiy]

Normally CPUState::cpu_index is used to pick the right CPU for various
operations. However default consecutive numbering does not always work
for POWERPC.

For example, on POWER7 (which supports 4 threads per core),
"-smp 8,threads=4" should create CPUs with indexes 0,1,2,3,4,5,6,7 and
"-smp 8,threads=1" should create CPUs with indexes 0,4,8,12,16,20,24,28.

These indexes are reflected in /proc/device-tree/cpus/PowerPC,POWER7@XX
and used to call KVM VCPU's ioctls. In order to achieve this,
kvmppc_fixup_cpu() was introduced. Roughly speaking, it multiplies
cpu_index by the number of threads per core.

This approach has disadvantages such as:
1. NUMA configuration stays broken after the fixup;
2. CPU-related commands from QEMU Monitor do not work properly as
the accept fixed CPU indexes and the user does not really know
what they are after fixup as the number of threads per core changes
between CPU versions and via QEMU command line.

This introduces a @cpu_dt_id field in the CPUPPCState struct which
is set from @cpu_index by default but can be fixed later to conform
the device tree requirements.
1. int ppc_get_vcpu_dt_id(CPUState *cs) - returns a device-tree ID
for a CPU;
2. CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id) - finds CPUState by
a device-tree CPU ID.
3. void ppc_fixup_cpu_dt_id(PowerPCCPU *cpu) - calculates correct
cpu_dt_id; this replaces kvmppc_fixup_cpu() as the numbers are not
just about KVM but about the device tree too.

This uses the new functions to:
1. fix emulated XICS hypercall handlers as they receive fixed CPU indexes;
2. fix XICS-KVM to enable in-kernel XICS on right CPU;
3. compose correct device-tree.

This removes @cpu_index fixup as @cpu_dt_id is used instead so QEMU monitor
can accept command-line CPU indexes again.

Cc: Paul Mackerras <paulus@samba.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Bharat Bhushan <bharat.bhushan@freescale.com>
Cc: Scott Wood <scottwood@freescale.com>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

It does not feel that we really need CPUState::kvm_cpu_id and
direct calling of kvm_arch_vcpu_id() would be enough.

Please comment. Thanks.

---
Changes:
v4:
* now there are cpu_index, cpu_dt_id and kvm_cpu_id
* kvmppc_fixup_cpu() was replaced with ppc_fixup_cpu_dt_id()

v3:
* althouth e500 does not tweak CPU indexes, e500 and openpic-kvm
are fixed too; hopefully it does not break anything.

v2:
* added PPC-specific ppc_get_vcpu_dt_id() and ppc_get_vcpu_by_dt_id()
* fixed kvm_arch_vcpu_id() to use ppc_get_vcpu_dt_id()
* fixed emulated XICS
* removed kvm_arch_vcpu_id() stub for non-KVM case
---
 hw/intc/openpic_kvm.c       |  2 +-
 hw/intc/xics.c              | 15 +++++++++++++--
 hw/intc/xics_kvm.c          |  8 ++++----
 hw/ppc/e500.c               |  5 +++--
 hw/ppc/ppc.c                | 37 +++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr.c              |  9 +++++----
 hw/ppc/spapr_hcall.c        |  2 +-
 hw/ppc/spapr_rtas.c         |  4 ++--
 include/qom/cpu.h           |  1 +
 kvm-all.c                   |  1 +
 target-ppc/cpu.h            |  7 +++++++
 target-ppc/kvm.c            | 10 +---------
 target-ppc/translate_init.c |  4 ++++
 13 files changed, 80 insertions(+), 25 deletions(-)

diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index c7f7b84..75d5897 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -228,7 +228,7 @@ int kvm_openpic_connect_vcpu(DeviceState *d, CPUState *cs)
 
     encap.cap = KVM_CAP_IRQ_MPIC;
     encap.args[0] = opp->fd;
-    encap.args[1] = cs->cpu_index;
+    encap.args[1] = cs->kvm_cpu_id;
 
     return kvm_vcpu_ioctl(cs, KVM_ENABLE_CAP, &encap);
 }
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index a333305..866ee08 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -33,6 +33,17 @@
 #include "qemu/error-report.h"
 #include "qapi/visitor.h"
 
+static int get_cpu_index_by_dt_id(int cpu_dt_id)
+{
+    CPUState *cs = ppc_get_vcpu_by_dt_id(cpu_dt_id);
+
+    if (cs) {
+        return cs->cpu_index;
+    }
+
+    return -1;
+}
+
 void xics_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
 {
     CPUState *cs = CPU(cpu);
@@ -659,7 +670,7 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPREnvironment *spapr,
 static target_ulong h_ipi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                           target_ulong opcode, target_ulong *args)
 {
-    target_ulong server = args[0];
+    target_ulong server = get_cpu_index_by_dt_id(args[0]);
     target_ulong mfrr = args[1];
 
     if (server >= spapr->icp->nr_servers) {
@@ -728,7 +739,7 @@ static void rtas_set_xive(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     }
 
     nr = rtas_ld(args, 0);
-    server = rtas_ld(args, 1);
+    server = get_cpu_index_by_dt_id(rtas_ld(args, 1));
     priority = rtas_ld(args, 2);
 
     if (!ics_valid_irq(ics, nr) || (server >= ics->icp->nr_servers)
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index c203646..e3556ba 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -65,7 +65,7 @@ static void icp_get_kvm_state(ICPState *ss)
     ret = kvm_vcpu_ioctl(ss->cs, KVM_GET_ONE_REG, &reg);
     if (ret != 0) {
         error_report("Unable to retrieve KVM interrupt controller state"
-                " for CPU %d: %s", ss->cs->cpu_index, strerror(errno));
+                " for CPU %d: %s", ss->cs->kvm_cpu_id, strerror(errno));
         exit(1);
     }
 
@@ -97,7 +97,7 @@ static int icp_set_kvm_state(ICPState *ss, int version_id)
     ret = kvm_vcpu_ioctl(ss->cs, KVM_SET_ONE_REG, &reg);
     if (ret != 0) {
         error_report("Unable to restore KVM interrupt controller state (0x%"
-                PRIx64 ") for CPU %d: %s", state, ss->cs->cpu_index,
+                PRIx64 ") for CPU %d: %s", state, ss->cs->kvm_cpu_id,
                 strerror(errno));
         return ret;
     }
@@ -325,7 +325,7 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
         struct kvm_enable_cap xics_enable_cap = {
             .cap = KVM_CAP_IRQ_XICS,
             .flags = 0,
-            .args = {icpkvm->kernel_xics_fd, cs->cpu_index, 0, 0},
+            .args = {icpkvm->kernel_xics_fd, cs->kvm_cpu_id, 0, 0},
         };
 
         ss->cs = cs;
@@ -333,7 +333,7 @@ static void xics_kvm_cpu_setup(XICSState *icp, PowerPCCPU *cpu)
         ret = kvm_vcpu_ioctl(ss->cs, KVM_ENABLE_CAP, &xics_enable_cap);
         if (ret < 0) {
             error_report("Unable to connect CPU%d to kernel XICS: %s",
-                    cs->cpu_index, strerror(errno));
+                    cs->kvm_cpu_id, strerror(errno));
             exit(1);
         }
     }
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index cfdd84b..212d9d7 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -248,12 +248,13 @@ static int ppce500_load_device_tree(QEMUMachineInitArgs *args,
         env = cpu->env_ptr;
 
         snprintf(cpu_name, sizeof(cpu_name), "/cpus/PowerPC,8544@%x",
-                 cpu->cpu_index);
+                 ppc_get_vcpu_dt_id(cpu));
         qemu_devtree_add_subnode(fdt, cpu_name);
         qemu_devtree_setprop_cell(fdt, cpu_name, "clock-frequency", clock_freq);
         qemu_devtree_setprop_cell(fdt, cpu_name, "timebase-frequency", tb_freq);
         qemu_devtree_setprop_string(fdt, cpu_name, "device_type", "cpu");
-        qemu_devtree_setprop_cell(fdt, cpu_name, "reg", cpu->cpu_index);
+        qemu_devtree_setprop_cell(fdt, cpu_name, "reg",
+                                  ppc_get_vcpu_dt_id(cpu));
         qemu_devtree_setprop_cell(fdt, cpu_name, "d-cache-line-size",
                                   env->dcache_line_size);
         qemu_devtree_setprop_cell(fdt, cpu_name, "i-cache-line-size",
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index bf2d3d4..df51a66 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -26,6 +26,7 @@
 #include "hw/ppc/ppc_e500.h"
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/cpus.h"
 #include "hw/timer/m48t59.h"
 #include "qemu/log.h"
 #include "hw/loader.h"
@@ -1362,3 +1363,39 @@ int PPC_NVRAM_set_params (nvram_t *nvram, uint16_t NVRAM_size,
 
     return 0;
 }
+
+/* CPU device-tree ID helpers */
+int ppc_get_vcpu_dt_id(CPUState *cs)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(cs);
+    CPUPPCState *env = &cpu->env;
+
+    return env->cpu_dt_id;
+}
+
+CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        CPUPPCState *env = &cpu->env;
+
+        if (env->cpu_dt_id == cpu_dt_id) {
+            return cs;
+        }
+    }
+
+    return NULL;
+}
+
+void ppc_fixup_cpu_dt_id(PowerPCCPU *cpu)
+{
+    CPUState *cs = CPU(cpu);
+    int smt;
+
+    /* Adjust cpu index for SMT */
+    smt = kvmppc_smt_threads();
+    cpu->env.cpu_dt_id = (cs->cpu_index / smp_threads) * smt
+        + (cs->cpu_index % smp_threads);
+}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 036246c..1bbe68b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -206,19 +206,20 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPREnvironment *spapr)
 
     CPU_FOREACH(cpu) {
         DeviceClass *dc = DEVICE_GET_CLASS(cpu);
+        int cpu_dt_id = ppc_get_vcpu_dt_id(cpu);
         uint32_t associativity[] = {cpu_to_be32(0x5),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(0x0),
                                     cpu_to_be32(cpu->numa_node),
-                                    cpu_to_be32(cpu->cpu_index)};
+                                    cpu_to_be32(cpu_dt_id)};
 
-        if ((cpu->cpu_index % smt) != 0) {
+        if ((cpu_dt_id % smt) != 0) {
             continue;
         }
 
         snprintf(cpu_model, 32, "/cpus/%s@%x", dc->fw_name,
-                 cpu->cpu_index);
+                 cpu_dt_id);
 
         offset = fdt_path_offset(fdt, cpu_model);
         if (offset < 0) {
@@ -367,7 +368,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
         CPUPPCState *env = &cpu->env;
         DeviceClass *dc = DEVICE_GET_CLASS(cs);
         PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
-        int index = cs->cpu_index;
+        int index = ppc_get_vcpu_dt_id(cs);
         uint32_t servers_prop[smp_threads];
         uint32_t gservers_prop[smp_threads * 2];
         char *nodename;
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f755a53..1acb0d0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -466,7 +466,7 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     CPUPPCState *tenv;
     CPUState *tcpu;
 
-    tcpu = qemu_get_cpu(procno);
+    tcpu = ppc_get_vcpu_by_dt_id(procno);
     if (!tcpu) {
         return H_PARAMETER;
     }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index eb542f2..f56dfca 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -139,7 +139,7 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU *cpu_,
     }
 
     id = rtas_ld(args, 0);
-    cpu = qemu_get_cpu(id);
+    cpu = ppc_get_vcpu_by_dt_id(id);
     if (cpu != NULL) {
         if (cpu->halted) {
             rtas_st(rets, 1, 0);
@@ -172,7 +172,7 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPREnvironment *spapr,
     start = rtas_ld(args, 1);
     r3 = rtas_ld(args, 2);
 
-    cs = qemu_get_cpu(id);
+    cs = ppc_get_vcpu_by_dt_id(id);
     if (cs != NULL) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
         CPUPPCState *env = &cpu->env;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7739e00..52fc76d 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -197,6 +197,7 @@ struct CPUState {
     bool kvm_vcpu_dirty;
     struct KVMState *kvm_state;
     struct kvm_run *kvm_run;
+    int kvm_cpu_id;
 
     /* TODO Move common fields from CPUArchState here. */
     int cpu_index; /* used by alpha TCG */
diff --git a/kvm-all.c b/kvm-all.c
index 4478969..0612690 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -243,6 +243,7 @@ int kvm_init_vcpu(CPUState *cpu)
     cpu->kvm_fd = ret;
     cpu->kvm_state = s;
     cpu->kvm_vcpu_dirty = true;
+    cpu->kvm_cpu_id = kvm_arch_vcpu_id(cpu);
 
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index bb84767..8052f6b 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1074,6 +1074,9 @@ struct CPUPPCState {
      */
     uint8_t fit_period[4];
     uint8_t wdt_period[4];
+
+    /* The CPU index used in the device tree. KVM uses this index too */
+    int cpu_dt_id;
 };
 
 #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
@@ -2154,4 +2157,8 @@ static inline bool cpu_has_work(CPUState *cpu)
 
 void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env);
 
+int ppc_get_vcpu_dt_id(CPUState *cs);
+CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
+void ppc_fixup_cpu_dt_id(PowerPCCPU *cpu);
+
 #endif /* !defined (__CPU_PPC_H__) */
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 80c0386..5d3da44 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -407,7 +407,7 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
 
 unsigned long kvm_arch_vcpu_id(CPUState *cpu)
 {
-    return cpu->cpu_index;
+    return ppc_get_vcpu_dt_id(cpu);
 }
 
 int kvm_arch_init_vcpu(CPUState *cs)
@@ -1774,14 +1774,6 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 
 int kvmppc_fixup_cpu(PowerPCCPU *cpu)
 {
-    CPUState *cs = CPU(cpu);
-    int smt;
-
-    /* Adjust cpu index for SMT */
-    smt = kvmppc_smt_threads();
-    cs->cpu_index = (cs->cpu_index / smp_threads) * smt
-        + (cs->cpu_index % smp_threads);
-
     return 0;
 }
 
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 35d1389..688696d 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7940,6 +7940,8 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     }
 #endif
 
+    ppc_fixup_cpu_dt_id(cpu);
+
     if (kvm_enabled()) {
         if (kvmppc_fixup_cpu(cpu) != 0) {
             error_setg(errp, "Unable to virtualize selected CPU with KVM");
@@ -8558,8 +8560,10 @@ static void ppc_cpu_initfn(Object *obj)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
+    cs->kvm_cpu_id = -1;
     cs->env_ptr = env;
     cpu_exec_init(env);
+    env->cpu_dt_id = cs->cpu_index;
 
     env->msr_mask = pcc->msr_mask;
     env->mmu_model = pcc->mmu_model;
-- 
1.8.4.rc4

Re: [Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Paolo Bonzini]

Il 15/11/2013 06:14, Alexey Kardashevskiy ha scritto:
> 
> It does not feel that we really need CPUState::kvm_cpu_id and
> direct calling of kvm_arch_vcpu_id() would be enough.

Indeed -- and it should be kvm_ppc_vcpu_id() since other architectures
do not need it.

Paolo

Re: [Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Alexey Kardashevskiy]

On 11/15/2013 09:40 PM, Paolo Bonzini wrote:
> Il 15/11/2013 06:14, Alexey Kardashevskiy ha scritto:
>>
>> It does not feel that we really need CPUState::kvm_cpu_id and
>> direct calling of kvm_arch_vcpu_id() would be enough.
> 
> Indeed -- and it should be kvm_ppc_vcpu_id() since other architectures
> do not need it.

And ignore kvm_arch_vcpu_id() for spapr/ppc?


-- 
Alexey

Re: [Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Paolo Bonzini]

Il 18/11/2013 04:02, Alexey Kardashevskiy ha scritto:
> On 11/15/2013 09:40 PM, Paolo Bonzini wrote:
>> Il 15/11/2013 06:14, Alexey Kardashevskiy ha scritto:
>>>
>>> It does not feel that we really need CPUState::kvm_cpu_id and
>>> direct calling of kvm_arch_vcpu_id() would be enough.
>>
>> Indeed -- and it should be kvm_ppc_vcpu_id() since other architectures
>> do not need it.
> 
> And ignore kvm_arch_vcpu_id() for spapr/ppc?

Sorry, brain fart - i meant ppc_get_vcpu_dt_id but it has already been
objected to earlier.  So the patch is ok for me, modulo removal of
kvm_cpu_id which you already proposed above.

Paolo

Re: [Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Andreas Färber]

Am 15.11.2013 06:14, schrieb Alexey Kardashevskiy:
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7739e00..52fc76d 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -197,6 +197,7 @@ struct CPUState {
>      bool kvm_vcpu_dirty;
>      struct KVMState *kvm_state;
>      struct kvm_run *kvm_run;
> +    int kvm_cpu_id;
>  
>      /* TODO Move common fields from CPUArchState here. */
>      int cpu_index; /* used by alpha TCG */

Here you are adding a field to CPUState, fine with me. (Please add a
documentation line above the struct then.)

> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index bb84767..8052f6b 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1074,6 +1074,9 @@ struct CPUPPCState {
>       */
>      uint8_t fit_period[4];
>      uint8_t wdt_period[4];
> +
> +    /* The CPU index used in the device tree. KVM uses this index too */
> +    int cpu_dt_id;

But I believe I have requested a number of times not to add random
fields to CPUPPCState unless they are accessed by TCG. Please place the
new field in PowerPCCPU instead and put the description into the struct
documentation.

>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
> @@ -2154,4 +2157,8 @@ static inline bool cpu_has_work(CPUState *cpu)
>  
>  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUPPCState *env);
>  
> +int ppc_get_vcpu_dt_id(CPUState *cs);
> +CPUState *ppc_get_vcpu_by_dt_id(int cpu_dt_id);
> +void ppc_fixup_cpu_dt_id(PowerPCCPU *cpu);
> +
>  #endif /* !defined (__CPU_PPC_H__) */

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 v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Alexey Kardashevskiy]

On 16.11.2013 2:11, Andreas Färber wrote:
> Am 15.11.2013 06:14, schrieb Alexey Kardashevskiy:
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 7739e00..52fc76d 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -197,6 +197,7 @@ struct CPUState {
>>      bool kvm_vcpu_dirty;
>>      struct KVMState *kvm_state;
>>      struct kvm_run *kvm_run;
>> +    int kvm_cpu_id;
>>  
>>      /* TODO Move common fields from CPUArchState here. */
>>      int cpu_index; /* used by alpha TCG */
> 
> Here you are adding a field to CPUState, fine with me. (Please add a
> documentation line above the struct then.)
> 
>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>> index bb84767..8052f6b 100644
>> --- a/target-ppc/cpu.h
>> +++ b/target-ppc/cpu.h
>> @@ -1074,6 +1074,9 @@ struct CPUPPCState {
>>       */
>>      uint8_t fit_period[4];
>>      uint8_t wdt_period[4];
>> +
>> +    /* The CPU index used in the device tree. KVM uses this index too */
>> +    int cpu_dt_id;
> 
> But I believe I have requested a number of times not to add random
> fields to CPUPPCState unless they are accessed by TCG. Please place the
> new field in PowerPCCPU instead and put the description into the struct
> documentation.


It is accessed by xics.c which is used in TCG. Or I misinterpret the
rule, do not I?




-- 
With best regards

Alexey Kardashevskiy -- icq: 52150396

Re: [Qemu-devel] [PATCH v4] ppc: introduce CPUPPCState::cpu_dt_id and CPUState::kvm_cpu_id

[Andreas Färber]

Am 15.11.2013 18:02, schrieb Alexey Kardashevskiy:
> On 16.11.2013 2:11, Andreas Färber wrote:
>> Am 15.11.2013 06:14, schrieb Alexey Kardashevskiy:
>>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>>> index 7739e00..52fc76d 100644
>>> --- a/include/qom/cpu.h
>>> +++ b/include/qom/cpu.h
>>> @@ -197,6 +197,7 @@ struct CPUState {
>>>      bool kvm_vcpu_dirty;
>>>      struct KVMState *kvm_state;
>>>      struct kvm_run *kvm_run;
>>> +    int kvm_cpu_id;
>>>  
>>>      /* TODO Move common fields from CPUArchState here. */
>>>      int cpu_index; /* used by alpha TCG */
>>
>> Here you are adding a field to CPUState, fine with me. (Please add a
>> documentation line above the struct then.)
>>
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index bb84767..8052f6b 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -1074,6 +1074,9 @@ struct CPUPPCState {
>>>       */
>>>      uint8_t fit_period[4];
>>>      uint8_t wdt_period[4];
>>> +
>>> +    /* The CPU index used in the device tree. KVM uses this index too */
>>> +    int cpu_dt_id;
>>
>> But I believe I have requested a number of times not to add random
>> fields to CPUPPCState unless they are accessed by TCG. Please place the
>> new field in PowerPCCPU instead and put the description into the struct
>> documentation.
> 
> 
> It is accessed by xics.c which is used in TCG. Or I misinterpret the
> rule, do not I?

Yes, you do. ;)

The question is, is the field accessed using offsetof(CPUPPCState, x)
such as in target-ppc/translate.c (search for TCG_AREG0). That's what
CPUPPCState is still needed for - mainly GPRs and the TLBs that TCG ops
load from / store to using host instructions with immediate offset.

Whether it is used in a HELPER() / helper_* function called from TCG
(target-ppc/*helper.c) has become less relevant since the optimization
of ppc_env_get_cpu() macro.

Neither of that seem to be the case here, unless I missed something.

I still hope we can one day get rid of CPUPPCState by having only
numeric-offset accesses from cpu+sizeof(CPUState) and all fields
(including TLB, as Paolo recently pointed out) directly in PowerPCCPU.

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