[Qemu-devel] Re: [PATCH 2/3] NUMA: promoting NUMA topology to BIOS and pin guest memory

[Anthony Liguori <anthony@codemonkey.ws>]

Andre Przywara wrote:
> This patch pushes the parsed NUMA topology via the firmware 
> configuration interface to the BIOS and pins the guest memory (if 
> desired).
>
> Signed-off-by: Andre Przywara <andre.przywara@amd.com>
>
> # HG changeset patch
> # User Andre Przywara <andre.przywara@amd.com>
> # Date 1228992161 -3600
> # Node ID 0501b7490a00ef7a77e69f846d332f797162052a
> # Parent  394d02758aa4358be3bcd14f9d59efaf42e89328
> promoting NUMA topology to BIOS and pin guest memory
>   

Do you have a BIOS patch too?

> diff -r 394d02758aa4 -r 0501b7490a00 Makefile.target
> --- a/Makefile.target	Thu Dec 11 11:36:21 2008 +0100
> +++ b/Makefile.target	Thu Dec 11 11:42:41 2008 +0100
> @@ -600,6 +600,10 @@ endif
>  endif
>  ifdef CONFIG_CS4231A
>  SOUND_HW += cs4231a.o
> +endif
> +
> +ifdef CONFIG_NUMA
> +LIBS += -lnuma
>  endif

Should introduce NUMALIBS and set this from the configure as the rest of 
the library dependencies work.

>  
>  ifdef CONFIG_VNC_TLS
> diff -r 394d02758aa4 -r 0501b7490a00 configure
> --- a/configure	Thu Dec 11 11:36:21 2008 +0100
> +++ b/configure	Thu Dec 11 11:42:41 2008 +0100
> @@ -368,6 +368,8 @@ for opt do
>    ;;
>    --enable-mixemu) mixemu="yes"
>    ;;
> +  --disable-numa) numa="no"
> +  ;;
>    --disable-aio) aio="no"
>    ;;
>    --disable-blobs) blobs="no"
>   

Need to set numa="yes" as a default.

> diff -r 394d02758aa4 -r 0501b7490a00 hw/pc.c
> --- a/hw/pc.c	Thu Dec 11 11:36:21 2008 +0100
> +++ b/hw/pc.c	Thu Dec 11 11:42:41 2008 +0100
> @@ -436,6 +436,12 @@ static void bochs_bios_init(void)
>      fw_cfg = fw_cfg_init(BIOS_CFG_IOPORT, BIOS_CFG_IOPORT + 1, 0, 0);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_ID, 1);
>      fw_cfg_add_i64(fw_cfg, FW_CFG_RAM_SIZE, (uint64_t)ram_size);
> +    fw_cfg_add_i16(fw_cfg, FW_CFG_NUMA_NODES, numnumanodes);
> +
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_MEM, (uint8_t*)node_mem,
> +        sizeof(node_mem[0]) * numnumanodes);
> +    fw_cfg_add_bytes(fw_cfg, FW_CFG_NUMA_NODE_CPUS, (uint8_t*)node_to_cpus,
> +        sizeof(node_to_cpus[0]) * numnumanodes);
>  }

This stuff (the firmware awareness) should be independent of the libnuma 
support.

>  
>  /* Generate an initial boot sector which sets state and jump to
> diff -r 394d02758aa4 -r 0501b7490a00 vl.c
> --- a/vl.c	Thu Dec 11 11:36:21 2008 +0100
> +++ b/vl.c	Thu Dec 11 11:42:41 2008 +0100
> @@ -93,6 +93,9 @@
>  
>  #include <linux/ppdev.h>
>  #include <linux/parport.h>
> +#ifdef CONFIG_NUMA
> +#include <numa.h>
> +#endif
>  #endif
>  #ifdef __sun__
>  #include <sys/stat.h>
> @@ -5449,6 +5452,21 @@ int main(int argc, char **argv, char **e
>          exit(1);
>      }
>  
> +#ifdef CONFIG_NUMA
> +    if (numnumanodes > 0 && numa_available() != -1) {
> +        unsigned long offset = 0;
> +        int i;
> +
> +        for (i = 0; i < numnumanodes; ++i) {
> +            if (hostnodes[i] != (uint64_t)-1) {
> +                numa_tonode_memory (phys_ram_base + offset, node_mem[i],
> +                    hostnodes[i] % (numa_max_node() + 1));
> +            }
> +            offset += node_mem[i];
> +        }
> +    }
> +#endif
>   

At this point, I'm okay with introducing the libnuma dependency for 
memory pinning.  I'm not sure I think we should even present this as a 
command line option though.  I think the command line option should just 
specify the NUMA topology and then we should use a monitor command for 
pinning (as you do on your next patch).

I'd like to see some more documentation of this functionality.

Regards,

Anthony Liguori

>      /* init the dynamic translator */
>      cpu_exec_init_all(tb_size * 1024 * 1024);
>