linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] powerpc/iommu/ddw: Fix endianness
@ 2014-09-09 11:22 Alexey Kardashevskiy
  2014-09-15  8:41 ` Alexey Kardashevskiy
  0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2014-09-09 11:22 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Nishanth Aravamudan, Alistair Popple, Anton Blanchard,
	Gavin Shan, linuxppc-dev, linux-kernel, Alexander Graf

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 <agraf@suse.de>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 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


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/iommu/ddw: Fix endianness
  2014-09-09 11:22 [PATCH] powerpc/iommu/ddw: Fix endianness Alexey Kardashevskiy
@ 2014-09-15  8:41 ` Alexey Kardashevskiy
  2014-09-15  9:18   ` Michael Ellerman
  0 siblings, 1 reply; 4+ messages in thread
From: Alexey Kardashevskiy @ 2014-09-15  8:41 UTC (permalink / raw)
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Nishanth Aravamudan, Alistair Popple, Anton Blanchard,
	Gavin Shan, linuxppc-dev, linux-kernel, Alexander Graf

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 <agraf@suse.de>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>



Ping, anyone?



> ---
>  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:
> 


-- 
Alexey

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/iommu/ddw: Fix endianness
  2014-09-15  8:41 ` Alexey Kardashevskiy
@ 2014-09-15  9:18   ` Michael Ellerman
  2014-09-15 10:26     ` Alexey Kardashevskiy
  0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2014-09-15  9:18 UTC (permalink / raw)
  To: Alexey Kardashevskiy
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Nishanth Aravamudan,
	Alistair Popple, Anton Blanchard, Gavin Shan, linuxppc-dev,
	linux-kernel, Alexander Graf

On Mon, 2014-09-15 at 18:41 +1000, Alexey Kardashevskiy wrote:
> On 09/09/2014 09:22 PM, Alexey Kardashevskiy wrote:
> > rtas_call() accepts and returns values in CPU endianness.

Sounds right.

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

Also sounds right.

> > 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?

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

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




^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] powerpc/iommu/ddw: Fix endianness
  2014-09-15  9:18   ` Michael Ellerman
@ 2014-09-15 10:26     ` Alexey Kardashevskiy
  0 siblings, 0 replies; 4+ messages in thread
From: Alexey Kardashevskiy @ 2014-09-15 10:26 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Benjamin Herrenschmidt, Paul Mackerras, Nishanth Aravamudan,
	Alistair Popple, Anton Blanchard, Gavin Shan, linuxppc-dev,
	linux-kernel, Alexander Graf

On 09/15/2014 07:18 PM, Michael Ellerman wrote:
> On Mon, 2014-09-15 at 18:41 +1000, Alexey Kardashevskiy wrote:
>> On 09/09/2014 09:22 PM, Alexey Kardashevskiy wrote:
>>> rtas_call() accepts and returns values in CPU endianness.
> 
> Sounds right.
> 
>>> of_read_number() accepts big-endian values but create.addr_hi/lo returned
>>> by rtas_call() are in CPU endiannes.
> 
> Also sounds right.
> 
>>> 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.


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



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-09-15 10:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-09 11:22 [PATCH] powerpc/iommu/ddw: Fix endianness Alexey Kardashevskiy
2014-09-15  8:41 ` Alexey Kardashevskiy
2014-09-15  9:18   ` Michael Ellerman
2014-09-15 10:26     ` Alexey Kardashevskiy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).