[RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Ying Fang]

An accurate cpu topology may help improve the cpu scheduler's decision
making when dealing with multi-core system. So cpu topology description
is helpful to provide guest with the right view. Cpu cache information may
also have slight impact on the sched domain, and even userspace software
may check the cpu cache information to do some optimizations. Thus this patch
series is posted to provide cpu and cache topology support for arm.

To make the cpu topology consistent with MPIDR, an vcpu ioctl
KVM_ARM_SET_MP_AFFINITY is introduced so that userspace can set MPIDR
according to the topology specified [1]. To describe the cpu topology
both fdt and ACPI are supported. To describe the cpu cache information,
a default cache hierarchy is given and can be made configurable later.
The cpu topology is built according to processor hierarchy node structure.
The cpu cache information is built according to cache type structure.

This patch series is partially based on the patches posted by Andrew Jone
years ago [2], I jumped in on it since some OS vendor cooperative partners
are eager for it. Thanks for Andrew's contribution. Please feel free to reply
to me if there is anything improper.

[1] https://patchwork.kernel.org/cover/11781317
[2] https://patchwork.ozlabs.org/project/qemu-devel/cover/20180704124923.32483-1-drjones@redhat.com

Andrew Jones (2):
  device_tree: add qemu_fdt_add_path
  hw/arm/virt: DT: add cpu-map

Ying Fang (10):
  linux headers: Update linux header with KVM_ARM_SET_MP_AFFINITY
  target/arm/kvm64: make MPIDR consistent with CPU Topology
  target/arm/kvm32: make MPIDR consistent with CPU Topology
  hw/arm/virt-acpi-build: distinguish possible and present cpus
  hw/acpi/aml-build: add processor hierarchy node structure
  hw/arm/virt-acpi-build: add PPTT table
  target/arm/cpu: Add CPU cache description for arm
  hw/arm/virt: add fdt cache information
  hw/acpi/aml-build: build ACPI CPU cache topology information
  hw/arm/virt-acpi-build: Enable CPU cache topology

 device_tree.c                |  24 +++++++
 hw/acpi/aml-build.c          |  68 +++++++++++++++++++
 hw/arm/virt-acpi-build.c     |  99 +++++++++++++++++++++++++--
 hw/arm/virt.c                | 128 ++++++++++++++++++++++++++++++++++-
 include/hw/acpi/acpi-defs.h  |  14 ++++
 include/hw/acpi/aml-build.h  |  11 +++
 include/hw/arm/virt.h        |   1 +
 include/sysemu/device_tree.h |   1 +
 linux-headers/linux/kvm.h    |   3 +
 target/arm/cpu.c             |  42 ++++++++++++
 target/arm/cpu.h             |  27 ++++++++
 target/arm/kvm32.c           |  46 ++++++++++---
 target/arm/kvm64.c           |  46 ++++++++++---
 13 files changed, 488 insertions(+), 22 deletions(-)

-- 
2.23.0

[RFC PATCH 01/12] linux headers: Update linux header with KVM_ARM_SET_MP_AFFINITY

[Ying Fang]

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 linux-headers/linux/kvm.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index a28c366737..461a2302e7 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1031,6 +1031,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PPC_SECURE_GUEST 181
 #define KVM_CAP_HALT_POLL 182
 #define KVM_CAP_ASYNC_PF_INT 183
+#define KVM_CAP_ARM_MP_AFFINITY 187
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1470,6 +1471,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
 /* Memory Encryption Commands */
 #define KVM_MEMORY_ENCRYPT_OP      _IOWR(KVMIO, 0xba, unsigned long)
+/* Available with KVM_CAP_ARM_MP_AFFINITY */
+#define KVM_ARM_SET_MP_AFFINITY    _IOWR(KVMIO, 0xbb, unsigned long)
 
 struct kvm_enc_region {
 	__u64 addr;
-- 
2.23.0

[RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Ying Fang]

MPIDR helps to provide an additional PE identification in a multiprocessor
system. This patch adds support for setting MPIDR from userspace, so that
MPIDR is consistent with CPU topology configured.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index ef1e960285..fcce261a10 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
 
 #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
 
+static int kvm_arm_set_mp_affinity(CPUState *cs)
+{
+    uint64_t mpidr;
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
+        /* Make MPIDR consistent with CPU topology */
+        MachineState *ms = MACHINE(qdev_get_machine());
+
+        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
+        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
+                                    & 0xff) << ARM_AFF1_SHIFT;
+        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
+                                    & 0xff) << ARM_AFF2_SHIFT;
+
+        /* Override mp affinity when KVM is in use */
+        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
+
+        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
+        mpidr |= (1ULL << 31);
+        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
+    } else {
+        /*
+         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
+         * own idea about MPIDR assignment, so we override our defaults with
+         * what we get from KVM.
+         */
+        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
+        if (ret) {
+            error_report("failed to set MPIDR");
+            return ret;
+        }
+        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
+        return ret;
+    }
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret;
-    uint64_t mpidr;
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
 
@@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         }
     }
 
-    /*
-     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
-     * Currently KVM has its own idea about MPIDR assignment, so we
-     * override our defaults with what we get from KVM.
-     */
-    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
+    ret = kvm_arm_set_mp_affinity(cs);
     if (ret) {
         return ret;
     }
-    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
 
     kvm_arm_init_debug(cs);
 
-- 
2.23.0

Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
> MPIDR helps to provide an additional PE identification in a multiprocessor
> system. This patch adds support for setting MPIDR from userspace, so that
> MPIDR is consistent with CPU topology configured.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index ef1e960285..fcce261a10 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
>  
>  #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
>  
> +static int kvm_arm_set_mp_affinity(CPUState *cs)
> +{
> +    uint64_t mpidr;
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
> +        /* Make MPIDR consistent with CPU topology */
> +        MachineState *ms = MACHINE(qdev_get_machine());
> +
> +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;

We should query KVM first to determine if it wants guests to see their PEs
as threads or not. If not, and ms->smp.threads is > 1, then that's an
error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
aff0 on it, as that could reduce IPI broadcast performance.

> +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
> +                                    & 0xff) << ARM_AFF1_SHIFT;
> +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
> +                                    & 0xff) << ARM_AFF2_SHIFT;
> +
> +        /* Override mp affinity when KVM is in use */
> +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> +
> +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
> +        mpidr |= (1ULL << 31);
> +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
> +    } else {
> +        /*
> +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
> +         * own idea about MPIDR assignment, so we override our defaults with
> +         * what we get from KVM.
> +         */
> +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> +        if (ret) {
> +            error_report("failed to set MPIDR");

We don't need this error, kvm_get_one_reg() has trace support already.
Anyway, the wording is wrong since it says 'set' instead of 'get'.

> +            return ret;
> +        }
> +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
> +        return ret;
> +    }
> +}
> +
>  int kvm_arch_init_vcpu(CPUState *cs)
>  {
>      int ret;
> -    uint64_t mpidr;
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>  
> @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>          }
>      }
>  
> -    /*
> -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
> -     * Currently KVM has its own idea about MPIDR assignment, so we
> -     * override our defaults with what we get from KVM.
> -     */
> -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> +    ret = kvm_arm_set_mp_affinity(cs);
>      if (ret) {
>          return ret;
>      }
> -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>  
>      kvm_arm_init_debug(cs);
>  
> -- 
> 2.23.0
> 
>

Thanks,
drew 

Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Andrew Jones]

On Thu, Sep 17, 2020 at 09:53:35AM +0200, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
> > MPIDR helps to provide an additional PE identification in a multiprocessor
> > system. This patch adds support for setting MPIDR from userspace, so that
> > MPIDR is consistent with CPU topology configured.
> > 
> > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > ---
> >  target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 38 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index ef1e960285..fcce261a10 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
> >  
> >  #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
> >  
> > +static int kvm_arm_set_mp_affinity(CPUState *cs)
> > +{
> > +    uint64_t mpidr;
> > +    ARMCPU *cpu = ARM_CPU(cs);
> > +
> > +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
> > +        /* Make MPIDR consistent with CPU topology */
> > +        MachineState *ms = MACHINE(qdev_get_machine());
> > +
> > +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
> 
> We should query KVM first to determine if it wants guests to see their PEs
> as threads or not. If not, and ms->smp.threads is > 1, then that's an
> error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
> aff0 on it, as that could reduce IPI broadcast performance.
> 
> > +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
> > +                                    & 0xff) << ARM_AFF1_SHIFT;
> > +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
> > +                                    & 0xff) << ARM_AFF2_SHIFT;

Also, as pointed out in the KVM thread, we should not be attempting to
describe topology with the MPIDR at all. Alexandru pointed out [*] as
evidence for that.

However, we do need to consider the limits on Aff0 imposed by the GIC.
See hw/arm/virt.c:virt_cpu_mp_affinity() for how we currently do it
for TCG. We should do something similar for KVM guests when we're taking
full control of the MPIDR.

[*] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7

Thanks,
drew

> > +
> > +        /* Override mp affinity when KVM is in use */
> > +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> > +
> > +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
> > +        mpidr |= (1ULL << 31);
> > +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
> > +    } else {
> > +        /*
> > +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
> > +         * own idea about MPIDR assignment, so we override our defaults with
> > +         * what we get from KVM.
> > +         */
> > +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> > +        if (ret) {
> > +            error_report("failed to set MPIDR");
> 
> We don't need this error, kvm_get_one_reg() has trace support already.
> Anyway, the wording is wrong since it says 'set' instead of 'get'.
> 
> > +            return ret;
> > +        }
> > +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
> > +        return ret;
> > +    }
> > +}
> > +
> >  int kvm_arch_init_vcpu(CPUState *cs)
> >  {
> >      int ret;
> > -    uint64_t mpidr;
> >      ARMCPU *cpu = ARM_CPU(cs);
> >      CPUARMState *env = &cpu->env;
> >  
> > @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
> >          }
> >      }
> >  
> > -    /*
> > -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
> > -     * Currently KVM has its own idea about MPIDR assignment, so we
> > -     * override our defaults with what we get from KVM.
> > -     */
> > -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> > +    ret = kvm_arm_set_mp_affinity(cs);
> >      if (ret) {
> >          return ret;
> >      }
> > -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> >  
> >      kvm_arm_init_debug(cs);
> >  
> > -- 
> > 2.23.0
> > 
> >
> 
> Thanks,
> drew 

Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Ying Fang]

On 9/17/2020 6:59 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 09:53:35AM +0200, Andrew Jones wrote:
>> On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
>>> MPIDR helps to provide an additional PE identification in a multiprocessor
>>> system. This patch adds support for setting MPIDR from userspace, so that
>>> MPIDR is consistent with CPU topology configured.
>>>
>>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>>> ---
>>>   target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>>>   1 file changed, 38 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>>> index ef1e960285..fcce261a10 100644
>>> --- a/target/arm/kvm64.c
>>> +++ b/target/arm/kvm64.c
>>> @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
>>>   
>>>   #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
>>>   
>>> +static int kvm_arm_set_mp_affinity(CPUState *cs)
>>> +{
>>> +    uint64_t mpidr;
>>> +    ARMCPU *cpu = ARM_CPU(cs);
>>> +
>>> +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
>>> +        /* Make MPIDR consistent with CPU topology */
>>> +        MachineState *ms = MACHINE(qdev_get_machine());
>>> +
>>> +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
>>
>> We should query KVM first to determine if it wants guests to see their PEs
>> as threads or not. If not, and ms->smp.threads is > 1, then that's an
>> error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
>> aff0 on it, as that could reduce IPI broadcast performance.
>>
>>> +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
>>> +                                    & 0xff) << ARM_AFF1_SHIFT;
>>> +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
>>> +                                    & 0xff) << ARM_AFF2_SHIFT;
> 
> Also, as pointed out in the KVM thread, we should not be attempting to
> describe topology with the MPIDR at all. Alexandru pointed out [*] as
> evidence for that.
> 
> However, we do need to consider the limits on Aff0 imposed by the GIC.
> See hw/arm/virt.c:virt_cpu_mp_affinity() for how we currently do it
> for TCG. We should do something similar for KVM guests when we're taking
> full control of the MPIDR.
> 
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7
> 
> Thanks,
> drew

Thanks for your information on MPIDR. As described in [*], MPIDR cannot
be trusted as the actual topology. After applying:
arm64: topology: Stop using MPIDR for topology information

Can we just use topology information from ACPI or fdt as topology and 
ignore MPIDR ?

> 
>>> +
>>> +        /* Override mp affinity when KVM is in use */
>>> +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>> +
>>> +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
>>> +        mpidr |= (1ULL << 31);
>>> +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
>>> +    } else {
>>> +        /*
>>> +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
>>> +         * own idea about MPIDR assignment, so we override our defaults with
>>> +         * what we get from KVM.
>>> +         */
>>> +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
>>> +        if (ret) {
>>> +            error_report("failed to set MPIDR");
>>
>> We don't need this error, kvm_get_one_reg() has trace support already.
>> Anyway, the wording is wrong since it says 'set' instead of 'get'.
>>
>>> +            return ret;
>>> +        }
>>> +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
>>> +        return ret;
>>> +    }
>>> +}
>>> +
>>>   int kvm_arch_init_vcpu(CPUState *cs)
>>>   {
>>>       int ret;
>>> -    uint64_t mpidr;
>>>       ARMCPU *cpu = ARM_CPU(cs);
>>>       CPUARMState *env = &cpu->env;
>>>   
>>> @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>>           }
>>>       }
>>>   
>>> -    /*
>>> -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
>>> -     * Currently KVM has its own idea about MPIDR assignment, so we
>>> -     * override our defaults with what we get from KVM.
>>> -     */
>>> -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
>>> +    ret = kvm_arm_set_mp_affinity(cs);
>>>       if (ret) {
>>>           return ret;
>>>       }
>>> -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>>   
>>>       kvm_arm_init_debug(cs);
>>>   
>>> -- 
>>> 2.23.0
>>>
>>>
>>
>> Thanks,
>> drew
> 
> .
> 

Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Andrew Jones]

