| Message ID | 1631894159-10146-1-git-send-email-ajaygargnsit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: (64-bit x86_64 machine) : fix -frame-larger-than warnings/errors. | expand |
On Fri, Sep 17, 2021, Ajay Garg wrote: > From: ajay <ajaygargnsit@gmail.com> > > Issue : > ======= > > In "kvm_hv_flush_tlb" and "kvm_hv_send_ipi" methods, defining > "u64 sparse_banks[64]" inside the methods (on the stack), causes the > stack-segment-memory-allocation to go beyond 1024 bytes, thus raising the > warning/error which breaks the build. > > Fix : > ===== > > Instead of defining "u64 sparse_banks [64]" inside the methods, we instead > define this array in the (only) client method "kvm_hv_hypercall", and then > pass the array (and its size) as additional arguments to the two methods. > Doing this, we do not exceed the 1024 bytes stack-segment-memory-allocation, > on any stack-segment of any method. This is a hack, and it's not guaranteed to work, e.g. if the compiler decided to inline the helpers, then presumably this problem would rear its head again. However, I don't think this is a problem any more. gcc-10 and clang-11 are both comfortably under 1024, even if I force both helpers to be inlined. Neither function has variables that would scale with NR_CPUS (and I verified high number of NR_CPUS for giggles). Can you try reproducing the behavior on the latest kvm/queue? I swear I've seen this in the past, but I couldn't find a commit that "fixed" any such warning. If it does repro, can you provide your .config and compiler version? Maybe your compiler is doing somethign funky? > Signed-off-by: ajay <ajaygargnsit@gmail.com> The SoB needs your full name. > --- > arch/x86/kvm/hyperv.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index 232a86a6faaf..5340be93daa4 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1750,7 +1750,8 @@ struct kvm_hv_hcall { > sse128_t xmm[HV_HYPERCALL_MAX_XMM_REGISTERS]; > }; > > -static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool ex) > +static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, > + bool ex, u64 *sparse_banks, u32 num_sparse_banks) > { > int i; > gpa_t gpa; > @@ -1762,10 +1763,11 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool > DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); > unsigned long *vcpu_mask; > u64 valid_bank_mask; > - u64 sparse_banks[64]; > int sparse_banks_len; > bool all_cpus; > > + memset(sparse_banks, 0, sizeof(u64) * num_sparse_banks); > + FWIW, the array size needs to be validated, there is other code in this function that assumes it's at least 64 entries.
Hi Sean, Thanks for your time. I have cloned the kernel many times since the time the patch was posted, and have not seen the issue again. One thing I distinctly remember that when the build was breaking, it was with staging-drivers disabled. Since then, I have disabled the staging-drivers. Today, I again enabled staging-drivers, and build went fine. So, I am ok with archiving this patch. We can revisit if someone else reports this/similar issue. Thanks and Regards, Ajay On Thu, Oct 14, 2021 at 12:30 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Sep 17, 2021, Ajay Garg wrote: > > From: ajay <ajaygargnsit@gmail.com> > > > > Issue : > > ======= > > > > In "kvm_hv_flush_tlb" and "kvm_hv_send_ipi" methods, defining > > "u64 sparse_banks[64]" inside the methods (on the stack), causes the > > stack-segment-memory-allocation to go beyond 1024 bytes, thus raising the > > warning/error which breaks the build. > > > > Fix : > > ===== > > > > Instead of defining "u64 sparse_banks [64]" inside the methods, we instead > > define this array in the (only) client method "kvm_hv_hypercall", and then > > pass the array (and its size) as additional arguments to the two methods. > > > Doing this, we do not exceed the 1024 bytes stack-segment-memory-allocation, > > on any stack-segment of any method. > > This is a hack, and it's not guaranteed to work, e.g. if the compiler decided to > inline the helpers, then presumably this problem would rear its head again. > > However, I don't think this is a problem any more. gcc-10 and clang-11 are both > comfortably under 1024, even if I force both helpers to be inlined. Neither > function has variables that would scale with NR_CPUS (and I verified high number > of NR_CPUS for giggles). Can you try reproducing the behavior on the latest > kvm/queue? I swear I've seen this in the past, but I couldn't find a commit that > "fixed" any such warning. > > If it does repro, can you provide your .config and compiler version? Maybe your > compiler is doing somethign funky? > > > Signed-off-by: ajay <ajaygargnsit@gmail.com> > > The SoB needs your full name. > > > --- > > arch/x86/kvm/hyperv.c | 34 ++++++++++++++++++++++++---------- > > 1 file changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > > index 232a86a6faaf..5340be93daa4 100644 > > --- a/arch/x86/kvm/hyperv.c > > +++ b/arch/x86/kvm/hyperv.c > > @@ -1750,7 +1750,8 @@ struct kvm_hv_hcall { > > sse128_t xmm[HV_HYPERCALL_MAX_XMM_REGISTERS]; > > }; > > > > -static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool ex) > > +static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, > > + bool ex, u64 *sparse_banks, u32 num_sparse_banks) > > > > { > > int i; > > gpa_t gpa; > > @@ -1762,10 +1763,11 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool > > DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); > > unsigned long *vcpu_mask; > > u64 valid_bank_mask; > > - u64 sparse_banks[64]; > > int sparse_banks_len; > > bool all_cpus; > > > > + memset(sparse_banks, 0, sizeof(u64) * num_sparse_banks); > > + > > FWIW, the array size needs to be validated, there is other code in this function > that assumes it's at least 64 entries.
On Sun, Oct 24, 2021, Ajay Garg wrote: > Hi Sean. > > Today I enabled a debug-build, and the compilation broke again. > > 1. > Please find attached the .config file. Aha! The problem is CONFIG_KASAN_STACK=y, which is selected (and can't be unselected) by CONFIG_KASAN=y when compiling with gcc (clang/LLVM is a stack hog in some cases so it's opt-in for clang). KASAN_STACK adds a redzone around every stack variable, which pushes the Hyper-V functions over the limit. > Please let know if/when I should float a v2 patch. I still don't love hoisting sparse_banks up a level, that info really shouldn't bleed into the caller. My alternative solution is to push vp_bitmap down into sparse_set_to_vcpu_mask(). That doesn't "free" up as much stack, but it's enough to get under the 1024 byte default. It's also nice in that it hides the VP index mismatch logic in sparse_set_to_vcpu_mask(). I'll post a proper patch tomorrow (completely untested): --- arch/x86/kvm/hyperv.c | 55 ++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 4f15c0165c05..80018cfab5c7 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1710,31 +1710,36 @@ int kvm_hv_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host) return kvm_hv_get_msr(vcpu, msr, pdata, host); } -static __always_inline unsigned long *sparse_set_to_vcpu_mask( - struct kvm *kvm, u64 *sparse_banks, u64 valid_bank_mask, - u64 *vp_bitmap, unsigned long *vcpu_bitmap) +static void sparse_set_to_vcpu_mask(struct kvm *kvm, u64 *sparse_banks, + u64 valid_bank_mask, unsigned long *vcpu_mask) { struct kvm_hv *hv = to_kvm_hv(kvm); + bool has_mismatch = atomic_read(&hv->num_mismatched_vp_indexes); + u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS]; struct kvm_vcpu *vcpu; int i, bank, sbank = 0; + u64 *bitmap; - memset(vp_bitmap, 0, - KVM_HV_MAX_SPARSE_VCPU_SET_BITS * sizeof(*vp_bitmap)); + BUILD_BUG_ON(sizeof(vp_bitmap) > + sizeof(*vcpu_mask) * BITS_TO_LONGS(KVM_MAX_VCPUS)); + + /* If vp_index == vcpu_idx for all vCPUs, fill vcpu_mask directly. */ + if (likely(!has_mismatch)) + bitmap = (u64 *)vcpu_mask; + + memset(bitmap, 0, sizeof(vp_bitmap)); for_each_set_bit(bank, (unsigned long *)&valid_bank_mask, KVM_HV_MAX_SPARSE_VCPU_SET_BITS) - vp_bitmap[bank] = sparse_banks[sbank++]; + bitmap[bank] = sparse_banks[sbank++]; - if (likely(!atomic_read(&hv->num_mismatched_vp_indexes))) { - /* for all vcpus vp_index == vcpu_idx */ - return (unsigned long *)vp_bitmap; - } + if (likely(!has_mismatch)) + return; - bitmap_zero(vcpu_bitmap, KVM_MAX_VCPUS); + bitmap_zero(vcpu_mask, KVM_MAX_VCPUS); kvm_for_each_vcpu(i, vcpu, kvm) { if (test_bit(kvm_hv_get_vpindex(vcpu), (unsigned long *)vp_bitmap)) - __set_bit(i, vcpu_bitmap); + __set_bit(i, vcpu_mask); } - return vcpu_bitmap; } struct kvm_hv_hcall { @@ -1756,9 +1761,7 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool struct kvm *kvm = vcpu->kvm; struct hv_tlb_flush_ex flush_ex; struct hv_tlb_flush flush; - u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS]; - DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); - unsigned long *vcpu_mask; + DECLARE_BITMAP(vcpu_mask, KVM_MAX_VCPUS); u64 valid_bank_mask; u64 sparse_banks[64]; int sparse_banks_len; @@ -1842,11 +1845,9 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool if (all_cpus) { kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH_GUEST); } else { - vcpu_mask = sparse_set_to_vcpu_mask(kvm, sparse_banks, valid_bank_mask, - vp_bitmap, vcpu_bitmap); + sparse_set_to_vcpu_mask(kvm, sparse_banks, valid_bank_mask, vcpu_mask); - kvm_make_vcpus_request_mask(kvm, KVM_REQ_TLB_FLUSH_GUEST, - vcpu_mask); + kvm_make_vcpus_request_mask(kvm, KVM_REQ_TLB_FLUSH_GUEST, vcpu_mask); } ret_success: @@ -1879,9 +1880,7 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool struct kvm *kvm = vcpu->kvm; struct hv_send_ipi_ex send_ipi_ex; struct hv_send_ipi send_ipi; - u64 vp_bitmap[KVM_HV_MAX_SPARSE_VCPU_SET_BITS]; - DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); - unsigned long *vcpu_mask; + DECLARE_BITMAP(vcpu_mask, KVM_MAX_VCPUS); unsigned long valid_bank_mask; u64 sparse_banks[64]; int sparse_banks_len; @@ -1937,11 +1936,13 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool if ((vector < HV_IPI_LOW_VECTOR) || (vector > HV_IPI_HIGH_VECTOR)) return HV_STATUS_INVALID_HYPERCALL_INPUT; - vcpu_mask = all_cpus ? NULL : - sparse_set_to_vcpu_mask(kvm, sparse_banks, valid_bank_mask, - vp_bitmap, vcpu_bitmap); + if (all_cpus) { + kvm_send_ipi_to_many(kvm, vector, NULL); + } else { + sparse_set_to_vcpu_mask(kvm, sparse_banks, valid_bank_mask, vcpu_mask); - kvm_send_ipi_to_many(kvm, vector, vcpu_mask); + kvm_send_ipi_to_many(kvm, vector, vcpu_mask); + } ret_success: return HV_STATUS_SUCCESS; -- 2.33.0.1079.g6e70778dc9-goog
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 232a86a6faaf..5340be93daa4 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1750,7 +1750,8 @@ struct kvm_hv_hcall { sse128_t xmm[HV_HYPERCALL_MAX_XMM_REGISTERS]; }; -static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool ex) +static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, + bool ex, u64 *sparse_banks, u32 num_sparse_banks) { int i; gpa_t gpa; @@ -1762,10 +1763,11 @@ static u64 kvm_hv_flush_tlb(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); unsigned long *vcpu_mask; u64 valid_bank_mask; - u64 sparse_banks[64]; int sparse_banks_len; bool all_cpus; + memset(sparse_banks, 0, sizeof(u64) * num_sparse_banks); + if (!ex) { if (hc->fast) { flush.address_space = hc->ingpa; @@ -1875,7 +1877,8 @@ static void kvm_send_ipi_to_many(struct kvm *kvm, u32 vector, } } -static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool ex) +static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, + bool ex, u64 *sparse_banks, u32 num_sparse_banks) { struct kvm *kvm = vcpu->kvm; struct hv_send_ipi_ex send_ipi_ex; @@ -1884,11 +1887,12 @@ static u64 kvm_hv_send_ipi(struct kvm_vcpu *vcpu, struct kvm_hv_hcall *hc, bool DECLARE_BITMAP(vcpu_bitmap, KVM_MAX_VCPUS); unsigned long *vcpu_mask; unsigned long valid_bank_mask; - u64 sparse_banks[64]; int sparse_banks_len; u32 vector; bool all_cpus; + memset(sparse_banks, 0, sizeof(u64) * num_sparse_banks); + if (!ex) { if (!hc->fast) { if (unlikely(kvm_read_guest(kvm, hc->ingpa, &send_ipi, @@ -2162,6 +2166,10 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) struct kvm_hv_hcall hc; u64 ret = HV_STATUS_SUCCESS; +#define NUM_SPARSE_BANKS 64 + + u64 sparse_banks[NUM_SPARSE_BANKS]; + /* * hypercall generates UD from non zero cpl and real mode * per HYPER-V spec @@ -2248,42 +2256,48 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_flush_tlb(vcpu, &hc, false); + ret = kvm_hv_flush_tlb(vcpu, &hc, false, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE: if (unlikely(hc.rep)) { ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_flush_tlb(vcpu, &hc, false); + ret = kvm_hv_flush_tlb(vcpu, &hc, false, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_FLUSH_VIRTUAL_ADDRESS_LIST_EX: if (unlikely(!hc.rep_cnt || hc.rep_idx)) { ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_flush_tlb(vcpu, &hc, true); + ret = kvm_hv_flush_tlb(vcpu, &hc, true, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE_EX: if (unlikely(hc.rep)) { ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_flush_tlb(vcpu, &hc, true); + ret = kvm_hv_flush_tlb(vcpu, &hc, true, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_SEND_IPI: if (unlikely(hc.rep)) { ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_send_ipi(vcpu, &hc, false); + ret = kvm_hv_send_ipi(vcpu, &hc, false, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_SEND_IPI_EX: if (unlikely(hc.fast || hc.rep)) { ret = HV_STATUS_INVALID_HYPERCALL_INPUT; break; } - ret = kvm_hv_send_ipi(vcpu, &hc, true); + ret = kvm_hv_send_ipi(vcpu, &hc, true, + sparse_banks, NUM_SPARSE_BANKS); break; case HVCALL_POST_DEBUG_DATA: case HVCALL_RETRIEVE_DEBUG_DATA: