Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Christian Borntraeger]

Am 09.10.2015 um 16:42 schrieb Paolo Bonzini:
> Christian, the question for you is towards the end...



[]
> 
>> --- a/virt/kvm/irqchip.c
>> +++ b/virt/kvm/irqchip.c
>> @@ -144,11 +144,13 @@ static int setup_routing_entry(struct 
>> kvm_irq_routing_table *rt,
>>  
>>  /*
>>   * Do not allow GSI to be mapped to the same irqchip more than once.
>> - * Allow only one to one mapping between GSI and MSI.
>> + * Allow only one to one mapping between GSI and MSI/Hyper-V SINT.
>>   */
>>  hlist_for_each_entry(ei, >map[ue->gsi], link)
>>  if (ei->type == KVM_IRQ_ROUTING_MSI ||
>>  ue->type == KVM_IRQ_ROUTING_MSI ||
>> +ei->type == KVM_IRQ_ROUTING_HV_SINT ||
>> +ue->type == KVM_IRQ_ROUTING_HV_SINT ||
>>  ue->u.irqchip.irqchip == ei->irqchip.irqchip)
>>  return r;
> 
> Christian, what's the desired behavior for s390 adapter interrupts here?
>  Should this actually become
> 
>   if (ei->type != KVM_IRQ_ROUTING_IRQCHIP ||
>   ue->type != KVM_IRQ_ROUTING_IRQCHIP ||
>   ue->u.irqchip.irqchip == ei->irqchip.irqchip)

Hmm, this is the failure path if we already have one routing entry, Right?
This will work with virtio ccw as we only setup one route, but I am not
sure about the upcoming PCI irqfd support which might add a 2nd adapter
route.

Adding Conny, Jens,Not sure about PC, 
As soon as we wire up the PCI irgfd, we want to register a 2nd route for
the same irqchip via flic, which will also be of type 
KVM_IRQ_ROUTING_S390_ADAPTER. Correct?


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Paolo Bonzini]

On 12/10/2015 10:48, Cornelia Huck wrote:
> Going back to Paolo's original question, I think changing the check
> to !KVM_IRQ_ROUTING_IRQCHIP makes sense, if I understand the code
> correctly. They seem to be the only special one.

Great.  Roman, Denis, can you do this then?

Thanks,

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Cornelia Huck]

On Mon, 12 Oct 2015 09:54:41 +0200
Christian Borntraeger  wrote:

> Am 09.10.2015 um 16:42 schrieb Paolo Bonzini:
> > Christian, the question for you is towards the end...
> 
> 
> 
> []
> > 
> >> --- a/virt/kvm/irqchip.c
> >> +++ b/virt/kvm/irqchip.c
> >> @@ -144,11 +144,13 @@ static int setup_routing_entry(struct 
> >> kvm_irq_routing_table *rt,
> >>  
> >>/*
> >> * Do not allow GSI to be mapped to the same irqchip more than once.
> >> -   * Allow only one to one mapping between GSI and MSI.
> >> +   * Allow only one to one mapping between GSI and MSI/Hyper-V SINT.
> >> */
> >>hlist_for_each_entry(ei, >map[ue->gsi], link)
> >>if (ei->type == KVM_IRQ_ROUTING_MSI ||
> >>ue->type == KVM_IRQ_ROUTING_MSI ||
> >> +  ei->type == KVM_IRQ_ROUTING_HV_SINT ||
> >> +  ue->type == KVM_IRQ_ROUTING_HV_SINT ||
> >>ue->u.irqchip.irqchip == ei->irqchip.irqchip)
> >>return r;
> > 
> > Christian, what's the desired behavior for s390 adapter interrupts here?
> >  Should this actually become
> > 
> > if (ei->type != KVM_IRQ_ROUTING_IRQCHIP ||
> > ue->type != KVM_IRQ_ROUTING_IRQCHIP ||
> > ue->u.irqchip.irqchip == ei->irqchip.irqchip)
> 
> Hmm, this is the failure path if we already have one routing entry, Right?
> This will work with virtio ccw as we only setup one route, but I am not
> sure about the upcoming PCI irqfd support which might add a 2nd adapter
> route.
> 
> Adding Conny, Jens,Not sure about PC, 
> As soon as we wire up the PCI irgfd, we want to register a 2nd route for
> the same irqchip via flic, which will also be of type 
> KVM_IRQ_ROUTING_S390_ADAPTER. Correct?

It's a bit different. The kernel basically does not see msi routes for
s390 pci at all, as qemu already transforms the msi route into an
adapter route before registering it (see kvm_arch_fixup_msi_route() in
qemu's target-s390x/kvm.c). So, in the end, all s390 kernels end up
using adapter routes, and none of them are duplicate (just one irqchip).

Going back to Paolo's original question, I think changing the check
to !KVM_IRQ_ROUTING_IRQCHIP makes sense, if I understand the code
correctly. They seem to be the only special one.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Roman Kagan]

On Fri, Oct 09, 2015 at 04:42:33PM +0200, Paolo Bonzini wrote:
> You need to add SYNIC vectors to the EOI exit bitmap, so that APICv
> (Xeon E5 or higher, Ivy Bridge or newer) is handled correctly.  You also
> need to check the auto EOI exit bitmap in __apic_accept_irq, and avoid
> going through kvm_x86_ops->deliver_posted_interrupt for auto EOI
> vectors.  Something like
> 
>   if (kvm_x86_ops->deliver_posted_interrupt &&
>   !test_bit(...))
> 
> in place of the existing "if (kvm_x86_ops->deliver_posted_interrupt)".

Indeed, missed that path, thanks!

> I really don't like this auto-EOI extension, but I guess that's the
> spec. :( If it wasn't for it, you could do everything very easily in
> userspace using Google's proposed MSR exit.

I guess you're right.  We'd probably have to (ab)use MSI for SINT
delivery, though.  Anyway the need to implement auto-EOI rules that out.

Thanks for the quick review, we'll try to address your comments in the
next round.

Roman.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Roman Kagan]

On Mon, Oct 12, 2015 at 10:58:36AM +0200, Paolo Bonzini wrote:
> On 12/10/2015 10:48, Cornelia Huck wrote:
> > Going back to Paolo's original question, I think changing the check
> > to !KVM_IRQ_ROUTING_IRQCHIP makes sense, if I understand the code
> > correctly. They seem to be the only special one.
> 
> Great.  Roman, Denis, can you do this then?

Sure, gonna be in the next round.

Thanks,
Roman.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Paolo Bonzini]

On 09/10/2015 17:53, Roman Kagan wrote:
> > I really don't like this auto-EOI extension, but I guess that's the
> > spec. :( If it wasn't for it, you could do everything very easily in
> > userspace using Google's proposed MSR exit.
> I guess you're right.  We'd probably have to (ab)use MSI for SINT
> delivery, though.

Not really an issue, as MSI on x86 is really just the external entry
point into the LAPIC, it makes sense that it be the external interface
into KVM's virtualized LAPIC.  Userspace split irqchip is (ab)using MSI
routes the same way.

> Anyway the need to implement auto-EOI rules that out.

Yup.  I look forward to reviewing v2!

Paolo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Denis V. Lunev]

From: Andrey Smetanin 

Synic is a lapic extension, which is controlled via MSRs and maintains
for each vCPU
 - 16 synthetic interrupt "lines" (SINT's); each can be configured to
   trigger a specific interrupt vector optionally with auto-EOI
   semantics
 - a message page in the guest memory with 16 256-byte per-SINT message
   slots
 - an event flag page in the guest memory with 16 2048-bit per-SINT
   event flag areas

The host triggers a SINT whenever it delivers a new message to the
corresponding slot or flips an event flag bit in the corresponding area.
The guest informs the host that it can try delivering a message by
explicitly asserting EOI in lapic or writing to End-Of-Message (EOM)
MSR.

The userspace (qemu) triggers interrupts and receives EOM notifications
via irqfd with resampler; for that, a GSI is allocated for each
configured SINT, and irq_routing api is extended to support GSI-SINT
mapping.

Signed-off-by: Andrey Smetanin 
Reviewed-by: Roman Kagan 
Signed-off-by: Denis V. Lunev 
CC: Vitaly Kuznetsov 
CC: "K. Y. Srinivasan" 
CC: Gleb Natapov 
CC: Paolo Bonzini 
---
 arch/powerpc/kvm/mpic.c |  18 +++
 arch/s390/kvm/interrupt.c   |  18 +++
 arch/x86/include/asm/kvm_host.h |  14 +++
 arch/x86/kvm/hyperv.c   | 266 
 arch/x86/kvm/hyperv.h   |  20 +++
 arch/x86/kvm/irq_comm.c |  16 +++
 arch/x86/kvm/lapic.c|  15 ++-
 arch/x86/kvm/lapic.h|   5 +
 arch/x86/kvm/x86.c  |   4 +
 drivers/hv/hyperv_vmbus.h   |   5 -
 include/linux/kvm_host.h|  12 ++
 include/uapi/linux/hyperv.h |  12 ++
 include/uapi/linux/kvm.h|   8 ++
 virt/kvm/arm/vgic.c |  18 +++
 virt/kvm/eventfd.c  |  35 +-
 virt/kvm/irqchip.c  |  24 +++-
 16 files changed, 475 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kvm/mpic.c b/arch/powerpc/kvm/mpic.c
index 6249cdc..01e7fb4 100644
--- a/arch/powerpc/kvm/mpic.c
+++ b/arch/powerpc/kvm/mpic.c
@@ -1850,3 +1850,21 @@ int kvm_set_routing_entry(struct 
kvm_kernel_irq_routing_entry *e,
 out:
return r;
 }
+
+/* Hyper-V Synic not implemented */
+int kvm_hv_set_sint(struct kvm_kernel_irq_routing_entry *e,
+   struct kvm *kvm, int irq_source_id, int level,
+   bool line_status)
+{
+   return -ENOTSUP;
+}
+
+int kvm_hv_get_sint_gsi(struct kvm_vcpu *vcpu, u32 sint)
+{
+   return -ENOTSUP;
+}
+
+int kvm_hv_set_sint_gsi(struct kvm *kvm, u32 vcpu_id, u32 sint, int gsi)
+{
+   return -ENOTSUP;
+}
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 5c2c169..7fa8d9d 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -2285,3 +2285,21 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 
__user *buf, int len)
 
