Re: [PATCH kernel 2/2] KVM: PPC: Allow backing bigger guest IOMMU pages with smaller physical pages

On Wed, May 2, 2018 at 4:26 PM, Alexey Kardashevskiy [off-list ref] wrote:

On 2/5/18 3:53 pm, Balbir Singh wrote:

quoted

On Wed,  2 May 2018 14:07:23 +1000
Alexey Kardashevskiy [off-list ref] wrote:

quoted

At the moment we only support in the host the IOMMU page sizes which
the guest is aware of, which is 4KB/64KB/16MB. However P9 does not support
16MB IOMMU pages, 2MB and 1GB pages are supported instead. We can still
emulate bigger guest pages (for example 16MB) with smaller host pages
(4KB/64KB/2MB).

This allows the physical IOMMU pages to use a page size smaller or equal
than the guest visible IOMMU page size.

Signed-off-by: Alexey Kardashevskiy <redacted>
---
 arch/powerpc/kvm/book3s_64_vio.c    | 66 +++++++++++++++++++++++++++++--------
 arch/powerpc/kvm/book3s_64_vio_hv.c | 52 +++++++++++++++++++++++++----
 2 files changed, 98 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
index 041e54d..e10d6a3 100644
--- a/arch/powerpc/kvm/book3s_64_vio.c
+++ b/arch/powerpc/kvm/book3s_64_vio.c

@@ -176,14 +176,12 @@ extern long kvm_spapr_tce_attach_iommu_group(struct kvm *kvm, int tablefd,

             if (!tbltmp)
                     continue;
-            /*
-             * Make sure hardware table parameters are exactly the same;
-             * this is used in the TCE handlers where boundary checks
-             * use only the first attached table.
-             */
-            if ((tbltmp->it_page_shift == stt->page_shift) &&
-                            (tbltmp->it_offset == stt->offset) &&
-                            (tbltmp->it_size == stt->size)) {
+            /* Make sure hardware table parameters are compatible */
+            if ((tbltmp->it_page_shift <= stt->page_shift) &&
+                            (tbltmp->it_offset << tbltmp->it_page_shift ==
+                             stt->offset << stt->page_shift) &&
+                            (tbltmp->it_size << tbltmp->it_page_shift ==
+                             stt->size << stt->page_shift)) {

Very difficult to parse

How about -

subpages_shift = (stt->page_shift - tbl->it_page_shift)

and then

matches_offset = !(tbltmp->it_offset - (stt->it_offset << subpages_shift));
matches_size = !(tbltmp->it_size - (stt->it_size << subpages_shift));

The condition then is just

              if ((tbltmp->it_page_shift == stt->page_shift) &&
                              matches_offset && matches_size) {

This is harder to parse. My variant is bulky but straight forward otherwise.

OK, I don't read this code as often, but found it_page_shift and page_shift
variants hard to understand, but I guess it means I need to read more code.

quoted

quoted

                     /*
                      * Reference the table to avoid races with
                      * add/remove DMA windows.

@@ -396,7 +394,7 @@ static long kvmppc_tce_iommu_mapped_dec(struct kvm *kvm,
     return H_SUCCESS;
 }

-static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
+static long kvmppc_tce_iommu_do_unmap(struct kvm *kvm,
             struct iommu_table *tbl, unsigned long entry)
 {
     enum dma_data_direction dir = DMA_NONE;

@@ -416,7 +414,25 @@ static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
     return ret;
 }

-long kvmppc_tce_iommu_map(struct kvm *kvm, struct iommu_table *tbl,
+static long kvmppc_tce_iommu_unmap(struct kvm *kvm,
+            struct kvmppc_spapr_tce_table *stt, struct iommu_table *tbl,
+            unsigned long entry)
+{
+    unsigned long ret = H_SUCCESS;
+    unsigned long subpages = 1ULL << (stt->page_shift - tbl->it_page_shift);
+    unsigned long io_entry = entry * subpages;

How do we know this multiplication does not overflow?

Entry, page_shift come from the userspace and checked in appropriate
ioctl/hcall handlers.

Yes, I am not sure how many safety nets we need, but I worry about
such things :)

quoted

quoted

+    unsigned long subpg;

Why not just i?

I can imagine pages so huge so backing them with 4K will overflow 32bit
anyway. It is very (very) unlikely but it is 64bit arch anyway and there is
no much point in not-long types anyway.

What David said, i is an easy iterator to understand :)

Balbir Singh.