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

[Jan Beulich <jbeulich@xxxxxxxx>]

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