| Message ID | 20210311151557.705863-1-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] spapr.c: remove 'ibm,chip-id' from DT | expand |
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 ? > 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. 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)))); >
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)))); >> >
On Thu, Mar 11, 2021 at 03:22:39PM -0300, Daniel Henrique Barboza wrote: > > > 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. Yes, we should do that.
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))));
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. 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. 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(-)