* [PATCH v3 0/2] kvm: sev: Add SNP guest request throttling @ 2023-01-19 21:34 Dionna Glaze 2023-01-19 21:34 ` [PATCH v3 1/2] kvm: sev: Add SEV-SNP " Dionna Glaze 2023-01-19 21:34 ` [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest Dionna Glaze 0 siblings, 2 replies; 5+ messages in thread From: Dionna Glaze @ 2023-01-19 21:34 UTC (permalink / raw) To: kvm, linux-kernel, x86; +Cc: Dionna Glaze This patch series is based on [RFC,v7,00/64] Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support The GHCB specification recommends that SNP guest requests should be rate limited. This 2 patch series adds such rate limiting with a 2 burst, 2 second interval per VM as the default values for two new kvm-amd module parameters: guest_request_throttle_s guest_request_throttle_burst This patch series cooperates with the guest series, Add throttling detection to sev-guest in order for guests to retry when throttled, rather than disable the VMPCK and fail to complete their request. Changes since v2: * Rebased on v7, changed "we" wording to passive voice. Changes since v1: * Added missing Ccs to patches. Dionna Glaze (2): kvm: sev: Add SEV-SNP guest request throttling kvm: sev: If ccp is busy, report throttled to guest arch/x86/include/asm/sev-common.h | 1 + arch/x86/kvm/svm/sev.c | 47 +++++++++++++++++++++++++++++-- arch/x86/kvm/svm/svm.h | 3 ++ include/uapi/linux/in.h | 1 + 4 files changed, 50 insertions(+), 2 deletions(-) -- 2.39.0.246.g2a6d74b583-goog ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/2] kvm: sev: Add SEV-SNP guest request throttling 2023-01-19 21:34 [PATCH v3 0/2] kvm: sev: Add SNP guest request throttling Dionna Glaze @ 2023-01-19 21:34 ` Dionna Glaze 2023-01-20 16:40 ` Sean Christopherson 2023-01-19 21:34 ` [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest Dionna Glaze 1 sibling, 1 reply; 5+ messages in thread From: Dionna Glaze @ 2023-01-19 21:34 UTC (permalink / raw) To: kvm, linux-kernel, x86 Cc: Dionna Glaze, Thomas Lendacky, Paolo Bonzini, Joerg Roedel, Peter Gonda, Borislav Petkov The AMD-SP is a precious resource that doesn't have a scheduler other than a mutex lock queue. To avoid customers from causing a DoS, a module_param-set rate limit is added with a default of 2 requests per 2 seconds. These defaults were chosen empirically with a the assumption that current server-grade SEV-SNP machines will rarely exceed 128 VMs under usual circumstance. The 2 burst per 2 seconds means on average 1 request every second. We allow 2 requests back to back to allow for the guest to query the certificate length in an extended guest request without a pause. The 1 second average is our target for quality of service since empirical tests show that 64 VMs can concurrently request an attestation report with a maximum latency of 1 second. We don't anticipate more concurrency than that for a seldom used request for a majority well- behaved set of VMs. The majority point is decided as >64 VMs given the assumed 128 VM count for "extreme load". The throttling code is 2 << 32 given that invalid length is 1 and 2 is the next available code. This was suggested by Tom Lendacky, and will be included in a new revision of the GHCB specification. Cc: Thomas Lendacky <Thomas.Lendacky@amd.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Joerg Roedel <jroedel@suse.de> Cc: Peter Gonda <pgonda@google.com> Cc: Borislav Petkov <bp@alien8.de> Signed-off-by: Dionna Glaze <dionnaglaze@google.com> --- arch/x86/include/asm/sev-common.h | 1 + arch/x86/kvm/svm/sev.c | 29 +++++++++++++++++++++++++++++ arch/x86/kvm/svm/svm.h | 3 +++ include/uapi/linux/in.h | 1 + 4 files changed, 34 insertions(+) diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h index 1b111cde8c82..e3a6b039480d 100644 --- a/arch/x86/include/asm/sev-common.h +++ b/arch/x86/include/asm/sev-common.h @@ -158,6 +158,7 @@ struct snp_psc_desc { /* Guest message request error code */ #define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) +#define SNP_GUEST_REQ_THROTTLED (((u64)2) << 32) #define GHCB_MSR_TERM_REQ 0x100 #define GHCB_MSR_TERM_REASON_SET_POS 12 diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index d0e58cffd1ed..cd9372ce6fc2 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -58,6 +58,14 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444); /* enable/disable SEV-SNP support */ static bool sev_snp_enabled = true; module_param_named(sev_snp, sev_snp_enabled, bool, 0444); + +/* Throttle guest requests to a burst # per this many seconds */ +unsigned int guest_request_throttle_s = 2; +module_param(guest_request_throttle_s, int, 0444); + +/* Throttle guest requests to this many per the above many seconds */ +unsigned int guest_request_throttle_burst = 2; +module_param(guest_request_throttle_burst, int, 0444); #else #define sev_enabled false #define sev_es_enabled false @@ -333,6 +341,9 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) goto e_free; mutex_init(&sev->guest_req_lock); + ratelimit_state_init(&sev->snp_guest_msg_rs, + guest_request_throttle_s * HZ, + guest_request_throttle_burst); ret = sev_snp_init(&argp->error, false); } else { ret = sev_platform_init(&argp->error); @@ -3595,6 +3606,14 @@ static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsig *rc = SEV_RET_INVALID_ADDRESS; } +static bool snp_throttle_guest_request(struct kvm_sev_info *sev) { + if (__ratelimit(&sev->snp_guest_msg_rs)) + return false; + + pr_info_ratelimited("svm: too many guest message requests\n"); + return true; +} + static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa) { struct sev_data_snp_guest_request data = {0}; @@ -3611,6 +3630,11 @@ static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t sev = &to_kvm_svm(kvm)->sev_info; + if (snp_throttle_guest_request(sev)) { + rc = SNP_GUEST_REQ_THROTTLED; + goto e_fail; + } + mutex_lock(&sev->guest_req_lock); rc = snp_setup_guest_buf(svm, &data, req_gpa, resp_gpa); @@ -3648,6 +3672,11 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp sev = &to_kvm_svm(kvm)->sev_info; + if (snp_throttle_guest_request(sev)) { + rc = SNP_GUEST_REQ_THROTTLED; + goto e_fail; + } + data_gpa = vcpu->arch.regs[VCPU_REGS_RAX]; data_npages = vcpu->arch.regs[VCPU_REGS_RBX]; diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index 8d1ba66860a4..7048f817efb0 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -18,6 +18,7 @@ #include <linux/kvm_types.h> #include <linux/kvm_host.h> #include <linux/bits.h> +#include <linux/ratelimit.h> #include <asm/svm.h> #include <asm/sev-common.h> @@ -105,6 +106,8 @@ struct kvm_sev_info { unsigned int snp_certs_len; /* Size of instance override for certs */ struct mutex guest_req_lock; + struct ratelimit_state snp_guest_msg_rs; /* Limit guest requests */ + u64 sev_features; /* Features set at VMSA creation */ }; diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h index f243ce665f74..07a4cb149305 100644 --- a/include/uapi/linux/in.h +++ b/include/uapi/linux/in.h @@ -20,6 +20,7 @@ #define _UAPI_LINUX_IN_H #include <linux/types.h> +#include <linux/stddef.h> #include <linux/libc-compat.h> #include <linux/socket.h> -- 2.39.0.246.g2a6d74b583-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 1/2] kvm: sev: Add SEV-SNP guest request throttling 2023-01-19 21:34 ` [PATCH v3 1/2] kvm: sev: Add SEV-SNP " Dionna Glaze @ 2023-01-20 16:40 ` Sean Christopherson 0 siblings, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2023-01-20 16:40 UTC (permalink / raw) To: Dionna Glaze Cc: kvm, linux-kernel, x86, Thomas Lendacky, Paolo Bonzini, Joerg Roedel, Peter Gonda, Borislav Petkov AMD folks, unless y'all object to the concept itself, can this be tacked onto the SNP series? Responsibility for responding to feedback and making changes can still be punted to Dionna (or whoever), I just get briefly confused every time this series is posted because I think it's addressing a problem that needs attention _now_. On Thu, Jan 19, 2023, Dionna Glaze wrote: > The AMD-SP is a precious resource that doesn't have a scheduler other > than a mutex lock queue. To avoid customers from causing a DoS, a > module_param-set rate limit is added with a default of 2 requests > per 2 seconds. > > These defaults were chosen empirically with a the assumption that > current server-grade SEV-SNP machines will rarely exceed 128 VMs under > usual circumstance. > > The throttling code is 2 << 32 given that invalid length is 1 and 2 is > the next available code. This was suggested by Tom Lendacky, and will > be included in a new revision of the GHCB specification. Why does throttling just punt back to the guest? E.g. why not exit to userspace and let userspace stall the vCPU? Is the guest expected to schedule out the task that's trying to make the request? > @@ -158,6 +158,7 @@ struct snp_psc_desc { > > /* Guest message request error code */ > #define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) > +#define SNP_GUEST_REQ_THROTTLED (((u64)2) << 32) Someone please add macros to define the shift and generate error codes, the above is way too hard to read for such a simple concept. E.g. I want to see something like #define SNP_GUEST_REQ_INVALID_LEN SNP_GUEST_REQ_ERROR_CODE(1) > #define GHCB_MSR_TERM_REQ 0x100 > #define GHCB_MSR_TERM_REASON_SET_POS 12 > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index d0e58cffd1ed..cd9372ce6fc2 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -58,6 +58,14 @@ module_param_named(sev_es, sev_es_enabled, bool, 0444); > /* enable/disable SEV-SNP support */ > static bool sev_snp_enabled = true; > module_param_named(sev_snp, sev_snp_enabled, bool, 0444); > + > +/* Throttle guest requests to a burst # per this many seconds */ > +unsigned int guest_request_throttle_s = 2; "seconds" seems like to coarse of a granularity, e.g. if userspace wants to allow one request every half-second. Why not go with milliseconds? As a bonus, the (IMO) odd "s" gets replaced with the more intuitive "ms". That said, I wonder if this should be a per-VM capability, not a module param. Since the throttling is per VM and not per user, making it a module param doesn't prevent a malicious user or even a compromised VMM from spamming the PSP, e.g. just spin up a big pile o' VMs. > +module_param(guest_request_throttle_s, int, 0444); > + > +/* Throttle guest requests to this many per the above many seconds */ > +unsigned int guest_request_throttle_burst = 2; > +module_param(guest_request_throttle_burst, int, 0444); > #else > #define sev_enabled false > #define sev_es_enabled false > @@ -333,6 +341,9 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp) > goto e_free; > > mutex_init(&sev->guest_req_lock); > + ratelimit_state_init(&sev->snp_guest_msg_rs, > + guest_request_throttle_s * HZ, > + guest_request_throttle_burst); > ret = sev_snp_init(&argp->error, false); > } else { > ret = sev_platform_init(&argp->error); > @@ -3595,6 +3606,14 @@ static void snp_cleanup_guest_buf(struct sev_data_snp_guest_request *data, unsig > *rc = SEV_RET_INVALID_ADDRESS; > } > > +static bool snp_throttle_guest_request(struct kvm_sev_info *sev) { Curly brace goes on its own line for functions. > + if (__ratelimit(&sev->snp_guest_msg_rs)) > + return false; > + > + pr_info_ratelimited("svm: too many guest message requests\n"); Drop the printk, it doesn't help understand _which_ guest is being throttled. If userspace really wants to get notified, then KVM should exit to userspace instead of throttling manually. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest 2023-01-19 21:34 [PATCH v3 0/2] kvm: sev: Add SNP guest request throttling Dionna Glaze 2023-01-19 21:34 ` [PATCH v3 1/2] kvm: sev: Add SEV-SNP " Dionna Glaze @ 2023-01-19 21:34 ` Dionna Glaze 2023-01-20 17:15 ` Tom Lendacky 1 sibling, 1 reply; 5+ messages in thread From: Dionna Glaze @ 2023-01-19 21:34 UTC (permalink / raw) To: kvm, linux-kernel, x86 Cc: Dionna Glaze, Thomas Lendacky, Paolo Bonzini, Joerg Roedel, Peter Gonda, Borislav Petkov The ccp driver can be overloaded even with 1 HZ throttling. The return value of -EBUSY means that there is no firmware error to report back to user space, so the guest VM would see this as exitinfo2 = 0. The false success can trick the guest to update its the message sequence number when it shouldn't have. Instead, when ccp returns -EBUSY, that is reported to userspace as the throttling return value. Cc: Thomas Lendacky <Thomas.Lendacky@amd.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Joerg Roedel <jroedel@suse.de> Cc: Peter Gonda <pgonda@google.com> Cc: Borislav Petkov <bp@alien8.de> Signed-off-by: Dionna Glaze <dionnaglaze@google.com> --- arch/x86/kvm/svm/sev.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index cd9372ce6fc2..7da1cc300d7b 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3642,7 +3642,14 @@ static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t goto unlock; rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); - if (rc) + + /* + * The ccp driver can return -EBUSY if the PSP is overloaded, so signal + * the request has been throttled. + */ + if (rc == -EBUSY) + rc = SNP_GUEST_REQ_THROTTLED; + else if (rc) /* use the firmware error code */ rc = err; @@ -3713,7 +3720,14 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp if (sev->snp_certs_len) data_npages = sev->snp_certs_len >> PAGE_SHIFT; - if (rc) { + /* + * The ccp driver can return -EBUSY if the PSP is overloaded, so signal + * the request has been throttled. + */ + if (rc == -EBUSY) { + rc = SNP_GUEST_REQ_THROTTLED; + goto cleanup; + } else if (rc) { /* * If buffer length is small then return the expected * length in rbx. -- 2.39.0.246.g2a6d74b583-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest 2023-01-19 21:34 ` [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest Dionna Glaze @ 2023-01-20 17:15 ` Tom Lendacky 0 siblings, 0 replies; 5+ messages in thread From: Tom Lendacky @ 2023-01-20 17:15 UTC (permalink / raw) To: Dionna Glaze, kvm, linux-kernel, x86, Ashish Kalra Cc: Paolo Bonzini, Joerg Roedel, Peter Gonda, Borislav Petkov On 1/19/23 15:34, Dionna Glaze wrote: Since you're building on SNP hypervisor patches, please keep @Ashish on direct copy. > The ccp driver can be overloaded even with 1 HZ throttling. The return > value of -EBUSY means that there is no firmware error to report back to > user space, so the guest VM would see this as exitinfo2 = 0. The false > success can trick the guest to update its the message sequence number > when it shouldn't have. > > Instead, when ccp returns -EBUSY, that is reported to userspace as the > throttling return value. Except the CCP driver doesn't return -EBUSY because it is overloaded. It will simply try to acquire the mutex and continue once it has it. There are a couple of places that return -EBUSY in the driver for other reasons, as well as other -E* values. It looks like these need to be handled properly by the SNP hypervisor patches so that a "success" isn't reported back. So this patch isn't necessary, but any -E* return value without having actually called the firmware needs to be handled properly. @Ashish, please work with Dionna on this. Thanks, Tom > > Cc: Thomas Lendacky <Thomas.Lendacky@amd.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Joerg Roedel <jroedel@suse.de> > Cc: Peter Gonda <pgonda@google.com> > Cc: Borislav Petkov <bp@alien8.de> > > Signed-off-by: Dionna Glaze <dionnaglaze@google.com> > --- > arch/x86/kvm/svm/sev.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index cd9372ce6fc2..7da1cc300d7b 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -3642,7 +3642,14 @@ static void snp_handle_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t > goto unlock; > > rc = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, &err); > - if (rc) > + > + /* > + * The ccp driver can return -EBUSY if the PSP is overloaded, so signal > + * the request has been throttled. > + */ > + if (rc == -EBUSY) > + rc = SNP_GUEST_REQ_THROTTLED; > + else if (rc) > /* use the firmware error code */ > rc = err; > > @@ -3713,7 +3720,14 @@ static void snp_handle_ext_guest_request(struct vcpu_svm *svm, gpa_t req_gpa, gp > if (sev->snp_certs_len) > data_npages = sev->snp_certs_len >> PAGE_SHIFT; > > - if (rc) { > + /* > + * The ccp driver can return -EBUSY if the PSP is overloaded, so signal > + * the request has been throttled. > + */ > + if (rc == -EBUSY) { > + rc = SNP_GUEST_REQ_THROTTLED; > + goto cleanup; > + } else if (rc) { > /* > * If buffer length is small then return the expected > * length in rbx. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-01-20 17:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-19 21:34 [PATCH v3 0/2] kvm: sev: Add SNP guest request throttling Dionna Glaze 2023-01-19 21:34 ` [PATCH v3 1/2] kvm: sev: Add SEV-SNP " Dionna Glaze 2023-01-20 16:40 ` Sean Christopherson 2023-01-19 21:34 ` [PATCH v3 2/2] kvm: sev: If ccp is busy, report throttled to guest Dionna Glaze 2023-01-20 17:15 ` Tom Lendacky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).