Re: [PATCH for-4.14 8/8] x86/hvm: enable emulated PIT for PVH dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.