* [PATCH RFC untested] kvm/x86: implement hv EOI assist
@ 2014-04-13 13:10 Michael S. Tsirkin
2014-04-21 21:40 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-04-13 13:10 UTC (permalink / raw)
To: Ronen Hod; +Cc: vrozenfe, pbonzini, kvm
It seems that it's easy to implement the EOI assist
on top of the PV EOI feature: simply convert the
page address to the format expected by PV EOI.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Warning: untested! As I'll be off-line for a couple of days,
sending this out for early review.
Review/comments/flames/testing reports welcome.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8ae1ff5..d84d750fc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1890,6 +1890,8 @@ static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
if (!(data & HV_X64_MSR_APIC_ASSIST_PAGE_ENABLE)) {
vcpu->arch.hv_vapic = data;
+ if (kvm_lapic_enable_pv_eoi(vcpu, 0))
+ return 1;
break;
}
gfn = data >> HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT;
@@ -1900,6 +1902,8 @@ static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
return 1;
vcpu->arch.hv_vapic = data;
mark_page_dirty(vcpu->kvm, gfn);
+ if (kvm_lapic_enable_pv_eoi(vcpu, gfn_to_gpa(gfn) | KVM_MSR_ENABLED))
+ return 1;
break;
}
case HV_X64_MSR_EOI:
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-13 13:10 [PATCH RFC untested] kvm/x86: implement hv EOI assist Michael S. Tsirkin
@ 2014-04-21 21:40 ` Marcelo Tosatti
2014-04-22 9:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2014-04-21 21:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> It seems that it's easy to implement the EOI assist
> on top of the PV EOI feature: simply convert the
> page address to the format expected by PV EOI.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Looks alright except:
- There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
path?
- Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
require it for PV-EOI over HV-APIC-ASSIST).
- MS docs mention "No EOI required" is set only if interrupt injected is edge
triggered.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-21 21:40 ` Marcelo Tosatti
@ 2014-04-22 9:11 ` Michael S. Tsirkin
2014-04-22 19:26 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-04-22 9:11 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Mon, Apr 21, 2014 at 06:40:17PM -0300, Marcelo Tosatti wrote:
> On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> > It seems that it's easy to implement the EOI assist
> > on top of the PV EOI feature: simply convert the
> > page address to the format expected by PV EOI.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Looks alright except:
>
> - There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
> path?
> - Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
> require it for PV-EOI over HV-APIC-ASSIST).
> - MS docs mention "No EOI required" is set only if interrupt injected is edge
> triggered.
Hmm I thought level interrupts are going through IOAPIC so that's already true isn't it?
if (!pv_eoi_enabled(vcpu) ||
/* IRR set or many bits in ISR: could be nested. */
apic->irr_pending ||
/* Cache not set: could be safe but we don't bother. */
apic->highest_isr_cache == -1 ||
---> /* Need EOI to update ioapic. */
kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
/*
* PV EOI was disabled by apic_sync_pv_eoi_from_guest
* so we need not do anything here.
*/
return;
}
In any case if some interrupt handler ignores this bit because it's
level, that's harmless since it will do EOI and then we'll clear the
bit, right?
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-22 9:11 ` Michael S. Tsirkin
@ 2014-04-22 19:26 ` Marcelo Tosatti
2014-04-22 22:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2014-04-22 19:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Tue, Apr 22, 2014 at 12:11:47PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 21, 2014 at 06:40:17PM -0300, Marcelo Tosatti wrote:
> > On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> > > It seems that it's easy to implement the EOI assist
> > > on top of the PV EOI feature: simply convert the
> > > page address to the format expected by PV EOI.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Looks alright except:
> >
> > - There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
> > path?
> > - Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
> > require it for PV-EOI over HV-APIC-ASSIST).
> > - MS docs mention "No EOI required" is set only if interrupt injected is edge
> > triggered.
>
> Hmm I thought level interrupts are going through IOAPIC so that's already true isn't it?
>
> if (!pv_eoi_enabled(vcpu) ||
> /* IRR set or many bits in ISR: could be nested. */
> apic->irr_pending ||
> /* Cache not set: could be safe but we don't bother. */
> apic->highest_isr_cache == -1 ||
> ---> /* Need EOI to update ioapic. */
> kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
Right.
> /*
> * PV EOI was disabled by apic_sync_pv_eoi_from_guest
> * so we need not do anything here.
> */
> return;
> }
>
> In any case if some interrupt handler ignores this bit because it's
> level, that's harmless since it will do EOI and then we'll clear the
> bit, right?
Yes.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-22 19:26 ` Marcelo Tosatti
@ 2014-04-22 22:12 ` Michael S. Tsirkin
2014-04-23 1:57 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-04-22 22:12 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Tue, Apr 22, 2014 at 04:26:48PM -0300, Marcelo Tosatti wrote:
> On Tue, Apr 22, 2014 at 12:11:47PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 21, 2014 at 06:40:17PM -0300, Marcelo Tosatti wrote:
> > > On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> > > > It seems that it's easy to implement the EOI assist
> > > > on top of the PV EOI feature: simply convert the
> > > > page address to the format expected by PV EOI.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > Looks alright except:
> > >
> > > - There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
> > > path?
I thought HV_X64_MSR_EOI is a separate optimization - it's an X2APIC
replacement that lets you do EOI with an MSR and not IO.
No?
> > > - Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
> > > require it for PV-EOI over HV-APIC-ASSIST).
Did not try migration yet - could you explain the issue please?
HV-APIC-ASSIST MSR is in the list of saved MSRs ...
> > > - MS docs mention "No EOI required" is set only if interrupt injected is edge
> > > triggered.
> >
> > Hmm I thought level interrupts are going through IOAPIC so that's already true isn't it?
> >
> > if (!pv_eoi_enabled(vcpu) ||
> > /* IRR set or many bits in ISR: could be nested. */
> > apic->irr_pending ||
> > /* Cache not set: could be safe but we don't bother. */
> > apic->highest_isr_cache == -1 ||
> > ---> /* Need EOI to update ioapic. */
> > kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
>
> Right.
>
> > /*
> > * PV EOI was disabled by apic_sync_pv_eoi_from_guest
> > * so we need not do anything here.
> > */
> > return;
> > }
> >
> > In any case if some interrupt handler ignores this bit because it's
> > level, that's harmless since it will do EOI and then we'll clear the
> > bit, right?
>
> Yes.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-22 22:12 ` Michael S. Tsirkin
@ 2014-04-23 1:57 ` Marcelo Tosatti
2014-04-23 5:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 8+ messages in thread
From: Marcelo Tosatti @ 2014-04-23 1:57 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Wed, Apr 23, 2014 at 01:12:49AM +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 22, 2014 at 04:26:48PM -0300, Marcelo Tosatti wrote:
> > On Tue, Apr 22, 2014 at 12:11:47PM +0300, Michael S. Tsirkin wrote:
> > > On Mon, Apr 21, 2014 at 06:40:17PM -0300, Marcelo Tosatti wrote:
> > > > On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> > > > > It seems that it's easy to implement the EOI assist
> > > > > on top of the PV EOI feature: simply convert the
> > > > > page address to the format expected by PV EOI.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > >
> > > > Looks alright except:
> > > >
> > > > - There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
> > > > path?
>
> I thought HV_X64_MSR_EOI is a separate optimization - it's an X2APIC
> replacement that lets you do EOI with an MSR and not IO.
> No?
Yes.
> > > > - Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
> > > > require it for PV-EOI over HV-APIC-ASSIST).
>
> Did not try migration yet - could you explain the issue please?
> HV-APIC-ASSIST MSR is in the list of saved MSRs ...
Restoration of MSR_KVM_PV_EOI_EN is required for migration under
when PVEOI enabled ?
> > > > - MS docs mention "No EOI required" is set only if interrupt injected is edge
> > > > triggered.
> > >
> > > Hmm I thought level interrupts are going through IOAPIC so that's already true isn't it?
> > >
> > > if (!pv_eoi_enabled(vcpu) ||
> > > /* IRR set or many bits in ISR: could be nested. */
> > > apic->irr_pending ||
> > > /* Cache not set: could be safe but we don't bother. */
> > > apic->highest_isr_cache == -1 ||
> > > ---> /* Need EOI to update ioapic. */
> > > kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
> >
> > Right.
> >
> > > /*
> > > * PV EOI was disabled by apic_sync_pv_eoi_from_guest
> > > * so we need not do anything here.
> > > */
> > > return;
> > > }
> > >
> > > In any case if some interrupt handler ignores this bit because it's
> > > level, that's harmless since it will do EOI and then we'll clear the
> > > bit, right?
> >
> > Yes.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-23 1:57 ` Marcelo Tosatti
@ 2014-04-23 5:52 ` Michael S. Tsirkin
2014-04-23 18:30 ` Marcelo Tosatti
0 siblings, 1 reply; 8+ messages in thread
From: Michael S. Tsirkin @ 2014-04-23 5:52 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Tue, Apr 22, 2014 at 10:57:48PM -0300, Marcelo Tosatti wrote:
> On Wed, Apr 23, 2014 at 01:12:49AM +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 22, 2014 at 04:26:48PM -0300, Marcelo Tosatti wrote:
> > > On Tue, Apr 22, 2014 at 12:11:47PM +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Apr 21, 2014 at 06:40:17PM -0300, Marcelo Tosatti wrote:
> > > > > On Sun, Apr 13, 2014 at 04:10:22PM +0300, Michael S. Tsirkin wrote:
> > > > > > It seems that it's easy to implement the EOI assist
> > > > > > on top of the PV EOI feature: simply convert the
> > > > > > page address to the format expected by PV EOI.
> > > > > >
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > >
> > > > > Looks alright except:
> > > > >
> > > > > - There is no handling of PV EOI data to be performed at HV_X64_MSR_EOI write
> > > > > path?
> >
> > I thought HV_X64_MSR_EOI is a separate optimization - it's an X2APIC
> > replacement that lets you do EOI with an MSR and not IO.
> > No?
>
> Yes.
>
> > > > > - Migration fails with PV-EOI not enabled in CPUID ? (perhaps could
> > > > > require it for PV-EOI over HV-APIC-ASSIST).
> >
> > Did not try migration yet - could you explain the issue please?
> > HV-APIC-ASSIST MSR is in the list of saved MSRs ...
>
> Restoration of MSR_KVM_PV_EOI_EN is required for migration under
> when PVEOI enabled ?
That's what I don't get.
Since after this patch, set of HV_X64_MSR_APIC_ASSIST_PAGE sets
KVM_PV_EOI_EN internally, I think restoring HV_X64_MSR_APIC_ASSIST_PAGE
would be sufficient.
No?
> > > > > - MS docs mention "No EOI required" is set only if interrupt injected is edge
> > > > > triggered.
> > > >
> > > > Hmm I thought level interrupts are going through IOAPIC so that's already true isn't it?
> > > >
> > > > if (!pv_eoi_enabled(vcpu) ||
> > > > /* IRR set or many bits in ISR: could be nested. */
> > > > apic->irr_pending ||
> > > > /* Cache not set: could be safe but we don't bother. */
> > > > apic->highest_isr_cache == -1 ||
> > > > ---> /* Need EOI to update ioapic. */
> > > > kvm_ioapic_handles_vector(vcpu->kvm, apic->highest_isr_cache)) {
> > >
> > > Right.
> > >
> > > > /*
> > > > * PV EOI was disabled by apic_sync_pv_eoi_from_guest
> > > > * so we need not do anything here.
> > > > */
> > > > return;
> > > > }
> > > >
> > > > In any case if some interrupt handler ignores this bit because it's
> > > > level, that's harmless since it will do EOI and then we'll clear the
> > > > bit, right?
> > >
> > > Yes.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH RFC untested] kvm/x86: implement hv EOI assist
2014-04-23 5:52 ` Michael S. Tsirkin
@ 2014-04-23 18:30 ` Marcelo Tosatti
0 siblings, 0 replies; 8+ messages in thread
From: Marcelo Tosatti @ 2014-04-23 18:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ronen Hod, vrozenfe, pbonzini, kvm
On Wed, Apr 23, 2014 at 08:52:03AM +0300, Michael S. Tsirkin wrote:
> > > HV-APIC-ASSIST MSR is in the list of saved MSRs ...
> >
> > Restoration of MSR_KVM_PV_EOI_EN is required for migration under
> > when PVEOI enabled ?
>
> That's what I don't get.
> Since after this patch, set of HV_X64_MSR_APIC_ASSIST_PAGE sets
> KVM_PV_EOI_EN internally, I think restoring HV_X64_MSR_APIC_ASSIST_PAGE
> would be sufficient.
> No?
Yes.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-04-23 18:30 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-13 13:10 [PATCH RFC untested] kvm/x86: implement hv EOI assist Michael S. Tsirkin
2014-04-21 21:40 ` Marcelo Tosatti
2014-04-22 9:11 ` Michael S. Tsirkin
2014-04-22 19:26 ` Marcelo Tosatti
2014-04-22 22:12 ` Michael S. Tsirkin
2014-04-23 1:57 ` Marcelo Tosatti
2014-04-23 5:52 ` Michael S. Tsirkin
2014-04-23 18:30 ` Marcelo Tosatti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).