[PATCH 0/3] xenoprof: XSA-313 follow-up

[PATCH 0/3] xenoprof: XSA-313 follow-up

[Jan Beulich]

Patch 1 was considered to become part of the XSA, but it was then
decided against. The other two are a little bit of cleanup, albeit
there's certainly far more room for tidying. Yet then again Paul,
in his mail from Mar 13, was asking whether we shouldn't drop
xenoprof altogether, at which point cleaning up the code would be
wasted effort.

1: adjust ordering of page sharing vs domain type setting
2: drop unused struct xenoprof fields
3: limit scope of types and #define-s

Jan

RE: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 15 April 2020 09:45
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
> 
> Patch 1 was considered to become part of the XSA, but it was then
> decided against. The other two are a little bit of cleanup, albeit
> there's certainly far more room for tidying. Yet then again Paul,
> in his mail from Mar 13, was asking whether we shouldn't drop
> xenoprof altogether, at which point cleaning up the code would be
> wasted effort.
> 

That's still my opinion. This is a large chunk of (only passively maintained) code which I think is of very limited value (since it relates to an old tool, and it only works for PV domains).

  Paul

[PATCH 1/3] xenoprof: adjust ordering of page sharing vs domain type setting

[Jan Beulich]

Buffer pages should be shared with "ignored" or "active" guests only
(besides, obviously, the primary profiling domain). Hence domain type
should be set to "ignored" before unsharing from the primary domain
(which implies even a previously "passive" domain may then access its
buffers, albeit that's not very useful unless it gets promoted to
"active" subsequently), i.e. such that no further writes of records to
the buffer would occur, and (at least for consistency) also before
sharing it (with the calling domain) from the XENOPROF_get_buffer path.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wl@xen.org>

--- a/xen/common/xenoprof.c
+++ b/xen/common/xenoprof.c
@@ -372,8 +372,8 @@ static void reset_passive(struct domain
     if ( x == NULL )
         return;
 
-    unshare_xenoprof_page_with_guest(x);
     x->domain_type = XENOPROF_DOMAIN_IGNORED;
+    unshare_xenoprof_page_with_guest(x);
 }
 
 static void reset_active_list(void)
@@ -654,6 +654,13 @@ static int xenoprof_op_get_buffer(XEN_GU
         if ( ret < 0 )
             return ret;
     }
+    else
+    {
+        d->xenoprof->domain_ready = 0;
+        d->xenoprof->domain_type = XENOPROF_DOMAIN_IGNORED;
+    }
+
+    d->xenoprof->is_primary = (xenoprof_primary_profiler == d);
 
     ret = share_xenoprof_page_with_guest(
         d, virt_to_mfn(d->xenoprof->rawbuf), d->xenoprof->npages);
