Re: [libvirt] [PATCH] spapr: make default PHB optionnal

On 07/12/2017 04:25 PM, Andrea Bolognani wrote: > [libvir-list added to the loop] > > On Tue, 2017-07-04 at 10:47 +0200, Greg Kurz wrote: > > On Tue, 4 Jul 2017 17:29:01 +1000 David Gibson <david(a)gibson.dropbear.id.au&gt; wrote: > > > On Mon, Jul 03, 2017 at 06:48:25PM +0200, Greg Kurz wrote: > > > > The sPAPR machine always create a default PHB during initialization, even > > > > if -nodefaults was passed on the command line. This forces the user to > > > > rely on -global if she wants to set properties of the default PHB, such > > > > as numa_node. > > > > This patch introduces a new machine create-default-phb property to control > > > > whether the default PHB must be created or not. It defaults to on in order > > > > to preserve old setups (which is also the motivation to not alter the > > > > current behavior of -nodefaults). > > > > If create-default-phb is set to off, the default PHB isn't created, nor > > > > any other device usually created with it. It is mandatory to provide > > > > a PHB on the command line to be able to use PCI devices (otherwise QEMU > > > > won't start). For example, the following creates a PHB with the same > > > > mappings as the default PHB and also sets the NUMA affinity: > > > > -machine type=pseries,create-default-phb=off \ > > > > -numa node,nodeid=0 -device spapr-pci-host-bridge,index=0,numa_node=0 > > > So, I agree that the distinction between default devices that are > > > disabled with -nodefaults and default devices that aren't is a big > > > mess in qemu configuration. But on the other hand this only addresses > > > one tiny aspect of that, and in the meantime means we will silently > > > ignore some other configuration options in some conditions. > > > So, what's the immediate benefit / use case for this? Setting numa_node for emulated devices is the benefit for now. On x86, I figured there is no way to set the numa_node for the root controller and the emulated devices sitting there all have numa_node set to -1. Only the devices on the pxb can have a sensible value specified.

Does it mean, the emulated devices/drivers don't care about the numa_node they are on?

Would it be fine on PPC to disallow setting the NUMA node for the default PHB because that is where all the emulated devices sit ? > > With the current code base, the only way to set properties of the default > > PHB, is to pass -global spapr-pci-host-bridge.prop=value for each property. > > The immediate benefit of this patch is to unify the way libvirt passes > > PHB description to the command line: > > ie, do: > > -machine type=pseries,create-default-phb=off \ > > -device spapr-pci-host-bridge,prop1=a,prop2=b,prop3=c \ > > -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f > > instead of: > > -machine type=pseries \ > > -global spapr-pci-host-bridge.prop1=a \ > > -global spapr-pci-host-bridge.prop2=b \ > > -global spapr-pci-host-bridge.prop3=c \ > > -device spapr-pci-host-bridge,prop1=d,prop2=e,prop3=f > So, I'm thinking about this mostly in terms of NUMA nodes > because that's the use case I'm aware of. > > The problem with using -global is not that it requires using > a different syntax to set properties for the default PHB, > but rather that such properties are then inherited by all > other PHBs unless explicitly overridden. Not creating the > default PHB at all would solve the issue. > > On the other hand, libvirt would then need to either > > 1) only allow setting NUMA nodes for PHBs if QEMU supports > the new option, leaving QEMU < 2.10 users behind; or > > 2) implement handling for both the new and old behavior. > > I'm not sure we could get away with 1), and going for 2) > means more work both for QEMU and libvirt developers for > very little actual gain, so I'd be inclined to scrap this > and just build the libvirt glue on top of the existing > interface. > > That is, of course, unless > > 1) having a random selection of PHBs not assigned to any > NUMA node is a sensible use case. This is something > we just can't do reliably with the current interface: > we can decide to set the NUMA node only for say, PHBs > 1 and 3 leaving 0 and 2 alone, but once we set it for > the default PHB we *have* to set it for all remaining > ones as well. libvirt will by default assign emulated > devices to the default PHB, so I would rather expect > users to leave that one alone and set a NUMA node for > all other PHBs; or > > 2) there are other properties outside of numa_node we > might want to deal with; or > > 3) it turns out it's okay to require a recent QEMU :) >