Re: [PATCH 2/6] Provide a mechanism to register domain owner of a PCI device.

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/09/09 12:51, Konrad Rzeszutek Wilk wrote:
> . and also to find the domain owner based on the PCI device and
> to unregister a domain owner of a PCI device.
>
> Signed-off-by: Konrad Rzeszutek Wilk<konrad.wilk@oracle.com>
> ---
>   arch/x86/include/asm/xen/pci.h |   16 +++++++++
>   arch/x86/xen/pci.c             |   73 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 89 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/pci.h b/arch/x86/include/asm/xen/pci.h
> index cb84abe..f4312d5 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -7,6 +7,11 @@ int xen_create_msi_irq(struct pci_dev *dev,
>   			struct msi_desc *msidesc,
>   			int type);
>   int xen_destroy_irq(int irq);
> +
> +int find_device_owner(struct pci_dev *dev);
> +int register_device_owner(struct pci_dev *dev, domid_t domain);
> +int unregister_device_owner(struct pci_dev *dev);
>    

Could you give these less generic names?

> +
>   #else
>   static inline int xen_register_gsi(u32 gsi, int triggering, int polarity)
>   {
> @@ -23,6 +28,17 @@ static inline int xen_destroy_irq(int irq)
>   {
>   	return -1;
>   }
> +
> +static inline int find_device_owner(struct pci_dev *dev) { return -1; }
> +static inline int register_device_owner(struct pci_dev *dev, domid_t domain)
> +{
> + 	return -1;
> +}
> +static inline int unregister_device_owner(struct pci_dev *dev)
> +{
> + 	return -1;
> +}
>    

Make the formatting consistent here.

> +
>   #endif
>
>   #if defined(CONFIG_PCI_MSI)&&  defined(CONFIG_XEN_DOM0_PCI)
> diff --git a/arch/x86/xen/pci.c b/arch/x86/xen/pci.c
> index 44d91ad..4fa734c 100644
> --- a/arch/x86/xen/pci.c
> +++ b/arch/x86/xen/pci.c
> @@ -2,6 +2,7 @@
>   #include<linux/acpi.h>
>   #include<linux/pci.h>
>   #include<linux/msi.h>
> +#include<linux/slab.h>
>
>   #include<asm/mpspec.h>
>   #include<asm/io_apic.h>
> @@ -109,3 +110,75 @@ error:
>   	return ret;
>   }
>   #endif
> +
> +struct device_owner {
>    

Again, a bit generic (the name should at least convey we're talking 
about owner domains).


> +	domid_t domain;
> +	struct pci_dev *dev;
> +	struct list_head list;
> +};
> +
> +static DEFINE_SPINLOCK(dev_domain_list_spinlock);
> +static struct list_head dev_domain_list = LIST_HEAD_INIT(dev_domain_list);
> +
> +struct device_owner *find_device(struct pci_dev *dev)
> +{
> +
> +	struct device_owner *owner;
>    
Stray blank line.
> +
> +	list_for_each_entry(owner,&dev_domain_list, list) {
> +		if (owner->dev == dev)
> +			return owner;
> +	}
> +	return NULL;
> +}
> +
> +int find_device_owner(struct pci_dev *dev)
> +{
> +
> +	struct device_owner *owner = NULL;
>    
Again.
> +
> +	owner = find_device(dev);
> +	if (!owner)
> +		return -ENODEV;
> +
> +	return owner->domain;
> +}
> +EXPORT_SYMBOL(find_device_owner);
> +
> +int register_device_owner(struct pci_dev *dev, domid_t domain)
> +{
> +	struct device_owner *owner;
> +	unsigned long flags;
> +
> +	if (find_device(dev))
>    
Should this be under the lock?
> +		return -EEXIST;
> +
> +	owner = kzalloc(sizeof(struct device_owner), GFP_KERNEL);
>    
Check for NULL.
> +	owner->domain = domain;
> +	owner->dev = dev;
> +
> +	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
> +	list_add_tail(&owner->list,&dev_domain_list);
> +	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(register_device_owner);
> +
> +int unregister_device_owner(struct pci_dev *dev)
> +{
> +	struct device_owner *owner = NULL;
> +	unsigned long flags;
> +
> +	owner = find_device(dev);
>    
Shouldn't this be under the lock too?
> +	if (!owner)
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&dev_domain_list_spinlock, flags);
> +	list_del(&owner->list);
> +	spin_unlock_irqrestore(&dev_domain_list_spinlock, flags);
> +
> +	kfree(owner);
> +	return 0;
> +}
> +EXPORT_SYMBOL(unregister_device_owner);
>    

Thanks,
     J