[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.