linux-kernel - Re: [PATCH v7 04/12] KVM: arm64: Make vcpu flag macros more generic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1a843735-0a94-4df7-a8b4-cad68ee2449a@linaro.org>
Date: Mon, 18 Nov 2024 09:22:27 +0000
From: James Clark <james.clark@...aro.org>
To: Marc Zyngier <maz@...nel.org>
Cc: suzuki.poulose@....com, oliver.upton@...ux.dev,
 coresight@...ts.linaro.org, kvmarm@...ts.linux.dev,
 Joey Gouly <joey.gouly@....com>, Zenghui Yu <yuzenghui@...wei.com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Mike Leach <mike.leach@...aro.org>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Mark Rutland <mark.rutland@....com>, Mark Brown <broonie@...nel.org>,
 Anshuman Khandual <anshuman.khandual@....com>,
 James Morse <james.morse@....com>, Fuad Tabba <tabba@...gle.com>,
 Shiqi Liu <shiqiliu@...t.edu.cn>, Raghavendra Rao Ananta
 <rananta@...gle.com>, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 04/12] KVM: arm64: Make vcpu flag macros more generic



On 18/11/2024 9:00 am, Marc Zyngier wrote:
> On Tue, 12 Nov 2024 10:37:03 +0000,
> James Clark <james.clark@...aro.org> wrote:
>>
>> Rename vcpu_* to kvm_* so that the same flags mechanism can be used in
>> places other than vcpu without being confusing. Wherever macros are
>> still related to vcpu like vcpu_get_flag() with hard coded v->arch, keep
>> the vcpu_* name, otherwise change it.
>>
>> Also move the "v->arch" access one macro higher for the same reason.
>>
>> This will be used for moving flags to host_data in a later commit.
>>
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>>   arch/arm64/include/asm/kvm_host.h | 88 +++++++++++++++----------------
>>   arch/arm64/kvm/hyp/exception.c    | 12 ++---
>>   arch/arm64/kvm/inject_fault.c     |  4 +-
>>   arch/arm64/kvm/mmio.c             | 10 ++--
>>   4 files changed, 57 insertions(+), 57 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index f333b189fb43..34aa59f498c4 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -790,22 +790,22 @@ struct kvm_vcpu_arch {
>>   /*
>>    * Each 'flag' is composed of a comma-separated triplet:
>>    *
>> - * - the flag-set it belongs to in the vcpu->arch structure
>> + * - the flag-set it belongs to in the structure pointed to by 'v'
>>    * - the value for that flag
>>    * - the mask for that flag
>>    *
>> - *  __vcpu_single_flag() builds such a triplet for a single-bit flag.
>> - * unpack_vcpu_flag() extract the flag value from the triplet for
>> + *  __kvm_single_flag() builds such a triplet for a single-bit flag.
>> + * unpack_kvm_flag() extract the flag value from the triplet for
>>    * direct use outside of the flag accessors.
>>    */
>> -#define __vcpu_single_flag(_set, _f)	_set, (_f), (_f)
>> +#define __kvm_single_flag(_set, _f)	_set, (_f), (_f)
>>   
>>   #define __unpack_flag(_set, _f, _m)	_f
>> -#define unpack_vcpu_flag(...)		__unpack_flag(__VA_ARGS__)
>> +#define unpack_kvm_flag(...)		__unpack_flag(__VA_ARGS__)
>>   
>>   #define __build_check_flag(v, flagset, f, m)			\
>>   	do {							\
>> -		typeof(v->arch.flagset) *_fset;			\
>> +		typeof(v.flagset) *_fset;			\
>>   								\
>>   		/* Check that the flags fit in the mask */	\
>>   		BUILD_BUG_ON(HWEIGHT(m) != HWEIGHT((f) | (m)));	\
>> @@ -813,11 +813,11 @@ struct kvm_vcpu_arch {
>>   		BUILD_BUG_ON((sizeof(*_fset) * 8) <= __fls(m));	\
>>   	} while (0)
>>   
>> -#define __vcpu_get_flag(v, flagset, f, m)			\
>> +#define __kvm_get_flag(v, flagset, f, m)			\
>>   	({							\
>>   		__build_check_flag(v, flagset, f, m);		\
>>   								\
>> -		READ_ONCE(v->arch.flagset) & (m);		\
>> +		READ_ONCE(v.flagset) & (m);		\
>>   	})
>>   
>>   /*
>> @@ -826,64 +826,64 @@ struct kvm_vcpu_arch {
>>    */
>>   #ifdef __KVM_NVHE_HYPERVISOR__
>>   /* the nVHE hypervisor is always non-preemptible */
>> -#define __vcpu_flags_preempt_disable()
>> -#define __vcpu_flags_preempt_enable()
>> +#define __kvm_flags_preempt_disable()
>> +#define __kvm_flags_preempt_enable()
>>   #else
>> -#define __vcpu_flags_preempt_disable()	preempt_disable()
>> -#define __vcpu_flags_preempt_enable()	preempt_enable()
>> +#define __kvm_flags_preempt_disable()	preempt_disable()
>> +#define __kvm_flags_preempt_enable()	preempt_enable()
>>   #endif
>>   
>> -#define __vcpu_set_flag(v, flagset, f, m)			\
>> +#define __kvm_set_flag(v, flagset, f, m)			\
> 
> Hell no. Never. The whole point of this naming is that we know what
> this applies to. Here, you might as well have replaced 'vcpu' with
> 'carrot', and the result would be the same.
> 
> Not to mention the insane churn this generates.
> 
> So no, not happening.
> 
> 	M.
> 

Fair enough, I wasn't feeling to strongly about this either, was just 
anticipating that there might be objection to bare flags if this more 
abstracted mechanism existed elsewhere.

Looks like Oliver already did it with just flags for the same end goal 
here [1], so I will drop this.

[1]: 
https://lore.kernel.org/kvmarm/20241115224924.2132364-4-oliver.upton@linux.dev/T/#u