[PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[Krister Johansen]

This problem was encountered on an arm64 system with a lot of memory.
Without kernel debug symbols installed, and with both kcore and kallsyms
available, perf managed to get confused and returned "unknown" for all
of the kernel symbols that it tried to look up.

On this system, stext fell within the vmalloc segment.  The kcore symbol
matching code tries to find the first segment that contains stext and
uses that to replace the segment generated from just the kallsyms
information.  In this case, however, there were two: a very large
vmalloc segment, and the text segment.  This caused perf to get confused
because multiple overlapping segments were inserted into the RB tree
that holds the discovered segments.  However, that alone wasn't
sufficient to cause the problem. Even when we could find the segment,
the offsets were adjusted in such a way that the newly generated symbols
didn't line up with the instruction addresses in the trace.  The most
obvious solution would be to consult which segment type is text from
kcore, but this information is not exposed to users.

Instead, select the smallest matching segment that contains stext
instead of the first matching segment.  This allows us to match the text
segment instead of vmalloc, if one is contained within the other.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
 tools/perf/util/symbol.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a3a165ae933a..14ac4189eaff 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 
 	/* Find the kernel map using the '_stext' symbol */
 	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
+		u64 replacement_size = 0;
 		list_for_each_entry(new_map, &md.maps, node) {
-			if (stext >= new_map->start && stext < new_map->end) {
+			u64 new_size = new_map->end - new_map->start;
+
+			if (!(stext >= new_map->start && stext < new_map->end))
+				continue;
+
+			if (!replacement_map || new_size < replacement_size) {
 				replacement_map = new_map;
-				break;
+				replacement_size = new_size;
 			}
 		}
 	}
-- 
2.25.1

Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[Adrian Hunter]

On 25/01/23 00:35, Krister Johansen wrote:
> This problem was encountered on an arm64 system with a lot of memory.
> Without kernel debug symbols installed, and with both kcore and kallsyms
> available, perf managed to get confused and returned "unknown" for all
> of the kernel symbols that it tried to look up.
> 
> On this system, stext fell within the vmalloc segment.  The kcore symbol
> matching code tries to find the first segment that contains stext and
> uses that to replace the segment generated from just the kallsyms
> information.  In this case, however, there were two: a very large
> vmalloc segment, and the text segment.  This caused perf to get confused
> because multiple overlapping segments were inserted into the RB tree
> that holds the discovered segments.  However, that alone wasn't
> sufficient to cause the problem. Even when we could find the segment,
> the offsets were adjusted in such a way that the newly generated symbols
> didn't line up with the instruction addresses in the trace.  The most
> obvious solution would be to consult which segment type is text from
> kcore, but this information is not exposed to users.
> 
> Instead, select the smallest matching segment that contains stext
> instead of the first matching segment.  This allows us to match the text
> segment instead of vmalloc, if one is contained within the other.
> 
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> ---
>  tools/perf/util/symbol.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a3a165ae933a..14ac4189eaff 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  
>  	/* Find the kernel map using the '_stext' symbol */
>  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> +		u64 replacement_size = 0;

We'd usually put a blank line here

>  		list_for_each_entry(new_map, &md.maps, node) {
> -			if (stext >= new_map->start && stext < new_map->end) {
> +			u64 new_size = new_map->end - new_map->start;
> +
> +			if (!(stext >= new_map->start && stext < new_map->end))
> +				continue;
> +

Really needs a comment, and please be specific e.g.

 ARM64 vmalloc segment overlaps the kernel text segment, so
 choosing the smaller segment will get the kernel text.



> +			if (!replacement_map || new_size < replacement_size) {
>  				replacement_map = new_map;
> -				break;
> +				replacement_size = new_size;
>  			}
>  		}
>  	}

Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[Adrian Hunter]

On 25/01/23 09:29, Adrian Hunter wrote:

Also subject line has spelling mistake, and should identify kcore
as the issue e.g.

perf symbol: Symbol lookup with kcore can fail if multiple segments match stext

> On 25/01/23 00:35, Krister Johansen wrote:
>> This problem was encountered on an arm64 system with a lot of memory.
>> Without kernel debug symbols installed, and with both kcore and kallsyms
>> available, perf managed to get confused and returned "unknown" for all
>> of the kernel symbols that it tried to look up.
>>
>> On this system, stext fell within the vmalloc segment.  The kcore symbol
>> matching code tries to find the first segment that contains stext and
>> uses that to replace the segment generated from just the kallsyms
>> information.  In this case, however, there were two: a very large
>> vmalloc segment, and the text segment.  This caused perf to get confused
>> because multiple overlapping segments were inserted into the RB tree
>> that holds the discovered segments.  However, that alone wasn't
>> sufficient to cause the problem. Even when we could find the segment,
>> the offsets were adjusted in such a way that the newly generated symbols
>> didn't line up with the instruction addresses in the trace.  The most
>> obvious solution would be to consult which segment type is text from
>> kcore, but this information is not exposed to users.
>>
>> Instead, select the smallest matching segment that contains stext
>> instead of the first matching segment.  This allows us to match the text
>> segment instead of vmalloc, if one is contained within the other.
>>
>> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
>> ---
>>  tools/perf/util/symbol.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index a3a165ae933a..14ac4189eaff 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>>  
>>  	/* Find the kernel map using the '_stext' symbol */
>>  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
>> +		u64 replacement_size = 0;
> 
> We'd usually put a blank line here
> 
>>  		list_for_each_entry(new_map, &md.maps, node) {
>> -			if (stext >= new_map->start && stext < new_map->end) {
>> +			u64 new_size = new_map->end - new_map->start;
>> +
>> +			if (!(stext >= new_map->start && stext < new_map->end))
>> +				continue;
>> +
> 
> Really needs a comment, and please be specific e.g.
> 
>  ARM64 vmalloc segment overlaps the kernel text segment, so
>  choosing the smaller segment will get the kernel text.
> 
> 
> 
>> +			if (!replacement_map || new_size < replacement_size) {
>>  				replacement_map = new_map;
>> -				break;
>> +				replacement_size = new_size;
>>  			}
>>  		}
>>  	}
> 

Re: [PATCH] perf/util: Symbol lookup can fail if multiple segmets match stext

[Krister Johansen]

Hi Adrian,

On Wed, Jan 25, 2023 at 09:37:59AM +0200, Adrian Hunter wrote:
> On 25/01/23 09:29, Adrian Hunter wrote:
> 
> Also subject line has spelling mistake, and should identify kcore
> as the issue e.g.
> 
> perf symbol: Symbol lookup with kcore can fail if multiple segments match stext
> 
> > On 25/01/23 00:35, Krister Johansen wrote:
> >> This problem was encountered on an arm64 system with a lot of memory.
> >> Without kernel debug symbols installed, and with both kcore and kallsyms
> >> available, perf managed to get confused and returned "unknown" for all
> >> of the kernel symbols that it tried to look up.
> >>
> >> On this system, stext fell within the vmalloc segment.  The kcore symbol
> >> matching code tries to find the first segment that contains stext and
> >> uses that to replace the segment generated from just the kallsyms
> >> information.  In this case, however, there were two: a very large
> >> vmalloc segment, and the text segment.  This caused perf to get confused
> >> because multiple overlapping segments were inserted into the RB tree
> >> that holds the discovered segments.  However, that alone wasn't
> >> sufficient to cause the problem. Even when we could find the segment,
> >> the offsets were adjusted in such a way that the newly generated symbols
> >> didn't line up with the instruction addresses in the trace.  The most
> >> obvious solution would be to consult which segment type is text from
> >> kcore, but this information is not exposed to users.
> >>
> >> Instead, select the smallest matching segment that contains stext
> >> instead of the first matching segment.  This allows us to match the text
> >> segment instead of vmalloc, if one is contained within the other.
> >>
> >> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> >> ---
> >>  tools/perf/util/symbol.c | 10 ++++++++--
> >>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> >> index a3a165ae933a..14ac4189eaff 100644
> >> --- a/tools/perf/util/symbol.c
> >> +++ b/tools/perf/util/symbol.c
> >> @@ -1368,10 +1368,16 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >>  
> >>  	/* Find the kernel map using the '_stext' symbol */
> >>  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> >> +		u64 replacement_size = 0;
> > 
> > We'd usually put a blank line here
> > 
> >>  		list_for_each_entry(new_map, &md.maps, node) {
> >> -			if (stext >= new_map->start && stext < new_map->end) {
> >> +			u64 new_size = new_map->end - new_map->start;
> >> +
> >> +			if (!(stext >= new_map->start && stext < new_map->end))
> >> +				continue;
> >> +
> > 
> > Really needs a comment, and please be specific e.g.
> > 
> >  ARM64 vmalloc segment overlaps the kernel text segment, so
> >  choosing the smaller segment will get the kernel text.
> > 
> > 
> > 
> >> +			if (!replacement_map || new_size < replacement_size) {
> >>  				replacement_map = new_map;
> >> -				break;
> >> +				replacement_size = new_size;
> >>  			}
> >>  		}
> >>  	}

Thanks for all the feedback.  I'll incorporate these changes and send
out a v2 shortly.

-K

[PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Krister Johansen]

Cpuid leaf 4 contains information about how the state of the tsc, its
mode, and some additional information.  A commit that is queued for
linux would like to use this to determine whether the tsc mode has been
set to 'no emulation' in order to make some decisions about which
clocksource is more reliable.

Expose this information in the public API headers so that they can
subsequently be imported into linux and used there.

Link: https://lore.kernel.org/xen-devel/eda8d9f2-3013-1b68-0df8-64d7f13ee35e@suse.com/
Link: https://lore.kernel.org/xen-devel/0835453d-9617-48d5-b2dc-77a2ac298bad@oracle.com/
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
v2:
  - Fix whitespace between comment and #defines (feedback from Jan Beulich)
  - Add tsc mode 3: no emulate TSC_AUX (feedback from Jan Beulich)
---
 xen/include/public/arch-x86/cpuid.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index 7ecd16ae05..090f7f0034 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -72,6 +72,14 @@
  * Sub-leaf 2: EAX: host tsc frequency in kHz
  */
 
+#define XEN_CPUID_TSC_EMULATED               (1u << 0)
+#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
+#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
+#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
+#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
+#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
+#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
+
 /*
  * Leaf 5 (0x40000x04)
  * HVM-specific features
-- 
2.25.1

[PATCH v2] perf/util: Symbol lookup with kcore can fail if multiple segments match stext

[Krister Johansen]

This problem was encountered on an arm64 system with a lot of memory.
Without kernel debug symbols installed, and with both kcore and kallsyms
available, perf managed to get confused and returned "unknown" for all
of the kernel symbols that it tried to look up.

On this system, stext fell within the vmalloc segment.  The kcore symbol
matching code tries to find the first segment that contains stext and
uses that to replace the segment generated from just the kallsyms
information.  In this case, however, there were two: a very large
vmalloc segment, and the text segment.  This caused perf to get confused
because multiple overlapping segments were inserted into the RB tree
that holds the discovered segments.  However, that alone wasn't
sufficient to cause the problem. Even when we could find the segment,
the offsets were adjusted in such a way that the newly generated symbols
didn't line up with the instruction addresses in the trace.  The most
obvious solution would be to consult which segment type is text from
kcore, but this information is not exposed to users.

Instead, select the smallest matching segment that contains stext
instead of the first matching segment.  This allows us to match the text
segment instead of vmalloc, if one is contained within the other.

Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
v2:
  - Correct whitespace, add comments, and fix-up subject. (Feedback from Adrian
    Hunter)
---
 tools/perf/util/symbol.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a3a165ae933a..98014f937568 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1368,10 +1368,23 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 
 	/* Find the kernel map using the '_stext' symbol */
 	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
+		u64 replacement_size = 0;
+
 		list_for_each_entry(new_map, &md.maps, node) {
-			if (stext >= new_map->start && stext < new_map->end) {
+			u64 new_size = new_map->end - new_map->start;
+
+			if (!(stext >= new_map->start && stext < new_map->end))
+				continue;
+
+			/*
+			 * On some architectures, ARM64 for example, the kernel
+			 * text can get allocated inside of the vmalloc segment.
+			 * Select the smallest matching segment, in case stext
+			 * falls within more than one in the list.
+			 */
+			if (!replacement_map || new_size < replacement_size) {
 				replacement_map = new_map;
-				break;
+				replacement_size = new_size;
 			}
 		}
 	}
-- 
2.25.1

Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if multiple segments match stext

[Adrian Hunter]

On 25/01/23 20:34, Krister Johansen wrote:
> This problem was encountered on an arm64 system with a lot of memory.
> Without kernel debug symbols installed, and with both kcore and kallsyms
> available, perf managed to get confused and returned "unknown" for all
> of the kernel symbols that it tried to look up.
> 
> On this system, stext fell within the vmalloc segment.  The kcore symbol
> matching code tries to find the first segment that contains stext and
> uses that to replace the segment generated from just the kallsyms
> information.  In this case, however, there were two: a very large
> vmalloc segment, and the text segment.  This caused perf to get confused
> because multiple overlapping segments were inserted into the RB tree
> that holds the discovered segments.  However, that alone wasn't
> sufficient to cause the problem. Even when we could find the segment,
> the offsets were adjusted in such a way that the newly generated symbols
> didn't line up with the instruction addresses in the trace.  The most
> obvious solution would be to consult which segment type is text from
> kcore, but this information is not exposed to users.
> 
> Instead, select the smallest matching segment that contains stext
> instead of the first matching segment.  This allows us to match the text
> segment instead of vmalloc, if one is contained within the other.
> 
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> v2:
>   - Correct whitespace, add comments, and fix-up subject. (Feedback from Adrian
>     Hunter)
> ---
>  tools/perf/util/symbol.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index a3a165ae933a..98014f937568 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1368,10 +1368,23 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  
>  	/* Find the kernel map using the '_stext' symbol */
>  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> +		u64 replacement_size = 0;
> +
>  		list_for_each_entry(new_map, &md.maps, node) {
> -			if (stext >= new_map->start && stext < new_map->end) {
> +			u64 new_size = new_map->end - new_map->start;
> +
> +			if (!(stext >= new_map->start && stext < new_map->end))
> +				continue;
> +
> +			/*
> +			 * On some architectures, ARM64 for example, the kernel
> +			 * text can get allocated inside of the vmalloc segment.
> +			 * Select the smallest matching segment, in case stext
> +			 * falls within more than one in the list.
> +			 */
> +			if (!replacement_map || new_size < replacement_size) {
>  				replacement_map = new_map;
> -				break;
> +				replacement_size = new_size;
>  			}
>  		}
>  	}

Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if multiple segments match stext

[Arnaldo Carvalho de Melo]

Em Mon, Jan 30, 2023 at 03:29:54PM +0200, Adrian Hunter escreveu:
> On 25/01/23 20:34, Krister Johansen wrote:
> > This problem was encountered on an arm64 system with a lot of memory.
> > Without kernel debug symbols installed, and with both kcore and kallsyms
> > available, perf managed to get confused and returned "unknown" for all
> > of the kernel symbols that it tried to look up.
> > 
> > On this system, stext fell within the vmalloc segment.  The kcore symbol
> > matching code tries to find the first segment that contains stext and
> > uses that to replace the segment generated from just the kallsyms
> > information.  In this case, however, there were two: a very large
> > vmalloc segment, and the text segment.  This caused perf to get confused
> > because multiple overlapping segments were inserted into the RB tree
> > that holds the discovered segments.  However, that alone wasn't
> > sufficient to cause the problem. Even when we could find the segment,
> > the offsets were adjusted in such a way that the newly generated symbols
> > didn't line up with the instruction addresses in the trace.  The most
> > obvious solution would be to consult which segment type is text from
> > kcore, but this information is not exposed to users.
> > 
> > Instead, select the smallest matching segment that contains stext
> > instead of the first matching segment.  This allows us to match the text
> > segment instead of vmalloc, if one is contained within the other.
> > 
> > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> 
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Thanks, applied.

- Arnaldo

 
> > ---
> > v2:
> >   - Correct whitespace, add comments, and fix-up subject. (Feedback from Adrian
> >     Hunter)
> > ---
> >  tools/perf/util/symbol.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index a3a165ae933a..98014f937568 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1368,10 +1368,23 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >  
> >  	/* Find the kernel map using the '_stext' symbol */
> >  	if (!kallsyms__get_function_start(kallsyms_filename, "_stext", &stext)) {
> > +		u64 replacement_size = 0;
> > +
> >  		list_for_each_entry(new_map, &md.maps, node) {
> > -			if (stext >= new_map->start && stext < new_map->end) {
> > +			u64 new_size = new_map->end - new_map->start;
> > +
> > +			if (!(stext >= new_map->start && stext < new_map->end))
> > +				continue;
> > +
> > +			/*
> > +			 * On some architectures, ARM64 for example, the kernel
> > +			 * text can get allocated inside of the vmalloc segment.
> > +			 * Select the smallest matching segment, in case stext
> > +			 * falls within more than one in the list.
> > +			 */
> > +			if (!replacement_map || new_size < replacement_size) {
> >  				replacement_map = new_map;
> > -				break;
> > +				replacement_size = new_size;
> >  			}
> >  		}
> >  	}
> 

-- 

- Arnaldo

Re: [PATCH v2] perf/util: Symbol lookup with kcore can fail if multiple segments match stext

[Krister Johansen]

On Wed, Feb 01, 2023 at 10:00:51PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 30, 2023 at 03:29:54PM +0200, Adrian Hunter escreveu:
> > On 25/01/23 20:34, Krister Johansen wrote:
> > > This problem was encountered on an arm64 system with a lot of memory.
> > > Without kernel debug symbols installed, and with both kcore and kallsyms
> > > available, perf managed to get confused and returned "unknown" for all
> > > of the kernel symbols that it tried to look up.
> > > 
> > > On this system, stext fell within the vmalloc segment.  The kcore symbol
> > > matching code tries to find the first segment that contains stext and
> > > uses that to replace the segment generated from just the kallsyms
> > > information.  In this case, however, there were two: a very large
> > > vmalloc segment, and the text segment.  This caused perf to get confused
> > > because multiple overlapping segments were inserted into the RB tree
> > > that holds the discovered segments.  However, that alone wasn't
> > > sufficient to cause the problem. Even when we could find the segment,
> > > the offsets were adjusted in such a way that the newly generated symbols
> > > didn't line up with the instruction addresses in the trace.  The most
> > > obvious solution would be to consult which segment type is text from
> > > kcore, but this information is not exposed to users.
> > > 
> > > Instead, select the smallest matching segment that contains stext
> > > instead of the first matching segment.  This allows us to match the text
> > > segment instead of vmalloc, if one is contained within the other.
> > > 
> > > Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
> > 
> > Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>
> 
> Thanks, applied.

Thanks, Arnaldo.  If it's not too late, would you be willing to tag this
one as:

Fixes: 5654997838c2 ("perf tools: Use the "_stest" symbol to identify the kernel map when loading kcore")
Cc: <stable@vger.kernel.org>

So that it makes it to the right stable branches?  I meant to do this
prior to sending it along.

-K

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Krister Johansen]

On Fri, Jan 27, 2023 at 08:16:18AM +0100, Jan Beulich wrote:
> On 26.01.2023 19:02, Krister Johansen wrote:
> > On Thu, Jan 26, 2023 at 10:57:01AM +0100, Jan Beulich wrote:
> >> On 25.01.2023 19:45, Krister Johansen wrote:
> >>> --- a/xen/include/public/arch-x86/cpuid.h
> >>> +++ b/xen/include/public/arch-x86/cpuid.h
> >>> @@ -72,6 +72,14 @@
> >>>   * Sub-leaf 2: EAX: host tsc frequency in kHz
> >>>   */
> >>>  
> >>> +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
> >>> +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
> >>> +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
> >>> +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
> >>> +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
> >>> +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
> >>> +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
> >>
> >> Actually I think we'd better stick to the names found in asm/time.h
> >> (and then replace their uses, dropping the #define-s there). If you
> >> agree, I'd be happy to make the adjustment while committing.
> > 
> > Just to confirm, this would be moving these:
> > 
> >    #define TSC_MODE_DEFAULT          0
> >    #define TSC_MODE_ALWAYS_EMULATE   1
> >    #define TSC_MODE_NEVER_EMULATE    2
> >    
> > To cpuid.h?  I'm generally fine with this.  I don't see anything in
> > Linux that's using these names.  The only question I have is whether
> > we'd still want to prefix the names with XEN so that if they're pulled
> > in to Linux it's clear that the define is Xen specific?  E.g. something
> > like this perhaps?
> > 
> >    #define XEN_TSC_MODE_DEFAULT          0
> >    #define XEN_TSC_MODE_ALWAYS_EMULATE   1
> >    #define XEN_TSC_MODE_NEVER_EMULATE    2
> > 
> > That does increase the number of files we'd need to touch to make the
> > change, though. (And the other defines in that file all start with
> > XEN_CPUID).
> > 
> > Though, if you mean doing it this way:
> > 
> >    #define XEN_CPUID_TSC_MODE_DEFAULT          0
> >    #define XEN_CPUID_TSC_MODE_ALWAYS_EMULATE   1
> >    #define XEN_CPUID_TSC_MODE_NEVER_EMULATE    2
> >  
> > then no objection to that at all.  Apologies for overlooking the naming
> > overlap when I put this together the first time.
> 
> Yes, it's the last variant you list that I was after. And I'd be okay to
> leave dropping the so far private constants to a separate follow-on patch.

Ok, thanks. I'll send you a v3 that makes these changes, unless you've
already fixed this up and committed the v2.  In that case, feel free to
disregard.

-K

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Jan Beulich]

On 26.01.2023 19:02, Krister Johansen wrote:
> On Thu, Jan 26, 2023 at 10:57:01AM +0100, Jan Beulich wrote:
>> On 25.01.2023 19:45, Krister Johansen wrote:
>>> --- a/xen/include/public/arch-x86/cpuid.h
>>> +++ b/xen/include/public/arch-x86/cpuid.h
>>> @@ -72,6 +72,14 @@
>>>   * Sub-leaf 2: EAX: host tsc frequency in kHz
>>>   */
>>>  
>>> +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
>>> +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
>>> +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
>>> +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
>>> +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
>>> +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
>>> +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
>>
>> Actually I think we'd better stick to the names found in asm/time.h
>> (and then replace their uses, dropping the #define-s there). If you
>> agree, I'd be happy to make the adjustment while committing.
> 
> Just to confirm, this would be moving these:
> 
>    #define TSC_MODE_DEFAULT          0
>    #define TSC_MODE_ALWAYS_EMULATE   1
>    #define TSC_MODE_NEVER_EMULATE    2
>    
> To cpuid.h?  I'm generally fine with this.  I don't see anything in
> Linux that's using these names.  The only question I have is whether
> we'd still want to prefix the names with XEN so that if they're pulled
> in to Linux it's clear that the define is Xen specific?  E.g. something
> like this perhaps?
> 
>    #define XEN_TSC_MODE_DEFAULT          0
>    #define XEN_TSC_MODE_ALWAYS_EMULATE   1
>    #define XEN_TSC_MODE_NEVER_EMULATE    2
> 
> That does increase the number of files we'd need to touch to make the
> change, though. (And the other defines in that file all start with
> XEN_CPUID).
> 
> Though, if you mean doing it this way:
> 
>    #define XEN_CPUID_TSC_MODE_DEFAULT          0
>    #define XEN_CPUID_TSC_MODE_ALWAYS_EMULATE   1
>    #define XEN_CPUID_TSC_MODE_NEVER_EMULATE    2
>  
> then no objection to that at all.  Apologies for overlooking the naming
> overlap when I put this together the first time.

Yes, it's the last variant you list that I was after. And I'd be okay to
leave dropping the so far private constants to a separate follow-on patch.

Jan

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Krister Johansen]

On Thu, Jan 26, 2023 at 10:57:01AM +0100, Jan Beulich wrote:
> On 25.01.2023 19:45, Krister Johansen wrote:
> > --- a/xen/include/public/arch-x86/cpuid.h
> > +++ b/xen/include/public/arch-x86/cpuid.h
> > @@ -72,6 +72,14 @@
> >   * Sub-leaf 2: EAX: host tsc frequency in kHz
> >   */
> >  
> > +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
> > +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
> > +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
> > +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
> > +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
> > +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
> > +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
> 
> Actually I think we'd better stick to the names found in asm/time.h
> (and then replace their uses, dropping the #define-s there). If you
> agree, I'd be happy to make the adjustment while committing.

Just to confirm, this would be moving these:

   #define TSC_MODE_DEFAULT          0
   #define TSC_MODE_ALWAYS_EMULATE   1
   #define TSC_MODE_NEVER_EMULATE    2
   
To cpuid.h?  I'm generally fine with this.  I don't see anything in
Linux that's using these names.  The only question I have is whether
we'd still want to prefix the names with XEN so that if they're pulled
in to Linux it's clear that the define is Xen specific?  E.g. something
like this perhaps?

   #define XEN_TSC_MODE_DEFAULT          0
   #define XEN_TSC_MODE_ALWAYS_EMULATE   1
   #define XEN_TSC_MODE_NEVER_EMULATE    2

That does increase the number of files we'd need to touch to make the
change, though. (And the other defines in that file all start with
XEN_CPUID).

Though, if you mean doing it this way:

   #define XEN_CPUID_TSC_MODE_DEFAULT          0
   #define XEN_CPUID_TSC_MODE_ALWAYS_EMULATE   1
   #define XEN_CPUID_TSC_MODE_NEVER_EMULATE    2
 
then no objection to that at all.  Apologies for overlooking the naming
overlap when I put this together the first time.

-K

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Krister Johansen]

On Thu, Jan 26, 2023 at 09:57:43AM +0100, Jan Beulich wrote:
> On 25.01.2023 19:45, Krister Johansen wrote:
> > v2:
> >   - Fix whitespace between comment and #defines (feedback from Jan Beulich)
> 
> Hmm, ...
> 
> > --- a/xen/include/public/arch-x86/cpuid.h
> > +++ b/xen/include/public/arch-x86/cpuid.h
> > @@ -72,6 +72,14 @@
> >   * Sub-leaf 2: EAX: host tsc frequency in kHz
> >   */
> >  
> > +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
> > +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
> > +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
> > +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
> > +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
> > +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
> > +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
> 
> ... while I'm fine with the leading blank line, what my earlier comment was
> about really are the two separate blocks of #define-s (the flag bits and the
> modes). I'll take care of this while committing; with the adjustment
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Sorry I miunderstood, and thanks for being willing to fix this up
while committing.

