[PATCH 0/3] tune preempt_[dis|en]able()

[PATCH 0/3] tune preempt_[dis|en]able()

[Juergen Gross]

There is no user of the preempt_count for non-debug builds. This series
removes it for that case.

Juergen Gross (3):
  xen: drop in_atomic()
  xen: drop preempt_count() for non-debug builds
  xen: build common/preempt.c only with CONFIG_DEBUG

 xen/arch/x86/hvm/hvm.c    | 16 ----------------
 xen/common/Makefile       |  2 +-
 xen/common/preempt.c      |  7 -------
 xen/include/xen/preempt.h |  8 +++++---
 4 files changed, 6 insertions(+), 27 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 0/3] tune preempt_[dis|en]able()

[Juergen Gross]

There is no user of the preempt_count for non-debug builds. This series
removes it for that case.

Juergen Gross (3):
  xen: drop in_atomic()
  xen: drop preempt_count() for non-debug builds
  xen: build common/preempt.c only with CONFIG_DEBUG

 xen/arch/x86/hvm/hvm.c    | 16 ----------------
 xen/common/Makefile       |  2 +-
 xen/common/preempt.c      |  7 -------
 xen/include/xen/preempt.h |  8 +++++---
 4 files changed, 6 insertions(+), 27 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

Currently there is only one user of in_atomic(), and that is in an
#ifdef 0 section. This has been so since Xen 4.1, so chances are
rather slim we suddenly want to enable it again.

Dropping in_atomic() will remove the last user of preempt_count()
in non-debug builds enabling further optimizations.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/hvm/hvm.c    | 16 ----------------
 xen/common/preempt.c      |  5 -----
 xen/include/xen/preempt.h |  2 --
 3 files changed, 23 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8993c2aa57..7b36765b97 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
 
     ASSERT(is_hvm_vcpu(v));
 
-    /*
-     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
-     * such as query_size. Grant-table code currently does copy_to/from_guest
-     * accesses under the big per-domain lock, which this test would disallow.
-     * The test is not needed until we implement sleeping-on-waitqueue when
-     * we access a paged-out frame, and that's post 4.1.0 now.
-     */
-#if 0
-    /*
-     * If the required guest memory is paged out, this function may sleep.
-     * Hence we bail immediately if called from atomic context.
-     */
-    if ( in_atomic() )
-        return HVMTRANS_unhandleable;
-#endif
-
     while ( todo > 0 )
     {
         enum hvm_translation_result res;
diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 3b4178fd44..20913e20d3 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -25,11 +25,6 @@
 
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
-bool_t in_atomic(void)
-{
-    return preempt_count() || in_irq() || !local_irq_is_enabled();
-}
-
 #ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void)
 {
diff --git a/xen/include/xen/preempt.h b/xen/include/xen/preempt.h
index bef83135a1..f715ca09bc 100644
--- a/xen/include/xen/preempt.h
+++ b/xen/include/xen/preempt.h
@@ -26,8 +26,6 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
     preempt_count()--;                          \
 } while (0)
 
-bool_t in_atomic(void);
-
 #ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void);
 #else
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

Currently there is only one user of in_atomic(), and that is in an
#ifdef 0 section. This has been so since Xen 4.1, so chances are
rather slim we suddenly want to enable it again.

Dropping in_atomic() will remove the last user of preempt_count()
in non-debug builds enabling further optimizations.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/hvm/hvm.c    | 16 ----------------
 xen/common/preempt.c      |  5 -----
 xen/include/xen/preempt.h |  2 --
 3 files changed, 23 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8993c2aa57..7b36765b97 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
 
     ASSERT(is_hvm_vcpu(v));
 
-    /*
-     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
-     * such as query_size. Grant-table code currently does copy_to/from_guest
-     * accesses under the big per-domain lock, which this test would disallow.
-     * The test is not needed until we implement sleeping-on-waitqueue when
-     * we access a paged-out frame, and that's post 4.1.0 now.
-     */
-#if 0
-    /*
-     * If the required guest memory is paged out, this function may sleep.
-     * Hence we bail immediately if called from atomic context.
-     */
-    if ( in_atomic() )
-        return HVMTRANS_unhandleable;
-#endif
-
     while ( todo > 0 )
     {
         enum hvm_translation_result res;
diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 3b4178fd44..20913e20d3 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -25,11 +25,6 @@
 
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
-bool_t in_atomic(void)
-{
-    return preempt_count() || in_irq() || !local_irq_is_enabled();
-}
-
 #ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void)
 {
diff --git a/xen/include/xen/preempt.h b/xen/include/xen/preempt.h
index bef83135a1..f715ca09bc 100644
--- a/xen/include/xen/preempt.h
+++ b/xen/include/xen/preempt.h
@@ -26,8 +26,6 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
     preempt_count()--;                          \
 } while (0)
 
-bool_t in_atomic(void);
-
 #ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void);
 #else
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

preempt_count() and the associated per-cpu variable __preempt_count
are tested in debug build only. So drop them for non-debug builds.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/preempt.c      | 2 +-
 xen/include/xen/preempt.h | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 20913e20d3..3077c51d52 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -23,9 +23,9 @@
 #include <xen/irq.h>
 #include <asm/system.h>
 
+#ifndef NDEBUG
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
-#ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void)
 {
     ASSERT(!preempt_count());
diff --git a/xen/include/xen/preempt.h b/xen/include/xen/preempt.h
index f715ca09bc..0bf49cc979 100644
--- a/xen/include/xen/preempt.h
+++ b/xen/include/xen/preempt.h
@@ -12,6 +12,8 @@
 #include <xen/types.h>
 #include <xen/percpu.h>
 
+#ifndef NDEBUG
+
 DECLARE_PER_CPU(unsigned int, __preempt_count);
 
 #define preempt_count() (this_cpu(__preempt_count))
@@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
     preempt_count()--;                          \
 } while (0)
 
-#ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void);
+
 #else
+#define preempt_disable()    barrier();
+#define preempt_enable()     barrier();
 #define ASSERT_NOT_IN_ATOMIC() ((void)0)
 #endif
 
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

preempt_count() and the associated per-cpu variable __preempt_count
are tested in debug build only. So drop them for non-debug builds.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/preempt.c      | 2 +-
 xen/include/xen/preempt.h | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 20913e20d3..3077c51d52 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -23,9 +23,9 @@
 #include <xen/irq.h>
 #include <asm/system.h>
 
+#ifndef NDEBUG
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
-#ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void)
 {
     ASSERT(!preempt_count());
diff --git a/xen/include/xen/preempt.h b/xen/include/xen/preempt.h
index f715ca09bc..0bf49cc979 100644
--- a/xen/include/xen/preempt.h
+++ b/xen/include/xen/preempt.h
@@ -12,6 +12,8 @@
 #include <xen/types.h>
 #include <xen/percpu.h>
 
+#ifndef NDEBUG
+
 DECLARE_PER_CPU(unsigned int, __preempt_count);
 
 #define preempt_count() (this_cpu(__preempt_count))
@@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
     preempt_count()--;                          \
 } while (0)
 
-#ifndef NDEBUG
 void ASSERT_NOT_IN_ATOMIC(void);
+
 #else
+#define preempt_disable()    barrier();
+#define preempt_enable()     barrier();
 #define ASSERT_NOT_IN_ATOMIC() ((void)0)
 #endif
 
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] xen: build common/preempt.c only with CONFIG_DEBUG

[Juergen Gross]

There is nothing left in common/preempt.c in non-debug builds.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/Makefile  | 2 +-
 xen/common/preempt.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 33d03b862f..d6576a3fc3 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -30,7 +30,7 @@ obj-y += notifier.o
 obj-y += page_alloc.o
 obj-$(CONFIG_HAS_PDX) += pdx.o
 obj-$(CONFIG_PERF_COUNTERS) += perfc.o
-obj-y += preempt.o
+obj-$(CONFIG_DEBUG) += preempt.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 3077c51d52..8bb099e8e8 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -23,7 +23,6 @@
 #include <xen/irq.h>
 #include <asm/system.h>
 
-#ifndef NDEBUG
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
 void ASSERT_NOT_IN_ATOMIC(void)
@@ -32,4 +31,3 @@ void ASSERT_NOT_IN_ATOMIC(void)
     ASSERT(!in_irq());
     ASSERT(local_irq_is_enabled());
 }
-#endif
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] xen: build common/preempt.c only with CONFIG_DEBUG

[Juergen Gross]

There is nothing left in common/preempt.c in non-debug builds.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/Makefile  | 2 +-
 xen/common/preempt.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/common/Makefile b/xen/common/Makefile
index 33d03b862f..d6576a3fc3 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -30,7 +30,7 @@ obj-y += notifier.o
 obj-y += page_alloc.o
 obj-$(CONFIG_HAS_PDX) += pdx.o
 obj-$(CONFIG_PERF_COUNTERS) += perfc.o
-obj-y += preempt.o
+obj-$(CONFIG_DEBUG) += preempt.o
 obj-y += random.o
 obj-y += rangeset.o
 obj-y += radix-tree.o
diff --git a/xen/common/preempt.c b/xen/common/preempt.c
index 3077c51d52..8bb099e8e8 100644
--- a/xen/common/preempt.c
+++ b/xen/common/preempt.c
@@ -23,7 +23,6 @@
 #include <xen/irq.h>
 #include <asm/system.h>
 
-#ifndef NDEBUG
 DEFINE_PER_CPU(unsigned int, __preempt_count);
 
 void ASSERT_NOT_IN_ATOMIC(void)
@@ -32,4 +31,3 @@ void ASSERT_NOT_IN_ATOMIC(void)
     ASSERT(!in_irq());
     ASSERT(local_irq_is_enabled());
 }
-#endif
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Andrew Cooper]

On 22/05/2019 10:45, Juergen Gross wrote:
> Currently there is only one user of in_atomic(), and that is in an
> #ifdef 0 section. This has been so since Xen 4.1, so chances are
> rather slim we suddenly want to enable it again.
>
> Dropping in_atomic() will remove the last user of preempt_count()
> in non-debug builds enabling further optimizations.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This code has never been used and is obviously not doing anything useful.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Andrew Cooper]

On 22/05/2019 10:45, Juergen Gross wrote:
> Currently there is only one user of in_atomic(), and that is in an
> #ifdef 0 section. This has been so since Xen 4.1, so chances are
> rather slim we suddenly want to enable it again.
>
> Dropping in_atomic() will remove the last user of preempt_count()
> in non-debug builds enabling further optimizations.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

This code has never been used and is obviously not doing anything useful.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

On 22/05/2019 10:45, Juergen Gross wrote:
> preempt_count() and the associated per-cpu variable __preempt_count
> are tested in debug build only. So drop them for non-debug builds.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

I'd be tempted to fold patches 2 and 3 together, because they are both
the same change, and it would reduce the churn.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
two folded into one.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

On 22/05/2019 10:45, Juergen Gross wrote:
> preempt_count() and the associated per-cpu variable __preempt_count
> are tested in debug build only. So drop them for non-debug builds.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

I'd be tempted to fold patches 2 and 3 together, because they are both
the same change, and it would reduce the churn.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
two folded into one.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>  
>      ASSERT(is_hvm_vcpu(v));
>  
> -    /*
> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
> -     * such as query_size. Grant-table code currently does copy_to/from_guest
> -     * accesses under the big per-domain lock, which this test would disallow.
> -     * The test is not needed until we implement sleeping-on-waitqueue when
> -     * we access a paged-out frame, and that's post 4.1.0 now.
> -     */
> -#if 0
> -    /*
> -     * If the required guest memory is paged out, this function may sleep.
> -     * Hence we bail immediately if called from atomic context.
> -     */
> -    if ( in_atomic() )
> -        return HVMTRANS_unhandleable;
> -#endif

Dealing with this TODO item is of course much appreciated, but
should it really be deleted altogether? The big-domain-lock issue
is gone afair, in which case dropping the #if 0 would seem
possible to me, even if it's not strictly needed without the sleep-
on-waitqueue behavior mentioned.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>  
>      ASSERT(is_hvm_vcpu(v));
>  
> -    /*
> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
> -     * such as query_size. Grant-table code currently does copy_to/from_guest
> -     * accesses under the big per-domain lock, which this test would disallow.
> -     * The test is not needed until we implement sleeping-on-waitqueue when
> -     * we access a paged-out frame, and that's post 4.1.0 now.
> -     */
> -#if 0
> -    /*
> -     * If the required guest memory is paged out, this function may sleep.
> -     * Hence we bail immediately if called from atomic context.
> -     */
> -    if ( in_atomic() )
> -        return HVMTRANS_unhandleable;
> -#endif

Dealing with this TODO item is of course much appreciated, but
should it really be deleted altogether? The big-domain-lock issue
is gone afair, in which case dropping the #if 0 would seem
possible to me, even if it's not strictly needed without the sleep-
on-waitqueue behavior mentioned.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
> @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
>      preempt_count()--;                          \
>  } while (0)
>  
> -#ifndef NDEBUG
>  void ASSERT_NOT_IN_ATOMIC(void);
> +
>  #else
> +#define preempt_disable()    barrier();
> +#define preempt_enable()     barrier();

Stray semicolons (could be dropped while committing if we really
want to go this route).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
> @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
>      preempt_count()--;                          \
>  } while (0)
>  
> -#ifndef NDEBUG
>  void ASSERT_NOT_IN_ATOMIC(void);
> +
>  #else
> +#define preempt_disable()    barrier();
> +#define preempt_enable()     barrier();

Stray semicolons (could be dropped while committing if we really
want to go this route).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

On 22/05/2019 12:00, Andrew Cooper wrote:
> On 22/05/2019 10:45, Juergen Gross wrote:
>> preempt_count() and the associated per-cpu variable __preempt_count
>> are tested in debug build only. So drop them for non-debug builds.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I'd be tempted to fold patches 2 and 3 together, because they are both
> the same change, and it would reduce the churn.

Fine with me.

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
> two folded into one.

Thanks,

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

On 22/05/2019 12:00, Andrew Cooper wrote:
> On 22/05/2019 10:45, Juergen Gross wrote:
>> preempt_count() and the associated per-cpu variable __preempt_count
>> are tested in debug build only. So drop them for non-debug builds.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I'd be tempted to fold patches 2 and 3 together, because they are both
> the same change, and it would reduce the churn.

Fine with me.

> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
> two folded into one.

Thanks,

Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

On 22/05/2019 12:12, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
>>      preempt_count()--;                          \
>>  } while (0)
>>  
>> -#ifndef NDEBUG
>>  void ASSERT_NOT_IN_ATOMIC(void);
>> +
>>  #else
>> +#define preempt_disable()    barrier();
>> +#define preempt_enable()     barrier();
> 
> Stray semicolons (could be dropped while committing if we really
> want to go this route).

Oh, right.

Will correct in V2.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

On 22/05/2019 12:12, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> @@ -26,9 +28,11 @@ DECLARE_PER_CPU(unsigned int, __preempt_count);
>>      preempt_count()--;                          \
>>  } while (0)
>>  
>> -#ifndef NDEBUG
>>  void ASSERT_NOT_IN_ATOMIC(void);
>> +
>>  #else
>> +#define preempt_disable()    barrier();
>> +#define preempt_enable()     barrier();
> 
> Stray semicolons (could be dropped while committing if we really
> want to go this route).

Oh, right.

Will correct in V2.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote:
> On 22/05/2019 10:45, Juergen Gross wrote:
>> preempt_count() and the associated per-cpu variable __preempt_count
>> are tested in debug build only. So drop them for non-debug builds.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I'd be tempted to fold patches 2 and 3 together, because they are both
> the same change, and it would reduce the churn.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
> two folded into one.

I'm a little surprised by this: Wasn't it you who generally
wanted what ASSERT() expands to (controlled by NDEBUG)
be independent of CONFIG_DEBUG, at some point down
the road? Aren't you even having ASSERT()s enabled in
release builds of XenServer, or am I misremembering? If so
patch 3 would move us in the wrong direction.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Jan Beulich]

>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote:
> On 22/05/2019 10:45, Juergen Gross wrote:
>> preempt_count() and the associated per-cpu variable __preempt_count
>> are tested in debug build only. So drop them for non-debug builds.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> I'd be tempted to fold patches 2 and 3 together, because they are both
> the same change, and it would reduce the churn.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
> two folded into one.

I'm a little surprised by this: Wasn't it you who generally
wanted what ASSERT() expands to (controlled by NDEBUG)
be independent of CONFIG_DEBUG, at some point down
the road? Aren't you even having ASSERT()s enabled in
release builds of XenServer, or am I misremembering? If so
patch 3 would move us in the wrong direction.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 22/05/2019 12:10, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>  
>>      ASSERT(is_hvm_vcpu(v));
>>  
>> -    /*
>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>> -     * accesses under the big per-domain lock, which this test would disallow.
>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>> -     */
>> -#if 0
>> -    /*
>> -     * If the required guest memory is paged out, this function may sleep.
>> -     * Hence we bail immediately if called from atomic context.
>> -     */
>> -    if ( in_atomic() )
>> -        return HVMTRANS_unhandleable;
>> -#endif
> 
> Dealing with this TODO item is of course much appreciated, but
> should it really be deleted altogether? The big-domain-lock issue
> is gone afair, in which case dropping the #if 0 would seem
> possible to me, even if it's not strictly needed without the sleep-
> on-waitqueue behavior mentioned.

Question is whether it is worth to keep it resulting in the need to
keep preempt_count() as well.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 22/05/2019 12:10, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>  
>>      ASSERT(is_hvm_vcpu(v));
>>  
>> -    /*
>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>> -     * accesses under the big per-domain lock, which this test would disallow.
>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>> -     */
>> -#if 0
>> -    /*
>> -     * If the required guest memory is paged out, this function may sleep.
>> -     * Hence we bail immediately if called from atomic context.
>> -     */
>> -    if ( in_atomic() )
>> -        return HVMTRANS_unhandleable;
>> -#endif
> 
> Dealing with this TODO item is of course much appreciated, but
> should it really be deleted altogether? The big-domain-lock issue
> is gone afair, in which case dropping the #if 0 would seem
> possible to me, even if it's not strictly needed without the sleep-
> on-waitqueue behavior mentioned.

Question is whether it is worth to keep it resulting in the need to
keep preempt_count() as well.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

On 22/05/2019 11:18, Jan Beulich wrote:
>>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> On 22/05/2019 10:45, Juergen Gross wrote:
>>> preempt_count() and the associated per-cpu variable __preempt_count
>>> are tested in debug build only. So drop them for non-debug builds.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> I'd be tempted to fold patches 2 and 3 together, because they are both
>> the same change, and it would reduce the churn.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
>> two folded into one.
> I'm a little surprised by this: Wasn't it you who generally
> wanted what ASSERT() expands to (controlled by NDEBUG)
> be independent of CONFIG_DEBUG, at some point down
> the road?

In some ideal world yes, but what is rather more important is actually
having optimisation control unrelated to NDEBUG.

> Aren't you even having ASSERT()s enabled in
> release builds of XenServer, or am I misremembering? If so
> patch 3 would move us in the wrong direction.

We build and ship a debug and a release hypervisor.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Andrew Cooper]

On 22/05/2019 11:18, Jan Beulich wrote:
>>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> On 22/05/2019 10:45, Juergen Gross wrote:
>>> preempt_count() and the associated per-cpu variable __preempt_count
>>> are tested in debug build only. So drop them for non-debug builds.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> I'd be tempted to fold patches 2 and 3 together, because they are both
>> the same change, and it would reduce the churn.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
>> two folded into one.
> I'm a little surprised by this: Wasn't it you who generally
> wanted what ASSERT() expands to (controlled by NDEBUG)
> be independent of CONFIG_DEBUG, at some point down
> the road?

In some ideal world yes, but what is rather more important is actually
having optimisation control unrelated to NDEBUG.

> Aren't you even having ASSERT()s enabled in
> release builds of XenServer, or am I misremembering? If so
> patch 3 would move us in the wrong direction.

We build and ship a debug and a release hypervisor.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 22.05.19 at 12:19, <jgross@suse.com> wrote:
> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>  
>>>      ASSERT(is_hvm_vcpu(v));
>>>  
>>> -    /*
>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>> -     * such as query_size. Grant-table code currently does 
> copy_to/from_guest
>>> -     * accesses under the big per-domain lock, which this test would 
> disallow.
>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>> -     */
>>> -#if 0
>>> -    /*
>>> -     * If the required guest memory is paged out, this function may sleep.
>>> -     * Hence we bail immediately if called from atomic context.
>>> -     */
>>> -    if ( in_atomic() )
>>> -        return HVMTRANS_unhandleable;
>>> -#endif
>> 
>> Dealing with this TODO item is of course much appreciated, but
>> should it really be deleted altogether? The big-domain-lock issue
>> is gone afair, in which case dropping the #if 0 would seem
>> possible to me, even if it's not strictly needed without the sleep-
>> on-waitqueue behavior mentioned.
> 
> Question is whether it is worth to keep it resulting in the need to
> keep preempt_count() as well.

Well, personally I think keeping it is a small price to pay. But seeing
Andrew's R-b he clearly thinks different. And just to be clear - I
don't really want to veto this change, as at the same time it's also
easy enough to put back if need be. But I'd like this to be give a
2nd consideration at least.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 22.05.19 at 12:19, <jgross@suse.com> wrote:
> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>  
>>>      ASSERT(is_hvm_vcpu(v));
>>>  
>>> -    /*
>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>> -     * such as query_size. Grant-table code currently does 
> copy_to/from_guest
>>> -     * accesses under the big per-domain lock, which this test would 
> disallow.
>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>> -     */
>>> -#if 0
>>> -    /*
>>> -     * If the required guest memory is paged out, this function may sleep.
>>> -     * Hence we bail immediately if called from atomic context.
>>> -     */
>>> -    if ( in_atomic() )
>>> -        return HVMTRANS_unhandleable;
>>> -#endif
>> 
>> Dealing with this TODO item is of course much appreciated, but
>> should it really be deleted altogether? The big-domain-lock issue
>> is gone afair, in which case dropping the #if 0 would seem
>> possible to me, even if it's not strictly needed without the sleep-
>> on-waitqueue behavior mentioned.
> 
> Question is whether it is worth to keep it resulting in the need to
> keep preempt_count() as well.

Well, personally I think keeping it is a small price to pay. But seeing
Andrew's R-b he clearly thinks different. And just to be clear - I
don't really want to veto this change, as at the same time it's also
easy enough to put back if need be. But I'd like this to be give a
2nd consideration at least.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 22/05/2019 12:10, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>  
>>      ASSERT(is_hvm_vcpu(v));
>>  
>> -    /*
>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>> -     * accesses under the big per-domain lock, which this test would disallow.
>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>> -     */
>> -#if 0
>> -    /*
>> -     * If the required guest memory is paged out, this function may sleep.
>> -     * Hence we bail immediately if called from atomic context.
>> -     */
>> -    if ( in_atomic() )
>> -        return HVMTRANS_unhandleable;
>> -#endif
> 
> Dealing with this TODO item is of course much appreciated, but
> should it really be deleted altogether? The big-domain-lock issue
> is gone afair, in which case dropping the #if 0 would seem
> possible to me, even if it's not strictly needed without the sleep-
> on-waitqueue behavior mentioned.

I just had a look and found the following path:

do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
  arch_do_domctl()
    raw_copy_from_guest()
      copy_from_user_hvm()
        hvm_copy_from_guest_linear()
          __hvm_copy()

So no, we can't do the in_atomic() test IMO.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 22/05/2019 12:10, Jan Beulich wrote:
>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>  
>>      ASSERT(is_hvm_vcpu(v));
>>  
>> -    /*
>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>> -     * accesses under the big per-domain lock, which this test would disallow.
>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>> -     */
>> -#if 0
>> -    /*
>> -     * If the required guest memory is paged out, this function may sleep.
>> -     * Hence we bail immediately if called from atomic context.
>> -     */
>> -    if ( in_atomic() )
>> -        return HVMTRANS_unhandleable;
>> -#endif
> 
> Dealing with this TODO item is of course much appreciated, but
> should it really be deleted altogether? The big-domain-lock issue
> is gone afair, in which case dropping the #if 0 would seem
> possible to me, even if it's not strictly needed without the sleep-
> on-waitqueue behavior mentioned.

I just had a look and found the following path:

do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
  arch_do_domctl()
    raw_copy_from_guest()
      copy_from_user_hvm()
        hvm_copy_from_guest_linear()
          __hvm_copy()

So no, we can't do the in_atomic() test IMO.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>  
>>>      ASSERT(is_hvm_vcpu(v));
>>>  
>>> -    /*
>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>> -     */
>>> -#if 0
>>> -    /*
>>> -     * If the required guest memory is paged out, this function may sleep.
>>> -     * Hence we bail immediately if called from atomic context.
>>> -     */
>>> -    if ( in_atomic() )
>>> -        return HVMTRANS_unhandleable;
>>> -#endif
>> 
>> Dealing with this TODO item is of course much appreciated, but
>> should it really be deleted altogether? The big-domain-lock issue
>> is gone afair, in which case dropping the #if 0 would seem
>> possible to me, even if it's not strictly needed without the sleep-
>> on-waitqueue behavior mentioned.
> 
> I just had a look and found the following path:
> 
> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>   arch_do_domctl()
>     raw_copy_from_guest()
>       copy_from_user_hvm()
>         hvm_copy_from_guest_linear()
>           __hvm_copy()
> 
> So no, we can't do the in_atomic() test IMO.

Oh, right - that's a PVH constraint that could probably not even
be thought of that the time the comment was written. I'm still
of the opinion though that at least the still applicable part of
the comment should be kept in place. Whether this means also
keeping in_atomic() itself is then an independent question, i.e.
I wouldn't consider it overly bad if there was no implementation
in the tree, but the above still served as documentation of what
would need to be re-added. Still my preference would be for it
to be kept.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>  
>>>      ASSERT(is_hvm_vcpu(v));
>>>  
>>> -    /*
>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>> -     */
>>> -#if 0
>>> -    /*
>>> -     * If the required guest memory is paged out, this function may sleep.
>>> -     * Hence we bail immediately if called from atomic context.
>>> -     */
>>> -    if ( in_atomic() )
>>> -        return HVMTRANS_unhandleable;
>>> -#endif
>> 
>> Dealing with this TODO item is of course much appreciated, but
>> should it really be deleted altogether? The big-domain-lock issue
>> is gone afair, in which case dropping the #if 0 would seem
>> possible to me, even if it's not strictly needed without the sleep-
>> on-waitqueue behavior mentioned.
> 
> I just had a look and found the following path:
> 
> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>   arch_do_domctl()
>     raw_copy_from_guest()
>       copy_from_user_hvm()
>         hvm_copy_from_guest_linear()
>           __hvm_copy()
> 
> So no, we can't do the in_atomic() test IMO.

Oh, right - that's a PVH constraint that could probably not even
be thought of that the time the comment was written. I'm still
of the opinion though that at least the still applicable part of
the comment should be kept in place. Whether this means also
keeping in_atomic() itself is then an independent question, i.e.
I wouldn't consider it overly bad if there was no implementation
in the tree, but the above still served as documentation of what
would need to be re-added. Still my preference would be for it
to be kept.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 24/05/2019 08:38, Jan Beulich wrote:
>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>  
>>>>      ASSERT(is_hvm_vcpu(v));
>>>>  
>>>> -    /*
>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>> -     */
>>>> -#if 0
>>>> -    /*
>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>> -     * Hence we bail immediately if called from atomic context.
>>>> -     */
>>>> -    if ( in_atomic() )
>>>> -        return HVMTRANS_unhandleable;
>>>> -#endif
>>>
>>> Dealing with this TODO item is of course much appreciated, but
>>> should it really be deleted altogether? The big-domain-lock issue
>>> is gone afair, in which case dropping the #if 0 would seem
>>> possible to me, even if it's not strictly needed without the sleep-
>>> on-waitqueue behavior mentioned.
>>
>> I just had a look and found the following path:
>>
>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>   arch_do_domctl()
>>     raw_copy_from_guest()
>>       copy_from_user_hvm()
>>         hvm_copy_from_guest_linear()
>>           __hvm_copy()
>>
>> So no, we can't do the in_atomic() test IMO.
> 
> Oh, right - that's a PVH constraint that could probably not even
> be thought of that the time the comment was written. I'm still
> of the opinion though that at least the still applicable part of
> the comment should be kept in place. Whether this means also
> keeping in_atomic() itself is then an independent question, i.e.
> I wouldn't consider it overly bad if there was no implementation
> in the tree, but the above still served as documentation of what
> would need to be re-added. Still my preference would be for it
> to be kept.

Would you be okay with replacing the removed stuff above with:

/*
 * If the required guest memory is paged out this function may sleep.
 * So in theory we should bail out if called in atomic context.
 * Unfortunately this is true for PVH dom0 doing domctl calls which
 * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
 * should never be paged out, so we are fine without testing for
 * atomic context.
 */


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Juergen Gross]

On 24/05/2019 08:38, Jan Beulich wrote:
>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>  
>>>>      ASSERT(is_hvm_vcpu(v));
>>>>  
>>>> -    /*
>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>> -     */
>>>> -#if 0
>>>> -    /*
>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>> -     * Hence we bail immediately if called from atomic context.
>>>> -     */
>>>> -    if ( in_atomic() )
>>>> -        return HVMTRANS_unhandleable;
>>>> -#endif
>>>
>>> Dealing with this TODO item is of course much appreciated, but
>>> should it really be deleted altogether? The big-domain-lock issue
>>> is gone afair, in which case dropping the #if 0 would seem
>>> possible to me, even if it's not strictly needed without the sleep-
>>> on-waitqueue behavior mentioned.
>>
>> I just had a look and found the following path:
>>
>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>   arch_do_domctl()
>>     raw_copy_from_guest()
>>       copy_from_user_hvm()
>>         hvm_copy_from_guest_linear()
>>           __hvm_copy()
>>
>> So no, we can't do the in_atomic() test IMO.
> 
> Oh, right - that's a PVH constraint that could probably not even
> be thought of that the time the comment was written. I'm still
> of the opinion though that at least the still applicable part of
> the comment should be kept in place. Whether this means also
> keeping in_atomic() itself is then an independent question, i.e.
> I wouldn't consider it overly bad if there was no implementation
> in the tree, but the above still served as documentation of what
> would need to be re-added. Still my preference would be for it
> to be kept.

Would you be okay with replacing the removed stuff above with:

/*
 * If the required guest memory is paged out this function may sleep.
 * So in theory we should bail out if called in atomic context.
 * Unfortunately this is true for PVH dom0 doing domctl calls which
 * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
 * should never be paged out, so we are fine without testing for
 * atomic context.
 */


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>  
>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>  
>>>>> -    /*
>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>> -     */
>>>>> -#if 0
>>>>> -    /*
>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>> -     */
>>>>> -    if ( in_atomic() )
>>>>> -        return HVMTRANS_unhandleable;
>>>>> -#endif
>>>>
>>>> Dealing with this TODO item is of course much appreciated, but
>>>> should it really be deleted altogether? The big-domain-lock issue
>>>> is gone afair, in which case dropping the #if 0 would seem
>>>> possible to me, even if it's not strictly needed without the sleep-
>>>> on-waitqueue behavior mentioned.
>>>
>>> I just had a look and found the following path:
>>>
>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>   arch_do_domctl()
>>>     raw_copy_from_guest()
>>>       copy_from_user_hvm()
>>>         hvm_copy_from_guest_linear()
>>>           __hvm_copy()
>>>
>>> So no, we can't do the in_atomic() test IMO.
>> 
>> Oh, right - that's a PVH constraint that could probably not even
>> be thought of that the time the comment was written. I'm still
>> of the opinion though that at least the still applicable part of
>> the comment should be kept in place. Whether this means also
>> keeping in_atomic() itself is then an independent question, i.e.
>> I wouldn't consider it overly bad if there was no implementation
>> in the tree, but the above still served as documentation of what
>> would need to be re-added. Still my preference would be for it
>> to be kept.
> 
> Would you be okay with replacing the removed stuff above with:
> 
> /*
>  * If the required guest memory is paged out this function may sleep.
>  * So in theory we should bail out if called in atomic context.
>  * Unfortunately this is true for PVH dom0 doing domctl calls which

... this is true at least for ...

>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>  * should never be paged out, so we are fine without testing for
>  * atomic context.
>  */

Not sure about this Dom0-specific remark: Are we certain there are
no other paths, similar to the gnttab one having been mentioned till
now?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>  
>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>  
>>>>> -    /*
>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>> -     */
>>>>> -#if 0
>>>>> -    /*
>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>> -     */
>>>>> -    if ( in_atomic() )
>>>>> -        return HVMTRANS_unhandleable;
>>>>> -#endif
>>>>
>>>> Dealing with this TODO item is of course much appreciated, but
>>>> should it really be deleted altogether? The big-domain-lock issue
>>>> is gone afair, in which case dropping the #if 0 would seem
>>>> possible to me, even if it's not strictly needed without the sleep-
>>>> on-waitqueue behavior mentioned.
>>>
>>> I just had a look and found the following path:
>>>
>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>   arch_do_domctl()
>>>     raw_copy_from_guest()
>>>       copy_from_user_hvm()
>>>         hvm_copy_from_guest_linear()
>>>           __hvm_copy()
>>>
>>> So no, we can't do the in_atomic() test IMO.
>> 
>> Oh, right - that's a PVH constraint that could probably not even
>> be thought of that the time the comment was written. I'm still
>> of the opinion though that at least the still applicable part of
>> the comment should be kept in place. Whether this means also
>> keeping in_atomic() itself is then an independent question, i.e.
>> I wouldn't consider it overly bad if there was no implementation
>> in the tree, but the above still served as documentation of what
>> would need to be re-added. Still my preference would be for it
>> to be kept.
> 
> Would you be okay with replacing the removed stuff above with:
> 
> /*
>  * If the required guest memory is paged out this function may sleep.
>  * So in theory we should bail out if called in atomic context.
>  * Unfortunately this is true for PVH dom0 doing domctl calls which

... this is true at least for ...

>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>  * should never be paged out, so we are fine without testing for
>  * atomic context.
>  */

Not sure about this Dom0-specific remark: Are we certain there are
no other paths, similar to the gnttab one having been mentioned till
now?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Andrew Cooper]

