Re: [Xen-devel] [PATCH 3/5] iommu: move iommu_get_ops() into common code

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.19 at 15:24, <paul.durrant@xxxxxxxxxx> wrote:
> Currently x86 and ARM differ in their implementation for no good reason.
> This patch moves the ARM variant of iommu_get/set_ops() helpers into
> common code and modifies them so they deal with the __initconstrel
> ops structures used by the x86 IOMMU vendor implementations (adding
> __initconstrel to the SMMU code to bring it in line). Consequently, a lack
> of init() method is now taken to mean uninitialized iommu_ops. Also, the
> printk warning in iommu_set_ops() now becomes an ASSERT.

When having submitted the indirect call overhead reduction series
including IOMMU changes for the first time, I was told that the Arm
folks would like to retain the ability to eventually support
heterogeneous IOMMUs (and hence I shouldn't provide patching
infrastructure there). A single global iommu_[gs]et_ops() is sort of
getting in the way of this as well, I think, and hence I'm not sure it
is a desirable step to make this so far Arm-specific arrangement
the general model. At least it would further complicate Arm side
changes towards that (mid / long term?) goal.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -21,6 +21,21 @@
>  #include <xen/keyhandler.h>
>  #include <xsm/xsm.h>
>  
> +static struct iommu_ops __read_mostly iommu_ops;
> +
> +const struct iommu_ops *iommu_get_ops(void)
> +{
> +    return &iommu_ops;
> +}
> +
> +void __init iommu_set_ops(const struct iommu_ops *ops)
> +{
> +    BUG_ON(!ops);
> +
> +    ASSERT(!iommu_ops.init || iommu_ops.init == ops->init);
> +    iommu_ops = *ops;
> +}

I realize that you merely move (and slightly re-arrange) what has
been there, but now that I look at it again I think ops->init should
also be verified to be non-NULL, or else installing such a set of
hooks would effectively revert back to the "no hooks yet" state.

> @@ -33,11 +32,7 @@ int __init iommu_hardware_setup(void)
>      if ( !iommu_init_ops )
>          return -ENODEV;
>  
> -    if ( !iommu_ops.init )
> -        iommu_ops = *iommu_init_ops->ops;
> -    else
> -        /* x2apic setup may have previously initialised the struct. */
> -        ASSERT(iommu_ops.init == iommu_init_ops->ops->init);
> +    iommu_set_ops(iommu_init_ops->ops);

I was specifically asked to add the comment that you get rid of.
While mentioning x2APIC in common code may no be appropriate,
I'm sure this could be worded in a more general way and attached
to the moved check.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel