Re: [PATCH v5 1/2] arm/arm64: KVM: Detect vGIC presence at runtime

[Peter Maydell]

On 21 April 2016 at 23:35, Alexander Graf  wrote:
> It might make sense to have a generic "system register couldn't get
> handled" exit code anyway. If nothing else, at least to display
> unhandled registers in the qemu context where they appear rather than in
> the kernel log (which a guest could deliberately clutter as it stands
> today).

Unhandled sysregs UNDEF so they get logged by the guest if anywhere,
I think.

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v5 1/2] arm/arm64: KVM: Detect vGIC presence at runtime

[Alexander Graf]

On 22.04.16 00:04, Peter Maydell wrote:
> On 21 April 2016 at 22:41, Alexander Graf  wrote:
>> So effectively all we'd need is to set CNTHCTL_EL2.EL1PCEN to 0 for
>> guests that have no in-kernel irqchip, no? We should then trap on all
>> timer accesses and be able to emulate them in user space where we can
>> inject IRQs into an emulated GIC again.
> 
> You'd still need to define a new userspace ABI for "we stopped
> for a system register trap" and the userspace code to emulate
> the timers as part of KVM rather than as part of TCG, which seems
> like a lot of effort for a mode that you really don't want to be
> using...

It might make sense to have a generic "system register couldn't get
handled" exit code anyway. If nothing else, at least to display
unhandled registers in the qemu context where they appear rather than in
the kernel log (which a guest could deliberately clutter as it stands
today).

With such an interface in place, the rest would only be a few lines of glue.

> For GICv3 it gets trickier again because the interface between
> the interrupt controller and the CPU is no longer as simple
> as "an FIQ line and an IRQ line". (In particular the interrupt
> controller needs to know the CPU's current exception level and
> security state to know if it should raise IRQ or FIQ, which means
> that it needs to be told every time the CPU changes EL. I haven't
> yet figured out if I should model this in the QEMU emulated GICv3
> by having a backdoor callback function for this or by biting
> the bullet and putting the GICv3 cpu interface really in the
> CPU with a properly modelled equivalent of the stream protocol...)

We moved the lapic into target-i386 as well, no? Given that it really is
very close to the cpu these days it might not be a bad idea.


Alex
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v5 1/2] arm/arm64: KVM: Detect vGIC presence at runtime

[Peter Maydell]

On 21 April 2016 at 22:41, Alexander Graf  wrote:
> So effectively all we'd need is to set CNTHCTL_EL2.EL1PCEN to 0 for
> guests that have no in-kernel irqchip, no? We should then trap on all
> timer accesses and be able to emulate them in user space where we can
> inject IRQs into an emulated GIC again.

You'd still need to define a new userspace ABI for "we stopped
for a system register trap" and the userspace code to emulate
the timers as part of KVM rather than as part of TCG, which seems
like a lot of effort for a mode that you really don't want to be
using...

For GICv3 it gets trickier again because the interface between
the interrupt controller and the CPU is no longer as simple
as "an FIQ line and an IRQ line". (In particular the interrupt
controller needs to know the CPU's current exception level and
security state to know if it should raise IRQ or FIQ, which means
that it needs to be told every time the CPU changes EL. I haven't
yet figured out if I should model this in the QEMU emulated GICv3
by having a backdoor callback function for this or by biting
the bullet and putting the GICv3 cpu interface really in the
CPU with a properly modelled equivalent of the stream protocol...)

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v7 1/6] KVM: arm/arm64: Add VGICv3 save/restore API documentation

[Peter Maydell]

On 21 April 2016 at 19:30, Christoffer Dall  wrote:
> So I agree about the latch state, that should be exported, if nothing
> else so that a VM can't use this particular change to detect a
> migration, for example.
>
> However, for the input level, I really do see this as a wire state
> between userspace and the kernel, and as something that userspace should
> re-establish after migration, since userspace already knows what the
> line level is, why does it need to pull it out of the kernel again?

I guess userspace could track the line level as well as telling the
kernel about it, but we would still need an API for telling the
kernel "here is your line level state after a migration", because
that's not the same as "the line level just changed because the
device signaled an interrupt". (For the former, we just want to set
your 'level' struct field value. For the latter, a 0->1 level change
may mean "set pending"; but for migration we're already telling you
the current pending state via a different ioctl and don't want to
mess that up when we're telling you about the 'level' info.)
And if we have to have migration API for directly writing level bits we
might as well have the corresponding directly-read-level-bits one...

thanks
-- PMM
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Christoffer Dall]

On Thu, Apr 21, 2016 at 07:41:01PM +0200, Eric Auger wrote:
> Hi Andre,
> On 04/15/2016 04:04 PM, Andre Przywara wrote:
> > From: Christoffer Dall 
> > 
> > The communication of a Linux IRQ number from outside the VGIC to the
> > vgic was a leftover from the day when the vgic code cared about how a
> > particular device injects virtual interrupts mapped to a physical
> > interrupt.
> > 
> > We can safely remove this notion, leaving all physical IRQ handling to
> > be done in the device driver (the arch timer in this case), which makes
> > room for a saner API for the new VGIC.
> > 
> > Signed-off-by: Christoffer Dall 
> > Signed-off-by: Andre Przywara 
> > ---
> >  include/kvm/arm_vgic.h|  3 +--
> >  virt/kvm/arm/arch_timer.c | 22 --
> >  virt/kvm/arm/vgic.c   | 20 ++--
> >  3 files changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> > index 43eeb18..49c559e 100644
> > --- a/include/kvm/arm_vgic.h
> > +++ b/include/kvm/arm_vgic.h
> > @@ -157,7 +157,6 @@ struct vgic_io_device {
> >  struct irq_phys_map {
> > u32 virt_irq;
> > u32 phys_irq;
> > -   u32 irq;
> >  };
> >  
> >  struct irq_phys_map_entry {
> > @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int 
> > cpuid,
> >  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
> >  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> >  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> > -  int virt_irq, int irq);
> > +  int virt_irq, int phys_irq);
> >  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
> >  
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index b4d96b1..cfdf88f 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
> > if (timer->active_cleared_last && !phys_active)
> > return;
> >  
> > -   ret = irq_set_irqchip_state(timer->map->irq,
> > +   ret = irq_set_irqchip_state(host_vtimer_irq,
> > IRQCHIP_STATE_ACTIVE,
> > phys_active);
> > WARN_ON(ret);
> > @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> >  {
> > struct arch_timer_cpu *timer = >arch.timer_cpu;
> > struct irq_phys_map *map;
> > +   struct irq_desc *desc;
> > +   struct irq_data *data;
> > +   int phys_irq;
> >  
> > /*
> >  * The vcpu timer irq number cannot be determined in
> > @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
> > kvm_timer_update_state(vcpu);
> >  
> > /*
> > +* Find the physical IRQ number corresponding to the host_vtimer_irq
> > +*/
> > +   desc = irq_to_desc(host_vtimer_irq);
> > +   if (!desc) {
> can this really happen?

this is just moving the logic.  We had this check before, so I assume
so...

> > +   kvm_err("%s: no interrupt descriptor\n", __func__);
> > +   return -EINVAL;
> > +   }
> > +
> > +   data = irq_desc_get_irq_data(desc);
> > +   while (data->parent_data)
> > +   data = data->parent_data;
> > +
> > +   phys_irq = data->hwirq;
> > +
> > +   /*
> >  * Tell the VGIC that the virtual interrupt is tied to a
> >  * physical interrupt. We do that once per VCPU.
> >  */
> > -   map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
> > +   map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
> > if (WARN_ON(IS_ERR(map)))
> > return PTR_ERR(map);
> >  
> > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> > index 2d7ae35..ac5838b 100644
> > --- a/virt/kvm/arm/vgic.c
> > +++ b/virt/kvm/arm/vgic.c
> > @@ -1723,27 +1723,13 @@ static struct list_head 
> > *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
> >   * Returns a valid pointer on success, and an error pointer otherwise
> >   */
> the doc comment must be updated
>  * @irq: The Linux IRQ number
> 
> Besides
> 
> Reviewed-by: Eric Auger 
> 
Thanks!

Andre, let me know if you need me to provide an updated patch or if you
can just tweak that comment.

-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v7 1/6] KVM: arm/arm64: Add VGICv3 save/restore API documentation

[Christoffer Dall]

On Wed, Apr 20, 2016 at 02:28:05PM +0100, Peter Maydell wrote:
> On 20 April 2016 at 11:59, Christoffer Dall  
> wrote:
> > On Friday, 15 April 2016, Peter Maydell  wrote:
> >> Nothing in here describes a mechanism for reading or writing the
> >> current interrupt line_level state from the kernel (which doesn't
> >> matter for edge triggered interrupts but does for level triggered
> >> interrupts). Do we need accessors for these or does somebody
> >> have a good rationale for why we don't need to migrate that data?
> >> (Christoffer?)
> 
> > I thought we relied on user space to migrate its device state including the
> > interrupt line output state and set the corresponding line state to the
> > kernel, but that may have been wrong, and would rely on userspace to call
> > the IRQ_LINE ioctl again.  Would that work?
> 
> I'm not sure; it's definitely not what we do today, anyway...
> 
> > How is it again, does QEMU preserve the IRQ output line state per device?
> 
> In QEMU device models, each device has its own state and generally
> tracks the level of its input lines as necessary. When state
> restore happens, the device on the sending end restores its own
> state but doesn't reassert the irq line; the assumption is that
> the device on the receiving end restores its own state including
> the level of its input lines.
> 
> > This may just work currently because we rely on the reads/writes to SPENDR
> > to preserve the state and apparently level triggered interrupts are
> > reinjected as needed.
> 
> I think the reason it more or less works is that as you say we
> write the pending bits back to the PENDR registers. There are
> two cases where this goes wrong for level triggered interrupts,
> though:
> (1) if a device deasserts its interrupt output before the
> guest OS gets round to acknowledging the interrupt, then
> what should happen is that the interrupt stops being pending;
> since we wrote to PENDR then the latch means we'll deliver
> the guest a spurious interrupt
> (2) if a guest OS's interrupt handler doesn't quiesce the
> device such that it deasserts the interrupt output, then
> what should happen is that when the interrupt is dismissed
> it remains pending and will trigger again; since we didn't
> copy the level state into the kernel then the kernel won't
> notice and won't deliver the interrupt again
> 
> Since the distributor/redistributor is fully emulated inside
> the kernel, we have the underlying state separately for
> levels and for the pending-latch, right? We don't need to
> make it impossible to access the latch separately just
> because that's what the hardware does...
> 
So I agree about the latch state, that should be exported, if nothing
else so that a VM can't use this particular change to detect a
migration, for example.

However, for the input level, I really do see this as a wire state
between userspace and the kernel, and as something that userspace should
re-establish after migration, since userspace already knows what the
line level is, why does it need to pull it out of the kernel again?

-Christoffer
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 7/7] KVM: arm/arm64: remove irq_phys_map pointer from kvm_vgic_map_phys_irq()

[Eric Auger]

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> Now that the virtual arch timer does not care about the irq_phys_map
> anymore, let's rework kvm_vgic_map_phys_irq() to return an error
> value instead. Any reference to thap mapping can later be done by
> passing the correct combination of VCPU and virtual IRQ number.
> This makes the irq_phys_map handling completely private to the
> VGIC code.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h|  3 +--
>  virt/kvm/arm/arch_timer.c |  8 
>  virt/kvm/arm/vgic.c   | 23 +++
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 49c559e..f842d7d 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -343,8 +343,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  unsigned int virt_irq, bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
> -struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -int virt_irq, int phys_irq);
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 1598e23..270f971 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -306,10 +306,10 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>const struct kvm_irq_level *irq)
>  {
>   struct arch_timer_cpu *timer = >arch.timer_cpu;
> - struct irq_phys_map *map;
>   struct irq_desc *desc;
>   struct irq_data *data;
>   int phys_irq;
> + int ret;
>  
>   /*
>* The vcpu timer irq number cannot be determined in
> @@ -347,9 +347,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>* Tell the VGIC that the virtual interrupt is tied to a
>* physical interrupt. We do that once per VCPU.
>*/
> - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
> - if (WARN_ON(IS_ERR(map)))
> - return PTR_ERR(map);
> + ret = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
> + if (ret)
> + return ret;
>  
>   timer->virt_irq = irq->irq;
>   return 0;
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index ac5838b..00386df 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1712,29 +1712,28 @@ static struct list_head 
> *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>  /**
>   * kvm_vgic_map_phys_irq - map a virtual IRQ to a physical IRQ
>   * @vcpu: The VCPU pointer
> - * @virt_irq: The virtual irq number
> - * @irq: The Linux IRQ number
> + * @virt_irq: The virtual IRQ number for the guest
> + * @phys_irq: The hardware IRQ number of the host
nit: not belonging to this patch ;-)
>   *
>   * Establish a mapping between a guest visible irq (@virt_irq) and a
> - * Linux irq (@irq). On injection, @virt_irq will be associated with
> - * the physical interrupt represented by @irq. This mapping can be
> + * hardware irq (@phys_irq). On injection, @virt_irq will be associated with
> + * the physical interrupt represented by @phys_irq. This mapping can be
same

Besides Reviewed-by: Eric Auger 

Have a nice evening

Cheers

Eric


>   * established multiple times as long as the parameters are the same.
>   *
> - * Returns a valid pointer on success, and an error pointer otherwise
> + * Returns 0 on success or an error value otherwise.
>   */
> -struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -int virt_irq, int phys_irq)
> +int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, int virt_irq, int phys_irq)
>  {
>   struct vgic_dist *dist = >kvm->arch.vgic;
>   struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>   struct irq_phys_map *map;
>   struct irq_phys_map_entry *entry;
> -
> + int ret = 0;
>  
>   /* Create a new mapping */
>   entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>   if (!entry)
> - return ERR_PTR(-ENOMEM);
> + return -ENOMEM;
>  
>   spin_lock(>irq_phys_map_lock);
>  
> @@ -1743,7 +1742,7 @@ struct irq_phys_map *kvm_vgic_map_phys_irq(struct 
> kvm_vcpu *vcpu,
>   if (map) {
>   /* Make sure this mapping matches */
>   if (map->phys_irq != phys_irq)
> - map = ERR_PTR(-EINVAL);
> + ret = -EINVAL;
>  
>   /* Found an existing, valid mapping */
>   goto out;
> @@ -1759,9 +1758,9 @@ out:
>   spin_unlock(>irq_phys_map_lock);
>   /* If we've found a hit in the existing list, free the 

Re: [PATCH 6/7] KVM: arm/arm64: remove irq_phys_map from the arch timer

[Eric Auger]

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> Now that the interface between the arch timer and the VGIC does not
> require passing the irq_phys_map entry pointer anymore, let's remove
> it from the virtual arch timer and use the virtual IRQ number instead
> directly.
> The remaining pointer returned by kvm_vgic_map_phys_irq() will be
> removed in the following patch.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_arch_timer.h |  4 ++--
>  virt/kvm/arm/arch_timer.c| 12 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index b651aed..1ed70f6 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -53,8 +53,8 @@ struct arch_timer_cpu {
>   /* Timer IRQ */
>   struct kvm_irq_levelirq;
>  
> - /* VGIC mapping */
> - struct irq_phys_map *map;
> + /* The virtual IRQ number */
> + unsigned intvirt_irq;
what's the difference between irq.irq and virt_irq? aren't they duplicates?
>  
>   /* Active IRQ state caching */
>   boolactive_cleared_last;
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index cfdf88f..1598e23 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -137,10 +137,10 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
> bool new_level)
>  
>   timer->active_cleared_last = false;
>   timer->irq.level = new_level;
> - trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
> + trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->virt_irq,
>  timer->irq.level);
>   ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> -  timer->map->virt_irq,
> +  timer->virt_irq,
>timer->irq.level);
>   WARN_ON(ret);
>  }
> @@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>   * to ensure that hardware interrupts from the timer triggers a guest
>   * exit.
>   */
> - if (timer->irq.level || kvm_vgic_map_is_active(vcpu, 
> timer->map->virt_irq))
> + if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->virt_irq))
>   phys_active = true;
>   else
>   phys_active = false;
> @@ -351,7 +351,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>   if (WARN_ON(IS_ERR(map)))
>   return PTR_ERR(map);
>  
> - timer->map = map;
> + timer->virt_irq = irq->irq;
you also have a duplicate above:
timer->irq.irq = irq->irq;
>   return 0;
>  }
>  
> @@ -494,8 +494,8 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>   struct arch_timer_cpu *timer = >arch.timer_cpu;
>  
>   timer_disarm(timer);
> - if (timer->map)
> - kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
> + if (timer->virt_irq)
> + kvm_vgic_unmap_phys_irq(vcpu, timer->virt_irq);
always unmap?

Cheers

