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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.02.14 at 13:46, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
>>> +    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.

While I'd prefer the latter, perhaps slightly adjusted to not as
heavily special case -ENOSYS (i.e. call the second function if any
error was returned from the first, and use the error value that
was not -ENOSYS unless both functions returned it).

>>> +#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"?

Exactly. All you need in this header are forward declarations of two
struct-s.

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

But then again this could easily be dealt with in ARM (providing a
no-op stub) or the generic code (checking the pointer to be non-
NULL), allowing future ports (or ARM itself, should it ever need to)
to use it.

Jan


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