On Sun, Jan 23, 2011 at 05:57:53PM +0200, Michael S. Tsirkin wrote: > On Sat, Jan 22, 2011 at 01:39:57AM +0900, Isaku Yamahata wrote: > > On Fri, Jan 21, 2011 at 04:29:41PM +0200, Michael S. Tsirkin wrote: > > > On Fri, Jan 21, 2011 at 07:44:16PM +0900, Isaku Yamahata wrote: > > > > On Thu, Jan 20, 2011 at 04:15:48PM +0200, Michael S. Tsirkin wrote: > > > > > On Thu, Jan 20, 2011 at 03:57:39PM +0900, Isaku Yamahata wrote: > > > > > > make pci_find_device() ARI aware. > > > > > > > > > > > > Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp> > > > > > > --- > > > > > > hw/pci.c | 7 +++++++ > > > > > > 1 files changed, 7 insertions(+), 0 deletions(-) > > > > > > > > > > > > diff --git a/hw/pci.c b/hw/pci.c > > > > > > index 8d0e3df..851f350 100644 > > > > > > --- a/hw/pci.c > > > > > > +++ b/hw/pci.c > > > > > > @@ -1596,11 +1596,18 @@ PCIBus *pci_find_bus(PCIBus *bus, int > > > > > > bus_num) > > > > > > > > > > > > PCIDevice *pci_find_device(PCIBus *bus, int bus_num, int slot, int > > > > > > function) > > > > > > { > > > > > > + PCIDevice *d; > > > > > > bus = pci_find_bus(bus, bus_num); > > > > > > > > > > > > if (!bus) > > > > > > return NULL; > > > > > > > > > > > > + d = bus->parent_dev; > > > > > > + if (d && pci_is_express(d) && > > > > > > + pcie_cap_get_type(d) == PCI_EXP_TYPE_DOWNSTREAM && > > > > > > + !pcie_cap_is_ari_enabled(d) && slot > 0) { > > > > > > + return NULL; > > > > > > + } > > > > > > return bus->devices[PCI_DEVFN(slot, function)]; > > > > > > } > > > > > > > > > > I'd like to split this express-specific code out in some way. > > > > > Further, the downstream port always has a single slot. > > > > > Maybe it shouldn't be a bus at all, but this needs some thought. > > > > > > > > Yes, it needs consideration. > > > > > > > > > > > > > How about we put flag in PCIBus that says that a single > > > > > slot is supported? Downstream ports would just set it. > > > > > > > > So such a flag must be set/clear by something like > > > > pcie_cap_ari_write_config() > > > > depending on ARI bit at runtime. > > > > > > Well, to figure it out, could you please describe what is the situation > > > your patch tries to fix? I would generally would like a reason for the > > > change to be given in commit logs, please try to avoid just restating > > > what the patch does. > > > > It seems that I should have added the comment to refer the spec. > > I'd like to implement ARI enable bit correctly. > > > > Downstream port(and root port) doesn't forward pci transaction for > > function > 7 by default for compatibility, > > Only when ARI forwarding enable bit of downstream/root port is set, > > the virtual p2p bridge forwards pci transaction for > > function > 7 (i.e. slot > 0). > > Oh, I see, yes, function > 7 gets described as slot >0. > I think this is what I missed. > Hmm, it'd pretty confusing. Should we fix this, > pass devfn all over?
Sounds to make sense. Although it seems only pci_find_device() will be affected at a glance, I'll look into it more closely. > I now understand what the code does, it just needs > a good comment that explains that at the moment > slot encodes the high bits of the device id. > > Also, let's replace pcie_cap_is_ari_enabled > with an inline that does all the relevant logic > E.g. > > /* With PCI Express Endpoints, there's a single device behind > each downstream port bus, and bits 3:7 of the function number get > encoded in the slot number (the Express spec calls it the Device > Number). This allows > 8 functions, but > these extended functions are only accessible when the > Alternative routing-ID Interpretation (ARI) > capability is enabled in the downstream port. With that capability > disabled the port enforces the Device Number field being 0.*/ > static inline > bool pcie_check_slot(PCIDevice *dev) > { > return !pci_is_express(dev) || !slot || > pcie_cap_get_type(dev) != PCI_EXP_TYPE_DOWNSTREAM || > (pci_get_long(dev->config + dev->exp.exp_cap + PCI_EXP_DEVCTL2) > & > PCI_EXP_DEVCTL2_ARI); > } Okay. > > 6.13 Alternative routing-ID Interpretation(ARI) > > 7.8.15 Device capabilites 2 register > > ARI forwarding supproted > > 7.8.16 Device control 2 register > > ARI forwarding Enable > > 5 ARI Forwarding Enable When set, the Downstream Port > > disables its traditional Device Number field being 0 enforcement > > when turning a Type 1 Configuration Request into a Type 0 > > Configuration Request, permitting access to Extended Functions > > in an ARI Device immediately below the Port. See Section 6.13. > > Default value of this bit is 0b. Must be hardwired to 0b if the ARI > > Forwarding Supported bit is 0b. > > Oh, the patch should check root port in addition to downstream port. > > It should? Where does it say so? I wasn't clear enough. pcie_check_slot() above should include something like !((type == downstream) || (type == root && the below is endpoint)) > > > Are you trying to create a device with > 8 functions? > > > If that is the case I suspect this is not the best way > > > to do this at all. > > > > > > > pcie device can have 256 functions instead of 8. > > > > > > Only if it's an ARI device. And note that if you have a device with > > > 256 functions and disable ARI in the port, it appears as > > > multiple devices. > > > > > > > Maybe we'd like to emulate how p2p bridge transfers pci transaction > > > > to child pci bus somehow. > > > > > > To support > 8 functions per device, some refactoring would be needed: > > > you can not figure out slot and function from the root bus, > > > it depends on ARI along the way. So APIs that pass in > > > decoded slot/function do not make sense anymore, > > > you must pass in devfn all the way. > > > > > > But since everyone decodes and encodes them in the same way, > > > many things will work even without decoding. > > > > I think there are only two issues to address. > > Configuration space and hot plug. > > info pci, maybe others (firmware path?) Oh yes, maybe. > > As you already described it, ARI is defined such that APIs can stay same, > > just interpret slot bits as part of function number. > > This patch addresses configuration space access. > > > > Multifunction hot plug hasn't been addressed even for conventional pci case. > > But at least we learned to deny hotplug attempts for functions > 0... > > > Some kind of refactoring would be necessary for it, I think. > > It's probably just a matter of figuring out what works with acpi. > Express has a native hotplug which likely works ok already ... > > > -- > > yamahata > -- yamahata