[PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2803 bytes --]

Rather than assuming only PV guests need special treatment (and
dealing with that directly when an IRQ gets set up), keep all guest MSI
IRQs masked until either the (HVM) guest unmasks them via vMSI or the
(PV, PVHVM, or PVH) guest sets up an event channel for it.

To not further clutter the common evtchn_bind_pirq() with x86-specific
code, introduce an arch_evtchn_bind_pirq() hook instead.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2502,6 +2502,25 @@ int unmap_domain_pirq_emuirq(struct doma
     return ret;
 }
 
+void arch_evtchn_bind_pirq(struct domain *d, int pirq)
+{
+    int irq = domain_pirq_to_irq(d, pirq);
+    struct irq_desc *desc;
+    unsigned long flags;
+
+    if ( irq <= 0 )
+        return;
+
+    if ( is_hvm_domain(d) )
+        map_domain_emuirq_pirq(d, pirq, IRQ_PT);
+
+    desc = irq_to_desc(irq);
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( desc->msi_desc )
+        guest_mask_msi_irq(desc, 0);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
 bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
 {
     return is_hvm_domain(d) && pirq &&
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -422,10 +422,7 @@ void guest_mask_msi_irq(struct irq_desc 
 
 static unsigned int startup_msi_irq(struct irq_desc *desc)
 {
-    bool_t guest_masked = (desc->status & IRQ_GUEST) &&
-                          is_hvm_domain(desc->msi_desc->dev->domain);
-
-    msi_set_mask_bit(desc, 0, guest_masked);
+    msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST));
     return 0;
 }
 
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -502,10 +502,7 @@ static long evtchn_bind_pirq(evtchn_bind
 
     bind->port = port;
 
-#ifdef CONFIG_X86
-    if ( is_hvm_domain(d) && domain_pirq_to_irq(d, pirq) > 0 )
-        map_domain_emuirq_pirq(d, pirq, IRQ_PT);
-#endif
+    arch_evtchn_bind_pirq(d, pirq);
 
  out:
     spin_unlock(&d->event_lock);
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d, 
 
 void arch_move_irqs(struct vcpu *v);
 
+#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
+
 /* Set IRQ type for an SPI */
 int irq_set_spi_type(unsigned int spi, unsigned int type);
 
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
 unsigned int arch_hwdom_irqs(domid_t);
 #endif
 
+#ifndef arch_evtchn_bind_pirq
+void arch_evtchn_bind_pirq(struct domain *, int pirq);
+#endif
+
 #endif /* __XEN_IRQ_H__ */




[-- Attachment #2: x86-MSI-pv-unmask.patch --]
[-- Type: text/plain, Size: 2865 bytes --]

x86/MSI: fix guest unmasking when handling IRQ via event channel

Rather than assuming only PV guests need special treatment (and
dealing with that directly when an IRQ gets set up), keep all guest MSI
IRQs masked until either the (HVM) guest unmasks them via vMSI or the
(PV, PVHVM, or PVH) guest sets up an event channel for it.

To not further clutter the common evtchn_bind_pirq() with x86-specific
code, introduce an arch_evtchn_bind_pirq() hook instead.

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Sander Eikelenboom <linux@eikelenboom.it>

--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2502,6 +2502,25 @@ int unmap_domain_pirq_emuirq(struct doma
     return ret;
 }
 
+void arch_evtchn_bind_pirq(struct domain *d, int pirq)
+{
+    int irq = domain_pirq_to_irq(d, pirq);
+    struct irq_desc *desc;
+    unsigned long flags;
+
+    if ( irq <= 0 )
+        return;
+
+    if ( is_hvm_domain(d) )
+        map_domain_emuirq_pirq(d, pirq, IRQ_PT);
+
+    desc = irq_to_desc(irq);
+    spin_lock_irqsave(&desc->lock, flags);
+    if ( desc->msi_desc )
+        guest_mask_msi_irq(desc, 0);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
 bool_t hvm_domain_use_pirq(const struct domain *d, const struct pirq *pirq)
 {
     return is_hvm_domain(d) && pirq &&
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -422,10 +422,7 @@ void guest_mask_msi_irq(struct irq_desc 
 
 static unsigned int startup_msi_irq(struct irq_desc *desc)
 {
-    bool_t guest_masked = (desc->status & IRQ_GUEST) &&
-                          is_hvm_domain(desc->msi_desc->dev->domain);
-
-    msi_set_mask_bit(desc, 0, guest_masked);
+    msi_set_mask_bit(desc, 0, !!(desc->status & IRQ_GUEST));
     return 0;
 }
 
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -502,10 +502,7 @@ static long evtchn_bind_pirq(evtchn_bind
 
     bind->port = port;
 
-#ifdef CONFIG_X86
-    if ( is_hvm_domain(d) && domain_pirq_to_irq(d, pirq) > 0 )
-        map_domain_emuirq_pirq(d, pirq, IRQ_PT);
-#endif
+    arch_evtchn_bind_pirq(d, pirq);
 
  out:
     spin_unlock(&d->event_lock);
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d, 
 
 void arch_move_irqs(struct vcpu *v);
 
+#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
+
 /* Set IRQ type for an SPI */
 int irq_set_spi_type(unsigned int spi, unsigned int type);
 
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
 unsigned int arch_hwdom_irqs(domid_t);
 #endif
 
+#ifndef arch_evtchn_bind_pirq
+void arch_evtchn_bind_pirq(struct domain *, int pirq);
+#endif
+
 #endif /* __XEN_IRQ_H__ */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Andrew Cooper]

On 08/07/2015 09:56, Jan Beulich wrote:
> Rather than assuming only PV guests need special treatment (and
> dealing with that directly when an IRQ gets set up), keep all guest MSI
> IRQs masked until either the (HVM) guest unmasks them via vMSI or the
> (PV, PVHVM, or PVH) guest sets up an event channel for it.
>
> To not further clutter the common evtchn_bind_pirq() with x86-specific
> code, introduce an arch_evtchn_bind_pirq() hook instead.
>
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>

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

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Julien Grall]

Hi,

On 08/07/2015 09:56, Jan Beulich wrote:
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d,
>
>   void arch_move_irqs(struct vcpu *v);
>
> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
> +

This addition is here in order to ensure that d and pirq are evaluated, 
right?

If so, I didn't find it obvious to understand. Why didn't you use a 
static inline? Or maybe add a comment explicitly say this is not 
implemented.

Regards,

-- 
Julien Grall

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[David Vrabel]

On 08/07/15 09:56, Jan Beulich wrote:
> Rather than assuming only PV guests need special treatment (and
> dealing with that directly when an IRQ gets set up), keep all guest MSI
> IRQs masked until either the (HVM) guest unmasks them via vMSI or the
> (PV, PVHVM, or PVH) guest sets up an event channel for it.
> 
> To not further clutter the common evtchn_bind_pirq() with x86-specific
> code, introduce an arch_evtchn_bind_pirq() hook instead.

Can you describe the symptoms of the bug being fixed here?

> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d, 
>  
>  void arch_move_irqs(struct vcpu *v);
>  
> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))

Would this be better as a inline function?

> +
>  /* Set IRQ type for an SPI */
>  int irq_set_spi_type(unsigned int spi, unsigned int type);
>  
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
>  unsigned int arch_hwdom_irqs(domid_t);
>  #endif
>  
> +#ifndef arch_evtchn_bind_pirq
> +void arch_evtchn_bind_pirq(struct domain *, int pirq);

... moving this into xen/include/asm-x86/irq.h

David

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Jan Beulich]

>>> On 08.07.15 at 11:07, <julien.grall@citrix.com> wrote:
> On 08/07/2015 09:56, Jan Beulich wrote:
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d,
>>
>>   void arch_move_irqs(struct vcpu *v);
>>
>> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
>> +
> 
> This addition is here in order to ensure that d and pirq are evaluated, 
> right?

Sure.

> If so, I didn't find it obvious to understand. Why didn't you use a 
> static inline? Or maybe add a comment explicitly say this is not 
> implemented.

A static inline could be used in this case, yes. But I see no
significant advantages. As to the comment - it is implemented,
it's just a no-op. And stating that it is a no-op would be
redundant with it obviously being so by looking at it.

Jan

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Jan Beulich]

>>> On 08.07.15 at 11:39, <david.vrabel@citrix.com> wrote:
> On 08/07/15 09:56, Jan Beulich wrote:
>> Rather than assuming only PV guests need special treatment (and
>> dealing with that directly when an IRQ gets set up), keep all guest MSI
>> IRQs masked until either the (HVM) guest unmasks them via vMSI or the
>> (PV, PVHVM, or PVH) guest sets up an event channel for it.
>> 
>> To not further clutter the common evtchn_bind_pirq() with x86-specific
>> code, introduce an arch_evtchn_bind_pirq() hook instead.
> 
> Can you describe the symptoms of the bug being fixed here?

Interrupts simply didn't get unmasked for PVHVM Linux guests.

>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d, 
>>  
>>  void arch_move_irqs(struct vcpu *v);
>>  
>> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
> 
> Would this be better as a inline function?
> 
>> +
>>  /* Set IRQ type for an SPI */
>>  int irq_set_spi_type(unsigned int spi, unsigned int type);
>>  
>> --- a/xen/include/xen/irq.h
>> +++ b/xen/include/xen/irq.h
>> @@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
>>  unsigned int arch_hwdom_irqs(domid_t);
>>  #endif
>>  
>> +#ifndef arch_evtchn_bind_pirq
>> +void arch_evtchn_bind_pirq(struct domain *, int pirq);
> 
> ... moving this into xen/include/asm-x86/irq.h

Oh, right, (also to Julien) - this is exactly the reason I do not want it
to be an inline function for ARM: I want the declaration here, not
replicated in every interested arch's header.

Jan

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Julien Grall]

On 08/07/2015 11:55, Jan Beulich wrote:
>>>> On 08.07.15 at 11:07, <julien.grall@citrix.com> wrote:
>> On 08/07/2015 09:56, Jan Beulich wrote:
>>> --- a/xen/include/asm-arm/irq.h
>>> +++ b/xen/include/asm-arm/irq.h
>>> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d,
>>>
>>>    void arch_move_irqs(struct vcpu *v);
>>>
>>> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
>>> +
>>
>> This addition is here in order to ensure that d and pirq are evaluated,
>> right?
>
> Sure.
>
>> If so, I didn't find it obvious to understand. Why didn't you use a
>> static inline? Or maybe add a comment explicitly say this is not
>> implemented.
>
> A static inline could be used in this case, yes. But I see no
> significant advantages. As to the comment - it is implemented,
> it's just a no-op. And stating that it is a no-op would be
> redundant with it obviously being so by looking at it.

It's not so obvious as I asked about it.

The first thing I saw was (d) + (pirq) and I though : "Why do we want to 
add a domain with a pirq?". I only see after the (void) and it just 
because I remembered we talked about similar case a year ago.

Having a comment doesn't hurt and help the comprehension.

-- 
Julien Grall

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[David Vrabel]

On 08/07/15 11:58, Jan Beulich wrote:
>>>> On 08.07.15 at 11:39, <david.vrabel@citrix.com> wrote:
>> On 08/07/15 09:56, Jan Beulich wrote:
>>> Rather than assuming only PV guests need special treatment (and
>>> dealing with that directly when an IRQ gets set up), keep all guest MSI
>>> IRQs masked until either the (HVM) guest unmasks them via vMSI or the
>>> (PV, PVHVM, or PVH) guest sets up an event channel for it.
>>>
>>> To not further clutter the common evtchn_bind_pirq() with x86-specific
>>> code, introduce an arch_evtchn_bind_pirq() hook instead.
>>
>> Can you describe the symptoms of the bug being fixed here?
> 
> Interrupts simply didn't get unmasked for PVHVM Linux guests.
> 
>>> --- a/xen/include/asm-arm/irq.h
>>> +++ b/xen/include/asm-arm/irq.h
>>> @@ -47,6 +47,8 @@ int release_guest_irq(struct domain *d, 
>>>  
>>>  void arch_move_irqs(struct vcpu *v);
>>>  
>>> +#define arch_evtchn_bind_pirq(d, pirq) ((void)((d) + (pirq)))
>>
>> Would this be better as a inline function?
>>
>>> +
>>>  /* Set IRQ type for an SPI */
>>>  int irq_set_spi_type(unsigned int spi, unsigned int type);
>>>  
>>> --- a/xen/include/xen/irq.h
>>> +++ b/xen/include/xen/irq.h
>>> @@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
>>>  unsigned int arch_hwdom_irqs(domid_t);
>>>  #endif
>>>  
>>> +#ifndef arch_evtchn_bind_pirq
>>> +void arch_evtchn_bind_pirq(struct domain *, int pirq);
>>
>> ... moving this into xen/include/asm-x86/irq.h
> 
> Oh, right, (also to Julien) - this is exactly the reason I do not want it
> to be an inline function for ARM: I want the declaration here, not
> replicated in every interested arch's header.

Ok.

FWIW, with this requirement I would (instead of the macros) add a weak
arch_evtchn_bind_pirq() that's a no-op.

David

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Jan Beulich]

>>> On 08.07.15 at 13:14, <david.vrabel@citrix.com> wrote:
> On 08/07/15 11:58, Jan Beulich wrote:
>>>>> On 08.07.15 at 11:39, <david.vrabel@citrix.com> wrote:
>>> On 08/07/15 09:56, Jan Beulich wrote:
>>>> +
>>>>  /* Set IRQ type for an SPI */
>>>>  int irq_set_spi_type(unsigned int spi, unsigned int type);
>>>>  
>>>> --- a/xen/include/xen/irq.h
>>>> +++ b/xen/include/xen/irq.h
>>>> @@ -172,4 +172,8 @@ unsigned int set_desc_affinity(struct ir
>>>>  unsigned int arch_hwdom_irqs(domid_t);
>>>>  #endif
>>>>  
>>>> +#ifndef arch_evtchn_bind_pirq
>>>> +void arch_evtchn_bind_pirq(struct domain *, int pirq);
>>>
>>> ... moving this into xen/include/asm-x86/irq.h
>> 
>> Oh, right, (also to Julien) - this is exactly the reason I do not want it
>> to be an inline function for ARM: I want the declaration here, not
>> replicated in every interested arch's header.
> 
> Ok.
> 
> FWIW, with this requirement I would (instead of the macros) add a weak
> arch_evtchn_bind_pirq() that's a no-op.

Yeah, that's how Linux likes to do it. But we learned the hard way
that weak conflicts with our making symbols hidden by default, so
no, weak is not an option either I'm afraid.

Jan

Re: [PATCH] x86/MSI: fix guest unmasking when handling IRQ via event channel

[Ian Campbell]

On Wed, 2015-07-08 at 09:56 +0100, Jan Beulich wrote:
> Rather than assuming only PV guests need special treatment (and
> dealing with that directly when an IRQ gets set up), keep all guest MSI
> IRQs masked until either the (HVM) guest unmasks them via vMSI or the
> (PV, PVHVM, or PVH) guest sets up an event channel for it.
> 
> To not further clutter the common evtchn_bind_pirq() with x86-specific
> code, introduce an arch_evtchn_bind_pirq() hook instead.
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Sander Eikelenboom <linux@eikelenboom.it>


For the ARM impact; Acked-by: Ian Campbell <ian.campbell@citrix.com>