On Thu, Sep 17, 2020 at 09:19:34PM +0800, Ying Fang wrote:
> 
> 
> On 9/17/2020 6:59 PM, Andrew Jones wrote:
> > On Thu, Sep 17, 2020 at 09:53:35AM +0200, Andrew Jones wrote:
> > > On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
> > > > MPIDR helps to provide an additional PE identification in a multiprocessor
> > > > system. This patch adds support for setting MPIDR from userspace, so that
> > > > MPIDR is consistent with CPU topology configured.
> > > > 
> > > > Signed-off-by: Ying Fang <fangying1@huawei.com>
> > > > ---
> > > >   target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
> > > >   1 file changed, 38 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > > > index ef1e960285..fcce261a10 100644
> > > > --- a/target/arm/kvm64.c
> > > > +++ b/target/arm/kvm64.c
> > > > @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
> > > >   #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
> > > > +static int kvm_arm_set_mp_affinity(CPUState *cs)
> > > > +{
> > > > +    uint64_t mpidr;
> > > > +    ARMCPU *cpu = ARM_CPU(cs);
> > > > +
> > > > +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
> > > > +        /* Make MPIDR consistent with CPU topology */
> > > > +        MachineState *ms = MACHINE(qdev_get_machine());
> > > > +
> > > > +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
> > > 
> > > We should query KVM first to determine if it wants guests to see their PEs
> > > as threads or not. If not, and ms->smp.threads is > 1, then that's an
> > > error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
> > > aff0 on it, as that could reduce IPI broadcast performance.
> > > 
> > > > +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
> > > > +                                    & 0xff) << ARM_AFF1_SHIFT;
> > > > +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
> > > > +                                    & 0xff) << ARM_AFF2_SHIFT;
> > 
> > Also, as pointed out in the KVM thread, we should not be attempting to
> > describe topology with the MPIDR at all. Alexandru pointed out [*] as
> > evidence for that.
> > 
> > However, we do need to consider the limits on Aff0 imposed by the GIC.
> > See hw/arm/virt.c:virt_cpu_mp_affinity() for how we currently do it
> > for TCG. We should do something similar for KVM guests when we're taking
> > full control of the MPIDR.
> > 
> > [*] https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/commit/?id=3102bc0e6ac7
> > 
> > Thanks,
> > drew
> 
> Thanks for your information on MPIDR. As described in [*], MPIDR cannot
> be trusted as the actual topology. After applying:
> arm64: topology: Stop using MPIDR for topology information
> 
> Can we just use topology information from ACPI or fdt as topology and ignore
> MPIDR ?

Well the MPIDR will still be used as a unique identifier and it must
still confirm to the architecture (e.g. 0-15 for Aff0 for gicv3), but,
yes, the point is that the guest kernel should never try to use it for
topology information. The guest kernel should only use DT or ACPI.

Thanks,
drew

> 
> > 
> > > > +
> > > > +        /* Override mp affinity when KVM is in use */
> > > > +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> > > > +
> > > > +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
> > > > +        mpidr |= (1ULL << 31);
> > > > +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
> > > > +    } else {
> > > > +        /*
> > > > +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
> > > > +         * own idea about MPIDR assignment, so we override our defaults with
> > > > +         * what we get from KVM.
> > > > +         */
> > > > +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> > > > +        if (ret) {
> > > > +            error_report("failed to set MPIDR");
> > > 
> > > We don't need this error, kvm_get_one_reg() has trace support already.
> > > Anyway, the wording is wrong since it says 'set' instead of 'get'.
> > > 
> > > > +            return ret;
> > > > +        }
> > > > +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
> > > > +        return ret;
> > > > +    }
> > > > +}
> > > > +
> > > >   int kvm_arch_init_vcpu(CPUState *cs)
> > > >   {
> > > >       int ret;
> > > > -    uint64_t mpidr;
> > > >       ARMCPU *cpu = ARM_CPU(cs);
> > > >       CPUARMState *env = &cpu->env;
> > > > @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
> > > >           }
> > > >       }
> > > > -    /*
> > > > -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
> > > > -     * Currently KVM has its own idea about MPIDR assignment, so we
> > > > -     * override our defaults with what we get from KVM.
> > > > -     */
> > > > -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
> > > > +    ret = kvm_arm_set_mp_affinity(cs);
> > > >       if (ret) {
> > > >           return ret;
> > > >       }
> > > > -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
> > > >       kvm_arm_init_debug(cs);
> > > > -- 
> > > > 2.23.0
> > > > 
> > > > 
> > > 
> > > Thanks,
> > > drew
> > 
> > .
> > 
> 

Re: [RFC PATCH 02/12] target/arm/kvm64: make MPIDR consistent with CPU Topology

[Ying Fang]

On 9/17/2020 3:53 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:23AM +0800, Ying Fang wrote:
>> MPIDR helps to provide an additional PE identification in a multiprocessor
>> system. This patch adds support for setting MPIDR from userspace, so that
>> MPIDR is consistent with CPU topology configured.
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   target/arm/kvm64.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index ef1e960285..fcce261a10 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -757,10 +757,46 @@ static int kvm_arm_sve_set_vls(CPUState *cs)
>>   
>>   #define ARM_CPU_ID_MPIDR       3, 0, 0, 0, 5
>>   
>> +static int kvm_arm_set_mp_affinity(CPUState *cs)
>> +{
>> +    uint64_t mpidr;
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +
>> +    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
>> +        /* Make MPIDR consistent with CPU topology */
>> +        MachineState *ms = MACHINE(qdev_get_machine());
>> +
>> +        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
> 
> We should query KVM first to determine if it wants guests to see their PEs
> as threads or not. If not, and ms->smp.threads is > 1, then that's an
> error. And, in any case, if ms->smp.threads == 1, then we shouldn't waste
> aff0 on it, as that could reduce IPI broadcast performance.

Yes, good catch. Should check against smp.threads before filling
the MPIDR value.

> 
>> +        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
>> +                                    & 0xff) << ARM_AFF1_SHIFT;
>> +        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
>> +                                    & 0xff) << ARM_AFF2_SHIFT;
>> +
>> +        /* Override mp affinity when KVM is in use */
>> +        cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>> +
>> +        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
>> +        mpidr |= (1ULL << 31);
>> +        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
>> +    } else {
>> +        /*
>> +         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
>> +         * own idea about MPIDR assignment, so we override our defaults with
>> +         * what we get from KVM.
>> +         */
>> +        int ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
>> +        if (ret) {
>> +            error_report("failed to set MPIDR");
> 
> We don't need this error, kvm_get_one_reg() has trace support already.
> Anyway, the wording is wrong since it says 'set' instead of 'get'.

Yes, my careless, I will fix it.

> 
>> +            return ret;
>> +        }
>> +        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
>> +        return ret;
>> +    }
>> +}
>> +
>>   int kvm_arch_init_vcpu(CPUState *cs)
>>   {
>>       int ret;
>> -    uint64_t mpidr;
>>       ARMCPU *cpu = ARM_CPU(cs);
>>       CPUARMState *env = &cpu->env;
>>   
>> @@ -814,16 +850,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>           }
>>       }
>>   
>> -    /*
>> -     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
>> -     * Currently KVM has its own idea about MPIDR assignment, so we
>> -     * override our defaults with what we get from KVM.
>> -     */
>> -    ret = kvm_get_one_reg(cs, ARM64_SYS_REG(ARM_CPU_ID_MPIDR), &mpidr);
>> +    ret = kvm_arm_set_mp_affinity(cs);
>>       if (ret) {
>>           return ret;
>>       }
>> -    cpu->mp_affinity = mpidr & ARM64_AFFINITY_MASK;
>>   
>>       kvm_arm_init_debug(cs);
>>   
>> -- 
>> 2.23.0
>>
>>
> 
> Thanks,
> drew
> 
> .
> 

[RFC PATCH 03/12] target/arm/kvm32: make MPIDR consistent with CPU Topology

[Ying Fang]

MPIDR helps to provide an additional PE identification in a multiprocessor
system. This patch adds support for setting MPIDR from userspace, so that
MPIDR is consistent with CPU topology configured.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 target/arm/kvm32.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0af46b41c8..85694dc8bf 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -201,11 +201,47 @@ int kvm_arm_cpreg_level(uint64_t regidx)
 
 #define ARM_CPU_ID_MPIDR       0, 0, 0, 5
 
+static int kvm_arm_set_mp_affinity(CPUState *cs)
+{
+    uint32_t mpidr;
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (kvm_check_extension(kvm_state, KVM_CAP_ARM_MP_AFFINITY)) {
+        /* Make MPIDR consistent with CPU topology */
+        MachineState *ms = MACHINE(qdev_get_machine());
+
+        mpidr = (kvm_arch_vcpu_id(cs) % ms->smp.threads) << ARM_AFF0_SHIFT;
+        mpidr |= ((kvm_arch_vcpu_id(cs) / ms->smp.threads % ms->smp.cores)
+                                        & 0xff) << ARM_AFF1_SHIFT;
+        mpidr |= (kvm_arch_vcpu_id(cs) / (ms->smp.cores * ms->smp.threads)
+                                       & 0xff) << ARM_AFF2_SHIFT;
+
+        /* Override mp affinity when KVM is in use */
+        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
+
+        /* Bit 31 is RES1 indicates the ARMv7 Multiprocessing Extensions */
+        mpidr |= (1ULL << 31);
+        return kvm_vcpu_ioctl(cs, KVM_ARM_SET_MP_AFFINITY, &mpidr);
+    } else {
+        /*
+         * When KVM_CAP_ARM_MP_AFFINITY is not supported, it means KVM has its
+         * own idea about MPIDR assignment, so we override our defaults with
+         * what we get from KVM.
+         */
+        int ret = kvm_get_one_reg(cs, ARM_CP15_REG32(ARM_CPU_ID_MPIDR), &mpidr);
+        if (ret) {
+            error_report("failed to set MPIDR");
+            return ret;
+        }
+        cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
+        return ret;
+    }
+}
+
 int kvm_arch_init_vcpu(CPUState *cs)
 {
     int ret;
     uint64_t v;
-    uint32_t mpidr;
     struct kvm_one_reg r;
     ARMCPU *cpu = ARM_CPU(cs);
 
@@ -244,16 +280,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return -EINVAL;
     }
 
-    /*
-     * When KVM is in use, PSCI is emulated in-kernel and not by qemu.
-     * Currently KVM has its own idea about MPIDR assignment, so we
-     * override our defaults with what we get from KVM.
-     */
-    ret = kvm_get_one_reg(cs, ARM_CP15_REG32(ARM_CPU_ID_MPIDR), &mpidr);
+    ret = kvm_arm_set_mp_affinity(cs);
     if (ret) {
         return ret;
     }
-    cpu->mp_affinity = mpidr & ARM32_AFFINITY_MASK;
 
     /* Check whether userspace can specify guest syndrome value */
     kvm_arm_init_serror_injection(cs);
-- 
2.23.0

Re: [RFC PATCH 03/12] target/arm/kvm32: make MPIDR consistent with CPU Topology

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:24AM +0800, Ying Fang wrote:
> MPIDR helps to provide an additional PE identification in a multiprocessor
> system. This patch adds support for setting MPIDR from userspace, so that
> MPIDR is consistent with CPU topology configured.
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  target/arm/kvm32.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 0af46b41c8..85694dc8bf 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c

This file no longer exists in mainline. Please rebase the whole series.

Thanks,
drew

Re: [RFC PATCH 03/12] target/arm/kvm32: make MPIDR consistent with CPU Topology

[Ying Fang]

On 9/17/2020 4:07 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:24AM +0800, Ying Fang wrote:
>> MPIDR helps to provide an additional PE identification in a multiprocessor
>> system. This patch adds support for setting MPIDR from userspace, so that
>> MPIDR is consistent with CPU topology configured.
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   target/arm/kvm32.c | 46 ++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 38 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
>> index 0af46b41c8..85694dc8bf 100644
>> --- a/target/arm/kvm32.c
>> +++ b/target/arm/kvm32.c
> 
> This file no longer exists in mainline. Please rebase the whole series.
Thanks, it is gone. Will rebase it.
> 
> Thanks,
> drew
> 
> .
> 

[RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Ying Fang]

From: Andrew Jones <drjones@redhat.com>

qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
also recursively adds any missing parent nodes.

Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 device_tree.c                | 24 ++++++++++++++++++++++++
 include/sysemu/device_tree.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index b335dae707..1854be3a02 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
     return retval;
 }
 
