[PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage

[PATCH 0/2] KVM: Fix missing GFP_KERNEL_ACCOUNT usage

[Sean Christopherson]

Fix (almost) all cases in KVM x86 where allocations that are tied to a           
task/VM are not correctly accounted.                                            
                                                                                
There are a handful of allocations in SEV code that I intentionally didn't
fix in this series.  I'm 95% certain those allocations can be eliminated
completely, changing them in this series only to delete them seemed
pointless.

The allocations in questions are for structs that are used to communicate
 with the PSP; they are temporary (freed in the same function that does
the allocation) and small (some are _tiny_, e.g. 4 bytes).   AFAICT, the
sole reason they are dynamically allocated is because the CCP driver uses
__pa() to retrieve the physical address that is passed to the PSP, and
__pa() does not work for vmalloc'd memory, which is in play when running
with CONFIG_VMAP_STACKS=y.  

I have functional code that uses a scratch buffer as a bounce buffer to
cleanly handle vmalloc'd memory in the CCP driver.  I'll hopefully get
that posted tomorrow.

Sean Christopherson (2):
  KVM: Account memory allocations for 'struct kvm_vcpu'
  KVM: x86: Account a variety of miscellaneous allocations

 arch/x86/kvm/svm/nested.c | 4 ++--
 arch/x86/kvm/svm/sev.c    | 6 +++---
 arch/x86/kvm/vmx/vmx.c    | 2 +-
 virt/kvm/kvm_main.c       | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu'

[Sean Christopherson]

Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
tied to a single task/VM.  For x86, the allocations were accounted up
until the allocation code was moved to common KVM.  For all other
architectures, vCPU allocations were never previously accounted, but only
because most architectures lack accounting in general (for KVM).

Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 383df23514b9..3884e9f30251 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 	if (r)
 		goto vcpu_decrement;
 
-	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
+	vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
 	if (!vcpu) {
 		r = -ENOMEM;
 		goto vcpu_decrement;
-- 
2.31.0.291.g576ba9dcdaf-goog

[PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations

[Sean Christopherson]

Switch to GFP_KERNEL_ACCOUNT for a handful of allocations that are
clearly associated with a single task/VM.

Note, there are a several SEV allocations that aren't accounted, but
those can (hopefully) be fixed by using the local stack for memory.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/nested.c | 4 ++--
 arch/x86/kvm/svm/sev.c    | 6 +++---
 arch/x86/kvm/vmx/vmx.c    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 8523f60adb92..4f9e8b80ef99 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1234,8 +1234,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	ret  = -ENOMEM;
-	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL);
-	save = kzalloc(sizeof(*save), GFP_KERNEL);
+	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL_ACCOUNT);
+	save = kzalloc(sizeof(*save), GFP_KERNEL_ACCOUNT);
 	if (!ctl || !save)
 		goto out_free;
 
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 83e00e524513..883ce6bf23b9 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -637,7 +637,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		}
 
 		ret = -ENOMEM;
-		blob = kmalloc(params.len, GFP_KERNEL);
+		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
 			goto e_free;
 
@@ -1074,7 +1074,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		}
 
 		ret = -ENOMEM;
-		blob = kmalloc(params.len, GFP_KERNEL);
+		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
 		if (!blob)
 			goto e_free;
 
@@ -1775,7 +1775,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 			       len, GHCB_SCRATCH_AREA_LIMIT);
 			return false;
 		}
-		scratch_va = kzalloc(len, GFP_KERNEL);
+		scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);
 		if (!scratch_va)
 			return false;
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c8a4a548e96b..5ab25376d718 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -562,7 +562,7 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu)
 	 * evmcs in singe VM shares same assist page.
 	 */
 	if (!*p_hv_pa_pg)
-		*p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL);
+		*p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT);
 
 	if (!*p_hv_pa_pg)
 		return -ENOMEM;
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu'

[Wanpeng Li]

On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> wrote:
>
> Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> tied to a single task/VM.  For x86, the allocations were accounted up
> until the allocation code was moved to common KVM.  For all other
> architectures, vCPU allocations were never previously accounted, but only
> because most architectures lack accounting in general (for KVM).
>
> Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 383df23514b9..3884e9f30251 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
>         if (r)
>                 goto vcpu_decrement;
>
> -       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> +       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);

kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
flag will guarantee further slab alloc will be charged to memcg.
Please refer to memcg_slab_pre_alloc_hook(). So the patch is
unnecessary.

    Wanpeng

Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu'

[Sean Christopherson]

On Wed, Mar 31, 2021, Wanpeng Li wrote:
> On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> wrote:
> >
> > Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> > tied to a single task/VM.  For x86, the allocations were accounted up
> > until the allocation code was moved to common KVM.  For all other
> > architectures, vCPU allocations were never previously accounted, but only
> > because most architectures lack accounting in general (for KVM).
> >
> > Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  virt/kvm/kvm_main.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 383df23514b9..3884e9f30251 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> >         if (r)
> >                 goto vcpu_decrement;
> >
> > -       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> > +       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
> 
> kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
> flag will guarantee further slab alloc will be charged to memcg.
> Please refer to memcg_slab_pre_alloc_hook(). So the patch is
> unnecessary.

Hmm, I missed that.  However, AFICT only SLAB/SLUB enforce SLAB_ACCOUNT, SLOB
does not appear to honor the flag.   The caveat to SLOB is that the
GFP_KERNEL_ACCOUNT will only come into play when allocating new pages, and so
allocations smaller than a page will be accounted incorrectly (I think).
But, a vcpu is larger than a page (on x86), which means the vcpu allocation will
always be correctly accounted.

