Re: [PATCH] KVM: X86: Dynamically allocating MSR number lists(msrs_to_save[], emulated_msrs[], msr_based_features[])

[cqiang]

On 11/5/2019 9:03 PM, Paolo Bonzini wrote:
> On 05/11/19 13:51, Xiaoyao Li wrote:
> > On 11/5/2019 7:30 PM, Paolo Bonzini wrote:
> > > On 05/11/19 10:20, Chenyi Qiang wrote:
> > > > The three msr number lists(msrs_to_save[], emulated_msrs[] and
> > > > msr_based_features[]) are global arrays of kvm.ko, which are
> > > > initialized/adjusted (copy supported MSRs forward to override the
> > > > unsupported MSRs) when installing kvm-{intel,amd}.ko, but it doesn't
> > > > reset these three arrays to their initial value when uninstalling
> > > > kvm-{intel,amd}.ko. Thus, at the next installation, kvm-{intel,amd}.ko
> > > > will initialize the modified arrays with some MSRs lost and some MSRs
> > > > duplicated.
> > > >
> > > > So allocate and initialize these three MSR number lists dynamically when
> > > > installing kvm-{intel,amd}.ko and free them when uninstalling.
> > > >
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Reviewed-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> > > > Signed-off-by: Chenyi Qiang <chenyi.qiang@xxxxxxxxx>
> > > > ---
> > > >  Â arch/x86/kvm/x86.c | 86 ++++++++++++++++++++++++++++++----------------
> > > >  Â 1 file changed, 57 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > index ff395f812719..08efcf6351cc 100644
> > > > --- a/arch/x86/kvm/x86.c
> > > > +++ b/arch/x86/kvm/x86.c
> > > > @@ -1132,13 +1132,15 @@ EXPORT_SYMBOL_GPL(kvm_rdpmc);
> > > >  ÂÂ * List of msr numbers which we expose to userspace through
> > > > KVM_GET_MSRS
> > > >  ÂÂ * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST.
> > > >  ÂÂ *
> > > > - * This list is modified at module load time to reflect the
> > > > + * The three msr number lists(msrs_to_save, emulated_msrs,
> > > > msr_based_features)
> > > > + * are allocated and initialized at module load time and freed at
> > > > unload time.
> > > > + * msrs_to_save is selected from the msrs_to_save_all to reflect the
> > > >  ÂÂ * capabilities of the host cpu. This capabilities test skips MSRs
> > > > that are
> > > > - * kvm-specific. Those are put in emulated_msrs; filtering of
> > > > emulated_msrs
> > > > + * kvm-specific. Those are put in emulated_msrs_all; filtering of
> > > > emulated_msrs
> > > >  ÂÂ * may depend on host virtualization features rather than host cpu
> > > > features.
> > > >  ÂÂ */
> > > >  Â -static u32 msrs_to_save[] = {
> > > > +const u32 msrs_to_save_all[] = {
> > >
> > > This can remain static.
> >
> > How about static const u32 msrs_to_save_all[] ?
> >
> > Or you think static is enough?
>
> "static const" is best indeed (that's what I meant, but I wasn't very
> clear).

Yes, considering the read-only property and scope of these arrays, 
"static const" is more accurate. Thanks.

> Paolo
>
> > > >  ÂÂÂÂÂ MSR_IA32_SYSENTER_CS, MSR_IA32_SYSENTER_ESP,
> > > > MSR_IA32_SYSENTER_EIP,
> > > >  ÂÂÂÂÂ MSR_STAR,
> > > >  Â #ifdef CONFIG_X86_64
> > > > @@ -1179,9 +1181,10 @@ static u32 msrs_to_save[] = {
> > > >  ÂÂÂÂÂ MSR_ARCH_PERFMON_EVENTSEL0 + 16, MSR_ARCH_PERFMON_EVENTSEL0 + 17,
> > > >  Â };
> > > >  Â +static u32 *msrs_to_save;
> > >
> > > You can use ARRAY_SIZE to allocate the destination arrays statically.
> >
> > It's much better, then we don't need to allocation and free.

Got it. Thanks!

> > > Paolo