+int qemu_fdt_add_path(void *fdt, const char *path)
+{
+    char *parent;
+    int offset;
+
+    offset = fdt_path_offset(fdt, path);
+    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
+        error_report("%s Couldn't find node %s: %s", __func__, path,
+                     fdt_strerror(offset));
+        exit(1);
+    }
+
+    if (offset != -FDT_ERR_NOTFOUND) {
+        return offset;
+    }
+
+    parent = g_strdup(path);
+    strrchr(parent, '/')[0] = '\0';
+    qemu_fdt_add_path(fdt, parent);
+    g_free(parent);
+
+    return qemu_fdt_add_subnode(fdt, path);
+}
+
 void qemu_fdt_dumpdtb(void *fdt, int size)
 {
     const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index 982c89345f..15fb98af98 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -104,6 +104,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
 uint32_t qemu_fdt_alloc_phandle(void *fdt);
 int qemu_fdt_nop_node(void *fdt, const char *node_path);
 int qemu_fdt_add_subnode(void *fdt, const char *name);
+int qemu_fdt_add_path(void *fdt, const char *path);
 
 #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
     do {                                                                      \
-- 
2.23.0

Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:25AM +0800, Ying Fang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> also recursively adds any missing parent nodes.
> 
> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  device_tree.c                | 24 ++++++++++++++++++++++++
>  include/sysemu/device_tree.h |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index b335dae707..1854be3a02 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>      return retval;
>  }
>  
> +int qemu_fdt_add_path(void *fdt, const char *path)
> +{
> +    char *parent;
> +    int offset;
> +
> +    offset = fdt_path_offset(fdt, path);
> +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> +        error_report("%s Couldn't find node %s: %s", __func__, path,
> +                     fdt_strerror(offset));
> +        exit(1);
> +    }
> +
> +    if (offset != -FDT_ERR_NOTFOUND) {
> +        return offset;
> +    }
> +
> +    parent = g_strdup(path);
> +    strrchr(parent, '/')[0] = '\0';
> +    qemu_fdt_add_path(fdt, parent);
> +    g_free(parent);
> +
> +    return qemu_fdt_add_subnode(fdt, path);
> +}

Igor didn't like the recursion when I posted this before so I changed
it when doing the refresh[*] that I gave to Salil Mehta. Salil also
works for Huawei, are you guys not working together?

[*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh

Thanks,
drew

> +
>  void qemu_fdt_dumpdtb(void *fdt, int size)
>  {
>      const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index 982c89345f..15fb98af98 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -104,6 +104,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>  uint32_t qemu_fdt_alloc_phandle(void *fdt);
>  int qemu_fdt_nop_node(void *fdt, const char *node_path);
>  int qemu_fdt_add_subnode(void *fdt, const char *name);
> +int qemu_fdt_add_path(void *fdt, const char *path);
>  
>  #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>      do {                                                                      \
> -- 
> 2.23.0
> 
> 

Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Ying Fang]

On 9/17/2020 4:12 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:25AM +0800, Ying Fang wrote:
>> From: Andrew Jones <drjones@redhat.com>
>>
>> qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
>> also recursively adds any missing parent nodes.
>>
>> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> ---
>>   device_tree.c                | 24 ++++++++++++++++++++++++
>>   include/sysemu/device_tree.h |  1 +
>>   2 files changed, 25 insertions(+)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index b335dae707..1854be3a02 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
>>       return retval;
>>   }
>>   
>> +int qemu_fdt_add_path(void *fdt, const char *path)
>> +{
>> +    char *parent;
>> +    int offset;
>> +
>> +    offset = fdt_path_offset(fdt, path);
>> +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
>> +        error_report("%s Couldn't find node %s: %s", __func__, path,
>> +                     fdt_strerror(offset));
>> +        exit(1);
>> +    }
>> +
>> +    if (offset != -FDT_ERR_NOTFOUND) {
>> +        return offset;
>> +    }
>> +
>> +    parent = g_strdup(path);
>> +    strrchr(parent, '/')[0] = '\0';
>> +    qemu_fdt_add_path(fdt, parent);
>> +    g_free(parent);
>> +
>> +    return qemu_fdt_add_subnode(fdt, path);
>> +}
> 
> Igor didn't like the recursion when I posted this before so I changed
> it when doing the refresh[*] that I gave to Salil Mehta. Salil also
> works for Huawei, are you guys not working together?
> 
> [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh

Thanks for the sync. I'll look into it. I did not know about the refresh
and the effort Salil Mehta has made on this. We are not in the same dept 
and work for different projects.

Thanks Ying.
> 
> Thanks,
> drew
> 
>> +
>>   void qemu_fdt_dumpdtb(void *fdt, int size)
>>   {
>>       const char *dumpdtb = qemu_opt_get(qemu_get_machine_opts(), "dumpdtb");
>> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
>> index 982c89345f..15fb98af98 100644
>> --- a/include/sysemu/device_tree.h
>> +++ b/include/sysemu/device_tree.h
>> @@ -104,6 +104,7 @@ uint32_t qemu_fdt_get_phandle(void *fdt, const char *path);
>>   uint32_t qemu_fdt_alloc_phandle(void *fdt);
>>   int qemu_fdt_nop_node(void *fdt, const char *node_path);
>>   int qemu_fdt_add_subnode(void *fdt, const char *name);
>> +int qemu_fdt_add_path(void *fdt, const char *path);
>>   
>>   #define qemu_fdt_setprop_cells(fdt, node_path, property, ...)                 \
>>       do {                                                                      \
>> -- 
>> 2.23.0
>>
>>
> 
> .
> 

RE: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Salil Mehta]

> From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> On Behalf Of Andrew Jones
> Sent: Thursday, September 17, 2020 9:13 AM
> To: fangying <fangying1@huawei.com>
> Cc: peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> Alexander Graf <agraf@suse.de>; qemu-devel@nongnu.org; Chenzhendong (alex)
> <alex.chen@huawei.com>; shannon.zhaosl@gmail.com; qemu-arm@nongnu.org;
> alistair.francis@wdc.com; imammedo@redhat.com
> Subject: Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path
> 
> On Thu, Sep 17, 2020 at 11:20:25AM +0800, Ying Fang wrote:
> > From: Andrew Jones <drjones@redhat.com>
> >
> > qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> > also recursively adds any missing parent nodes.
> >
> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  device_tree.c                | 24 ++++++++++++++++++++++++
> >  include/sysemu/device_tree.h |  1 +
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/device_tree.c b/device_tree.c
> > index b335dae707..1854be3a02 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> >      return retval;
> >  }
> >
> > +int qemu_fdt_add_path(void *fdt, const char *path)
> > +{
> > +    char *parent;
> > +    int offset;
> > +
> > +    offset = fdt_path_offset(fdt, path);
> > +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> > +        error_report("%s Couldn't find node %s: %s", __func__, path,
> > +                     fdt_strerror(offset));
> > +        exit(1);
> > +    }
> > +
> > +    if (offset != -FDT_ERR_NOTFOUND) {
> > +        return offset;
> > +    }
> > +
> > +    parent = g_strdup(path);
> > +    strrchr(parent, '/')[0] = '\0';
> > +    qemu_fdt_add_path(fdt, parent);
> > +    g_free(parent);
> > +
> > +    return qemu_fdt_add_subnode(fdt, path);
> > +}
> 
> Igor didn't like the recursion when I posted this before so I changed
> it when doing the refresh[*] that I gave to Salil Mehta. Salil also
> works for Huawei, are you guys not working together?
> 
> [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh
> 


I was not aware of this work going on. I am based at Cambridge and Fangying
in Hangzhou and work for different teams.

Yes, I have it and have integrated it with the Virtual CPU hotplug branch
and I am testing them.

I have also rebased virtual cpu hotplug patches and integrated the PMU
related changes recently been discussed in other mail-chain. Plus, I am
also dealing with the MPIDR/affinity part from the Kernel which has been
discussed earlier with the Marc Zyngier. 

It looks some of the changes are common here like ability to set MPIDR
of the vcpus at the time of their creation inside KVM. And the PPTT
table changes which were present in your refresh branch as well. Changes
related to the possible and present vcpus might need a sync as well.

@Andrew, should I take out the part which is common and affects the
virtual cpu hotplug and push them separately. This way I can have part
of the changes related to the vcpu hotplug done which will also cover
this patch-set requirements as well? 

@Fangying, will this work for you?


Salil. 

Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Andrew Jones]

On Fri, Sep 18, 2020 at 12:25:19AM +0000, Salil Mehta wrote:
> 
> > From: Qemu-arm [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > On Behalf Of Andrew Jones
> > Sent: Thursday, September 17, 2020 9:13 AM
> > To: fangying <fangying1@huawei.com>
> > Cc: peter.maydell@linaro.org; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> > Alexander Graf <agraf@suse.de>; qemu-devel@nongnu.org; Chenzhendong (alex)
> > <alex.chen@huawei.com>; shannon.zhaosl@gmail.com; qemu-arm@nongnu.org;
> > alistair.francis@wdc.com; imammedo@redhat.com
> > Subject: Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path
> > 
> > On Thu, Sep 17, 2020 at 11:20:25AM +0800, Ying Fang wrote:
> > > From: Andrew Jones <drjones@redhat.com>
> > >
> > > qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> > > also recursively adds any missing parent nodes.
> > >
> > > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > > Cc: Alexander Graf <agraf@suse.de>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  device_tree.c                | 24 ++++++++++++++++++++++++
> > >  include/sysemu/device_tree.h |  1 +
> > >  2 files changed, 25 insertions(+)
> > >
> > > diff --git a/device_tree.c b/device_tree.c
> > > index b335dae707..1854be3a02 100644
> > > --- a/device_tree.c
> > > +++ b/device_tree.c
> > > @@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> > >      return retval;
> > >  }
> > >
> > > +int qemu_fdt_add_path(void *fdt, const char *path)
> > > +{
> > > +    char *parent;
> > > +    int offset;
> > > +
> > > +    offset = fdt_path_offset(fdt, path);
> > > +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> > > +        error_report("%s Couldn't find node %s: %s", __func__, path,
> > > +                     fdt_strerror(offset));
> > > +        exit(1);
> > > +    }
> > > +
> > > +    if (offset != -FDT_ERR_NOTFOUND) {
> > > +        return offset;
> > > +    }
> > > +
> > > +    parent = g_strdup(path);
> > > +    strrchr(parent, '/')[0] = '\0';
> > > +    qemu_fdt_add_path(fdt, parent);
> > > +    g_free(parent);
> > > +
> > > +    return qemu_fdt_add_subnode(fdt, path);
> > > +}
> > 
> > Igor didn't like the recursion when I posted this before so I changed
> > it when doing the refresh[*] that I gave to Salil Mehta. Salil also
> > works for Huawei, are you guys not working together?
> > 
> > [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh
> > 
> 
> 
> I was not aware of this work going on. I am based at Cambridge and Fangying
> in Hangzhou and work for different teams.
> 
> Yes, I have it and have integrated it with the Virtual CPU hotplug branch
> and I am testing them.
> 
> I have also rebased virtual cpu hotplug patches and integrated the PMU
> related changes recently been discussed in other mail-chain. Plus, I am
> also dealing with the MPIDR/affinity part from the Kernel which has been
> discussed earlier with the Marc Zyngier. 
> 
> It looks some of the changes are common here like ability to set MPIDR
> of the vcpus at the time of their creation inside KVM. And the PPTT
> table changes which were present in your refresh branch as well. Changes
> related to the possible and present vcpus might need a sync as well.
> 
> @Andrew, should I take out the part which is common and affects the
> virtual cpu hotplug and push them separately. This way I can have part
> of the changes related to the vcpu hotplug done which will also cover
> this patch-set requirements as well? 

Whatever works best for you and Ying Fang. It looks like this series
only focuses on topology. It's not considering present and possible
cpus, but it probably should. It also adds the cache hierarchy stuff,
but I'm not sure it's approaching that in the right way. I think it
may make sense to put this series on hold and take another look at
your hotplug series when it's reposted before deciding what to do.

Thanks,
drew

> 
> @Fangying, will this work for you?
> 
> 
> Salil. 
> 
> 

RE: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path

[Salil Mehta]

> From: Andrew Jones [mailto:drjones@redhat.com]
> Sent: Friday, September 18, 2020 7:07 AM
> 
> On Fri, Sep 18, 2020 at 12:25:19AM +0000, Salil Mehta wrote:
> >
> > > From: Qemu-arm
> [mailto:qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org]
> > > On Behalf Of Andrew Jones
> > > Sent: Thursday, September 17, 2020 9:13 AM
> > > To: fangying <fangying1@huawei.com>
> > > Cc: peter.maydell@linaro.org; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>;
> > > Alexander Graf <agraf@suse.de>; qemu-devel@nongnu.org; Chenzhendong (alex)
> > > <alex.chen@huawei.com>; shannon.zhaosl@gmail.com; qemu-arm@nongnu.org;
> > > alistair.francis@wdc.com; imammedo@redhat.com
> > > Subject: Re: [RFC PATCH 04/12] device_tree: add qemu_fdt_add_path
> > >
> > > On Thu, Sep 17, 2020 at 11:20:25AM +0800, Ying Fang wrote:
> > > > From: Andrew Jones <drjones@redhat.com>
> > > >
> > > > qemu_fdt_add_path works like qemu_fdt_add_subnode, except it
> > > > also recursively adds any missing parent nodes.
> > > >
> > > > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > > > Cc: Alexander Graf <agraf@suse.de>
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > >  device_tree.c                | 24 ++++++++++++++++++++++++
> > > >  include/sysemu/device_tree.h |  1 +
> > > >  2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/device_tree.c b/device_tree.c
> > > > index b335dae707..1854be3a02 100644
> > > > --- a/device_tree.c
> > > > +++ b/device_tree.c
> > > > @@ -524,6 +524,30 @@ int qemu_fdt_add_subnode(void *fdt, const char *name)
> > > >      return retval;
> > > >  }
> > > >
> > > > +int qemu_fdt_add_path(void *fdt, const char *path)
> > > > +{
> > > > +    char *parent;
> > > > +    int offset;
> > > > +
> > > > +    offset = fdt_path_offset(fdt, path);
> > > > +    if (offset < 0 && offset != -FDT_ERR_NOTFOUND) {
> > > > +        error_report("%s Couldn't find node %s: %s", __func__, path,
> > > > +                     fdt_strerror(offset));
> > > > +        exit(1);
> > > > +    }
> > > > +
> > > > +    if (offset != -FDT_ERR_NOTFOUND) {
> > > > +        return offset;
> > > > +    }
> > > > +
> > > > +    parent = g_strdup(path);
> > > > +    strrchr(parent, '/')[0] = '\0';
> > > > +    qemu_fdt_add_path(fdt, parent);
> > > > +    g_free(parent);
> > > > +
> > > > +    return qemu_fdt_add_subnode(fdt, path);
> > > > +}
> > >
> > > Igor didn't like the recursion when I posted this before so I changed
> > > it when doing the refresh[*] that I gave to Salil Mehta. Salil also
> > > works for Huawei, are you guys not working together?
> > >
> > > [*] https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh
> > >
> >
> >
> > I was not aware of this work going on. I am based at Cambridge and Fangying
> > in Hangzhou and work for different teams.
> >
> > Yes, I have it and have integrated it with the Virtual CPU hotplug branch
> > and I am testing them.
> >
> > I have also rebased virtual cpu hotplug patches and integrated the PMU
> > related changes recently been discussed in other mail-chain. Plus, I am
> > also dealing with the MPIDR/affinity part from the Kernel which has been
> > discussed earlier with the Marc Zyngier.
> >
> > It looks some of the changes are common here like ability to set MPIDR
> > of the vcpus at the time of their creation inside KVM. And the PPTT
> > table changes which were present in your refresh branch as well. Changes
> > related to the possible and present vcpus might need a sync as well.
> >
> > @Andrew, should I take out the part which is common and affects the
> > virtual cpu hotplug and push them separately. This way I can have part
> > of the changes related to the vcpu hotplug done which will also cover
> > this patch-set requirements as well?
> 
> Whatever works best for you and Ying Fang. It looks like this series
> only focuses on topology. It's not considering present and possible
> cpus, but it probably should. It also adds the cache hierarchy stuff,
> but I'm not sure it's approaching that in the right way. I think it
> may make sense to put this series on hold and take another look at
> your hotplug series when it's reposted before deciding what to do.