@@ -662,10 +669,6 @@ static int xenoprof_op_get_buffer(XEN_GU
 
     xenoprof_reset_buf(d);
 
-    d->xenoprof->domain_type  = XENOPROF_DOMAIN_IGNORED;
-    d->xenoprof->domain_ready = 0;
-    d->xenoprof->is_primary   = (xenoprof_primary_profiler == current->domain);
-        
     xenoprof_get_buffer.nbuf = d->xenoprof->nbuf;
     xenoprof_get_buffer.bufsize = d->xenoprof->bufsize;
     if ( !paging_mode_translate(d) )

[PATCH 2/3] xenoprof: drop unused struct xenoprof fields

[Jan Beulich]

Both is_primary and domain_ready are only ever written to. Drop both
fields and restrict structure visibility to just the one involved CU.
While doing so (and just for starters) make "is_compat" properly bool.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/xenoprof.c
+++ b/xen/common/xenoprof.c
@@ -33,6 +33,23 @@ static DEFINE_SPINLOCK(pmu_owner_lock);
 int pmu_owner = 0;
 int pmu_hvm_refcount = 0;
 
+struct xenoprof_vcpu {
+    int event_size;
+    xenoprof_buf_t *buffer;
+};
+
+struct xenoprof {
+    char *rawbuf;
+    int npages;
+    int nbuf;
+    int bufsize;
+    int domain_type;
+#ifdef CONFIG_COMPAT
+    bool is_compat;
+#endif
+    struct xenoprof_vcpu *vcpu;
+};
+
 static struct domain *active_domains[MAX_OPROF_DOMAINS];
 static int active_ready[MAX_OPROF_DOMAINS];
 static unsigned int adomains;
@@ -259,7 +276,6 @@ static int alloc_xenoprof_struct(
     d->xenoprof->npages = npages;
     d->xenoprof->nbuf = nvcpu;
     d->xenoprof->bufsize = bufsize;
-    d->xenoprof->domain_ready = 0;
     d->xenoprof->domain_type = XENOPROF_DOMAIN_IGNORED;
 
     /* Update buffer pointers for active vcpus */
@@ -327,7 +343,6 @@ static int set_active(struct domain *d)
     if ( x == NULL )
         return -EPERM;
 
-    x->domain_ready = 1;
     x->domain_type = XENOPROF_DOMAIN_ACTIVE;
     active_ready[ind] = 1;
     activated++;
@@ -348,7 +363,6 @@ static int reset_active(struct domain *d
     if ( x == NULL )
         return -EPERM;
 
-    x->domain_ready = 0;
     x->domain_type = XENOPROF_DOMAIN_IGNORED;
     active_ready[ind] = 0;
     active_domains[ind] = NULL;
@@ -655,12 +669,7 @@ static int xenoprof_op_get_buffer(XEN_GU
             return ret;
     }
     else
-    {
-        d->xenoprof->domain_ready = 0;
         d->xenoprof->domain_type = XENOPROF_DOMAIN_IGNORED;
-    }
-
-    d->xenoprof->is_primary = (xenoprof_primary_profiler == d);
 
     ret = share_xenoprof_page_with_guest(
         d, virt_to_mfn(d->xenoprof->rawbuf), d->xenoprof->npages);
--- a/xen/include/xen/xenoprof.h
+++ b/xen/include/xen/xenoprof.h
@@ -40,25 +40,6 @@ typedef union {
 } xenoprof_buf_t;
 #endif
 
-struct xenoprof_vcpu {
-    int event_size;
-    xenoprof_buf_t *buffer;
-};
-
-struct xenoprof {
-    char *rawbuf;
-    int npages;
-    int nbuf;
-    int bufsize;
-    int domain_type;
-    int domain_ready;
-    int is_primary;
-#ifdef CONFIG_COMPAT
-    int is_compat;
-#endif
-    struct xenoprof_vcpu *vcpu;
-};
-
 #ifndef CONFIG_COMPAT
 #define XENOPROF_COMPAT(x) 0
 #define xenoprof_buf(d, b, field) ACCESS_ONCE((b)->field)

[PATCH 3/3] xenoprof: limit scope of types and #define-s

[Jan Beulich]

Quite a few of the items are used by xenoprof.c only, so move them there
to limit their visibility as well as the amount of re-building needed in
case of changes. Also drop the inclusion of the public header there.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/oprofile/nmi_int.c
+++ b/xen/arch/x86/oprofile/nmi_int.c
@@ -19,7 +19,7 @@
 #include <xen/string.h>
 #include <xen/delay.h>
 #include <xen/xenoprof.h>
-#include <public/xen.h>
+#include <public/xenoprof.h>
 #include <asm/msr.h>
 #include <asm/apic.h>
 #include <asm/regs.h>
--- a/xen/common/xenoprof.c
+++ b/xen/common/xenoprof.c
@@ -23,6 +23,32 @@
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
+#define XENOPROF_DOMAIN_IGNORED    0
+#define XENOPROF_DOMAIN_ACTIVE     1
+#define XENOPROF_DOMAIN_PASSIVE    2
+
+#define XENOPROF_IDLE              0
+#define XENOPROF_INITIALIZED       1
+#define XENOPROF_COUNTERS_RESERVED 2
+#define XENOPROF_READY             3
+#define XENOPROF_PROFILING         4
+
+#ifndef CONFIG_COMPAT
+#define XENOPROF_COMPAT(x) false
+typedef struct xenoprof_buf xenoprof_buf_t;
+#define xenoprof_buf(d, b, field) ACCESS_ONCE((b)->field)
+#else
+#include <compat/xenoprof.h>
+#define XENOPROF_COMPAT(x) ((x)->is_compat)
+typedef union {
+    struct xenoprof_buf native;
+    struct compat_oprof_buf compat;
+} xenoprof_buf_t;
+#define xenoprof_buf(d, b, field) ACCESS_ONCE(*(!(d)->xenoprof->is_compat \
+                                                ? &(b)->native.field \
+                                                : &(b)->compat.field))
+#endif
+
 /* Limit amount of pages used for shared buffer (per domain) */
 #define MAX_OPROF_SHARED_PAGES 32
 
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -29,6 +29,7 @@
 #include <public/platform.h>
 #include <public/version.h>
 #include <public/hvm/params.h>
+#include <public/xenoprof.h>
 #include <public/xsm/flask_op.h>
 
 #include <avc.h>
--- a/xen/include/asm-x86/xenoprof.h
+++ b/xen/include/asm-x86/xenoprof.h
@@ -26,6 +26,8 @@ struct vcpu;
 
 #ifdef CONFIG_XENOPROF
 
+#include <public/xen.h>
+
 int nmi_reserve_counters(void);
 int nmi_setup_events(void);
 int nmi_enable_virq(void);
--- a/xen/include/xen/xenoprof.h
+++ b/xen/include/xen/xenoprof.h
@@ -11,7 +11,6 @@
 #define __XEN_XENOPROF_H__
 
 #include <xen/inttypes.h>
-#include <public/xenoprof.h>
 #include <asm/xenoprof.h>
 
 #define PMU_OWNER_NONE          0
@@ -20,37 +19,9 @@
 
 #ifdef CONFIG_XENOPROF
 
-#define XENOPROF_DOMAIN_IGNORED    0
-#define XENOPROF_DOMAIN_ACTIVE     1
-#define XENOPROF_DOMAIN_PASSIVE    2
-
-#define XENOPROF_IDLE              0
-#define XENOPROF_INITIALIZED       1
-#define XENOPROF_COUNTERS_RESERVED 2
-#define XENOPROF_READY             3
-#define XENOPROF_PROFILING         4
-
-#ifndef CONFIG_COMPAT
-typedef struct xenoprof_buf xenoprof_buf_t;
-#else
-#include <compat/xenoprof.h>
-typedef union {
-	struct xenoprof_buf native;
-	struct compat_oprof_buf compat;
-} xenoprof_buf_t;
-#endif
-
-#ifndef CONFIG_COMPAT
-#define XENOPROF_COMPAT(x) 0
-#define xenoprof_buf(d, b, field) ACCESS_ONCE((b)->field)
-#else
-#define XENOPROF_COMPAT(x) ((x)->is_compat)
-#define xenoprof_buf(d, b, field) ACCESS_ONCE(*(!(d)->xenoprof->is_compat \
-                                                ? &(b)->native.field \
-                                                : &(b)->compat.field))
-#endif
-
 struct domain;
