Re: [PATCH 07/10] xen/physinfo: add arm SVE arch capability and vector length

From: Bertrand Marquis <Bertrand.Marquis@arm.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Luca Fancellu <Luca.Fancellu@arm.com>,
	Wei Chen <Wei.Chen@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 07/10] xen/physinfo: add arm SVE arch capability and vector length
Date: Thu, 23 Feb 2023 14:00:32 +0000	[thread overview]
Message-ID: <83BA2CB8-3CF5-4E4D-993C-026D0A19D277@arm.com> (raw)
In-Reply-To: <083e8a1f-8db6-350a-9138-58751c3fb44b@suse.com>

Hi Jan,

> On 13 Feb 2023, at 09:36, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 10.02.2023 16:54, Luca Fancellu wrote:
>>> On 2 Feb 2023, at 12:05, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 02.02.2023 12:08, Luca Fancellu wrote:
>>>> (is hw_cap only for x86?)
>>> 
>>> I suppose it is, but I also expect it would better go away than be moved.
>>> It doesn't hold a complete set of information, and it has been superseded.
>>> 
>>> Question is (and I think I did raise this before, perhaps in different
>>> context) whether Arm wouldn't want to follow x86 in how hardware as well
>>> as hypervisor derived / used ones are exposed to the tool stack
>>> (XEN_SYSCTL_get_cpu_featureset). I guess there's nothing really precluding
>>> that data to consist of more than just boolean fields.
>> 
>> Yes I guess that infrastructure could work, however I don’t have the bandwidth to
>> put it in place at the moment, so I would like the Arm maintainers to give me a
>> suggestion on how I can expose the vector length to XL if putting its value here
>> needs to be avoided
> 
> Since you've got a reply from Andrew boiling down to the same suggestion
> (or should I even say request), I guess it wants seriously considering
> to introduce abstract base infrastructure first. As Andrew says, time not
> invested now will very likely mean yet more time to be invested later.
> 
>>>> --- a/xen/include/public/sysctl.h
>>>> +++ b/xen/include/public/sysctl.h
>>>> @@ -18,7 +18,7 @@
>>>> #include "domctl.h"
>>>> #include "physdev.h"
>>>> 
>>>> -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
>>>> +#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016
>>> 
>>> Why? You ...
>>> 
>>>> @@ -104,7 +110,8 @@ struct xen_sysctl_physinfo {
>>>>    uint32_t cpu_khz;
>>>>    uint32_t capabilities;/* XEN_SYSCTL_PHYSCAP_??? */
>>>>    uint32_t arch_capabilities;/* XEN_SYSCTL_PHYSCAP_{X86,ARM,...}_??? */
>>>> -    uint32_t pad;
>>>> +    uint16_t arm_sve_vl_bits;
>>>> +    uint16_t pad;
>>>>    uint64_aligned_t total_pages;
>>>>    uint64_aligned_t free_pages;
>>>>    uint64_aligned_t scrub_pages;
>>> 
>>> ... add no new fields, and the only producer of the data zero-fills the
>>> struct first thing.
>> 
>> Yes that is right, I will wait to understand how I can proceed here:
>> 
>> 1) rename arch_capabilities to arm_sve_vl_bits and put vector length there.
>> 2) leave arch_capabilities untouched, no flag creation/setting, create uint32_t arm_sve_vl_bits field removing “pad”,
>>    Use its value to determine if SVE is present or not.
>> 3) ??
> 
> 3) Introduce suitable #define(s) to use part of arch_capabilities for your
> purpose, without renaming the field. (But that's of course only if Arm
> maintainers agree with you on _not_ going the proper feature handling route
> right away.)

As Luca said, he does not have the required bandwidth to do this so I think it is ok for him to go with your solution 3.

@Julien/Stefano: any thoughts here ?

Bertrand

> 
> Jan