Ok fine. Let me collaborate with him internally first. Either of us
will have to rebase our patches on others code.


thanks


> 
> Thanks,
> drew
> 
> >
> > @Fangying, will this work for you?
> >
> >
> > Salil.
> >
> >

[RFC PATCH 05/12] hw/arm/virt: DT: add cpu-map

[Ying Fang]

From: Andrew Jones <drjones@redhat.com>

Support devicetree CPU topology descriptions.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c         | 37 ++++++++++++++++++++++++++++++++++++-
 include/hw/arm/virt.h |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index acf9bfbece..71f7dbb317 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -348,7 +348,10 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
     int cpu;
     int addr_cells = 1;
     const MachineState *ms = MACHINE(vms);
-
+    VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
+    unsigned int smp_cores = ms->smp.cores;
+    unsigned int smp_threads = ms->smp.threads;
+    bool cpu_topology_enabled = !vmc->ignore_cpu_topology;
     /*
      * From Documentation/devicetree/bindings/arm/cpus.txt
      *  On ARM v8 64-bit systems value should be set to 2,
@@ -404,8 +407,37 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
+                              qemu_fdt_alloc_phandle(vms->fdt));
+
         g_free(nodename);
     }
+    if (cpu_topology_enabled) {
+        /* Add vcpu topology by fdt node cpu-map. */
+        qemu_fdt_add_subnode(vms->fdt, "/cpus/cpu-map");
+
+        for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
+            char *cpu_path = g_strdup_printf("/cpus/cpu@%d", cpu);
+            char *map_path;
+
+            if (smp_threads > 1) {
+                map_path = g_strdup_printf(
+                           "/cpus/cpu-map/%s%d/%s%d/%s%d",
+                           "cluster", cpu / (smp_cores * smp_threads),
+                           "core", (cpu / smp_threads) % smp_cores,
+                           "thread", cpu % smp_threads);
+            } else {
+                map_path = g_strdup_printf(
+                           "/cpus/cpu-map/%s%d/%s%d",
+                           "cluster", cpu / smp_cores,
+                           "core", cpu % smp_cores);
+            }
+            qemu_fdt_add_path(vms->fdt, map_path);
+            qemu_fdt_setprop_phandle(vms->fdt, map_path, "cpu", cpu_path);
+            g_free(map_path);
+            g_free(cpu_path);
+        }
+    }
 }
 
 static void fdt_add_its_gic_node(VirtMachineState *vms)
@@ -2553,8 +2585,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 2)
 
 static void virt_machine_5_1_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_5_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+    vmc->ignore_cpu_topology = true;
 }
 DEFINE_VIRT_MACHINE(5, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index dff67e1bef..d37c6b7858 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -119,6 +119,7 @@ typedef struct {
     MachineClass parent;
     bool disallow_affinity_adjustment;
     bool no_its;
+    bool ignore_cpu_topology;
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
-- 
2.23.0

Re: [RFC PATCH 05/12] hw/arm/virt: DT: add cpu-map

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:26AM +0800, Ying Fang wrote:
> From: Andrew Jones <drjones@redhat.com>
> 
> Support devicetree CPU topology descriptions.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

There's a new version of this patch on the refresh branch.
https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh

drew

[RFC PATCH 06/12] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Ying Fang]

When building ACPI tables regarding CPUs we should always build
them for the number of possible CPUs, not the number of present
CPUs. We then ensure only the present CPUs are enabled.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt-acpi-build.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 9efd7a3881..f1d574b5d3 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -56,14 +56,18 @@
 
 #define ARM_SPI_BASE 32
 
-static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
+static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms)
 {
     uint16_t i;
+    CPUArchIdList *possible_cpus = MACHINE(vms)->possible_cpus;
 
-    for (i = 0; i < smp_cpus; i++) {
+    for (i = 0; i < possible_cpus->len; i++) {
         Aml *dev = aml_device("C%.03X", i);
         aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
         aml_append(dev, aml_name_decl("_UID", aml_int(i)));
+        if (possible_cpus->cpus[i].cpu == NULL) {
+            aml_append(dev, aml_name_decl("_STA", aml_int(0)));
+        }
         aml_append(scope, dev);
     }
 }
@@ -635,6 +639,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     const int *irqmap = vms->irqmap;
     AcpiMadtGenericDistributor *gicd;
     AcpiMadtGenericMsiFrame *gic_msi;
+    int possible_cpus = MACHINE(vms)->possible_cpus->len;
     int i;
 
     acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
@@ -645,7 +650,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     gicd->base_address = cpu_to_le64(memmap[VIRT_GIC_DIST].base);
     gicd->version = vms->gic_version;
 
-    for (i = 0; i < vms->smp_cpus; i++) {
+    for (i = 0; i < possible_cpus; i++) {
         AcpiMadtGenericCpuInterface *gicc = acpi_data_push(table_data,
                                                            sizeof(*gicc));
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(i));
@@ -660,7 +665,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
         gicc->cpu_interface_number = cpu_to_le32(i);
         gicc->arm_mpidr = cpu_to_le64(armcpu->mp_affinity);
         gicc->uid = cpu_to_le32(i);
-        gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        if (i < vms->smp_cpus) {
+            gicc->flags = cpu_to_le32(ACPI_MADT_GICC_ENABLED);
+        }
 
         if (arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
             gicc->performance_interrupt = cpu_to_le32(PPI(VIRTUAL_PMU_IRQ));
@@ -764,7 +771,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      * the RTC ACPI device at all when using UEFI.
      */
     scope = aml_scope("\\_SB");
-    acpi_dsdt_add_cpus(scope, vms->smp_cpus);
+    acpi_dsdt_add_cpus(scope, vms);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     if (vmc->acpi_expose_flash) {
-- 
2.23.0

Re: [RFC PATCH 06/12] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:27AM +0800, Ying Fang wrote:
> When building ACPI tables regarding CPUs we should always build
> them for the number of possible CPUs, not the number of present
> CPUs. We then ensure only the present CPUs are enabled.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

I approached this in a different way in the refresh, so the this patch
was dropped, but the refresh is completely untested, so something similar
may still be necessary.

Thanks,
drew

Re: [RFC PATCH 06/12] hw/arm/virt-acpi-build: distinguish possible and present cpus

[Ying Fang]

On 9/17/2020 4:20 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:27AM +0800, Ying Fang wrote:
>> When building ACPI tables regarding CPUs we should always build
>> them for the number of possible CPUs, not the number of present
>> CPUs. We then ensure only the present CPUs are enabled.
>>
>> Signed-off-by: Andrew Jones <drjones@redhat.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>>   hw/arm/virt-acpi-build.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> I approached this in a different way in the refresh, so the this patch
> was dropped, but the refresh is completely untested, so something similar
> may still be necessary.

Nice work, I'll open it and take a look.
Thanks.
> 
> Thanks,
> drew
> 
> .
> 

[RFC PATCH 07/12] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang]

Add the processor hierarchy node structures to build ACPI information
for CPU topology. Three helpers are introduced:

(1) build_socket_hierarchy for socket description structure
(2) build_processor_hierarchy for processor description structure
(3) build_smt_hierarchy for thread (logic processor) description structure

Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
---
 hw/acpi/aml-build.c         | 37 +++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h |  7 +++++++
 2 files changed, 44 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f6fbc9b95d..13eb6e1345 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1754,6 +1754,43 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
                  table_data->len - slit_start, 1, NULL, NULL);
 }
 
+/*
+ * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
+ */
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, 0);          /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
+    build_append_int_noprefix(tbl, 1, 4);  /* Flags: Physical package */
+    build_append_int_noprefix(tbl, parent, 4);  /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, 0);          /* Type 0 - processor */
+    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
+    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
+    build_append_int_noprefix(tbl, parent, 4); /* Parent */
+    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+}
+
+void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+{
+    build_append_byte(tbl, 0);            /* Type 0 - processor */
+    build_append_byte(tbl, 20);           /* Length, add private resources */
+    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
+    build_append_int_noprefix(tbl, 0x0e, 4);    /* Processor is a thread */
+    build_append_int_noprefix(tbl, parent , 4); /* parent */
+    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
+    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
+}
+
 /* build rev1/rev3/rev5.1 FADT */
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id)
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index d27da03d64..ff4c6a38f3 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -435,6 +435,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 
 void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
 
+void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+
+void build_processor_hierarchy(GArray *tbl, uint32_t flags,
+                               uint32_t parent, uint32_t id);
+
+void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+
 void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
                 const char *oem_id, const char *oem_table_id);
 
-- 
2.23.0

Re: [RFC PATCH 07/12] hw/acpi/aml-build: add processor hierarchy node structure

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:28AM +0800, Ying Fang wrote:
> Add the processor hierarchy node structures to build ACPI information
> for CPU topology. Three helpers are introduced:
> 
> (1) build_socket_hierarchy for socket description structure
> (2) build_processor_hierarchy for processor description structure
> (3) build_smt_hierarchy for thread (logic processor) description structure
> 
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
> ---
>  hw/acpi/aml-build.c         | 37 +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h |  7 +++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index f6fbc9b95d..13eb6e1345 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1754,6 +1754,43 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
>                   table_data->len - slit_start, 1, NULL, NULL);
>  }
>  
> +/*
> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
> + */
> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, 0);          /* Type 0 - processor */
> +    build_append_byte(tbl, 20);         /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
> +    build_append_int_noprefix(tbl, 1, 4);  /* Flags: Physical package */
> +    build_append_int_noprefix(tbl, parent, 4);  /* Parent */
> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> +}
> +
> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> +                               uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, 0);          /* Type 0 - processor */
> +    build_append_byte(tbl, 20);         /* Length, no private resources */
> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
> +}

I see you took this from
https://patchwork.ozlabs.org/project/qemu-devel/patch/20180704124923.32483-6-drjones@redhat.com/
(even though you neglected to mention that)

I've tweaked my implementation of it slightly per Igor's comments for the
refresh. See build_processor_hierarchy_node() in
https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11

> +
> +void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
> +{
> +    build_append_byte(tbl, 0);            /* Type 0 - processor */
> +    build_append_byte(tbl, 20);           /* Length, add private resources */
> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
> +    build_append_int_noprefix(tbl, 0x0e, 4);    /* Processor is a thread */
> +    build_append_int_noprefix(tbl, parent , 4); /* parent */
> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
> +}
> +
>  /* build rev1/rev3/rev5.1 FADT */
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id)
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index d27da03d64..ff4c6a38f3 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -435,6 +435,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>  
>  void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
>  
> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> +
> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
> +                               uint32_t parent, uint32_t id);
> +
> +void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);

Why add build_socket_hierarchy() and build_smt_hierarchy() ?

> +
>  void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>                  const char *oem_id, const char *oem_table_id);
>  
> -- 
> 2.23.0
> 

Thanks,
drew

Re: [RFC PATCH 07/12] hw/acpi/aml-build: add processor hierarchy node structure

[Ying Fang]

On 9/17/2020 4:27 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:28AM +0800, Ying Fang wrote:
>> Add the processor hierarchy node structures to build ACPI information
>> for CPU topology. Three helpers are introduced:
>>
>> (1) build_socket_hierarchy for socket description structure
>> (2) build_processor_hierarchy for processor description structure
>> (3) build_smt_hierarchy for thread (logic processor) description structure
>>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
>> ---
>>   hw/acpi/aml-build.c         | 37 +++++++++++++++++++++++++++++++++++++
>>   include/hw/acpi/aml-build.h |  7 +++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index f6fbc9b95d..13eb6e1345 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1754,6 +1754,43 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
>>                    table_data->len - slit_start, 1, NULL, NULL);
>>   }
>>   
>> +/*
>> + * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
>> + */
>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, 0);          /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>> +    build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
>> +    build_append_int_noprefix(tbl, 1, 4);  /* Flags: Physical package */
>> +    build_append_int_noprefix(tbl, parent, 4);  /* Parent */
>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>> +}
>> +
>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>> +                               uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, 0);          /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);         /* Length, no private resources */
>> +    build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
>> +    build_append_int_noprefix(tbl, flags, 4);  /* Flags */
>> +    build_append_int_noprefix(tbl, parent, 4); /* Parent */
>> +    build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
>> +    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
>> +}
> 
> I see you took this from
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20180704124923.32483-6-drjones@redhat.com/
> (even though you neglected to mention that)
> 
> I've tweaked my implementation of it slightly per Igor's comments for the
> refresh. See build_processor_hierarchy_node() in
> https://github.com/rhdrjones/qemu/commit/439b38d67ca1f2cbfa5b9892a822b651ebd05c11
Ok, I will sync with your work and test it.
> 
>> +
>> +void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
>> +{
>> +    build_append_byte(tbl, 0);            /* Type 0 - processor */
>> +    build_append_byte(tbl, 20);           /* Length, add private resources */
>> +    build_append_int_noprefix(tbl, 0, 2); /* Reserved */
>> +    build_append_int_noprefix(tbl, 0x0e, 4);    /* Processor is a thread */
>> +    build_append_int_noprefix(tbl, parent , 4); /* parent */
>> +    build_append_int_noprefix(tbl, id, 4);      /* ACPI processor ID */
>> +    build_append_int_noprefix(tbl, 0, 4);       /* Num of private resources */
>> +}
>> +
>>   /* build rev1/rev3/rev5.1 FADT */
>>   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>                   const char *oem_id, const char *oem_table_id)
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index d27da03d64..ff4c6a38f3 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -435,6 +435,13 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
>>   
>>   void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
>>   
>> +void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
>> +
>> +void build_processor_hierarchy(GArray *tbl, uint32_t flags,
>> +                               uint32_t parent, uint32_t id);
>> +
>> +void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
> 
> Why add build_socket_hierarchy() and build_smt_hierarchy() ?

To distinguish between socket, core and thread topology level,
build_socket_hierarchy and build_smt_hierarchy are introduced.
They will make the code logical in built_pptt much more much 
straightforward I think.

> 
>> +
>>   void build_fadt(GArray *tbl, BIOSLinker *linker, const AcpiFadtData *f,
>>                   const char *oem_id, const char *oem_table_id);
>>   
>> -- 
>> 2.23.0
>>
> 
> Thanks,
> drew
> 
> .
> 

[RFC PATCH 08/12] hw/arm/virt-acpi-build: add PPTT table

[Ying Fang]

Add the Processor Properties Topology Table (PPTT) to present CPU topology
information to the guest.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt-acpi-build.c | 42 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f1d574b5d3..b5aa3d3c83 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -594,6 +594,42 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  "SRAT", table_data->len - srat_start, 3, NULL, NULL);
 }
 
+static void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms)
+{
+    int pptt_start = table_data->len;
+    int uid = 0, cpus = 0, socket;
+    unsigned int smp_cores = ms->smp.cores;
+    unsigned int smp_threads = ms->smp.threads;
+
+    acpi_data_push(table_data, sizeof(AcpiTableHeader));
+
+    for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
+        uint32_t socket_offset = table_data->len - pptt_start;
+        int core;
+
+        build_socket_hierarchy(table_data, 0, socket);
+
+        for (core = 0; core < smp_cores; core++) {
+            uint32_t core_offset = table_data->len - pptt_start;
+            int thread;
+
+            if (smp_threads <= 1) {
+                build_processor_hierarchy(table_data, 2, socket_offset, uid++);
+             } else {
+                build_processor_hierarchy(table_data, 0, socket_offset, core);
+                for (thread = 0; thread < smp_threads; thread++) {
+                    build_smt_hierarchy(table_data, core_offset, uid++);
+                }
+             }
+        }
+        cpus += smp_cores * smp_threads;
+    }
+
+    build_header(linker, table_data,
+                 (void *)(table_data->data + pptt_start), "PPTT",
+                 table_data->len - pptt_start, 2, NULL, NULL);
+}
+
 /* GTDT */
 static void
 build_gtdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
@@ -834,6 +870,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     unsigned dsdt, xsdt;
     GArray *tables_blob = tables->table_data;
     MachineState *ms = MACHINE(vms);
+    bool cpu_topology_enabled = !vmc->ignore_cpu_topology;
 
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
@@ -853,6 +890,11 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
     acpi_add_table(table_offsets, tables_blob);
     build_madt(tables_blob, tables->linker, vms);
 
+    if (cpu_topology_enabled) {
+        acpi_add_table(table_offsets, tables_blob);
+        build_pptt(tables_blob, tables->linker, ms);
+    }
+
     acpi_add_table(table_offsets, tables_blob);
     build_gtdt(tables_blob, tables->linker, vms);
 
-- 
2.23.0

Re: [RFC PATCH 08/12] hw/arm/virt-acpi-build: add PPTT table

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:29AM +0800, Ying Fang wrote:
> Add the Processor Properties Topology Table (PPTT) to present CPU topology
> information to the guest.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
>  hw/arm/virt-acpi-build.c | 42 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>

There's a new version of this patch on the refresh branch.
https://github.com/rhdrjones/qemu/commits/virt-cpu-topology-refresh

drew 

[RFC PATCH 09/12] target/arm/cpu: Add CPU cache description for arm

[Ying Fang]

Add the CPUCacheInfo structure to hold CPU cache information for ARM cpus.
A classic three level cache topology is used here. The default cache
capacity is given and userspace can overwrite these values.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 target/arm/cpu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/cpu.h | 27 +++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index c179e0752d..efa8e1974a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -27,6 +27,7 @@
 #include "qapi/visitor.h"
 #include "cpu.h"
 #include "internals.h"
+#include "qemu/units.h"
 #include "exec/exec-all.h"
 #include "hw/qdev-properties.h"
 #if !defined(CONFIG_USER_ONLY)
@@ -998,6 +999,45 @@ uint64_t arm_cpu_mp_affinity(int idx, uint8_t clustersz)
     return (Aff1 << ARM_AFF1_SHIFT) | Aff0;
 }
 
+static CPUCaches default_cache_info = {
+    .l1d_cache = &(CPUCacheInfo) {
+    .type = DATA_CACHE,
+        .level = 1,
+        .size = 64 * KiB,
+        .line_size = 64,
+        .associativity = 4,
+        .sets = 256,
+        .attributes = 0x02,
+    },
+    .l1i_cache = &(CPUCacheInfo) {
+        .type = INSTRUCTION_CACHE,
+        .level = 1,
+        .size = 64 * KiB,
+        .line_size = 64,
+        .associativity = 4,
+        .sets = 256,
+        .attributes = 0x04,
+    },
+    .l2_cache = &(CPUCacheInfo) {
+        .type = UNIFIED_CACHE,
+        .level = 2,
+        .size = 512 * KiB,
+        .line_size = 64,
+        .associativity = 8,
+        .sets = 1024,
+        .attributes = 0x0a,
+    },
+    .l3_cache = &(CPUCacheInfo) {
+        .type = UNIFIED_CACHE,
+        .level = 3,
+        .size = 65536 * KiB,
+        .line_size = 64,
+        .associativity = 15,
+        .sets = 2048,
+        .attributes = 0x0a,
+    },
+};
+
 static void cpreg_hashtable_data_destroy(gpointer data)
 {
     /*
@@ -1835,6 +1875,8 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
         }
     }
 
+    cpu->caches = default_cache_info;
+
     qemu_init_vcpu(cs);
     cpu_reset(cs);
 
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a1c7d8ebae..e9e3817e20 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -745,6 +745,30 @@ typedef enum ARMPSCIState {
 
 typedef struct ARMISARegisters ARMISARegisters;
 
+/* Cache information type */
+enum CacheType {
+    DATA_CACHE,
+    INSTRUCTION_CACHE,
+    UNIFIED_CACHE
+};
+
+typedef struct CPUCacheInfo {
+    enum CacheType type;      /* Cache Type*/
+    uint8_t level;
+    uint32_t size;            /* Size in bytes */
+    uint16_t line_size;       /* Line size in bytes */
+    uint8_t associativity;    /* Cache associativity */
+    uint32_t sets;            /* Number of sets */
+    uint8_t attributes;       /* Cache attributest  */
+} CPUCacheInfo;
+
+typedef struct CPUCaches {
+        CPUCacheInfo *l1d_cache;
+        CPUCacheInfo *l1i_cache;
+        CPUCacheInfo *l2_cache;
+        CPUCacheInfo *l3_cache;
+} CPUCaches;
+
 /**
  * ARMCPU:
  * @env: #CPUARMState
@@ -986,6 +1010,9 @@ struct ARMCPU {
 
     /* Generic timer counter frequency, in Hz */
     uint64_t gt_cntfrq_hz;
+
+    /* CPU cache information */
+    CPUCaches caches;
 };
 
 unsigned int gt_cntfrq_period_ns(ARMCPU *cpu);
-- 
2.23.0

Re: [RFC PATCH 09/12] target/arm/cpu: Add CPU cache description for arm

[Andrew Jones]

On Thu, Sep 17, 2020 at 11:20:30AM +0800, Ying Fang wrote:
> Add the CPUCacheInfo structure to hold CPU cache information for ARM cpus.
> A classic three level cache topology is used here. The default cache
> capacity is given and userspace can overwrite these values.

Doesn't TCG already have some sort of fake cache hierarchy? If so, then
we shouldn't be adding another one, we should be simply describing the
one we already have. For KVM, we shouldn't describe anything other than
what is actually on the host.

drew

Re: [RFC PATCH 09/12] target/arm/cpu: Add CPU cache description for arm

[Ying Fang]

On 9/17/2020 4:39 PM, Andrew Jones wrote:
> On Thu, Sep 17, 2020 at 11:20:30AM +0800, Ying Fang wrote:
>> Add the CPUCacheInfo structure to hold CPU cache information for ARM cpus.
>> A classic three level cache topology is used here. The default cache
>> capacity is given and userspace can overwrite these values.
> 
> Doesn't TCG already have some sort of fake cache hierarchy? If so, then

TCG may have some sort of fake cache hierarchy via CCSIDR.

> we shouldn't be adding another one, we should be simply describing the
> one we already have. For KVM, we shouldn't describe anything other than
> what is actually on the host.

Yes, I agreed. Cache capacity should be the with host otherwise it may
have bad impact on guest performance, we can do that by query from the
host and make cache capacity configurable from userspace.

Dario Faggioli is going to give a talk about it in KVM forum [1].

[1] 
https://kvmforum2020.sched.com/event/eE1y/virtual-topology-for-virtual-machines-friend-or-foe-dario-faggioli-suse?iframe=no&w=100%&sidebar=yes&bg=no

Thanks.

> 
> drew
> 
> .
> 

[RFC PATCH 10/12] hw/arm/virt: add fdt cache information

[Ying Fang]

Support devicetree CPU cache information descriptions

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/arm/virt.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 71f7dbb317..74b748ae35 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -343,6 +343,89 @@ static void fdt_add_timer_nodes(const VirtMachineState *vms)
                        GIC_FDT_IRQ_TYPE_PPI, ARCH_TIMER_NS_EL2_IRQ, irqflags);
 }
 
