Re: [PATCH]Add MSI support to PV_dom0

[Jeremy Fitzhardinge <jeremy@goop.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 <yunhong.jiang@intel.com> 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