Re: [Xen-devel] [PATCH v2 08/15] xen/passthrough: iommu: Split generic IOMMU code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.02.14 at 23:16, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> +int iommu_do_domctl(
> +    struct xen_domctl *domctl, struct domain *d,
> +    XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> -    const struct iommu_ops *ops = iommu_get_ops();
> -    if ( iommu_intremap )
> -        ops->read_msi_from_ire(msi_desc, msg);
> -}
> +    int ret = 0;
>  
> -unsigned int iommu_read_apic_from_ire(unsigned int apic, unsigned int reg)
> -{
> -    const struct iommu_ops *ops = iommu_get_ops();
> -    return ops->read_apic_from_ire(apic, reg);
> -}
> +    if ( !iommu_enabled )
> +        return -ENOSYS;
>  
> -int __init iommu_setup_hpet_msi(struct msi_desc *msi)
> -{
> -    const struct iommu_ops *ops = iommu_get_ops();
> -    return ops->setup_hpet_msi ? ops->setup_hpet_msi(msi) : -ENODEV;
> -}
> +    switch ( domctl->cmd )
> +    {
> +#ifdef HAS_PCI
> +    case XEN_DOMCTL_get_device_group:
> +    case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_assign_device:
> +    case XEN_DOMCTL_deassign_device:
> +        ret = iommu_do_pci_domctl(domctl, d, u_domctl);
> +        break;
> +#endif
> +    default:
> +        ret = -ENOSYS;
> +    }

Please simply have the default case chain to iommu_do_pci_domctl(),
avoiding the need to change two source files when new sub-ops get
added.

Also, last case in the set of case statements or not - the default
case should also have a break statement.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
>...
> @@ -980,6 +983,440 @@ static int __init setup_dump_pcidevs(void)
>  }
>  __initcall(setup_dump_pcidevs);
>  
> +static int iommu_populate_page_table(struct domain *d)
> +{

Now why is this function being moved here? It doesn't appear to
have anything PCI specific at all.

> --- /dev/null
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -0,0 +1,65 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/iommu.h>
> +#include <xen/paging.h>
> +#include <xen/guest_access.h>
> +#include <xen/event.h>
> +#include <xen/softirq.h>
> +#include <xsm/xsm.h>
> +
> +void iommu_update_ire_from_apic(
> +    unsigned int apic, unsigned int reg, unsigned int value)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    ops->update_ire_from_apic(apic, reg, value);
> +}

While one might argue that this one is x86-specific (albeit from past
IA64 days we know it isn't entirely), ...

> +
> +int iommu_update_ire_from_msi(
> +    struct msi_desc *msi_desc, struct msi_msg *msg)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    return iommu_intremap ? ops->update_ire_from_msi(msi_desc, msg) : 0;
> +}

... this one clearly isn't - I'm sure once you support PCI on ARM, you
will also want/need to support MSI. (The same then of course goes
for the respective functions' declarations.)

> --- /dev/null
> +++ b/xen/include/asm-x86/iommu.h
> @@ -0,0 +1,46 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program; if not, write to the Free Software Foundation, Inc., 59 
> Temple
> + * Place - Suite 330, Boston, MA 02111-1307 USA.
> +*/
> +#ifndef __ARCH_X86_IOMMU_H__
> +#define __ARCH_X86_IOMMU_H__
> +
> +#define MAX_IOMMUS 32
> +
> +#include <asm/msi.h>

Please don't, if at all possible.

> @@ -84,52 +82,56 @@ void free_hvm_irq_dpci(struct hvm_irq_dpci *dpci);
>  bool_t pt_irq_need_timer(uint32_t flags);
>  
>  #define PT_IRQ_TIME_OUT MILLISECS(8)
> +#endif /* HAS_PCI */
>  
> +#ifdef CONFIG_X86
>  struct msi_desc;
>  struct msi_msg;
> +#endif /* CONFIG_X86 */

Hardly - this again is a direct descendant from PCI.

> +#ifdef CONFIG_X86
>      void (*update_ire_from_apic)(unsigned int apic, unsigned int reg, 
> unsigned int value);
>      int (*update_ire_from_msi)(struct msi_desc *msi_desc, struct msi_msg 
> *msg);
>      void (*read_msi_from_ire)(struct msi_desc *msi_desc, struct msi_msg 
> *msg);
>      unsigned int (*read_apic_from_ire)(unsigned int apic, unsigned int reg);
>      int (*setup_hpet_msi)(struct msi_desc *);
> +    void (*share_p2m)(struct domain *d);
> +#endif /* CONFIG_X86 */

Is that last one really x86-specific in any way?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel