| Message ID | C8EEA077.17867%wexu2@cisco.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: > Isaku, > > To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM > monitor, we can do hotplug like: > pci_add auto|<bus:dev> nic|storage > to hotplug a device to PCIe/PCI bus. > > But we have two problems here: > (1) command line for example, "-net nic,addr=<bus:dev>" always failed > because it cannot find the bus. > (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the > device will be attached to PCI bus, in stead of PCIe ports > > I solved the first problem. The root cause is pcie_root_write_config (which > init SECONDARY_BUS config) happened after pci_nic_init. The latter use > pci_find_bus to find the parent bus but failed because config space for > secondary bus still not initialized. Okay, now the issue is clear to me. When I tried to clean up pci bridge code, I included the similar lines in bridge initialization function. But Michael complained about it, so I dropped the lines for merge. I think there are two ways to address it. - initialize secondary/subordinate bus register by qemu i.e. something like the patch below. - use qdev device path i.e. qbus_find() instead of pci_find_bus(). I don't know if the corresponding command line option already exists or not. My preference is the former, but discussion is necessary to get consensus. My plan was to raise the issue when I address pv pci bus numbering issue. I've expected that other developers wouldn't think the issue important because multiple pci bus can't be used easily right now. thanks, > > The fix is like: > > [wexu2@wexu2-lnx contents]$ git diff hw/pci_bridge.c > diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c > index c048305..042c1f9 100644 > --- a/hw/pci_bridge.c > +++ b/hw/pci_bridge.c > @@ -203,6 +203,7 @@ void pci_bridge_set_bus_number(PCIBridge *br, > uint16_t domain; > uint8_t primary_bus; > uint8_t devfn; > + uint8_t *conf; > > if (!pci_bus_number) { > pci_bus_number = qemu_mallocz(sizeof(*pci_bus_number)); > @@ -215,10 +216,15 @@ void pci_bridge_set_bus_number(PCIBridge *br, > > d = &br->dev; > devfn = d->devfn; > + conf = d->config; > > br->sec_bus = secondary; > br->sub_bus = subordinate; > > + //TODO wexu2 workaround of "-net nic,addr=<bus:dev>" > + pci_set_byte(conf + PCI_SECONDARY_BUS, secondary); > + pci_set_byte(conf + PCI_SUBORDINATE_BUS, subordinate); > + > bus = d->bus; > primary_bus = 0; > if ((d = pci_bridge_get_device(bus)) != NULL){ > > The fix is a little bit hacky -- welcome any advice. > > For 2nd issue, current code assume PCI bus is flat and not straight forward > to change the behavior. > > Wei Xu > > > On 10/27/10 10:16 PM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: > > > Oh, now I'm seeing your point. Some codes of qemu assume that > > the pci bus is flat. They need fixes. > > > > Right now I've found > > - parse_pci_devfn in qdev-properties.c > > This corresponds to (1) > > - pci_get_bus_devfn() in pci.c > > This corresponds to (2) > > > > thanks, > > > > On Wed, Oct 27, 2010 at 08:51:29PM -0700, Wei Xu wrote: > >> Isaku, > >> > >> I found two issues: > >> (1) command line option is not working: even with addr=<bus>, it complained > >> bus cannot be found; > >> (2) monitor: pci_add auto ... cannot work. > >> > >> Wei > >> > >> > >> On 10/27/10 8:03 PM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: > >> > >>> On Wed, Oct 27, 2010 at 09:51:47AM -0700, Wei Xu wrote: > >>>> Isaku, > >>>> > >>>> I have one question regarding EP and PCIe bus: now only way is to use > >>>> "pci_add <bus:dev> .." to hot-plug the device to a PCIe port. Looks like > >>>> command line with -device parameter automatically use PCI bus? Is it > >>>> possible to support it? Have to provide the <bus:dev> in -device parameter? > >>> > >>> I'm not sure I got your question right. > >>> We can populate the device under an express port with -device command line > >>> option. At least it should be possible. > >>> > >>> For implementation detail, qemu doesn't distinguish pci express > >>> from conventional pci so strictly. > >>> The only difference is whether the bridge(pci host bridge or > >>> pci-to-pci bridge) is just express port because I implemented express > >>> ports as pci-to-pci bridge. > >>> > >>> Or do you want to add express port with command line option? > >>> If so, not possible at the moment. > >> >
Isaku, We are on same page now; I also consider the qdev_find way. Both ways can work and let's wait for others' opinions... Wei On 10/28/10 4:25 AM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: > On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: >> Isaku, >> >> To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM >> monitor, we can do hotplug like: >> pci_add auto|<bus:dev> nic|storage >> to hotplug a device to PCIe/PCI bus. >> >> But we have two problems here: >> (1) command line for example, "-net nic,addr=<bus:dev>" always failed >> because it cannot find the bus. >> (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the >> device will be attached to PCI bus, in stead of PCIe ports >> >> I solved the first problem. The root cause is pcie_root_write_config (which >> init SECONDARY_BUS config) happened after pci_nic_init. The latter use >> pci_find_bus to find the parent bus but failed because config space for >> secondary bus still not initialized. > > Okay, now the issue is clear to me. > When I tried to clean up pci bridge code, I included the similar lines > in bridge initialization function. > But Michael complained about it, so I dropped the lines for merge. > > I think there are two ways to address it. > - initialize secondary/subordinate bus register by qemu > i.e. something like the patch below. > > - use qdev device path > i.e. qbus_find() instead of pci_find_bus(). > I don't know if the corresponding command line option already exists or not. > > My preference is the former, but discussion is necessary to get consensus. > My plan was to raise the issue when I address pv pci bus numbering issue. > I've expected that other developers wouldn't think the issue important > because multiple pci bus can't be used easily right now. > > thanks, > >> >> The fix is like: >> >> [wexu2@wexu2-lnx contents]$ git diff hw/pci_bridge.c >> diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c >> index c048305..042c1f9 100644 >> --- a/hw/pci_bridge.c >> +++ b/hw/pci_bridge.c >> @@ -203,6 +203,7 @@ void pci_bridge_set_bus_number(PCIBridge *br, >> uint16_t domain; >> uint8_t primary_bus; >> uint8_t devfn; >> + uint8_t *conf; >> >> if (!pci_bus_number) { >> pci_bus_number = qemu_mallocz(sizeof(*pci_bus_number)); >> @@ -215,10 +216,15 @@ void pci_bridge_set_bus_number(PCIBridge *br, >> >> d = &br->dev; >> devfn = d->devfn; >> + conf = d->config; >> >> br->sec_bus = secondary; >> br->sub_bus = subordinate; >> >> + //TODO wexu2 workaround of "-net nic,addr=<bus:dev>" >> + pci_set_byte(conf + PCI_SECONDARY_BUS, secondary); >> + pci_set_byte(conf + PCI_SUBORDINATE_BUS, subordinate); >> + >> bus = d->bus; >> primary_bus = 0; >> if ((d = pci_bridge_get_device(bus)) != NULL){ >> >> The fix is a little bit hacky -- welcome any advice. >> >> For 2nd issue, current code assume PCI bus is flat and not straight forward >> to change the behavior. >> >> Wei Xu >> >> >> On 10/27/10 10:16 PM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: >> >>> Oh, now I'm seeing your point. Some codes of qemu assume that >>> the pci bus is flat. They need fixes. >>> >>> Right now I've found >>> - parse_pci_devfn in qdev-properties.c >>> This corresponds to (1) >>> - pci_get_bus_devfn() in pci.c >>> This corresponds to (2) >>> >>> thanks, >>> >>> On Wed, Oct 27, 2010 at 08:51:29PM -0700, Wei Xu wrote: >>>> Isaku, >>>> >>>> I found two issues: >>>> (1) command line option is not working: even with addr=<bus>, it complained >>>> bus cannot be found; >>>> (2) monitor: pci_add auto ... cannot work. >>>> >>>> Wei >>>> >>>> >>>> On 10/27/10 8:03 PM, "Isaku Yamahata" <yamahata@valinux.co.jp> wrote: >>>> >>>>> On Wed, Oct 27, 2010 at 09:51:47AM -0700, Wei Xu wrote: >>>>>> Isaku, >>>>>> >>>>>> I have one question regarding EP and PCIe bus: now only way is to use >>>>>> "pci_add <bus:dev> .." to hot-plug the device to a PCIe port. Looks like >>>>>> command line with -device parameter automatically use PCI bus? Is it >>>>>> possible to support it? Have to provide the <bus:dev> in -device >>>>>> parameter? >>>>> >>>>> I'm not sure I got your question right. >>>>> We can populate the device under an express port with -device command line >>>>> option. At least it should be possible. >>>>> >>>>> For implementation detail, qemu doesn't distinguish pci express >>>>> from conventional pci so strictly. >>>>> The only difference is whether the bridge(pci host bridge or >>>>> pci-to-pci bridge) is just express port because I implemented express >>>>> ports as pci-to-pci bridge. >>>>> >>>>> Or do you want to add express port with command line option? >>>>> If so, not possible at the moment. >>>> >>
Isaku Yamahata <yamahata@valinux.co.jp> writes: > On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: >> Isaku, >> >> To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM >> monitor, we can do hotplug like: >> pci_add auto|<bus:dev> nic|storage >> to hotplug a device to PCIe/PCI bus. >> >> But we have two problems here: >> (1) command line for example, "-net nic,addr=<bus:dev>" always failed >> because it cannot find the bus. >> (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the >> device will be attached to PCI bus, in stead of PCIe ports >> >> I solved the first problem. The root cause is pcie_root_write_config (which >> init SECONDARY_BUS config) happened after pci_nic_init. The latter use >> pci_find_bus to find the parent bus but failed because config space for >> secondary bus still not initialized. > > Okay, now the issue is clear to me. > When I tried to clean up pci bridge code, I included the similar lines > in bridge initialization function. > But Michael complained about it, so I dropped the lines for merge. > > I think there are two ways to address it. > - initialize secondary/subordinate bus register by qemu > i.e. something like the patch below. > > - use qdev device path > i.e. qbus_find() instead of pci_find_bus(). > I don't know if the corresponding command line option already exists or not. > > My preference is the former, but discussion is necessary to get consensus. > My plan was to raise the issue when I address pv pci bus numbering issue. > I've expected that other developers wouldn't think the issue important > because multiple pci bus can't be used easily right now. I'm afraid I'm missing context here. Could you restate the problem? New devices and buses need to work with -device / device_add. pci_add and -net nic are for backwards compatibility. Folks used to them may appreciate you making them work for PCIe. Longer term, they should either go away or become syntactic sugar.
On Fri, Oct 29, 2010 at 10:02:47AM +0200, Markus Armbruster wrote: > Isaku Yamahata <yamahata@valinux.co.jp> writes: > > > On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: > >> Isaku, > >> > >> To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM > >> monitor, we can do hotplug like: > >> pci_add auto|<bus:dev> nic|storage > >> to hotplug a device to PCIe/PCI bus. > >> > >> But we have two problems here: > >> (1) command line for example, "-net nic,addr=<bus:dev>" always failed > >> because it cannot find the bus. > >> (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the > >> device will be attached to PCI bus, in stead of PCIe ports > >> > >> I solved the first problem. The root cause is pcie_root_write_config (which > >> init SECONDARY_BUS config) happened after pci_nic_init. The latter use > >> pci_find_bus to find the parent bus but failed because config space for > >> secondary bus still not initialized. > > > > Okay, now the issue is clear to me. > > When I tried to clean up pci bridge code, I included the similar lines > > in bridge initialization function. > > But Michael complained about it, so I dropped the lines for merge. > > > > I think there are two ways to address it. > > - initialize secondary/subordinate bus register by qemu > > i.e. something like the patch below. > > > > - use qdev device path > > i.e. qbus_find() instead of pci_find_bus(). > > I don't know if the corresponding command line option already exists or not. > > > > My preference is the former, but discussion is necessary to get consensus. > > My plan was to raise the issue when I address pv pci bus numbering issue. > > I've expected that other developers wouldn't think the issue important > > because multiple pci bus can't be used easily right now. > > I'm afraid I'm missing context here. Could you restate the problem? Wei, please correct/clarify me if I'm wrong. The issues is that pci address, (segment, bus, dev, fn), doesn't always work as an argument to qemu command line or qemu monitor because pci bus numbering is done by guest BIOS/OS. In flat pci bus case, i.e. there is a single pci bus of bus=0, fortunately it works. But if there are multiple pci buses, (segment, bus, dev, fn) doesn't work as expected until guest bios/os numbers pci buses. For example if pci bus address with non-0 bus number is passed as qemu command line option in order to add pci device, qemu fails to find the pci bus. > New devices and buses need to work with -device / device_add. pci_add > and -net nic are for backwards compatibility. Folks used to them may > appreciate you making them work for PCIe. Longer term, they should > either go away or become syntactic sugar. The issue is caused not by pcie, but by multiple pci buses. I suppose that -device/device_add uses qdev path, not pci address, so it would be okay. pci_add or -net nic wouldn't work. (or only works with bus=0)
[Note cc: Michael & Gerd] Isaku Yamahata <yamahata@valinux.co.jp> writes: > On Fri, Oct 29, 2010 at 10:02:47AM +0200, Markus Armbruster wrote: >> Isaku Yamahata <yamahata@valinux.co.jp> writes: >> >> > On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: >> >> Isaku, >> >> >> >> To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM >> >> monitor, we can do hotplug like: >> >> pci_add auto|<bus:dev> nic|storage >> >> to hotplug a device to PCIe/PCI bus. >> >> >> >> But we have two problems here: >> >> (1) command line for example, "-net nic,addr=<bus:dev>" always failed >> >> because it cannot find the bus. >> >> (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the >> >> device will be attached to PCI bus, in stead of PCIe ports >> >> >> >> I solved the first problem. The root cause is pcie_root_write_config (which >> >> init SECONDARY_BUS config) happened after pci_nic_init. The latter use >> >> pci_find_bus to find the parent bus but failed because config space for >> >> secondary bus still not initialized. >> > >> > Okay, now the issue is clear to me. >> > When I tried to clean up pci bridge code, I included the similar lines >> > in bridge initialization function. >> > But Michael complained about it, so I dropped the lines for merge. >> > >> > I think there are two ways to address it. >> > - initialize secondary/subordinate bus register by qemu >> > i.e. something like the patch below. >> > >> > - use qdev device path >> > i.e. qbus_find() instead of pci_find_bus(). >> > I don't know if the corresponding command line option already exists or not. >> > >> > My preference is the former, but discussion is necessary to get consensus. >> > My plan was to raise the issue when I address pv pci bus numbering issue. >> > I've expected that other developers wouldn't think the issue important >> > because multiple pci bus can't be used easily right now. >> >> I'm afraid I'm missing context here. Could you restate the problem? > > Wei, please correct/clarify me if I'm wrong. > > The issues is that pci address, (segment, bus, dev, fn), doesn't always > work as an argument to qemu command line or qemu monitor because > pci bus numbering is done by guest BIOS/OS. > In flat pci bus case, i.e. there is a single pci bus of bus=0, > fortunately it works. > But if there are multiple pci buses, (segment, bus, dev, fn) > doesn't work as expected until guest bios/os numbers pci buses. > For example if pci bus address with non-0 bus number is passed as > qemu command line option in order to add pci device, qemu fails to > find the pci bus. Guest-assigned addresses need not be stable and predictable. Using them for QEMU device configuration feels wrong to me. Is there a way to name a PCI bus that is independent of guest software? >> New devices and buses need to work with -device / device_add. pci_add >> and -net nic are for backwards compatibility. Folks used to them may >> appreciate you making them work for PCIe. Longer term, they should >> either go away or become syntactic sugar. > > The issue is caused not by pcie, but by multiple pci buses. Got it. > I suppose that -device/device_add uses qdev path, not pci address, > so it would be okay. Yes, except qdev paths are a bit of a mess right now. Separate problem. > pci_add or -net nic wouldn't work. (or only works with bus=0)
On Fri, Oct 29, 2010 at 11:54:27AM +0200, Markus Armbruster wrote: > [Note cc: Michael & Gerd] > > Isaku Yamahata <yamahata@valinux.co.jp> writes: > > > On Fri, Oct 29, 2010 at 10:02:47AM +0200, Markus Armbruster wrote: > >> Isaku Yamahata <yamahata@valinux.co.jp> writes: > >> > >> > On Thu, Oct 28, 2010 at 03:37:27AM -0700, Wei Xu wrote: > >> >> Isaku, > >> >> > >> >> To make things clear, let me rephrase the problem. With q35/vPCIe, in VMM > >> >> monitor, we can do hotplug like: > >> >> pci_add auto|<bus:dev> nic|storage > >> >> to hotplug a device to PCIe/PCI bus. > >> >> > >> >> But we have two problems here: > >> >> (1) command line for example, "-net nic,addr=<bus:dev>" always failed > >> >> because it cannot find the bus. > >> >> (2) If "pci_add auto" in monitor or no "addr=<bus:dev" in command line, the > >> >> device will be attached to PCI bus, in stead of PCIe ports > >> >> > >> >> I solved the first problem. The root cause is pcie_root_write_config (which > >> >> init SECONDARY_BUS config) happened after pci_nic_init. The latter use > >> >> pci_find_bus to find the parent bus but failed because config space for > >> >> secondary bus still not initialized. > >> > > >> > Okay, now the issue is clear to me. > >> > When I tried to clean up pci bridge code, I included the similar lines > >> > in bridge initialization function. > >> > But Michael complained about it, so I dropped the lines for merge. > >> > > >> > I think there are two ways to address it. > >> > - initialize secondary/subordinate bus register by qemu > >> > i.e. something like the patch below. > >> > > >> > - use qdev device path > >> > i.e. qbus_find() instead of pci_find_bus(). > >> > I don't know if the corresponding command line option already exists or not. > >> > > >> > My preference is the former, but discussion is necessary to get consensus. > >> > My plan was to raise the issue when I address pv pci bus numbering issue. > >> > I've expected that other developers wouldn't think the issue important > >> > because multiple pci bus can't be used easily right now. > >> > >> I'm afraid I'm missing context here. Could you restate the problem? > > > > Wei, please correct/clarify me if I'm wrong. > > > > The issues is that pci address, (segment, bus, dev, fn), doesn't always > > work as an argument to qemu command line or qemu monitor because > > pci bus numbering is done by guest BIOS/OS. > > In flat pci bus case, i.e. there is a single pci bus of bus=0, > > fortunately it works. > > But if there are multiple pci buses, (segment, bus, dev, fn) > > doesn't work as expected until guest bios/os numbers pci buses. > > For example if pci bus address with non-0 bus number is passed as > > qemu command line option in order to add pci device, qemu fails to > > find the pci bus. > > Guest-assigned addresses need not be stable and predictable. Using them > for QEMU device configuration feels wrong to me. > > Is there a way to name a PCI bus that is independent of guest software? What was proposed was a hierarchical path from root to the device. E.g. segment/device/device/device, specifying the device numbers of each bus on the parent bus. No one got around to implementing this. > >> New devices and buses need to work with -device / device_add. pci_add > >> and -net nic are for backwards compatibility. Folks used to them may > >> appreciate you making them work for PCIe. Longer term, they should > >> either go away or become syntactic sugar. > > > > The issue is caused not by pcie, but by multiple pci buses. > > Got it. > > > I suppose that -device/device_add uses qdev path, not pci address, > > so it would be okay. > > Yes, except qdev paths are a bit of a mess right now. Separate problem. > > > pci_add or -net nic wouldn't work. (or only works with bus=0)
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c index c048305..042c1f9 100644 --- a/hw/pci_bridge.c +++ b/hw/pci_bridge.c @@ -203,6 +203,7 @@ void pci_bridge_set_bus_number(PCIBridge *br, uint16_t domain; uint8_t primary_bus; uint8_t devfn; + uint8_t *conf; if (!pci_bus_number) { pci_bus_number = qemu_mallocz(sizeof(*pci_bus_number)); @@ -215,10 +216,15 @@ void pci_bridge_set_bus_number(PCIBridge *br, d = &br->dev; devfn = d->devfn; + conf = d->config; br->sec_bus = secondary; br->sub_bus = subordinate; + //TODO wexu2 workaround of "-net nic,addr=<bus:dev>" + pci_set_byte(conf + PCI_SECONDARY_BUS, secondary); + pci_set_byte(conf + PCI_SUBORDINATE_BUS, subordinate); + bus = d->bus; primary_bus = 0; if ((d = pci_bridge_get_device(bus)) != NULL){