+static void fdt_add_l3cache_nodes(const VirtMachineState *vms)
+{
+    int i;
+    const MachineState *ms = MACHINE(vms);
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+    unsigned int smp_cores = ms->smp.cores;
+    unsigned int sockets = ms->smp.max_cpus / smp_cores;
+
+    for (i = 0; i < sockets; i++) {
+        char *nodename = g_strdup_printf("/cpus/l3-cache%d", i);
+        qemu_fdt_add_subnode(vms->fdt, nodename);
+        qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "cache");
+        qemu_fdt_setprop_string(vms->fdt, nodename, "cache-unified", "true");
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-level", 3);
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-size",
+                              cpu->caches.l3_cache->size);
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-line-size",
+                              cpu->caches.l3_cache->line_size);
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-sets",
+                              cpu->caches.l3_cache->sets);
+        qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
+                              qemu_fdt_alloc_phandle(vms->fdt));
+        g_free(nodename);
+    }
+}
+
+static void fdt_add_l2cache_nodes(const VirtMachineState *vms)
+{
+    int i, j;
+    const MachineState *ms = MACHINE(vms);
+    unsigned int smp_cores = ms->smp.cores;
+    signed int sockets = ms->smp.max_cpus / smp_cores;
+    ARMCPU *cpu = ARM_CPU(first_cpu);
+
+    for (i = 0; i < sockets; i++) {
+        char *next_path = g_strdup_printf("/cpus/l3-cache%d", i);
+        for (j = 0; j < smp_cores; j++) {
+            char *nodename = g_strdup_printf("/cpus/l2-cache%d",
+                                  i * smp_cores + j);
+            qemu_fdt_add_subnode(vms->fdt, nodename);
+            qemu_fdt_setprop_string(vms->fdt, nodename, "compatible", "cache");
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-size",
+                                  cpu->caches.l2_cache->size);
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-line-size",
+                                  cpu->caches.l2_cache->line_size);
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "cache-sets",
+                                  cpu->caches.l2_cache->sets);
+            qemu_fdt_setprop_phandle(vms->fdt, nodename,
+                                  "next-level-cache", next_path);
+            qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
+                                  qemu_fdt_alloc_phandle(vms->fdt));
+            g_free(nodename);
+        }
+        g_free(next_path);
+    }
+}
+
+static void fdt_add_l1cache_prop(const VirtMachineState *vms,
+                            char *nodename, int cpu_index)
+{
+
+    ARMCPU *cpu = ARM_CPU(qemu_get_cpu(cpu_index));
+    CPUCaches caches = cpu->caches;
+
+    char *cachename = g_strdup_printf("/cpus/l2-cache%d", cpu_index);
+
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "d-cache-size",
+                          caches.l1d_cache->size);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "d-cache-line-size",
+                          caches.l1d_cache->line_size);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "d-cache-sets",
+                          caches.l1d_cache->sets);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "i-cache-size",
+                          caches.l1i_cache->size);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "i-cache-line-size",
+                          caches.l1i_cache->line_size);
+    qemu_fdt_setprop_cell(vms->fdt, nodename, "i-cache-sets",
+                          caches.l1i_cache->sets);
+    qemu_fdt_setprop_phandle(vms->fdt, nodename, "next-level-cache",
+                          cachename);
+    g_free(cachename);
+}
+
 static void fdt_add_cpu_nodes(const VirtMachineState *vms)
 {
     int cpu;
@@ -378,6 +461,11 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
     qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#address-cells", addr_cells);
     qemu_fdt_setprop_cell(vms->fdt, "/cpus", "#size-cells", 0x0);
 
+    if (cpu_topology_enabled) {
+        fdt_add_l3cache_nodes(vms);
+        fdt_add_l2cache_nodes(vms);
+    }
+
     for (cpu = vms->smp_cpus - 1; cpu >= 0; cpu--) {
         char *nodename = g_strdup_printf("/cpus/cpu@%d", cpu);
         ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(cpu));
@@ -407,6 +495,9 @@ static void fdt_add_cpu_nodes(const VirtMachineState *vms)
                 ms->possible_cpus->cpus[cs->cpu_index].props.node_id);
         }
 
+        if (cpu_topology_enabled) {
+            fdt_add_l1cache_prop(vms, nodename, cpu);
+        }
         qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle",
                               qemu_fdt_alloc_phandle(vms->fdt));
 
-- 
2.23.0

[RFC PATCH 11/12] hw/acpi/aml-build: build ACPI CPU cache topology information

[Ying Fang]

To build cache information, An AcpiCacheInfo structure is defined to
hold the Type 1 cache structure according to ACPI spec v6.3 5.2.29.2.
A helper function build_cache_hierarchy is introduced to encode the
cache information.

Signed-off-by: Ying Fang <fangying1@huawei.com>
---
 hw/acpi/aml-build.c         | 26 ++++++++++++++++++++++++++
 include/hw/acpi/acpi-defs.h |  8 ++++++++
 include/hw/acpi/aml-build.h |  3 +++
 3 files changed, 37 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 13eb6e1345..123eb032cd 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1754,6 +1754,32 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms)
                  table_data->len - slit_start, 1, NULL, NULL);
 }
 
+/* ACPI 6.3: 5.29.2 Cache type structure (Type 1) */
+static void build_cache_head(GArray *tbl, uint32_t next_level)
+{
+    build_append_byte(tbl, 1);
+    build_append_byte(tbl, 24);
+    build_append_int_noprefix(tbl, 0, 2);
+    build_append_int_noprefix(tbl, 0x7f, 4);
+    build_append_int_noprefix(tbl, next_level, 4);
+}
+
+static void build_cache_tail(GArray *tbl, AcpiCacheInfo *cache_info)
+{
+    build_append_int_noprefix(tbl, cache_info->size, 4);
+    build_append_int_noprefix(tbl, cache_info->sets, 4);
+    build_append_byte(tbl, cache_info->associativity);
+    build_append_byte(tbl, cache_info->attributes);
+    build_append_int_noprefix(tbl, cache_info->line_size, 2);
+}
+
+void build_cache_hierarchy(GArray *tbl,
+              uint32_t next_level, AcpiCacheInfo *cache_info)
+{
+    build_cache_head(tbl, next_level);
+    build_cache_tail(tbl, cache_info);
+}
+
 /*
  * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
  */
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 38a42f409a..3df38ab449 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -618,4 +618,12 @@ struct AcpiIortRC {
 } QEMU_PACKED;
 typedef struct AcpiIortRC AcpiIortRC;
 
+typedef struct AcpiCacheInfo {
+    uint32_t size;
+    uint32_t sets;
+    uint8_t  associativity;
+    uint8_t  attributes;
+    uint16_t line_size;
+} AcpiCacheInfo;
+
 #endif
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index ff4c6a38f3..ced1ae6a83 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -435,6 +435,9 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
 
 void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
 
+void build_cache_hierarchy(GArray *tbl,
+              uint32_t next_level, AcpiCacheInfo *cache_info);
+
 void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
 
 void build_processor_hierarchy(GArray *tbl, uint32_t flags,
-- 
2.23.0

[RFC PATCH 12/12] hw/arm/virt-acpi-build: Enable CPU cache topology

[Ying Fang]

A helper struct AcpiCacheOffset is introduced to describe the offset
of three level caches. The cache hierarchy is built according to
ACPI spec v6.3 5.2.29.2. Let's enable CPU cache topology now.

Signed-off-by: Ying Fang <fangying1@huawei.com>
Signed-off-by: Henglong Fan <fanhenglong@huawei.com>
---
 hw/acpi/aml-build.c         | 19 +++++++++-----
 hw/arm/virt-acpi-build.c    | 52 ++++++++++++++++++++++++++++++++-----
 include/hw/acpi/acpi-defs.h |  6 +++++
 include/hw/acpi/aml-build.h |  7 ++---
 4 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 123eb032cd..f8d74f3f10 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1783,27 +1783,32 @@ void build_cache_hierarchy(GArray *tbl,
 /*
  * ACPI 6.3: 5.2.29.1 Processor hierarchy node structure (Type 0)
  */
-void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
+void build_socket_hierarchy(GArray *tbl, uint32_t parent,
+                            uint32_t offset, uint32_t id)
 {
     build_append_byte(tbl, 0);          /* Type 0 - processor */
-    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_byte(tbl, 24);         /* Length, no private resources */
     build_append_int_noprefix(tbl, 0, 2);  /* Reserved */
     build_append_int_noprefix(tbl, 1, 4);  /* Flags: Physical package */
     build_append_int_noprefix(tbl, parent, 4);  /* Parent */
     build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
-    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+    build_append_int_noprefix(tbl, 1, 4);  /*  Number of private resources */
+    build_append_int_noprefix(tbl, offset, 4);  /* Private resources */
 }
 
-void build_processor_hierarchy(GArray *tbl, uint32_t flags,
-                               uint32_t parent, uint32_t id)
+void build_processor_hierarchy(GArray *tbl, uint32_t flags, uint32_t parent,
+                               AcpiCacheOffset offset, uint32_t id)
 {
     build_append_byte(tbl, 0);          /* Type 0 - processor */
-    build_append_byte(tbl, 20);         /* Length, no private resources */
+    build_append_byte(tbl, 32);         /* Length, no private resources */
     build_append_int_noprefix(tbl, 0, 2);      /* Reserved */
     build_append_int_noprefix(tbl, flags, 4);  /* Flags */
     build_append_int_noprefix(tbl, parent, 4); /* Parent */
     build_append_int_noprefix(tbl, id, 4);     /* ACPI processor ID */
-    build_append_int_noprefix(tbl, 0, 4);  /* Number of private resources */
+    build_append_int_noprefix(tbl, 3, 4);  /* Number of private resources */
+    build_append_int_noprefix(tbl, offset.l1d_offset, 4);/* Private resources */
+    build_append_int_noprefix(tbl, offset.l1i_offset, 4);/* Private resources */
+    build_append_int_noprefix(tbl, offset.l2_offset, 4); /* Private resources */
 }
 
 void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b5aa3d3c83..375fb9e24f 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -594,29 +594,69 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                  "SRAT", table_data->len - srat_start, 3, NULL, NULL);
 }
 
