[Qemu-devel] [PATCH] spapr_iommu: drop erroneous check in h_put_tce_indirect() and useless enum

[Qemu-devel] [PATCH] spapr_iommu: drop erroneous check in h_put_tce_indirect() and useless enum

[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.

While we're here, since we drop its last user, let's kill the sPAPRTCEAccess
enum as well.

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

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 hw/ppc/spapr_iommu.c |   11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 8cd9dba9ac4d..2ae8684ca5bc 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -29,13 +29,6 @@
 
 #include <libfdt.h>
 
-enum sPAPRTCEAccess {
-    SPAPR_TCE_FAULT = 0,
-    SPAPR_TCE_RO = 1,
-    SPAPR_TCE_WO = 2,
-    SPAPR_TCE_RW = 3,
-};
-
 #define IOMMU_PAGE_SIZE(shift)      (1ULL << (shift))
 #define IOMMU_PAGE_MASK(shift)      (~(IOMMU_PAGE_SIZE(shift) - 1))
 
@@ -267,9 +260,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] spapr_iommu: drop erroneous check in h_put_tce_indirect() and useless enum

[Greg Kurz]

On Tue, 16 Jun 2015 12:09:33 +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.
> 
> While we're here, since we drop its last user, let's kill the sPAPRTCEAccess
> enum as well.
> 

According to Alexey's feedback (see link below), we should convert SPAPR_TCE_xxx
to IOMMU_xxx instead of dropping sPAPRTCEAccess.

https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg04249.html

Please ignore this patch, I'll send another one.

--
Greg

> This patch doesn't fix any bug, it is only code cleanup.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_iommu.c |   11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 8cd9dba9ac4d..2ae8684ca5bc 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -29,13 +29,6 @@
> 
>  #include <libfdt.h>
> 
> -enum sPAPRTCEAccess {
> -    SPAPR_TCE_FAULT = 0,
> -    SPAPR_TCE_RO = 1,
> -    SPAPR_TCE_WO = 2,
> -    SPAPR_TCE_RW = 3,
> -};
> -
>  #define IOMMU_PAGE_SIZE(shift)      (1ULL << (shift))
>  #define IOMMU_PAGE_MASK(shift)      (~(IOMMU_PAGE_SIZE(shift) - 1))
> 
> @@ -267,9 +260,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) {
> 
>