[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v1 08/10] iommu: Split iommu_hwdom_init() into arch specific parts



Hi Jan,

On 15/05/2017 08:20, Jan Beulich wrote:
On 12.05.17 at 17:34, <JBeulich@xxxxxxxx> wrote:
On 12.05.17 at 17:25, <olekstysh@xxxxxxxxx> wrote:
On Fri, May 12, 2017 at 5:41 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
On 10.05.17 at 16:03, <olekstysh@xxxxxxxxx> wrote:
The "retrieving mapping" code has never executed since
iommu_use_hap_pt(d) always returned true on ARM so far. But, with
introducing the non-shared IOMMU patch series we can no longer keep
this code as is due to the lack of M2P support.

In order to retain the current behavior for x86 this code was completely
moved to x86 specific part.
For ARM we just need to populate IOMMU page table if need_iommu flag
is already set and the IOMMU is non-shared.

So, the logic on ARM was changed a bit, but no functional change for x86.

Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
CC: Jan Beulich <jbeulich@xxxxxxxx>
CC: Julien Grall <julien.grall@xxxxxxx>

---
   Changes in V1:
      - Clarify patch description.

My prior objection stands: You don't make clear why architecture-
agnostic code needs to be moved into an architecture-specific file.

Is the following description more cleaner?

Although this code looks like architecture-agnostic code, there is
only one thing that prevents it
to be *completely* architecture-agnostic -> mfn_to_gmfn helper.
As we don't have an M2P on ARM, it always returns incoming mfn.
And if domain is not direct mapped we will get into the trouble using that.

We didn't care about this code before because it has never been executed
(iommu_use_hap_pt(d) always returned true on ARM so far).
But, with introducing the non-shared IOMMU patch series we can no longer keep
this code as is since it will be executed for non-shared IOMMU.
So, move this code to x86-specific file in order not to expose a possible bug.

Yes, this helps.

Having thought about this some more, what's still missing is a
clear explanation why this new need of a non-stub mfn_to_gmfn()
isn't finally enough of a reason to introduce an M2P on ARM. We
currently have several uses already which ARM fakes one way or
another:
- gnttab_shared_gmfn()

This does not use mfn_to_gmfn on ARM.

- gnttab_status_gmfn()

gnttab_status_gmfn() returns 0 so far. I have to look at this one.

- memory_exchange()

Memory exchange does not work on ARM today and will require more work than that. When I looked at the code a couple of years ago, it was possible to drop the call to mfn_to_gmfn().

- getdomaininfo()

We could rework to store the gmfn in arch_domain.

With this I think there's quite a bit of justification needed to keep
going without M2P on ARM.

As said in a previous thread, I made a quick calculation, ARM32 supports up 40-bit PA and IPA (e.g guest address), which means 28-bits of MFN/GFN. The GFN would have to be stored in a 32-bit for alignment, so we would need 2^28 * 4 = 1GiB of virtual address space and potentially physical memory. We don't have 1GB of VA space free on 32-bit right now.

ARM64 currently supports up to 48-bit PA and 48-bit IPA, which means 36-bits of MFN/GFN. The GFN would have to be stored in 64-bit for alignment, so we would need 2^36 * 8 = 512GiB of virtual address space and potentially physical memory. While virtual address space is not a problem, the memory is a problem for embedded platform. We want Xen to be as lean as possible.

So the M2P is not a solution on ARM. A better approach is to drop those calls from common code and replace by something different (as we did for gnttab_shared_mfn).

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.