-static void build_pptt(GArray *table_data, BIOSLinker *linker, MachineState *ms)
+static inline void arm_acpi_cache_info(CPUCacheInfo *cpu_cache,
+                                       AcpiCacheInfo *acpi_cache)
 {
+    acpi_cache->size = cpu_cache->size;
+    acpi_cache->sets = cpu_cache->sets;
+    acpi_cache->associativity = cpu_cache->associativity;
+    acpi_cache->attributes = cpu_cache->attributes;
+    acpi_cache->line_size = cpu_cache->line_size;
+}
+
+static void build_pptt(GArray *table_data, BIOSLinker *linker,
+                       VirtMachineState *vms)
+{
+    MachineState *ms = MACHINE(vms);
     int pptt_start = table_data->len;
     int uid = 0, cpus = 0, socket;
     unsigned int smp_cores = ms->smp.cores;
     unsigned int smp_threads = ms->smp.threads;
+    AcpiCacheOffset offset;
+    ARMCPU *cpu = ARM_CPU(qemu_get_cpu(cpus));
+    AcpiCacheInfo cache_info;
 
     acpi_data_push(table_data, sizeof(AcpiTableHeader));
 
     for (socket = 0; cpus < ms->possible_cpus->len; socket++) {
-        uint32_t socket_offset = table_data->len - pptt_start;
+        uint32_t l3_offset = table_data->len - pptt_start;
+        uint32_t socket_offset;
         int core;
 
-        build_socket_hierarchy(table_data, 0, socket);
+        /* L3 cache type structure */
+        arm_acpi_cache_info(cpu->caches.l3_cache, &cache_info);
+        build_cache_hierarchy(table_data, 0, &cache_info);
+
+        socket_offset = table_data->len - pptt_start;
+        build_socket_hierarchy(table_data, 0, l3_offset, socket);
 
         for (core = 0; core < smp_cores; core++) {
             uint32_t core_offset = table_data->len - pptt_start;
             int thread;
 
+            /* L2 cache tpe structure */
+            offset.l2_offset = table_data->len - pptt_start;
+            arm_acpi_cache_info(cpu->caches.l2_cache, &cache_info);
+            build_cache_hierarchy(table_data, 0, &cache_info);
+
+            /* L1d cache type structure */
+            offset.l1d_offset = table_data->len - pptt_start;
+            arm_acpi_cache_info(cpu->caches.l1d_cache, &cache_info);
+            build_cache_hierarchy(table_data, offset.l2_offset, &cache_info);
+
+            /* L1i cache type structure */
+            offset.l1i_offset = table_data->len - pptt_start;
+            arm_acpi_cache_info(cpu->caches.l1i_cache, &cache_info);
+            build_cache_hierarchy(table_data, offset.l2_offset, &cache_info);
+
+            core_offset = table_data->len - pptt_start;
             if (smp_threads <= 1) {
-                build_processor_hierarchy(table_data, 2, socket_offset, uid++);
+                build_processor_hierarchy(table_data, 2, socket_offset,
+                                          offset, uid++);
              } else {
-                build_processor_hierarchy(table_data, 0, socket_offset, core);
+
+                build_processor_hierarchy(table_data, 0, socket_offset,
+                                          offset, core);
                 for (thread = 0; thread < smp_threads; thread++) {
                     build_smt_hierarchy(table_data, core_offset, uid++);
                 }
@@ -892,7 +932,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
 
     if (cpu_topology_enabled) {
         acpi_add_table(table_offsets, tables_blob);
-        build_pptt(tables_blob, tables->linker, ms);
+        build_pptt(tables_blob, tables->linker, vms);
     }
 
     acpi_add_table(table_offsets, tables_blob);
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
index 3df38ab449..e48b7fa506 100644
--- a/include/hw/acpi/acpi-defs.h
+++ b/include/hw/acpi/acpi-defs.h
@@ -626,4 +626,10 @@ typedef struct AcpiCacheInfo {
     uint16_t line_size;
 } AcpiCacheInfo;
 
+typedef struct AcpiCacheOffset {
+    uint32_t l1d_offset;
+    uint32_t l1i_offset;
+    uint32_t l2_offset;
+} AcpiCacheOffset;
+
 #endif
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index ced1ae6a83..984c5dec3b 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -438,10 +438,11 @@ void build_slit(GArray *table_data, BIOSLinker *linker, MachineState *ms);
 void build_cache_hierarchy(GArray *tbl,
               uint32_t next_level, AcpiCacheInfo *cache_info);
 
-void build_socket_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
+void build_socket_hierarchy(GArray *tbl, uint32_t parent,
+                            uint32_t offset, uint32_t id);
 
-void build_processor_hierarchy(GArray *tbl, uint32_t flags,
-                               uint32_t parent, uint32_t id);
+void build_processor_hierarchy(GArray *tbl, uint32_t flags, uint32_t parent,
+                               AcpiCacheOffset offset, uint32_t id);
 
 void build_smt_hierarchy(GArray *tbl, uint32_t parent, uint32_t id);
 
-- 
2.23.0

RE: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Zengtao (B)]

Cc valentin

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
> On Behalf Of Ying Fang
> Sent: Thursday, September 17, 2020 11:20 AM
> To: qemu-devel@nongnu.org
> Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
> Chenzhendong (alex); shannon.zhaosl@gmail.com;
> qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
> imammedo@redhat.com
> Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
> topology support
> 
> An accurate cpu topology may help improve the cpu scheduler's
> decision
> making when dealing with multi-core system. So cpu topology
> description
> is helpful to provide guest with the right view. Cpu cache information
> may
> also have slight impact on the sched domain, and even userspace
> software
> may check the cpu cache information to do some optimizations. Thus
> this patch
> series is posted to provide cpu and cache topology support for arm.
> 
> To make the cpu topology consistent with MPIDR, an vcpu ioctl

For aarch64, the cpu topology don't depends on the MPDIR.
See https://patchwork.kernel.org/patch/11744387/ 

> KVM_ARM_SET_MP_AFFINITY is introduced so that userspace can set
> MPIDR
> according to the topology specified [1]. To describe the cpu topology
> both fdt and ACPI are supported. To describe the cpu cache
> information,
> a default cache hierarchy is given and can be made configurable later.
> The cpu topology is built according to processor hierarchy node
> structure.
> The cpu cache information is built according to cache type structure.
> 
> This patch series is partially based on the patches posted by Andrew
> Jone
> years ago [2], I jumped in on it since some OS vendor cooperative
> partners
> are eager for it. Thanks for Andrew's contribution. Please feel free to
> reply
> to me if there is anything improper.
> 
> [1] https://patchwork.kernel.org/cover/11781317
> [2]
> https://patchwork.ozlabs.org/project/qemu-devel/cover/2018070412
> 4923.32483-1-drjones@redhat.com
> 
> Andrew Jones (2):
>   device_tree: add qemu_fdt_add_path
>   hw/arm/virt: DT: add cpu-map
> 
> Ying Fang (10):
>   linux headers: Update linux header with
> KVM_ARM_SET_MP_AFFINITY
>   target/arm/kvm64: make MPIDR consistent with CPU Topology
>   target/arm/kvm32: make MPIDR consistent with CPU Topology
>   hw/arm/virt-acpi-build: distinguish possible and present cpus
>   hw/acpi/aml-build: add processor hierarchy node structure
>   hw/arm/virt-acpi-build: add PPTT table
>   target/arm/cpu: Add CPU cache description for arm
>   hw/arm/virt: add fdt cache information
>   hw/acpi/aml-build: build ACPI CPU cache topology information
>   hw/arm/virt-acpi-build: Enable CPU cache topology
> 
>  device_tree.c                |  24 +++++++
>  hw/acpi/aml-build.c          |  68 +++++++++++++++++++
>  hw/arm/virt-acpi-build.c     |  99
> +++++++++++++++++++++++++--
>  hw/arm/virt.c                | 128
> ++++++++++++++++++++++++++++++++++-
>  include/hw/acpi/acpi-defs.h  |  14 ++++
>  include/hw/acpi/aml-build.h  |  11 +++
>  include/hw/arm/virt.h        |   1 +
>  include/sysemu/device_tree.h |   1 +
>  linux-headers/linux/kvm.h    |   3 +
>  target/arm/cpu.c             |  42 ++++++++++++
>  target/arm/cpu.h             |  27 ++++++++
>  target/arm/kvm32.c           |  46 ++++++++++---
>  target/arm/kvm64.c           |  46 ++++++++++---
>  13 files changed, 488 insertions(+), 22 deletions(-)
> 
> --
> 2.23.0
> 

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Andrew Jones]

On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
> Cc valentin
> 
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
> > On Behalf Of Ying Fang
> > Sent: Thursday, September 17, 2020 11:20 AM
> > To: qemu-devel@nongnu.org
> > Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
> > Chenzhendong (alex); shannon.zhaosl@gmail.com;
> > qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
> > imammedo@redhat.com
> > Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
> > topology support
> > 
> > An accurate cpu topology may help improve the cpu scheduler's
> > decision
> > making when dealing with multi-core system. So cpu topology
> > description
> > is helpful to provide guest with the right view. Cpu cache information
> > may
> > also have slight impact on the sched domain, and even userspace
> > software
> > may check the cpu cache information to do some optimizations. Thus
> > this patch
> > series is posted to provide cpu and cache topology support for arm.
> > 
> > To make the cpu topology consistent with MPIDR, an vcpu ioctl
> 
> For aarch64, the cpu topology don't depends on the MPDIR.
> See https://patchwork.kernel.org/patch/11744387/ 
>

The topology should not be inferred from the MPIDR Aff fields,
but MPIDR is the CPU identifier. When describing a topology
with ACPI or DT the CPU elements in the topology description
must map to actual CPUs. MPIDR is that mapping link. KVM
currently determines what the MPIDR of a VCPU is. If KVM
userspace is going to determine the VCPU topology, then it
also needs control over the MPIDR values, otherwise it
becomes quite messy trying to get the mapping right.

Thanks,
drew

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Ying Fang]

On 10/14/2020 2:08 AM, Andrew Jones wrote:
> On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
>> Cc valentin
>>
>>> -----Original Message-----
>>> From: Qemu-devel
>>> [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
>>> On Behalf Of Ying Fang
>>> Sent: Thursday, September 17, 2020 11:20 AM
>>> To: qemu-devel@nongnu.org
>>> Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
>>> Chenzhendong (alex); shannon.zhaosl@gmail.com;
>>> qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
>>> imammedo@redhat.com
>>> Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
>>> topology support
>>>
>>> An accurate cpu topology may help improve the cpu scheduler's
>>> decision
>>> making when dealing with multi-core system. So cpu topology
>>> description
>>> is helpful to provide guest with the right view. Cpu cache information
>>> may
>>> also have slight impact on the sched domain, and even userspace
>>> software
>>> may check the cpu cache information to do some optimizations. Thus
>>> this patch
>>> series is posted to provide cpu and cache topology support for arm.
>>>
>>> To make the cpu topology consistent with MPIDR, an vcpu ioctl
>>
>> For aarch64, the cpu topology don't depends on the MPDIR.
>> See https://patchwork.kernel.org/patch/11744387/
>>
> 
> The topology should not be inferred from the MPIDR Aff fields,

MPIDR is abused by ARM OEM manufactures. It is only used as a
identifer for a specific cpu, not representation of the topology.

> but MPIDR is the CPU identifier. When describing a topology
> with ACPI or DT the CPU elements in the topology description
> must map to actual CPUs. MPIDR is that mapping link. KVM
> currently determines what the MPIDR of a VCPU is. If KVM

KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
into affinity levels. See reset_mpidr in sys_regs.c

> userspace is going to determine the VCPU topology, then it
> also needs control over the MPIDR values, otherwise it
> becomes quite messy trying to get the mapping right.
If we are going to control MPIDR, shall we assign MPIDR with
vcpu_id or map topology hierarchy into affinity levels or any
other link schema ?

> 
> Thanks,
> drew
> 
> .
> 
Thanks Ying.

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Andrew Jones]

On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:
> 
> 
> On 10/14/2020 2:08 AM, Andrew Jones wrote:
> > On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
> > > Cc valentin
> > > 
> > > > -----Original Message-----
> > > > From: Qemu-devel
> > > > [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
> > > > On Behalf Of Ying Fang
> > > > Sent: Thursday, September 17, 2020 11:20 AM
> > > > To: qemu-devel@nongnu.org
> > > > Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
> > > > Chenzhendong (alex); shannon.zhaosl@gmail.com;
> > > > qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
> > > > imammedo@redhat.com
> > > > Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
> > > > topology support
> > > > 
> > > > An accurate cpu topology may help improve the cpu scheduler's
> > > > decision
> > > > making when dealing with multi-core system. So cpu topology
> > > > description
> > > > is helpful to provide guest with the right view. Cpu cache information
> > > > may
> > > > also have slight impact on the sched domain, and even userspace
> > > > software
> > > > may check the cpu cache information to do some optimizations. Thus
> > > > this patch
> > > > series is posted to provide cpu and cache topology support for arm.
> > > > 
> > > > To make the cpu topology consistent with MPIDR, an vcpu ioctl
> > > 
> > > For aarch64, the cpu topology don't depends on the MPDIR.
> > > See https://patchwork.kernel.org/patch/11744387/
> > > 
> > 
> > The topology should not be inferred from the MPIDR Aff fields,
> 
> MPIDR is abused by ARM OEM manufactures. It is only used as a
> identifer for a specific cpu, not representation of the topology.

Right, which is why I stated topology should not be inferred from
it.

> 
> > but MPIDR is the CPU identifier. When describing a topology
> > with ACPI or DT the CPU elements in the topology description
> > must map to actual CPUs. MPIDR is that mapping link. KVM
> > currently determines what the MPIDR of a VCPU is. If KVM
> 
> KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
> into affinity levels. See reset_mpidr in sys_regs.c

I know, but how KVM assigns MPIDRs today is not really important
to KVM userspace. KVM userspace shouldn't depend on a KVM
algorithm, as it could change.

> 
> > userspace is going to determine the VCPU topology, then it
> > also needs control over the MPIDR values, otherwise it
> > becomes quite messy trying to get the mapping right.
> If we are going to control MPIDR, shall we assign MPIDR with
> vcpu_id or map topology hierarchy into affinity levels or any
> other link schema ?
>

We can assign them to whatever we want, as long as they're
unique and as long as Aff0 is assigned per the GIC requirements,
e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
Aff3,Aff2,Aff1 fields should actually be peers with respect to
the GIC.

We shouldn't try to encode topology in the MPIDR in any way,
so we might as well simply increment a counter to assign them,
which could possibly be the same as the VCPU ID.

Thanks,
drew

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Ying Fang]

On 10/15/2020 3:59 PM, Andrew Jones wrote:
> On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:
>>
>>
>> On 10/14/2020 2:08 AM, Andrew Jones wrote:
>>> On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
>>>> Cc valentin
>>>>
>>>>> -----Original Message-----
>>>>> From: Qemu-devel
>>>>> [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
>>>>> On Behalf Of Ying Fang
>>>>> Sent: Thursday, September 17, 2020 11:20 AM
>>>>> To: qemu-devel@nongnu.org
>>>>> Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
>>>>> Chenzhendong (alex); shannon.zhaosl@gmail.com;
>>>>> qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
>>>>> imammedo@redhat.com
>>>>> Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
>>>>> topology support
>>>>>
>>>>> An accurate cpu topology may help improve the cpu scheduler's
>>>>> decision
>>>>> making when dealing with multi-core system. So cpu topology
>>>>> description
>>>>> is helpful to provide guest with the right view. Cpu cache information
>>>>> may
>>>>> also have slight impact on the sched domain, and even userspace
>>>>> software
>>>>> may check the cpu cache information to do some optimizations. Thus
>>>>> this patch
>>>>> series is posted to provide cpu and cache topology support for arm.
>>>>>
>>>>> To make the cpu topology consistent with MPIDR, an vcpu ioctl
>>>>
>>>> For aarch64, the cpu topology don't depends on the MPDIR.
>>>> See https://patchwork.kernel.org/patch/11744387/
>>>>
>>>
>>> The topology should not be inferred from the MPIDR Aff fields,
>>
>> MPIDR is abused by ARM OEM manufactures. It is only used as a
>> identifer for a specific cpu, not representation of the topology.
> 
> Right, which is why I stated topology should not be inferred from
> it.
> 
>>
>>> but MPIDR is the CPU identifier. When describing a topology
>>> with ACPI or DT the CPU elements in the topology description
>>> must map to actual CPUs. MPIDR is that mapping link. KVM
>>> currently determines what the MPIDR of a VCPU is. If KVM
>>
>> KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
>> into affinity levels. See reset_mpidr in sys_regs.c
> 
> I know, but how KVM assigns MPIDRs today is not really important
> to KVM userspace. KVM userspace shouldn't depend on a KVM
> algorithm, as it could change.
> 
>>
>>> userspace is going to determine the VCPU topology, then it
>>> also needs control over the MPIDR values, otherwise it
>>> becomes quite messy trying to get the mapping right.
>> If we are going to control MPIDR, shall we assign MPIDR with
>> vcpu_id or map topology hierarchy into affinity levels or any
>> other link schema ?
>>
> 
> We can assign them to whatever we want, as long as they're
> unique and as long as Aff0 is assigned per the GIC requirements,
> e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
> pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
> Aff3,Aff2,Aff1 fields should actually be peers with respect to
> the GIC.

Still not clear why vCPU's MPIDR need to match pPCPU's GIC affinity.
Maybe I should read spec for GICv3.

> 
> We shouldn't try to encode topology in the MPIDR in any way,
> so we might as well simply increment a counter to assign them,
> which could possibly be the same as the VCPU ID.

Hmm, then we can leave it as it is.

> 
> Thanks,
> drew
> 
> .
> 

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Andrew Jones]

On Fri, Oct 16, 2020 at 05:40:02PM +0800, Ying Fang wrote:
> 
> 
> On 10/15/2020 3:59 PM, Andrew Jones wrote:
> > On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:
> > > 
> > > 
> > > On 10/14/2020 2:08 AM, Andrew Jones wrote:
> > > > On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
> > > > > Cc valentin
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Qemu-devel
> > > > > > [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
> > > > > > On Behalf Of Ying Fang
> > > > > > Sent: Thursday, September 17, 2020 11:20 AM
> > > > > > To: qemu-devel@nongnu.org
> > > > > > Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
> > > > > > Chenzhendong (alex); shannon.zhaosl@gmail.com;
> > > > > > qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
> > > > > > imammedo@redhat.com
> > > > > > Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
> > > > > > topology support
> > > > > > 
> > > > > > An accurate cpu topology may help improve the cpu scheduler's
> > > > > > decision
> > > > > > making when dealing with multi-core system. So cpu topology
> > > > > > description
> > > > > > is helpful to provide guest with the right view. Cpu cache information
> > > > > > may
> > > > > > also have slight impact on the sched domain, and even userspace
> > > > > > software
> > > > > > may check the cpu cache information to do some optimizations. Thus
> > > > > > this patch
> > > > > > series is posted to provide cpu and cache topology support for arm.
> > > > > > 
> > > > > > To make the cpu topology consistent with MPIDR, an vcpu ioctl
> > > > > 
> > > > > For aarch64, the cpu topology don't depends on the MPDIR.
> > > > > See https://patchwork.kernel.org/patch/11744387/
> > > > > 
> > > > 
> > > > The topology should not be inferred from the MPIDR Aff fields,
> > > 
> > > MPIDR is abused by ARM OEM manufactures. It is only used as a
> > > identifer for a specific cpu, not representation of the topology.
> > 
> > Right, which is why I stated topology should not be inferred from
> > it.
> > 
> > > 
> > > > but MPIDR is the CPU identifier. When describing a topology
> > > > with ACPI or DT the CPU elements in the topology description
> > > > must map to actual CPUs. MPIDR is that mapping link. KVM
> > > > currently determines what the MPIDR of a VCPU is. If KVM
> > > 
> > > KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
> > > into affinity levels. See reset_mpidr in sys_regs.c
> > 
> > I know, but how KVM assigns MPIDRs today is not really important
> > to KVM userspace. KVM userspace shouldn't depend on a KVM
> > algorithm, as it could change.
> > 
> > > 
> > > > userspace is going to determine the VCPU topology, then it
> > > > also needs control over the MPIDR values, otherwise it
> > > > becomes quite messy trying to get the mapping right.
> > > If we are going to control MPIDR, shall we assign MPIDR with
> > > vcpu_id or map topology hierarchy into affinity levels or any
> > > other link schema ?
> > > 
> > 
> > We can assign them to whatever we want, as long as they're
> > unique and as long as Aff0 is assigned per the GIC requirements,
> > e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
> > pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
> > Aff3,Aff2,Aff1 fields should actually be peers with respect to
> > the GIC.
> 
> Still not clear why vCPU's MPIDR need to match pPCPU's GIC affinity.
> Maybe I should read spec for GICv3.

Look at how IPIs are efficiently sent to "peers", where the definition
of a peer is that only Aff0 differs in its MPIDR. But, gicv3's
optimizations can only handle 16 peers. If we want pinned VCPUs to
have the same performance as PCPUS, then we should maintain this
Aff0 limit.

Thanks,
drew

> 
> > 
> > We shouldn't try to encode topology in the MPIDR in any way,
> > so we might as well simply increment a counter to assign them,
> > which could possibly be the same as the VCPU ID.
> 
> Hmm, then we can leave it as it is.
> 
> > 
> > Thanks,
> > drew
> > 
> > .
> > 
> 

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Ying Fang]

On 10/16/2020 6:07 PM, Andrew Jones wrote:
> On Fri, Oct 16, 2020 at 05:40:02PM +0800, Ying Fang wrote:
>>
>>
>> On 10/15/2020 3:59 PM, Andrew Jones wrote:
>>> On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:
>>>>
>>>>
>>>> On 10/14/2020 2:08 AM, Andrew Jones wrote:
>>>>> On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
>>>>>> Cc valentin
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Qemu-devel
>>>>>>> [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
>>>>>>> On Behalf Of Ying Fang
>>>>>>> Sent: Thursday, September 17, 2020 11:20 AM
>>>>>>> To: qemu-devel@nongnu.org
>>>>>>> Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
>>>>>>> Chenzhendong (alex); shannon.zhaosl@gmail.com;
>>>>>>> qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
>>>>>>> imammedo@redhat.com
>>>>>>> Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
>>>>>>> topology support
>>>>>>>
>>>>>>> An accurate cpu topology may help improve the cpu scheduler's
>>>>>>> decision
>>>>>>> making when dealing with multi-core system. So cpu topology
>>>>>>> description
>>>>>>> is helpful to provide guest with the right view. Cpu cache information
>>>>>>> may
>>>>>>> also have slight impact on the sched domain, and even userspace
>>>>>>> software
>>>>>>> may check the cpu cache information to do some optimizations. Thus
>>>>>>> this patch
>>>>>>> series is posted to provide cpu and cache topology support for arm.
>>>>>>>
>>>>>>> To make the cpu topology consistent with MPIDR, an vcpu ioctl
>>>>>>
>>>>>> For aarch64, the cpu topology don't depends on the MPDIR.
>>>>>> See https://patchwork.kernel.org/patch/11744387/
>>>>>>
>>>>>
>>>>> The topology should not be inferred from the MPIDR Aff fields,
>>>>
>>>> MPIDR is abused by ARM OEM manufactures. It is only used as a
>>>> identifer for a specific cpu, not representation of the topology.
>>>
>>> Right, which is why I stated topology should not be inferred from
>>> it.
>>>
>>>>
>>>>> but MPIDR is the CPU identifier. When describing a topology
>>>>> with ACPI or DT the CPU elements in the topology description
>>>>> must map to actual CPUs. MPIDR is that mapping link. KVM
>>>>> currently determines what the MPIDR of a VCPU is. If KVM
>>>>
>>>> KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
>>>> into affinity levels. See reset_mpidr in sys_regs.c
>>>
>>> I know, but how KVM assigns MPIDRs today is not really important
>>> to KVM userspace. KVM userspace shouldn't depend on a KVM
>>> algorithm, as it could change.
>>>
>>>>
>>>>> userspace is going to determine the VCPU topology, then it
>>>>> also needs control over the MPIDR values, otherwise it
>>>>> becomes quite messy trying to get the mapping right.
>>>> If we are going to control MPIDR, shall we assign MPIDR with
>>>> vcpu_id or map topology hierarchy into affinity levels or any
>>>> other link schema ?
>>>>
>>>
>>> We can assign them to whatever we want, as long as they're
>>> unique and as long as Aff0 is assigned per the GIC requirements,
>>> e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
>>> pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
>>> Aff3,Aff2,Aff1 fields should actually be peers with respect to
>>> the GIC.
>>
>> Still not clear why vCPU's MPIDR need to match pPCPU's GIC affinity.
>> Maybe I should read spec for GICv3.
> 
> Look at how IPIs are efficiently sent to "peers", where the definition
> of a peer is that only Aff0 differs in its MPIDR. But, gicv3's
> optimizations can only handle 16 peers. If we want pinned VCPUs to
> have the same performance as PCPUS, then we should maintain this
> Aff0 limit.

Yes I see. I think *virt_cpu_mp_affinity* in qemu has limit
on the clustersz. It groups every 16 vCPUs into a cluster
and then mapped into the first two affinity levels.

Thanks.
Ying.

> 
> Thanks,
> drew
> 
>>
>>>
>>> We shouldn't try to encode topology in the MPIDR in any way,
>>> so we might as well simply increment a counter to assign them,
>>> which could possibly be the same as the VCPU ID.
>>
>> Hmm, then we can leave it as it is.
>>
>>>
>>> Thanks,
>>> drew
>>>
>>> .
>>>
>>
> 
> .
> 

Re: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache topology support

[Andrew Jones]

On Tue, Oct 20, 2020 at 10:52:11AM +0800, Ying Fang wrote:
> 
> 
> On 10/16/2020 6:07 PM, Andrew Jones wrote:
> > On Fri, Oct 16, 2020 at 05:40:02PM +0800, Ying Fang wrote:
> > > 
> > > 
> > > On 10/15/2020 3:59 PM, Andrew Jones wrote:
> > > > On Thu, Oct 15, 2020 at 10:07:16AM +0800, Ying Fang wrote:
> > > > > 
> > > > > 
> > > > > On 10/14/2020 2:08 AM, Andrew Jones wrote:
> > > > > > On Tue, Oct 13, 2020 at 12:11:20PM +0000, Zengtao (B) wrote:
> > > > > > > Cc valentin
> > > > > > > 
> > > > > > > > -----Original Message-----
> > > > > > > > From: Qemu-devel
> > > > > > > > [mailto:qemu-devel-bounces+prime.zeng=hisilicon.com@nongnu.org]
> > > > > > > > On Behalf Of Ying Fang
> > > > > > > > Sent: Thursday, September 17, 2020 11:20 AM
> > > > > > > > To: qemu-devel@nongnu.org
> > > > > > > > Cc: peter.maydell@linaro.org; drjones@redhat.com; Zhanghailiang;
> > > > > > > > Chenzhendong (alex); shannon.zhaosl@gmail.com;
> > > > > > > > qemu-arm@nongnu.org; alistair.francis@wdc.com; fangying;
> > > > > > > > imammedo@redhat.com
> > > > > > > > Subject: [RFC PATCH 00/12] hw/arm/virt: Introduce cpu and cache
> > > > > > > > topology support
> > > > > > > > 
> > > > > > > > An accurate cpu topology may help improve the cpu scheduler's
> > > > > > > > decision
> > > > > > > > making when dealing with multi-core system. So cpu topology
> > > > > > > > description
> > > > > > > > is helpful to provide guest with the right view. Cpu cache information
> > > > > > > > may
> > > > > > > > also have slight impact on the sched domain, and even userspace
> > > > > > > > software
> > > > > > > > may check the cpu cache information to do some optimizations. Thus
> > > > > > > > this patch
> > > > > > > > series is posted to provide cpu and cache topology support for arm.
> > > > > > > > 
> > > > > > > > To make the cpu topology consistent with MPIDR, an vcpu ioctl
> > > > > > > 
> > > > > > > For aarch64, the cpu topology don't depends on the MPDIR.
> > > > > > > See https://patchwork.kernel.org/patch/11744387/
> > > > > > > 
> > > > > > 
> > > > > > The topology should not be inferred from the MPIDR Aff fields,
> > > > > 
> > > > > MPIDR is abused by ARM OEM manufactures. It is only used as a
> > > > > identifer for a specific cpu, not representation of the topology.
> > > > 
> > > > Right, which is why I stated topology should not be inferred from
> > > > it.
> > > > 
> > > > > 
> > > > > > but MPIDR is the CPU identifier. When describing a topology
> > > > > > with ACPI or DT the CPU elements in the topology description
> > > > > > must map to actual CPUs. MPIDR is that mapping link. KVM
> > > > > > currently determines what the MPIDR of a VCPU is. If KVM
> > > > > 
> > > > > KVM currently assigns MPIDR with vcpu->vcpu_id which mapped
> > > > > into affinity levels. See reset_mpidr in sys_regs.c
> > > > 
> > > > I know, but how KVM assigns MPIDRs today is not really important
> > > > to KVM userspace. KVM userspace shouldn't depend on a KVM
> > > > algorithm, as it could change.
> > > > 
> > > > > 
> > > > > > userspace is going to determine the VCPU topology, then it
> > > > > > also needs control over the MPIDR values, otherwise it
> > > > > > becomes quite messy trying to get the mapping right.
> > > > > If we are going to control MPIDR, shall we assign MPIDR with
> > > > > vcpu_id or map topology hierarchy into affinity levels or any
> > > > > other link schema ?
> > > > > 
> > > > 
> > > > We can assign them to whatever we want, as long as they're
> > > > unique and as long as Aff0 is assigned per the GIC requirements,
> > > > e.g. GICv3 requires that Aff0 be from 0 to 0xf. Also, when
> > > > pinning VCPUs to PCPUs we should ensure that MPIDRs with matching
> > > > Aff3,Aff2,Aff1 fields should actually be peers with respect to
> > > > the GIC.
> > > 
> > > Still not clear why vCPU's MPIDR need to match pPCPU's GIC affinity.
> > > Maybe I should read spec for GICv3.
> > 
> > Look at how IPIs are efficiently sent to "peers", where the definition
> > of a peer is that only Aff0 differs in its MPIDR. But, gicv3's
> > optimizations can only handle 16 peers. If we want pinned VCPUs to
> > have the same performance as PCPUS, then we should maintain this
> > Aff0 limit.
> 
> Yes I see. I think *virt_cpu_mp_affinity* in qemu has limit
> on the clustersz. It groups every 16 vCPUs into a cluster
> and then mapped into the first two affinity levels.
>

Right, and it's probably sufficient to just switch to this function
for assigning affinity fields of MPIDRs for both TCG and KVM. Currently
it's only for TCG, as the comment in it explains, but it does the same
thing as KVM anyway. So, while nothing should change from the view of
the guest, userspace gains control over the MPIDRs, and that allows it
to build VCPU topologies more smoothly and in advance of VCPU creation.

Thanks,
drew