WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 2/9] xen/pci: Add xen_[find|register|unregister]_

To: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 2/9] xen/pci: Add xen_[find|register|unregister]_device_domain_owner functions.
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Mon, 13 Dec 2010 11:28:09 -0800
Cc: Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, Jan Beulich <JBeulich@xxxxxxxxxx>, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Delivery-date: Mon, 13 Dec 2010 11:29:06 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1292263303-31680-3-git-send-email-konrad.wilk@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1292263303-31680-1-git-send-email-konrad.wilk@xxxxxxxxxx> <1292263303-31680-3-git-send-email-konrad.wilk@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101103 Fedora/1.0-0.33.b2pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.6
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@xxxxxxxxxx>
> ---
>  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);


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

<Prev in Thread] Current Thread [Next in Thread>