Re: [PATCH 0/2] Dangling fixes for ARM iommu

From: Julien Grall <julien.grall@arm.com>
To: Andrii Anisov <andrii.anisov@gmail.com>, xen-devel@lists.xenproject.org
Cc: Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Andrii Anisov <andrii_anisov@epam.com>
Subject: Re: [PATCH 0/2] Dangling fixes for ARM iommu
Date: Tue, 22 Jan 2019 14:31:33 +0000	[thread overview]
Message-ID: <3a213999-d67a-9cff-d3bf-3d4cf5baa5b3@arm.com> (raw)
In-Reply-To: <77aca14e-dd88-855c-4d7f-2e3a600a5e7a@gmail.com>

On 1/22/19 1:48 PM, Andrii Anisov wrote:
> Hello Julien,

Hi,

> On 21.01.19 19:29, Julien Grall wrote:
>> (+ Juergen)
>> All patches candidate for Xen 4.12 should have the release manager 
>> CCed and explain the pros/cons to have those patches for this release. 
>> It is also useful if you add for-4.12 (or similar) in the [...] so we 
>> can prioritize it.
> 
> I've got the point. Thank you for adding Juergen.
> 
>> Indeed code base evolved, for instance iommu_use_hap_pt has a slight 
>> different behavior now.
> 
> Actually you properly pointed the problem. I'm not sure how it happened, 
> because I did test build and run for those patches yesterday. But now, 
> progressing with our stuff porting, I realized that the patch 1 is 
> missing dependency on `xen/sched.h` required by `iommu_use_hap_pt`.
> 
>> So please stripped my RB
> OK.
> 
>> The AB is arguable as it is quite old.
> I could drop it for v2.

Yes please. But see below.

> 
>> Secondly, it often happens that some patches have the required 
>> acked-by and reviewed-by but are not merged because they makes little 
>> sense without the rest of the series. 
> 
> Yes, sure, but fixes are still relevant and are quite autonomous. The 
> only relation to the series is that they were discovered during its 
> implementation.

A fix would be if you found a bug in a configuration that we are meant 
to support.

> 
>> In that case, the current code base does not support the case where 
>> the P2M is not shared with the IOMMU and does not support new IOMMU 
>> bindings in the current setup.
> 
> It is the intention of the rest of the series.

The rest of series is not ready and not targeting 4.12. So why should we 
get them merged?

> 
>> So I am not convinced they should be included in Xen 4.12 or even 
>> without the rest of the series.
> IMO, the pure fixes, like patch 2, and the first hunk of patch 1 should 
> be OK for 4.12.

The first hunk of patch 1 aside, we never supported the new IOMMU 
bindings nor unsharing the P2M. So what do you actually fix?

Supporting unshared P2M will require more work given because the code 
relies on mfn_to_gmfn that is not implemented on Arm. So accepting this 
patch standalone would be misleading as the rest of the series is not 
going to be merged in Xen 4.12.

Similarly, we don't support new IOMMU bindings. The patch #2 alone is 
going to add more trouble as now Dom0 would not be able to use the IOMMU 
if it were not hidden.

So I still question the usefulness of those 2 patches outside of the series.

Cheers,

-- 
Julien Grall

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