+struct vcpu;
+struct cpu_user_regs;
 
 int acquire_pmu_ownership(int pmu_ownership);
 void release_pmu_ownership(int pmu_ownership);

Re: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Andrew Cooper]

On 15/04/2020 09:50, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 15 April 2020 09:45
>> To: xen-devel@lists.xenproject.org
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
>> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
>> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
>>
>> Patch 1 was considered to become part of the XSA, but it was then
>> decided against. The other two are a little bit of cleanup, albeit
>> there's certainly far more room for tidying. Yet then again Paul,
>> in his mail from Mar 13, was asking whether we shouldn't drop
>> xenoprof altogether, at which point cleaning up the code would be
>> wasted effort.
>>
> That's still my opinion. This is a large chunk of (only passively maintained) code which I think is of very limited value (since it relates to an old tool, and it only works for PV domains).

... and yet, noone has bothered getting any other profiler in to a
half-usable state.

You can already Kconfig it out, and yes it is a PITA to use on modern
systems where at the minimum, you need to patch the CPU model whitelist,
and in some cases extend the MSR whitelist in Xen, but at this point
where there are 0 viable alternatives for profiling, removing it would
be a deeply short-sighted move.

~Andrew

Re: [PATCH 2/3] xenoprof: drop unused struct xenoprof fields

[Wei Liu]

On Wed, Apr 15, 2020 at 10:53:20AM +0200, Jan Beulich wrote:
> Both is_primary and domain_ready are only ever written to. Drop both
> fields and restrict structure visibility to just the one involved CU.
> While doing so (and just for starters) make "is_compat" properly bool.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wl@xen.org>

Re: [PATCH 3/3] xenoprof: limit scope of types and #define-s

[Wei Liu]

On Wed, Apr 15, 2020 at 10:53:38AM +0200, Jan Beulich wrote:
> Quite a few of the items are used by xenoprof.c only, so move them there
> to limit their visibility as well as the amount of re-building needed in
> case of changes. Also drop the inclusion of the public header there.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I haven't read this patch carefully, but what it does is a good thing in
general and I trust our build test can catch any fallout from this sort
of change, so:

