Re: [Xen-devel] [PATCH for-4.13 v4 3/3] x86/vioapic: sync PIR to IRR when modifying entries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.11.2019 16:59, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -212,6 +212,44 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, 
> unsigned int trig,
>      return ret;
>  }
>  
> +static inline int pit_channel0_enabled(void)
> +{
> +    return pt_active(&current->domain->arch.vpit.pt0);
> +}

Rather than moving it up here, could I talk you into taking the
opportunity and move it into hvm/vpt.h? This really isn't a
vIO-APIC function, and hence should never have been placed in
this file.

> +static void sync_vcpus_pir(struct domain *d, union vioapic_redir_entry *ent,

const (twice)?

> +                           unsigned int irq)
> +{
> +    DECLARE_BITMAP(vcpus, MAX_VIRT_CPUS) = { };

Same comment here as for patch 2.

> +    switch ( ent->fields.delivery_mode )
> +    {
> +    case dest_LowestPrio:
> +    case dest_Fixed:
> +#ifdef IRQ0_SPECIAL_ROUTING
> +        if ( (irq == hvm_isa_irq_to_gsi(0)) && pit_channel0_enabled() )
> +        {
> +            __set_bit(0, vcpus);
> +            break;
> +        }
> +#endif
> +        hvm_intr_get_dests(d, ent->fields.dest_id, ent->fields.dest_mode,
> +                           ent->fields.delivery_mode, vcpus);
> +        break;
> +
> +    case dest_NMI:
> +        /* Nothing to do, NMIs are not signaled on the PIR. */
> +        break;
> +
> +    default:
> +        gdprintk(XENLOG_WARNING, "unsupported delivery mode %02u\n",
> +                 ent->fields.delivery_mode);
> +        break;
> +    }
> +
> +    domain_sync_vlapic_pir(d, vcpus);
> +}

I realize it may be less of a risk for 4.13 this way, but there's
quite a bit of logic duplication with vioapic_deliver(), which
would be nice to be taken care of by breaking out similar logic
into one or more helpers.

> @@ -235,6 +273,9 @@ static void vioapic_write_redirent(
>      pent = &vioapic->redirtbl[idx];
>      ent  = *pent;
>  
> +    if ( !ent.fields.mask && hvm_funcs.deliver_posted_intr )
> +        sync_vcpus_pir(d, pent, vioapic->base_gsi + idx);

Just like for MSI, don't you want to do this _after_ having
updated everything? It may not matter much because this is
within a region with the necessary lock held, but it would at
least look better from an abstract pov.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel