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

[Julien Grall <julien.grall@linaro.org>]

Hi Jan,

On 02/24/2014 10:39 AM, Jan Beulich wrote:
>>>> On 23.02.14 at 23:16, Julien Grall <julien.grall@linaro.org> 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.

I wrote in this manner because we will add soon "iommu_do_dt_domctl" to
handle DOMCTL for device tree assignment.

For one of this function we will have to deal with this trick. Or ... we
can do:

   ret = iommu_do_pci_domctl(...)
   if ( ret != ENOSYS )
     return ret;
   ret = iommu_do_dt_domctl(...)

IHMO, I would prefer the former solution.

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

I will add it.

>> --- 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.

Because this function is only used in assign_device.

I also remember this function can't work on ARM (arch.relmem_list
doesn't exists). I will move this code in x86/iommu.c because it's seems
architecture initialization.

> 
>> --- /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.)

Right, I guess it's the same for iommu_read_msi_from_ire.

>> --- /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.

I'm not sure to understand ... what do you mean? Don't include "asm/msi.h"?

>> @@ -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.

I will #ifdef HAS_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?

On ARM, P2M are share by default. You don't need to call this function
explicitly. So I think we can safely say it's x86-specific.

Developper won't call this function by mistake.

Cheers,

-- 
Julien Grall