| Message ID | 011801d0947a$a9e1e8a0$fda5b9e0$@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hi, I see that you added mpidr to ARMCpu and initialized it in virt.c then you use it in mpidr_read. Thus you must fix all other virtual machines in hw/arm not just virt.c as it is not initialized for them. I have no other comments. Best regards, S.P. > -----Original Message----- > From: Pavel Fedin [mailto:p.fedin@samsung.com] > Sent: Friday, 22 May, 2015 1:33 PM > To: qemu-devel@nongnu.org > Cc: peter.maydell@linaro.org; 'Ashok Kumar'; Shlomo Pongratz > Subject: [PATCH] Use Aff1 with mpidr > > This is an improved and KVM-aware alternative to > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00942.html. > Changes are: > 1. MPIDR value (minus feature bits) is now directly stored in CPU instance. > 2. Default assignment is based on original rule (8 CPUs per cluster). > 3. If KVM is used, MPIDR values are overridden with those from KVM. This is > necessary for proper KVM PSCI functioning. > 4. Some cosmetic changes which would make further expansion of this code > easier. > 5. Added some debugging macros because CPU ID assignment is tricky part, > and if there are some problems, i think there should be a quick way to make > sure they are correct. > This does not have an RFC mark because it is perfectly okay to be committed > alone, it does not break anything. Commit message follows. Cc'ed to all > interested parties because i really hope to get things going somewhere this > time. > > In order to support up to 128 cores with GIC-500 (GICv3 implementation) > affinity1 must be used. GIC-500 support up to 32 clusters with up to > 8 cores in a cluster. So for example, if one wishes to have 16 cores, the > options are: 2 clusters of 8 cores each, 4 clusters with 4 cores each Currently > only the first option is supported for TCG. However, KVM expects to have > the same clusters layout as host machine has. Therefore, if we use 64-bit > KVM we override CPU IDs with those provided by the KVM. For 32-bit > systems it is OK to leave the default because so far we do not have more > than 8 CPUs on any of them. > This implementation has a potential to be extended with some methods > which would define cluster layout instead of hardcoded CPUS_PER_CLUSTER > definition. This would allow to emulate real machines with different layouts. > However, in case of KVM we would still have to inherit IDs from the host. > > Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com> > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > --- > hw/arm/virt.c | 6 +++++- > target-arm/cpu-qom.h | 3 +++ > target-arm/cpu.c | 17 +++++++++++++++++ > target-arm/helper.c | 9 +++------ > target-arm/kvm64.c | 25 +++++++++++++++++++++++++ > target-arm/psci.c | 20 ++++++++++++++++++-- > 6 files changed, 71 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo > *vbi) > "enable-method", "psci"); > } > > - qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu); > + /* > + * If cpus node's #address-cells property is set to 1 > + * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1. > + */ > + qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", > + armcpu->mpidr); > g_free(nodename); > } > } > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index > ed5a644..a382a09 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -159,6 +159,7 @@ typedef struct ARMCPU { > uint64_t id_aa64mmfr1; > uint32_t dbgdidr; > uint32_t clidr; > + uint64_t mpidr; /* Without feature bits */ > /* The elements of this array are the CCSIDR values for each cache, > * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc. > */ > @@ -171,6 +172,8 @@ typedef struct ARMCPU { > uint64_t rvbar; > } ARMCPU; > > +#define CPUS_PER_CLUSTER 8 > + > #define TYPE_AARCH64_CPU "aarch64-cpu" > #define AARCH64_CPU_CLASS(klass) \ > OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU) > diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 3ca3fa8..7dc2595 > 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -31,6 +31,12 @@ > #include "sysemu/kvm.h" > #include "kvm_arm.h" > > +#ifdef DEBUG > +# define DPRINTF(format, ...) printf("armcpu: " format , ## > +__VA_ARGS__) #else # define DPRINTF(format, ...) do { } while (0) > +#endif > + > static void arm_cpu_set_pc(CPUState *cs, vaddr value) { > ARMCPU *cpu = ARM_CPU(cs); > @@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs = CPU(obj); > ARMCPU *cpu = ARM_CPU(obj); > static bool inited; > + uint32_t Aff1, Aff0; > > cs->env_ptr = &cpu->env; > cpu_exec_init(&cpu->env); > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > g_free, g_free); > > + /* > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > + * in later ARM ARM versions), or any of the higher affinity level fields, > + * so these bits always RAZ. > + */ > + Aff1 = cs->cpu_index / CPUS_PER_CLUSTER; > + Aff0 = cs->cpu_index % CPUS_PER_CLUSTER; > + cpu->mpidr = (Aff1 << 8) | Aff0; > + DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index, > + cpu->mpidr); > + > #ifndef CONFIG_USER_ONLY > /* Our inbound IRQ and FIQ lines */ > if (kvm_enabled()) { > diff --git a/target-arm/helper.c b/target-arm/helper.c index > 5d0f011..9535290 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2036,12 +2036,9 @@ static const ARMCPRegInfo > strongarm_cp_reginfo[] = { > > static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri) { > - CPUState *cs = CPU(arm_env_get_cpu(env)); > - uint32_t mpidr = cs->cpu_index; > - /* We don't support setting cluster ID ([8..11]) (known as Aff1 > - * in later ARM ARM versions), or any of the higher affinity level fields, > - * so these bits always RAZ. > - */ > + ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env)); > + uint64_t mpidr = cpu->mpidr; > + > if (arm_feature(env, ARM_FEATURE_V7MP)) { > mpidr |= (1U << 31); > /* Cores which are uniprocessor (non-coherent) diff --git a/target- > arm/kvm64.c b/target-arm/kvm64.c index 93c1ca8..99fa34e 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -25,6 +25,12 @@ > #include "internals.h" > #include "hw/arm/arm.h" > > +#ifdef DEBUG > +# define DPRINTF(format, ...) printf("arm-kvm64: " format , ## > +__VA_ARGS__) #else # define DPRINTF(format, ...) do { } while (0) > +#endif > + > static inline void set_feature(uint64_t *features, int feature) { > *features |= 1ULL << feature; > @@ -77,6 +83,10 @@ bool > kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc) > return true; > } > > +#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL > +#define ARM_CPU_ID 3, 0, 0, 0 > +#define ARM_CPU_ID_MPIDR 5 > + > int kvm_arch_init_vcpu(CPUState *cs) > { > int ret; > @@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs) > return ret; > } > > + /* > + * When KVM is in use, psci is emulated in-kernel and not by qemu. > + * In order for it to work correctly we should use correct MPIDR values, > + * which appear to be inherited from the host. > + * So we override our defaults by asking KVM about these values. > + */ > + ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID, > ARM_CPU_ID_MPIDR), > + &cpu->mpidr); > + if (ret) { > + return ret; > + } > + cpu->mpidr &= ARM_MPIDR_HWID_BITMASK; > + DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu, > + cs->cpu_index, cpu->mpidr); > + > return kvm_arm_init_cpreg_list(cpu); } > > diff --git a/target-arm/psci.c b/target-arm/psci.c index d8fafab..2905be6 > 100644 > --- a/target-arm/psci.c > +++ b/target-arm/psci.c > @@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) > } > } > > +static uint32_t mpidr_to_idx(uint64_t mpidr) { > + uint32_t Aff1, Aff0; > + > + /* > + * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0] > + * GIC 500 code currently supports 32 clusters with 8 cores each > + * but no more than 128 cores. Future version will have flexible > + * affinity selection > + * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too > + */ > + Aff1 = (mpidr & 0xff00) >> 8; > + Aff0 = mpidr & 0xff; > + return Aff1 * CPUS_PER_CLUSTER + Aff0; } > + > void arm_handle_psci_call(ARMCPU *cpu) > { > /* > @@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu) > > switch (param[2]) { > case 0: > - target_cpu_state = qemu_get_cpu(mpidr & 0xff); > + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); > if (!target_cpu_state) { > ret = QEMU_PSCI_RET_INVALID_PARAMS; > break; > @@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu) > context_id = param[3]; > > /* change to the cpu we are powering up */ > - target_cpu_state = qemu_get_cpu(mpidr & 0xff); > + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); > if (!target_cpu_state) { > ret = QEMU_PSCI_RET_INVALID_PARAMS; > break; > -- > 1.9.5.msysgit.0 >
On Fri, 22 May 2015 13:32:54 +0300 Pavel Fedin <p.fedin@samsung.com> wrote: > This is an improved and KVM-aware alternative to > https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg00942.html. Changes are: > 1. MPIDR value (minus feature bits) is now directly stored in CPU instance. > 2. Default assignment is based on original rule (8 CPUs per cluster). > 3. If KVM is used, MPIDR values are overridden with those from KVM. This is necessary for > proper KVM PSCI functioning. > 4. Some cosmetic changes which would make further expansion of this code easier. > 5. Added some debugging macros because CPU ID assignment is tricky part, and if there are > some problems, i think there should be a quick way to make sure they are correct. > This does not have an RFC mark because it is perfectly okay to be committed alone, it > does not break anything. Commit message follows. Cc'ed to all interested parties because i > really hope to get things going somewhere this time. > > In order to support up to 128 cores with GIC-500 (GICv3 implementation) > affinity1 must be used. GIC-500 support up to 32 clusters with up to > 8 cores in a cluster. So for example, if one wishes to have 16 cores, > the options are: 2 clusters of 8 cores each, 4 clusters with 4 cores each > Currently only the first option is supported for TCG. However, KVM expects > to have the same clusters layout as host machine has. Therefore, if we use > 64-bit KVM we override CPU IDs with those provided by the KVM. For 32-bit > systems it is OK to leave the default because so far we do not have more > than 8 CPUs on any of them. > This implementation has a potential to be extended with some methods which > would define cluster layout instead of hardcoded CPUS_PER_CLUSTER > definition. This would allow to emulate real machines with different > layouts. However, in case of KVM we would still have to inherit IDs from > the host. > > Signed-off-by: Shlomo Pongratz <shlomo.pongratz@huawei.com> > Signed-off-by: Pavel Fedin <p.fedin@samsung.com> > --- > hw/arm/virt.c | 6 +++++- > target-arm/cpu-qom.h | 3 +++ > target-arm/cpu.c | 17 +++++++++++++++++ > target-arm/helper.c | 9 +++------ > target-arm/kvm64.c | 25 +++++++++++++++++++++++++ > target-arm/psci.c | 20 ++++++++++++++++++-- > 6 files changed, 71 insertions(+), 9 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a7f9a10..a1186c5 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) > "enable-method", "psci"); > } > > - qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu); > + /* > + * If cpus node's #address-cells property is set to 1 > + * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1. > + */ > + qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", armcpu->mpidr); > g_free(nodename); > } > } > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > index ed5a644..a382a09 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -159,6 +159,7 @@ typedef struct ARMCPU { > uint64_t id_aa64mmfr1; > uint32_t dbgdidr; > uint32_t clidr; > + uint64_t mpidr; /* Without feature bits */ > /* The elements of this array are the CCSIDR values for each cache, > * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc. > */ > @@ -171,6 +172,8 @@ typedef struct ARMCPU { > uint64_t rvbar; > } ARMCPU; > > +#define CPUS_PER_CLUSTER 8 > + > #define TYPE_AARCH64_CPU "aarch64-cpu" > #define AARCH64_CPU_CLASS(klass) \ > OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU) > diff --git a/target-arm/cpu.c b/target-arm/cpu.c > index 3ca3fa8..7dc2595 100644 > --- a/target-arm/cpu.c > +++ b/target-arm/cpu.c > @@ -31,6 +31,12 @@ > #include "sysemu/kvm.h" > #include "kvm_arm.h" > > +#ifdef DEBUG > +# define DPRINTF(format, ...) printf("armcpu: " format , ## __VA_ARGS__) > +#else > +# define DPRINTF(format, ...) do { } while (0) > +#endif > + > static void arm_cpu_set_pc(CPUState *cs, vaddr value) > { > ARMCPU *cpu = ARM_CPU(cs); > @@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj) > CPUState *cs = CPU(obj); > ARMCPU *cpu = ARM_CPU(obj); > static bool inited; > + uint32_t Aff1, Aff0; > > cs->env_ptr = &cpu->env; > cpu_exec_init(&cpu->env); > cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, > g_free, g_free); > > + /* > + * We don't support setting cluster ID ([16..23]) (known as Aff2 > + * in later ARM ARM versions), or any of the higher affinity level fields, > + * so these bits always RAZ. > + */ > + Aff1 = cs->cpu_index / CPUS_PER_CLUSTER; > + Aff0 = cs->cpu_index % CPUS_PER_CLUSTER; > + cpu->mpidr = (Aff1 << 8) | Aff0; > + DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index, cpu->mpidr); > + > #ifndef CONFIG_USER_ONLY > /* Our inbound IRQ and FIQ lines */ > if (kvm_enabled()) { > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 5d0f011..9535290 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -2036,12 +2036,9 @@ static const ARMCPRegInfo strongarm_cp_reginfo[] = { > > static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri) > { > - CPUState *cs = CPU(arm_env_get_cpu(env)); > - uint32_t mpidr = cs->cpu_index; > - /* We don't support setting cluster ID ([8..11]) (known as Aff1 > - * in later ARM ARM versions), or any of the higher affinity level fields, > - * so these bits always RAZ. > - */ > + ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env)); > + uint64_t mpidr = cpu->mpidr; > + > if (arm_feature(env, ARM_FEATURE_V7MP)) { > mpidr |= (1U << 31); > /* Cores which are uniprocessor (non-coherent) > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c > index 93c1ca8..99fa34e 100644 > --- a/target-arm/kvm64.c > +++ b/target-arm/kvm64.c > @@ -25,6 +25,12 @@ > #include "internals.h" > #include "hw/arm/arm.h" > > +#ifdef DEBUG > +# define DPRINTF(format, ...) printf("arm-kvm64: " format , ## __VA_ARGS__) > +#else > +# define DPRINTF(format, ...) do { } while (0) > +#endif > + > static inline void set_feature(uint64_t *features, int feature) > { > *features |= 1ULL << feature; > @@ -77,6 +83,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc) > return true; > } > > +#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL > +#define ARM_CPU_ID 3, 0, 0, 0 > +#define ARM_CPU_ID_MPIDR 5 > + > int kvm_arch_init_vcpu(CPUState *cs) > { > int ret; > @@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs) > return ret; > } > > + /* > + * When KVM is in use, psci is emulated in-kernel and not by qemu. > + * In order for it to work correctly we should use correct MPIDR values, > + * which appear to be inherited from the host. why it must be inherited from host? Could you point out to KVM code that depends on it? > + * So we override our defaults by asking KVM about these values. > + */ > + ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR), > + &cpu->mpidr); > + if (ret) { > + return ret; > + } > + cpu->mpidr &= ARM_MPIDR_HWID_BITMASK; > + DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu, > + cs->cpu_index, cpu->mpidr); > + > return kvm_arm_init_cpreg_list(cpu); > } > > diff --git a/target-arm/psci.c b/target-arm/psci.c > index d8fafab..2905be6 100644 > --- a/target-arm/psci.c > +++ b/target-arm/psci.c > @@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) > } > } > > +static uint32_t mpidr_to_idx(uint64_t mpidr) > +{ > + uint32_t Aff1, Aff0; > + > + /* > + * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0] > + * GIC 500 code currently supports 32 clusters with 8 cores each > + * but no more than 128 cores. Future version will have flexible > + * affinity selection > + * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too > + */ > + Aff1 = (mpidr & 0xff00) >> 8; > + Aff0 = mpidr & 0xff; > + return Aff1 * CPUS_PER_CLUSTER + Aff0; > +} > + > void arm_handle_psci_call(ARMCPU *cpu) > { > /* > @@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu) > > switch (param[2]) { > case 0: > - target_cpu_state = qemu_get_cpu(mpidr & 0xff); > + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); > if (!target_cpu_state) { > ret = QEMU_PSCI_RET_INVALID_PARAMS; > break; > @@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu) > context_id = param[3]; > > /* change to the cpu we are powering up */ > - target_cpu_state = qemu_get_cpu(mpidr & 0xff); > + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); > if (!target_cpu_state) { > ret = QEMU_PSCI_RET_INVALID_PARAMS; > break;
Hello! > > + /* > > + * When KVM is in use, psci is emulated in-kernel and not by qemu. > > + * In order for it to work correctly we should use correct MPIDR values, > > + * which appear to be inherited from the host. > why it must be inherited from host? > Could you point out to KVM code that depends on it? It is PSCI, i have tested this and experienced this problem. In KVM guest PSCI is handled by "hvc" call and is done entirely in kernel, no qemu code is involved. If i supply wrong IDs the processors will simply not power up. I have checked how kvmtool (found here: git://linux-arm.org/linux-ap.git, tools/kvm) handles this. Related parts are: tools/kvm/arm/fdt.c - generate_cpu_nodes() - "reg" property assignment: --- cut --- static void generate_cpu_nodes(void *fdt, struct kvm *kvm) { int cpu; _FDT(fdt_begin_node(fdt, "cpus")); _FDT(fdt_property_cell(fdt, "#address-cells", 0x1)); _FDT(fdt_property_cell(fdt, "#size-cells", 0x0)); for (cpu = 0; cpu < kvm->nrcpus; ++cpu) { char cpu_name[CPU_NAME_MAX_LEN]; struct kvm_cpu *vcpu = kvm->cpus[cpu]; unsigned long mpidr = kvm_cpu__get_vcpu_mpidr(vcpu); mpidr &= ARM_MPIDR_HWID_BITMASK; snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%lx", mpidr); _FDT(fdt_begin_node(fdt, cpu_name)); _FDT(fdt_property_string(fdt, "device_type", "cpu")); _FDT(fdt_property_string(fdt, "compatible", vcpu->cpu_compatible)); if (kvm->nrcpus > 1) _FDT(fdt_property_string(fdt, "enable-method", "psci")); _FDT(fdt_property_cell(fdt, "reg", mpidr)); _FDT(fdt_end_node(fdt)); } _FDT(fdt_end_node(fdt)); } --- cut --- tools/kvm/arm/aarch64/kvm-cpu.c - kvm_cpu__get_vcpu_mpidr() - obtains CPU ID from the kernel: --- cut --- unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu) { struct kvm_one_reg reg; u64 mpidr; reg.id = ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR); reg.addr = (u64)&mpidr; if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, ®) < 0) die("KVM_GET_ONE_REG failed (get_mpidr vcpu%ld", vcpu->cpu_id); return mpidr; } --- cut --- So i just decided to do the same thing in KVM. However, to tell the truth, i actually do not know whether it is possible to do it in reverse and assign MPIDR to VCPUs using KVM_SET_ONE_REG ioctl instead. I can try it if you think it's more appropriate. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On Fri, 29 May 2015 11:53:38 +0300 Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > > > + /* > > > + * When KVM is in use, psci is emulated in-kernel and not by qemu. > > > + * In order for it to work correctly we should use correct MPIDR values, > > > + * which appear to be inherited from the host. > > why it must be inherited from host? > > Could you point out to KVM code that depends on it? > > It is PSCI, i have tested this and experienced this problem. In KVM guest PSCI is handled > by "hvc" call and is done entirely in kernel, no qemu code is involved. If i supply wrong > IDs the processors will simply not power up. > I have checked how kvmtool (found here: git://linux-arm.org/linux-ap.git, tools/kvm) > handles this. Related parts are: > tools/kvm/arm/fdt.c - generate_cpu_nodes() - "reg" property assignment: > --- cut --- > static void generate_cpu_nodes(void *fdt, struct kvm *kvm) > { > int cpu; > > _FDT(fdt_begin_node(fdt, "cpus")); > _FDT(fdt_property_cell(fdt, "#address-cells", 0x1)); > _FDT(fdt_property_cell(fdt, "#size-cells", 0x0)); > > for (cpu = 0; cpu < kvm->nrcpus; ++cpu) { > char cpu_name[CPU_NAME_MAX_LEN]; > struct kvm_cpu *vcpu = kvm->cpus[cpu]; > unsigned long mpidr = kvm_cpu__get_vcpu_mpidr(vcpu); > > mpidr &= ARM_MPIDR_HWID_BITMASK; > snprintf(cpu_name, CPU_NAME_MAX_LEN, "cpu@%lx", mpidr); > > _FDT(fdt_begin_node(fdt, cpu_name)); > _FDT(fdt_property_string(fdt, "device_type", "cpu")); > _FDT(fdt_property_string(fdt, "compatible", vcpu->cpu_compatible)); > > if (kvm->nrcpus > 1) > _FDT(fdt_property_string(fdt, "enable-method", "psci")); > > _FDT(fdt_property_cell(fdt, "reg", mpidr)); > _FDT(fdt_end_node(fdt)); > } > > _FDT(fdt_end_node(fdt)); > } > --- cut --- > tools/kvm/arm/aarch64/kvm-cpu.c - kvm_cpu__get_vcpu_mpidr() - obtains CPU ID from the > kernel: > --- cut --- > unsigned long kvm_cpu__get_vcpu_mpidr(struct kvm_cpu *vcpu) > { > struct kvm_one_reg reg; > u64 mpidr; > > reg.id = ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR); > reg.addr = (u64)&mpidr; > if (ioctl(vcpu->vcpu_fd, KVM_GET_ONE_REG, ®) < 0) > die("KVM_GET_ONE_REG failed (get_mpidr vcpu%ld", vcpu->cpu_id); > > return mpidr; > } > --- cut --- > > So i just decided to do the same thing in KVM. However, to tell the truth, i actually do > not know whether it is possible to do it in reverse and assign MPIDR to VCPUs using > KVM_SET_ONE_REG ioctl instead. I can try it if you think it's more appropriate. That's what I see as correct way to go and along the way teach KVM to not derive mpidr encoding from vcpuid and use QEMU provided mpidr value, that way mpidr would be consistent. As it currently stands QEMU notion of mpidr and KVM's will diverge onece CPU count goes above 16 CPUs. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > >
Hello! > That's what I see as correct way to go > and along the way teach KVM to not derive mpidr encoding from vcpuid > and use QEMU provided mpidr value, that way mpidr would be consistent. Yes, and this would make modelling of real hardware working much better. However, unfortunately, i have just tested it, and it does not work. MPIDR can be set for the vCPU, and i believe it will even give back the value when read. But PSCI completely ignores this setting. > As it currently stands QEMU notion of mpidr and KVM's will diverge > onece CPU count goes above 16 CPUs. Yes, but fortunately the code which cares about it (PSCI and Shlomo's GICv3 software emulation) is not used with KVM. So i think we have no other choice if we want to be compatible with current KVM APIs. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On Fri, 29 May 2015 15:26:57 +0300 Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > > That's what I see as correct way to go > > and along the way teach KVM to not derive mpidr encoding from vcpuid > > and use QEMU provided mpidr value, that way mpidr would be consistent. > > Yes, and this would make modelling of real hardware working much better. However, > unfortunately, i have just tested it, and it does not work. MPIDR can be set for the vCPU, > and i believe it will even give back the value when read. But PSCI completely ignores this > setting. > > > As it currently stands QEMU notion of mpidr and KVM's will diverge > > onece CPU count goes above 16 CPUs. > > Yes, but fortunately the code which cares about it (PSCI and Shlomo's GICv3 software > emulation) is not used with KVM. So i think we have no other choice if we want to be > compatible with current KVM APIs. Well KVM side should be fixed instead of driving us along wrong route. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > >
Hello!
> Well KVM side should be fixed instead of driving us along wrong route.
It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which
might not happen.
But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on
them?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
Hi!
> Well KVM side should be fixed instead of driving us along wrong route.
I have studied the question a bit more, and i discovered that MPIDR access on ARM is not
trapped by KVM. And guest would always get the same value as host would. Theoretically you
could modify the kernel so that PSCI accepts IDs set by qemu, but in this case we would
have inconsistency between device tree and real IDs. And there seems to be no way round.
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
On 2015/5/30 1:37, Pavel Fedin wrote: > Hi! > >> Well KVM side should be fixed instead of driving us along wrong route. > > I have studied the question a bit more, and i discovered that MPIDR access on ARM is not > trapped by KVM. And guest would always get the same value as host would. Theoretically you Yes, it doesn't trap but there is one register "vmpidr_el2" which is used for virtualization. When guest reads mpidr, it will get the value of vmpidr_el2. And when context switching, hyp will restore the value of vmpidr_el2 and the value is got from MPIDR_EL1 which is set by reset_mpidr(). hyp.s: .macro restore_sysregs // x2: base address for cpu context // x3: tmp register add x3, x2, #CPU_SYSREG_OFFSET(MPIDR_EL1) ----cut---- msr vmpidr_el2, x4 ----cut---- .endm
On Fri, 29 May 2015 16:41:01 +0300 Pavel Fedin <p.fedin@samsung.com> wrote: > Hello! > > > Well KVM side should be fixed instead of driving us along wrong route. > > It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which > might not happen. > But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on > them? I think arm target is not counted as stable yet, so we don't have to keep compatibility layer yet. > > Kind regards, > Pavel Fedin > Expert Engineer > Samsung Electronics Research center Russia > >
On 2 June 2015 at 16:32, Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 29 May 2015 16:41:01 +0300 > Pavel Fedin <p.fedin@samsung.com> wrote: > >> Hello! >> >> > Well KVM side should be fixed instead of driving us along wrong route. >> >> It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which >> might not happen. >> But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on >> them? > I think arm target is not counted as stable yet, so we don't have to keep > compatibility layer yet. We don't want to break running KVM on older kernels on ARM. It's OK if an older kernel means you don't get access to shiny new features (like GICv3 support, maybe), but QEMU binaries and configurations that used to work on those kernels should continue to work on those kernels. thanks -- PMM
On Tue, 2 Jun 2015 16:42:31 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 2 June 2015 at 16:32, Igor Mammedov <imammedo@redhat.com> wrote: > > On Fri, 29 May 2015 16:41:01 +0300 > > Pavel Fedin <p.fedin@samsung.com> wrote: > > > >> Hello! > >> > >> > Well KVM side should be fixed instead of driving us along wrong route. > >> > >> It can be fixed... Perhaps... If kernel developers acknowledge this is a problem, which > >> might not happen. > >> But still we will have older kernels, and what? Don't you want 64-bit ARM KVM work on > >> them? > > I think arm target is not counted as stable yet, so we don't have to keep > > compatibility layer yet. > > We don't want to break running KVM on older kernels on ARM. > It's OK if an older kernel means you don't get access to shiny > new features (like GICv3 support, maybe), but QEMU binaries > and configurations that used to work on those kernels should > continue to work on those kernels. perhaps we would need to start using machine types then like we do for x86 or something like this. > > thanks > -- PMM
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a7f9a10..a1186c5 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -317,7 +317,11 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi) "enable-method", "psci"); } - qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", cpu); + /* + * If cpus node's #address-cells property is set to 1 + * The reg cell bits [23:0] must be set to bits [23:0] of MPIDR_EL1. + */ + qemu_fdt_setprop_cell(vbi->fdt, nodename, "reg", armcpu->mpidr); g_free(nodename); } } diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index ed5a644..a382a09 100644 --- a/target-arm/cpu-qom.h +++ b/target-arm/cpu-qom.h @@ -159,6 +159,7 @@ typedef struct ARMCPU { uint64_t id_aa64mmfr1; uint32_t dbgdidr; uint32_t clidr; + uint64_t mpidr; /* Without feature bits */ /* The elements of this array are the CCSIDR values for each cache, * in the order L1DCache, L1ICache, L2DCache, L2ICache, etc. */ @@ -171,6 +172,8 @@ typedef struct ARMCPU { uint64_t rvbar; } ARMCPU; +#define CPUS_PER_CLUSTER 8 + #define TYPE_AARCH64_CPU "aarch64-cpu" #define AARCH64_CPU_CLASS(klass) \ OBJECT_CLASS_CHECK(AArch64CPUClass, (klass), TYPE_AARCH64_CPU) diff --git a/target-arm/cpu.c b/target-arm/cpu.c index 3ca3fa8..7dc2595 100644 --- a/target-arm/cpu.c +++ b/target-arm/cpu.c @@ -31,6 +31,12 @@ #include "sysemu/kvm.h" #include "kvm_arm.h" +#ifdef DEBUG +# define DPRINTF(format, ...) printf("armcpu: " format , ## __VA_ARGS__) +#else +# define DPRINTF(format, ...) do { } while (0) +#endif + static void arm_cpu_set_pc(CPUState *cs, vaddr value) { ARMCPU *cpu = ARM_CPU(cs); @@ -367,12 +373,23 @@ static void arm_cpu_initfn(Object *obj) CPUState *cs = CPU(obj); ARMCPU *cpu = ARM_CPU(obj); static bool inited; + uint32_t Aff1, Aff0; cs->env_ptr = &cpu->env; cpu_exec_init(&cpu->env); cpu->cp_regs = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); + /* + * We don't support setting cluster ID ([16..23]) (known as Aff2 + * in later ARM ARM versions), or any of the higher affinity level fields, + * so these bits always RAZ. + */ + Aff1 = cs->cpu_index / CPUS_PER_CLUSTER; + Aff0 = cs->cpu_index % CPUS_PER_CLUSTER; + cpu->mpidr = (Aff1 << 8) | Aff0; + DPRINTF("Init(%p): index %d mpidr 0x%jX\n", cpu, cs->cpu_index, cpu->mpidr); + #ifndef CONFIG_USER_ONLY /* Our inbound IRQ and FIQ lines */ if (kvm_enabled()) { diff --git a/target-arm/helper.c b/target-arm/helper.c index 5d0f011..9535290 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -2036,12 +2036,9 @@ static const ARMCPRegInfo strongarm_cp_reginfo[] = { static uint64_t mpidr_read(CPUARMState *env, const ARMCPRegInfo *ri) { - CPUState *cs = CPU(arm_env_get_cpu(env)); - uint32_t mpidr = cs->cpu_index; - /* We don't support setting cluster ID ([8..11]) (known as Aff1 - * in later ARM ARM versions), or any of the higher affinity level fields, - * so these bits always RAZ. - */ + ARMCPU *cpu = ARM_CPU(arm_env_get_cpu(env)); + uint64_t mpidr = cpu->mpidr; + if (arm_feature(env, ARM_FEATURE_V7MP)) { mpidr |= (1U << 31); /* Cores which are uniprocessor (non-coherent) diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c index 93c1ca8..99fa34e 100644 --- a/target-arm/kvm64.c +++ b/target-arm/kvm64.c @@ -25,6 +25,12 @@ #include "internals.h" #include "hw/arm/arm.h" +#ifdef DEBUG +# define DPRINTF(format, ...) printf("arm-kvm64: " format , ## __VA_ARGS__) +#else +# define DPRINTF(format, ...) do { } while (0) +#endif + static inline void set_feature(uint64_t *features, int feature) { *features |= 1ULL << feature; @@ -77,6 +83,10 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc) return true; } +#define ARM_MPIDR_HWID_BITMASK 0xFF00FFFFFFULL +#define ARM_CPU_ID 3, 0, 0, 0 +#define ARM_CPU_ID_MPIDR 5 + int kvm_arch_init_vcpu(CPUState *cs) { int ret; @@ -107,6 +117,21 @@ int kvm_arch_init_vcpu(CPUState *cs) return ret; } + /* + * When KVM is in use, psci is emulated in-kernel and not by qemu. + * In order for it to work correctly we should use correct MPIDR values, + * which appear to be inherited from the host. + * So we override our defaults by asking KVM about these values. + */ + ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID, ARM_CPU_ID_MPIDR), + &cpu->mpidr); + if (ret) { + return ret; + } + cpu->mpidr &= ARM_MPIDR_HWID_BITMASK; + DPRINTF("Init(%p): index %d MPIDR 0x%jX\n", cpu, + cs->cpu_index, cpu->mpidr); + return kvm_arm_init_cpreg_list(cpu); } diff --git a/target-arm/psci.c b/target-arm/psci.c index d8fafab..2905be6 100644 --- a/target-arm/psci.c +++ b/target-arm/psci.c @@ -72,6 +72,22 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) } } +static uint32_t mpidr_to_idx(uint64_t mpidr) +{ + uint32_t Aff1, Aff0; + + /* + * MPIDR_EL1 [RES0:affinity-3:RES1:U:RES0:MT:affinity-1:affinity-0] + * GIC 500 code currently supports 32 clusters with 8 cores each + * but no more than 128 cores. Future version will have flexible + * affinity selection + * GIC 400 supports 8 cores so 0x7 for Aff0 is O.K. too + */ + Aff1 = (mpidr & 0xff00) >> 8; + Aff0 = mpidr & 0xff; + return Aff1 * CPUS_PER_CLUSTER + Aff0; +} + void arm_handle_psci_call(ARMCPU *cpu) { /* @@ -121,7 +137,7 @@ void arm_handle_psci_call(ARMCPU *cpu) switch (param[2]) { case 0: - target_cpu_state = qemu_get_cpu(mpidr & 0xff); + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); if (!target_cpu_state) { ret = QEMU_PSCI_RET_INVALID_PARAMS; break; @@ -153,7 +169,7 @@ void arm_handle_psci_call(ARMCPU *cpu) context_id = param[3]; /* change to the cpu we are powering up */ - target_cpu_state = qemu_get_cpu(mpidr & 0xff); + target_cpu_state = qemu_get_cpu(mpidr_to_idx(mpidr)); if (!target_cpu_state) { ret = QEMU_PSCI_RET_INVALID_PARAMS; break;