[Xen-devel] [PATCH] Re-enable MSI support

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Currently the MSI is disabled because of some lock issue. This patch try to 
cleanup the lock related to MSI lock. Because I have no AMD's iommu 
environment, so I didn't test AMD's platform, WeiWang, can you please have a 
check on AMD's code path?

Signed-off-by: Jiang Yunhong <yunhong.jiang@xxxxxxxxx>

The current issue related including:
1) The sequence of pci_dev's lock and irq_desc's lock is not same. For example, 
In pci_cleanup_msi, it will get pci_dev lock before msi_free_vectors get 
irq_desc's lock, while in pci_disable_msi , it will get irq_desc's lock before 
pci_dev lock.
2) The sequence of pci_dev's lock and pcidevs_lock is not same.  For example, 
reassign_device_ownership  will get pci_dev's lock before get pcidevs_lock, 
while pci_lock_domai_pdev will get pcidevs_lock before pci_dev's lock.
3) The lock to bus2bridge is not always confusing. 
4) xmalloc with irq_save in several code path, which cause debug version failed.

Also some minor issue exits,(can be identified by the patch), including:  the 
spin_lock_irqsave and spin_lock for iommu->lock and hd->mapping_lock; race 
condition may happen in XEN_DOMCTL_assign_device and  
XEN_DOMCTL_deassign_device, the rangeset is freed in rangeset_domain_destroy() 
before the unmap_domain_pirq try to deny the irq access in arch_domain_destro, 
etc.

This patch try to do some cleanup for these issues.
1) The basic idea is to remove the pci_dev's lock, instead, we try to use the 
big pcidevs_lock to protect the whole pci_dev stuff. It including both pci_dev 
adding/removing, and also the assignment of the devices. We checked the code 
and seems there is no critical code path for this. We try to use fine-grained 
lock and seems the code will be very tricky.
2) Split the pci_enable_msi into two step, firstly it will just construct the 
msi_desc through pci_enable_msi without holding the irq_desc lock, and then it 
will setup msi through setup_msi_irq with irq_desc holded.
3) Change the iommu->lock and hd->mapping_lock to be irq_save. 
4) Fix to some minor issues.

Now the lock sequence is: pcidevs_lock -> domai's event_lock -> iommu's lock -> 
hvm_iommu's mapping_lock. The irq_desc's lock will always be the last lock be 
hold for peformance consideration.

Something need notice:
1) It is tricky to support multi-vector MSI. The reason is: the mask bit is in 
the same register shared by multiple vector, that means the ISR need compete 
for this register and need hold another lock, and it will cause things very 
tricky (the support is buggy in current code). So in fact, I think the current 
code does not support it.
2) I'm not sure if pci_remove_device need some improvement, to make sure the 
resouce is freed, since it assume the pci device is owned by other domain 
still. For example, it may need call pci_clean_dpci_irqs(). But because I'm not 
sure how will this function be used, so I didn't touch it. 

One thing left is the IOMMU's page fault handler, which will try to access the 
vt-d's mapping table and context table currently. We can simply remove the 
print_vtd_entries in iommu_page_fault_do_one, or place the print task in a 
delay work. 

I tried to split the patch to some smaller one, but seems the logic is 
something related, so I give up later.

Thanks
Yunhong Jiang

Attachment: msi_lock.patch
Description: msi_lock.patch

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel