Re: [Xen-devel] [PATCH] iommu: avoid triggering ASSERT_UNREACHABLE() on ARM...

[Julien Grall <julien.grall@xxxxxxx>]

Hi Paul,

On 9/26/19 11:03 AM, Paul Durrant wrote:
> ...when the IOMMU is not enabled.
>
> 80ff3d338dc9 "iommu: tidy up iommu_use_hap_pt() and need_iommu_pt_sync()
> macros" introduced CONFIG_IOMMU_FORCE_PT_SHARE, which causes the global
> 'iommu_hap_pt_share' option to be replaced with a #define-d value of true.
> In this configuration, calling clear_iommu_hap_pt_share() will result
> trigger the aforementioned ASSERT.
>
> CONFIG_IOMMU_FORCE_PT_SHARE is always selected for ARM builds and,
> because clear_iommu_hap_pt_share() is called by the common iommu_setup()
> function if the IOMMU is not enabled, it is no longer safe to disable the
> IOMMU on ARM systems.
>
> However, running with the IOMMU disabled is a valid configuration for ARM
> systems, so this patch rectifies the problem by removing the call to
> clear_iommu_hap_pt_share() from common code. As a side effect of this,
> however, it becomes possible on x86 systems for iommu_enabled to be false
> but iommu_hap_pt_share to be true. Thus the code in sysctl.c
> needs to be changed to make sure that XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share
> is not erroneously advertised when the IOMMU has been disabled.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Reported-by: Oleksandr <olekstysh@xxxxxxxxx>
With one NIT below:

Acked-by: Julien Grall <julien.grall@xxxxxxx>

> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wl@xxxxxxx>
> ---
>   xen/common/sysctl.c             | 6 ++++--
>   xen/drivers/passthrough/iommu.c | 1 -
>   2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index e8763c7fdf..f88a285e7f 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -268,9 +268,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>           pi->max_mfn = get_upper_mfn_bound();
>           arch_do_physinfo(pi);
>           if ( iommu_enabled )
> +        {
>               pi->capabilities |= XEN_SYSCTL_PHYSCAP_directio;
> -        if ( iommu_hap_pt_share )
> -            pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> +            if ( iommu_hap_pt_share )
> +                pi->capabilities |= XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share;
> +        }
>   
>           if ( copy_to_guest(u_sysctl, op, 1) )
>               ret = -EFAULT;
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index e8fddc2dc7..c33ca70ec9 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -456,7 +456,6 @@ int __init iommu_setup(void)
>       if ( !iommu_enabled )
>       {
>           iommu_intremap = 0;
> -        clear_iommu_hap_pt_share();
>       }
NIT: The {} can now be removed.

I can fix it on commit.

Cheers,

--
Julien Grall

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