From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [PATCH]Add MSI support to PV_dom0 Date: Wed, 13 May 2009 10:31:03 -0700 Message-ID: <4A0B03D7.7010802@goop.org> References: <4A09D69F.4010109@goop.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Jiang, Yunhong" Cc: Xen-devel , Matthew Wilcox List-Id: xen-devel@lists.xenproject.org Jiang, Yunhong wrote: >> moment, a Linux irq == the gsi, which is a convention I kept for Xen >> > > I suspect if this will always work, because > 1) It may not work on x86_32, because gsi is compressed in x86_32, or we will not support that? > Yinghai Lu has posted patches to remove gsi compression, now that dynamic irq arrays mean we have no practical limit on irq numbers. > 2) Even in x86_64, I suspect it may not work still. Currently Xen defined NR_IRQS as 256, unless specified in Rules.mk. So what will happen if the gsi > 256? I suspect the PHYSDEVOP_alloc_irq_vector hypercall will fail. > > The reason is, the pirq is virtual interrupt line between Xen/guest, and is determined by Xen HV, while irq/gsi is defined by guest/dom0. Current Xen's dom0 has a mapping between pirq/irq, so I'm not sure if that logic is needed in pv_dom0 still. > Well, we could decouple Xen pirq from Linux irq fairly easily if needed. Or just limit the range of identity mapped irqs (by adjusting get_nr_hw_irqs()/identity_mapped_irq()) so that the common case will still get identities but we can leave room in the sub-256 irq space for pirqs. > Sure, I will do like that. In fact, my draft implemented this way. > BTW, what do you mean of "an ops vec"??? > A struct msi_ops (or something) with function pointers to appropriate functions. But I think a better approach is to just pull everything out into xen_setup_msi_irqs(). >>> void arch_teardown_msi_irq(unsigned int irq) { - destroy_irq(irq); + >>> if (xen_destroy_irq(irq)) + destroy_irq(irq); >>> >> Yes, this little pattern should be put into a single place. >> > > What do you mean of put into a single place? > Have a destroy_msi_irq() which contains this logic, and call it from whenever it is needed. >> (base + 0x04) >> Isn't this defined somewhere else? If not, is there a better >> place to define it? >> > > It is defined in drivers/pci/msi.h. As I don't want to touch anything in drivers/pci/ directory, so I put it here (sorry that I should place it under some .h file). > It is always best to put generically useful stuff in the most appropriate common place. >>> commit 134186e0b4526908b4c64c1454caff5db6ddf972 Author: Jiang, >>> Yunhong Date: Thu May 7 21:22:26 2009 >>> +0800 Add the pci wrap function, to notify Xen of all PCI >>> information. Currently Xen depends on Dom0 to notify all PCI >>> information. When Xen try to setup MSI for a pci device, it will >>> depends on this information. This method need more discussion, since >>> it add some ugly hook to pci_bus_type. >>> >> Yes, this is a bit unpleasant. I can't see any immediately >> satisfying solution, partly because >> I don't fully understand what needs to be done in these hooks. >> Why does it need to do this at probe >> time rather than when setting up the interrupts? >> > > The main purpose of this hook is to keep Xen have the list of PCI device in the system, include those hot plug/unpluged devices, before the device begin function, I think the main purpose is for VT-d support in Xen, so it is in probe time. > > In fact, MSI should works without this list. Unfortunately, currently Xen will check if the device exists or not when enabling MSI/MSI-X, and will return -ENODEV if the device is not in the list. > Well, couldn't we quietly add it to the list just before setting up the first interrupt for the device? I guess that wouldn't work for VT-d support, unless we can defer it until we first see the device in some Xen-specific code. Matthew, do you have any thoughts about a cleaner way to hook into probe/remove? Would a arch_pci_device_probe() hook in pci_device_probe() be a reasonable way to handle it? > But I do think this code is very ugly, and I suspect if we can push this code into upstream successfully. I know Winston is working on PCI hotplug in Xen side, I will talk with him to see if any plan to change this logic. > Either way we're eventually going to need something like it for VT-d, won't we? >>> +static int pci_ari_enabled(struct pci_bus *bus) +{ + return >>> bus->self && bus->self->ari_enabled; +} >>> >> Is this a generally useful predicate? >> > > I just copied from Xen's dom0 code. I will check it . > It looks like something that could be in a common pci header. J