return n;
 }
+
+/* Hyper-V Synic not implemented */
+int kvm_hv_set_sint(struct kvm_kernel_irq_routing_entry *e,
+   struct kvm *kvm, int irq_source_id, int level,
+   bool line_status)
+{
+   return -ENOTSUP;
+}
+
+int kvm_hv_get_sint_gsi(struct kvm_vcpu *vcpu, u32 sint)
+{
+   return -ENOTSUP;
+}
+
+int kvm_hv_set_sint_gsi(struct kvm *kvm, u32 vcpu_id, u32 sint, int gsi)
+{
+   return -ENOTSUP;
+}
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index cdbdb55..e614a543 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -374,10 +375,23 @@ struct kvm_mtrr {
struct list_head head;
 };
 
+/* Hyper-V synthetic interrupt controller */
+struct kvm_vcpu_hv_synic {
+   u64 version;
+   u64 control;
+   u64 msg_page;
+   u64 evt_page;
+   atomic64_t sint[HV_SYNIC_SINT_COUNT];
+   atomic_t sint_to_gsi[HV_SYNIC_SINT_COUNT];
+   DECLARE_BITMAP(auto_eoi_bitmap, 256);
+   DECLARE_BITMAP(vec_bitmap, 256);
+};
+
 /* Hyper-V per vcpu emulation context */
 struct kvm_vcpu_hv {
u64 hv_vapic;
s64 runtime_offset;
+   struct kvm_vcpu_hv_synic synic;
 };
 
 struct kvm_vcpu_arch {
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index 62cf8c9..15c3c02 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -23,13 +23,265 @@
 
 #include "x86.h"
 #include "lapic.h"
+#include "ioapic.h"
 #include "hyperv.h"
 
 #include 
+#include 
 #include 
 
 #include "trace.h"
 
+static inline u64 synic_read_sint(struct kvm_vcpu_hv_synic *synic, int sint)
+{
+   return atomic64_read(>sint[sint]);
+}
+
+static inline int synic_get_sint_vector(u64 sint_value)
+{
+   if (sint_value & HV_SYNIC_SINT_MASKED)
+   return -1;
+   return sint_value & HV_SYNIC_SINT_VECTOR_MASK;
+}
+
+static bool 

Re: [PATCH 1/2] kvm/x86: Hyper-V synthetic interrupt controller

[Paolo Bonzini]

Christian, the question for you is towards the end...

On 09/10/2015 15:39, Denis V. Lunev wrote:
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 62cf8c9..15c3c02 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -23,13 +23,265 @@
>  
>  #include "x86.h"
>  #include "lapic.h"
> +#include "ioapic.h"
>  #include "hyperv.h"
>  
>  #include 
> +#include 
>  #include 
>  
>  #include "trace.h"
>  
> +static inline u64 synic_read_sint(struct kvm_vcpu_hv_synic *synic, int sint)
> +{
> + return atomic64_read(>sint[sint]);
> +}
> +
> +static inline int synic_get_sint_vector(u64 sint_value)
> +{
> + if (sint_value & HV_SYNIC_SINT_MASKED)
> + return -1;
> + return sint_value & HV_SYNIC_SINT_VECTOR_MASK;
> +}
> +
> +static bool synic_has_active_vector(struct kvm_vcpu_hv_synic *synic,
> + int vector, int sint_to_skip, int sint_mask)
> +{
> + u64 sint_value;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(synic->sint); i++) {
> + if (i == sint_to_skip)
> + continue;
> + sint_value = synic_read_sint(synic, i);
> + if ((synic_get_sint_vector(sint_value) == vector) &&
> + ((sint_mask == 0) || (sint_value & sint_mask)))

Coding style, no parentheses around && or ||:

if (synic_get_sint_vector(sint_value) == vector &&
(sint_mask == 0 || sint_value & sint_mask)


> + return true;
> + }
> + return false;
> +}
> +
> +static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint, u64 
> data)
> +{
> + int vector;
> +
> + vector = data & HV_SYNIC_SINT_VECTOR_MASK;
> + if (vector < 16)
> + return 1;
> + /*
> +  * Guest may configure multiple SINTs to use the same vector, so
> +  * we maintain a bitmap of vectors handled by synic, and a
> +  * bitmap of vectors with auto-eoi behavoir.  The bitmaps are

Typo (behavior).

> +  * updated here, and atomically queried on fast paths.
> +  */
> +
> + if (!(data & HV_SYNIC_SINT_MASKED)) {
> + __set_bit(vector, synic->vec_bitmap);
> + if (data & HV_SYNIC_SINT_AUTO_EOI)
> + __set_bit(vector, synic->auto_eoi_bitmap);
> + } else {
> + if (!synic_has_active_vector(synic, vector, sint, 0))
> + __clear_bit(vector, synic->vec_bitmap);
> + if (!synic_has_active_vector(synic, vector, sint,
> +  HV_SYNIC_SINT_AUTO_EOI))
> + __clear_bit(vector, synic->auto_eoi_bitmap);

I think you could do the clears after the atomic64_set?  Then you do not
need anymore the third argument to synic_has_active_vector.  Actually I
think it's simpler if you just make two functions,
synic_is_vector_connected and synic_is_vector_auto_eoi.  There is some
code duplication, but the functions are trivial.

> @@ -123,6 +125,15 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>   return kvm_irq_delivery_to_apic(kvm, NULL, , NULL);
>  }
>  
> +int kvm_hv_set_sint(struct kvm_kernel_irq_routing_entry *e,
> + struct kvm *kvm, int irq_source_id, int level,
> + bool line_status)
> +{
> + if (!level)
> + return -1;
> +
> + return kvm_hv_synic_set_irq(kvm, e->hv_sint.vcpu, e->hv_sint.sint);
> +}
>  
>  static int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *e,
>struct kvm *kvm)
> @@ -289,6 +300,11 @@ int kvm_set_routing_entry(struct 
> kvm_kernel_irq_routing_entry *e,
>   e->msi.address_hi = ue->u.msi.address_hi;
>   e->msi.data = ue->u.msi.data;
>   break;
> + case KVM_IRQ_ROUTING_HV_SINT:
> + e->set = kvm_hv_set_sint;
> + e->hv_sint.vcpu = ue->u.hv_sint.vcpu;
> + e->hv_sint.sint = ue->u.hv_sint.sint;
> + break;
>   default:
>   goto out;
>   }
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 944b38a..63edbec 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -41,6 +41,7 @@
>  #include "trace.h"
>  #include "x86.h"
>  #include "cpuid.h"
> +#include "hyperv.h"
>  
>  #ifndef CONFIG_X86_64
>  #define mod_64(x, y) ((x) - (y) * div64_u64(x, y))
> @@ -128,11 +129,6 @@ static inline int apic_enabled(struct kvm_lapic *apic)
>   (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \
>APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER)
>  
> -static inline int kvm_apic_id(struct kvm_lapic *apic)
> -{
> - return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff;
> -}
> -
>  /* The logical map is definitely wrong if we have multiple
>   * modes at the same time.  (Physical map is always right.)
>   */
> @@ -972,6 +968,9 @@ static int apic_set_eoi(struct kvm_lapic *apic)
>   apic_clear_isr(vector, apic);
>   apic_update_ppr(apic);
>  
> +