On 24/05/2019 09:39, Jan Beulich wrote:
>>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
>> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>>  
>>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>>  
>>>>>> -    /*
>>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>>> -     */
>>>>>> -#if 0
>>>>>> -    /*
>>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>>> -     */
>>>>>> -    if ( in_atomic() )
>>>>>> -        return HVMTRANS_unhandleable;
>>>>>> -#endif
>>>>> Dealing with this TODO item is of course much appreciated, but
>>>>> should it really be deleted altogether? The big-domain-lock issue
>>>>> is gone afair, in which case dropping the #if 0 would seem
>>>>> possible to me, even if it's not strictly needed without the sleep-
>>>>> on-waitqueue behavior mentioned.
>>>> I just had a look and found the following path:
>>>>
>>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>>   arch_do_domctl()
>>>>     raw_copy_from_guest()
>>>>       copy_from_user_hvm()
>>>>         hvm_copy_from_guest_linear()
>>>>           __hvm_copy()
>>>>
>>>> So no, we can't do the in_atomic() test IMO.
>>> Oh, right - that's a PVH constraint that could probably not even
>>> be thought of that the time the comment was written. I'm still
>>> of the opinion though that at least the still applicable part of
>>> the comment should be kept in place. Whether this means also
>>> keeping in_atomic() itself is then an independent question, i.e.
>>> I wouldn't consider it overly bad if there was no implementation
>>> in the tree, but the above still served as documentation of what
>>> would need to be re-added. Still my preference would be for it
>>> to be kept.
>> Would you be okay with replacing the removed stuff above with:
>>
>> /*
>>  * If the required guest memory is paged out this function may sleep.
>>  * So in theory we should bail out if called in atomic context.
>>  * Unfortunately this is true for PVH dom0 doing domctl calls which
> ... this is true at least for ...
>
>>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>>  * should never be paged out, so we are fine without testing for
>>  * atomic context.
>>  */
> Not sure about this Dom0-specific remark: Are we certain there are
> no other paths, similar to the gnttab one having been mentioned till
> now?

Why is __hvm_copy() so special?  It is just one of many places which can
end up touching guest memory.

A comment here isn't going to help anyone who might find themselves with
problems.

Given that the test has never been used, and no issues have been raised,
and this path isn't AFAICT special, I don't see why it should be
special-cased.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Andrew Cooper]

On 24/05/2019 09:39, Jan Beulich wrote:
>>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
>> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>>  
>>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>>  
>>>>>> -    /*
>>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>>> -     * such as query_size. Grant-table code currently does copy_to/from_guest
>>>>>> -     * accesses under the big per-domain lock, which this test would disallow.
>>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>>> -     */
>>>>>> -#if 0
>>>>>> -    /*
>>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>>> -     */
>>>>>> -    if ( in_atomic() )
>>>>>> -        return HVMTRANS_unhandleable;
>>>>>> -#endif
>>>>> Dealing with this TODO item is of course much appreciated, but
>>>>> should it really be deleted altogether? The big-domain-lock issue
>>>>> is gone afair, in which case dropping the #if 0 would seem
>>>>> possible to me, even if it's not strictly needed without the sleep-
>>>>> on-waitqueue behavior mentioned.
>>>> I just had a look and found the following path:
>>>>
>>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>>   arch_do_domctl()
>>>>     raw_copy_from_guest()
>>>>       copy_from_user_hvm()
>>>>         hvm_copy_from_guest_linear()
>>>>           __hvm_copy()
>>>>
>>>> So no, we can't do the in_atomic() test IMO.
>>> Oh, right - that's a PVH constraint that could probably not even
>>> be thought of that the time the comment was written. I'm still
>>> of the opinion though that at least the still applicable part of
>>> the comment should be kept in place. Whether this means also
>>> keeping in_atomic() itself is then an independent question, i.e.
>>> I wouldn't consider it overly bad if there was no implementation
>>> in the tree, but the above still served as documentation of what
>>> would need to be re-added. Still my preference would be for it
>>> to be kept.
>> Would you be okay with replacing the removed stuff above with:
>>
>> /*
>>  * If the required guest memory is paged out this function may sleep.
>>  * So in theory we should bail out if called in atomic context.
>>  * Unfortunately this is true for PVH dom0 doing domctl calls which
> ... this is true at least for ...
>
>>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>>  * should never be paged out, so we are fine without testing for
>>  * atomic context.
>>  */
> Not sure about this Dom0-specific remark: Are we certain there are
> no other paths, similar to the gnttab one having been mentioned till
> now?

Why is __hvm_copy() so special?  It is just one of many places which can
end up touching guest memory.

A comment here isn't going to help anyone who might find themselves with
problems.

Given that the test has never been used, and no issues have been raised,
and this path isn't AFAICT special, I don't see why it should be
special-cased.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 14:30, <andrew.cooper3@citrix.com> wrote:
> On 24/05/2019 09:39, Jan Beulich wrote:
>>>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
>>> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>>>  
>>>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>>>  
>>>>>>> -    /*
>>>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>>>> -     * such as query_size. Grant-table code currently does 
> copy_to/from_guest
>>>>>>> -     * accesses under the big per-domain lock, which this test would 
> disallow.
>>>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>>>> -     */
>>>>>>> -#if 0
>>>>>>> -    /*
>>>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>>>> -     */
>>>>>>> -    if ( in_atomic() )
>>>>>>> -        return HVMTRANS_unhandleable;
>>>>>>> -#endif
>>>>>> Dealing with this TODO item is of course much appreciated, but
>>>>>> should it really be deleted altogether? The big-domain-lock issue
>>>>>> is gone afair, in which case dropping the #if 0 would seem
>>>>>> possible to me, even if it's not strictly needed without the sleep-
>>>>>> on-waitqueue behavior mentioned.
>>>>> I just had a look and found the following path:
>>>>>
>>>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>>>   arch_do_domctl()
>>>>>     raw_copy_from_guest()
>>>>>       copy_from_user_hvm()
>>>>>         hvm_copy_from_guest_linear()
>>>>>           __hvm_copy()
>>>>>
>>>>> So no, we can't do the in_atomic() test IMO.
>>>> Oh, right - that's a PVH constraint that could probably not even
>>>> be thought of that the time the comment was written. I'm still
>>>> of the opinion though that at least the still applicable part of
>>>> the comment should be kept in place. Whether this means also
>>>> keeping in_atomic() itself is then an independent question, i.e.
>>>> I wouldn't consider it overly bad if there was no implementation
>>>> in the tree, but the above still served as documentation of what
>>>> would need to be re-added. Still my preference would be for it
>>>> to be kept.
>>> Would you be okay with replacing the removed stuff above with:
>>>
>>> /*
>>>  * If the required guest memory is paged out this function may sleep.
>>>  * So in theory we should bail out if called in atomic context.
>>>  * Unfortunately this is true for PVH dom0 doing domctl calls which
>> ... this is true at least for ...
>>
>>>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>>>  * should never be paged out, so we are fine without testing for
>>>  * atomic context.
>>>  */
>> Not sure about this Dom0-specific remark: Are we certain there are
>> no other paths, similar to the gnttab one having been mentioned till
>> now?
> 
> Why is __hvm_copy() so special?  It is just one of many places which can
> end up touching guest memory.

Are you sure? I think everything that can touch guest (HVM) memory
actually ends up calling into this function.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/3] xen: drop in_atomic()

[Jan Beulich]

>>> On 24.05.19 at 14:30, <andrew.cooper3@citrix.com> wrote:
> On 24/05/2019 09:39, Jan Beulich wrote:
>>>>> On 24.05.19 at 10:34, <jgross@suse.com> wrote:
>>> On 24/05/2019 08:38, Jan Beulich wrote:
>>>>>>> On 24.05.19 at 07:41, <jgross@suse.com> wrote:
>>>>> On 22/05/2019 12:10, Jan Beulich wrote:
>>>>>>>>> On 22.05.19 at 11:45, <jgross@suse.com> wrote:
>>>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>>>> @@ -3185,22 +3185,6 @@ static enum hvm_translation_result __hvm_copy(
>>>>>>>  
>>>>>>>      ASSERT(is_hvm_vcpu(v));
>>>>>>>  
>>>>>>> -    /*
>>>>>>> -     * XXX Disable for 4.1.0: PV-on-HVM drivers will do grant-table ops
>>>>>>> -     * such as query_size. Grant-table code currently does 
> copy_to/from_guest
>>>>>>> -     * accesses under the big per-domain lock, which this test would 
> disallow.
>>>>>>> -     * The test is not needed until we implement sleeping-on-waitqueue when
>>>>>>> -     * we access a paged-out frame, and that's post 4.1.0 now.
>>>>>>> -     */
>>>>>>> -#if 0
>>>>>>> -    /*
>>>>>>> -     * If the required guest memory is paged out, this function may sleep.
>>>>>>> -     * Hence we bail immediately if called from atomic context.
>>>>>>> -     */
>>>>>>> -    if ( in_atomic() )
>>>>>>> -        return HVMTRANS_unhandleable;
>>>>>>> -#endif
>>>>>> Dealing with this TODO item is of course much appreciated, but
>>>>>> should it really be deleted altogether? The big-domain-lock issue
>>>>>> is gone afair, in which case dropping the #if 0 would seem
>>>>>> possible to me, even if it's not strictly needed without the sleep-
>>>>>> on-waitqueue behavior mentioned.
>>>>> I just had a look and found the following path:
>>>>>
>>>>> do_domctl() (takes domctl_lock and hypercall_deadlock_mutex)
>>>>>   arch_do_domctl()
>>>>>     raw_copy_from_guest()
>>>>>       copy_from_user_hvm()
>>>>>         hvm_copy_from_guest_linear()
>>>>>           __hvm_copy()
>>>>>
>>>>> So no, we can't do the in_atomic() test IMO.
>>>> Oh, right - that's a PVH constraint that could probably not even
>>>> be thought of that the time the comment was written. I'm still
>>>> of the opinion though that at least the still applicable part of
>>>> the comment should be kept in place. Whether this means also
>>>> keeping in_atomic() itself is then an independent question, i.e.
>>>> I wouldn't consider it overly bad if there was no implementation
>>>> in the tree, but the above still served as documentation of what
>>>> would need to be re-added. Still my preference would be for it
>>>> to be kept.
>>> Would you be okay with replacing the removed stuff above with:
>>>
>>> /*
>>>  * If the required guest memory is paged out this function may sleep.
>>>  * So in theory we should bail out if called in atomic context.
>>>  * Unfortunately this is true for PVH dom0 doing domctl calls which
>> ... this is true at least for ...
>>
>>>  * holds the domctl lock when accessing dom0 memory. OTOH dom0 memory
>>>  * should never be paged out, so we are fine without testing for
>>>  * atomic context.
>>>  */
>> Not sure about this Dom0-specific remark: Are we certain there are
>> no other paths, similar to the gnttab one having been mentioned till
>> now?
> 
> Why is __hvm_copy() so special?  It is just one of many places which can
> end up touching guest memory.

Are you sure? I think everything that can touch guest (HVM) memory
actually ends up calling into this function.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] xen: drop preempt_count() for non-debug builds

[Juergen Gross]

On 22/05/2019 12:18, Jan Beulich wrote:
>>>> On 22.05.19 at 12:00, <andrew.cooper3@citrix.com> wrote:
>> On 22/05/2019 10:45, Juergen Gross wrote:
>>> preempt_count() and the associated per-cpu variable __preempt_count
>>> are tested in debug build only. So drop them for non-debug builds.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> I'd be tempted to fold patches 2 and 3 together, because they are both
>> the same change, and it would reduce the churn.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, ideally with the
>> two folded into one.
> 
> I'm a little surprised by this: Wasn't it you who generally
> wanted what ASSERT() expands to (controlled by NDEBUG)
> be independent of CONFIG_DEBUG, at some point down
> the road? Aren't you even having ASSERT()s enabled in
> release builds of XenServer, or am I misremembering? If so
> patch 3 would move us in the wrong direction.

A possibility to solve that would be the addition of
CONFIG_ASSERT defaulting to CONFIG_DEBUG.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel