Re: [Xen-devel] [RFC PATCH 09/49] ARM: VGIC: change to level-IRQ compatible IRQ injection interface

2018-02-13 Thread Julien Grall 

Hi,

On 12/02/18 14:24, Andre Przywara wrote:

What would you expect the caller to do on error? Except printing an
error message?


I don't know either. Comparing this to hardware, an IRQ is usually
fire-and-forget (separating the interrupt line from the interrupt state
here), so a device doesn't really handle the case when an IRQ does not
make it through (it can't know easily anyway). However the whole state
machine might get busted in the process (if no one lowers the line, for
instance).
So looking at this printing a message looks like the best choice.

I checked all users of vgic_inject_irq(), at the moment all IRQ numbers
passed in look safe: they are either hardcoded (timer, evtchn) or
validated before (hardware IRQs, when they are tied to a virtual IRQ).
So indeed we *should* never see an invalid IRQ number, at the moment.
I need to check how this changes with the ITS, though.

So we could change the prototype (back) to void, but print some error
message if the vgic_get_irq() call fails within vgic_inject_irq().


Sounds good to me. Make sure to have those message using the log level 
guest debug message. I might even be tempt to use dgprintk(...) here so 
they get dropped in non-debug build.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 09/49] ARM: VGIC: change to level-IRQ compatible IRQ injection interface

2018-02-12 Thread Julien Grall 



On 12/02/18 11:59, Andre Przywara wrote:

Hi,


Hi Andre,


On 12/02/18 11:15, Julien Grall wrote:

Hi Andre,

On 09/02/18 14:38, Andre Przywara wrote:

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5f47aa84a9..2fc6e19625 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct
vcpu *new, unsigned int irq)
   vgic_remove_irq_from_queues(old, p);
   irq_set_affinity(p->desc, cpumask_of(new->processor));
   spin_unlock_irqrestore(>arch.vgic.lock, flags);
-    vgic_vcpu_inject_irq(new, irq);
+    vgic_inject_irq(new->domain, new, irq, true);
   return true;
   }
   /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
@@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir,
enum gic_sgi_mode irqmode,
   sgir, target->list);
   continue;
   }
-    vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
+    vgic_inject_irq(d, d->vcpu[vcpuid], virq, true);
   }
   break;
   case SGI_TARGET_OTHERS:
@@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t
sgir, enum gic_sgi_mode irqmode,
   {
   if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
    is_vcpu_online(d->vcpu[i]) )
-    vgic_vcpu_inject_irq(d->vcpu[i], virq);
+    vgic_inject_irq(d, d->vcpu[i], virq, true);
   }
   break;
   case SGI_TARGET_SELF:
   perfc_incr(vgic_sgi_self);
-    vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq);
+    vgic_inject_irq(d, current, virq, true);
   break;
   default:
   gprintk(XENLOG_WARNING,
@@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v,
struct pending_irq *p)
   gic_remove_from_lr_pending(v, p);
   }
   -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
+int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+    bool level)


Looking at the code after the series has been applied, no one is caring
about the return value of vgic_inject_irq. So what is the rationale
behind changing the return type from void to int?


The KVM version returns an error value, in particular when:
- the VGIC has not been initialized yet
- we can't determine the VCPU for a private interrupt
- the interrupt ID is invalid (SPI beyond limit, not mapped LPI)
In the moment it's not very useful for Xen: the first two conditions
don't really happen, consequently I removed those checks. But the third
check may become interesting once we get LPIs. Also since Xen currently
uses a void prototype for injection, *this* patch *now* doesn't exploit
the newly gained possibility of properly handling errors. From briefly
checking all the users, all of them seem to be in void functions, so
indeed an error return is not very useful.
The reasons I kept it in was to allow introduction of checks later. I
think having a function returning an error where some users ignore this
is better than the other way round.


I don't think it is much better. This is a way to expose yet another 
security issue because the return is not correctly checked (see XSA-246 
for instance). Any return value should be checked or have a comment 
explaining why it is fine.




So of course I can easily make this void, but I wonder what we do in
those cases where the SPI is not valid, for instance? Shall we print
some (rate-limited) warning?


I can understand why KVM needs such interface as the interrupt 
controller may be emulated QEMU. But I can't see why a SPI would not be 
valid in Xen context (except programming error). So could give an example?


What would you expect the caller to do on error? Except printing an 
error message?


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 09/49] ARM: VGIC: change to level-IRQ compatible IRQ injection interface

2018-02-12 Thread Andre Przywara 
Hi,

On 12/02/18 11:15, Julien Grall wrote:
> Hi Andre,
> 
> On 09/02/18 14:38, Andre Przywara wrote:
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 5f47aa84a9..2fc6e19625 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct
>> vcpu *new, unsigned int irq)
>>   vgic_remove_irq_from_queues(old, p);
>>   irq_set_affinity(p->desc, cpumask_of(new->processor));
>>   spin_unlock_irqrestore(>arch.vgic.lock, flags);
>> -    vgic_vcpu_inject_irq(new, irq);
>> +    vgic_inject_irq(new->domain, new, irq, true);
>>   return true;
>>   }
>>   /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
>> @@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir,
>> enum gic_sgi_mode irqmode,
>>   sgir, target->list);
>>   continue;
>>   }
>> -    vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
>> +    vgic_inject_irq(d, d->vcpu[vcpuid], virq, true);
>>   }
>>   break;
>>   case SGI_TARGET_OTHERS:
>> @@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t
>> sgir, enum gic_sgi_mode irqmode,
>>   {
>>   if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
>>    is_vcpu_online(d->vcpu[i]) )
>> -    vgic_vcpu_inject_irq(d->vcpu[i], virq);
>> +    vgic_inject_irq(d, d->vcpu[i], virq, true);
>>   }
>>   break;
>>   case SGI_TARGET_SELF:
>>   perfc_incr(vgic_sgi_self);
>> -    vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq);
>> +    vgic_inject_irq(d, current, virq, true);
>>   break;
>>   default:
>>   gprintk(XENLOG_WARNING,
>> @@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v,
>> struct pending_irq *p)
>>   gic_remove_from_lr_pending(v, p);
>>   }
>>   -void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
>> +int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
>> +    bool level)
> 
> Looking at the code after the series has been applied, no one is caring
> about the return value of vgic_inject_irq. So what is the rationale
> behind changing the return type from void to int?

The KVM version returns an error value, in particular when:
- the VGIC has not been initialized yet
- we can't determine the VCPU for a private interrupt
- the interrupt ID is invalid (SPI beyond limit, not mapped LPI)
In the moment it's not very useful for Xen: the first two conditions
don't really happen, consequently I removed those checks. But the third
check may become interesting once we get LPIs. Also since Xen currently
uses a void prototype for injection, *this* patch *now* doesn't exploit
the newly gained possibility of properly handling errors. From briefly
checking all the users, all of them seem to be in void functions, so
indeed an error return is not very useful.
The reasons I kept it in was to allow introduction of checks later. I
think having a function returning an error where some users ignore this
is better than the other way round.

So of course I can easily make this void, but I wonder what we do in
those cases where the SPI is not valid, for instance? Shall we print
some (rate-limited) warning?

Cheers,
Andre.

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

Re: [Xen-devel] [RFC PATCH 09/49] ARM: VGIC: change to level-IRQ compatible IRQ injection interface

2018-02-12 Thread Julien Grall 

Hi Andre,

On 09/02/18 14:38, Andre Przywara wrote:

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5f47aa84a9..2fc6e19625 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -285,7 +285,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
unsigned int irq)
  vgic_remove_irq_from_queues(old, p);
  irq_set_affinity(p->desc, cpumask_of(new->processor));
  spin_unlock_irqrestore(>arch.vgic.lock, flags);
-vgic_vcpu_inject_irq(new, irq);
+vgic_inject_irq(new->domain, new, irq, true);
  return true;
  }
  /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
@@ -444,7 +444,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
gic_sgi_mode irqmode,
  sgir, target->list);
  continue;
  }
-vgic_vcpu_inject_irq(d->vcpu[vcpuid], virq);
+vgic_inject_irq(d, d->vcpu[vcpuid], virq, true);
  }
  break;
  case SGI_TARGET_OTHERS:
@@ -453,12 +453,12 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
gic_sgi_mode irqmode,
  {
  if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
   is_vcpu_online(d->vcpu[i]) )
-vgic_vcpu_inject_irq(d->vcpu[i], virq);
+vgic_inject_irq(d, d->vcpu[i], virq, true);
  }
  break;
  case SGI_TARGET_SELF:
  perfc_incr(vgic_sgi_self);
-vgic_vcpu_inject_irq(d->vcpu[current->vcpu_id], virq);
+vgic_inject_irq(d, current, virq, true);
  break;
  default:
  gprintk(XENLOG_WARNING,
@@ -518,13 +518,29 @@ void vgic_remove_irq_from_queues(struct vcpu *v, struct 
pending_irq *p)
  gic_remove_from_lr_pending(v, p);
  }
  
-void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)

+int vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+bool level)


Looking at the code after the series has been applied, no one is caring 
about the return value of vgic_inject_irq. So what is the rationale 
behind changing the return type from void to int?


Cheers,

--
Julien Grall

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