Messages in this thread | | | Subject | Re: [PATCH] x86/paravirt: Add missing noinstr to arch_local*() helpers | From | Jürgen Groß <> | Date | Tue, 11 Aug 2020 09:04:48 +0200 |
| |
On 11.08.20 09:00, Marco Elver wrote: > On Fri, 7 Aug 2020 at 17:19, Marco Elver <elver@google.com> wrote: >> On Fri, Aug 07, 2020 at 02:08PM +0200, Marco Elver wrote: >>> On Fri, 7 Aug 2020 at 14:04, Jürgen Groß <jgross@suse.com> wrote: >>>> >>>> On 07.08.20 13:38, Marco Elver wrote: >>>>> On Fri, Aug 07, 2020 at 12:35PM +0200, Jürgen Groß wrote: > ... >>>>>> I think CONFIG_PARAVIRT_XXL shouldn't matter, but I'm not completely >>>>>> sure about that. CONFIG_PARAVIRT_SPINLOCKS would be my primary suspect. >>>>> >>>>> Yes, PARAVIRT_XXL doesn't make a different. When disabling >>>>> PARAVIRT_SPINLOCKS, however, the warnings go away. >>>> >>>> Thanks for testing! >>>> >>>> I take it you are doing the tests in a KVM guest? >>> >>> Yes, correct. >>> >>>> If so I have a gut feeling that the use of local_irq_save() and >>>> local_irq_restore() in kvm_wait() might be fishy. I might be completely >>>> wrong here, though. >>> >>> Happy to help debug more, although I might need patches or pointers >>> what to play with. >>> >>>> BTW, I think Xen's variant of pv spinlocks is fine (no playing with IRQ >>>> on/off). >>>> >>>> Hyper-V seems to do the same as KVM, and kicking another vcpu could be >>>> problematic as well, as it is just using IPI. >> >> I experimented a bit more, and the below patch seems to solve the >> warnings. However, that was based on your pointer about kvm_wait(), and >> I can't quite tell if it is the right solution. >> >> My hypothesis here is simply that kvm_wait() may be called in a place >> where we get the same case I mentioned to Peter, >> >> raw_local_irq_save(); /* or other IRQs off without tracing */ >> ... >> kvm_wait() /* IRQ state tracing gets confused */ >> ... >> raw_local_irq_restore(); >> >> and therefore, using raw variants in kvm_wait() works. It's also safe >> because it doesn't call any other libraries that would result in corrupt >> IRQ state AFAIK. > > Just to follow-up, it'd still be nice to fix this. Suggestions? > > I could send the below as a patch, but can only go off my above > hypothesis and the fact that syzbot is happier, so not entirely > convincing.
Peter has told me via IRC he will look soon further into this.
Your finding suggests that the pv-lock implementation for Hyper-V needs some tweaking, too. For that purpose I'm adding Wei to Cc.
Juergen
> > Thanks, > -- Marco > >> ------ >8 ------ >> >> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c >> index 233c77d056c9..1d412d1466f0 100644 >> --- a/arch/x86/kernel/kvm.c >> +++ b/arch/x86/kernel/kvm.c >> @@ -797,7 +797,7 @@ static void kvm_wait(u8 *ptr, u8 val) >> if (in_nmi()) >> return; >> >> - local_irq_save(flags); >> + raw_local_irq_save(flags); >> >> if (READ_ONCE(*ptr) != val) >> goto out; >> @@ -810,10 +810,10 @@ static void kvm_wait(u8 *ptr, u8 val) >> if (arch_irqs_disabled_flags(flags)) >> halt(); >> else >> - safe_halt(); >> + raw_safe_halt(); >> >> out: >> - local_irq_restore(flags); >> + raw_local_irq_restore(flags); >> } >> >> #ifdef CONFIG_X86_32 >
| |