Acked-by: Wei Liu <wl@xen.org>

Re: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Julien Grall]

Hi Paul,

On 15/04/2020 09:50, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 15 April 2020 09:45
>> To: xen-devel@lists.xenproject.org
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
>> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
>> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
>>
>> Patch 1 was considered to become part of the XSA, but it was then
>> decided against. The other two are a little bit of cleanup, albeit
>> there's certainly far more room for tidying. Yet then again Paul,
>> in his mail from Mar 13, was asking whether we shouldn't drop
>> xenoprof altogether, at which point cleaning up the code would be
>> wasted effort.
>>
> 
> That's still my opinion. This is a large chunk of (only passively maintained) code which I think is of very limited value (since it relates to an old tool, and it only works for PV domains).

While there are no active user we are aware of, this is an example on 
how to implement a profiler backend with Xen. So I would agree with 
Andrew here.

IIRC, the reason behind your request is it makes difficult for your 
xenheap work. Am I correct? If so, do you have a thread explaining the 
issues?

Cheers,

-- 
Julien Grall

RE: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 20 April 2020 15:15
> To: paul@xen.org; 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org
> Cc: 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian
> Jackson' <ian.jackson@eu.citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu'
> <wl@xen.org>
> Subject: Re: [PATCH 0/3] xenoprof: XSA-313 follow-up
> 
> Hi Paul,
> 
> On 15/04/2020 09:50, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 15 April 2020 09:45
> >> To: xen-devel@lists.xenproject.org
> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian
> Jackson
> >> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> >> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
> >>
> >> Patch 1 was considered to become part of the XSA, but it was then
> >> decided against. The other two are a little bit of cleanup, albeit
> >> there's certainly far more room for tidying. Yet then again Paul,
> >> in his mail from Mar 13, was asking whether we shouldn't drop
> >> xenoprof altogether, at which point cleaning up the code would be
> >> wasted effort.
> >>
> >
> > That's still my opinion. This is a large chunk of (only passively maintained) code which I think is
> of very limited value (since it relates to an old tool, and it only works for PV domains).
> 
> While there are no active user we are aware of, this is an example on
> how to implement a profiler backend with Xen. So I would agree with
> Andrew here.
> 
> IIRC, the reason behind your request is it makes difficult for your
> xenheap work. Am I correct? If so, do you have a thread explaining the
> issues?

After shared info and grant table, it is the only other occurrence of a xenheap page shared with a non-system domain. Also, it cannot be trivially replaced with an 'extra' domheap page because its assignment changes. Thus a whole bunch of cleanup work that I was hoping to do (largely in domain_relinquish_resources and free_domheap_pages) is either ruled out, or would have to special-case this type of page.
Also, I am unconvinced that PV guests are sufficiently common these days (apart from dom0) that profiling them is of any real use.

  Paul

Re: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Julien Grall]

Hi Paul,

On 20/04/2020 16:01, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 20 April 2020 15:15
>> To: paul@xen.org; 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org
>> Cc: 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian
>> Jackson' <ian.jackson@eu.citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu'
>> <wl@xen.org>
>> Subject: Re: [PATCH 0/3] xenoprof: XSA-313 follow-up
>>
>> Hi Paul,
>>
>> On 15/04/2020 09:50, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 15 April 2020 09:45
>>>> To: xen-devel@lists.xenproject.org
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian
>> Jackson
>>>> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
>>>> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
>>>> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
>>>>
>>>> Patch 1 was considered to become part of the XSA, but it was then
>>>> decided against. The other two are a little bit of cleanup, albeit
>>>> there's certainly far more room for tidying. Yet then again Paul,
>>>> in his mail from Mar 13, was asking whether we shouldn't drop
>>>> xenoprof altogether, at which point cleaning up the code would be
>>>> wasted effort.
>>>>
>>>
>>> That's still my opinion. This is a large chunk of (only passively maintained) code which I think is
>> of very limited value (since it relates to an old tool, and it only works for PV domains).
>>
>> While there are no active user we are aware of, this is an example on
>> how to implement a profiler backend with Xen. So I would agree with
>> Andrew here.
>>
>> IIRC, the reason behind your request is it makes difficult for your
>> xenheap work. Am I correct? If so, do you have a thread explaining the
>> issues?
> 
> After shared info and grant table, it is the only other occurrence of a xenheap page shared with a non-system domain. Also, it cannot be trivially replaced with an 'extra' domheap page because its assignment changes. Thus a whole bunch of cleanup work that I was hoping to do (largely in domain_relinquish_resources and free_domheap_pages) is either ruled out, or would have to special-case this type of page.

My knowledge of xenoprof is very limited, so I might be wrong.

 From my understanding, a buffer can only be shared between two domains:
    - When in passive mode, the buffer will be shared with the primary 
profiler (always the hwdom per the check in xenoprof_op_init()).
    - When in active mode, the buffer will be shared with the domain 
requesting to be profiled.

Would it be possible to consider to have a separate buffer for the 
passive mode and active mode?

> Also, I am unconvinced that PV guests are sufficiently common these days (apart from dom0) that profiling them is of any real use.

Even an HVM guest can't profile itself, I think it would be useful to 
have dom0 to profile an HVM guest. Are you suggesting this doesn't work?

Cheers,

-- 
Julien Grall

RE: [PATCH 0/3] xenoprof: XSA-313 follow-up

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 24 April 2020 11:35
> To: paul@xen.org; 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org
> Cc: 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian
> Jackson' <ian.jackson@eu.citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu'
> <wl@xen.org>
> Subject: Re: [PATCH 0/3] xenoprof: XSA-313 follow-up
> 
> Hi Paul,
> 
> On 20/04/2020 16:01, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: 20 April 2020 15:15
> >> To: paul@xen.org; 'Jan Beulich' <jbeulich@suse.com>; xen-devel@lists.xenproject.org
> >> Cc: 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian
> >> Jackson' <ian.jackson@eu.citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Wei Liu'
> >> <wl@xen.org>
> >> Subject: Re: [PATCH 0/3] xenoprof: XSA-313 follow-up
> >>
> >> Hi Paul,
> >>
> >> On 15/04/2020 09:50, Paul Durrant wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 15 April 2020 09:45
> >>>> To: xen-devel@lists.xenproject.org
> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian
> >> Jackson
> >>>> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> >>>> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>
> >>>> Subject: [PATCH 0/3] xenoprof: XSA-313 follow-up
> >>>>
> >>>> Patch 1 was considered to become part of the XSA, but it was then
> >>>> decided against. The other two are a little bit of cleanup, albeit
> >>>> there's certainly far more room for tidying. Yet then again Paul,
> >>>> in his mail from Mar 13, was asking whether we shouldn't drop
> >>>> xenoprof altogether, at which point cleaning up the code would be
> >>>> wasted effort.
> >>>>
> >>>
> >>> That's still my opinion. This is a large chunk of (only passively maintained) code which I think
> is
> >> of very limited value (since it relates to an old tool, and it only works for PV domains).
> >>
> >> While there are no active user we are aware of, this is an example on
> >> how to implement a profiler backend with Xen. So I would agree with
> >> Andrew here.
> >>
> >> IIRC, the reason behind your request is it makes difficult for your
> >> xenheap work. Am I correct? If so, do you have a thread explaining the
> >> issues?
> >
> > After shared info and grant table, it is the only other occurrence of a xenheap page shared with a
> non-system domain. Also, it cannot be trivially replaced with an 'extra' domheap page because its
> assignment changes. Thus a whole bunch of cleanup work that I was hoping to do (largely in
> domain_relinquish_resources and free_domheap_pages) is either ruled out, or would have to special-case
> this type of page.
> 
> My knowledge of xenoprof is very limited, so I might be wrong.
> 
>  From my understanding, a buffer can only be shared between two domains:
>     - When in passive mode, the buffer will be shared with the primary
> profiler (always the hwdom per the check in xenoprof_op_init()).
>     - When in active mode, the buffer will be shared with the domain
> requesting to be profiled.
> 
> Would it be possible to consider to have a separate buffer for the
> passive mode and active mode?

I think that may well work, yes.

> 
> > Also, I am unconvinced that PV guests are sufficiently common these days (apart from dom0) that
> profiling them is of any real use.
> 
> Even an HVM guest can't profile itself, I think it would be useful to
> have dom0 to profile an HVM guest. Are you suggesting this doesn't work?
> 

No. That may work for a PV dom0; I'll have to try it. So, yes, passive profiling may indeed still have value.

  Paul