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

[Jan Beulich <jbeulich@xxxxxxxx>]

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:
>>> This change aims to render the control interface of MSR intercepts identical
>>> between SVM and VMX code, so that the control of the MSR intercept in common
>>> code can be done through an hvm_funcs callback.
>>>
>>> Create two new functions:
>>> - svm_set_msr_intercept(), enables interception of read/write accesses to 
>>> the
>>>    corresponding MSR, by setting the corresponding read/write bits in the 
>>> MSRPM
>>>    based on the flags
>>> - svm_clear_msr_intercept(), disables interception of read/write accesses to
>>>    the corresponding MSR, by clearing the corresponding read/write bits in 
>>> the
>>>    MSRPM based on the flags
>>
>> In how far is VMX'es present model better than SVM's? They both have pros and
>> cons, depending on the specific use. I'm not asking to do it the other way
>> around (at least not yet), I'd merely like to understand why we're going to
>> gain two new hooks (if I'm not mistaken) when we could also get away with
>> just one.
> 
> SVM approach always touches both read/write bits (either by setting or 
> clearing them). I thought that using the SVM approach on VMX could be 
> considered a functional change (because there are parts where VMX 
> assumes that a bit is already set or cleared and does not touch it).

As per my comment on the last patch a question is whether both actually
need to become uniform. But if they do, then the new model should imo
be followed right away, and that VMX'es simply leaving bits alone when
already in known state.

>>> --- 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().

Jan