Re: [Xen-devel] [PATCH for-4.12] arm/p2m: do not invalidate p2m root if it is shared with IOMMU

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 1/28/19 3:34 PM, Andrii Anisov wrote:
> From: Andrii Anisov <andrii_anisov@xxxxxxxx>
>
> In case if the p2m table is shared to IOMMU, invalidating it turns IOMMU to
> translation faults that could be not repaired.
>
> Fixed patch check for the corresponded condition and has a comment for one
> introduced p2m_invalidate_root() call, but miss them for another. So put the
> `if` and the comment in place.
This was missed on purpose. Let me explain why. The call to 
p2m_invalidate_root() arch_domain_creation_finished is called by *all* 
the domain at boot to try to optimize the set/way case.
The check iommu_use_hap_pt in that context is to prevent guest not using 
Set/Way to become unusable under the IOMMU use-case.
In your case, you seem to have a guest OS using set/way and yet sharing 
the P2M with the IOMMU. You have the choice between:
        1) Crashing on IOMMU fault
	2) Become very slow and potentially unusable because you now have to go 
through the full P2M every time you do a Set/Way.
1) was my favored option because Set/Way should really not be used by 
the guest. It was implemented by courtesy to the guest OS and I would 
not rely on everything working.
Can you explain what is your use-case (OS used, IOMMU, platform...)?

Cheers,

> Fixes: 2148a12 ("xen/arm: Track page accessed between batch of Set/Way 
> operations")
> Signed-off-by: Andrii Anisov <andrii_anisov@xxxxxxxx>
> ---
>   xen/arch/arm/p2m.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 059a391..2367e09 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1708,8 +1708,13 @@ void p2m_flush_vm(struct vcpu *v)
>       /*
>        * Invalidate the p2m to track which page was modified by the guest
>        * between call of p2m_flush_vm().
> +     *
> +     * This is only turned when IOMMU is not used or the page-table are
> +     * not shared because bit[0] (e.g valid bit) unset will result
> +     * IOMMU fault that could be not fixed-up.
>        */
> -    p2m_invalidate_root(p2m);
> +    if ( !iommu_use_hap_pt(v->domain) )
> +        p2m_invalidate_root(p2m);
>   
>       v->arch.need_flush_to_ram = false;
>   }
--
Julien Grall

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