Re: [PATCH 2/4] x86/svm: split svm_intercept_msr() into svm_{set,clear}_msr_intercept()

From: Jan Beulich <jbeulich@suse.com>
To: Xenia Ragiadakou <burzalodowa@gmail.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 2/4] x86/svm: split svm_intercept_msr() into svm_{set,clear}_msr_intercept()
Date: Tue, 28 Feb 2023 17:14:59 +0100	[thread overview]
Message-ID: <6e2fe766-1560-0f44-047b-d914048ef34e@suse.com> (raw)
In-Reply-To: <afc00a90-753c-18c1-65ed-8b8635874b2a@gmail.com>

On 28.02.2023 16:17, Xenia Ragiadakou wrote:
> On 2/28/23 17:10, Jan Beulich wrote:
>> On 28.02.2023 16:05, Xenia Ragiadakou wrote:
>>> On 2/28/23 16:20, Jan Beulich wrote:
>>>> On 27.02.2023 08:56, Xenia Ragiadakou wrote:
>>>>> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>>>> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
>>>>> @@ -585,13 +585,12 @@ void svm_destroy_vmcb(struct vcpu *v);
>>>>>    
>>>>>    void setup_vmcb_dump(void);
>>>>>    
>>>>> -#define MSR_INTERCEPT_NONE    0
>>>>> -#define MSR_INTERCEPT_READ    1
>>>>> -#define MSR_INTERCEPT_WRITE   2
>>>>> -#define MSR_INTERCEPT_RW      (MSR_INTERCEPT_WRITE | MSR_INTERCEPT_READ)
>>>>> -void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable);
>>>>> -#define svm_disable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_NONE)
>>>>> -#define svm_enable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_RW)
>>>>> +void svm_set_msr_intercept(struct vcpu *v, uint32_t msr, int flags);
>>>>> +void svm_clear_msr_intercept(struct vcpu *v, uint32_t msr, int flags);
>>>>> +#define svm_disable_intercept_for_msr(v, msr) \
>>>>> +    svm_clear_msr_intercept((v), (msr), MSR_RW)
>>>>> +#define svm_enable_intercept_for_msr(v, msr) \
>>>>> +    svm_set_intercept_msr((v), (msr), MSR_RW)
>>>>
>>>> Please avoid excess parentheses. Also could you clarify why you retain
>>>> these shorthands when you don't use them in the conversion that you're
>>>> doing (e.g. ())? Are you intending them to go
>>>> away down the road?
>>>
>>> Ok.
>>> I did not understand the question. Which shorthands?
>>
>> svm_disable_intercept_for_msr() and svm_enable_intercept_for_msr().
> 
> Are you suggesting to replace svm_{en,dis}able_intercept_for_msr() with 
> svm_{ser,clear}_msr_intercept()?  svm_disable_intercept_for_msr() is 
> used in svm.c and vmcb.c.

I'm suggesting one of two possible routes leading to consistent use:
1) drop the shorthands
2) retain the shorthands and don't ever open-code them
Depending on which route we want to go either your code adjustments in
this regard are fine, and only a remark would want adding that they're
retained until remaining uses can be cleaned up, or you want to use
them in your changes wherever possible.

Jan