Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 21/08/15 09:41, Jan Beulich wrote:
> While moving our XenoLinux patches to 4.2-rc I noticed bogus "already
> mapped" messages resulting from Linux (legitimately) writing RTEs with
> only the mask bit set. Clearly we shouldn't even attempt to create a
> pIRQ <-> IRQ mapping from such RTEs.

Oops.  Agreed.

>
> In the course of this I also found that the respective message isn't
> really useful without also printing the pre-existing mapping. And I
> noticed that map_domain_pirq() allowed IRQ0 to get through, despite us
> never allowing and domain to control that interrupt.

s/and/a/ ? (I can't quite parse the original statement)

Also, doesn't irq_access_permitted() catch the irq0 case?

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> with one suggestion

>
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2371,9 +2371,14 @@ int ioapic_guest_write(unsigned long phy
>       * pirq and irq mapping. Where the GSI is greater than 256, we assume
>       * that dom0 pirq == irq.
>       */
> -    pirq = (irq >= 256) ? irq : rte.vector;
> -    if ( (pirq < 0) || (pirq >= hardware_domain->nr_pirqs) )
> -        return -EINVAL;
> +    if ( !rte.mask )
> +    {
> +        pirq = (irq >= 256) ? irq : rte.vector;
> +        if ( pirq >= hardware_domain->nr_pirqs )
> +            return -EINVAL;
> +    }
> +    else
> +        pirq = -1;
>      
>      if ( desc->action )
>      {
> @@ -2408,12 +2413,15 @@ int ioapic_guest_write(unsigned long phy
>  
>          printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq);
>      }
> -    spin_lock(&hardware_domain->event_lock);
> -    ret = map_domain_pirq(hardware_domain, pirq, irq,
> -            MAP_PIRQ_TYPE_GSI, NULL);
> -    spin_unlock(&hardware_domain->event_lock);
> -    if ( ret < 0 )
> -        return ret;
> +    if ( pirq >= 0 )
> +    {
> +        spin_lock(&hardware_domain->event_lock);
> +        ret = map_domain_pirq(hardware_domain, pirq, irq,
> +                              MAP_PIRQ_TYPE_GSI, NULL);
> +        spin_unlock(&hardware_domain->event_lock);
> +        if ( ret < 0 )
> +            return ret;
> +    }
>  
>      spin_lock_irqsave(&ioapic_lock, flags);
>      /* Set the correct irq-handling type. */
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1906,7 +1906,7 @@ int map_domain_pirq(
>      if ( !irq_access_permitted(current->domain, irq))
>          return -EPERM;
>  

I would be tempted to put a comment here stating that irq0 is definitely
a xen-reserved irq.  Otherwise, it is odd to have the mismatch between
pirq and irq.

~Andrew

> -    if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs )
> +    if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs )
>      {
>          dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n",
>                  d->domain_id, pirq, irq);
> @@ -1919,8 +1919,9 @@ int map_domain_pirq(
>      if ( (old_irq > 0 && (old_irq != irq) ) ||
>           (old_pirq && (old_pirq != pirq)) )
>      {
> -        dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n",
> -                d->domain_id, pirq, irq);
> +        dprintk(XENLOG_G_WARNING,
> +                "dom%d: pirq %d or irq %d already mapped (%d,%d)\n",
> +                d->domain_id, pirq, irq, old_pirq, old_irq);
>          return 0;
>      }
>  
>
>
>