Re: [GIT PULL] KVM/arm updates for 4.18-rc2
2018-06-22 13:49+0100, Marc Zyngier: > Radim, Paolo, > > This is the first batch of fixes for 4.18, mostly dealing with the > fallout from Dave's lazy FPSIMD handling. We also have the disabling > of the compat interface on arm64 (it never had it the first place), > and a relaxation on the alignment of the GICv3 compatibility > interface. > > Please pull. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] KVM: Enforce error in ioctl for compat tasks when !KVM_COMPAT
2018-06-21 12:40+0100, Marc Zyngier: > Radim, Paolo: are you happy with me taking this through the kvmarm tree? Yes, please do. Acked-by: Radim Krčmář ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 0/5] KVM/arm fixes for 4.17-rc2
2018-04-20 17:07+0100, Marc Zyngier: > Radim, Paolo, > > Here is the first batch of KVM/arm fixes post 4.17 merge window. Not a > lot to report, apart from the slightly scary VMID allocation race that > has been sitting there from day 1. > > Please pull. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [GIT PULL] KVM/ARM updates for v4.17
2018-03-28 13:51+0100, Marc Zyngier: > Paolo, Radim, > > This is the (rather big) set of updates for KVM/ARM for v4.17. The > main features are the set of VHE optimizations taking advantage of > CPUs implementing ARMv8.1, together with the EL2 randomization patches > that are the foundation for mitigating the so-called variant 3a > security issue (affecting Cortex-A57 and A72). > > The rest is the usual mix of vgic fixes and minor improvements. > > Note that the breakup below is slightly misleading, as it includes > fixes that have already landed in mainline (I've done a direct merge > of the fixes branch in order to spare everyone some horrible > conflicts). > > Also, we've had to revert a pretty important patch for Qualcomm > servers due to some more conflicts with the arm64 tree). That patch > will be resent once both trees have been pulled into Linus' (the > sooner, the better). KVM tree will likely be merged in the latter half of next week. > Please pull. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 00/28] KVM/ARM Changes for v4.16
2018-01-31 10:34+0100, Christoffer Dall: > Hi Paolo and Radim, > > Apologies for getting this pull request to you rather late; there were a > number of issues that I wanted to make sure we could fix properly before > including some of the changes in this pull request. > > The changes for this version include icache invalidation optimizations > (improving VM startup time), support for forwarded level-triggered > interrupts (improved performance for timers and passthrough platform > devices), a small fix for power-management notifiers, and some cosmetic > changes. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 00/19] KVM/ARM Fixes for v4.15
2017-12-04 14:56+0100, Christoffer Dall: > From: Christoffer Dall > > Hi Paolo and Radim, > > Here's the first round of fixes for KVM/ARM for v4.15. This is a fairly large > set of fixes, partially because we spotted a handful of issues from running > the > SMATCH static analysis on the code (thanks to AKASHI Takahiro). > > In more details, this pull request fixes: > - A number of issues in the vgic discovered using SMATCH > - A bit one-off calculation in out stage base address mask (32-bit and >64-bit) > - Fixes to single-step debugging instructions that trap for other >reasons such as MMMIO aborts > - Printing unavailable hyp mode as error > - Potential spinlock deadlock in the vgic > - Avoid calling vgic vcpu free more than once > - Broken bit calculation for big endian systems Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 00/26] KVM/ARM Changes for v4.15
2017-11-07 11:47+0100, Christoffer Dall: > Hi Paolo and Radim, > > Here is the first round of KVM/ARM Changes for v4.15. I will follow up > with a second pull request based on tip:irq/core containing the KVM/ARM > side of the GICv4 support. > > Changes in this pull requestinclude: > - Optimized arch timer handling for KVM/ARM > - Improvements to the VGIC ITS code and introduction of an ITS reset >ioctl > - Unification of the 32-bit fault injection logic > - More exact external abort matching logic Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 05/37] KVM: Record the executing ioctl number on the vcpu struct
2017-10-13 19:31+0200, Christoffer Dall: > On Fri, Oct 13, 2017 at 07:13:07PM +0200, Radim Krčmář wrote: > > I think that other (special) callsites of vcpu_load()/vcpu_put() have a > > well defined IOCTL that can be used instead of vcpu->ioctl, so we could > > just pass the ioctl value all the way to arch code and never save it > > anywhere, > > I don't think that works; what would you do with preempt notifier calls? Right, BUG :), I didn't consider them before and they need to know. > One solution is to add a parameter to vcpu_put, lie for vcpu_load, which > also sets the ioctl, and other callers than the final vcpu_put in > kvm_vcpu_ioctl() just pass the existing value, where the kvm_vcpu_ioctl > call can pass 0 which gets set before releasing the mutex. > > Can you think of a more elegant solution? Not really, only thought of touching preempt notifiers and it seems to be more complicated. I think we shouldn't restore ioctl on vcpu_put() at all -- the value isn't well defined outside of the mutex, so there is no point in looking and we can just zero the ioctl. Actually, I wouldn't rely on the existing value at all because that. The need for load/put depends on the current code path, not on the one we race with. x86 seems to be the only user of vcpu_load() outside of kvm_vcpu_ioctl() and the callers are either under a VM ioctl or under a VM destruction paths (invalid IOCTL) and we can just hardcode that. Passing 0 to all other vcpu_load()s and unconditionally zeroing ioctl before mutex_unlock() should work. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 05/37] KVM: Record the executing ioctl number on the vcpu struct
2017-10-12 12:41+0200, Christoffer Dall:
> Some architectures may decide to do different things during
> kvm_arch_vcpu_load depending on the ioctl being executed. For example,
> arm64 is about to do significant work in vcpu load/put when running a
> vcpu, but not when doing things like KVM_SET_ONE_REG or
> KVM_SET_MP_STATE.
>
> Therefore, store the ioctl number that we are executing on the VCPU
> during the first vcpu_load() which succeeds in getting the vcpu->mutex
> and set the ioctl number to 0 when exiting kvm_vcpu_ioctl() after
> successfully loading the vcpu.
>
> Cc: Paolo Bonzini
> Cc: Radim Krčmář
> Signed-off-by: Christoffer Dall
> ---
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> @@ -147,12 +147,13 @@ bool kvm_is_reserved_pfn(kvm_pfn_t pfn)
> /*
> * Switches to specified vcpu, until a matching vcpu_put()
> */
> -int vcpu_load(struct kvm_vcpu *vcpu)
> +int vcpu_load(struct kvm_vcpu *vcpu, unsigned int ioctl)
> {
> int cpu;
>
> if (mutex_lock_killable(&vcpu->mutex))
> return -EINTR;
> + vcpu->ioctl = ioctl;
This seems to prevent races by protecting the store by a mutex, but
> cpu = get_cpu();
> preempt_notifier_register(&vcpu->preempt_notifier);
> kvm_arch_vcpu_load(vcpu, cpu);
> @@ -2529,7 +2530,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
> #endif
>
>
> - r = vcpu_load(vcpu);
> + r = vcpu_load(vcpu, ioctl);
> if (r)
> return r;
> switch (ioctl) {
> @@ -2704,6 +2705,7 @@ static long kvm_vcpu_ioctl(struct file *filp,
> }
> out:
> vcpu_put(vcpu);
> + vcpu->ioctl = 0;
we should still have a race as we clear ioctl only after releasing the
lock. For example malicious userspace could break KVM terms of use and
issue !KVM_RUN followed by KVM_RUN, so we would have these races:
!KVM_RUN | KVM_RUN
mutex_lock_killable(&vcpu->mutex); |
vcpu->ioctl = !KVM_RUN; |
... | mutex_lock_killable(&vcpu->mutex);
mutex_unlock(&vcpu->mutex); |
| vcpu->ioctl = KVM_RUN;
| kvm_arch_vcpu_load() // variant 1
vcpu->ioctl = 0; | ...
| kvm_arch_vcpu_load() // variant 2
| vcpu_put()
where the observed value of vcpu->ioctl in vcpu_put() would not
correctly pair with vcpu_load() or worse, kvm_arch_arch_load() in
KVM_RUN would execute with vcpu->ioctl = 0.
I think that other (special) callsites of vcpu_load()/vcpu_put() have a
well defined IOCTL that can be used instead of vcpu->ioctl, so we could
just pass the ioctl value all the way to arch code and never save it
anywhere,
thanks.
> kfree(fpu);
> kfree(kvm_sregs);
> return r;
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 0/4] KVM/ARM fixes for 4.13-rc4
2017-08-03 16:28+0100, Marc Zyngier: > Paolo, Radim, > > Here's a pull request for a few KVM/ARM updates. Nothing major except > for a VM destruction race (Suzuki has a knack for finding those...). > > Please pull. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvm-unit-tests] arm64: timer: Use correct counter for !pending_before
2017-08-03 08:25+0200, Christoffer Dall: > We were using the virtual counter to calculate a timer cval which is 10 > seconds in the future, but this obviously doesn't work for the physical > timer which is bases on the physical counter. > > Make sure we use a properly paired timer/counter pair. > > Reported-by: Andrew Jones > Signed-off-by: Christoffer Dall > --- Applied, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 0/3] KVM/ARM Fixes for v4.12-rc5
2017-06-06 22:21+0200, Christoffer Dall: > On Tue, Jun 06, 2017 at 03:56:26PM +0200, Christoffer Dall wrote: > > Hi Radim and Paolo, > > > > Three additional fixes for KVM/ARM for v4.12. > > We didn't expect three additional fixes to pop up so quickly, so I'm > going to send another pull request tomorrow. > > Please hold off on applying this one if you haven't done so already. > > Sorry about that. No problem, I only had it applied locally. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 00/13] KVM/ARM Fixes for v4.12-rc2
2017-05-18 11:47+0200, Christoffer Dall: > Hi Radim and Paolo, > > Here is a fairly large set of fixes post -rc1 for KVM/ARM. > > The fixes include: > - A fix for a build failure introduced in -rc1 when tracepoints are >enabled on 32-bit ARM. > - Disabling use of stack pointer protection in the hyp code which can >cause panics. > - A handful of VGIC fixes. > - A fix to the init of the redistributors on GICv3 systems that >prevented boot with kvmtool on GICv3 systems introduced in -rc1. > - A number of race conditions fixed in our MMU handling code. > - A fix for the guest being able to program the debug extensions for >the host on the 32-bit side. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] kvm: Fix mmu_notifier release race
2017-04-24 11:10+0100, Suzuki K Poulose: > The KVM uses mmu_notifier (wherever available) to keep track > of the changes to the mm of the guest. The guest shadow page > tables are released when the VM exits via mmu_notifier->ops.release(). > There is a rare chance that the mmu_notifier->release could be > called more than once via two different paths, which could end > up in use-after-free of kvm instance (such as [0]). > > e.g: > > thread Athread B > --- -- > > get_signal-> kvm_destroy_vm()-> > do_exit->mmu_notifier_unregister-> > exit_mm-> > kvm_arch_flush_shadow_all()-> > exit_mmap-> spin_lock(&kvm->mmu_lock) > mmu_notifier_release-> > kvm_arch_flush_shadow_all()-> . > ... spin_lock(&kvm->mmu_lock) . > spin_unlock(&kvm->mmu_lock) > kvm_arch_free_kvm() >*** use after free of kvm *** I don't understand this race ... a piece of code in mmu_notifier_unregister() says: /* * Wait for any running method to finish, of course including * ->release if it was run by mmu_notifier_release instead of us. */ synchronize_srcu(&srcu); and code before that removes the notifier from the list, so it cannot be called after we pass this point. mmu_notifier_release() does roughly the same and explains it as: /* * synchronize_srcu here prevents mmu_notifier_release from returning to * exit_mmap (which would proceed with freeing all pages in the mm) * until the ->release method returns, if it was invoked by * mmu_notifier_unregister. * * The mmu_notifier_mm can't go away from under us because one mm_count * is held by exit_mmap. */ synchronize_srcu(&srcu); The call of mmu_notifier->release is protected by srcu in both cases and while it seems possible that mmu_notifier->release would be called twice, I don't see a combination that could result in use-after-free from mmu_notifier_release after mmu_notifier_unregister() has returned. Doesn't [2/2] solve the exact same issue (that the release method cannot be called twice in parallel)? Thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-08 15:32-0400, Paolo Bonzini: > > > Makes sense. My pitch at the documentation after dropping READ_ONCE(): > > > > I'm confused again, I thought you wanted to keep READ_ONCE(). > > > > > > > > /* > > >* The return value of kvm_request_pending() is implicitly volatile > > > > why is that, actually? "that" is the return value? kvm_request_pending() is 'inline static' so the compiler can prove that the function only returns vcpu->requests and that the surrounding code doesn't change it, so various optimizations are possible. Or the implicitly volatile bit? We know that vcpu->requests can change at any time without action of the execution thread, which makes it volatile, but we don't tell that to the compiler, hence implicit. READ_ONCE(vcpu->requests) is *(volatile unsigned long *)&vcpu->requests and makes it explicitly volatile. > > >* and must be protected from reordering by the caller. > > >*/ > > > > Can we be specific about what that means? (e.g. must be preceded by a > > full smp_mb() - or whatever the case is). > > You can play devil's advocate both ways and argue that READ_ONCE is > better, or that it is unnecessary hence worse. You can write good > comments in either case. That's how I read Radim's message. But > I think we all agree on keeping it in the end. Exactly, I am in favor of READ_ONCE() as I don't like to keep the compiler informed, just wanted to cover all options, because they are not that different ... minute details are the hardest to decide. :) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-06 16:25+0200, Christoffer Dall:
> On Wed, Apr 05, 2017 at 10:20:17PM +0200, Radim Krčmář wrote:
>> 2017-04-05 19:39+0200, Christoffer Dall:
>> > On Wed, Apr 05, 2017 at 03:10:50PM +0200, Radim Krčmář wrote:
>> x86 uses KVM_REQ_MCLOCK_INPROGRESS for synchronization between cores and
>> the use in this series looked very similar.
>>
>> >> * memory barriers are necessary for correct behavior, see
>> >> * Documentation/virtual/kvm/vcpu-requests.rst.
>> >> *
>> >> * READ_ONCE() is not necessary for correctness, but simplifies
>> >> * reasoning by constricting the generated code.
>> >> */
>> >>
>> >> I considered READ_ONCE() to be self-documenting. :)
>> >
>> > I realize that I'm probably unusually slow in this whole area, but using
>> > READ_ONCE() where unnecessary doesn't help my reasoning, but makes me
>> > wonder which part of this I didn't understand, so I don't seem to agree
>> > with the statement that it simplifies reasoning.
>>
>> No, I think it is a matter of approach. When I see a READ_ONCE()
>> without a comment, I think that the programmer was aware that this
>> memory can change at any time and was defensive about it.
>
> I think it means that you have to read it exactly once at the exact flow
> in the code where it's placed.
The compiler can still reorder surrounding non-volatile code, but
reading exactly once is the subset of meaning that READ_ONCE() should
have. Not assigning it any more meaning sounds good.
>> I consider this use to simplify future development:
>> We think now that READ_ONCE() is not needed, but vcpu->requests is still
>> volatile and future changes in code might make READ_ONCE() necessary.
>> Preemptively putting READ_ONCE() there saves us thinking or hard-to-find
>> bugs.
>>
>
> I'm always a bit sceptical about such reasoning as I think without a
> complete understanding of what needs to change when doing changes, we're
> likely to get it wrong anyway.
I think we cannot achieve and maintain a complete understanding, so
getting things wrong is just a matter of time.
It is almost impossible to break ordering of vcpu->requests, though.
>> > To me, READ_ONCE() indicates that there's some flow in the code where
>> > it's essential that the compiler doesn't generate multiple loads, but
>> > that we only see a momentary single-read snapshot of the value, and this
>> > doesn't seem to be the case.
>>
>> The compiler can also squash multiple reads together, which is more
>> dangerous in this case as we would not notice a new requests. Avoiding
>> READ_ONCE() requires a better knowledge of the compiler algorithms that
>> prove which variable can be optimized.
>
> Isn't that covered by the memory barriers that imply compiler barriers
> that we (will) have between checking the mode and the requests variable?
It is, asm volatile ("" ::: "memory") is enough.
The minimal conditions that would require explicit barrier:
1) not having vcpu->mode(), because it cannot work without memory
barriers
2) the instruction that disables interrupts doesn't have "memory"
constraint (the smp_rmb in between is not necessary here)
And of course, there would have to be no functions that would contain a
compiler barrier or their bodies remained unknown in between disabling
interrupts and checking requests ...
>> The difference is really minor and I agree that the comment is bad.
>> The only comment I'm happy with is nothing, though ... even "READ_ONCE()
>> is not necessary" is wrong as that might change without us noticing.
>
> "READ_ONCE() is not necessary" while actually using READ_ONCE() is a
> terrible comment because it makes readers just doubt the correctness of
> the code.
>
> Regardless of whether or not we end up using READ_ONCE(), I think we
> should document exactly what the requirements are for accessing this
> variable at this time, i.e. any assumption about preceding barriers or
> other flows of events that we rely on.
Makes sense. My pitch at the documentation after dropping READ_ONCE():
/*
* The return value of kvm_request_pending() is implicitly volatile
* and must be protected from reordering by the caller.
*/
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/9] KVM: Add documentation for VCPU requests
2017-04-06 12:18+0200, Christian Borntraeger: > On 03/31/2017 06:06 PM, Andrew Jones wrote: >> Signed-off-by: Andrew Jones >> +VCPU Requests and Guest Mode >> + > > FWIW, s390 does not implement the guest mode. Maybe add some words that not > all architectures implement that? Or do we expect Radims rework soon? Yes, mention that it is an optional optimization sounds good. I won't be adding guest->mode to s390 (at least in the beginning). This means that kvm_arch_vcpu_should_kick() should return true. In kvm_make_all_cpus_request(), there is kvm_vcpu_exiting_guest_mode(vcpu) != OUTSIDE_GUEST_MODE that ought to be replaced with kvm_arch_vcpu_should_kick(), and it should return true if the arch doesn't implement guest->mode. But on s390, the condition currently always returns false ... is that IPI-less behavior of kvm_make_all_cpus_request intended on s390? Thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/9] KVM: Add documentation for VCPU requests
2017-04-05 20:29+0200, Paolo Bonzini:
> On 05/04/2017 19:45, Christoffer Dall wrote:
But the problem is that kvm_make_all_cpus_request() only sends IPIs to
CPUs where the mode was different from OUTSIDE_GUEST_MODE, so there it's
about !OUTSIDE_GUEST_MODE rather than !IN_GUEST_MODE, so there's some
subtlety here which I feel like it's dangerous to paper over.
>>> Right, that needs fixing in the code.
>> Really? I thought Paolo said that this is the intended behavior and
>> semantics; non-urgent requests that should just be serviced before the
>> next guest entry.
>
> Indeed, that's right...
>
>>> guest_mode is just an optimization that allows us to skip sending the
>>> IPI when the VCPU is known to handle the request as soon as possible.
>>>
>>> IN_GUEST_MODE: we must force VM exit or the request could never be
>>> handled
>>> EXITING_GUEST_MODE: another request already forces the VM exit and
>>> we're just waiting for the VCPU to notice our request
>>> OUTSIDE_GUEST_MODE: KVM is going to notice our request without any
>>> intervention
>>> READING_SHADOW_PAGE_TABLES: same as OUTSIDE_GUEST_MODE -- rename to
>>> unwieldly OUTSIDE_GUEST_MODE_READING_SHADOW_PAGE_TABLES?
>> Again, I thought Paolo was arguing that EXITING_GUEST_MODE makes the
>> whole thing work because you check that after checking requests?
>
> ... but apparently I was wrong here, see my email from this morning.
Ok, I'll prepare a patch that uses kvm_arch_vcpu_should_kick() in
kvm_make_all_cpus_request().
>>> The kick is needed only in IN_GUEST_MODE and a wake up is needed in case
>>> where the guest is halted OUTSIDE_GUEST_MODE ...
>>> Hm, maybe we should add a halt state too?
>> Wouldn't that be swait_active(&vcpu->wq) ? You could add a wrapper
>> though.
>
> Yes. I think the wrapper is probably unnecessary.
The state would only make sense if it were different -- there is a time
when the VCPU is halted, but swait_active(&vcpu->wq) returns false.
Probably doesn't have any good use now, though.
>> What I think you need is a way to distinguish the semantics of calling
>> kvm_make_all_cpus_request(), perhaps by adding a 'bool wake_up'
>> parameter.
>
> That would be fine.
I think the wakeup is more tied to the request type than to the place
calling it, so we could say which requests are "lazy" and don't do
wakeup and have kvm_make_all_cpus_request() without an extra argument.
In the end, I would like if kvm_make_all_cpus_request() was an optimized
variant of
kvm_for_each_vcpu(i, vcpu, kvm) {
kvm_make_request(vcpu, request);
kvm_vcpu_kick(vcpu);
}
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-05 19:39+0200, Christoffer Dall: > On Wed, Apr 05, 2017 at 03:10:50PM +0200, Radim Krčmář wrote: >> 2017-04-04 18:41+0200, Andrew Jones: >> > On Tue, Apr 04, 2017 at 05:30:14PM +0200, Christoffer Dall wrote: >> >> On Fri, Mar 31, 2017 at 06:06:50PM +0200, Andrew Jones wrote: >> >> > From: Radim Krčmář >> >> > >> >> > A first step in vcpu->requests encapsulation. >> >> >> >> Could we have a note here on why we need to access vcpu->requests using >> >> READ_ONCE now? >> > >> > Sure, maybe we should put the note as a comment above the read in >> > kvm_request_pending(). Something like >> > >> > /* >> > * vcpu->requests reads may appear in sequences that have strict >> > * data or control dependencies. Use READ_ONCE() to ensure the >> > * compiler does not do anything that breaks the required ordering. >> > */ >> > >> > Radim? >> >> Uses of vcpu->requests should already have barriers that take care of >> the ordering. I think the main reason for READ_ONCE() is to tell >> programmers that requests are special, but predictable. > > I don't know what to do with "special, but predictable", unfortunately. > In fact, I don't even think I know what you mean. With "special" to stand for the idea that vcpu->requests can change outside of the current execution thread. Letting the programmer assume additional guarantees makes the generated code and resulting behavior more predictable. >> READ_ONCE() is not necessary in any use I'm aware of, but there is no >> harm in telling the compiler that vcpu->requests are what we think they >> are ... > > Hmmm, I'm equally lost. vcpu->requests are volatile, so we need to assume that they can change at any moment when using them. I would prefer if vcpu->requests were of an atomic type and READ_ONCE() is about as close we can get without a major overhaul. >> >> /* >> * vcpu->requests are a lockless synchronization mechanism, where > > is the requests a synchronization mechanism? I think of it more as a > cross-thread communication protocol. Partly, synchronization is too restrictive and communication seems too generic, but probably still better. No idea how to shortly describe the part of vcpu->requests that prevents VM entry and that setting a request kicks VM out of guest mode. x86 uses KVM_REQ_MCLOCK_INPROGRESS for synchronization between cores and the use in this series looked very similar. >> * memory barriers are necessary for correct behavior, see >> * Documentation/virtual/kvm/vcpu-requests.rst. >> * >> * READ_ONCE() is not necessary for correctness, but simplifies >> * reasoning by constricting the generated code. >> */ >> >> I considered READ_ONCE() to be self-documenting. :) > > I realize that I'm probably unusually slow in this whole area, but using > READ_ONCE() where unnecessary doesn't help my reasoning, but makes me > wonder which part of this I didn't understand, so I don't seem to agree > with the statement that it simplifies reasoning. No, I think it is a matter of approach. When I see a READ_ONCE() without a comment, I think that the programmer was aware that this memory can change at any time and was defensive about it. I consider this use to simplify future development: We think now that READ_ONCE() is not needed, but vcpu->requests is still volatile and future changes in code might make READ_ONCE() necessary. Preemptively putting READ_ONCE() there saves us thinking or hard-to-find bugs. > Really, if there is no reason to use it, I don't think we should use it. I am leaning towards READ_ONCE() as the default for implicitly volatile memory, but commenting why we didn't have to use READ_ONCE() sounds good too. > To me, READ_ONCE() indicates that there's some flow in the code where > it's essential that the compiler doesn't generate multiple loads, but > that we only see a momentary single-read snapshot of the value, and this > doesn't seem to be the case. The compiler can also squash multiple reads together, which is more dangerous in this case as we would not notice a new requests. Avoiding READ_ONCE() requires a better knowledge of the compiler algorithms that prove which variable can be optimized. The difference is really minor and I agree that the comment is bad. The only comment I'm happy with is nothing, though ... even "READ_ONCE() is not necessary" is wrong as that might change without us noticing. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 0/5] KVM/ARM Fixes for v4.11-rc6
2017-04-05 12:13+0200, Christoffer Dall: > Hi Paolo and Radim, > > Please pull a handfull of fixes for the next -rc release of v4.11. > > They include: > - Addressing a problem with GICv3 userspace save/restore > - Clarify GICv2 userspace save/restore ABI > - Be more careful in clearing GIC LRs > - Add missing synchronization primitive to our MMU handling code Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 2/9] KVM: Add documentation for VCPU requests
2017-04-04 19:23+0200, Christoffer Dall: > On Tue, Apr 04, 2017 at 07:06:00PM +0200, Andrew Jones wrote: >> On Tue, Apr 04, 2017 at 05:24:03PM +0200, Christoffer Dall wrote: >> > On Fri, Mar 31, 2017 at 06:06:51PM +0200, Andrew Jones wrote: >> > > +and will definitely see the request, or is outside guest mode, but has >> > > yet >> > > +to do its final request check, and therefore when it does, it will see >> > > the >> > > +request, then things will work. However, the transition from outside to >> > > +inside guest mode, after the last request check has been made, opens a >> > > +window where a request could be made, but the VCPU would not see until >> > > it >> > > +exits guest mode some time later. See the table below. >> > >> > This text, and the table below, only deals with the details of entering >> > the guest. Should we talk about kvm_vcpu_exiting_guest_mode() and >> > anything related to exiting the guest? >> >> I think all !IN_GUEST_MODE should behave the same, so I was avoiding >> the use of EXITING_GUEST_MODE and OUTSIDE_GUEST_MODE, which wouldn't be >> hard to address, but then I'd also have to address >> READING_SHADOW_PAGE_TABLES, which may complicate the document more than >> necessary. I'm not sure we need to address a VCPU exiting guest mode, >> other than making sure it's clear that a VCPU that exits must check >> requests before it enters again. > > But the problem is that kvm_make_all_cpus_request() only sends IPIs to > CPUs where the mode was different from OUTSIDE_GUEST_MODE, so there it's > about !OUTSIDE_GUEST_MODE rather than !IN_GUEST_MODE, so there's some > subtlety here which I feel like it's dangerous to paper over. Right, that needs fixing in the code. guest_mode is just an optimization that allows us to skip sending the IPI when the VCPU is known to handle the request as soon as possible. IN_GUEST_MODE: we must force VM exit or the request could never be handled EXITING_GUEST_MODE: another request already forces the VM exit and we're just waiting for the VCPU to notice our request OUTSIDE_GUEST_MODE: KVM is going to notice our request without any intervention READING_SHADOW_PAGE_TABLES: same as OUTSIDE_GUEST_MODE -- rename to unwieldly OUTSIDE_GUEST_MODE_READING_SHADOW_PAGE_TABLES? The kick is needed only in IN_GUEST_MODE and a wake up is needed in case where the guest is halted OUTSIDE_GUEST_MODE ... Hm, maybe we should add a halt state too? ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 1/9] KVM: add kvm_request_pending
2017-04-04 18:41+0200, Andrew Jones: > On Tue, Apr 04, 2017 at 05:30:14PM +0200, Christoffer Dall wrote: >> On Fri, Mar 31, 2017 at 06:06:50PM +0200, Andrew Jones wrote: >> > From: Radim Krčmář >> > >> > A first step in vcpu->requests encapsulation. >> >> Could we have a note here on why we need to access vcpu->requests using >> READ_ONCE now? > > Sure, maybe we should put the note as a comment above the read in > kvm_request_pending(). Something like > > /* > * vcpu->requests reads may appear in sequences that have strict > * data or control dependencies. Use READ_ONCE() to ensure the > * compiler does not do anything that breaks the required ordering. > */ > > Radim? Uses of vcpu->requests should already have barriers that take care of the ordering. I think the main reason for READ_ONCE() is to tell programmers that requests are special, but predictable. READ_ONCE() is not necessary in any use I'm aware of, but there is no harm in telling the compiler that vcpu->requests are what we think they are ... /* * vcpu->requests are a lockless synchronization mechanism, where * memory barriers are necessary for correct behavior, see * Documentation/virtual/kvm/vcpu-requests.rst. * * READ_ONCE() is not necessary for correctness, but simplifies * reasoning by constricting the generated code. */ I considered READ_ONCE() to be self-documenting. :) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 00/10] KVM/ARM fixes for 4.11-rc2
2017-03-09 09:55+, Marc Zyngier: > Paolo, Radim, > > Here's the KVM/ARM updates for 4.11-rc2. The usual bag of vgic > updates, making the memslot array large enough to handle guests with > tons of devices assigned to them, a tidying up of exception handling, > and a rather annoying TLB handling issue on VHE systems. Pulled, but what made you change the GPG key into a revoked one? # gpg: Signature made Thu 09 Mar 2017 10:36:07 AM CET # gpg:using RSA key AB309C74B93B1EA1 # gpg: Good signature from "Marc Zyngier " # gpg: WARNING: This key has been revoked by its owner! # gpg: This could mean that the signature is forged. # gpg: reason for revocation: Key has been compromised # gpg: revocation comment: Revoked after kernel.org hacking # Primary key fingerprint: 6958 C9F2 233C 5E9D 6CCA 818A AB30 9C74 B93B 1EA1 Thanks. > Please pull, > > Thanks, > > M. > > The following changes since commit c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201: > > Linux 4.11-rc1 (2017-03-05 12:59:56 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git > tags/kvm-arm-for-4.11-rc2 > > for you to fetch changes up to 955a3fc6d2a1c11d6d00bce4f3816100ce0530cf: > > KVM: arm64: Increase number of user memslots to 512 (2017-03-09 09:13:50 > +) > > > KVM/ARM updates for v4.11-rc2 > > vgic updates: > - Honour disabling the ITS > - Don't deadlock when deactivating own interrupts via MMIO > - Correctly expose the lact of IRQ/FIQ bypass on GICv3 > > I/O virtualization: > - Make KVM_CAP_NR_MEMSLOTS big enough for large guests with > many PCIe devices > > General bug fixes: > - Gracefully handle exception generated with syndroms that > the host doesn't understand > - Properly invalidate TLBs on VHE systems > > > Andre Przywara (1): > KVM: arm/arm64: VGIC: Fix command handling while ITS being disabled > > Jintack Lim (1): > KVM: arm/arm64: Let vcpu thread modify its own active state > > Linu Cherian (4): > KVM: Add documentation for KVM_CAP_NR_MEMSLOTS > KVM: arm/arm64: Enable KVM_CAP_NR_MEMSLOTS on arm/arm64 > KVM: arm/arm64: Remove KVM_PRIVATE_MEM_SLOTS definition that are unused > KVM: arm64: Increase number of user memslots to 512 > > Marc Zyngier (2): > arm64: KVM: VHE: Clear HCR_TGE when invalidating guest TLBs > KVM: arm/arm64: vgic-v3: Don't pretend to support IRQ/FIQ bypass > > Mark Rutland (2): > arm: KVM: Survive unknown traps from guests > arm64: KVM: Survive unknown traps from guests > > Documentation/virtual/kvm/api.txt | 4 ++ > arch/arm/include/asm/kvm_arm.h | 1 + > arch/arm/include/asm/kvm_host.h| 1 - > arch/arm/kvm/arm.c | 3 + > arch/arm/kvm/handle_exit.c | 19 --- > arch/arm64/include/asm/kvm_host.h | 3 +- > arch/arm64/kvm/handle_exit.c | 19 --- > arch/arm64/kvm/hyp/tlb.c | 64 +++--- > include/linux/irqchip/arm-gic-v3.h | 2 + > virt/kvm/arm/vgic/vgic-its.c | 109 > ++--- > virt/kvm/arm/vgic/vgic-mmio.c | 32 --- > virt/kvm/arm/vgic/vgic-v3.c| 5 +- > 12 files changed, 183 insertions(+), 79 deletions(-) ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts
2017-03-08 03:45-0800, Christoffer Dall:
> On Tue, Mar 07, 2017 at 08:34:33PM +0100, Radim Krčmář wrote:
> > 2017-03-07 17:58+0100, Andrew Jones:
> > > On Mon, Mar 06, 2017 at 07:16:09AM -0800, Christoffer Dall wrote:
> > > > From: Christoffer Dall
> > > >
> > > > We found a deadlock when changing the active state of an interrupt while
> > > > the interrupt is queued on the LR of the running VCPU.
> > > >
> > > > Defend KVM against this bug in the future now when we've introduced a
> > > > fix.
> > > >
> > > > Signed-off-by: Christoffer Dall
> > > > ---
> > > > Sending with the right subject prefix this time.
> > > >
> > > > arm/gic.c | 43 +++
> > > > arm/unittests.cfg | 14 +-
> > > > lib/arm/asm/gic.h | 2 ++
> > > > 3 files changed, 58 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arm/gic.c b/arm/gic.c
> > > > index 3054d45..82f6632 100644
> > > > --- a/arm/gic.c
> > > > +++ b/arm/gic.c
> > > > @@ -254,6 +254,47 @@ static struct gic gicv3 = {
> > > > },
> > > > };
> > > >
> > > > +static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> > > > +{
> > > > + u32 irqstat = gic_read_iar();
> > > > + u32 irqnr = gic_iar_irqnr(irqstat);
> > > > +
> > > > + if (irqnr != GICC_INT_SPURIOUS) {
> > > > + void *base;
> > > > + u32 val = 1 << IPI_IRQ;
> > > > +
> > > > + if (gic_version() == 2)
> > > > + base = gicv2_dist_base();
> > > > + else
> > > > + base = gicv3_redist_base();
> > >
> > > Using the redistributor interface is correct with the current
> > > gic_enable_defaults(), because it enables affinity routing. I
> > > wonder if we shouldn't confirm it's enabled first though.
> > >
> > > > +
> > > > + writel(val, base + GICD_ICACTIVER);
> > >
> > > For gicv3, with affinity routing enabled, the offset is
> > > technically named GICR_ICACTIVER0, but that's also 0x380, so
> > > it doesn't matter.
> > >
> > > Not sure we need the 'val' variable.
> > >
> > > > +
> > > > + smp_rmb(); /* pairs with wmb in stats_reset */
> > > > + ++acked[smp_processor_id()];
> > > > + check_irqnr(irqnr);
> > > > + smp_wmb(); /* pairs with rmb in check_acked */
> > > > + } else {
> > > > + ++spurious[smp_processor_id()];
> > > > + smp_wmb();
> > > > + }
> > > > +}
> > > > +
> > > > +static void run_active_clear_test(void)
> > > > +{
> > > > + report_prefix_push("active");
> > > > + gic_enable_defaults();
> > > > +#ifdef __arm__
> > > > + install_exception_handler(EXCPTN_IRQ, ipi_clear_active_handler);
> > > > +#else
> > > > + install_irq_handler(EL1H_IRQ, ipi_clear_active_handler);
> > > > +#endif
> > > > + local_irq_enable();
> > > > +
> > > > + ipi_test_self();
> > > > + report_prefix_pop();
> > > > +}
> > > > +
> > > > int main(int argc, char **argv)
> > > > {
> > > > char pfx[8];
> > > > @@ -290,6 +331,8 @@ int main(int argc, char **argv)
> > > > cpu == IPI_SENDER ? ipi_send :
> > > > ipi_recv);
> > > > }
> > > > ipi_recv();
> > > > + } else if (strcmp(argv[1], "active") == 0) {
> > > > + run_active_clear_test();
> > >
> > > test_active_clear() ?
> > >
> > > > } else {
> > > > report_abort("Unknown subtest '%s'", argv[1]);
> > > > }
> > > > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > > > index c98658f..32d9858 100644
> > > > --- a/arm/unittests.cfg
> > > > +++ b/arm/unittests.cfg
> > > > @
Re: [kvm-unit-tests PATCH] arm/arm64: gic: Test changing active state of interrupts
2017-03-07 17:58+0100, Andrew Jones:
> On Mon, Mar 06, 2017 at 07:16:09AM -0800, Christoffer Dall wrote:
> > From: Christoffer Dall
> >
> > We found a deadlock when changing the active state of an interrupt while
> > the interrupt is queued on the LR of the running VCPU.
> >
> > Defend KVM against this bug in the future now when we've introduced a
> > fix.
> >
> > Signed-off-by: Christoffer Dall
> > ---
> > Sending with the right subject prefix this time.
> >
> > arm/gic.c | 43 +++
> > arm/unittests.cfg | 14 +-
> > lib/arm/asm/gic.h | 2 ++
> > 3 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/arm/gic.c b/arm/gic.c
> > index 3054d45..82f6632 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -254,6 +254,47 @@ static struct gic gicv3 = {
> > },
> > };
> >
> > +static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> > +{
> > + u32 irqstat = gic_read_iar();
> > + u32 irqnr = gic_iar_irqnr(irqstat);
> > +
> > + if (irqnr != GICC_INT_SPURIOUS) {
> > + void *base;
> > + u32 val = 1 << IPI_IRQ;
> > +
> > + if (gic_version() == 2)
> > + base = gicv2_dist_base();
> > + else
> > + base = gicv3_redist_base();
>
> Using the redistributor interface is correct with the current
> gic_enable_defaults(), because it enables affinity routing. I
> wonder if we shouldn't confirm it's enabled first though.
>
> > +
> > + writel(val, base + GICD_ICACTIVER);
>
> For gicv3, with affinity routing enabled, the offset is
> technically named GICR_ICACTIVER0, but that's also 0x380, so
> it doesn't matter.
>
> Not sure we need the 'val' variable.
>
> > +
> > + smp_rmb(); /* pairs with wmb in stats_reset */
> > + ++acked[smp_processor_id()];
> > + check_irqnr(irqnr);
> > + smp_wmb(); /* pairs with rmb in check_acked */
> > + } else {
> > + ++spurious[smp_processor_id()];
> > + smp_wmb();
> > + }
> > +}
> > +
> > +static void run_active_clear_test(void)
> > +{
> > + report_prefix_push("active");
> > + gic_enable_defaults();
> > +#ifdef __arm__
> > + install_exception_handler(EXCPTN_IRQ, ipi_clear_active_handler);
> > +#else
> > + install_irq_handler(EL1H_IRQ, ipi_clear_active_handler);
> > +#endif
> > + local_irq_enable();
> > +
> > + ipi_test_self();
> > + report_prefix_pop();
> > +}
> > +
> > int main(int argc, char **argv)
> > {
> > char pfx[8];
> > @@ -290,6 +331,8 @@ int main(int argc, char **argv)
> > cpu == IPI_SENDER ? ipi_send : ipi_recv);
> > }
> > ipi_recv();
> > + } else if (strcmp(argv[1], "active") == 0) {
> > + run_active_clear_test();
>
> test_active_clear() ?
>
> > } else {
> > report_abort("Unknown subtest '%s'", argv[1]);
> > }
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index c98658f..32d9858 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -26,7 +26,7 @@
> >
> > ##
> >
> > #
> > -# Test that the configured number of processors (smp = ), and
> > +/# Test that the configured number of processors (smp = ), and
>
> stray character here
>
> > # that the configured amount of memory (-m ) are correctly setup
> > # by the framework.
> > #
> > @@ -92,6 +92,18 @@ smp = $MAX_SMP
> > extra_params = -machine gic-version=3 -append 'ipi'
> > groups = gic
> >
> > +[gicv2-active]
> > +file = gic.flat
> > +smp = $((($MAX_SMP < 8)?$MAX_SMP:8))
> > +extra_params = -machine gic-version=2 -append 'active'
> > +groups = gic
> > +
> > +[gicv3-active]
> > +file = gic.flat
> > +smp = $MAX_SMP
> > +extra_params = -machine gic-version=3 -append 'active'
> > +groups = gic
> > +
> > # Test PSCI emulation
> > [psci]
> > file = psci.flat
> > diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
> > index c8186f2..c688ccc 100644
> > --- a/lib/arm/asm/gic.h
> > +++ b/lib/arm/asm/gic.h
> > @@ -12,6 +12,8 @@
> > #define GICD_TYPER 0x0004
> > #define GICD_IGROUPR 0x0080
> > #define GICD_ISENABLER 0x0100
> > +#define GICD_ISACTIVER 0x0300
> > +#define GICD_ICACTIVER 0x0380
> > #define GICD_IPRIORITYR0x0400
> > #define GICD_SGIR 0x0f00
> >
> > --
> > 2.5.0
> >
>
> Everything besides the stray character in unittests.cfg is on
> nit level, and the stray character can probably be cleaned up
> on commit, so
>
> Reviewed-by: Andrew Jones
Christoffer, I have fixed the slash in a local repo, ready to push if
you do not want any other changes.
Btw. can the host be rebooted without magic-SysRq after hitting the bug?
(I was wondering if the bug was bad enough for the nodefault group, but
Drew didn't point it out and the h
Re: [PATCH kvm-unit-tests v2 0/2] arm/arm64: introduce PSCI tests
Applied, thanks. 2017-03-03 15:30+0100, Andrew Jones: > Andrew Jones (1): > arm/arm64: psci: support testing tcg > > Levente Kurusa (1): > arm/arm64: Add PSCI tests > > arm/Makefile.common | 1 + > arm/psci.c | 150 > > arm/unittests.cfg | 6 +++ > 3 files changed, 157 insertions(+) > create mode 100644 arm/psci.c > > -- > 2.9.3 > ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvm-unit-tests 1/2] arm/arm64: Add PSCI tests
2017-02-27 18:32+0100, Andrew Jones:
> From: Levente Kurusa
>
> The cpu_on test exposed two races in KVM's PSCI emulation.
>
> Signed-off-by: Levente Kurusa
> [Rewrote the cpu_on test to improve the chance of hitting the race.
> Also added affinity-info tests and made some minor cleanups. -drew]
> Signed-off-by: Andrew Jones
> ---
> arm/Makefile.common | 1 +
> arm/psci.c | 112
>
> arm/unittests.cfg | 6 +++
> 3 files changed, 119 insertions(+)
> create mode 100644 arm/psci.c
>
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f7193c3e6d64..74c7394eb7c1 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -14,6 +14,7 @@ tests-common += $(TEST_DIR)/spinlock-test.flat
> tests-common += $(TEST_DIR)/pci-test.flat
> tests-common += $(TEST_DIR)/pmu.flat
> tests-common += $(TEST_DIR)/gic.flat
> +tests-common += $(TEST_DIR)/psci.flat
>
> tests-all = $(tests-common) $(tests)
> all: $(tests-all)
> diff --git a/arm/psci.c b/arm/psci.c
> new file mode 100644
> index ..e571afb92c7e
> --- /dev/null
> +++ b/arm/psci.c
> @@ -0,0 +1,112 @@
> +/*
> + * PSCI tests
> + *
> + * Copyright (C) 2017, Red Hat, Inc.
> + * Author: Levente Kurusa
> + * Author: Andrew Jones
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include
> +#include
> +#include
> +
> +static bool psci_invalid_function(void)
> +{
> + return psci_invoke(1337, 0, 0, 0) == PSCI_RET_NOT_SUPPORTED;
> +}
> +
> +static int psci_affinity_info(unsigned long target_affinity, uint32_t
> lowest_affinity_level)
> +{
> +#ifdef __arm__
> + return psci_invoke(PSCI_0_2_FN_AFFINITY_INFO, target_affinity,
> lowest_affinity_level, 0);
> +#else
> + return psci_invoke(PSCI_0_2_FN64_AFFINITY_INFO, target_affinity,
> lowest_affinity_level, 0);
> +#endif
> +}
> +
> +static bool psci_affinity_info_on(void)
> +{
> + return psci_affinity_info(cpus[0], 0) == PSCI_0_2_AFFINITY_LEVEL_ON;
> +}
> +
> +static bool psci_affinity_info_off(void)
> +{
> + return psci_affinity_info(cpus[1], 0) == PSCI_0_2_AFFINITY_LEVEL_OFF;
Please check that smp > 1 here
> +}
> +
> +static int cpu_on_ret[NR_CPUS];
> +static cpumask_t cpu_on_ready, cpu_on_done;
> +static volatile int cpu_on_start;
> +
> +static void cpu_on_secondary_entry(void)
> +{
> + int cpu = smp_processor_id();
> +
> + cpumask_set_cpu(cpu, &cpu_on_ready);
> + while (!cpu_on_start)
> + cpu_relax();
> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
> + cpumask_set_cpu(cpu, &cpu_on_done);
> + halt();
> +}
> +
> +static bool psci_cpu_on_test(void)
> +{
> + bool failed = false;
> + int cpu;
and here too.
Thanks.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH] KVM: race-free exit from KVM_RUN without POSIX signals
2017-02-15 15:43+0100, Paolo Bonzini:
> The purpose of the KVM_SET_SIGNAL_MASK API is to let userspace "kick"
> a VCPU out of KVM_RUN through a POSIX signal. A signal is attached
> to a dummy signal handler; by blocking the signal outside KVM_RUN and
> unblocking it inside, this possible race is closed:
>
> VCPU thread service thread
>--
> check flag
> set flag
> raise signal
> (signal handler does nothing)
> KVM_RUN
>
> However, one issue with KVM_SET_SIGNAL_MASK is that it has to take
> tsk->sighand->siglock on every KVM_RUN. This lock is often on a
> remote NUMA node, because it is on the node of a thread's creator.
> Taking this lock can be very expensive if there are many userspace
> exits (as is the case for SMP Windows VMs without Hyper-V reference
> time counter).
>
> As an alternative, we can put the flag directly in kvm_run so that
> KVM can see it:
>
> VCPU thread service thread
>--
> raise signal
> signal handler
> set run->immediate_exit
> KVM_RUN
> check run->immediate_exit
>
> Signed-off-by: Paolo Bonzini
> ---
The old immediate exit with signal did more work, but none of it should
affect user-space, so it looks like another minor optimization,
Reviewed-by: Radim Krčmář
> change from RFC:
> - implement in each architecture to ensure MMIO is completed
> [Radim]
> - do not clear the flag [David Hildenbrand, offlist]
>
> Documentation/virtual/kvm/api.txt | 13 -
> arch/arm/kvm/arm.c| 4
> arch/mips/kvm/mips.c | 7 ++-
> arch/powerpc/kvm/powerpc.c| 6 +-
> arch/s390/kvm/kvm-s390.c | 4
> arch/x86/kvm/x86.c| 6 +-
> include/uapi/linux/kvm.h | 4 +++-
> 7 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/virtual/kvm/api.txt
> b/Documentation/virtual/kvm/api.txt
> index e4f2cdcf78eb..925b1b6be073 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -3389,7 +3389,18 @@ struct kvm_run {
> Request that KVM_RUN return when it becomes possible to inject external
> interrupts into the guest. Useful in conjunction with KVM_INTERRUPT.
>
> - __u8 padding1[7];
> + __u8 immediate_exit;
> +
> +This field is polled once when KVM_RUN starts; if non-zero, KVM_RUN
> +exits immediately, returning -EINTR. In the common scenario where a
> +signal is used to "kick" a VCPU out of KVM_RUN, this field can be used
> +to avoid usage of KVM_SET_SIGNAL_MASK, which has worse scalability.
> +Rather than blocking the signal outside KVM_RUN, userspace can set up
> +a signal handler that sets run->immediate_exit to a non-zero value.
> +
> +This field is ignored if KVM_CAP_IMMEDIATE_EXIT is not available.
> +
> + __u8 padding1[6];
>
> /* out */
> __u32 exit_reason;
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 21c493a9e5c9..c9a2103faeb9 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -206,6 +206,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long
> ext)
> case KVM_CAP_ARM_PSCI_0_2:
> case KVM_CAP_READONLY_MEM:
> case KVM_CAP_MP_STATE:
> + case KVM_CAP_IMMEDIATE_EXIT:
> r = 1;
> break;
> case KVM_CAP_COALESCED_MMIO:
> @@ -604,6 +605,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct
> kvm_run *run)
> return ret;
> }
>
> + if (run->immediate_exit)
> + return -EINTR;
> +
> if (vcpu->sigset_active)
> sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);
>
> diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c
> index 31ee5ee0010b..ed81e5ac1426 100644
> --- a/arch/mips/kvm/mips.c
> +++ b/arch/mips/kvm/mips.c
> @@ -397,7 +397,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> *vcpu,
>
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> {
> - int r = 0;
> + int r = -EINTR;
> sigset_t sigsaved;
>
> if (vcpu->sigset_active)
> @@ -409,6 +409,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct
> kvm_run *run)
> vcpu->mmio_needed = 0;
> }
>
Re: [PATCH 0/3] KVM/ARM updates for 4.10-rc4
2017-01-13 11:31+, Marc Zyngier: > Radim, Paolo, > > Here's the KVM/ARM updates for 4.10-rc4. Two timer fixes, and one vgic > fix for a deadlock that's been reported this week (which should land > into stable). Pulled to kvm/master, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL] KVM/ARM updates for 4.9-rc7
2016-12-01 09:25+, Marc Zyngier: > Paolo, Radim, > > Hopefully, this is the last update for 4.9. This time, a single patch > that prevents bogus acknoledgement of interrupts. > > It'd be great if this could make it into v4.9-final Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [kvm-unit-tests PATCH] configure: honour $ARCH and $CROSS_COMPILE
2016-11-21 18:22+, Andre Przywara:
> Both environment variables seem to be standard in cross-compilation
> environments, especially with Linux.
> Let the configure script take those into account when setting the default
> values for --arch and --cross-prefix. Explicitly specifying the latter
> on the configure command line still works as expected.
>
> Signed-off-by: Andre Przywara
> ---
> Hi,
>
> this maybe a personal itch to scratch here, since I set these two
> variables in my environment via a (sourced) script here and never have
> to care about the particular cross-compiler prefix, for instance.
> It looks rather generic, though, so I was wondering if this is useful
> upstream as well.
Definitely useful, thanks.
> diff --git a/configure b/configure
> @@ -8,8 +8,9 @@ objdump=objdump
> ar=ar
> addr2line=addr2line
> arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> +[ -n "$ARCH" ] && arch="$ARCH"
> host=$arch
Host should be set before we override arch, though.
I can swap those two lines when applying, but would prefer something
like:
host=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
arch=${ARCH:-$host}
> -cross_prefix=
> +cross_prefix=${CROSS_COMPILE}
> endian=""
> pretty_print_stacks=yes
(And --help is not printing the default value for cross_prefix, which
would be nice to change when we can have a non-empty one now.)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL] KVM/ARM updates for 4.9-rc6
2016-11-18 09:39+, Marc Zyngier: > Paolo, Radim, > > Please find below the pull request for a couple of fixes for the > PMU emulation, courtesy of Wei. Both patches are candidates for stable. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 00/50] KVM/ARM Changes for v4.9
2016-09-27 20:05+0200, Christoffer Dall: > Hi Paolo and Radim, > > Here are the KVM/ARM Changes for v4.9. They include: > > - Various cleanups and removal of redundant code > - Two important fixes for not using an in-kernel irqchip > - A bit of optimizations > - Handle SError exceptions and present them to guests if appropriate > - Proxying of GICV access at EL2 if guest mappings are unsafe > - GICv3 on AArch32 on ARMv8 > - Preparations for GICv3 save/restore, including ABI docs Pulled to kvm/next, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH 1/2] config: move x86 kvm_guest.config to a common locaton
2016-09-08 13:41-0500, Rob Herring: > kvm_guest.config is useful for KVM guests on other arches, and nothing > in it appears to be x86 specific, so just move the whole file. Kbuild > will find it in either location. > > Signed-off-by: Rob Herring > Cc: Christoffer Dall > Cc: Marc Zyngier > Cc: Paolo Bonzini > Cc: "Radim Krčmář" > Cc: kvmarm@lists.cs.columbia.edu > Cc: k...@vger.kernel.org > --- Applied them both to kvm/queue, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 0/2] KVM: Synchronize KVM devices list access and create ops
2016-08-09 19:12+0200, Christoffer Dall: > Currently accesses the kvm->devices list is not synchronized by any > mechanism which can potentially lead to data corruption. Further, a > number of the create operations on the individual devices are racy and > would allow creation of multiple devices, opposite to the intention. > > Factor out portions of the XICS create operation into a separate init > operation and protect the remaining list accesses and create operations > with the kvm->lock. > > Tested on arm/arm64 and compile-tested on powerpc for the xics changes. > Tested-by on other archs would be appreciated. Applied, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL] KVM/ARM updates for Linux 4.8, take #1
2016-07-22 18:28+0100, Marc Zyngier: > Paolo, Radim, > > Please find below the first batch of 4.8 updates for KVM/ARM. Biggest > feature is the long awaited GICv3 ITS emulation, allowing MSIs to be > delivered into guests running on GICv3 HW. The other big feature is > the removal of the old vgic implementation. Less visible is the revamp > of the way we deal with the HYP initialization (by keeping an idmap > around), and making HYP page tables honor the kernel's own protection. > > This may generate a few conflicts, all of which have been seen in > -next. 3 which are KVM specific (and pretty easy to resolve), and > another one in include/irqchip/arm-gic-v3.h. They should be resolved > as in -next. > > Please pull! Pulled into kvm/next, KVM_CAP_MSI_DEVID is 131. > 54 files changed, 2698 insertions(+), 6065 deletions(-) <3 ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v9 0/6] KVM: arm/arm64: gsi routing support
2016-07-22 16:20+, Eric Auger: > With the advent of GICv3 ITS in-kernel emulation, KVM MSI routing > becomes mandated for proper VIRTIO-PCI vhost integration. Changes to interfaces and common code look ok, Acked-by: Radim Krčmář Good work. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v8 5/7] KVM: arm/arm64: Enable irqchip routing
2016-07-22 13:46+, Eric Auger:
> This patch adds compilation and link against irqchip.
>
> Main motivation behind using irqchip code is to enable MSI
> routing code. In the future irqchip routing may also be useful
> when targeting multiple irqchips.
>
> Routing standard callbacks now are implemented in vgic-irqfd:
> - kvm_set_routing_entry
> - kvm_set_irq
> - kvm_set_msi
>
> They only are supported with new_vgic code.
>
> Both HAVE_KVM_IRQCHIP and HAVE_KVM_IRQ_ROUTING are defined.
> KVM_CAP_IRQ_ROUTING is advertised and KVM_SET_GSI_ROUTING is allowed.
>
> So from now on IRQCHIP routing is enabled and a routing table entry
> must exist for irqfd injection to succeed for a given SPI. This patch
> builds a default flat irqchip routing table (gsi=irqchip.pin) covering
> all the VGIC SPI indexes. This routing table is overwritten by the
> first first user-space call to KVM_SET_GSI_ROUTING ioctl.
>
> MSI routing setup is not yet allowed.
>
> Signed-off-by: Eric Auger
>
> ---
> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
> @@ -17,36 +17,101 @@
> #include
> #include
> #include
> +#include
> +#include "vgic.h"
>
> -int kvm_irq_map_gsi(struct kvm *kvm,
> - struct kvm_kernel_irq_routing_entry *entries,
> - int gsi)
> +/**
> + * vgic_irqfd_set_irq: inject the IRQ corresponding to the
> + * irqchip routing entry
> + *
> + * This is the entry point for irqfd IRQ injection
> + */
> +static int vgic_irqfd_set_irq(struct kvm_kernel_irq_routing_entry *e,
> + struct kvm *kvm, int irq_source_id,
> + int level, bool line_status)
> {
> - return 0;
> + unsigned int spi_id = e->irqchip.pin + VGIC_NR_PRIVATE_IRQS;
> + struct vgic_dist *dist = &kvm->arch.vgic;
> +
> + if (spi_id > min(dist->nr_spis, VGIC_MAX_SPI))
This is more strict that vgic_valid_spi(), because spi_id between
"dist->nr_spis" and "dist->nr_spis + VGIC_NR_PRIVATE_IRQS" is not
allowed, which probably wasn't intended.
And shouldn't nr_spis always be less that VGIC_MAX_SPI?
Thanks.
> + return -EINVAL;
> + return kvm_vgic_inject_irq(kvm, 0, spi_id, level);
> }
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v8 6/7] KVM: arm/arm64: Enable MSI routing
2016-07-22 13:46+, Eric Auger:
> Up to now, only irqchip routing entries could be set. This patch
> adds the capability to insert MSI routing entries.
>
> For ARM64, let's also increase KVM_MAX_IRQ_ROUTES to 4096: this
> include SPI irqchip routes plus MSI routes. In the future this
> might be extended.
>
> Signed-off-by: Eric Auger
> Reviewed-by: Andre Przywara
>
> ---
> v7 -> v8:
> - adapt to changes in kvm_kernel_irq_routing_entry and check the
> user entry flags according to the user entry type
>
> v6 -> v7:
> - added Andre's R-b
>
> v2 -> v3:
> - remove any reference to KVM_IRQ_ROUTING_EXTENDED_MSI type
> - unconditionnaly uapi flags and devid downto the kernel
> routing entry struct
> - handle KVM_MSI_VALID_DEVID flag in kvm_set_irq_routing
> - note about KVM_CAP_MSI_DEVID moved in the first patch file
> of the series
>
> v1 -> v2:
> - adapt to new routing entry types
>
> RFC -> PATCH:
> - move api MSI routing updates into that patch file
> - use new devid field of user api struct
> ---
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> @@ -201,23 +202,25 @@ int kvm_set_irq_routing(struct kvm *kvm,
> new->chip[i][j] = -1;
>
> for (i = 0; i < nr; ++i) {
> - struct kvm_kernel_irq_routing_entry *e;
> -
> r = -ENOMEM;
> e = kzalloc(sizeof(*e), GFP_KERNEL);
> if (!e)
> goto out;
>
> r = -EINVAL;
> - if (ue->flags) {
> - kfree(e);
> - goto out;
> + switch (ue->type) {
> + case KVM_IRQ_ROUTING_IRQCHIP:
> + if (ue->flags)
> + goto free_entry;
> + break;
> + case KVM_IRQ_ROUTING_MSI:
> + if (ue->flags & ~KVM_MSI_VALID_DEVID)
> + goto free_entry;
> + break;
The function is common for all arches and there are currently two other
routing types (S390_ADAPTER and HV_SINT) that ought to be checked as
well, so "default" instead of "KVM_IRQ_ROUTING_IRQCHIP" would be better.
> }
> r = setup_routing_entry(new, e, ue);
> - if (r) {
> - kfree(e);
> - goto out;
> - }
> + if (r)
> + goto free_entry;
> ++ue;
> }
>
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 7/7] KVM: arm: enable KVM_SIGNAL_MSI and MSI routing
2016-07-22 15:52+0200, Auger Eric:
> On 22/07/2016 15:39, Radim Krčmář wrote:
>> 2016-07-21 23:10+0200, Auger Eric:
>>> On 21/07/2016 18:33, Radim Krčmář wrote:
>>>> 2016-07-18 13:25+, Eric Auger:
>>>>> If the ITS modality is not available, let's simply support MSI
>>>>> injection by transforming the MSI.data into an SPI ID.
>>>>>
>>>>> This becomes possible to use KVM_SIGNAL_MSI ioctl and MSI
>>>>> routing for arm too.
>>>>>
>>>>> Signed-off-by: Eric Auger
>>>>>
>>>>> ---
>>>>> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c
>>>>> b/virt/kvm/arm/vgic/vgic-irqfd.c
>>>>> +static int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
>>>>> +{
>>>>> + if (msi->flags & KVM_MSI_VALID_DEVID)
>>>>> + return -EINVAL;
>>>>> + if (!vgic_valid_spi(kvm, msi->data))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
>>>>
>>>> Hm, this isn't very MSI related ...
>>>>
>>>> arm already has KVM_IRQ_LINE/kvm_vm_ioctl_irq_line with
>>>> KVM_ARM_IRQ_TYPE_SPI that does
>>>> kvm_vgic_inject_irq(kvm, 0, irq_num, level)
>>>>
>>>> Is that interface lacking?
>>>
>>> You mean KVM_SIGNAL_MSI? Well at QEMU level, for ARM/ARM64 is doesn't.
>>
>> No, I meant KVM_IRQ_LINE, the one that is used to deliver SPI today.
>> Or isn't it?
>>
>>> For kvm-tools I guess, Andre manages without.
>>>
>>> My first feeling was it is part of the KVM API and we can implement it
>>> easily for GICv2M, as we do for GICv3 ITS . This can avoid a user app to
>>> do what QEMU implements as "kvm_gsi_direct_mapping" and manage the
>>> translation into the semantic of the ARM GSI.
>>
>> I think that reusing KVM_SIGNAL_MSI and KVM_IRQ_ROUTING_MSI for SPI is
>> unfortunate.
>>
>> SPI only uses msi.data, which makes remaining fields in the msi struct
>> arbitrary and [5/7] defined KVM_IRQ_ROUTING_IRQCHIP for SPI, so two
>> route types now do the same, but only sometimes (without ITS), which
>> makes the situation even less understandable ...
>>
>> Delivering SPI as KVM_IRQ_ROUTING_IRQCHIP seems more sensible and if we
>> wanted ad-hoc delivery of KVM_IRQ_ROUTING_IRQCHIP, then I would prefer a
>> new interface to two different meanings for KVM_SIGNAL_MSI:
>> KVM_SIGNAL_MSI was created because we didn't have anything that could
>> inject an interrupt without setting up a route with KVM_SET_GSI_ROUTING
>> and we are still missing a generic interface to do that.
>>
>>> But Well, if you prefer we do not implement it for GICv2M, since
>>> considered as far fetched I can remove this patch.
>>
>> I do, thanks. Documentation in [6/7] was ahead and needs changing then.
>
> Argh just saw your reply after sending v8. Will respin immediatly.
>
> Sorry for the confusion
No problem. Give me half an hour for a review, please. :)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 7/7] KVM: arm: enable KVM_SIGNAL_MSI and MSI routing
2016-07-21 23:10+0200, Auger Eric:
> On 21/07/2016 18:33, Radim Krčmář wrote:
>> 2016-07-18 13:25+, Eric Auger:
>>> If the ITS modality is not available, let's simply support MSI
>>> injection by transforming the MSI.data into an SPI ID.
>>>
>>> This becomes possible to use KVM_SIGNAL_MSI ioctl and MSI
>>> routing for arm too.
>>>
>>> Signed-off-by: Eric Auger
>>>
>>> ---
>>> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
>>> +static int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
>>> +{
>>> + if (msi->flags & KVM_MSI_VALID_DEVID)
>>> + return -EINVAL;
>>> + if (!vgic_valid_spi(kvm, msi->data))
>>> + return -EINVAL;
>>> +
>>> + return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
>>
>> Hm, this isn't very MSI related ...
>>
>> arm already has KVM_IRQ_LINE/kvm_vm_ioctl_irq_line with
>> KVM_ARM_IRQ_TYPE_SPI that does
>> kvm_vgic_inject_irq(kvm, 0, irq_num, level)
>>
>> Is that interface lacking?
>
> You mean KVM_SIGNAL_MSI? Well at QEMU level, for ARM/ARM64 is doesn't.
No, I meant KVM_IRQ_LINE, the one that is used to deliver SPI today.
Or isn't it?
> For kvm-tools I guess, Andre manages without.
>
> My first feeling was it is part of the KVM API and we can implement it
> easily for GICv2M, as we do for GICv3 ITS . This can avoid a user app to
> do what QEMU implements as "kvm_gsi_direct_mapping" and manage the
> translation into the semantic of the ARM GSI.
I think that reusing KVM_SIGNAL_MSI and KVM_IRQ_ROUTING_MSI for SPI is
unfortunate.
SPI only uses msi.data, which makes remaining fields in the msi struct
arbitrary and [5/7] defined KVM_IRQ_ROUTING_IRQCHIP for SPI, so two
route types now do the same, but only sometimes (without ITS), which
makes the situation even less understandable ...
Delivering SPI as KVM_IRQ_ROUTING_IRQCHIP seems more sensible and if we
wanted ad-hoc delivery of KVM_IRQ_ROUTING_IRQCHIP, then I would prefer a
new interface to two different meanings for KVM_SIGNAL_MSI:
KVM_SIGNAL_MSI was created because we didn't have anything that could
inject an interrupt without setting up a route with KVM_SET_GSI_ROUTING
and we are still missing a generic interface to do that.
> But Well, if you prefer we do not implement it for GICv2M, since
> considered as far fetched I can remove this patch.
I do, thanks. Documentation in [6/7] was ahead and needs changing then.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 2/7] KVM: kvm_host: add devid in kvm_kernel_irq_routing_entry
2016-07-21 17:54+0100, Marc Zyngier:
> On 21/07/16 17:13, Radim Krčmář wrote:
>> 2016-07-18 13:25+, Eric Auger:
>>> Extend kvm_kernel_irq_routing_entry to transport the device id
>>> field, devid. A new flags field makes possible to indicate the
>>> devid is valid. Those additions are used for ARM GICv3 ITS MSI
>>> injection.
>>>
>>> Signed-off-by: Eric Auger
>>> Acked-by: Christoffer Dall
>>>
>>> ---
>>> v6 -> v7:
>>> - added msi_ prefix to flags and dev_id fields
>>>
>>> v4 -> v5:
>>> - add Christoffer's R-b
>>>
>>> v2 -> v3:
>>> - add flags
>>>
>>> v1 -> v2:
>>> - replace msi_msg field by a struct composed of msi_msg and devid
>>>
>>> RFC -> PATCH:
>>> - reword the commit message after change in first patch (uapi)
>>> ---
>>> include/linux/kvm_host.h | 6 +-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>> index c87fe6f..3d2cbb4 100644
>>> --- a/include/linux/kvm_host.h
>>> +++ b/include/linux/kvm_host.h
>>> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry {
>>> unsigned irqchip;
>>> unsigned pin;
>>> } irqchip;
>>> - struct msi_msg msi;
>>> + struct {
>>> + struct msi_msg msi;
>>> + u32 msi_flags;
>>> + u32 msi_devid;
>>
>> I'd much rather see them as msi.flags and msi.devid.
>
> That's not really possible, as msi_msg is the core data structure for
> MSIs, and nobody really expects this tructure to change.
>
>> I didn't notice a code that passes struct msi_msg anywhere, so using an
>> ad-hoc struct like irqchip or defining a new one would work fine.
>
> I guess we could have an arm-specific one:
>
> struct arm_msi {
> struct msi_msg msg;
> u32 flags;
> u32 devid;
> };
>
> and use that. Would that be OK with you?
The feature wants to be arch-neutral, so I would rather ignore struct
msi_msg. kvm_kernel_irq_routing_entry practically mirrors routing
entries from KVM API and there we have:
struct kvm_msi {
__u32 address_lo;
__u32 address_hi;
__u32 data;
__u32 flags;
__u32 devid;
__u8 pad[12];
};
I think that something like
struct {
u32 address_lo;
u32 address_hi;
u32 data;
u32 flags;
u32 devid;
} msi;
would get us consistency, minimal changes to existing code, no namespace
hierarchy, and no "." vs "_" mistakes at a good price of some code
duplication and concept separation.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 1/7] KVM: api: pass the devid in the msi routing entry
2016-07-21 17:43+0100, Andre Przywara: > Hi Radim, > > On 21/07/16 17:01, Radim Krčmář wrote: >> 2016-07-18 13:25+, Eric Auger: >>> On ARM, the MSI msg (address and data) comes along with >>> out-of-band device ID information. The device ID encodes the >>> device that writes the MSI msg. Let's convey the device id in >>> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in >>> kvm_irq_routing_entry to indicate the msi devid is populated. >>> >>> Signed-off-by: Eric Auger >>> Reviewed-by: Andre Przywara >>> >>> --- >>> >>> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier >>> + for the device that wrote the MSI message. >>> + For PCI, this is usually a BFD identifier in the lower 16 bits. >>> + >>> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to >>> +provide the device ID. If this capability is not set, userland cannot >>> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set. >> >> It would be better to enforce this mentioned dependency on set >> KVM_CAP_MSI_DEVID, but is the dependency even required? >> It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID >> couldn't have been set by old userspaces, therefor it is ok to only make >> it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does >> now. (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being >> merged at the same time.) >> >> Then there would be little point in having KVM_CAP_MSI_DEVID enableable, >> so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid >> devid? > > KVM_CAP_MSI_DEVID tells userland that it's fine to set the > KVM_MSI_VALID_DEVID flag (because the kernel would bark otherwise). > > KVM_MSI_VALID_DEVID tells the kernel that there is some meaningful > device ID data in the field formerly known as "pad". > > IIRC we started with the VALID_DEVID flag, then found that we need the > CAP because we repurposed the pad field. > > Does that make sense? Admittedly this _is_ confusing ;-) It does, thanks. Some capability is need and I thought that KVM_CAP_MSI_DEVID has to be enabled by userspace before KVM_MSI_VALID_DEVID can be used, which isn't the case. It is enabled conditionally based on vgic ITS ... my bad. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 7/7] KVM: arm: enable KVM_SIGNAL_MSI and MSI routing
2016-07-18 13:25+, Eric Auger:
> If the ITS modality is not available, let's simply support MSI
> injection by transforming the MSI.data into an SPI ID.
>
> This becomes possible to use KVM_SIGNAL_MSI ioctl and MSI
> routing for arm too.
>
> Signed-off-by: Eric Auger
>
> ---
>
> v6 -> v7:
> - move vgic_v2m_inject_msi into vgic-irqfd
>
> v4 -> v5:
> - on vgic_v2m_inject_msi check the msi->data is within the SPI range
> - move KVM_HAVE_MSI in the KVM section (to be symetrical with ARM64)
>
> v2 -> v3:
> - reword the commit message
> - add sanity check about devid provision
>
> v1 -> v2:
> - introduce vgic_v2m_inject_msi in vgic-v2-emul.c following Andre's
> advice
> ---
> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
> +static int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
> +{
> + if (msi->flags & KVM_MSI_VALID_DEVID)
> + return -EINVAL;
> + if (!vgic_valid_spi(kvm, msi->data))
> + return -EINVAL;
> +
> + return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
Hm, this isn't very MSI related ...
arm already has KVM_IRQ_LINE/kvm_vm_ioctl_irq_line with
KVM_ARM_IRQ_TYPE_SPI that does
kvm_vgic_inject_irq(kvm, 0, irq_num, level)
Is that interface lacking?
Thanks.
> +}
> +
> +/**
> * kvm_set_msi: inject the MSI corresponding to the
> * MSI routing entry
> *
> @@ -96,7 +113,7 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
> msi.devid = e->msi_devid;
>
> if (!vgic_has_its(kvm))
> - return -ENODEV;
> + return vgic_v2m_inject_msi(kvm, &msi);
>
> return vgic_its_inject_msi(kvm, &msi);
> }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 6/7] KVM: arm/arm64: enable MSI routing
2016-07-18 13:25+, Eric Auger:
> Up to now, only irqchip routing entries could be set. This patch
> adds the capability to insert MSI routing entries.
>
> For ARM64, let's also increase KVM_MAX_IRQ_ROUTES to 4096: this
> include SPI irqchip routes plus MSI routes. In the future this
> might be extended.
>
> Signed-off-by: Eric Auger
> Reviewed-by: Andre Przywara
>
> ---
> v6 -> v7:
> - added Andre's R-b
>
> v2 -> v3:
> - remove any reference to KVM_IRQ_ROUTING_EXTENDED_MSI type
> - unconditionnaly uapi flags and devid downto the kernel
> routing entry struct
> - handle KVM_MSI_VALID_DEVID flag in kvm_set_irq_routing
> - note about KVM_CAP_MSI_DEVID moved in the first patch file
> of the series
>
> v1 -> v2:
> - adapt to new routing entry types
>
> RFC -> PATCH:
> - move api MSI routing updates into that patch file
> - use new devid field of user api struct
> ---
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> @@ -209,7 +209,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
> goto out;
>
> r = -EINVAL;
> - if (ue->flags) {
> + if (ue->flags & ~KVM_MSI_VALID_DEVID) {
This allows KVM_MSI_VALID_DEVID flag for all route types, which is not
what we want.
To avoid checking separately when setting MSI route in every arch,
I think we could have a switch (ue->type) here as well.
> kfree(e);
> goto out;
> }
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 2/7] KVM: kvm_host: add devid in kvm_kernel_irq_routing_entry
2016-07-18 13:25+, Eric Auger:
> Extend kvm_kernel_irq_routing_entry to transport the device id
> field, devid. A new flags field makes possible to indicate the
> devid is valid. Those additions are used for ARM GICv3 ITS MSI
> injection.
>
> Signed-off-by: Eric Auger
> Acked-by: Christoffer Dall
>
> ---
> v6 -> v7:
> - added msi_ prefix to flags and dev_id fields
>
> v4 -> v5:
> - add Christoffer's R-b
>
> v2 -> v3:
> - add flags
>
> v1 -> v2:
> - replace msi_msg field by a struct composed of msi_msg and devid
>
> RFC -> PATCH:
> - reword the commit message after change in first patch (uapi)
> ---
> include/linux/kvm_host.h | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index c87fe6f..3d2cbb4 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -317,7 +317,11 @@ struct kvm_kernel_irq_routing_entry {
> unsigned irqchip;
> unsigned pin;
> } irqchip;
> - struct msi_msg msi;
> + struct {
> + struct msi_msg msi;
> + u32 msi_flags;
> + u32 msi_devid;
I'd much rather see them as msi.flags and msi.devid.
I didn't notice a code that passes struct msi_msg anywhere, so using an
ad-hoc struct like irqchip or defining a new one would work fine.
Thanks.
> + };
> struct kvm_s390_adapter_int adapter;
> struct kvm_hv_sint hv_sint;
> };
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v7 1/7] KVM: api: pass the devid in the msi routing entry
2016-07-18 13:25+, Eric Auger:
> On ARM, the MSI msg (address and data) comes along with
> out-of-band device ID information. The device ID encodes the
> device that writes the MSI msg. Let's convey the device id in
> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
> kvm_irq_routing_entry to indicate the msi devid is populated.
>
> Signed-off-by: Eric Auger
> Reviewed-by: Andre Przywara
>
> ---
>
> v6 -> v7:
> - Added Andre's R-b
>
> v4 -> v5:
> - some rephrasing in api.txt according to Christoffer's comments
> v2 -> v3:
> - replace usage of KVM_IRQ_ROUTING_EXTENDED_MSI type by
> usage of KVM_MSI_VALID_DEVID flag
> - add note about KVM_CAP_MSI_DEVID capability
>
> v1 -> v2:
> - devid id passed in kvm_irq_routing_msi instead of in
> kvm_irq_routing_entry
>
> RFC -> PATCH
> - remove kvm_irq_routing_extended_msi and use union instead
> ---
> diff --git a/Documentation/virtual/kvm/api.txt
> b/Documentation/virtual/kvm/api.txt
> @@ -1479,9 +1483,20 @@ struct kvm_irq_routing_msi {
> __u32 address_lo;
> __u32 address_hi;
> __u32 data;
> - __u32 pad;
> + union {
> + __u32 pad;
> + __u32 devid;
> + };
> };
>
> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
> + for the device that wrote the MSI message.
> + For PCI, this is usually a BFD identifier in the lower 16 bits.
> +
> +The per-VM KVM_CAP_MSI_DEVID capability advertises the requirement to
> +provide the device ID. If this capability is not set, userland cannot
> +rely on the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
It would be better to enforce this mentioned dependency on set
KVM_CAP_MSI_DEVID, but is the dependency even required?
It seems we were checking flags for zero, so KVM_MSI_VALID_DEVID
couldn't have been set by old userspaces, therefor it is ok to only make
it depend only on the presence of KVM_CAP_MSI_DEVID, like the patch does
now. (I assume KVM_MSI_VALID_DEVID and KVM_CAP_MSI_DEVID are being
merged at the same time.)
Then there would be little point in having KVM_CAP_MSI_DEVID enableable,
so does enabling KVM_CAP_MSI_DEVID mean that every MSI must have a valid
devid?
Thanks.
---
I'm confused about the purpose behind two dynamic flags that seem to do
that same thing, but those are just nitpicks, the API looks good in
general.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: VHE: Context switch MDSCR_EL1
2016-07-21 14:24+0100, Marc Zyngier:
> On Thu, 21 Jul 2016 15:17:20 +0200
> Radim Krčmář wrote:
>
> Hi Radim,
>
>> 2016-07-19 13:56+0100, Marc Zyngier:
>> > The kprobe enablement work has uncovered that changes made by
>> > a guest to MDSCR_EL1 were propagated to the host when VHE was
>> > enabled, leading to unexpected exception being delivered.
>> >
>> > Moving this register to the list of registers that are always
>> > context-switched fixes the issue.
>> >
>> > Fixes: 9c6c35683286 ("arm64: KVM: VHE: Split save/restore of registers
>> > shared between guest and host")
>> > Cc: sta...@vger.kernel.org #4.6
>> > Reported-by: Tirumalesh Chalamarla
>> > Tested-by: Tirumalesh Chalamarla
>> > Signed-off-by: Marc Zyngier
>> > ---
>>
>> I would take this patch to 4.7 directly through the main tree.
>> Are your plans different?
>
> If you're happy to take it now, that works for me.
The fix is clear, tested, and would go to 4.7.stable. Better to have it
in 4.7, IMO.
>I can either send a
> PR right away, or you can apply it directly, whichever works best for
> you.
It is just one patch so I'll apply it and send a pull request to Linus
on Saturday; less cummulative work that way. :)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2] arm64: KVM: VHE: Context switch MDSCR_EL1
2016-07-19 13:56+0100, Marc Zyngier:
> The kprobe enablement work has uncovered that changes made by
> a guest to MDSCR_EL1 were propagated to the host when VHE was
> enabled, leading to unexpected exception being delivered.
>
> Moving this register to the list of registers that are always
> context-switched fixes the issue.
>
> Fixes: 9c6c35683286 ("arm64: KVM: VHE: Split save/restore of registers shared
> between guest and host")
> Cc: sta...@vger.kernel.org #4.6
> Reported-by: Tirumalesh Chalamarla
> Tested-by: Tirumalesh Chalamarla
> Signed-off-by: Marc Zyngier
> ---
I would take this patch to 4.7 directly through the main tree.
Are your plans different?
Thanks.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [RFC v6 4/6] KVM: arm/arm64: enable irqchip routing
2016-07-08 10:52+0200, Andrew Jones: > On Fri, Jul 08, 2016 at 10:16:53AM +0200, Auger Eric wrote: >> On 07/07/2016 19:20, Andrew Jones wrote: >> > On Wed, Jul 06, 2016 at 10:47:53AM +0200, Eric Auger wrote: >> >> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c >> >> @@ -29,7 +29,9 @@ >> >> #include >> >> #include >> >> #include >> >> +#if !defined(CONFIG_ARM) && !defined(CONFIG_ARM64) >> >> #include "irq.h" >> >> +#endif >> > >> > Instead of doing this, shouldn't we add arch/arm[64]/kvm/irq.h files. >> > Probably a simple one like ./arch/s390/kvm/irq.h ? >> >> Well I considered this solution in the past but I did not find much to >> put there (it was even void). typically irqchip_in_kernel is in >> include/kvm/arm_vgic.h since the macro can be shared between arm/arm64. > > I think I'd prefer a nearly empty file to the #ifdef's, but Paolo and > Radim should chime in. I concur, hiding ugliness in header files is what we strive for. The files could #include , which might make their existence easier to understand. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 0/8] KVM/ARM Fixes for v4.7-rc2
2016-06-02 12:20+0200, Christoffer Dall: > Hi Paolo and Radim, > > The following changes since commit 1a695a905c18548062509178b98bc91e67510864: > > Linux 4.7-rc1 (2016-05-29 09:29:24 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git > tags/kvm-arm-for-v4.7-rc2 > > for you to fetch changes up to 05fb05a6ca25e02ad8c31bc440b3c4996864f379: > > KVM: arm/arm64: vgic-new: Removel harmful BUG_ON (2016-06-02 11:52:21 +0200) > > They contain fixes for the vgic; two of the patches address a bug introduced > in > v4.6 while the rest are for the new vgic. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PULL 0/4] KVM/ARM Fixes for v4.6-rc4
2016-04-08 11:52+0200, Christoffer Dall: > Hi Paolo and Radim, > > Here's another set of fixes for KVM/ARM for this release. I guess this was > expected given the size of our patch queue for this merge window. Pulled, thanks. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
