| Message ID | 1448464821-8199-7-git-send-email-asmetanin@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 25/11/2015 16:20, Andrey Smetanin wrote: > +static void synic_clear_sint_msg_pending(struct kvm_vcpu_hv_synic *synic, > + u32 sint) > +{ > + struct kvm_vcpu *vcpu = synic_to_vcpu(synic); > + struct page *page; > + gpa_t gpa; > + struct hv_message *msg; > + struct hv_message_page *msg_page; > + > + gpa = synic->msg_page & PAGE_MASK; > + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); > + if (is_error_page(page)) { > + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n", > + gpa); > + return; > + } > + msg_page = kmap_atomic(page); But the message page is not being pinned, is it? Paolo > + msg = &msg_page->sint_message[sint]; > + msg->header.message_flags.msg_pending = 0; > + > + kunmap_atomic(msg_page); > + kvm_release_page_dirty(page); > + kvm_vcpu_mark_page_dirty(vcpu, gpa >> PAGE_SHIFT); > +} > +
On 11/25/2015 07:52 PM, Paolo Bonzini wrote: > > > On 25/11/2015 16:20, Andrey Smetanin wrote: >> +static void synic_clear_sint_msg_pending(struct kvm_vcpu_hv_synic *synic, >> + u32 sint) >> +{ >> + struct kvm_vcpu *vcpu = synic_to_vcpu(synic); >> + struct page *page; >> + gpa_t gpa; >> + struct hv_message *msg; >> + struct hv_message_page *msg_page; >> + >> + gpa = synic->msg_page & PAGE_MASK; >> + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); >> + if (is_error_page(page)) { >> + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n", >> + gpa); >> + return; >> + } >> + msg_page = kmap_atomic(page); > > But the message page is not being pinned, is it? > Actually I don't know anything about pinning. Is it pinning against page swapping ? Could you please clarify and provide an API to use in this case ? > Paolo > >> + msg = &msg_page->sint_message[sint]; >> + msg->header.message_flags.msg_pending = 0; >> + >> + kunmap_atomic(msg_page); >> + kvm_release_page_dirty(page); >> + kvm_vcpu_mark_page_dirty(vcpu, gpa >> PAGE_SHIFT); >> +} >> +
On 25/11/2015 17:55, Andrey Smetanin wrote: >> >> + gpa = synic->msg_page & PAGE_MASK; >> + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); >> + if (is_error_page(page)) { >> + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n", >> + gpa); >> + return; >> + } >> + msg_page = kmap_atomic(page); > > But the message page is not being pinned, is it? > > Actually I don't know anything about pinning. > Is it pinning against page swapping ? Yes. Unless the page is pinned, kmap_atomic can fail. However, I don't think that kvm_hv_notify_acked_sint is called from atomic context. It is only called from apic_set_eoi. Could you just use kvm_vcpu_write_guest_page? By the way, do you need to do this also in kvm_get_apic_interrupt, for auto EOI interrupts? Thanks, Paolo > Could you please clarify and provide an API to use in this case ?
On 11/25/2015 08:14 PM, Paolo Bonzini wrote: > > > On 25/11/2015 17:55, Andrey Smetanin wrote: >>> >>> + gpa = synic->msg_page & PAGE_MASK; >>> + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); >>> + if (is_error_page(page)) { >>> + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n", >>> + gpa); >>> + return; >>> + } >>> + msg_page = kmap_atomic(page); >> >> But the message page is not being pinned, is it? >> >> Actually I don't know anything about pinning. >> Is it pinning against page swapping ? > > Yes. Unless the page is pinned, kmap_atomic can fail. kmap_atomic() can't fail for a valid page struct. Does kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct which may pass is_error_page(page) check but can leads to incorrect behavior inside kmap_atomic()? > > However, I don't think that kvm_hv_notify_acked_sint is called from > atomic context. It is only called from apic_set_eoi. Could you just > use kvm_vcpu_write_guest_page? In this case I can use kvm_vcpu_write_guest_page(), but in the 'PATCH v1 7/7' I do the same page mapping method to sync_cmpxchg() at guest message page address to exclusively acquire message page slot(see synic_deliver_msg()). So we need some method to map and access atomically memory of guest page in KVM. Does any method to pin and map guest page in kernel exists? Or should we use mlock() for this page in QEMU part ? > > By the way, do you need to do this also in kvm_get_apic_interrupt, for > auto EOI interrupts? No we don't need this because in case of auto EOI interrupts, if ->msg_pending was set, host will receive HV_X64_MSR_EOM write request which calls kvm_hv_notify_acked_sint(). > > Thanks, > > Paolo > >> Could you please clarify and provide an API to use in this case ?
On 26/11/2015 10:06, Andrey Smetanin wrote: > > > On 11/25/2015 08:14 PM, Paolo Bonzini wrote: >> >> >> On 25/11/2015 17:55, Andrey Smetanin wrote: >>>> >>>> + gpa = synic->msg_page & PAGE_MASK; >>>> + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); >>>> + if (is_error_page(page)) { >>>> + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa >>>> 0x%llx\n", >>>> + gpa); >>>> + return; >>>> + } >>>> + msg_page = kmap_atomic(page); >>> >>> But the message page is not being pinned, is it? >>> >>> Actually I don't know anything about pinning. >>> Is it pinning against page swapping ? >> >> Yes. Unless the page is pinned, kmap_atomic can fail. > kmap_atomic() can't fail for a valid page struct. Does > kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct > which may pass is_error_page(page) check but can leads to incorrect > behavior inside kmap_atomic()? No, you're right. Nevermind, I was confused because I thought you needed kmap_atomic rather than kmap. Here using kmap_atomic is just an optimization, so it's okay. (If you needed kmap_atomic, the problem would have been that kvm_vcpu_gfn_to_page() can sleep). In patch 7/7 you're also not in atomic context, so kvm_vcpu_gfn_to_page is okay. Shouldn't have reviewed the patch when tired. :) Then the patches look good, I think. With a testcase I can try them out and hopefully merge them for Linux 4.5 / QEMU 2.6. Paolo
On 11/26/2015 05:43 PM, Paolo Bonzini wrote: > > > On 26/11/2015 10:06, Andrey Smetanin wrote: >> >> >> On 11/25/2015 08:14 PM, Paolo Bonzini wrote: >>> >>> >>> On 25/11/2015 17:55, Andrey Smetanin wrote: >>>>> >>>>> + gpa = synic->msg_page & PAGE_MASK; >>>>> + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); >>>>> + if (is_error_page(page)) { >>>>> + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa >>>>> 0x%llx\n", >>>>> + gpa); >>>>> + return; >>>>> + } >>>>> + msg_page = kmap_atomic(page); >>>> >>>> But the message page is not being pinned, is it? >>>> >>>> Actually I don't know anything about pinning. >>>> Is it pinning against page swapping ? >>> >>> Yes. Unless the page is pinned, kmap_atomic can fail. >> kmap_atomic() can't fail for a valid page struct. Does >> kvm_vcpu_gfn_to_page() can provide invalid page(swapped page) struct >> which may pass is_error_page(page) check but can leads to incorrect >> behavior inside kmap_atomic()? > > No, you're right. Nevermind, I was confused because I thought you > needed kmap_atomic rather than kmap. Here using kmap_atomic is just an > optimization, so it's okay. (If you needed kmap_atomic, the problem > would have been that kvm_vcpu_gfn_to_page() can sleep). > > In patch 7/7 you're also not in atomic context, so kvm_vcpu_gfn_to_page > is okay. > > Shouldn't have reviewed the patch when tired. :) > > Then the patches look good, I think. With a testcase I can try them out > and hopefully merge them for Linux 4.5 / QEMU 2.6. Thank you! We already have a working Hyper-V SynIC timers kvm-unit-tests test case. We are going to send appropriate patches seria into kvm-unit-tests git. But kvm-unit-tests master now broken: > make objcopy -O elf32-i386 x86/memory.elf x86/memory.flat make: *** No rule to make target 'x86/pku.o', needed by 'x86/pku.elf'. Stop. The problem is in latest commit 3da70799dd3cf1169c4668b4a3fd6f598528b8b9. The commit adds 'pku' test case building, but not added any pku.c implementation file. [root@asm-pc kvm-unit-tests]# ls -al x86/pku.c ls: cannot access x86/pku.c: No such file or directory Could you please fix it ? > > Paolo >
On 26/11/2015 16:53, Andrey Smetanin wrote: >> Then the patches look good, I think. With a testcase I can try them out >> and hopefully merge them for Linux 4.5 / QEMU 2.6. > > Thank you! > > We already have a working Hyper-V SynIC timers kvm-unit-tests test case. > We are going to send appropriate patches seria into kvm-unit-tests git. > > But kvm-unit-tests master now broken: >> make > objcopy -O elf32-i386 x86/memory.elf x86/memory.flat > make: *** No rule to make target 'x86/pku.o', needed by 'x86/pku.elf'. > Stop. > > The problem is in latest commit 3da70799dd3cf1169c4668b4a3fd6f598528b8b9. > > The commit adds 'pku' test case building, but not added any pku.c > implementation file. > > [root@asm-pc kvm-unit-tests]# ls -al x86/pku.c > ls: cannot access x86/pku.c: No such file or directory > > > Could you please fix it ? Done, sorry. Paolo
On Wed, Nov 25, 2015 at 06:20:20PM +0300, Andrey Smetanin wrote: > The SynIC message protocol mandates that the message slot is claimed > by atomically setting message type to something other than HVMSG_NONE. > If another message is to be delivered while the slot is still busy, > message pending flag is asserted to indicate to the guest that the > hypervisor wants to be notified when the slot is released. > > To make sure the protocol works regardless of where the message > sources are (kernel or userspace), clear the pending flag on SINT ACK > notification, and let the message sources compete for the slot again. > > Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com> > CC: Gleb Natapov <gleb@kernel.org> > CC: Paolo Bonzini <pbonzini@redhat.com> > CC: "K. Y. Srinivasan" <kys@microsoft.com> > CC: Haiyang Zhang <haiyangz@microsoft.com> > CC: Vitaly Kuznetsov <vkuznets@redhat.com> > CC: Roman Kagan <rkagan@virtuozzo.com> > CC: Denis V. Lunev <den@openvz.org> > CC: qemu-devel@nongnu.org Reviewed-by: Roman Kagan <rkagan@virtuozzo.com> Roman.
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index 9958926..6412b6b 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -27,6 +27,7 @@ #include "hyperv.h" #include <linux/kvm_host.h> +#include <linux/highmem.h> #include <asm/apicdef.h> #include <trace/events/kvm.h> @@ -116,13 +117,43 @@ static struct kvm_vcpu_hv_synic *synic_get(struct kvm *kvm, u32 vcpu_id) return (synic->active) ? synic : NULL; } +static void synic_clear_sint_msg_pending(struct kvm_vcpu_hv_synic *synic, + u32 sint) +{ + struct kvm_vcpu *vcpu = synic_to_vcpu(synic); + struct page *page; + gpa_t gpa; + struct hv_message *msg; + struct hv_message_page *msg_page; + + gpa = synic->msg_page & PAGE_MASK; + page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT); + if (is_error_page(page)) { + vcpu_err(vcpu, "Hyper-V SynIC can't get msg page, gpa 0x%llx\n", + gpa); + return; + } + msg_page = kmap_atomic(page); + + msg = &msg_page->sint_message[sint]; + msg->header.message_flags.msg_pending = 0; + + kunmap_atomic(msg_page); + kvm_release_page_dirty(page); + kvm_vcpu_mark_page_dirty(vcpu, gpa >> PAGE_SHIFT); +} + static void kvm_hv_notify_acked_sint(struct kvm_vcpu *vcpu, u32 sint) { struct kvm *kvm = vcpu->kvm; + struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu); int gsi, idx; vcpu_debug(vcpu, "Hyper-V SynIC acked sint %d\n", sint); + if (synic->msg_page & HV_SYNIC_SIMP_ENABLE) + synic_clear_sint_msg_pending(synic, sint); + idx = srcu_read_lock(&kvm->irq_srcu); gsi = atomic_read(&vcpu_to_synic(vcpu)->sint_to_gsi[sint]); if (gsi != -1) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 2911919..9b64c8c 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -450,6 +450,8 @@ struct kvm { #define vcpu_debug(vcpu, fmt, ...) \ kvm_debug("vcpu%i " fmt, (vcpu)->vcpu_id, ## __VA_ARGS__) +#define vcpu_err(vcpu, fmt, ...) \ + kvm_err("vcpu%i " fmt, (vcpu)->vcpu_id, ## __VA_ARGS__) static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i) {
The SynIC message protocol mandates that the message slot is claimed by atomically setting message type to something other than HVMSG_NONE. If another message is to be delivered while the slot is still busy, message pending flag is asserted to indicate to the guest that the hypervisor wants to be notified when the slot is released. To make sure the protocol works regardless of where the message sources are (kernel or userspace), clear the pending flag on SINT ACK notification, and let the message sources compete for the slot again. Signed-off-by: Andrey Smetanin <asmetanin@virtuozzo.com> CC: Gleb Natapov <gleb@kernel.org> CC: Paolo Bonzini <pbonzini@redhat.com> CC: "K. Y. Srinivasan" <kys@microsoft.com> CC: Haiyang Zhang <haiyangz@microsoft.com> CC: Vitaly Kuznetsov <vkuznets@redhat.com> CC: Roman Kagan <rkagan@virtuozzo.com> CC: Denis V. Lunev <den@openvz.org> CC: qemu-devel@nongnu.org --- arch/x86/kvm/hyperv.c | 31 +++++++++++++++++++++++++++++++ include/linux/kvm_host.h | 2 ++ 2 files changed, 33 insertions(+)