-K

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Jan Beulich]

On 25.01.2023 19:45, Krister Johansen wrote:
> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -72,6 +72,14 @@
>   * Sub-leaf 2: EAX: host tsc frequency in kHz
>   */
>  
> +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
> +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
> +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
> +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
> +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
> +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
> +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)

Actually I think we'd better stick to the names found in asm/time.h
(and then replace their uses, dropping the #define-s there). If you
agree, I'd be happy to make the adjustment while committing.

Jan

Re: [PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Jan Beulich]

On 25.01.2023 19:45, Krister Johansen wrote:
> v2:
>   - Fix whitespace between comment and #defines (feedback from Jan Beulich)

Hmm, ...

> --- a/xen/include/public/arch-x86/cpuid.h
> +++ b/xen/include/public/arch-x86/cpuid.h
> @@ -72,6 +72,14 @@
>   * Sub-leaf 2: EAX: host tsc frequency in kHz
>   */
>  
> +#define XEN_CPUID_TSC_EMULATED               (1u << 0)
> +#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
> +#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
> +#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
> +#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
> +#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
> +#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)

... while I'm fine with the leading blank line, what my earlier comment was
about really are the two separate blocks of #define-s (the flag bits and the
modes). I'll take care of this while committing; with the adjustment

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

Jan