I've no idea if anyone actually uses KVM+SLOB, let alone cares about accounting
in the that case.  But, it would be nice for KVM to be consistent with the other
kmem_cache usage in KVM, all of which do double up on SLAB_ACCOUNT +
GFP_KERNEL_ACCOUNT.

Maybe rewrite the changelog and drop the Fixes?

Re: [PATCH 1/2] KVM: Account memory allocations for 'struct kvm_vcpu'

[Wanpeng Li]

On Wed, 31 Mar 2021 at 11:24, Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Mar 31, 2021, Wanpeng Li wrote:
> > On Wed, 31 Mar 2021 at 10:32, Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > Use GFP_KERNEL_ACCOUNT for the vCPU allocations, the vCPUs are very much
> > > tied to a single task/VM.  For x86, the allocations were accounted up
> > > until the allocation code was moved to common KVM.  For all other
> > > architectures, vCPU allocations were never previously accounted, but only
> > > because most architectures lack accounting in general (for KVM).
> > >
> > > Fixes: e529ef66e6b5 ("KVM: Move vcpu alloc and init invocation to common code")
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  virt/kvm/kvm_main.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 383df23514b9..3884e9f30251 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -3182,7 +3182,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> > >         if (r)
> > >                 goto vcpu_decrement;
> > >
> > > -       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
> > > +       vcpu = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL_ACCOUNT);
> >
> > kvm_vcpu_cache is created with SLAB_ACCOUNT flag in kvm_init(), this
> > flag will guarantee further slab alloc will be charged to memcg.
> > Please refer to memcg_slab_pre_alloc_hook(). So the patch is
> > unnecessary.
>
> Hmm, I missed that.  However, AFICT only SLAB/SLUB enforce SLAB_ACCOUNT, SLOB
> does not appear to honor the flag.   The caveat to SLOB is that the
> GFP_KERNEL_ACCOUNT will only come into play when allocating new pages, and so
> allocations smaller than a page will be accounted incorrectly (I think).
> But, a vcpu is larger than a page (on x86), which means the vcpu allocation will
> always be correctly accounted.
>
> I've no idea if anyone actually uses KVM+SLOB, let alone cares about accounting

I asked maintainer Christoph in 2013, he told me "Well, I have never
seen non experimental systems that use SLOB. Others have claimed they
exist. It's mostly of academic interest."

> in the that case.  But, it would be nice for KVM to be consistent with the other
> kmem_cache usage in KVM, all of which do double up on SLAB_ACCOUNT +
> GFP_KERNEL_ACCOUNT.
>
> Maybe rewrite the changelog and drop the Fixes?

Agreed.

    Wanpeng

Re: [PATCH 2/2] KVM: x86: Account a variety of miscellaneous allocations

[Paolo Bonzini]

On 31/03/21 04:30, Sean Christopherson wrote:
> Switch to GFP_KERNEL_ACCOUNT for a handful of allocations that are
> clearly associated with a single task/VM.
> 
> Note, there are a several SEV allocations that aren't accounted, but
> those can (hopefully) be fixed by using the local stack for memory.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/svm/nested.c | 4 ++--
>   arch/x86/kvm/svm/sev.c    | 6 +++---
>   arch/x86/kvm/vmx/vmx.c    | 2 +-
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 8523f60adb92..4f9e8b80ef99 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1234,8 +1234,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>   		return -EINVAL;
>   
>   	ret  = -ENOMEM;
> -	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL);
> -	save = kzalloc(sizeof(*save), GFP_KERNEL);
> +	ctl  = kzalloc(sizeof(*ctl),  GFP_KERNEL_ACCOUNT);
> +	save = kzalloc(sizeof(*save), GFP_KERNEL_ACCOUNT);
>   	if (!ctl || !save)
>   		goto out_free;
>   
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 83e00e524513..883ce6bf23b9 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -637,7 +637,7 @@ static int sev_launch_measure(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		}
>   
>   		ret = -ENOMEM;
> -		blob = kmalloc(params.len, GFP_KERNEL);
> +		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
>   		if (!blob)
>   			goto e_free;
>   
> @@ -1074,7 +1074,7 @@ static int sev_get_attestation_report(struct kvm *kvm, struct kvm_sev_cmd *argp)
>   		}
>   
>   		ret = -ENOMEM;
> -		blob = kmalloc(params.len, GFP_KERNEL);
> +		blob = kmalloc(params.len, GFP_KERNEL_ACCOUNT);
>   		if (!blob)
>   			goto e_free;
>   
> @@ -1775,7 +1775,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   			       len, GHCB_SCRATCH_AREA_LIMIT);
>   			return false;
>   		}
> -		scratch_va = kzalloc(len, GFP_KERNEL);
> +		scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT);
>   		if (!scratch_va)
>   			return false;
>   
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c8a4a548e96b..5ab25376d718 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -562,7 +562,7 @@ static int hv_enable_direct_tlbflush(struct kvm_vcpu *vcpu)
>   	 * evmcs in singe VM shares same assist page.
>   	 */
>   	if (!*p_hv_pa_pg)
> -		*p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +		*p_hv_pa_pg = kzalloc(PAGE_SIZE, GFP_KERNEL_ACCOUNT);
>   
>   	if (!*p_hv_pa_pg)
>   		return -ENOMEM;
> 

Queued, thanks.

Paolo