| Message ID | 20200612155640.4101-9-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vpt: fixes for vpt and enable vPIT for PVH dom0 | expand |
On 12/06/2020 16:56, Roger Pau Monne wrote: > Some video BIOS require a PIT in order to work properly, hence classic > PV dom0 gets partial access to the physical PIT as long as it's not in > use by Xen. Is this actually true today? I can believe that it may have been necessary on old hardware, but the structure of systems has changed massively over the past 20 years, and the PIT is very legacy these days. We shouldn't be blindly propagating bodges like this forward. ~Andrew
On Mon, Jun 15, 2020 at 04:33:33PM +0100, Andrew Cooper wrote: > On 12/06/2020 16:56, Roger Pau Monne wrote: > > Some video BIOS require a PIT in order to work properly, hence classic > > PV dom0 gets partial access to the physical PIT as long as it's not in > > use by Xen. > > Is this actually true today? TBH I have no idea and asked the same thing myself. > I can believe that it may have been necessary on old hardware, but the > structure of systems has changed massively over the past 20 years, and > the PIT is very legacy these days. I also wondered whether video BIOSes really changed in the last 20 years, I really have no idea. FWIW, Wikipedia still lists PIT as being used by video BIOSes on x86 systems [0] but there are no references to specific models or video BIOSes. Alternatively we could add this as a Xen command line option, ie: dom0=pit or some such. It's however not very nice to not get output because the video BIOS doesn't function properly due to lack of PIT. Thanks, Roger. [0] https://en.wikipedia.org/wiki/Intel_8253#IBM_PC_programming_tips_and_hints
On 12.06.2020 17:56, Roger Pau Monne wrote: > Some video BIOS require a PIT in order to work properly, hence classic > PV dom0 gets partial access to the physical PIT as long as it's not in > use by Xen. > > Since PVH dom0 is built on top of HVM support, there's already an > emulated PIT implementation available for use. Tweak the emulated PIT > code so it injects interrupts directly into the vIO-APIC if the legacy > PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0 > in the likely case there's an interrupt overwrite in the MADT ACPI Same nit again as for the earlier patch (also applicable to a code comment below). > @@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d, > > emflags = config->arch.emulation_flags; > > - if ( is_hardware_domain(d) && is_pv_domain(d) ) > + if ( is_hardware_domain(d) ) > emflags |= XEN_X86_EMU_PIT; Wouldn't this better go into create_dom0(), where all the other flags get set? Or otherwise all of that be moved here (to cover the late-hwdom case)? > --- a/xen/arch/x86/emul-i8254.c > +++ b/xen/arch/x86/emul-i8254.c > @@ -168,6 +168,7 @@ static void pit_load_count(PITState *pit, int channel, int val) > u32 period; > struct hvm_hw_pit_channel *s = &pit->hw.channels[channel]; > struct vcpu *v = vpit_vcpu(pit); > + const struct domain *d = v ? v->domain : NULL; I think this local variable would better be omitted - its initializer may raise questions, while at its use sites v can't be NULL afaict. Plus it's not needed ... > @@ -190,14 +191,18 @@ static void pit_load_count(PITState *pit, int channel, int val) > case 3: > /* Periodic timer. */ > TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period); > - create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, > + create_periodic_time(v, &pit->pt0, period, period, > + has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0), > + pit_time_fired, > &pit->count_load_time[channel], false); > break; > case 1: > case 4: > /* One-shot timer. */ > TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0); > - create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired, > + create_periodic_time(v, &pit->pt0, period, 0, > + has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0), > + pit_time_fired, > &pit->count_load_time[channel], false); > break; > default: ... on this path. > --- a/xen/arch/x86/hvm/vioapic.c > +++ b/xen/arch/x86/hvm/vioapic.c > @@ -268,7 +268,14 @@ static void vioapic_write_redirent( > > spin_unlock(&d->arch.hvm.irq_lock); > > - if ( is_hardware_domain(d) && unmasked ) > + if ( is_hardware_domain(d) && unmasked && > + /* > + * A PVH dom0 can have an emulated PIT that should respect any > + * interrupt overwrites found in the ACPI MADT table, so we need to > + * check to which GSI the ISA IRQ 0 is mapped in order to prevent > + * identity mapping it. > + */ > + (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) ) Isn't has_vpit() true now for Dom0, and hence that part of the condition is kind of pointless? And shouldn't Dom0 never have seen physical IRQ 0 in the first place (we don't allow PV Dom0 to use that IRQ either, after all)? Jan
On Thu, Jun 18, 2020 at 06:05:21PM +0200, Jan Beulich wrote: > On 12.06.2020 17:56, Roger Pau Monne wrote: > > Some video BIOS require a PIT in order to work properly, hence classic > > PV dom0 gets partial access to the physical PIT as long as it's not in > > use by Xen. > > > > Since PVH dom0 is built on top of HVM support, there's already an > > emulated PIT implementation available for use. Tweak the emulated PIT > > code so it injects interrupts directly into the vIO-APIC if the legacy > > PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0 > > in the likely case there's an interrupt overwrite in the MADT ACPI > > Same nit again as for the earlier patch (also applicable to a code > comment below). > > > @@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d, > > > > emflags = config->arch.emulation_flags; > > > > - if ( is_hardware_domain(d) && is_pv_domain(d) ) > > + if ( is_hardware_domain(d) ) > > emflags |= XEN_X86_EMU_PIT; > > Wouldn't this better go into create_dom0(), where all the other > flags get set? Or otherwise all of that be moved here (to cover > the late-hwdom case)? I've just moved all setting of the emulation_flags to arch_domain_create so it's done at the same place for PV and PVH. > > --- a/xen/arch/x86/hvm/vioapic.c > > +++ b/xen/arch/x86/hvm/vioapic.c > > @@ -268,7 +268,14 @@ static void vioapic_write_redirent( > > > > spin_unlock(&d->arch.hvm.irq_lock); > > > > - if ( is_hardware_domain(d) && unmasked ) > > + if ( is_hardware_domain(d) && unmasked && > > + /* > > + * A PVH dom0 can have an emulated PIT that should respect any > > + * interrupt overwrites found in the ACPI MADT table, so we need to > > + * check to which GSI the ISA IRQ 0 is mapped in order to prevent > > + * identity mapping it. > > + */ > > + (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) ) > > Isn't has_vpit() true now for Dom0, and hence that part of the > condition is kind of pointless? Well, yes, but I think we should strive for the code to be prepared to deal with both vPIT enabled or disabled, and hence shouldn't make assumptions. > And shouldn't Dom0 never have seen > physical IRQ 0 in the first place (we don't allow PV Dom0 to use > that IRQ either, after all)? Yes, that will fail in map_domain_pirq, so a PVH dom0 won't be able to bind IRQ 0 anyway. Roger.
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index fee6c3931a..dc0b4c2284 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -512,7 +512,8 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags) if ( is_hvm_domain(d) ) { if ( is_hardware_domain(d) && - emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) ) + emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC | + X86_EMU_PIT) ) return false; if ( !is_hardware_domain(d) && emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) && @@ -578,7 +579,7 @@ int arch_domain_create(struct domain *d, emflags = config->arch.emulation_flags; - if ( is_hardware_domain(d) && is_pv_domain(d) ) + if ( is_hardware_domain(d) ) emflags |= XEN_X86_EMU_PIT; if ( emflags & ~XEN_X86_EMU_ALL ) diff --git a/xen/arch/x86/emul-i8254.c b/xen/arch/x86/emul-i8254.c index 73be4188ad..5aef0fe852 100644 --- a/xen/arch/x86/emul-i8254.c +++ b/xen/arch/x86/emul-i8254.c @@ -168,6 +168,7 @@ static void pit_load_count(PITState *pit, int channel, int val) u32 period; struct hvm_hw_pit_channel *s = &pit->hw.channels[channel]; struct vcpu *v = vpit_vcpu(pit); + const struct domain *d = v ? v->domain : NULL; ASSERT(spin_is_locked(&pit->lock)); @@ -190,14 +191,18 @@ static void pit_load_count(PITState *pit, int channel, int val) case 3: /* Periodic timer. */ TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, period); - create_periodic_time(v, &pit->pt0, period, period, 0, pit_time_fired, + create_periodic_time(v, &pit->pt0, period, period, + has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0), + pit_time_fired, &pit->count_load_time[channel], false); break; case 1: case 4: /* One-shot timer. */ TRACE_2D(TRC_HVM_EMUL_PIT_START_TIMER, period, 0); - create_periodic_time(v, &pit->pt0, period, 0, 0, pit_time_fired, + create_periodic_time(v, &pit->pt0, period, 0, + has_vpic(d) ? 0 : hvm_isa_irq_to_gsi(d, 0), + pit_time_fired, &pit->count_load_time[channel], false); break; default: @@ -455,7 +460,8 @@ void pit_reset(struct domain *d) { TRACE_0D(TRC_HVM_EMUL_PIT_STOP_TIMER); destroy_periodic_time(&pit->pt0); - pit->pt0.source = PTSRC_isa; + ASSERT(has_vpic(d) || has_vioapic(d)); + pit->pt0.source = has_vpic(d) ? PTSRC_isa : PTSRC_ioapic; } spin_lock(&pit->lock); diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c index 34bec715b7..8b95e4412f 100644 --- a/xen/arch/x86/hvm/vioapic.c +++ b/xen/arch/x86/hvm/vioapic.c @@ -268,7 +268,14 @@ static void vioapic_write_redirent( spin_unlock(&d->arch.hvm.irq_lock); - if ( is_hardware_domain(d) && unmasked ) + if ( is_hardware_domain(d) && unmasked && + /* + * A PVH dom0 can have an emulated PIT that should respect any + * interrupt overwrites found in the ACPI MADT table, so we need to + * check to which GSI the ISA IRQ 0 is mapped in order to prevent + * identity mapping it. + */ + (!has_vpit(d) || gsi != hvm_isa_irq_to_gsi(d, 0)) ) { /* * NB: don't call vioapic_hwdom_map_gsi while holding hvm.irq_lock
Some video BIOS require a PIT in order to work properly, hence classic PV dom0 gets partial access to the physical PIT as long as it's not in use by Xen. Since PVH dom0 is built on top of HVM support, there's already an emulated PIT implementation available for use. Tweak the emulated PIT code so it injects interrupts directly into the vIO-APIC if the legacy PIC (i8259) is disabled. Make sure the GSI used matches the ISA IRQ 0 in the likely case there's an interrupt overwrite in the MADT ACPI table. Finally prevent the passthrough of the GSI that belongs to the PIT, since interrupts will be generated by the emulated PIT instead of the physical one. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/domain.c | 5 +++-- xen/arch/x86/emul-i8254.c | 12 +++++++++--- xen/arch/x86/hvm/vioapic.c | 9 ++++++++- 3 files changed, 20 insertions(+), 6 deletions(-)