Eric
>  }
>  
>  void kvm_timer_enable(struct kvm *kvm)
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/7] KVM: arm/arm64: Remove the IRQ field from struct irq_phys_map

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> From: Christoffer Dall 
> 
> The communication of a Linux IRQ number from outside the VGIC to the
> vgic was a leftover from the day when the vgic code cared about how a
> particular device injects virtual interrupts mapped to a physical
> interrupt.
> 
> We can safely remove this notion, leaving all physical IRQ handling to
> be done in the device driver (the arch timer in this case), which makes
> room for a saner API for the new VGIC.
> 
> Signed-off-by: Christoffer Dall 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h|  3 +--
>  virt/kvm/arm/arch_timer.c | 22 --
>  virt/kvm/arm/vgic.c   | 20 ++--
>  3 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 43eeb18..49c559e 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -157,7 +157,6 @@ struct vgic_io_device {
>  struct irq_phys_map {
>   u32 virt_irq;
>   u32 phys_irq;
> - u32 irq;
>  };
>  
>  struct irq_phys_map_entry {
> @@ -345,7 +344,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -int virt_irq, int irq);
> +int virt_irq, int phys_irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index b4d96b1..cfdf88f 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -274,7 +274,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>   if (timer->active_cleared_last && !phys_active)
>   return;
>  
> - ret = irq_set_irqchip_state(timer->map->irq,
> + ret = irq_set_irqchip_state(host_vtimer_irq,
>   IRQCHIP_STATE_ACTIVE,
>   phys_active);
>   WARN_ON(ret);
> @@ -307,6 +307,9 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  {
>   struct arch_timer_cpu *timer = >arch.timer_cpu;
>   struct irq_phys_map *map;
> + struct irq_desc *desc;
> + struct irq_data *data;
> + int phys_irq;
>  
>   /*
>* The vcpu timer irq number cannot be determined in
> @@ -326,10 +329,25 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>   kvm_timer_update_state(vcpu);
>  
>   /*
> +  * Find the physical IRQ number corresponding to the host_vtimer_irq
> +  */
> + desc = irq_to_desc(host_vtimer_irq);
> + if (!desc) {
can this really happen?
> + kvm_err("%s: no interrupt descriptor\n", __func__);
> + return -EINVAL;
> + }
> +
> + data = irq_desc_get_irq_data(desc);
> + while (data->parent_data)
> + data = data->parent_data;
> +
> + phys_irq = data->hwirq;
> +
> + /*
>* Tell the VGIC that the virtual interrupt is tied to a
>* physical interrupt. We do that once per VCPU.
>*/
> - map = kvm_vgic_map_phys_irq(vcpu, irq->irq, host_vtimer_irq);
> + map = kvm_vgic_map_phys_irq(vcpu, irq->irq, phys_irq);
>   if (WARN_ON(IS_ERR(map)))
>   return PTR_ERR(map);
>  
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 2d7ae35..ac5838b 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1723,27 +1723,13 @@ static struct list_head 
> *vgic_get_irq_phys_map_list(struct kvm_vcpu *vcpu,
>   * Returns a valid pointer on success, and an error pointer otherwise
>   */
the doc comment must be updated
 * @irq: The Linux IRQ number

Besides

Reviewed-by: Eric Auger 

Cheers

Eric

>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> -int virt_irq, int irq)
> +int virt_irq, int phys_irq)
>  {
>   struct vgic_dist *dist = >kvm->arch.vgic;
>   struct list_head *root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>   struct irq_phys_map *map;
>   struct irq_phys_map_entry *entry;
> - struct irq_desc *desc;
> - struct irq_data *data;
> - int phys_irq;
>  
> - desc = irq_to_desc(irq);
> - if (!desc) {
> - kvm_err("%s: no interrupt descriptor\n", __func__);
> - return ERR_PTR(-EINVAL);
> - }
> -
> - data = irq_desc_get_irq_data(desc);
> - while (data->parent_data)
> - data = data->parent_data;
> -
> - phys_irq = data->hwirq;
>  
>   /* Create a new mapping */
>   entry = 

Re: [PATCH 4/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_unmap_phys_irq()

[Eric Auger]

Reviewed-by: Eric Auger 

Eric

On 04/15/2016 04:04 PM, Andre Przywara wrote:
> kvm_vgic_unmap_phys_irq() only needs the virtual IRQ number, so let's
> just pass that between the arch timer and the VGIC to get rid of
> the irq_phys_map pointer.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h|  2 +-
>  virt/kvm/arm/arch_timer.c |  2 +-
>  virt/kvm/arm/vgic.c   | 11 ---
>  3 files changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 5a34adc..43eeb18 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -346,7 +346,7 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  int virt_irq, int irq);
> -int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> +int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 793465b..b4d96b1 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -477,7 +477,7 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu)
>  
>   timer_disarm(timer);
>   if (timer->map)
> - kvm_vgic_unmap_phys_irq(vcpu, timer->map);
> + kvm_vgic_unmap_phys_irq(vcpu, timer->map->virt_irq);
>  }
>  
>  void kvm_timer_enable(struct kvm *kvm)
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 6911327..2d7ae35 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1813,25 +1813,22 @@ static void vgic_free_phys_irq_map_rcu(struct 
> rcu_head *rcu)
>  /**
>   * kvm_vgic_unmap_phys_irq - Remove a virtual to physical IRQ mapping
>   * @vcpu: The VCPU pointer
> - * @map: The pointer to a mapping obtained through kvm_vgic_map_phys_irq
> + * @virt_irq: The virtual IRQ number to be unmapped
>   *
>   * Remove an existing mapping between virtual and physical interrupts.
>   */
> -int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, unsigned int virt_irq)
>  {
>   struct vgic_dist *dist = >kvm->arch.vgic;
>   struct irq_phys_map_entry *entry;
>   struct list_head *root;
>  
> - if (!map)
> - return -EINVAL;
> -
> - root = vgic_get_irq_phys_map_list(vcpu, map->virt_irq);
> + root = vgic_get_irq_phys_map_list(vcpu, virt_irq);
>  
>   spin_lock(>irq_phys_map_lock);
>  
>   list_for_each_entry(entry, root, entry) {
> - if (>map == map) {
> + if (entry->map.virt_irq == virt_irq) {
>   list_del_rcu(>entry);
>   call_rcu(>rcu, vgic_free_phys_irq_map_rcu);
>   break;
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/7] KVM: arm/arm64: directly pass virtual IRQ number on injecting mapped IRQ

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> When we want to inject a hardware mapped IRQ into a guest, we actually
> only need the virtual IRQ number from the irq_phys_map.
> So let's pass this number directly from the arch timer to the VGIC
> to avoid using the map as a parameter.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h| 2 +-
>  virt/kvm/arm/arch_timer.c | 2 +-
>  virt/kvm/arm/vgic.c   | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 281caf8..c4574da 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -341,7 +341,7 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu);
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int irq_num,
>   bool level);
>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
> -struct irq_phys_map *map, bool level);
> +unsigned int virt_irq, bool level);
>  void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg);
>  int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index a9ad4fe..eb56f1e 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -140,7 +140,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
> bool new_level)
>   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->map->virt_irq,
>  timer->irq.level);
>   ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> -  timer->map,
> +  timer->map->virt_irq,
>timer->irq.level);
>   WARN_ON(ret);
>  }
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 7282881..9937d41 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1667,7 +1667,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
> unsigned int irq_num,
>   * kvm_vgic_inject_mapped_irq - Inject a physically mapped IRQ to the vgic
>   * @kvm: The VM structure pointer
>   * @cpuid:   The CPU for PPIs
> - * @map: Pointer to a irq_phys_map structure describing the mapping
> + * @virt_irq: The virtual IRQ to be injected
>   * @level:   Edge-triggered:  true:  to trigger the interrupt
>   * false: to ignore the call
>   *Level-sensitive  true:  raise the input signal
> @@ -1678,7 +1678,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
> unsigned int irq_num,
>   * being HIGH and 0 being LOW and all devices being active-HIGH.
>   */
>  int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid,
> -struct irq_phys_map *map, bool level)
> +unsigned int virt_irq, bool level)
>  {
Could make sense to merge kvm_vgic_inject_mapped_irq and
kvm_vgic_inject_irq and just add a bool argument telling whether the
request comes from the userspace (if I remember well we wanted to
prevent the userspace from injecting a mapping irq). This would avoid
duplication. We can make it later though.

Reviewed-by: Eric Auger 

Cheers

Eric


>   int ret;
>  
> @@ -1686,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int 
> cpuid,
>   if (ret)
>   return ret;
>  
> - return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
> + return vgic_update_irq_pending(kvm, cpuid, virt_irq, level);
>  }
>  
>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/7] KVM: arm/arm64: directly pass virtual IRQ number on kvm_vgic_map_is_active()

[Eric Auger]

Hi Andre,
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> For getting the active state of a mapped IRQ, we actually only need
> the virtual IRQ number, not the pointer to the mapping entry.
> Pass the virtual IRQ number from the arch timer to the VGIC directly.
> 
> Signed-off-by: Andre Przywara 
> ---
>  include/kvm/arm_vgic.h| 2 +-
>  virt/kvm/arm/arch_timer.c | 2 +-
>  virt/kvm/arm/vgic.c   | 6 +++---
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index c4574da..5a34adc 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -347,7 +347,7 @@ int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu);
>  struct irq_phys_map *kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu,
>  int virt_irq, int irq);
>  int kvm_vgic_unmap_phys_irq(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> -bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map);
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq);
>  
>  #define irqchip_in_kernel(k) (!!((k)->arch.vgic.in_kernel))
>  #define vgic_initialized(k)  (!!((k)->arch.vgic.nr_cpus))
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index eb56f1e..793465b 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -246,7 +246,7 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
>   * to ensure that hardware interrupts from the timer triggers a guest
>   * exit.
>   */
> - if (timer->irq.level || kvm_vgic_map_is_active(vcpu, timer->map))
> + if (timer->irq.level || kvm_vgic_map_is_active(vcpu, 
> timer->map->virt_irq))
>   phys_active = true;
>   else
>   phys_active = false;
nit:
phys_active =
(timer->irq.level ||
kvm_vgic_map_is_active(vcpu, timer->map->virt_irq)); ?

checkpatch warning:

#40: FILE: virt/kvm/arm/arch_timer.c:278:
+   if (timer->irq.level || kvm_vgic_map_is_active(vcpu,
timer->map->virt_irq))

Besides
Reviewed-by: Eric Auger 

Cheers

