[PATCH] powerpc/iommu/ddw: Fix endianness

rtas_call() accepts and returns values in CPU endianness.

of_read_number() accepts big-endian values but create.addr_hi/lo returned
by rtas_call() are in CPU endiannes.

The dynamic_dma_window_prop struct defines all members as BE so let's
make it true.

struct dynamic_dma_window_prop {
        __be32  liobn;          /* tce table number */
        __be64  dma_base;       /* address hi,lo */
        __be32  tce_shift;      /* ilog2(tce_page_size) */
        __be32  window_shift;   /* ilog2(tce_window_size) */
};

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Alexander Graf <redacted>
Signed-off-by: Alexey Kardashevskiy <redacted>
---
 arch/powerpc/platforms/pseries/iommu.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 7c1d77c..700020a 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c

@@ -750,7 +750,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
 		pr_debug("%s successfully cleared tces in window.\n",
 			 np->full_name);
 
-	ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+	ret = rtas_call(be32_to_cpu(ddw_avail[2]), 1, 1, NULL, liobn);
 	if (ret)
 		pr_warning("%s: failed to remove direct window: rtas returned "
 			"%d to ibm,remove-pe-dma-window(%x) %llx\n",

@@ -842,7 +842,7 @@ static int query_ddw(struct pci_dev *dev, const u32 *ddw_avail,
 		cfg_addr = edev->pe_config_addr;
 	buid = edev->phb->buid;
 
-	ret = rtas_call(ddw_avail[0], 3, 5, (u32 *)query,
+	ret = rtas_call(be32_to_cpu(ddw_avail[0]), 3, 5, (u32 *)query,
 		  cfg_addr, BUID_HI(buid), BUID_LO(buid));
 	dev_info(&dev->dev, "ibm,query-pe-dma-windows(%x) %x %x %x"
 		" returned %d\n", ddw_avail[0], cfg_addr, BUID_HI(buid),

@@ -874,8 +874,9 @@ static int create_ddw(struct pci_dev *dev, const u32 *ddw_avail,
 
 	do {
 		/* extra outputs are LIOBN and dma-addr (hi, lo) */
-		ret = rtas_call(ddw_avail[1], 5, 4, (u32 *)create, cfg_addr,
-				BUID_HI(buid), BUID_LO(buid), page_shift, window_shift);
+		ret = rtas_call(be32_to_cpu(ddw_avail[1]), 5, 4, (u32 *)create,
+				cfg_addr, BUID_HI(buid), BUID_LO(buid),
+				page_shift, window_shift);
 	} while (rtas_busy_delay(ret));
 	dev_info(&dev->dev,
 		"ibm,create-pe-dma-window(%x) %x %x %x %x %x returned %d "

@@ -972,11 +973,11 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 		dev_dbg(&dev->dev, "no free dynamic windows");
 		goto out_failed;
 	}
-	if (be32_to_cpu(query.page_size) & 4) {
+	if (query.page_size & 4) {
 		page_shift = 24; /* 16MB */
-	} else if (be32_to_cpu(query.page_size) & 2) {
+	} else if (query.page_size & 2) {
 		page_shift = 16; /* 64kB */
-	} else if (be32_to_cpu(query.page_size) & 1) {
+	} else if (query.page_size & 1) {
 		page_shift = 12; /* 4kB */
 	} else {
 		dev_dbg(&dev->dev, "no supported direct page size in mask %x",

@@ -987,7 +988,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 	/* verify the window * number of ptes will map the partition */
 	/* check largest block * page size > max memory hotplug addr */
 	max_addr = memory_hotplug_max();
-	if (be32_to_cpu(query.largest_available_block) < (max_addr >> page_shift)) {
+	if (query.largest_available_block < (max_addr >> page_shift)) {
 		dev_dbg(&dev->dev, "can't map partiton max 0x%llx with %u "
 			  "%llu-sized pages\n", max_addr,  query.largest_available_block,
 			  1ULL << page_shift);

@@ -1014,8 +1015,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 	if (ret != 0)
 		goto out_free_prop;
 
-	ddwprop->liobn = create.liobn;
-	ddwprop->dma_base = cpu_to_be64(of_read_number(&create.addr_hi, 2));
+	ddwprop->liobn = cpu_to_be32(create.liobn);
+	ddwprop->dma_base = cpu_to_be64(((u64)create.addr_hi << 32) |
+			create.addr_lo);
 	ddwprop->tce_shift = cpu_to_be32(page_shift);
 	ddwprop->window_shift = cpu_to_be32(len);
 

@@ -1048,7 +1050,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
 	list_add(&window->list, &direct_window_list);
 	spin_unlock(&direct_window_list_lock);
 
-	dma_addr = of_read_number(&create.addr_hi, 2);
+	dma_addr = be64_to_cpu(ddwprop->dma_base);
 	goto out_unlock;
 
 out_free_window:

-- 
2.0.0

On 09/09/2014 09:22 PM, Alexey Kardashevskiy wrote:

rtas_call() accepts and returns values in CPU endianness.

of_read_number() accepts big-endian values but create.addr_hi/lo returned
by rtas_call() are in CPU endiannes.

The dynamic_dma_window_prop struct defines all members as BE so let's
make it true.

struct dynamic_dma_window_prop {
        __be32  liobn;          /* tce table number */
        __be64  dma_base;       /* address hi,lo */
        __be32  tce_shift;      /* ilog2(tce_page_size) */
        __be32  window_shift;   /* ilog2(tce_window_size) */
};

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Alexander Graf <redacted>
Signed-off-by: Alexey Kardashevskiy <redacted>

Ping, anyone?

-- 
Alexey

On Mon, 2014-09-15 at 18:41 +1000, Alexey Kardashevskiy wrote:

On 09/09/2014 09:22 PM, Alexey Kardashevskiy wrote:

quoted

rtas_call() accepts and returns values in CPU endianness.

Sounds right.

quoted

of_read_number() accepts big-endian values but create.addr_hi/lo returned
by rtas_call() are in CPU endiannes.

Also sounds right.

quoted

The dynamic_dma_window_prop struct defines all members as BE so let's
make it true.

It does. But why does it do that? It seems to be allocated and setup in
enable_ddw() and then the only place I see it used is in
tce_setrange_multi_pSeriesLP()/tce_clearrange_multi_pSeriesLP(), which both
unpack it again. What am I missing?

quoted

struct dynamic_dma_window_prop {
        __be32  liobn;          /* tce table number */
        __be64  dma_base;       /* address hi,lo */
        __be32  tce_shift;      /* ilog2(tce_page_size) */
        __be32  window_shift;   /* ilog2(tce_window_size) */
};

We do read them from the device tree in find_existing_ddw_windows(), but if
that's the only place then the conversion to cpu endian should happen there.

quoted

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 7c1d77c..700020a 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c

@@ -750,7 +750,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
 		pr_debug("%s successfully cleared tces in window.\n",
 			 np->full_name);
 
-	ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+	ret = rtas_call(be32_to_cpu(ddw_avail[2]), 1, 1, NULL, liobn);

The conversion should happen once where ever ddw_avail comes out of the device
tree, rather than everywhere it's used.

cheers

On 09/15/2014 07:18 PM, Michael Ellerman wrote:

On Mon, 2014-09-15 at 18:41 +1000, Alexey Kardashevskiy wrote:

quoted

On 09/09/2014 09:22 PM, Alexey Kardashevskiy wrote:

quoted

rtas_call() accepts and returns values in CPU endianness.

Sounds right.

quoted

quoted

of_read_number() accepts big-endian values but create.addr_hi/lo returned
by rtas_call() are in CPU endiannes.

Also sounds right.

quoted

quoted

The dynamic_dma_window_prop struct defines all members as BE so let's
make it true.

It does. But why does it do that? It seems to be allocated and setup in
enable_ddw() and then the only place I see it used is in
tce_setrange_multi_pSeriesLP()/tce_clearrange_multi_pSeriesLP(), which both
unpack it again. What am I missing?

I do not know why they are BE. I just know that create_ddw() returns
ddw_create_response struct which members are declared as BE but they are
not as rtas_call() already made them CPU-endian.

May be rtas_call() must not be used for structs. Or these structs must be
fixed to be CPU endian. Cannot choose what/how to fix here. Sure I still
can miss something here and it is all correct and I have to fix QEMU.

quoted

quoted

struct dynamic_dma_window_prop {
        __be32  liobn;          /* tce table number */
        __be64  dma_base;       /* address hi,lo */
        __be32  tce_shift;      /* ilog2(tce_page_size) */
        __be32  window_shift;   /* ilog2(tce_window_size) */
};

We do read them from the device tree in find_existing_ddw_windows(), but if
that's the only place then the conversion to cpu endian should happen there.

enable_ddw() kmalloc's ddwprop which is of this dynamic_dma_window_prop type.

Then enable_ddw() initializes properties of that ddwprop thing but does it
incorrectly.

Then it calls walk_system_ram_range() which eventually calls
tce_setrange_multi_pSeriesLP() to map every single page of guest's ram, the
dynamic_dma_window_prop struct pointer is a void* argument of that callback.



find_existing_ddw_windows() handles something called
"linux,direct64-ddr-window-info" which is not a part of DDW at all and it
is not from PAPR and this patch is not about it.

quoted

quoted

diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 7c1d77c..700020a 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c

@@ -750,7 +750,7 @@ static void remove_ddw(struct device_node *np, bool remove_prop)
 		pr_debug("%s successfully cleared tces in window.\n",
 			 np->full_name);
 
-	ret = rtas_call(ddw_avail[2], 1, 1, NULL, liobn);
+	ret = rtas_call(be32_to_cpu(ddw_avail[2]), 1, 1, NULL, liobn);

The conversion should happen once where ever ddw_avail comes out of the device
tree, rather than everywhere it's used.

ddw_avail is a pointer to device tree property value:
ddw_avail = of_get_property(np, "ibm,ddw-applicable", &len);

This contains 3 tokens, only 2 of them are actually used by the DDW code in
its current state and each of them is used just once in the guest's
lifetime. Older guest kernels would use the "reset" extension token but
again - only once. I fail to see the point in caching CPU-endian values of
these tokens.

cheers

-- 
Alexey