[PATCH v2] xen/x86: public: add TSC defines for cpuid leaf 4

[Krister Johansen]

Cpuid leaf 4 contains information about how the state of the tsc, its
mode, and some additional information.  A commit that is queued for
linux would like to use this to determine whether the tsc mode has been
set to 'no emulation' in order to make some decisions about which
clocksource is more reliable.

Expose this information in the public API headers so that they can
subsequently be imported into linux and used there.

Link: https://lore.kernel.org/xen-devel/eda8d9f2-3013-1b68-0df8-64d7f13ee35e@suse.com/
Link: https://lore.kernel.org/xen-devel/0835453d-9617-48d5-b2dc-77a2ac298bad@oracle.com/
Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>
---
v2.1:
  - Correct In-Reply-To header for proper threading
v2:
  - Fix whitespace between comment and #defines (feedback from Jan Beulich)
  - Add tsc mode 3: no emulate TSC_AUX (feedback from Jan Beulich)
---
 xen/include/public/arch-x86/cpuid.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index 7ecd16ae05..090f7f0034 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -72,6 +72,14 @@
  * Sub-leaf 2: EAX: host tsc frequency in kHz
  */
 
+#define XEN_CPUID_TSC_EMULATED               (1u << 0)
+#define XEN_CPUID_HOST_TSC_RELIABLE          (1u << 1)
+#define XEN_CPUID_RDTSCP_INSTR_AVAIL         (1u << 2)
+#define XEN_CPUID_TSC_MODE_DEFAULT           (0)
+#define XEN_CPUID_TSC_MODE_EMULATE           (1u)
+#define XEN_CPUID_TSC_MODE_NOEMULATE         (2u)
+#define XEN_CPUID_TSC_MODE_NOEMULATE_TSC_AUX (3u)
+
 /*
  * Leaf 5 (0x40000x04)
  * HVM-specific features
-- 
2.25.1