kvm/x86: implement hv EOI assist
RFC PATCH 1/1 kvm/x86: implement hv EOI assist
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.
---
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.
arch/x86/kvm/x86.c +4 -0
@@ -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 :
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
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.
Quoted 7 lines · Marcelo Tosatti
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
Quoted 8 lines
Looks alright except:
- There is no handling of PV EOI data to be performed at
HV_X64_MSR_EOIwrite 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?
Signature · 1 line
MST
Quoted 19 lines · Michael S. Tsirkin
Quoted 7 lines · Marcelo Tosatti
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>Quoted 8 lines
Looks alright except:
- There is no handling of PV EOI data to be performed at
HV_X64_MSR_EOIwrite 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?
Quoted 7 lines
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.
Quoted 10 lines
/* * 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.
Quoted 7 lines · Marcelo Tosatti
Quoted 6 lines · Michael S. Tsirkin
Quoted 5 lines · Marcelo Tosatti
Quoted 4 lines · Michael S. Tsirkin
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.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>Looks alright except:
- There is no handling of PV EOI data to be performed at
HV_X64_MSR_EOIwrite 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?
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- 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 ...
Quoted 27 lines
Quoted 5 lines
Quoted 2 lines
- 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?
Quoted 7 lines
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.
Quoted 10 lines
/* * 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.
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Marcelo Tosatti
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Marcelo Tosatti
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>
Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Looks alright except:
- There is no handling of PV EOI data to be performed at
HV_X64_MSR_EOIwrite path?I thought
HV_X64_MSR_EOIis a separate optimization - it's an X2APIC replacement that lets you do EOI with an MSR and not IO. No?
Yes.
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- 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 ?
Quoted 27 lines
Quoted 27 lines
Quoted 5 lines
Quoted 2 lines
- 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?
Quoted 7 lines
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.
Quoted 10 lines
/* * 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.
Quoted 22 lines · Marcelo Tosatti
Quoted 10 lines · Michael S. Tsirkin
Quoted 9 lines · Marcelo Tosatti
Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Marcelo Tosatti
Quoted 5 lines · Michael S. Tsirkin
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 <[email protected]>Quoted 8 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Looks alright except:
- There is no handling of PV EOI data to be performed at
HV_X64_MSR_EOIwrite path?I thought
HV_X64_MSR_EOIis a separate optimization - it's an X2APIC replacement that lets you do EOI with an MSR and not IO. No?Yes.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
- 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_ENis 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?
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 5 lines
Quoted 2 lines
- 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?
Quoted 7 lines
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.
Quoted 10 lines
/* * 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.
Quoted 10 lines · Michael S. Tsirkin
Quoted 4 lines
Quoted 1 line
HV-APIC-ASSIST MSR is in the list of saved MSRs ...
Restoration of
MSR_KVM_PV_EOI_ENis required for migration under when PVEOI enabled ?That's what I don't get.
Since after this patch, set ofHV_X64_MSR_APIC_ASSIST_PAGEsetsKVM_PV_EOI_ENinternally, I think restoringHV_X64_MSR_APIC_ASSIST_PAGEwould be sufficient.
No?
Yes.