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

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

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:
> > > > 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.
But the SVM implementation does not assume. I can do it and remove the 
no functional change part.
> > > > --- 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.
--
Xenia