On Wed, 23 Sep 2020 16:34:56 -0300
Daniel Henrique Barboza <danielhb...@gmail.com> wrote:
> This is the first guest visible change introduced in
> spapr_numa.c. The previous settings of both reference-points
> and maxdomains were too restrictive, but enough for the
> existing associativity we're setting in the resources.
>
> We'll change that in the following patches, populating the
> associativity arrays based on user input. For those changes
> to be effective, reference-points and maxdomains must be
> more flexible. After this patch, we'll have 4 distinct
> levels of NUMA (0x4, 0x3, 0x2, 0x1) and maxdomains will
> allow for any type of configuration the user intends to
> do - under the scope and limitations of PAPR itself, of
> course.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
> ---
> hw/ppc/spapr_numa.c | 27 ++++++++++++++++++---------
> 1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 180800b2f3..688391278e 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -222,21 +222,30 @@ int
> spapr_numa_write_assoc_lookup_arrays(SpaprMachineState *spapr, void *fdt,
> */
> void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int rtas)
> {
> + MachineState *ms = MACHINE(spapr);
> SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
> uint32_t refpoints[] = {
> cpu_to_be32(0x4),
> - cpu_to_be32(0x4),
> + cpu_to_be32(0x3),
> cpu_to_be32(0x2),
> + cpu_to_be32(0x1),
> };
> uint32_t nr_refpoints = ARRAY_SIZE(refpoints);
> - uint32_t maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
> - uint32_t maxdomains[] = {
> - cpu_to_be32(4),
> - maxdomain,
> - maxdomain,
> - maxdomain,
> - cpu_to_be32(spapr->gpu_numa_id),
> - };
> + uint32_t maxdomain = cpu_to_be32(ms->numa_state->num_nodes +
> + spapr->gpu_numa_id);
> + uint32_t maxdomains[] = {0x4, maxdomain, maxdomain, maxdomain,
> maxdomain};
> +
It seems maxdomains[0] should be cpu_to_be32(0x4) and spaces are missing.
Maybe keep the previous multi-line declaration style ? This seems to produce
a nicer diff for the reviewer:
uint32_t maxdomains[] = {
cpu_to_be32(4),
maxdomain,
maxdomain,
maxdomain,
- cpu_to_be32(spapr->gpu_numa_id),
+ maxdomain,
};
> + if (spapr_machine_using_legacy_numa(spapr)) {
> + refpoints[1] = cpu_to_be32(0x4);
> + refpoints[2] = cpu_to_be32(0x2);
I'd rather have an explicit view of the legacy layouts for clarity...
> + nr_refpoints = 3;
> +
> + maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
> + maxdomains[1] = maxdomain;
> + maxdomains[2] = maxdomain;
> + maxdomains[3] = maxdomain;
> + maxdomains[4] = cpu_to_be32(spapr->gpu_numa_id);
... and here.
eg.
if (spapr_machine_using_legacy_numa(spapr)) {
uint32_t legacy_refpoints[] = {
cpu_to_be32(0x4),
cpu_to_be32(0x4),
cpu_to_be32(0x2),
};
uint32_t legacy_maxdomain = cpu_to_be32(spapr->gpu_numa_id > 1 ? 1 : 0);
uint32_t legacy_maxdomains[] = {
cpu_to_be32(4),
legacy_maxdomain,
legacy_maxdomain,
legacy_maxdomain,
cpu_to_be32(spapr->gpu_numa_id),
};
nr_refpoints = 3;
memcpy(refpoints, legacy_refpoints, sizeof(legacy_refpoints));
memcpy(maxdomains, legacy_maxdomains, sizeof(legacy_maxdomains));
}
This allows to instantly see how things are expected to appear
in the FDT, without having to mentally patch the refpoints[] and
maxdomains[] arrays. This also makes the diff easier to review.
> + }
>
> if (smc->pre_5_1_assoc_refpoints) {
> nr_refpoints = 2;