Eric
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 9937d41..6911327 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1103,18 +1103,18 @@ static bool dist_active_irq(struct kvm_vcpu *vcpu)
>   return test_bit(vcpu->vcpu_id, dist->irq_active_on_cpu);
>  }
>  
> -bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, struct irq_phys_map *map)
> +bool kvm_vgic_map_is_active(struct kvm_vcpu *vcpu, unsigned int virt_irq)
>  {
>   int i;
>  
>   for (i = 0; i < vcpu->arch.vgic_cpu.nr_lr; i++) {
>   struct vgic_lr vlr = vgic_get_lr(vcpu, i);
>  
> - if (vlr.irq == map->virt_irq && vlr.state & LR_STATE_ACTIVE)
> + if (vlr.irq == virt_irq && vlr.state & LR_STATE_ACTIVE)
>   return true;
>   }
>  
> - return vgic_irq_is_active(vcpu, map->virt_irq);
> + return vgic_irq_is_active(vcpu, virt_irq);
>  }
>  
>  /*
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 1/7] KVM: arm/arm64: remove unneeded map parameter for vgic_update_irq_pending()

[Eric Auger]

Reviewed-by: Eric Auger 

Eric
On 04/15/2016 04:04 PM, Andre Przywara wrote:
> We actually don't use the irq_phys_map parameter in
> vgic_update_irq_pending(), so let's just remove it.
> 
> Signed-off-by: Andre Przywara 
> ---
>  virt/kvm/arm/vgic.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
> index 00429b3..7282881 100644
> --- a/virt/kvm/arm/vgic.c
> +++ b/virt/kvm/arm/vgic.c
> @@ -1522,7 +1522,6 @@ static int vgic_validate_injection(struct kvm_vcpu 
> *vcpu, int irq, int level)
>  }
>  
>  static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> -struct irq_phys_map *map,
>  unsigned int irq_num, bool level)
>  {
>   struct vgic_dist *dist = >arch.vgic;
> @@ -1661,7 +1660,7 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, 
> unsigned int irq_num,
>   if (map)
>   return -EINVAL;
>  
> - return vgic_update_irq_pending(kvm, cpuid, NULL, irq_num, level);
> + return vgic_update_irq_pending(kvm, cpuid, irq_num, level);
>  }
>  
>  /**
> @@ -1687,7 +1686,7 @@ int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int 
> cpuid,
>   if (ret)
>   return ret;
>  
> - return vgic_update_irq_pending(kvm, cpuid, map, map->virt_irq, level);
> + return vgic_update_irq_pending(kvm, cpuid, map->virt_irq, level);
>  }
>  
>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 7/7] KVM: arm: enable KVM_SIGNAL_MSI and MSI routing

[Eric Auger]

On 04/14/2016 02:12 PM, Christoffer Dall wrote:
> On Mon, Apr 04, 2016 at 10:47:37AM +0200, Eric Auger wrote:
>> If the ITS modality is not available, let's simply support MSI
>> injection by transforming the MSI.data into an SPI ID.
>>
>> This becomes possible to use KVM_SIGNAL_MSI ioctl and MSI
>> routing for arm too.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>>
>> v2 -> v3:
>> - reword the commit message
>> - add sanity check about devid provision
>>
>> v1 -> v2:
>> - introduce vgic_v2m_inject_msi in vgic-v2-emul.c following Andre's
>>   advice
>>
>> Conflicts:
>>  arch/arm/kvm/Kconfig
>>
>> Conflicts:
>>  arch/arm/kvm/Kconfig
>> ---
>>  arch/arm/kvm/Kconfig   |  1 +
>>  virt/kvm/arm/vgic/vgic-v2.c| 15 +++
>>  virt/kvm/arm/vgic/vgic.h   |  1 +
>>  virt/kvm/arm/vgic/vgic_irqfd.c |  2 +-
>>  4 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/kvm/Kconfig b/arch/arm/kvm/Kconfig
>> index 92c3aec..67019e5 100644
>> --- a/arch/arm/kvm/Kconfig
>> +++ b/arch/arm/kvm/Kconfig
>> @@ -50,6 +50,7 @@ config KVM_NEW_VGIC
>>  bool "New VGIC implementation"
>>  depends on KVM
>>  default y
>> +select HAVE_KVM_MSI
>>  select HAVE_KVM_IRQCHIP
>>  select HAVE_KVM_IRQ_ROUTING
>>  ---help---
>> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
>> index 5f7c289..7400af0 100644
>> --- a/virt/kvm/arm/vgic/vgic-v2.c
>> +++ b/virt/kvm/arm/vgic/vgic-v2.c
>> @@ -248,6 +248,21 @@ void vgic_v2_enable(struct kvm_vcpu *vcpu)
>>  vcpu->arch.vgic_cpu.vgic_v2.vgic_hcr = GICH_HCR_EN;
>>  }
>>  
>> +/**
>> + * vgic_v2m_inject_msi: emulates GICv2M MSI injection by injecting
>> + * the SPI ID matching the msi data
>> + *
>> + * @kvm: pointer to the kvm struct
>> + * @msi: the msi struct handle
>> + */
>> +int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi)
>> +{
>> +if (msi->flags & KVM_MSI_VALID_DEVID)
>> +return -EINVAL;
>> +
>> +return kvm_vgic_inject_irq(kvm, 0, msi->data, 1);
> 
> I think you need to validate the msi->data here, otherwise
> vgic_get_irq() is going to raise warnings etc.
OK will check it is within the SPI ID range.

Thank you for your time!

Best Regards

Eric


> 
>> +}
>> +
>>  int vgic_v2_map_resources(struct kvm *kvm)
>>  {
>>  struct vgic_dist *dist = >arch.vgic;
>> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
>> index 223c778..354a865 100644
>> --- a/virt/kvm/arm/vgic/vgic.h
>> +++ b/virt/kvm/arm/vgic/vgic.h
>> @@ -44,6 +44,7 @@ int vgic_v2_probe(struct device_node *vgic_node);
>>  int vgic_v2_map_resources(struct kvm *kvm);
>>  int vgic_register_dist_regions(struct kvm *kvm, gpa_t dist_base_address,
>> enum vgic_type);
>> +int vgic_v2m_inject_msi(struct kvm *kvm, struct kvm_msi *msi);
>>  
>>  #ifdef CONFIG_KVM_ARM_VGIC_V3
>>  void vgic_v3_irq_change_affinity(struct kvm *kvm, u32 intid, u64 mpidr);
>> diff --git a/virt/kvm/arm/vgic/vgic_irqfd.c b/virt/kvm/arm/vgic/vgic_irqfd.c
>> index a3a7f02..588cdd6 100644
>> --- a/virt/kvm/arm/vgic/vgic_irqfd.c
>> +++ b/virt/kvm/arm/vgic/vgic_irqfd.c
>> @@ -100,7 +100,7 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>>  msi.devid = e->devid;
>>  
>>  if (!vgic_has_its(kvm))
>> -return -ENODEV;
>> +return vgic_v2m_inject_msi(kvm, );
>>  
>>  return vits_inject_msi(kvm, );
>>  }
>> -- 
>> 1.9.1
>>

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] arm64: Fix EL1/EL2 early init inconsistencies with VHE

[Marc Zyngier]

Hi Dave,

