[Qemu-devel] [PATCH v2 0/2] spapr_iommu: TCE permission bits fixes

[Qemu-devel] [PATCH v2 0/2] spapr_iommu: TCE permission bits fixes

[Greg Kurz]

This series fixes some minor nits in the sPAPR IOMMU code. It supercedes
my previous post:

"spapr_iommu: drop erroneous check in h_put_tce_indirect() and useless enum"

---

Greg Kurz (2):
      spapr_iommu: drop erroneous check in h_put_tce_indirect()
      spapr_iommu: translate sPAPRTCEAccess to IOMMUAccessFlags


 hw/ppc/spapr_iommu.c |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

--
Greg

[Qemu-devel] [PATCH v2 1/2] spapr_iommu: drop erroneous check in h_put_tce_indirect()

[Greg Kurz]

The tce_list variable is not a TCE but the address to a TCE: we shouldn't
clear permission bits as we do now. And this is dead code anyway since we
check tce_list is 4K aligned a few lines above.

This patch doesn't fix any bug, it is only code cleanup.

Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_iommu.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 8cd9dba9ac4d..ddd0ea5cd4dd 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -267,9 +267,7 @@ static target_ulong h_put_tce_indirect(PowerPCCPU *cpu,
     ioba &= page_mask;
 
     for (i = 0; i < npages; ++i, ioba += page_size) {
-        target_ulong off = (tce_list & ~SPAPR_TCE_RW) +
-                                i * sizeof(target_ulong);
-        tce = ldq_be_phys(cs->as, off);
+        tce = ldq_be_phys(cs->as, tce_list + i * sizeof(target_ulong));
 
         ret = put_tce_emu(tcet, ioba, tce);
         if (ret) {

Re: [Qemu-devel] [PATCH v2 1/2] spapr_iommu: drop erroneous check in h_put_tce_indirect()

[Greg Kurz]

On Tue, 16 Jun 2015 18:26:47 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> The tce_list variable is not a TCE but the address to a TCE: we shouldn't
> clear permission bits as we do now. And this is dead code anyway since we
> check tce_list is 4K aligned a few lines above.
> 
> This patch doesn't fix any bug, it is only code cleanup.
> 
> Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_iommu.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Of course, I did forget the changelog...

v2:
- suggested by aik
- keep sPAPRTCEAccess, a translation helper is introduced in the next
  patch

> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 8cd9dba9ac4d..ddd0ea5cd4dd 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -267,9 +267,7 @@ static target_ulong h_put_tce_indirect(PowerPCCPU *cpu,
>      ioba &= page_mask;
> 
>      for (i = 0; i < npages; ++i, ioba += page_size) {
> -        target_ulong off = (tce_list & ~SPAPR_TCE_RW) +
> -                                i * sizeof(target_ulong);
> -        tce = ldq_be_phys(cs->as, off);
> +        tce = ldq_be_phys(cs->as, tce_list + i * sizeof(target_ulong));
> 
>          ret = put_tce_emu(tcet, ioba, tce);
>          if (ret) {
> 
> 

[Qemu-devel] [PATCH v2 2/2] spapr_iommu: translate sPAPRTCEAccess to IOMMUAccessFlags

[Greg Kurz]

The fact that these enums have matching values is pure coincidence. We
actually need to translate from the PAPR definition to the QEMU one.

This patch doesn't fix any bug, it is only code cleanup.

Suggested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_iommu.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index ddd0ea5cd4dd..9133912bb983 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -60,6 +60,20 @@ sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn)
     return NULL;
 }
 
+static IOMMUAccessFlags spapr_tce_iommu_access_flags(uint64_t tce)
+{
+    switch (tce & SPAPR_TCE_RW) {
+    case SPAPR_TCE_FAULT:
+        return IOMMU_NONE;
+    case SPAPR_TCE_RO:
+        return IOMMU_RO;
+    case SPAPR_TCE_WO:
+        return IOMMU_WO;
+    default: /* SPAPR_TCE_RW */
+        return IOMMU_RW;
+    }
+}
+
 /* Called from RCU critical section */
 static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
                                                bool is_write)
@@ -82,7 +96,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
         ret.iova = addr & page_mask;
         ret.translated_addr = tce & page_mask;
         ret.addr_mask = ~page_mask;
-        ret.perm = tce & IOMMU_RW;
+        ret.perm = spapr_tce_iommu_access_flags(tce);
     }
     trace_spapr_iommu_xlate(tcet->liobn, addr, ret.iova, ret.perm,
                             ret.addr_mask);
@@ -233,7 +247,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,
     entry.iova = ioba & page_mask;
     entry.translated_addr = tce & page_mask;
     entry.addr_mask = ~page_mask;
-    entry.perm = tce & IOMMU_RW;
+    entry.perm = spapr_tce_iommu_access_flags(tce);
     memory_region_notify_iommu(&tcet->iommu, entry);
 
     return H_SUCCESS;

Re: [Qemu-devel] [PATCH v2 0/2] spapr_iommu: TCE permission bits fixes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Tue, Jun 16, 2015 at 06:26:39PM +0200, Greg Kurz wrote:
> This series fixes some minor nits in the sPAPR IOMMU code. It supercedes
> my previous post:
> 
> "spapr_iommu: drop erroneous check in h_put_tce_indirect() and
> useless enum"

Thanks, applied to spapr-next.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]