qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/1] spapr.c: remove 'ibm,chip-id' from DT


From: Daniel Henrique Barboza
Subject: Re: [PATCH 1/1] spapr.c: remove 'ibm,chip-id' from DT
Date: Thu, 11 Mar 2021 15:22:39 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0



On 3/11/21 1:29 PM, Greg Kurz wrote:
On Thu, 11 Mar 2021 12:15:57 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

The attribute 'ibm,chip-id' does not exist in PAPR. This alone would be
enough reason to remove it from the spapr DT, but before doing that,
let's give a brief context on how and why it was introduced.

'ibm,chip-id' was added in the spapr DT by commit 10582ff83279. This
commit references kernel commit 256f2d4b463d ("powerpc: Use ibm, chip-id
property to compute cpu_core_mask if available"). In this kernel commit,
the 'ibm,chip-id' DT attribute is being used to calculate the
cpu_core_mask in traverse_siblings_chip_id(). This function still need
to consider 'ibm,chip-id' not being available as well to avoid breaking
older guests.

Later on, kernel commit df52f6714071 ("powerpc/smp: Rework CPU topology
construction") removed traverse_siblings_chip_id() and its callers,
making the CPU topology calculation independent of the 'ibm,chip-id'
attribute. Today, the kernel uses 'ibm,chip-id' for PowerNV related code
only - the pseries kernel does not rely on it.


Thanks for the archaeology ! So this means that the pseries kernel used
to rely on it up to kernel v4.14, right ?


The pseries kernel up to 4.14 used to consider the existence of 'ibm,chip-id',
but it also had an error path for its absence as well.


All that said, since it's not in PAPR and the pseries kernel does not
rely on it, let's remove ibm,chip-id from the DT.


Even if it isn't part of PAPR, this is something that we've been
exposing to guests with existing machine types and someone could
have found a use for it or still using a pre-4.14 kernel, e.g.
debian stretch still relies on 4.9.

I understand that it's generally not cool to change guest visible information.

If we want to be on the safe, I can send a v2 where this change if effective 
only
on pseries-6.0.0 and newer.


Thanks,


DHB



In theory we should change this behavior in the default machine only.
But in case David is okay to merge the patch anyway,

Reviewed-by: Greg Kurz <groug@kaod.org>

CC: Alexey Kardashevskiy <aik@ozlabs.ru>
Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
  hw/ppc/spapr.c | 4 ----
  1 file changed, 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 85fe65f894..d2e448fd9c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -657,7 +657,6 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int 
offset,
      uint32_t page_sizes_prop[64];
      size_t page_sizes_prop_size;
      unsigned int smp_threads = ms->smp.threads;
-    uint32_t vcpus_per_socket = smp_threads * ms->smp.cores;
      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
      int compat_smt = MIN(smp_threads, ppc_compat_max_vthreads(cpu));
      SpaprDrc *drc;
@@ -744,9 +743,6 @@ static void spapr_dt_cpu(CPUState *cs, void *fdt, int 
offset,
spapr_dt_pa_features(spapr, cpu, fdt, offset); - _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
-                           cs->cpu_index / vcpus_per_socket)));
-
      _FDT((fdt_setprop(fdt, offset, "ibm,pft-size",
                        pft_size_prop, sizeof(pft_size_prop))));




reply via email to

[Prev in Thread] Current Thread [Next in Thread]