| Message ID | 20240423165328.2853870-4-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Fix supported VM_TYPES caps | expand |
On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote: > Zero out all of kvm_caps when loading a new vendor module to ensure that > KVM can't inadvertently rely on global initialization of a field, and add > a comment above the definition of kvm_caps to call out that all fields > needs to be explicitly computed during vendor module load. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 44ce187bad89..8f3979d5fc80 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -92,6 +92,11 @@ > #define MAX_IO_MSRS 256 > #define KVM_MAX_MCE_BANKS 32 > > +/* > + * Note, kvm_caps fields should *never* have default values, all fields must be > + * recomputed from scratch during vendor module load, e.g. to account for a > + * vendor module being reloaded with different module parameters. > + */ > struct kvm_caps kvm_caps __read_mostly; > EXPORT_SYMBOL_GPL(kvm_caps); > > @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > return -EIO; > } > > + memset(&kvm_caps, 0, sizeof(kvm_caps)); > + > x86_emulator_cache = kvm_alloc_emulator_cache(); > if (!x86_emulator_cache) { > pr_err("failed to allocate cache for x86 emulator\n"); Why do the memset() here particularly? Isn't it better to put ... memset(&kvm_caps, 0, sizeof(kvm_caps)); kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM); kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P; ... together so it can be easily seen? We can even have a helper to do above to "reset kvm_caps to default values" I think.
On Wed, Apr 24, 2024, Kai Huang wrote: > On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote: > > Zero out all of kvm_caps when loading a new vendor module to ensure that > > KVM can't inadvertently rely on global initialization of a field, and add > > a comment above the definition of kvm_caps to call out that all fields > > needs to be explicitly computed during vendor module load. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/x86.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 44ce187bad89..8f3979d5fc80 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -92,6 +92,11 @@ > > #define MAX_IO_MSRS 256 > > #define KVM_MAX_MCE_BANKS 32 > > > > +/* > > + * Note, kvm_caps fields should *never* have default values, all fields must be > > + * recomputed from scratch during vendor module load, e.g. to account for a > > + * vendor module being reloaded with different module parameters. > > + */ > > struct kvm_caps kvm_caps __read_mostly; > > EXPORT_SYMBOL_GPL(kvm_caps); > > > > @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > > return -EIO; > > } > > > > + memset(&kvm_caps, 0, sizeof(kvm_caps)); > > + > > x86_emulator_cache = kvm_alloc_emulator_cache(); > > if (!x86_emulator_cache) { > > pr_err("failed to allocate cache for x86 emulator\n"); > > Why do the memset() here particularly? So that it happens as early as possible, e.g. in case kvm_mmu_vendor_module_init() or some other function comes along and modifies kvm_caps. > Isn't it better to put ... > > memset(&kvm_caps, 0, sizeof(kvm_caps)); > kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM); > kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P; > > ... together so it can be easily seen? > > We can even have a helper to do above to "reset kvm_caps to default > values" I think. Hmm, I don't think a helper is necessary, but I do agree that having all of the explicit initialization in one place would be better. The alternative would be to use |= for BIT(KVM_X86_DEFAULT_VM), and MCG_CTL_P | MCG_SER_P, but I don't think that would be an improvement. I'll tweak the first two patches to set the hardcoded caps earlier. The main reason I don't want to add a helper is that coming up with a name would be tricky. E.g. "kvm_reset_caps()" isn't a great fit because the caps are "reset" throughout module loading. "kvm_set_default_caps()" kinda fits, but they aren't so much that they are KVM's defaults, rather they are the caps that KVM can always support regardless of hardware support, e.g. supported_xcr0 isn't optional, it just depends on hardware.
On Wed, 2024-04-24 at 08:33 -0700, Sean Christopherson wrote: > On Wed, Apr 24, 2024, Kai Huang wrote: > > On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote: > > > Zero out all of kvm_caps when loading a new vendor module to ensure that > > > KVM can't inadvertently rely on global initialization of a field, and add > > > a comment above the definition of kvm_caps to call out that all fields > > > needs to be explicitly computed during vendor module load. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/x86.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > index 44ce187bad89..8f3979d5fc80 100644 > > > --- a/arch/x86/kvm/x86.c > > > +++ b/arch/x86/kvm/x86.c > > > @@ -92,6 +92,11 @@ > > > #define MAX_IO_MSRS 256 > > > #define KVM_MAX_MCE_BANKS 32 > > > > > > +/* > > > + * Note, kvm_caps fields should *never* have default values, all fields must be > > > + * recomputed from scratch during vendor module load, e.g. to account for a > > > + * vendor module being reloaded with different module parameters. > > > + */ > > > struct kvm_caps kvm_caps __read_mostly; > > > EXPORT_SYMBOL_GPL(kvm_caps); > > > > > > @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > > > return -EIO; > > > } > > > > > > + memset(&kvm_caps, 0, sizeof(kvm_caps)); > > > + > > > x86_emulator_cache = kvm_alloc_emulator_cache(); > > > if (!x86_emulator_cache) { > > > pr_err("failed to allocate cache for x86 emulator\n"); > > > > Why do the memset() here particularly? > > So that it happens as early as possible, e.g. in case kvm_mmu_vendor_module_init() > or some other function comes along and modifies kvm_caps. > > > Isn't it better to put ... > > > > memset(&kvm_caps, 0, sizeof(kvm_caps)); > > kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM); > > kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P; > > > > ... together so it can be easily seen? > > > > We can even have a helper to do above to "reset kvm_caps to default > > values" I think. > > Hmm, I don't think a helper is necessary, but I do agree that having all of the > explicit initialization in one place would be better. The alternative would be > to use |= for BIT(KVM_X86_DEFAULT_VM), and MCG_CTL_P | MCG_SER_P, but I don't > think that would be an improvement. I'll tweak the first two patches to set the > hardcoded caps earlier. > > The main reason I don't want to add a helper is that coming up with a name would > be tricky. E.g. "kvm_reset_caps()" isn't a great fit because the caps are "reset" > throughout module loading. "kvm_set_default_caps()" kinda fits, but they aren't > so much that they are KVM's defaults, rather they are the caps that KVM can always > support regardless of hardware support, e.g. supported_xcr0 isn't optional, it > just depends on hardware. Personally I am fine with kvm_set_default_caps(), but obviously no strong opinion here. :-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 44ce187bad89..8f3979d5fc80 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -92,6 +92,11 @@ #define MAX_IO_MSRS 256 #define KVM_MAX_MCE_BANKS 32 +/* + * Note, kvm_caps fields should *never* have default values, all fields must be + * recomputed from scratch during vendor module load, e.g. to account for a + * vendor module being reloaded with different module parameters. + */ struct kvm_caps kvm_caps __read_mostly; EXPORT_SYMBOL_GPL(kvm_caps); @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) return -EIO; } + memset(&kvm_caps, 0, sizeof(kvm_caps)); + x86_emulator_cache = kvm_alloc_emulator_cache(); if (!x86_emulator_cache) { pr_err("failed to allocate cache for x86 emulator\n");
Zero out all of kvm_caps when loading a new vendor module to ensure that KVM can't inadvertently rely on global initialization of a field, and add a comment above the definition of kvm_caps to call out that all fields needs to be explicitly computed during vendor module load. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 7 +++++++ 1 file changed, 7 insertions(+)