Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory
       [not found] <Aw: [Linux] [ARM] Granting memory obtained from the DMA API>
@ 2020-08-25  9:31 ` Simon Leiner
  2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
  2020-08-25 23:55   ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Stefano Stabellini
  0 siblings, 2 replies; 17+ messages in thread
From: Simon Leiner @ 2020-08-25  9:31 UTC (permalink / raw)
  To: xen-devel, sstabellini, jgross; +Cc: julien

On some architectures (like ARM), virt_to_gfn cannot be used for
vmalloc'd memory because of its reliance on virt_to_phys. This patch
introduces a check for vmalloc'd addresses and obtains the PFN using
vmalloc_to_pfn in that case.

Signed-off-by: Simon Leiner <simon@leiner.me>
---
 drivers/xen/xenbus/xenbus_client.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
index 786fbb7d8be0..907bcbb93afb 100644
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -379,8 +379,14 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
 	int i, j;
 
 	for (i = 0; i < nr_pages; i++) {
-		err = gnttab_grant_foreign_access(dev->otherend_id,
-						  virt_to_gfn(vaddr), 0);
+		unsigned long gfn;
+
+		if (is_vmalloc_addr(vaddr))
+			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
+		else
+			gfn = virt_to_gfn(vaddr);
+
+		err = gnttab_grant_foreign_access(dev->otherend_id, gfn, 0);
 		if (err < 0) {
 			xenbus_dev_fatal(dev, err,
 					 "granting access to ring page");
-- 
2.24.3 (Apple Git-128)



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

* [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25  9:31 ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Simon Leiner
@ 2020-08-25  9:31   ` Simon Leiner
  2020-08-25 12:16     ` Jan Beulich
                       ` (2 more replies)
  2020-08-25 23:55   ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Stefano Stabellini
  1 sibling, 3 replies; 17+ messages in thread
From: Simon Leiner @ 2020-08-25  9:31 UTC (permalink / raw)
  To: xen-devel, sstabellini, jgross; +Cc: julien

As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
used with vmalloc'd addresses. This patch introduces a warning, when
virt_to_gfn is used in this way.

Signed-off-by: Simon Leiner <simon@leiner.me>
---
 include/xen/arm/page.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
index d7f6af50e200..b0d303b633d0 100644
--- a/include/xen/arm/page.h
+++ b/include/xen/arm/page.h
@@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
 #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
 
 /* VIRT <-> GUEST conversion */
-#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
+#define virt_to_gfn(v)                                                         \
+	({                                                                     \
+		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
+		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
+	})
 #define gfn_to_virt(m)		(__va(gfn_to_pfn(m) << XEN_PAGE_SHIFT))
 
 /* Only used in PV code. But ARM guests are always HVM. */
-- 
2.24.3 (Apple Git-128)



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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
@ 2020-08-25 12:16     ` Jan Beulich
  2020-08-25 23:48       ` Stefano Stabellini
  2020-08-25 23:58     ` Stefano Stabellini
  2020-08-26 18:37     ` Julien Grall
  2 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2020-08-25 12:16 UTC (permalink / raw)
  To: Simon Leiner; +Cc: xen-devel, sstabellini, jgross, julien

On 25.08.2020 11:31, Simon Leiner wrote:
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>  
>  /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> +	})

Shouldn't such a check cover more than just the vmalloc range,
i.e. everything outside of what __va() can validly return?

Jan


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25 12:16     ` Jan Beulich
@ 2020-08-25 23:48       ` Stefano Stabellini
  2020-08-26  6:20         ` Jan Beulich
  0 siblings, 1 reply; 17+ messages in thread
From: Stefano Stabellini @ 2020-08-25 23:48 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Simon Leiner, xen-devel, sstabellini, jgross, julien

On Tue, 25 Aug 2020, Jan Beulich wrote:
> On 25.08.2020 11:31, Simon Leiner wrote:
> > --- a/include/xen/arm/page.h
> > +++ b/include/xen/arm/page.h
> > @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
> >  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
> >  
> >  /* VIRT <-> GUEST conversion */
> > -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> > +#define virt_to_gfn(v)                                                         \
> > +	({                                                                     \
> > +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> > +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> > +	})
> 
> Shouldn't such a check cover more than just the vmalloc range,
> i.e. everything outside of what __va() can validly return?

Keep in mind that this patch is meant as sister patch to
https://marc.info/?l=xen-devel&m=159834800203817
so it makes sense to check for vmalloc addresses specifically.

That said, I am not aware of a good way to implement the __va test you
are suggesting.


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

* Re: [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory
  2020-08-25  9:31 ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Simon Leiner
  2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
@ 2020-08-25 23:55   ` Stefano Stabellini
  1 sibling, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2020-08-25 23:55 UTC (permalink / raw)
  To: Simon Leiner; +Cc: xen-devel, sstabellini, jgross, julien

On Tue, 25 Aug 2020, Simon Leiner wrote:
> On some architectures (like ARM), virt_to_gfn cannot be used for
> vmalloc'd memory because of its reliance on virt_to_phys. This patch
> introduces a check for vmalloc'd addresses and obtains the PFN using
> vmalloc_to_pfn in that case.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  drivers/xen/xenbus/xenbus_client.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
> index 786fbb7d8be0..907bcbb93afb 100644
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -379,8 +379,14 @@ int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
>  	int i, j;
>  
>  	for (i = 0; i < nr_pages; i++) {
> -		err = gnttab_grant_foreign_access(dev->otherend_id,
> -						  virt_to_gfn(vaddr), 0);
> +		unsigned long gfn;
> +
> +		if (is_vmalloc_addr(vaddr))
> +			gfn = pfn_to_gfn(vmalloc_to_pfn(vaddr));
> +		else
> +			gfn = virt_to_gfn(vaddr);
> +
> +		err = gnttab_grant_foreign_access(dev->otherend_id, gfn, 0);
>  		if (err < 0) {
>  			xenbus_dev_fatal(dev, err,
>  					 "granting access to ring page");
> -- 
> 2.24.3 (Apple Git-128)
> 


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
  2020-08-25 12:16     ` Jan Beulich
@ 2020-08-25 23:58     ` Stefano Stabellini
  2020-08-26 18:37     ` Julien Grall
  2 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2020-08-25 23:58 UTC (permalink / raw)
  To: Simon Leiner; +Cc: xen-devel, sstabellini, jgross, julien

On Tue, 25 Aug 2020, Simon Leiner wrote:
> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
> used with vmalloc'd addresses. This patch introduces a warning, when
> virt_to_gfn is used in this way.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  include/xen/arm/page.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index d7f6af50e200..b0d303b633d0 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>  
>  /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
> +	})
>  #define gfn_to_virt(m)		(__va(gfn_to_pfn(m) << XEN_PAGE_SHIFT))
>  
>  /* Only used in PV code. But ARM guests are always HVM. */
> -- 
> 2.24.3 (Apple Git-128)
> 


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25 23:48       ` Stefano Stabellini
@ 2020-08-26  6:20         ` Jan Beulich
  2020-08-26  7:50           ` Simon Leiner
  0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2020-08-26  6:20 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: Simon Leiner, xen-devel, jgross, julien

On 26.08.2020 01:48, Stefano Stabellini wrote:
> On Tue, 25 Aug 2020, Jan Beulich wrote:
>> On 25.08.2020 11:31, Simon Leiner wrote:
>>> --- a/include/xen/arm/page.h
>>> +++ b/include/xen/arm/page.h
>>> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>>>  #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>>>  
>>>  /* VIRT <-> GUEST conversion */
>>> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
>>> +#define virt_to_gfn(v)                                                         \
>>> +	({                                                                     \
>>> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \
>>> +		pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT);                 \
>>> +	})
>>
>> Shouldn't such a check cover more than just the vmalloc range,
>> i.e. everything outside of what __va() can validly return?
> 
> Keep in mind that this patch is meant as sister patch to
> https://marc.info/?l=xen-devel&m=159834800203817
> so it makes sense to check for vmalloc addresses specifically.

I had seen that patch before writing the reply, and I had assumed
precisely what you say. It was for this reason that I did point
out the limitation: While there's a specific issue for the vmalloc
range, any other addresses outside the direct mapping area are as
problematic (aiui).

> That said, I am not aware of a good way to implement the __va test you
> are suggesting.

Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
the impression that they're just linear transformations of the
address, which would seem to suggest there's a pre-determined
address range used for the direct map.

Jan


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26  6:20         ` Jan Beulich
@ 2020-08-26  7:50           ` Simon Leiner
  2020-08-26  7:59             ` Jürgen Groß
  0 siblings, 1 reply; 17+ messages in thread
From: Simon Leiner @ 2020-08-26  7:50 UTC (permalink / raw)
  To: Jan Beulich, Stefano Stabellini; +Cc: xen-devel, jgross, julien

On 26.08.20 08:20, Jan Beulich wrote:
> Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
> the impression that they're just linear transformations of the
> address, which would seem to suggest there's a pre-determined
> address range used for the direct map.

From eyeballing the kernel code, it seems like __is_lm_address() is
suitable for such a check, especially since it is also used in
__virt_to_phys() (the one in arch/arm64/mm/physaddr.c).

Greetings,
Simon




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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26  7:50           ` Simon Leiner
@ 2020-08-26  7:59             ` Jürgen Groß
  2020-08-26  8:14               ` Simon Leiner
  0 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2020-08-26  7:59 UTC (permalink / raw)
  To: Simon Leiner, Jan Beulich, Stefano Stabellini; +Cc: xen-devel, julien

On 26.08.20 09:50, Simon Leiner wrote:
> On 26.08.20 08:20, Jan Beulich wrote:
>> Hmm, to me __phys_to_virt() and __virt_to_phys_nodebug() give
>> the impression that they're just linear transformations of the
>> address, which would seem to suggest there's a pre-determined
>> address range used for the direct map.
> 
>  From eyeballing the kernel code, it seems like __is_lm_address() is
> suitable for such a check, especially since it is also used in
> __virt_to_phys() (the one in arch/arm64/mm/physaddr.c).

This seems to be an Arm specific function.

virt_addr_valid() seems to be a good fit.


Juergen


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26  7:59             ` Jürgen Groß
@ 2020-08-26  8:14               ` Simon Leiner
  2020-08-26  8:27                 ` Jürgen Groß
  0 siblings, 1 reply; 17+ messages in thread
From: Simon Leiner @ 2020-08-26  8:14 UTC (permalink / raw)
  To: Jürgen Groß, Jan Beulich, Stefano Stabellini; +Cc: xen-devel, julien

On 26.08.20 09:59, Jürgen Groß wrote:
> This seems to be an Arm specific function.

Is that a problem? The caller site is also ARM specific.

> virt_addr_valid() seems to be a good fit.

If you prefer that anyway, I will change it and resubmit that part of
the patch.

Simon


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26  8:14               ` Simon Leiner
@ 2020-08-26  8:27                 ` Jürgen Groß
  2020-08-26  8:30                   ` Simon Leiner
  0 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2020-08-26  8:27 UTC (permalink / raw)
  To: Simon Leiner, Jan Beulich, Stefano Stabellini; +Cc: xen-devel, julien

On 26.08.20 10:14, Simon Leiner wrote:
> On 26.08.20 09:59, Jürgen Groß wrote:
>> This seems to be an Arm specific function.
> 
> Is that a problem? The caller site is also ARM specific.

The caller site is ARM specific, but __is_lm_address() is defined for
arm64 only.

> 
>> virt_addr_valid() seems to be a good fit.
> 
> If you prefer that anyway, I will change it and resubmit that part of
> the patch.

No need for resend, I can fix that when committing.


Juergen


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26  8:27                 ` Jürgen Groß
@ 2020-08-26  8:30                   ` Simon Leiner
  0 siblings, 0 replies; 17+ messages in thread
From: Simon Leiner @ 2020-08-26  8:30 UTC (permalink / raw)
  To: Jürgen Groß, Jan Beulich, Stefano Stabellini; +Cc: xen-devel, julien

On 26.08.20 10:27, Jürgen Groß wrote:
> On 26.08.20 10:14, Simon Leiner wrote:
>> On 26.08.20 09:59, Jürgen Groß wrote:
>>> This seems to be an Arm specific function.
>>
>> Is that a problem? The caller site is also ARM specific.
> 
> The caller site is ARM specific, but __is_lm_address() is defined for
> arm64 only.

Oh, I did not catch that - you're right, thanks :-)


> No need for resend, I can fix that when committing. 

Thanks!

Simon


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
  2020-08-25 12:16     ` Jan Beulich
  2020-08-25 23:58     ` Stefano Stabellini
@ 2020-08-26 18:37     ` Julien Grall
  2020-08-27  5:21       ` Jürgen Groß
  2 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2020-08-26 18:37 UTC (permalink / raw)
  To: Simon Leiner, xen-devel, sstabellini, jgross

Hi Simon,

On 25/08/2020 10:31, Simon Leiner wrote:
> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
> used with vmalloc'd addresses. This patch introduces a warning, when
> virt_to_gfn is used in this way.
> 
> Signed-off-by: Simon Leiner <simon@leiner.me>
> ---
>   include/xen/arm/page.h | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
> index d7f6af50e200..b0d303b633d0 100644
> --- a/include/xen/arm/page.h
> +++ b/include/xen/arm/page.h
> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned long bfn)
>   #define bfn_to_local_pfn(bfn)	bfn_to_pfn(bfn)
>   
>   /* VIRT <-> GUEST conversion */
> -#define virt_to_gfn(v)		(pfn_to_gfn(virt_to_phys(v) >> XEN_PAGE_SHIFT))
> +#define virt_to_gfn(v)                                                         \
> +	({                                                                     \
> +		WARN_ON_ONCE(is_vmalloc_addr(v));                              \

virt_to_gfn() will usually be called from generic code. WARN_ON_ONCE() 
will only catch one instance and it means we would have to fix the first 
instance and then re-run to catch the others.

So I think we want to switch to WARN_ON() here.

Cheers,

-- 
Julien Grall


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-26 18:37     ` Julien Grall
@ 2020-08-27  5:21       ` Jürgen Groß
  2020-08-27  8:24         ` Julien Grall
  0 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2020-08-27  5:21 UTC (permalink / raw)
  To: Julien Grall, Simon Leiner, xen-devel, sstabellini

On 26.08.20 20:37, Julien Grall wrote:
> Hi Simon,
> 
> On 25/08/2020 10:31, Simon Leiner wrote:
>> As virt_to_gfn uses virt_to_phys, it will return invalid addresses when
>> used with vmalloc'd addresses. This patch introduces a warning, when
>> virt_to_gfn is used in this way.
>>
>> Signed-off-by: Simon Leiner <simon@leiner.me>
>> ---
>>   include/xen/arm/page.h | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/xen/arm/page.h b/include/xen/arm/page.h
>> index d7f6af50e200..b0d303b633d0 100644
>> --- a/include/xen/arm/page.h
>> +++ b/include/xen/arm/page.h
>> @@ -76,7 +76,11 @@ static inline unsigned long bfn_to_pfn(unsigned 
>> long bfn)
>>   #define bfn_to_local_pfn(bfn)    bfn_to_pfn(bfn)
>>   /* VIRT <-> GUEST conversion */
>> -#define virt_to_gfn(v)        (pfn_to_gfn(virt_to_phys(v) >> 
>> XEN_PAGE_SHIFT))
>> +#define 
>> virt_to_gfn(v)                                                         \
>> +    
>> ({                                                                     \
>> +        WARN_ON_ONCE(is_vmalloc_addr(v));                              \
> 
> virt_to_gfn() will usually be called from generic code. WARN_ON_ONCE() 

"Usually" is a bit gross here. The only generic call site I could find
is xenbus_grant_ring(). All other instances (I counted 22) are not
generic at all.

> will only catch one instance and it means we would have to fix the first 
> instance and then re-run to catch the others.
> 
> So I think we want to switch to WARN_ON() here.

No, please don't. In case there would be a frequent path the result
would be a basically unusable system due to massive console clobbering.


Juergen


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-27  5:21       ` Jürgen Groß
@ 2020-08-27  8:24         ` Julien Grall
  2020-08-27  8:35           ` Jürgen Groß
  0 siblings, 1 reply; 17+ messages in thread
From: Julien Grall @ 2020-08-27  8:24 UTC (permalink / raw)
  To: Jürgen Groß, Simon Leiner, xen-devel, sstabellini



On 27/08/2020 06:21, Jürgen Groß wrote:
> On 26.08.20 20:37, Julien Grall wrote:
> "Usually" is a bit gross here. The only generic call site I could find
> is xenbus_grant_ring(). All other instances (I counted 22) are not
> generic at all.
> 
>> will only catch one instance and it means we would have to fix the 
>> first instance and then re-run to catch the others.
>>
>> So I think we want to switch to WARN_ON() here.
> 
> No, please don't. In case there would be a frequent path the result
> would be a basically unusable system due to massive console clobbering.

Right, but if that's really happenning then you have a much bigger 
problem on your platform because the address returned will be invalid.

So I still don't see the advantage of WARN_ON_ONCE() here.

Cheers,

> 
> 
> Juergen

-- 
Julien Grall


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-27  8:24         ` Julien Grall
@ 2020-08-27  8:35           ` Jürgen Groß
  2020-08-27 16:04             ` Stefano Stabellini
  0 siblings, 1 reply; 17+ messages in thread
From: Jürgen Groß @ 2020-08-27  8:35 UTC (permalink / raw)
  To: Julien Grall, Simon Leiner, xen-devel, sstabellini

On 27.08.20 10:24, Julien Grall wrote:
> 
> 
> On 27/08/2020 06:21, Jürgen Groß wrote:
>> On 26.08.20 20:37, Julien Grall wrote:
>> "Usually" is a bit gross here. The only generic call site I could find
>> is xenbus_grant_ring(). All other instances (I counted 22) are not
>> generic at all.
>>
>>> will only catch one instance and it means we would have to fix the 
>>> first instance and then re-run to catch the others.
>>>
>>> So I think we want to switch to WARN_ON() here.
>>
>> No, please don't. In case there would be a frequent path the result
>> would be a basically unusable system due to massive console clobbering.
> 
> Right, but if that's really happenning then you have a much bigger 
> problem on your platform because the address returned will be invalid.
> 
> So I still don't see the advantage of WARN_ON_ONCE() here.

Depends of the (potential) source of the warnings. I think we can agree
that e.g. a problem in the pv network stack is rather improbable, as it
would have been detected long ago.

If, however, the problem is being introduced by one of the rather new
pv-drivers (like sound, pvcalls, 9pfs) it is perfectly fine to assume
the overall system is still functional even without those drivers
working correctly. Having a message storm from those sources is still
quite undesirable IMO and doesn't really help.


Juergen


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

* Re: [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn
  2020-08-27  8:35           ` Jürgen Groß
@ 2020-08-27 16:04             ` Stefano Stabellini
  0 siblings, 0 replies; 17+ messages in thread
From: Stefano Stabellini @ 2020-08-27 16:04 UTC (permalink / raw)
  To: Jürgen Groß; +Cc: Julien Grall, Simon Leiner, xen-devel, sstabellini


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

On Thu, 27 Aug 2020, Jürgen Groß wrote:
> On 27.08.20 10:24, Julien Grall wrote:
> > 
> > 
> > On 27/08/2020 06:21, Jürgen Groß wrote:
> > > On 26.08.20 20:37, Julien Grall wrote:
> > > "Usually" is a bit gross here. The only generic call site I could find
> > > is xenbus_grant_ring(). All other instances (I counted 22) are not
> > > generic at all.
> > > 
> > > > will only catch one instance and it means we would have to fix the first
> > > > instance and then re-run to catch the others.
> > > > 
> > > > So I think we want to switch to WARN_ON() here.
> > > 
> > > No, please don't. In case there would be a frequent path the result
> > > would be a basically unusable system due to massive console clobbering.
> > 
> > Right, but if that's really happenning then you have a much bigger problem
> > on your platform because the address returned will be invalid.
> > 
> > So I still don't see the advantage of WARN_ON_ONCE() here.
> 
> Depends of the (potential) source of the warnings. I think we can agree
> that e.g. a problem in the pv network stack is rather improbable, as it
> would have been detected long ago.
> 
> If, however, the problem is being introduced by one of the rather new
> pv-drivers (like sound, pvcalls, 9pfs) it is perfectly fine to assume
> the overall system is still functional even without those drivers
> working correctly. Having a message storm from those sources is still
> quite undesirable IMO and doesn't really help.

I agree with this.

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

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <Aw: [Linux] [ARM] Granting memory obtained from the DMA API>
2020-08-25  9:31 ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Simon Leiner
2020-08-25  9:31   ` [PATCH 2/2] arm/xen: Add misuse warning to virt_to_gfn Simon Leiner
2020-08-25 12:16     ` Jan Beulich
2020-08-25 23:48       ` Stefano Stabellini
2020-08-26  6:20         ` Jan Beulich
2020-08-26  7:50           ` Simon Leiner
2020-08-26  7:59             ` Jürgen Groß
2020-08-26  8:14               ` Simon Leiner
2020-08-26  8:27                 ` Jürgen Groß
2020-08-26  8:30                   ` Simon Leiner
2020-08-25 23:58     ` Stefano Stabellini
2020-08-26 18:37     ` Julien Grall
2020-08-27  5:21       ` Jürgen Groß
2020-08-27  8:24         ` Julien Grall
2020-08-27  8:35           ` Jürgen Groß
2020-08-27 16:04             ` Stefano Stabellini
2020-08-25 23:55   ` [PATCH 1/2] xen/xenbus: Fix granting of vmalloc'd memory Stefano Stabellini

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git