Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 10:40:14PM +0300, Michael S. Tsirkin wrote: > On Tue, Apr 10, 2012 at 10:33:54PM +0300, Gleb Natapov wrote: > > > We don't try to match what HV does 100% anyway. > > > > > We should. The same code will be used for HV. > > Only where it makes sense, that is where the functionality > is sufficiently similar. > You can sprinkle additional ifs in the code, but I do not see the point. > > > > We have to notify IOAPIC about EOI ASAP. It > > > > may hold another interrupt for us that has to be delivered. > > > > > > You mean the ack notifiers? We can skip just for the vectors > > > which have ack notifiers or only if there are no notifiers. > > > > > No. I mean: > > > > if (!ent->fields.mask && (ioapic->irr & (1 << i))) > > ioapic_service(ioapic, i); > > Hmm. -- Gleb. -- 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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 10:33:54PM +0300, Gleb Natapov wrote: > > We don't try to match what HV does 100% anyway. > > > We should. The same code will be used for HV. Only where it makes sense, that is where the functionality is sufficiently similar. > > > We have to notify IOAPIC about EOI ASAP. It > > > may hold another interrupt for us that has to be delivered. > > > > You mean the ack notifiers? We can skip just for the vectors > > which have ack notifiers or only if there are no notifiers. > > > No. I mean: > > if (!ent->fields.mask && (ioapic->irr & (1 << i))) > ioapic_service(ioapic, i); Hmm. -- 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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 10:30:04PM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 08:59:21PM +0300, Gleb Natapov wrote:
> > Heh, I was working on that too.
> >
> > On Tue, Apr 10, 2012 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 10, 2012 at 05:03:22PM +0300, Avi Kivity wrote:
> > > > On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> > > > > I took a stub at implementing PV EOI using shared memory.
> > > > > This should reduce the number of exits an interrupt
> > > > > causes as much as by half.
> > > > >
> > > > > A partially complete draft for both host and guest parts
> > > > > is below.
> > > > >
> > > > > The idea is simple: there's a bit, per APIC, in guest memory,
> > > > > that tells the guest that it does not need EOI.
> > > > > We set it before injecting an interrupt and clear
> > > > > before injecting a nested one. Guest tests it using
> > > > > a test and clear operation - this is necessary
> > > > > so that host can detect interrupt nesting -
> > > > > and if set, it can skip the EOI MSR.
> > > > >
> > > > > There's a new MSR to set the address of said register
> > > > > in guest memory. Otherwise not much changes:
> > > > > - Guest EOI is not required
> > > > > - ISR is automatically cleared before injection
> > > > >
> > > > > Some things are incomplete: add feature negotiation
> > > > > options, qemu support for said options.
> > > > > No testing was done beyond compiling the kernel.
> > > > >
> > > > > I would appreciate early feedback.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin
> > > > >
> > > > > --
> > > > >
> > > > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > > > > index d854101..8430f41 100644
> > > > > --- a/arch/x86/include/asm/apic.h
> > > > > +++ b/arch/x86/include/asm/apic.h
> > > > > @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void)
> > > > > { return 0; }
> > > > >
> > > > > #endif /* CONFIG_X86_LOCAL_APIC */
> > > > >
> > > > > +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> > > > > +
> > > > > static inline void ack_APIC_irq(void)
> > > > > {
> > > > > + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> > > > > + return;
> > > > > +
> > > >
> > > > While __test_and_clear_bit() is implemented in a single instruction,
> > > > it's not required to be. Better have the instruction there explicitly.
> > > >
> > > > > /*
> > > > >* ack_APIC_irq() actually gets compiled as a single instruction
> > > > >* ... yummie.
> > > > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > > > b/arch/x86/include/asm/kvm_host.h
> > > > > index e216ba0..0ee1472 100644
> > > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > > @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> > > > > u64 length;
> > > > > u64 status;
> > > > > } osvw;
> > > > > +
> > > > > + struct {
> > > > > + u64 msr_val;
> > > > > + struct gfn_to_hva_cache data;
> > > > > + int vector;
> > > > > + } eoi;
> > > > > };
> > > >
> > > > Needs to be cleared on INIT.
> > >
> > > You mean kvm_arch_vcpu_reset?
> > >
> > > > >
> > > > >
> > > > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > > > smp_processor_id());
> > > > > }
> > > > >
> > > > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > > > +MSR_KVM_EOI_ENABLED);
> > > > > +
> > > >
> > > > Clear on kexec.
> > >
> > > With register_reboot_notifier?
> > >
> > > > > if (has_steal_clock)
> > > > > kvm_register_steal_time();
> > > > > }
> > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > index 8584322..9e38e12 100644
> > > > > --- a/arch/x86/kvm/lapic.c
> > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu,
> > > > > struct kvm_lapic_irq *irq)
> > > > > irq->level, irq->trig_mode);
> > > > > }
> > > > >
> > > > > -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > > > +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> > > > > +{
> > > > > +
> > > > > + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data,
> > > > > &val,
> > > > > + sizeof(val));
> > > > > +}
> > > > > +
> > > > > +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> > > > > +{
> > > > > +
> > > > > + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data,
> > > > > val,
> > > > > + sizeof(*val));
> > > > > +}
> > > > > +
> > > > > +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> > > > > +}
> > > > > +
> > > > > +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> > > > > +{
> > > > > + u32 val;
> > > > >
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 08:59:21PM +0300, Gleb Natapov wrote:
> Heh, I was working on that too.
>
> On Tue, Apr 10, 2012 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 05:03:22PM +0300, Avi Kivity wrote:
> > > On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> > > > I took a stub at implementing PV EOI using shared memory.
> > > > This should reduce the number of exits an interrupt
> > > > causes as much as by half.
> > > >
> > > > A partially complete draft for both host and guest parts
> > > > is below.
> > > >
> > > > The idea is simple: there's a bit, per APIC, in guest memory,
> > > > that tells the guest that it does not need EOI.
> > > > We set it before injecting an interrupt and clear
> > > > before injecting a nested one. Guest tests it using
> > > > a test and clear operation - this is necessary
> > > > so that host can detect interrupt nesting -
> > > > and if set, it can skip the EOI MSR.
> > > >
> > > > There's a new MSR to set the address of said register
> > > > in guest memory. Otherwise not much changes:
> > > > - Guest EOI is not required
> > > > - ISR is automatically cleared before injection
> > > >
> > > > Some things are incomplete: add feature negotiation
> > > > options, qemu support for said options.
> > > > No testing was done beyond compiling the kernel.
> > > >
> > > > I would appreciate early feedback.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin
> > > >
> > > > --
> > > >
> > > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > > > index d854101..8430f41 100644
> > > > --- a/arch/x86/include/asm/apic.h
> > > > +++ b/arch/x86/include/asm/apic.h
> > > > @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) {
> > > > return 0; }
> > > >
> > > > #endif /* CONFIG_X86_LOCAL_APIC */
> > > >
> > > > +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> > > > +
> > > > static inline void ack_APIC_irq(void)
> > > > {
> > > > + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> > > > + return;
> > > > +
> > >
> > > While __test_and_clear_bit() is implemented in a single instruction,
> > > it's not required to be. Better have the instruction there explicitly.
> > >
> > > > /*
> > > > * ack_APIC_irq() actually gets compiled as a single instruction
> > > > * ... yummie.
> > > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > > b/arch/x86/include/asm/kvm_host.h
> > > > index e216ba0..0ee1472 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> > > > u64 length;
> > > > u64 status;
> > > > } osvw;
> > > > +
> > > > + struct {
> > > > + u64 msr_val;
> > > > + struct gfn_to_hva_cache data;
> > > > + int vector;
> > > > + } eoi;
> > > > };
> > >
> > > Needs to be cleared on INIT.
> >
> > You mean kvm_arch_vcpu_reset?
> >
> > > >
> > > >
> > > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > >smp_processor_id());
> > > > }
> > > >
> > > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > > + MSR_KVM_EOI_ENABLED);
> > > > +
> > >
> > > Clear on kexec.
> >
> > With register_reboot_notifier?
> >
> > > > if (has_steal_clock)
> > > > kvm_register_steal_time();
> > > > }
> > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > index 8584322..9e38e12 100644
> > > > --- a/arch/x86/kvm/lapic.c
> > > > +++ b/arch/x86/kvm/lapic.c
> > > > @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct
> > > > kvm_lapic_irq *irq)
> > > > irq->level, irq->trig_mode);
> > > > }
> > > >
> > > > -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > > +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> > > > +{
> > > > +
> > > > + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data,
> > > > &val,
> > > > + sizeof(val));
> > > > +}
> > > > +
> > > > +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> > > > +{
> > > > +
> > > > + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data,
> > > > val,
> > > > + sizeof(*val));
> > > > +}
> > > > +
> > > > +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> > > > +}
> > > > +
> > > > +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> > > > +{
> > > > + u32 val;
> > > > + if (eoi_get_user(vcpu, &val) < 0)
> > > > + apic_debug("Can't read EOI MSR value: 0x%llx\n",
> > > > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > > > + if (!(val & 0x1))
> > > > + vcpu->arch.eoi.vector = -1;
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
Heh, I was working on that too.
On Tue, Apr 10, 2012 at 05:26:18PM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 05:03:22PM +0300, Avi Kivity wrote:
> > On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> > > I took a stub at implementing PV EOI using shared memory.
> > > This should reduce the number of exits an interrupt
> > > causes as much as by half.
> > >
> > > A partially complete draft for both host and guest parts
> > > is below.
> > >
> > > The idea is simple: there's a bit, per APIC, in guest memory,
> > > that tells the guest that it does not need EOI.
> > > We set it before injecting an interrupt and clear
> > > before injecting a nested one. Guest tests it using
> > > a test and clear operation - this is necessary
> > > so that host can detect interrupt nesting -
> > > and if set, it can skip the EOI MSR.
> > >
> > > There's a new MSR to set the address of said register
> > > in guest memory. Otherwise not much changes:
> > > - Guest EOI is not required
> > > - ISR is automatically cleared before injection
> > >
> > > Some things are incomplete: add feature negotiation
> > > options, qemu support for said options.
> > > No testing was done beyond compiling the kernel.
> > >
> > > I would appreciate early feedback.
> > >
> > > Signed-off-by: Michael S. Tsirkin
> > >
> > > --
> > >
> > > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > > index d854101..8430f41 100644
> > > --- a/arch/x86/include/asm/apic.h
> > > +++ b/arch/x86/include/asm/apic.h
> > > @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) {
> > > return 0; }
> > >
> > > #endif /* CONFIG_X86_LOCAL_APIC */
> > >
> > > +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> > > +
> > > static inline void ack_APIC_irq(void)
> > > {
> > > + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> > > + return;
> > > +
> >
> > While __test_and_clear_bit() is implemented in a single instruction,
> > it's not required to be. Better have the instruction there explicitly.
> >
> > > /*
> > >* ack_APIC_irq() actually gets compiled as a single instruction
> > >* ... yummie.
> > > diff --git a/arch/x86/include/asm/kvm_host.h
> > > b/arch/x86/include/asm/kvm_host.h
> > > index e216ba0..0ee1472 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> > > u64 length;
> > > u64 status;
> > > } osvw;
> > > +
> > > + struct {
> > > + u64 msr_val;
> > > + struct gfn_to_hva_cache data;
> > > + int vector;
> > > + } eoi;
> > > };
> >
> > Needs to be cleared on INIT.
>
> You mean kvm_arch_vcpu_reset?
>
> > >
> > >
> > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > smp_processor_id());
> > > }
> > >
> > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > +MSR_KVM_EOI_ENABLED);
> > > +
> >
> > Clear on kexec.
>
> With register_reboot_notifier?
>
> > > if (has_steal_clock)
> > > kvm_register_steal_time();
> > > }
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 8584322..9e38e12 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct
> > > kvm_lapic_irq *irq)
> > > irq->level, irq->trig_mode);
> > > }
> > >
> > > -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > > +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> > > +{
> > > +
> > > + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
> > > + sizeof(val));
> > > +}
> > > +
> > > +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> > > +{
> > > +
> > > + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
> > > + sizeof(*val));
> > > +}
> > > +
> > > +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> > > +{
> > > + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> > > +}
> > > +
> > > +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> > > +{
> > > + u32 val;
> > > + if (eoi_get_user(vcpu, &val) < 0)
> > > + apic_debug("Can't read EOI MSR value: 0x%llx\n",
> > > +(unsigned long long)vcpi->arch.eoi.msr_val);
> > > + if (!(val & 0x1))
> > > + vcpu->arch.eoi.vector = -1;
> > > + return vcpu->arch.eoi.vector;
> > > +}
> > > +
> > > +static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
> > > +{
> > > + BUG_ON(vcpu->arch.eoi.vector != -1);
> > > + if (eoi_put_user(vcpu, 0x1) < 0) {
> > > + apic_debug("Can't set EOI MSR value: 0x%llx\n",
> > > +(unsigned long long)vcpi->arch.eoi.msr_val);
> > > + return;
> > > + }
> > > + vcpu->arch.eoi.vector = vector;
> > > +}
> > > +
> > > +static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
> > > +{
>
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 07:08:26PM +0300, Avi Kivity wrote: > On 04/10/2012 06:14 PM, Michael S. Tsirkin wrote: > > On Tue, Apr 10, 2012 at 06:00:51PM +0300, Avi Kivity wrote: > > > On 04/10/2012 05:53 PM, Michael S. Tsirkin wrote: > > > > > > > > > > > > Yes. But we can and it's easier than figuring out priorities. > > > > > > I am guessing such collisions are rare, right? > > > > > > > > > > It's pretty easy, if there is something in IRR but > > > > > kvm_lapic_has_interrupt() returns -1, then we need to disable eoi > > > > > avoidance. > > > > > > > > I only see kvm_apic_has_interrupt - is this what you mean? > > > > > > Yes, sorry. > > > > > > It's not clear whether to do the check in kvm_apic_has_interrupt() or > > > kvm_apic_get_interrupt() - the latter is called only after interrupts > > > are enabled, so it looks like a better place (EOIs while interrupts are > > > disabled have no effect). But need to make sure those functions are > > > actually called, since they're protected by KVM_REQ_EVENT. > > > > Sorry not sure what you mean by "make sure" - read the code carefully? > > Yes. And I mean, get called at the right time. OK, Review will help here. > > > > > > Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c > > > > Hmm. Disabling for level handles the ack notifiers > > issue as well, which I forgot about. > > It's a tough call. You think looking at > > TMR in kvm_get_apic_interrupt is safe? > > Yes, it's read only from the guest point of view IIRC. > > > > > > > > > > Why do we care about > > > > > level-triggered interrupts? Everything uses MSI or edge-triggered > > > > > IOAPIC interrupts these days. > > > > > > > > Well lots of emulated devices don't yet. > > > > They probably should but it's nice to be able to > > > > test with e.g. e1000 emulation not just virtio. > > > > > > > > > e1000 doesn't support msi? > > > > qemu emulation doesn't. > > > > Can be changed if someone's really interested. But really, avoiding > EOIs for e1000 won't help it much. It will help test EOI avoidance. > -- > error compiling committee.c: too many arguments to function -- 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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On 04/10/2012 06:14 PM, Michael S. Tsirkin wrote: > On Tue, Apr 10, 2012 at 06:00:51PM +0300, Avi Kivity wrote: > > On 04/10/2012 05:53 PM, Michael S. Tsirkin wrote: > > > > > > > > > > Yes. But we can and it's easier than figuring out priorities. > > > > > I am guessing such collisions are rare, right? > > > > > > > > It's pretty easy, if there is something in IRR but > > > > kvm_lapic_has_interrupt() returns -1, then we need to disable eoi > > > > avoidance. > > > > > > I only see kvm_apic_has_interrupt - is this what you mean? > > > > Yes, sorry. > > > > It's not clear whether to do the check in kvm_apic_has_interrupt() or > > kvm_apic_get_interrupt() - the latter is called only after interrupts > > are enabled, so it looks like a better place (EOIs while interrupts are > > disabled have no effect). But need to make sure those functions are > > actually called, since they're protected by KVM_REQ_EVENT. > > Sorry not sure what you mean by "make sure" - read the code carefully? Yes. And I mean, get called at the right time. > > > > Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c > > Hmm. Disabling for level handles the ack notifiers > issue as well, which I forgot about. > It's a tough call. You think looking at > TMR in kvm_get_apic_interrupt is safe? Yes, it's read only from the guest point of view IIRC. > > > > > > > Why do we care about > > > > level-triggered interrupts? Everything uses MSI or edge-triggered > > > > IOAPIC interrupts these days. > > > > > > Well lots of emulated devices don't yet. > > > They probably should but it's nice to be able to > > > test with e.g. e1000 emulation not just virtio. > > > > > > e1000 doesn't support msi? > > qemu emulation doesn't. > Can be changed if someone's really interested. But really, avoiding EOIs for e1000 won't help it much. -- error compiling committee.c: too many arguments to function -- 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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 06:00:51PM +0300, Avi Kivity wrote:
> On 04/10/2012 05:53 PM, Michael S. Tsirkin wrote:
> > > >
> > > > Yes. But we can and it's easier than figuring out priorities.
> > > > I am guessing such collisions are rare, right?
> > >
> > > It's pretty easy, if there is something in IRR but
> > > kvm_lapic_has_interrupt() returns -1, then we need to disable eoi
> > > avoidance.
> >
> > I only see kvm_apic_has_interrupt - is this what you mean?
>
> Yes, sorry.
>
> It's not clear whether to do the check in kvm_apic_has_interrupt() or
> kvm_apic_get_interrupt() - the latter is called only after interrupts
> are enabled, so it looks like a better place (EOIs while interrupts are
> disabled have no effect). But need to make sure those functions are
> actually called, since they're protected by KVM_REQ_EVENT.
Sorry not sure what you mean by "make sure" - read the code carefully?
> > > > I'll add a trace to make sure.
> > > >
> > > > > > + if (v != -1)
> > > > > > + apic_set_vector(v, apic->regs +
> > > > > > APIC_ISR);
> > > > > > + } else {
> > > > > > + eoi_set_pending_vector(vcpu, vector);
> > > > > > + set_isr = false;
> > > > >
> > > > > Weird. Just set it normally. Remember that reading the ISR needs to
> > > > > return the correct value.
> > > >
> > > > Marcelo said linux does not normally read ISR - not true?
> > >
> > > It's true and it's irrelevant. We aren't coding a feature to what linux
> > > does now, but for what linux or another guest may do in the future.
> >
> > Right. So you think reading ISR has value
> > in combination with PV EOI for future guests?
> > I'm not arguing either way just curious.
>
> I don't. But we need to preserve the same interface the APIC has
> presented for thousands of years (well, almost).
Talk about overstatements :)
> >
> > > > Note this has no effect if the PV optimization is not enabled.
> > > >
> > > > > We need to process the avoided EOI before any APIC read/writes, to be
> > > > > sure the guest sees the updated values. Same for IOAPIC, EOI affects
> > > > > remote_irr. That may been we need to sample it after every exit, or
> > > > > perhaps disable the feature for level-triggered interrupts.
> > > >
> > > > Disabling would be very sad. Can we sample on remote irr read?
> > >
> > > That can be done from another vcpu.
> >
> > We still can handle it, right? Where's the code that handles that read?
>
> Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c
Hmm. Disabling for level handles the ack notifiers
issue as well, which I forgot about.
It's a tough call. You think looking at
TMR in kvm_get_apic_interrupt is safe?
> >
> > > Why do we care about
> > > level-triggered interrupts? Everything uses MSI or edge-triggered
> > > IOAPIC interrupts these days.
> >
> > Well lots of emulated devices don't yet.
> > They probably should but it's nice to be able to
> > test with e.g. e1000 emulation not just virtio.
>
>
> e1000 doesn't support msi?
qemu emulation doesn't.
> >
> > Besides, kvm_get_apic_interrupt
> > simply doesn't know about the triggering mode at the moment.
> >
>
>
> --
> error compiling committee.c: too many arguments to function
--
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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On 04/10/2012 05:53 PM, Michael S. Tsirkin wrote:
> > >
> > > Yes. But we can and it's easier than figuring out priorities.
> > > I am guessing such collisions are rare, right?
> >
> > It's pretty easy, if there is something in IRR but
> > kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.
>
> I only see kvm_apic_has_interrupt - is this what you mean?
Yes, sorry.
It's not clear whether to do the check in kvm_apic_has_interrupt() or
kvm_apic_get_interrupt() - the latter is called only after interrupts
are enabled, so it looks like a better place (EOIs while interrupts are
disabled have no effect). But need to make sure those functions are
actually called, since they're protected by KVM_REQ_EVENT.
> > > I'll add a trace to make sure.
> > >
> > > > > + if (v != -1)
> > > > > + apic_set_vector(v, apic->regs +
> > > > > APIC_ISR);
> > > > > + } else {
> > > > > + eoi_set_pending_vector(vcpu, vector);
> > > > > + set_isr = false;
> > > >
> > > > Weird. Just set it normally. Remember that reading the ISR needs to
> > > > return the correct value.
> > >
> > > Marcelo said linux does not normally read ISR - not true?
> >
> > It's true and it's irrelevant. We aren't coding a feature to what linux
> > does now, but for what linux or another guest may do in the future.
>
> Right. So you think reading ISR has value
> in combination with PV EOI for future guests?
> I'm not arguing either way just curious.
I don't. But we need to preserve the same interface the APIC has
presented for thousands of years (well, almost).
>
> > > Note this has no effect if the PV optimization is not enabled.
> > >
> > > > We need to process the avoided EOI before any APIC read/writes, to be
> > > > sure the guest sees the updated values. Same for IOAPIC, EOI affects
> > > > remote_irr. That may been we need to sample it after every exit, or
> > > > perhaps disable the feature for level-triggered interrupts.
> > >
> > > Disabling would be very sad. Can we sample on remote irr read?
> >
> > That can be done from another vcpu.
>
> We still can handle it, right? Where's the code that handles that read?
Better to keep everything per-cpu. The code is in virt/kvm/ioapic.c
>
> > Why do we care about
> > level-triggered interrupts? Everything uses MSI or edge-triggered
> > IOAPIC interrupts these days.
>
> Well lots of emulated devices don't yet.
> They probably should but it's nice to be able to
> test with e.g. e1000 emulation not just virtio.
e1000 doesn't support msi?
>
> Besides, kvm_get_apic_interrupt
> simply doesn't know about the triggering mode at the moment.
>
--
error compiling committee.c: too many arguments to function
--
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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 05:03:22PM +0300, Avi Kivity wrote:
> On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> > I took a stub at implementing PV EOI using shared memory.
> > This should reduce the number of exits an interrupt
> > causes as much as by half.
> >
> > A partially complete draft for both host and guest parts
> > is below.
> >
> > The idea is simple: there's a bit, per APIC, in guest memory,
> > that tells the guest that it does not need EOI.
> > We set it before injecting an interrupt and clear
> > before injecting a nested one. Guest tests it using
> > a test and clear operation - this is necessary
> > so that host can detect interrupt nesting -
> > and if set, it can skip the EOI MSR.
> >
> > There's a new MSR to set the address of said register
> > in guest memory. Otherwise not much changes:
> > - Guest EOI is not required
> > - ISR is automatically cleared before injection
> >
> > Some things are incomplete: add feature negotiation
> > options, qemu support for said options.
> > No testing was done beyond compiling the kernel.
> >
> > I would appreciate early feedback.
> >
> > Signed-off-by: Michael S. Tsirkin
> >
> > --
> >
> > diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> > index d854101..8430f41 100644
> > --- a/arch/x86/include/asm/apic.h
> > +++ b/arch/x86/include/asm/apic.h
> > @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) {
> > return 0; }
> >
> > #endif /* CONFIG_X86_LOCAL_APIC */
> >
> > +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> > +
> > static inline void ack_APIC_irq(void)
> > {
> > + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> > + return;
> > +
>
> While __test_and_clear_bit() is implemented in a single instruction,
> it's not required to be. Better have the instruction there explicitly.
>
> > /*
> > * ack_APIC_irq() actually gets compiled as a single instruction
> > * ... yummie.
> > diff --git a/arch/x86/include/asm/kvm_host.h
> > b/arch/x86/include/asm/kvm_host.h
> > index e216ba0..0ee1472 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> > u64 length;
> > u64 status;
> > } osvw;
> > +
> > + struct {
> > + u64 msr_val;
> > + struct gfn_to_hva_cache data;
> > + int vector;
> > + } eoi;
> > };
>
> Needs to be cleared on INIT.
You mean kvm_arch_vcpu_reset?
> >
> >
> > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> >smp_processor_id());
> > }
> >
> > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > + MSR_KVM_EOI_ENABLED);
> > +
>
> Clear on kexec.
With register_reboot_notifier?
> > if (has_steal_clock)
> > kvm_register_steal_time();
> > }
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 8584322..9e38e12 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct
> > kvm_lapic_irq *irq)
> > irq->level, irq->trig_mode);
> > }
> >
> > -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> > +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> > +{
> > +
> > + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
> > + sizeof(val));
> > +}
> > +
> > +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> > +{
> > +
> > + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
> > + sizeof(*val));
> > +}
> > +
> > +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> > +{
> > + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> > +}
> > +
> > +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> > +{
> > + u32 val;
> > + if (eoi_get_user(vcpu, &val) < 0)
> > + apic_debug("Can't read EOI MSR value: 0x%llx\n",
> > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > + if (!(val & 0x1))
> > + vcpu->arch.eoi.vector = -1;
> > + return vcpu->arch.eoi.vector;
> > +}
> > +
> > +static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
> > +{
> > + BUG_ON(vcpu->arch.eoi.vector != -1);
> > + if (eoi_put_user(vcpu, 0x1) < 0) {
> > + apic_debug("Can't set EOI MSR value: 0x%llx\n",
> > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > + return;
> > + }
> > + vcpu->arch.eoi.vector = vector;
> > +}
> > +
> > +static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
> > +{
> > + int vector;
> > + vector = vcpu->arch.eoi.vector;
> > + if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
> > + apic_debug("Can't clear EOI MSR value: 0x%llx\n",
> > + (unsigned long long)vcpi->arch.eoi.msr_val);
> > + return -1;
> > + }
> > + vcpu->arch.eoi.
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On Tue, Apr 10, 2012 at 05:33:18PM +0300, Avi Kivity wrote:
> On 04/10/2012 05:26 PM, Michael S. Tsirkin wrote:
> > > > u64 status;
> > > > } osvw;
> > > > +
> > > > + struct {
> > > > + u64 msr_val;
> > > > + struct gfn_to_hva_cache data;
> > > > + int vector;
> > > > + } eoi;
> > > > };
> > >
> > > Needs to be cleared on INIT.
> >
> > You mean kvm_arch_vcpu_reset?
>
> Yes, or kvm_lapic_reset().
>
> > > >
> > > >
> > > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > >smp_processor_id());
> > > > }
> > > >
> > > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > > + MSR_KVM_EOI_ENABLED);
> > > > +
> > >
> > > Clear on kexec.
> >
> > With register_reboot_notifier?
>
> Yes, we already clear some kvm msrs there.
>
> > > >
> > > > - apic_set_vector(vector, apic->regs + APIC_ISR);
> > > > + if (eoi_enabled(vcpu)) {
> > > > + /* Anything pending? If yes disable eoi optimization. */
> > > > + if (unlikely(apic_find_highest_isr(apic) >= 0)) {
> > > > + int v = eoi_clr_pending_vector(vcpu);
> > >
> > > ISR != pending, that's IRR. If ISR vector has lower priority than the
> > > new vector, then we don't need to disable eoi avoidance.
> >
> > Yes. But we can and it's easier than figuring out priorities.
> > I am guessing such collisions are rare, right?
>
> It's pretty easy, if there is something in IRR but
> kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.
I only see kvm_apic_has_interrupt - is this what you mean?
> > I'll add a trace to make sure.
> >
> > > > + if (v != -1)
> > > > + apic_set_vector(v, apic->regs +
> > > > APIC_ISR);
> > > > + } else {
> > > > + eoi_set_pending_vector(vcpu, vector);
> > > > + set_isr = false;
> > >
> > > Weird. Just set it normally. Remember that reading the ISR needs to
> > > return the correct value.
> >
> > Marcelo said linux does not normally read ISR - not true?
>
> It's true and it's irrelevant. We aren't coding a feature to what linux
> does now, but for what linux or another guest may do in the future.
Right. So you think reading ISR has value
in combination with PV EOI for future guests?
I'm not arguing either way just curious.
> > Note this has no effect if the PV optimization is not enabled.
> >
> > > We need to process the avoided EOI before any APIC read/writes, to be
> > > sure the guest sees the updated values. Same for IOAPIC, EOI affects
> > > remote_irr. That may been we need to sample it after every exit, or
> > > perhaps disable the feature for level-triggered interrupts.
> >
> > Disabling would be very sad. Can we sample on remote irr read?
>
> That can be done from another vcpu.
We still can handle it, right? Where's the code that handles that read?
> Why do we care about
> level-triggered interrupts? Everything uses MSI or edge-triggered
> IOAPIC interrupts these days.
Well lots of emulated devices don't yet.
They probably should but it's nice to be able to
test with e.g. e1000 emulation not just virtio.
Besides, kvm_get_apic_interrupt
simply doesn't know about the triggering mode at the moment.
--
MST
--
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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On 04/10/2012 05:26 PM, Michael S. Tsirkin wrote:
> > > u64 status;
> > > } osvw;
> > > +
> > > + struct {
> > > + u64 msr_val;
> > > + struct gfn_to_hva_cache data;
> > > + int vector;
> > > + } eoi;
> > > };
> >
> > Needs to be cleared on INIT.
>
> You mean kvm_arch_vcpu_reset?
Yes, or kvm_lapic_reset().
> > >
> > >
> > > @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> > > smp_processor_id());
> > > }
> > >
> > > + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> > > +MSR_KVM_EOI_ENABLED);
> > > +
> >
> > Clear on kexec.
>
> With register_reboot_notifier?
Yes, we already clear some kvm msrs there.
> > >
> > > - apic_set_vector(vector, apic->regs + APIC_ISR);
> > > + if (eoi_enabled(vcpu)) {
> > > + /* Anything pending? If yes disable eoi optimization. */
> > > + if (unlikely(apic_find_highest_isr(apic) >= 0)) {
> > > + int v = eoi_clr_pending_vector(vcpu);
> >
> > ISR != pending, that's IRR. If ISR vector has lower priority than the
> > new vector, then we don't need to disable eoi avoidance.
>
> Yes. But we can and it's easier than figuring out priorities.
> I am guessing such collisions are rare, right?
It's pretty easy, if there is something in IRR but
kvm_lapic_has_interrupt() returns -1, then we need to disable eoi avoidance.
> I'll add a trace to make sure.
>
> > > + if (v != -1)
> > > + apic_set_vector(v, apic->regs + APIC_ISR);
> > > + } else {
> > > + eoi_set_pending_vector(vcpu, vector);
> > > + set_isr = false;
> >
> > Weird. Just set it normally. Remember that reading the ISR needs to
> > return the correct value.
>
> Marcelo said linux does not normally read ISR - not true?
It's true and it's irrelevant. We aren't coding a feature to what linux
does now, but for what linux or another guest may do in the future.
> Note this has no effect if the PV optimization is not enabled.
>
> > We need to process the avoided EOI before any APIC read/writes, to be
> > sure the guest sees the updated values. Same for IOAPIC, EOI affects
> > remote_irr. That may been we need to sample it after every exit, or
> > perhaps disable the feature for level-triggered interrupts.
>
> Disabling would be very sad. Can we sample on remote irr read?
That can be done from another vcpu. Why do we care about
level-triggered interrupts? Everything uses MSI or edge-triggered
IOAPIC interrupts these days.
--
error compiling committee.c: too many arguments to function
--
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
Re: [PATCHv0 dont apply] RFC: kvm eoi PV using shared memory
On 04/10/2012 04:27 PM, Michael S. Tsirkin wrote:
> I took a stub at implementing PV EOI using shared memory.
> This should reduce the number of exits an interrupt
> causes as much as by half.
>
> A partially complete draft for both host and guest parts
> is below.
>
> The idea is simple: there's a bit, per APIC, in guest memory,
> that tells the guest that it does not need EOI.
> We set it before injecting an interrupt and clear
> before injecting a nested one. Guest tests it using
> a test and clear operation - this is necessary
> so that host can detect interrupt nesting -
> and if set, it can skip the EOI MSR.
>
> There's a new MSR to set the address of said register
> in guest memory. Otherwise not much changes:
> - Guest EOI is not required
> - ISR is automatically cleared before injection
>
> Some things are incomplete: add feature negotiation
> options, qemu support for said options.
> No testing was done beyond compiling the kernel.
>
> I would appreciate early feedback.
>
> Signed-off-by: Michael S. Tsirkin
>
> --
>
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index d854101..8430f41 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -457,8 +457,13 @@ static inline u32 safe_apic_wait_icr_idle(void) { return
> 0; }
>
> #endif /* CONFIG_X86_LOCAL_APIC */
>
> +DECLARE_EARLY_PER_CPU(unsigned long, apic_eoi);
> +
> static inline void ack_APIC_irq(void)
> {
> + if (__test_and_clear_bit(0, &__get_cpu_var(apic_eoi)))
> + return;
> +
While __test_and_clear_bit() is implemented in a single instruction,
it's not required to be. Better have the instruction there explicitly.
> /*
>* ack_APIC_irq() actually gets compiled as a single instruction
>* ... yummie.
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e216ba0..0ee1472 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -481,6 +481,12 @@ struct kvm_vcpu_arch {
> u64 length;
> u64 status;
> } osvw;
> +
> + struct {
> + u64 msr_val;
> + struct gfn_to_hva_cache data;
> + int vector;
> + } eoi;
> };
Needs to be cleared on INIT.
>
>
> @@ -307,6 +308,9 @@ void __cpuinit kvm_guest_cpu_init(void)
> smp_processor_id());
> }
>
> + wrmsrl(MSR_KVM_EOI_EN, __pa(this_cpu_ptr(apic_eoi)) |
> +MSR_KVM_EOI_ENABLED);
> +
Clear on kexec.
> if (has_steal_clock)
> kvm_register_steal_time();
> }
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 8584322..9e38e12 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -265,7 +265,61 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct
> kvm_lapic_irq *irq)
> irq->level, irq->trig_mode);
> }
>
> -static inline int apic_find_highest_isr(struct kvm_lapic *apic)
> +static int eoi_put_user(struct kvm_vcpu *vcpu, u32 val)
> +{
> +
> + return kvm_write_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, &val,
> + sizeof(val));
> +}
> +
> +static int eoi_get_user(struct kvm_vcpu *vcpu, u32 *val)
> +{
> +
> + return kvm_read_guest_cached(vcpu->kvm, &vcpu->arch.eoi.data, val,
> + sizeof(*val));
> +}
> +
> +static inline bool eoi_enabled(struct kvm_vcpu *vcpu)
> +{
> + return (vcpu->arch.eoi.msr_val & MSR_KVM_EOI_ENABLED);
> +}
> +
> +static int eoi_get_pending_vector(struct kvm_vcpu *vcpu)
> +{
> + u32 val;
> + if (eoi_get_user(vcpu, &val) < 0)
> + apic_debug("Can't read EOI MSR value: 0x%llx\n",
> +(unsigned long long)vcpi->arch.eoi.msr_val);
> + if (!(val & 0x1))
> + vcpu->arch.eoi.vector = -1;
> + return vcpu->arch.eoi.vector;
> +}
> +
> +static void eoi_set_pending_vector(struct kvm_vcpu *vcpu, int vector)
> +{
> + BUG_ON(vcpu->arch.eoi.vector != -1);
> + if (eoi_put_user(vcpu, 0x1) < 0) {
> + apic_debug("Can't set EOI MSR value: 0x%llx\n",
> +(unsigned long long)vcpi->arch.eoi.msr_val);
> + return;
> + }
> + vcpu->arch.eoi.vector = vector;
> +}
> +
> +static int eoi_clr_pending_vector(struct kvm_vcpu *vcpu)
> +{
> + int vector;
> + vector = vcpu->arch.eoi.vector;
> + if (vector != -1 && eoi_put_user(vcpu, 0x0) < 0) {
> + apic_debug("Can't clear EOI MSR value: 0x%llx\n",
> +(unsigned long long)vcpi->arch.eoi.msr_val);
> + return -1;
> + }
> + vcpu->arch.eoi.vector = -1;
> + return vector;
> +}
> +
> +static inline int __apic_find_highest_isr(struct kvm_lapic *apic)
> {
> int result;
>
> @@ -275,6 +329,17 @@ static inline int apic_find_highest_isr(struct kvm_lapic
> *apic)
> return result;
> }
>
> +static inline int apic_find_highest_isr(struct kvm_lapi