On 18/04/16 18:57, Dave Martin wrote:
> When using the Virtualisation Host Extensions, EL1 is not used in
> the host and requires no separate configuration.
> 
> In addition, with VHE enabled, non-hyp-specific EL2 configuration
> that does not need to be done early will be done anyway in
> __cpu_setup via the _EL1 system register aliases.  In particular,
> the layout and definition of CPTR_EL2 are changed by enabling VHE
> so that they resemble CPACR_EL1, so existing code to initialise
> CPTR_EL2 becomes architecturally wrong in this case.
> 
> This patch simply skips the affected initialisation code in the
> non-VHE case.
> 
> Signed-off-by: Dave Martin 
> ---
> 
> Note -- not tested yet, and I'm still unclear on whether this is the
> correct architectural approach...

This looks correct to me. Given that we're not leaving EL2, there is no
reason to do things earlier than they are done at EL1, and this fixes an
obvious bug on CPTR_EL2 access.

I've given it a go on a VHE model, and it ran just fine, so:

Reviewed-by: Marc Zyngier 

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC v4 0/7] KVM: arm/arm64: gsi routing support

[Eric Auger]

Hi Christoffer,
On 04/14/2016 02:04 PM, Christoffer Dall wrote:
> On Mon, Apr 04, 2016 at 10:47:30AM +0200, Eric Auger wrote:
>> With the advent of GICv3 ITS in-kernel emulation, KVM MSI routing
>> becomes mandated for proper VIRTIO-PCI vhost integration.
>>
>> In QEMU, when the VIRTIO-PCI device is programmed with the MSI message,
>> we previously used direct_mapping trick: this consists in extracting the
>> SPI ID found in the MSI message and associating an irqfd to that SPI ID.
>> When vhost worker thread gets a new buffer it signals the irqfd and kvm
>> then injects this SPI ID on guest. That way although the guest uses MSIs,
>> no MSI emulation is used.
>>
>> This worked fine with GICv2m but does not work anymore with GICV3 ITS.
>> Indeed this latter implements IRQ translation: what is found in the MSI
>> message no more is the target SPI ID but is an intermediate event ID used
>> in the translation process.
>>
>> Hence true MSI routing is needed so that the vhost back channel irqfd is
>> associated to a dummy gsi ID, routed towards the programmed MSI. When KVM
> 
> Doesn't the guest have to program the device with some ID?  So how is
> this dummy GSI ID assigned?
Please apologize for the delay.

We have this path:
irqfd -> dummy GSI -> MSI -> LPI ID

the MSI route does the link between the dummy GSI and the MSI.
the device id is embedded in the route entry definition (devid field of
kvm_irq_routing_msi)
The eventid is put in the MSI data

see https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg06178.html
(Pavel's vITS RFC)

in QEMU, kvm-all.c/kvm_irqchip_add_msi_route allocates the "dummy" gsi
(kvm_irqchip_get_virq) and builds the routing entry.


> 
>> injects the MSI through the in-kernel ITS emulation, the MSI is properly
>> translated and eventually the SPI ID associated to the event ID is injected
>> on guest.
> 
> Isn't it an LPI that is injected to the guest when you have an emulated
> ITS then?
It is definitively an LPI ID.
> 
>>
>> irqchip routing does not sound to be really useful on arm but usage of
> 
> Is this also true if you had multiple emulated ITS devices?
> 
> (I just don't want to block our options of doing this in the future if
> ever required.)

yes irqchip routing may be useful to handle multiple guest irqchips. I
meant: currently irqchip routing comes with this series although this is
not the series' primary goal. There is no special limitation except it
does not apply to KVM_IRQ_LINE ioctl because of the gsi semantic chosen
in the past, for ARM:

bits:  | 31 ... 24 | 23  ... 16 | 15...0 |
field: | irq_type  | vcpu_index | irq_id |

Best Regards

Eric

> 
>> MSI routing also mandates to integrate irqchip routing. The initial
>> implementation of irqfd on arm must be upgraded with the integration
>> of kvm irqchip.c code and the implementation of its standard hooks
>> in the architecture specific part.
>>
>> In case KVM_SET_GSI_ROUTING ioctl is not called, a default routing
>> table with flat irqchip routing entries is built enabling to inject gsi
>> corresponding to the SPI indexes seen by the guest.
>>
>> As soon as KVM_SET_GSI_ROUTING is called, user-space overwrites this
>> default routing table and is responsible for building the whole routing
>> table.
>>
>> for arm/arm64 KVM_SET_GSI_ROUTING has a limited support:
>> - only applies to KVM_IRQFD and not to KVM_IRQ_LINE
>>
>> - irqchip routing was tested on Calxeda midway (VFIO with irqfd)
>>   with and without explicit routing
>> - MSI routing without GICv3 ITS was tested using APM Xgene-I
>>   (qemu VIRTIO-PCI vhost-net without gsi_direct_mapping).
>> - MSI routing with GICv3 ITS is *NOT* tested.
>>
>> Code can be found at 
>> https://git.linaro.org/people/eric.auger/linux.git/shortlog/refs/heads/v4.5-rc6-its-emul-v4-gsi-routing-v4
>>
>> The series applies on top of Andre's branch featuring NEW-VGIC and in-kernel
>> ITS emulation series:
>> http://www.linux-arm.org/git?p=linux-ap.git;a=log;h=refs/heads/its-emul/v4
>>
>> [1]: [PATCH v4 00/12] KVM: arm64: GICv3 ITS emulation
>>  http://www.spinics.net/lists/arm-kernel/msg492770.html
>> [2]: [RFC PATCH 00/45] KVM: arm/arm64: Rework virtual GIC emulation
>>  http://www.spinics.net/lists/arm-kernel/msg492639.html
>>
>> GSI flat routing setup on QEMU can be found at:
>> https://lists.nongnu.org/archive/html/qemu-devel/2015-07/msg06262.html
>>
>> History:
>> v3 -> v4:
>> - rebase on top of NEW-VGIC RFC and ITS emulation series v4. This is not
>>   a stable foundation yet. Hence the revert to RFC. This v4 mostly is a
>>   reflesh/reminder.
>> - rewrite the cover letter
>>
>> v2 -> v3:
>> - don't use KVM_IRQ_ROUTING_EXTENDED_MSI type at uapi and kernel level 
>> anymore;
>>   use KVM_MSI_VALID_DEVID flag instead
>> - propagate user flags downto the kernel to make sure the userspace
>>   correctly set devid in GICv3 ITS case (still under discussion)
>>
>> v1 -> v2:
>> - user API changed:
>>   x devid id passed in kvm_irq_routing_msi
>>   x kept the new 

Re: [PATCH v4 1/7] KVM: api: pass the devid in the msi routing entry

[Eric Auger]

Christoffer,
On 04/14/2016 02:04 PM, Christoffer Dall wrote:
> On Mon, Apr 04, 2016 at 10:47:31AM +0200, Eric Auger wrote:
>> On ARM, the MSI msg (address and data) comes along with
>> out-of-band device ID information. The device ID encodes the
>> device that writes the MSI msg. Let's convey the device id in
>> kvm_irq_routing_msi and use KVM_MSI_VALID_DEVID flag value in
>> kvm_irq_routing_entry to indicate the msi devid is populated.
>>
>> Signed-off-by: Eric Auger 
>>
>> ---
>> v2 -> v3:
>> - replace usage of KVM_IRQ_ROUTING_EXTENDED_MSI type by
>>   usage of KVM_MSI_VALID_DEVID flag
>> - add note about KVM_CAP_MSI_DEVID capability
>>
>> v1 -> v2:
>> - devid id passed in kvm_irq_routing_msi instead of in
>>   kvm_irq_routing_entry
>>
>> RFC -> PATCH
>> - remove kvm_irq_routing_extended_msi and use union instead
>> ---
>>  Documentation/virtual/kvm/api.txt | 18 --
>>  include/uapi/linux/kvm.h  |  5 -
>>  2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt 
>> b/Documentation/virtual/kvm/api.txt
>> index 536f19b..c436bb6 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -1462,7 +1462,10 @@ struct kvm_irq_routing_entry {
>>  #define KVM_IRQ_ROUTING_S390_ADAPTER 3
>>  #define KVM_IRQ_ROUTING_HV_SINT 4
>>  
>> -No flags are specified so far, the corresponding field must be set to zero.
>> +flags:
>> +- KVM_MSI_VALID_DEVID: used along with KVM_IRQ_ROUTING_MSI
>> +  routing entry type, it tells the msi devid contains a valid value.
> 
> s/it tells the msi devid/specifies that the devid field/
ok
> 
>> +- zero otherwise
>>  
>>  struct kvm_irq_routing_irqchip {
>>  __u32 irqchip;
>> @@ -1473,9 +1476,20 @@ struct kvm_irq_routing_msi {
>>  __u32 address_lo;
>>  __u32 address_hi;
>>  __u32 data;
>> -__u32 pad;
>> +union {
>> +__u32 pad;
>> +__u32 devid;
>> +};
>>  };
>>  
>> +devid: If KVM_MSI_VALID_DEVID is set, contains a unique device identifier
>> +   for the device that wrote the MSI message.
>> +   For PCI, this is usually a BFD identifier in the lower 16 bits.
>> +
>> +The per-VM KVM_CAP_MSI_DEVID capability advertises the need to provide
> 
> "the need to" or "the possibility to"?  Is there ever a situation where
> KVM_CAP_MSI_DEVID is set but you don't need to provide a device ID and
> set the flag?
I could replace by "requirement":
if it is not requested to pass a devid, we don't set the flag. If it is
we pass it.

if the user fails passing the devid and this ends up in the in-kernel
ITS emul, injection will fail.

Thanks

Eric
> 
>> +the device ID. If this capability is not set, userland cannot rely on
>> +the kernel to allow the KVM_MSI_VALID_DEVID flag being set.
>> +
>>  struct kvm_irq_routing_s390_adapter {
>>  __u64 ind_addr;
>>  __u64 summary_addr;
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index 6a02871..52a973f 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -865,7 +865,10 @@ struct kvm_irq_routing_msi {
>>  __u32 address_lo;
>>  __u32 address_hi;
>>  __u32 data;
>> -__u32 pad;
>> +union {
>> +__u32 pad;
>> +__u32 devid;
>> +};
>>  };
>>  
>>  struct kvm_irq_routing_s390_adapter {
>> -- 
>> 1.9.1
>>

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] arm64: KVM: Allow userspace to configure guest MPIDR_EL1

[Ashok Kumar]

On Thu, Apr 21, 2016 at 11:56:03AM +0200, Andrew Jones wrote:
> On Thu, Apr 21, 2016 at 10:25:24AM +0100, Marc Zyngier wrote:
> > Hey Andrew,
> > 
> > On 21/04/16 08:04, Andrew Jones wrote:
> > > On Wed, Apr 20, 2016 at 06:33:54PM +0100, Marc Zyngier wrote:
> > >> On Wed, 20 Apr 2016 07:08:39 -0700
> > >> Ashok Kumar  wrote:
> > >>
> > >>> For guests with NUMA configuration, Node ID needs to
> > >>> be recorded in the respective affinity byte of MPIDR_EL1.
> > >>
> > >> As others have said before, the mapping between the NUMA hierarchy and
> > >> MPIDR_EL1 are completely arbitrary, and only the firmware description
> > >> can help the kernel in interpreting the affinity levels.
> > >>
> > >> If you want any patch like this one to be considered, I'd like to see
> > >> the corresponding userspace that:
> > >>
> > >> - programs the affinity into the vcpus,
> > > 
> > > I have a start on this for QEMU that I can dust off and send as an RFC
> > > soon.
> > > 
> > >> - pins the vcpus to specific physical CPUs,
> > > 
> > > This wouldn't be part of the userspace directly interacting with KVM,
> > > but rather a higher level (even higher than libvirt, e.g.
> > > openstack/ovirt). I also don't think we should need to worry about
> > > which/how the phyiscal cpus get chosen. Let's assume that entity
> > > knows how to best map the guest's virtual topology to a physical one.
> > 
> > Surely the platform emulation userspace has to implement the pinning
> > itself, because I can't see high level tools being involved in the
> > creation of the vcpu threads themselves.
> 
> The pinning comes after the threads are created, but before they are
> run. The virtual topology created for a guest may or may not map well
> to the physical topology of a given host. That's not the problem of
> the emulation though. That's a problem of a high level application
> trying to fit it.
> 
> > 
> > Also, I'd like to have a "simple" tool to test this without having to
> > deploy openstack (the day this becomes mandatory for kernel development,
> > I'll move my carrier to something more... agricultural).
> > 
> > So something in QEMU would be really good...
> > 
> 
> To test the virtual topology only requires booting a guest, whether
> the vcpus are pinned or not. To test that it was worth the effort to
> create a virtual topology does require the pinning, and the perf
> measuring. However we still don't need the pinning in QEMU. We can
> start a guest paused, run a script that does a handful of tasksets,
> and then resumes the guest. Or, just use libvirt, which allows one
> to save vcpu affinities, and thus on guest launch it will automatically
> do the affinity setting for you.
> 
> > > 
> > >> - exposes the corresponding firmware description (either DT or ACPI) to
> > >>   the kernel.
> > > 
> > > The QEMU patches I've started on already generate the DT (the cpu-map
> > > node). I started looking into how to do it for ACPI too, but there
> > > were some questions about whether or not the topology description
> > > tables added to the 6.1 spec were sufficient. I can send the DT part
> > > soon, and continue to look into the ACPI part later though.
> > 
> > That'd be great. Can you please sync with Ashok so that we have
> > something consistent between the two of you?
> 
> Yup. I'm hoping Ashok will chime in to share any userspace status
> they have.

I tested it using qemu's arm numa patchset [1] and I don't have any
changes for cpu-map.
I just used qemu's thread,core,socket information from "-smp" command
line argument to populate the affinity.
I was hoping to see the reception for this patch and then post qemu
changes.

[1] https://lists.gnu.org/archive/html/qemu-arm/2016-01/msg00363.html

Thanks,
Ashok
> 
> Thanks,
> drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] arm64: KVM: Allow userspace to configure guest MPIDR_EL1

[Andrew Jones]

On Thu, Apr 21, 2016 at 10:25:24AM +0100, Marc Zyngier wrote:
> Hey Andrew,
> 
> On 21/04/16 08:04, Andrew Jones wrote:
> > On Wed, Apr 20, 2016 at 06:33:54PM +0100, Marc Zyngier wrote:
> >> On Wed, 20 Apr 2016 07:08:39 -0700
> >> Ashok Kumar  wrote:
> >>
> >>> For guests with NUMA configuration, Node ID needs to
> >>> be recorded in the respective affinity byte of MPIDR_EL1.
> >>
> >> As others have said before, the mapping between the NUMA hierarchy and
> >> MPIDR_EL1 are completely arbitrary, and only the firmware description
> >> can help the kernel in interpreting the affinity levels.
> >>
> >> If you want any patch like this one to be considered, I'd like to see
> >> the corresponding userspace that:
> >>
> >> - programs the affinity into the vcpus,
> > 
> > I have a start on this for QEMU that I can dust off and send as an RFC
> > soon.
> > 
> >> - pins the vcpus to specific physical CPUs,
> > 
> > This wouldn't be part of the userspace directly interacting with KVM,
> > but rather a higher level (even higher than libvirt, e.g.
> > openstack/ovirt). I also don't think we should need to worry about
> > which/how the phyiscal cpus get chosen. Let's assume that entity
> > knows how to best map the guest's virtual topology to a physical one.
> 
> Surely the platform emulation userspace has to implement the pinning
> itself, because I can't see high level tools being involved in the
> creation of the vcpu threads themselves.

The pinning comes after the threads are created, but before they are
run. The virtual topology created for a guest may or may not map well
to the physical topology of a given host. That's not the problem of
the emulation though. That's a problem of a high level application
trying to fit it.

> 
> Also, I'd like to have a "simple" tool to test this without having to
> deploy openstack (the day this becomes mandatory for kernel development,
> I'll move my carrier to something more... agricultural).
> 
> So something in QEMU would be really good...
> 

To test the virtual topology only requires booting a guest, whether
the vcpus are pinned or not. To test that it was worth the effort to
create a virtual topology does require the pinning, and the perf
measuring. However we still don't need the pinning in QEMU. We can
start a guest paused, run a script that does a handful of tasksets,
and then resumes the guest. Or, just use libvirt, which allows one
to save vcpu affinities, and thus on guest launch it will automatically
do the affinity setting for you.

> > 
> >> - exposes the corresponding firmware description (either DT or ACPI) to
> >>   the kernel.
> > 
> > The QEMU patches I've started on already generate the DT (the cpu-map
> > node). I started looking into how to do it for ACPI too, but there
> > were some questions about whether or not the topology description
> > tables added to the 6.1 spec were sufficient. I can send the DT part
> > soon, and continue to look into the ACPI part later though.
> 
> That'd be great. Can you please sync with Ashok so that we have
> something consistent between the two of you?

Yup. I'm hoping Ashok will chime in to share any userspace status
they have.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] arm64: KVM: Allow userspace to configure guest MPIDR_EL1

[Marc Zyngier]

Hey Andrew,

On 21/04/16 08:04, Andrew Jones wrote:
> On Wed, Apr 20, 2016 at 06:33:54PM +0100, Marc Zyngier wrote:
>> On Wed, 20 Apr 2016 07:08:39 -0700
>> Ashok Kumar  wrote:
>>
>>> For guests with NUMA configuration, Node ID needs to
>>> be recorded in the respective affinity byte of MPIDR_EL1.
>>
>> As others have said before, the mapping between the NUMA hierarchy and
>> MPIDR_EL1 are completely arbitrary, and only the firmware description
>> can help the kernel in interpreting the affinity levels.
>>
>> If you want any patch like this one to be considered, I'd like to see
>> the corresponding userspace that:
>>
>> - programs the affinity into the vcpus,
> 
> I have a start on this for QEMU that I can dust off and send as an RFC
> soon.
> 
>> - pins the vcpus to specific physical CPUs,
> 
> This wouldn't be part of the userspace directly interacting with KVM,
> but rather a higher level (even higher than libvirt, e.g.
> openstack/ovirt). I also don't think we should need to worry about
> which/how the phyiscal cpus get chosen. Let's assume that entity
> knows how to best map the guest's virtual topology to a physical one.

Surely the platform emulation userspace has to implement the pinning
itself, because I can't see high level tools being involved in the
creation of the vcpu threads themselves.

Also, I'd like to have a "simple" tool to test this without having to
deploy openstack (the day this becomes mandatory for kernel development,
I'll move my carrier to something more... agricultural).

So something in QEMU would be really good...

> 
>> - exposes the corresponding firmware description (either DT or ACPI) to
>>   the kernel.
> 
> The QEMU patches I've started on already generate the DT (the cpu-map
> node). I started looking into how to do it for ACPI too, but there
> were some questions about whether or not the topology description
> tables added to the 6.1 spec were sufficient. I can send the DT part
> soon, and continue to look into the ACPI part later though.

That'd be great. Can you please sync with Ashok so that we have
something consistent between the two of you?

Thanks!

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH] arm64: KVM: Allow userspace to configure guest MPIDR_EL1

[Andrew Jones]

On Wed, Apr 20, 2016 at 06:33:54PM +0100, Marc Zyngier wrote:
> On Wed, 20 Apr 2016 07:08:39 -0700
> Ashok Kumar  wrote:
> 
> > For guests with NUMA configuration, Node ID needs to
> > be recorded in the respective affinity byte of MPIDR_EL1.
> 
> As others have said before, the mapping between the NUMA hierarchy and
> MPIDR_EL1 are completely arbitrary, and only the firmware description
> can help the kernel in interpreting the affinity levels.
> 
> If you want any patch like this one to be considered, I'd like to see
> the corresponding userspace that:
> 
> - programs the affinity into the vcpus,

I have a start on this for QEMU that I can dust off and send as an RFC
soon.

> - pins the vcpus to specific physical CPUs,

This wouldn't be part of the userspace directly interacting with KVM,
but rather a higher level (even higher than libvirt, e.g.
openstack/ovirt). I also don't think we should need to worry about
which/how the phyiscal cpus get chosen. Let's assume that entity
knows how to best map the guest's virtual topology to a physical one.

> - exposes the corresponding firmware description (either DT or ACPI) to
>   the kernel.

The QEMU patches I've started on already generate the DT (the cpu-map
node). I started looking into how to do it for ACPI too, but there
were some questions about whether or not the topology description
tables added to the 6.1 spec were sufficient. I can send the DT part
soon, and continue to look into the ACPI part later though.

Thanks,
drew
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm