| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 13 Dec 2010 11:28:09 -0800
From: Jeremy Fitzhardinge <jeremy@...p.org>
To: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
CC: xen-devel@...ts.xensource.com, Jan Beulich <JBeulich@...ell.com>,
linux-kernel@...r.kernel.org,
Konrad Rzeszutek Wilk <konrad@...nel.org>,
Stefano Stabellini <stefano.stabellini@...citrix.com>
Subject: Re: [PATCH 2/9] xen/pci: Add xen_[find|register|unregister]_device_domain_owner
functions.
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@...cle.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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists