| Message ID | 1368059472-25071-7-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > Currently pci_get_primary_bus() searches the list of root buses for one > with domain 0. But since host buses are always registered with domain 0, > this just amounts to finding the only PCI host bus. > > This simplifies the implementation by defining the primary PCI bus to > be the first one registered, using a global variable to track it. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> This is the only part that I dislike. How about an explicit API to set the primary bus? Let machine types set it. > --- > hw/pci/pci.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index a3c192c..b25a1a1 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -96,6 +96,7 @@ struct PCIHostBus { > QLIST_ENTRY(PCIHostBus) next; > }; > static QLIST_HEAD(, PCIHostBus) host_buses; > +static PCIBus *pci_primary_bus; > > static const VMStateDescription vmstate_pcibus = { > .name = "PCIBUS", > @@ -241,6 +242,12 @@ static int pcibus_reset(BusState *qbus) > static void pci_host_bus_register(int domain, PCIBus *bus) > { > struct PCIHostBus *host; > + > + /* If this is the first one, assume it's the primary bus */ > + if (!pci_primary_bus) { > + pci_primary_bus = bus; > + } > + > host = g_malloc0(sizeof(*host)); > host->domain = domain; > host->bus = bus; > @@ -249,15 +256,7 @@ static void pci_host_bus_register(int domain, PCIBus *bus) > > PCIBus *pci_get_primary_bus(void) > { > - struct PCIHostBus *host; > - > - QLIST_FOREACH(host, &host_buses, next) { > - if (host->domain == 0) { > - return host->bus; > - } > - } > - > - return NULL; > + return pci_primary_bus; > } > > PCIBus *pci_device_root_bus(const PCIDevice *d) > @@ -300,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent, > > /* host bridge */ > QLIST_INIT(&bus->child); > + > pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */ > > vmstate_register(NULL, -1, &vmstate_pcibus, bus); > -- > 1.7.10.4
On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > Currently pci_get_primary_bus() searches the list of root buses for one > with domain 0. But since host buses are always registered with domain 0, > this just amounts to finding the only PCI host bus. > > This simplifies the implementation by defining the primary PCI bus to > be the first one registered, using a global variable to track it. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Or better: can we just fail if there is more than one root? > --- > hw/pci/pci.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index a3c192c..b25a1a1 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -96,6 +96,7 @@ struct PCIHostBus { > QLIST_ENTRY(PCIHostBus) next; > }; > static QLIST_HEAD(, PCIHostBus) host_buses; > +static PCIBus *pci_primary_bus; > > static const VMStateDescription vmstate_pcibus = { > .name = "PCIBUS", > @@ -241,6 +242,12 @@ static int pcibus_reset(BusState *qbus) > static void pci_host_bus_register(int domain, PCIBus *bus) > { > struct PCIHostBus *host; > + > + /* If this is the first one, assume it's the primary bus */ > + if (!pci_primary_bus) { > + pci_primary_bus = bus; > + } > + > host = g_malloc0(sizeof(*host)); > host->domain = domain; > host->bus = bus; > @@ -249,15 +256,7 @@ static void pci_host_bus_register(int domain, PCIBus *bus) > > PCIBus *pci_get_primary_bus(void) > { > - struct PCIHostBus *host; > - > - QLIST_FOREACH(host, &host_buses, next) { > - if (host->domain == 0) { > - return host->bus; > - } > - } > - > - return NULL; > + return pci_primary_bus; > } > > PCIBus *pci_device_root_bus(const PCIDevice *d) > @@ -300,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent, > > /* host bridge */ > QLIST_INIT(&bus->child); > + > pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */ > > vmstate_register(NULL, -1, &vmstate_pcibus, bus); > -- > 1.7.10.4
On Thu, May 23, 2013 at 02:01:57PM +0300, Michael S. Tsirkin wrote: > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > Currently pci_get_primary_bus() searches the list of root buses for one > > with domain 0. But since host buses are always registered with domain 0, > > this just amounts to finding the only PCI host bus. > > > > This simplifies the implementation by defining the primary PCI bus to > > be the first one registered, using a global variable to track it. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > This is the only part that I dislike. > How about an explicit API to set the primary bus? > Let machine types set it. I guess, though I was hoping to avoid changing every bit of platform code that sets up a PCI bus.
On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > Currently pci_get_primary_bus() searches the list of root buses for one > > with domain 0. But since host buses are always registered with domain 0, > > this just amounts to finding the only PCI host bus. > > > > This simplifies the implementation by defining the primary PCI bus to > > be the first one registered, using a global variable to track it. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > Or better: can we just fail if there is more than > one root? That might work, I'll look into doing that.
On Thu, May 23, 2013 at 10:16:13PM +1000, David Gibson wrote: > On Thu, May 23, 2013 at 02:01:57PM +0300, Michael S. Tsirkin wrote: > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > with domain 0. But since host buses are always registered with domain 0, > > > this just amounts to finding the only PCI host bus. > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > be the first one registered, using a global variable to track it. > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > This is the only part that I dislike. > > How about an explicit API to set the primary bus? > > Let machine types set it. > > I guess, though I was hoping to avoid changing every bit of platform > code that sets up a PCI bus. Yes, that's a lot of churn. Maybe we don't need a primary bus at all? If there's one root, it's simple: check it's the only one and return. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > with domain 0. But since host buses are always registered with domain 0, > > > this just amounts to finding the only PCI host bus. > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > be the first one registered, using a global variable to track it. > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > Or better: can we just fail if there is more than > > one root? > > That might work, I'll look into doing that. So, the difficulty with this is that then any machine with multiple PCI bridges could not use pci_nic_init(), since it calls pci_get_bus_devfn() which calls pci_find_primary_bus() which would always fail. And using pci_nic_init() is more or less mandatory in the machine_init function to support old-style nic configuration. Suggestions?
On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > with domain 0. But since host buses are always registered with domain 0, > > > > this just amounts to finding the only PCI host bus. > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > be the first one registered, using a global variable to track it. > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > Or better: can we just fail if there is more than > > > one root? > > > > That might work, I'll look into doing that. > > So, the difficulty with this is that then any machine with multiple > PCI bridges could not use pci_nic_init(), since it calls > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > always fail. And using pci_nic_init() is more or less mandatory in > the machine_init function to support old-style nic configuration. > > Suggestions? Oh, fwiw, my latest work-in-progress can be had at git://github.com/dgibson/qemu.git ('pci' branch) This is the only comment remaining to be addressed from the last round, so I'm hoping we can come to some consensus here and I'll repost.
On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > with domain 0. But since host buses are always registered with domain 0, > > > > this just amounts to finding the only PCI host bus. > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > be the first one registered, using a global variable to track it. > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > Or better: can we just fail if there is more than > > > one root? > > > > That might work, I'll look into doing that. > > So, the difficulty with this is that then any machine with multiple > PCI bridges could not use pci_nic_init(), since it calls > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > always fail. And using pci_nic_init() is more or less mandatory in > the machine_init function to support old-style nic configuration. > > Suggestions? You mean multiple PCI roots? Well, there are no legacy machines with multiple roots to support, are there? So why do we need to support legacy flags for these new configurations? > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > Or better: can we just fail if there is more than > > > > one root? > > > > > > That might work, I'll look into doing that. > > > > So, the difficulty with this is that then any machine with multiple > > PCI bridges could not use pci_nic_init(), since it calls > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > always fail. And using pci_nic_init() is more or less mandatory in > > the machine_init function to support old-style nic configuration. > > > > Suggestions? > > You mean multiple PCI roots? > Well, there are no legacy machines with multiple roots to support, are > there? So why do we need to support legacy flags for these new > configurations? Because people expect them. Plus on spapr we already support the legacy nic options; it would be very strange for them to suddenly break when we add a second host bridge.
On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > Or better: can we just fail if there is more than > > > > > one root? > > > > > > > > That might work, I'll look into doing that. > > > > > > So, the difficulty with this is that then any machine with multiple > > > PCI bridges could not use pci_nic_init(), since it calls > > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > > always fail. And using pci_nic_init() is more or less mandatory in > > > the machine_init function to support old-style nic configuration. > > > > > > Suggestions? > > > > You mean multiple PCI roots? > > Well, there are no legacy machines with multiple roots to support, are > > there? So why do we need to support legacy flags for these new > > configurations? > > Because people expect them. People can learn, somehow they will learn to add a new root, so they can learn to use -device too. So let's make it fail on multiple roots, and output a message along the lines of "please use -device virtio-net-pci instead". > Plus on spapr we already support the > legacy nic options; it would be very strange for them to suddenly > break when we add a second host bridge. Not sure who "we" is here. IMHO user should ask for a new machine type with two roots explicitly. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > Or better: can we just fail if there is more than > > > > > > one root? > > > > > > > > > > That might work, I'll look into doing that. > > > > > > > > So, the difficulty with this is that then any machine with multiple > > > > PCI bridges could not use pci_nic_init(), since it calls > > > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > > > always fail. And using pci_nic_init() is more or less mandatory in > > > > the machine_init function to support old-style nic configuration. > > > > > > > > Suggestions? > > > > > > You mean multiple PCI roots? > > > Well, there are no legacy machines with multiple roots to support, are > > > there? So why do we need to support legacy flags for these new > > > configurations? > > > > Because people expect them. > > People can learn, somehow they will learn to add a new root, so they can > learn to use -device too. Hrm. I'd kind of like a second (third?) opinion on that. Anthony? > So let's make it fail on multiple roots, and output a message along the > lines of "please use -device virtio-net-pci instead". How to produce a meaningful error like that isn't totally obvious, since the test for multiple roots is down in find_primary_pci_bus() (or whatever), and once we get back up to pci_nic_init() we just know that pci_get_bus_devfn() failed for some reason. > > Plus on spapr we already support the > > legacy nic options; it would be very strange for them to suddenly > > break when we add a second host bridge. > > Not sure who "we" is here. IMHO user should ask for a new > machine type with two roots explicitly. You seem to be thinking of the number of host bridges as a fixed property of the platform, which it isn't on spapr. PCI host bridges are just another device. Large scale real hardware can easily have dozens of them. In qemu we create one always as a convenience, but users can (and will have to, for vfio) create additional ones trivially with -device. [Which raises another complication as a tangent. People (and libvirt) don't generally expect -nodefaults to remove the PCI bridge, but arguably it should on spapr, since a PAPR guest with no PCI is perfectly viable but there's currently no way to specify such a thing.]
On Wed, May 29, 2013 at 09:04:00PM +1000, David Gibson wrote: > On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > > > Or better: can we just fail if there is more than > > > > > > > one root? > > > > > > > > > > > > That might work, I'll look into doing that. > > > > > > > > > > So, the difficulty with this is that then any machine with multiple > > > > > PCI bridges could not use pci_nic_init(), since it calls > > > > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > > > > always fail. And using pci_nic_init() is more or less mandatory in > > > > > the machine_init function to support old-style nic configuration. > > > > > > > > > > Suggestions? > > > > > > > > You mean multiple PCI roots? > > > > Well, there are no legacy machines with multiple roots to support, are > > > > there? So why do we need to support legacy flags for these new > > > > configurations? > > > > > > Because people expect them. > > > > People can learn, somehow they will learn to add a new root, so they can > > learn to use -device too. > > Hrm. I'd kind of like a second (third?) opinion on that. Anthony? > > > So let's make it fail on multiple roots, and output a message along the > > lines of "please use -device virtio-net-pci instead". > > How to produce a meaningful error like that isn't totally obvious, > since the test for multiple roots is down in find_primary_pci_bus() > (or whatever), and once we get back up to pci_nic_init() we just know > that pci_get_bus_devfn() failed for some reason. What other possible reason for it to fail? > > > Plus on spapr we already support the > > > legacy nic options; it would be very strange for them to suddenly > > > break when we add a second host bridge. > > > > Not sure who "we" is here. IMHO user should ask for a new > > machine type with two roots explicitly. > > You seem to be thinking of the number of host bridges as a fixed > property of the platform, which it isn't on spapr. PCI host bridges > are just another device. Large scale real hardware can easily have > dozens of them. Absolutely. I'm not thinking of it as fixed. I'm thinking of the *default* number of pci host bridges as fixed. If a user is smart enough to use -device to create a host bridge, said user can learn about -device for creating a nic. > In qemu we create one always as a convenience, but > users can (and will have to, for vfio) create additional ones > trivially with -device. So they know about -device then. > [Which raises another complication as a tangent. People (and libvirt) > don't generally expect -nodefaults to remove the PCI bridge, but > arguably it should on spapr, since a PAPR guest with no PCI is > perfectly viable but there's currently no way to specify such a > thing.] I guess the problem is not what they expect generally, but specifically that some users might rely on spapr with -nodefaults having PCI? I don't have any ideas besides introducing a new machine type that is same as spapr but without the default PCI host bridge. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Wed, May 29, 2013 at 03:22:29PM +0300, Michael S. Tsirkin wrote: > On Wed, May 29, 2013 at 09:04:00PM +1000, David Gibson wrote: > > On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > > > > > Or better: can we just fail if there is more than > > > > > > > > one root? > > > > > > > > > > > > > > That might work, I'll look into doing that. > > > > > > > > > > > > So, the difficulty with this is that then any machine with multiple > > > > > > PCI bridges could not use pci_nic_init(), since it calls > > > > > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > > > > > always fail. And using pci_nic_init() is more or less mandatory in > > > > > > the machine_init function to support old-style nic configuration. > > > > > > > > > > > > Suggestions? > > > > > > > > > > You mean multiple PCI roots? > > > > > Well, there are no legacy machines with multiple roots to support, are > > > > > there? So why do we need to support legacy flags for these new > > > > > configurations? > > > > > > > > Because people expect them. > > > > > > People can learn, somehow they will learn to add a new root, so they can > > > learn to use -device too. > > > > Hrm. I'd kind of like a second (third?) opinion on that. Anthony? > > > > > So let's make it fail on multiple roots, and output a message along the > > > lines of "please use -device virtio-net-pci instead". > > > > How to produce a meaningful error like that isn't totally obvious, > > since the test for multiple roots is down in find_primary_pci_bus() > > (or whatever), and once we get back up to pci_nic_init() we just know > > that pci_get_bus_devfn() failed for some reason. > > What other possible reason for it to fail? Unparseable address (it can be user specified) or internal failure to initialize the device are the first two that spring to mind.. > > > > Plus on spapr we already support the > > > > legacy nic options; it would be very strange for them to suddenly > > > > break when we add a second host bridge. > > > > > > Not sure who "we" is here. IMHO user should ask for a new > > > machine type with two roots explicitly. > > > > You seem to be thinking of the number of host bridges as a fixed > > property of the platform, which it isn't on spapr. PCI host bridges > > are just another device. Large scale real hardware can easily have > > dozens of them. > > Absolutely. I'm not thinking of it as fixed. > I'm thinking of the *default* number of pci host bridges > as fixed. If a user is smart enough to use -device to create > a host bridge, said user can learn about -device for creating > a nic. Hm, I guess. I'm still uncomfortable with breaking a documented option, even if its not the preferred method these days. > > In qemu we create one always as a convenience, but > > users can (and will have to, for vfio) create additional ones > > trivially with -device. > > So they know about -device then. > > > [Which raises another complication as a tangent. People (and libvirt) > > don't generally expect -nodefaults to remove the PCI bridge, but > > arguably it should on spapr, since a PAPR guest with no PCI is > > perfectly viable but there's currently no way to specify such a > > thing.] > > I guess the problem is not what they expect generally, > but specifically that some users might rely on spapr with > -nodefaults having PCI? I'm pretty sure libvirt will rely on that, if nothing else. > I don't have any ideas besides introducing a new machine type > that is same as spapr but without the default PCI host bridge.
On Thu, May 30, 2013 at 01:34:41PM +1000, David Gibson wrote: > On Wed, May 29, 2013 at 03:22:29PM +0300, Michael S. Tsirkin wrote: > > On Wed, May 29, 2013 at 09:04:00PM +1000, David Gibson wrote: > > > On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > > > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David Gibson wrote: > > > > > > > > > > Currently pci_get_primary_bus() searches the list of root buses for one > > > > > > > > > > with domain 0. But since host buses are always registered with domain 0, > > > > > > > > > > this just amounts to finding the only PCI host bus. > > > > > > > > > > > > > > > > > > > > This simplifies the implementation by defining the primary PCI bus to > > > > > > > > > > be the first one registered, using a global variable to track it. > > > > > > > > > > > > > > > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > > > > > > > > > > > > > > > > Or better: can we just fail if there is more than > > > > > > > > > one root? > > > > > > > > > > > > > > > > That might work, I'll look into doing that. > > > > > > > > > > > > > > So, the difficulty with this is that then any machine with multiple > > > > > > > PCI bridges could not use pci_nic_init(), since it calls > > > > > > > pci_get_bus_devfn() which calls pci_find_primary_bus() which would > > > > > > > always fail. And using pci_nic_init() is more or less mandatory in > > > > > > > the machine_init function to support old-style nic configuration. > > > > > > > > > > > > > > Suggestions? > > > > > > > > > > > > You mean multiple PCI roots? > > > > > > Well, there are no legacy machines with multiple roots to support, are > > > > > > there? So why do we need to support legacy flags for these new > > > > > > configurations? > > > > > > > > > > Because people expect them. > > > > > > > > People can learn, somehow they will learn to add a new root, so they can > > > > learn to use -device too. > > > > > > Hrm. I'd kind of like a second (third?) opinion on that. Anthony? > > > > > > > So let's make it fail on multiple roots, and output a message along the > > > > lines of "please use -device virtio-net-pci instead". > > > > > > How to produce a meaningful error like that isn't totally obvious, > > > since the test for multiple roots is down in find_primary_pci_bus() > > > (or whatever), and once we get back up to pci_nic_init() we just know > > > that pci_get_bus_devfn() failed for some reason. > > > > What other possible reason for it to fail? > > Unparseable address (it can be user specified) or internal failure to > initialize the device are the first two that spring to mind.. Well, let's change the API in some way. How about we pass root to pci_get_bus_devfn? > > > > > Plus on spapr we already support the > > > > > legacy nic options; it would be very strange for them to suddenly > > > > > break when we add a second host bridge. > > > > > > > > Not sure who "we" is here. IMHO user should ask for a new > > > > machine type with two roots explicitly. > > > > > > You seem to be thinking of the number of host bridges as a fixed > > > property of the platform, which it isn't on spapr. PCI host bridges > > > are just another device. Large scale real hardware can easily have > > > dozens of them. > > > > Absolutely. I'm not thinking of it as fixed. > > I'm thinking of the *default* number of pci host bridges > > as fixed. If a user is smart enough to use -device to create > > a host bridge, said user can learn about -device for creating > > a nic. > > Hm, I guess. I'm still uncomfortable with breaking a documented > option, even if its not the preferred method these days. Let's add > > > In qemu we create one always as a convenience, but > > > users can (and will have to, for vfio) create additional ones > > > trivially with -device. > > > > So they know about -device then. > > > > > [Which raises another complication as a tangent. People (and libvirt) > > > don't generally expect -nodefaults to remove the PCI bridge, but > > > arguably it should on spapr, since a PAPR guest with no PCI is > > > perfectly viable but there's currently no way to specify such a > > > thing.] > > > > I guess the problem is not what they expect generally, > > but specifically that some users might rely on spapr with > > -nodefaults having PCI? > > I'm pretty sure libvirt will rely on that, if nothing else. > > > I don't have any ideas besides introducing a new machine type > > that is same as spapr but without the default PCI host bridge. > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Thu, May 30, 2013 at 08:02:27AM +0300, Michael S. Tsirkin wrote: > On Thu, May 30, 2013 at 01:34:41PM +1000, David Gibson wrote: > > On Wed, May 29, 2013 at 03:22:29PM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 29, 2013 at 09:04:00PM +1000, David Gibson wrote: > > > > On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > > > > > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > > > > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > > > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David > Gibson wrote: [snip] > > > > > So let's make it fail on multiple roots, and output a message along the > > > > > lines of "please use -device virtio-net-pci instead". > > > > > > > > How to produce a meaningful error like that isn't totally obvious, > > > > since the test for multiple roots is down in find_primary_pci_bus() > > > > (or whatever), and once we get back up to pci_nic_init() we just know > > > > that pci_get_bus_devfn() failed for some reason. > > > > > > What other possible reason for it to fail? > > > > Unparseable address (it can be user specified) or internal failure to > > initialize the device are the first two that spring to mind.. > > Well, let's change the API in some way. How about we > pass root to pci_get_bus_devfn? Alrighty, that I can do. I was initially hesitant, since at least notionally the given PCI address string can include a domain, but we're already pretty much explicitly disabling that, and none of the built-in examples use it, so I think it's fine. > > > > > > Plus on spapr we already support the > > > > > > legacy nic options; it would be very strange for them to suddenly > > > > > > break when we add a second host bridge. > > > > > > > > > > Not sure who "we" is here. IMHO user should ask for a new > > > > > machine type with two roots explicitly. > > > > > > > > You seem to be thinking of the number of host bridges as a fixed > > > > property of the platform, which it isn't on spapr. PCI host bridges > > > > are just another device. Large scale real hardware can easily have > > > > dozens of them. > > > > > > Absolutely. I'm not thinking of it as fixed. > > > I'm thinking of the *default* number of pci host bridges > > > as fixed. If a user is smart enough to use -device to create > > > a host bridge, said user can learn about -device for creating > > > a nic. > > > > Hm, I guess. I'm still uncomfortable with breaking a documented > > option, even if its not the preferred method these days. > > Let's add Uh.. was there supposed to be the rest of a sentence there?
On Thu, Jun 06, 2013 at 05:39:11PM +1000, David Gibson wrote: > On Thu, May 30, 2013 at 08:02:27AM +0300, Michael S. Tsirkin wrote: > > On Thu, May 30, 2013 at 01:34:41PM +1000, David Gibson wrote: > > > On Wed, May 29, 2013 at 03:22:29PM +0300, Michael S. Tsirkin wrote: > > > > On Wed, May 29, 2013 at 09:04:00PM +1000, David Gibson wrote: > > > > > On Wed, May 29, 2013 at 01:17:13PM +0300, Michael S. Tsirkin wrote: > > > > > > On Wed, May 29, 2013 at 08:06:42PM +1000, David Gibson wrote: > > > > > > > On Wed, May 29, 2013 at 12:55:53PM +0300, Michael S. Tsirkin wrote: > > > > > > > > On Wed, May 29, 2013 at 07:43:41PM +1000, David Gibson wrote: > > > > > > > > > On Thu, May 23, 2013 at 10:16:27PM +1000, David Gibson wrote: > > > > > > > > > > On Thu, May 23, 2013 at 02:22:30PM +0300, Michael S. Tsirkin wrote: > > > > > > > > > > > On Thu, May 09, 2013 at 10:31:10AM +1000, David > > Gibson wrote: > [snip] > > > > > > So let's make it fail on multiple roots, and output a message along the > > > > > > lines of "please use -device virtio-net-pci instead". > > > > > > > > > > How to produce a meaningful error like that isn't totally obvious, > > > > > since the test for multiple roots is down in find_primary_pci_bus() > > > > > (or whatever), and once we get back up to pci_nic_init() we just know > > > > > that pci_get_bus_devfn() failed for some reason. > > > > > > > > What other possible reason for it to fail? > > > > > > Unparseable address (it can be user specified) or internal failure to > > > initialize the device are the first two that spring to mind.. > > > > Well, let's change the API in some way. How about we > > pass root to pci_get_bus_devfn? > > Alrighty, that I can do. I was initially hesitant, since at least > notionally the given PCI address string can include a domain, but > we're already pretty much explicitly disabling that, and none of the > built-in examples use it, so I think it's fine. > > > > > > > > Plus on spapr we already support the > > > > > > > legacy nic options; it would be very strange for them to suddenly > > > > > > > break when we add a second host bridge. > > > > > > > > > > > > Not sure who "we" is here. IMHO user should ask for a new > > > > > > machine type with two roots explicitly. > > > > > > > > > > You seem to be thinking of the number of host bridges as a fixed > > > > > property of the platform, which it isn't on spapr. PCI host bridges > > > > > are just another device. Large scale real hardware can easily have > > > > > dozens of them. > > > > > > > > Absolutely. I'm not thinking of it as fixed. > > > > I'm thinking of the *default* number of pci host bridges > > > > as fixed. If a user is smart enough to use -device to create > > > > a host bridge, said user can learn about -device for creating > > > > a nic. > > > > > > Hm, I guess. I'm still uncomfortable with breaking a documented > > > option, even if its not the preferred method these days. > > > > Let's add > > Uh.. was there supposed to be the rest of a sentence there? I meant let's add documentation that says -net nic is deprecated, and not supported with multiple root devices, and to use -device instead. > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index a3c192c..b25a1a1 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -96,6 +96,7 @@ struct PCIHostBus { QLIST_ENTRY(PCIHostBus) next; }; static QLIST_HEAD(, PCIHostBus) host_buses; +static PCIBus *pci_primary_bus; static const VMStateDescription vmstate_pcibus = { .name = "PCIBUS", @@ -241,6 +242,12 @@ static int pcibus_reset(BusState *qbus) static void pci_host_bus_register(int domain, PCIBus *bus) { struct PCIHostBus *host; + + /* If this is the first one, assume it's the primary bus */ + if (!pci_primary_bus) { + pci_primary_bus = bus; + } + host = g_malloc0(sizeof(*host)); host->domain = domain; host->bus = bus; @@ -249,15 +256,7 @@ static void pci_host_bus_register(int domain, PCIBus *bus) PCIBus *pci_get_primary_bus(void) { - struct PCIHostBus *host; - - QLIST_FOREACH(host, &host_buses, next) { - if (host->domain == 0) { - return host->bus; - } - } - - return NULL; + return pci_primary_bus; } PCIBus *pci_device_root_bus(const PCIDevice *d) @@ -300,6 +299,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent, /* host bridge */ QLIST_INIT(&bus->child); + pci_host_bus_register(0, bus); /* for now only pci domain 0 is supported */ vmstate_register(NULL, -1, &vmstate_pcibus, bus);
Currently pci_get_primary_bus() searches the list of root buses for one with domain 0. But since host buses are always registered with domain 0, this just amounts to finding the only PCI host bus. This simplifies the implementation by defining the primary PCI bus to be the first one registered, using a global variable to track it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/pci/pci.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)