[PATCH 0/2 qemu] hw/acpi/hmat: Misc fixes

[PATCH 0/2 qemu] hw/acpi/hmat: Misc fixes

[Jonathan Cameron via]

Two unrelated fixes here:
1) Linux really doesn't like it when you claim non existent memory
   is directly connected to an initiator (here a CPU).
   It is a nonsense entry, though I also plan to try and get
   a relaxation of the condition into the kernel.
   Maybe we need to care about migration, but I suspect no one
   cares about this corner case (hence no one noticed the
   problem!)

2) An access outside of the allocated array when building the
   the latency and bandwidth tables.  Given this crashes QEMU
   for me, I think we are fine with the potential table change.

Some notes on 1:
- This structure is almost entirely pointless in general - most
  of the fields were removed in HMAT v2.
  What remains, is meant to convey memory controller location
  when the memory is in a different Proximity Domain from the
  memory controller (e.g. a SoC with both HBM and DDR will present
  2 NUMA domains but memory controllers will be wherever we describe
  the CPUs as being - typically with the DDR)
  Currently QEMU creates these to indicate direct connection between
  a CPU domain and memory in the same domain. Using the Proximity
  domain in SRAT conveys the same. This adds no information.

Notes on 2:
- I debated a follow up patch removing the entires in the table
  for initiators on nodes that don't have any initiators.
  QEMU won't let you use them as initiators in the LB entries
  anyway so there is no way to set those entries and they
  end up reported as 0. OK for Bandwidth as no one is going to use
  the zero bandwidth channel, but that's a very attractive latency,
  but that's fine as no one will read the number as there are
  no initiators? (right?)

  There is a corner case in ACPI that bites us here.
  ACPI Proximity domains are only defined in SRAT, but nothing says
  they need to be fully defined.  Generic Initiators are optional
  afterall (newish feature) so it was common to use _PXM in DSDT
  to define where various platform devices were (and PCI but that's
  still not read by Linux - a story of pain and broken systems for
  another day). That's fine if they are in a node with CPUs
  (initiators) but not so much if they   happen to be in a memory
  only node. Today I think the only thing we can make hit this
  condition in QEMU is a PCI Expander Bridge which doesn't initiate
  transactions. But things behind it do and there are drivers out
  there that do buffer placement based on SLIT distances. I'd
  expect HMAT users to follow soon.

  It would be nice to think all such systems will use Generic Port
  Affinity Structures (and I have patches for those to follow shortly)
  but that's overly optimistic beyond CXL where the kernel will use
  them and which drove their introduction.

Jonathan Cameron (2):
  hmat acpi: Do not add Memory Proximity Domain Attributes Structure
    targetting non existent memory.
  hmat acpi: Fix out of bounds access due to missing use of indirection

 hw/acpi/hmat.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/2] hmat acpi: Do not add Memory Proximity Domain Attributes Structure targetting non existent memory.

[Jonathan Cameron via]

If qemu is started with a proximity node containing CPUs alone,
it will provide one of these structures to say memory in this
node is directly connected to itself.

This description is arguably pointless even if there is memory
in the node.  If there is no memory present, and hence no SRAT
entry it breaks Linux HMAT passing and the table is rejected.

https://elixir.bootlin.com/linux/latest/source/drivers/acpi/numa/hmat.c#L444

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/acpi/hmat.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 3042d223c8..723ae28d32 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -204,6 +204,13 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
     build_append_int_noprefix(table_data, 0, 4); /* Reserved */
 
     for (i = 0; i < numa_state->num_nodes; i++) {
+        /*
+         * Linux rejects whole HMAT table if a node with no memory
+         * has one of these structures listing it as a target.
+         */
+        if (!numa_state->nodes[i].node_mem) {
+            continue;
+        }
         flags = 0;
 
         if (numa_state->nodes[i].initiator < MAX_NODES) {
-- 
2.39.2

Re: [PATCH 1/2] hmat acpi: Do not add Memory Proximity Domain Attributes Structure targetting non existent memory.

[Gregory Price]

On Thu, Feb 29, 2024 at 04:25:44PM +0000, Jonathan Cameron wrote:
> If qemu is started with a proximity node containing CPUs alone,
> it will provide one of these structures to say memory in this
> node is directly connected to itself.
> 
> This description is arguably pointless even if there is memory
> in the node.  If there is no memory present, and hence no SRAT
> entry it breaks Linux HMAT passing and the table is rejected.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/acpi/numa/hmat.c#L444
> 

Nit: This link becomes out of date pretty much immediately, consider
using a versioned link.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  hw/acpi/hmat.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> index 3042d223c8..723ae28d32 100644
> --- a/hw/acpi/hmat.c
> +++ b/hw/acpi/hmat.c
> @@ -204,6 +204,13 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
>      build_append_int_noprefix(table_data, 0, 4); /* Reserved */
>  
>      for (i = 0; i < numa_state->num_nodes; i++) {
> +        /*
> +         * Linux rejects whole HMAT table if a node with no memory
> +         * has one of these structures listing it as a target.
> +         */
> +        if (!numa_state->nodes[i].node_mem) {
> +            continue;
> +        }
>          flags = 0;
>  
>          if (numa_state->nodes[i].initiator < MAX_NODES) {
> -- 
> 2.39.2
> 
> 

Re: [PATCH 1/2] hmat acpi: Do not add Memory Proximity Domain Attributes Structure targetting non existent memory.

[Jonathan Cameron via]

On Thu, 29 Feb 2024 12:28:51 -0500
Gregory Price <gregory.price@memverge.com> wrote:

> On Thu, Feb 29, 2024 at 04:25:44PM +0000, Jonathan Cameron wrote:
> > If qemu is started with a proximity node containing CPUs alone,
> > it will provide one of these structures to say memory in this
> > node is directly connected to itself.
> > 
> > This description is arguably pointless even if there is memory
> > in the node.  If there is no memory present, and hence no SRAT
> > entry it breaks Linux HMAT passing and the table is rejected.
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/acpi/numa/hmat.c#L444
> >   
> 
> Nit: This link becomes out of date pretty much immediately, consider
> using a versioned link.
Good point.

https://elixir.bootlin.com/linux/v6.7/source/drivers/acpi/numa/hmat.c#L444

> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  hw/acpi/hmat.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
> > index 3042d223c8..723ae28d32 100644
> > --- a/hw/acpi/hmat.c
> > +++ b/hw/acpi/hmat.c
> > @@ -204,6 +204,13 @@ static void hmat_build_table_structs(GArray *table_data, NumaState *numa_state)
> >      build_append_int_noprefix(table_data, 0, 4); /* Reserved */
> >  
> >      for (i = 0; i < numa_state->num_nodes; i++) {
> > +        /*
> > +         * Linux rejects whole HMAT table if a node with no memory
> > +         * has one of these structures listing it as a target.
> > +         */
> > +        if (!numa_state->nodes[i].node_mem) {
> > +            continue;
> > +        }
> >          flags = 0;
> >  
> >          if (numa_state->nodes[i].initiator < MAX_NODES) {
> > -- 
> > 2.39.2
> > 
> >   

[PATCH 2/2] hmat acpi: Fix out of bounds access due to missing use of indirection

[Jonathan Cameron via]

With a numa set up such as

-numa nodeid=0,cpus=0 \
-numa nodeid=1,memdev=mem \
-numa nodeid=2,cpus=1

and appropriate hmat_lb entries the initiator list is correctly
computed and writen to HMAT as 0,2 but then the LB data is accessed
using the node id (here 2), landing outside the entry_list array.

Stash the reverse lookup when writing the initiator list and use
it to get the correct array index index.

Fixes: 4586a2cb83 ("hmat acpi: Build System Locality Latency and Bandwidth Information Structure(s)")
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 hw/acpi/hmat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 723ae28d32..b933ae3c06 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -78,6 +78,7 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
                           uint32_t *initiator_list)
 {
     int i, index;
+    uint32_t initiator_to_index[MAX_NODES] = {};
     HMAT_LB_Data *lb_data;
     uint16_t *entry_list;
     uint32_t base;
@@ -121,6 +122,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
     /* Initiator Proximity Domain List */
     for (i = 0; i < num_initiator; i++) {
         build_append_int_noprefix(table_data, initiator_list[i], 4);
+        /* Reverse mapping for array possitions */
+        initiator_to_index[initiator_list[i]] = i;
     }
 
     /* Target Proximity Domain List */
@@ -132,7 +135,8 @@ static void build_hmat_lb(GArray *table_data, HMAT_LB_Info *hmat_lb,
     entry_list = g_new0(uint16_t, num_initiator * num_target);
     for (i = 0; i < hmat_lb->list->len; i++) {
         lb_data = &g_array_index(hmat_lb->list, HMAT_LB_Data, i);
-        index = lb_data->initiator * num_target + lb_data->target;
+        index = initiator_to_index[lb_data->initiator] * num_target +
+            lb_data->target;
 
         entry_list[index] = (uint16_t)(lb_data->data / hmat_lb->base);
     }
-- 
2.39.2