Hi,
On 25/09/2019 17:10, Paul Durrant wrote:
>> -----Original Message-----
>> From: Oleksandr <olekst...@gmail.com>
>> Sent: 25 September 2019 16:50
>> To: Paul Durrant <paul.durr...@citrix.com>; 'Jan Beulich' <jbeul...@suse.com>
>> Cc: Petre Pircalabu <ppircal...@bitdefender.com>; Stefano Stabellini
>> <sstabell...@kernel.org>; Wei Liu
>> <w...@xen.org>; KonradRzeszutek Wilk <konrad.w...@oracle.com>; Andrew Cooper
>> <andrew.coop...@citrix.com>; David Scott <d...@recoil.org>; Tim (Xen.org)
>> <t...@xen.org>; George Dunlap
>> <george.dun...@citrix.com>; Tamas K Lengyel <ta...@tklengyel.com>; Ian
>> Jackson
>> <ian.jack...@citrix.com>; Anthony Perard <anthony.per...@citrix.com>;
>> xen-devel@lists.xenproject.org;
>> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Roger Pau Monne
>> <roger....@citrix.com>; Julien Grall
>> <julien.gr...@arm.com>
>> Subject: Re: [Xen-devel] [PATCH v13 0/4] add per-domain IOMMU control
>>
>>
>> [CC Julien]
>>
>>
>> Hi Paul
>>
>> I may mistake, but looks like
>>
>> 80ff3d338dc93260b41ffeeebb0f852c2edef9ce iommu: tidy up
>> iommu_use_hap_pt() and need_iommu_pt_sync() macros
>>
>> triggers ASSERT_UNREACHABLE on Arm if no IOMMU has been found (I built
>> with my platform's IOMMU driver disabled: # CONFIG_IPMMU_VMSA is not set) .
>>
>> So, iommu_setup() calls clear_iommu_hap_pt_share() with
>> iommu_hap_pt_share being set (CONFIG_IOMMU_FORCE_PT_SHARE=y) which,
>> actually, triggers ASSERT.
>>
>
> Here a minimal patch, leaving 'force pt share' in place. Does this avoid the
> problem?
>
> ---8<---
> 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/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 7c3003f3f1..6a10a24128 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -68,8 +68,6 @@ static inline void clear_iommu_hap_pt_share(void)
> {
> #ifndef iommu_hap_pt_share
> iommu_hap_pt_share = false;
> -#elif iommu_hap_pt_share
> - ASSERT_UNREACHABLE();
> #endif
IHMO, calling this function is a mistake on platform only supporting
shared page-table so the ASSERT() should be kept here.
This raises the question why the function is actually called from common
code. iommu_hap_enabled() should technically not be used in any code if
the IOMMU is not enabled/present. So what are you trying to prevent here?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
