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

[Andre Przywara]

Anthony Liguori wrote:
> 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 <address@hidden>
> >
> > # HG changeset patch
> > # User Andre Przywara <address@hidden>
> > # Date 1228992161 -3600
> > # Node ID 0501b7490a00ef7a77e69f846d332f797162052a
> > # Parent  394d02758aa4358be3bcd14f9d59efaf42e89328
> > promoting NUMA topology to BIOS and pin guest memory
> >   
>
> Do you have a BIOS patch too?
Actually I was waiting with this part as you said you wanted to sync the 
QEMU's BIOS with the upstream BOCHS one, which would made my patch a lot 
easier. I can prepare a patch based on the current version in QEMU, but 
that diff would include code which is already in upstream BOCHS, which 
would complicate the next merge.

> >  
> >  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.
Well, it seems there are two ways to do this in QEMU's configure:
1. (as in aio): Default to yes, optionally disable, if still set to yes 
compile check
2. (as in brlapi): no default, optionally set to "no", if empty string 
compile check and set to yes or no accordingly
Seems like I copied the wrong version ;-)

> > 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.
Moved to the first patch.

Thanks for the detailed review. I will address your other comments as well.

Regards,
Andre.

--
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 277-84917
----to satisfy European Law for business letters:
AMD Saxony Limited Liability Company & Co. KG,
Wilschdorfer Landstr. 101, 01109 Dresden, Germany
Register Court Dresden: HRA 4896, General Partner authorized
to represent: AMD Saxony LLC (Wilmington, Delaware, US)
General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy