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
