Re: [PATCH 2/9] xen/pci: Add xen_[find|register|unregister]_device_domain_owner functions.

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/13/2010 10:01 AM, Konrad Rzeszutek Wilk wrote:
> Xen PCI backend performs ownership (MSI/MSI-X) changes on the behalf of
> the guest. This means we need some mechanism to find, set and unset
> the domain id of the guest.

Clarify this a little?  "Guest" is ambigious in this context; do you
mean set the owning domain of the device?

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/include/asm/xen/pci.h |   16 +++++++++
>  arch/x86/pci/xen.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 2329b3e..8474b4b 100644
> --- a/arch/x86/include/asm/xen/pci.h
> +++ b/arch/x86/include/asm/xen/pci.h
> @@ -15,10 +15,26 @@ static inline int pci_xen_hvm_init(void)
>  #endif
>  #if defined(CONFIG_XEN_DOM0)
>  void __init xen_setup_pirqs(void);
> +int xen_find_device_domain_owner(struct pci_dev *dev);
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain);
> +int xen_unregister_device_domain_owner(struct pci_dev *dev);
>  #else
>  static inline void __init xen_setup_pirqs(void)
>  {
>  }
> +static inline int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
> +static inline int xen_register_device_domain_owner(struct pci_dev *dev,
> + 						   uint16_t domain)
> +{
> +	return -1;
> +}
> +static inline int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	return -1;
> +}
>  #endif
>  
>  #if defined(CONFIG_PCI_MSI)
> diff --git a/arch/x86/pci/xen.c b/arch/x86/pci/xen.c
> index 117f5b8..6d2a986 100644
> --- a/arch/x86/pci/xen.c
> +++ b/arch/x86/pci/xen.c
> @@ -412,3 +412,76 @@ void __init xen_setup_pirqs(void)
>  	}
>  }
>  #endif
> +
> +struct xen_device_domain_owner {
> +	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);
> +
> +static struct xen_device_domain_owner *find_device(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	list_for_each_entry(owner, &dev_domain_list, list) {
> +		if (owner->dev == dev)
> +			return owner;
> +	}
> +	return NULL;
> +}
> +
> +int xen_find_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +	int domain = -ENODEV;

ENODEV seems odd.  ENOENT?

> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (owner)
> +		domain = owner->domain;
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return domain;
> +}
> +EXPORT_SYMBOL(xen_find_device_domain_owner);
> +
> +int xen_register_device_domain_owner(struct pci_dev *dev, uint16_t domain)

uint16_t seems like an odd type to use.  You return "int" for the domain
id above.  Xen may use a 16-bit domain identifier, but I think that if
you want to express that here there should be a xen_domid_t or
something.  But just an ordinary integer type would be just as good.

> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	owner = kzalloc(sizeof(struct xen_device_domain_owner), GFP_KERNEL);
> +	if (!owner)
> +		return -ENODEV;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	if (find_device(dev)) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		kfree(owner);
> +		return -EEXIST;

Not that its really a big deal, but I really prefer the single-exit pattern:

	if (find_device(dev)) {
		err = -EEXIST;
		goto out;
	}
...

out:
	spin_unlock(&dev_domain_list_spinlock);
	return err;
}

so that the lock/unlock can be easily matched by eye.

(Same below.)

    J

> +	}
> +	owner->domain = domain;
> +	owner->dev = dev;
> +	list_add_tail(&owner->list, &dev_domain_list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	return 0;
> +}
> +EXPORT_SYMBOL(xen_register_device_domain_owner);
> +
> +int xen_unregister_device_domain_owner(struct pci_dev *dev)
> +{
> +	struct xen_device_domain_owner *owner;
> +
> +	spin_lock(&dev_domain_list_spinlock);
> +	owner = find_device(dev);
> +	if (!owner) {
> +		spin_unlock(&dev_domain_list_spinlock);
> +		return -ENODEV;
> +	}
> +	list_del(&owner->list);
> +	spin_unlock(&dev_domain_list_spinlock);
> +	kfree(owner);
> +	return 0;
> +}
> +EXPORT_SYMBOL(xen_unregister_device_domain_owner);