Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On 01/02/17 10:07, Christoffer Dall wrote:
> On Wed, Feb 01, 2017 at 03:40:10AM -0500, Jintack Lim wrote:
>> On Wed, Feb 1, 2017 at 3:04 AM, Christoffer Dall
>>  wrote:
>>> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
 On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
 wrote:
> Now that we maintain the EL1 physical timer register states of VMs,
> update the physical timer interrupt level along with the virtual one.
>
> Note that the emulated EL1 physical timer is not mapped to any hardware
> timer, so we call a proper vgic function.
>
> Signed-off-by: Jintack Lim 
> ---
>  virt/kvm/arm/arch_timer.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0f6e935..3b6bd50 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> kvm_vcpu *vcpu, bool new_level,
> WARN_ON(ret);
>  }
>
> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> +struct arch_timer_context *timer)
> +{
> +   int ret;
> +
> +   BUG_ON(!vgic_initialized(vcpu->kvm));

 Although I've added my fair share of BUG_ON() in the code base, I've
 since reconsidered my position. If we get in a situation where the vgic
 is not initialized, maybe it would be better to just WARN_ON and return
 early rather than killing the whole box. Thoughts?

>>>
>>> Could we help this series along by saying that since this BUG_ON already
>>> exists in the kvm_timer_update_mapped_irq function, then it just
>>> preserves functionality and it's up to someone else (me) to remove the
>>> BUG_ON from both functions later in life?
>>>
>>
>> Sounds good to me :) Thanks!
>>
> 
> So just as you thought you were getting off easy...
> 
> The reason we now have kvm_timer_update_irq and
> kvm_timer_update_mapped_irq is that we have the following two vgic
> functions:
> 
> kvm_vgic_inject_irq
> kvm_vgic_inject_mapped_irq
> 
> But the only difference between the two is what they pass
> as the mapped_irq argument to vgic_update_irq_pending.
> 
> What about if we just had this as a precursor patch:
> 
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 6a084cd..91ecf48 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -175,7 +175,8 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
> bool new_level)
>   timer->irq.level = new_level;
>   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
>  timer->irq.level);
> - ret = kvm_vgic_inject_mapped_irq(vcpu->kvm, vcpu->vcpu_id,
> +
> + ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
>timer->irq.irq,
>timer->irq.level);
>   WARN_ON(ret);
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index dea12df..4c87fd0 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -336,8 +336,7 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct 
> vgic_irq *irq)
>  }
>  
>  static int vgic_update_irq_pending(struct kvm *kvm, int cpuid,
> -unsigned int intid, bool level,
> -bool mapped_irq)
> +unsigned int intid, bool level)
>  {
>   struct kvm_vcpu *vcpu;
>   struct vgic_irq *irq;
> @@ -357,11 +356,6 @@ static int vgic_update_irq_pending(struct kvm *kvm, int 
> cpuid,
>   if (!irq)
>   return -EINVAL;
>  
> - if (irq->hw != mapped_irq) {
> - vgic_put_irq(kvm, irq);
> - return -EINVAL;
> - }
> -
>   spin_lock(>irq_lock);
>  
>   if (!vgic_validate_injection(irq, level)) {
> @@ -399,13 +393,7 @@ static int vgic_update_irq_pending(struct kvm *kvm, int 
> cpuid,
>  int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, unsigned int intid,
>   bool level)
>  {
> - return vgic_update_irq_pending(kvm, cpuid, intid, level, false);
> -}
> -
> -int kvm_vgic_inject_mapped_irq(struct kvm *kvm, int cpuid, unsigned int 
> intid,
> -bool level)
> -{
> - return vgic_update_irq_pending(kvm, cpuid, intid, level, true);
> + return vgic_update_irq_pending(kvm, cpuid, intid, level);
>  }
>  
>  int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq)
> 
> 
> That would make this patch simpler.  If so, I can send out the above
> patch with a proper description.

Indeed. And while you're at it, rename vgic_update_irq_pending to
kvm_vgic_inject_irq, as I don't think we need this simple stub?

Thanks,

M.
-- 
Jazz is not dead. It just smells 

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On 01/02/17 08:04, Christoffer Dall wrote:
> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
>> wrote:
>>> Now that we maintain the EL1 physical timer register states of VMs,
>>> update the physical timer interrupt level along with the virtual one.
>>>
>>> Note that the emulated EL1 physical timer is not mapped to any hardware
>>> timer, so we call a proper vgic function.
>>>
>>> Signed-off-by: Jintack Lim 
>>> ---
>>>  virt/kvm/arm/arch_timer.c | 20 
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 0f6e935..3b6bd50 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
>>> kvm_vcpu *vcpu, bool new_level,
>>> WARN_ON(ret);
>>>  }
>>>  
>>> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>>> +struct arch_timer_context *timer)
>>> +{
>>> +   int ret;
>>> +
>>> +   BUG_ON(!vgic_initialized(vcpu->kvm));
>>
>> Although I've added my fair share of BUG_ON() in the code base, I've
>> since reconsidered my position. If we get in a situation where the vgic
>> is not initialized, maybe it would be better to just WARN_ON and return
>> early rather than killing the whole box. Thoughts?
>>
> 
> Could we help this series along by saying that since this BUG_ON already
> exists in the kvm_timer_update_mapped_irq function, then it just
> preserves functionality and it's up to someone else (me) to remove the
> BUG_ON from both functions later in life?

Works for me.

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 v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Jintack Lim]

On Wed, Feb 1, 2017 at 3:04 AM, Christoffer Dall
 wrote:
> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
>> wrote:
>> > Now that we maintain the EL1 physical timer register states of VMs,
>> > update the physical timer interrupt level along with the virtual one.
>> >
>> > Note that the emulated EL1 physical timer is not mapped to any hardware
>> > timer, so we call a proper vgic function.
>> >
>> > Signed-off-by: Jintack Lim 
>> > ---
>> >  virt/kvm/arm/arch_timer.c | 20 
>> >  1 file changed, 20 insertions(+)
>> >
>> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> > index 0f6e935..3b6bd50 100644
>> > --- a/virt/kvm/arm/arch_timer.c
>> > +++ b/virt/kvm/arm/arch_timer.c
>> > @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
>> > kvm_vcpu *vcpu, bool new_level,
>> > WARN_ON(ret);
>> >  }
>> >
>> > +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>> > +struct arch_timer_context *timer)
>> > +{
>> > +   int ret;
>> > +
>> > +   BUG_ON(!vgic_initialized(vcpu->kvm));
>>
>> Although I've added my fair share of BUG_ON() in the code base, I've
>> since reconsidered my position. If we get in a situation where the vgic
>> is not initialized, maybe it would be better to just WARN_ON and return
>> early rather than killing the whole box. Thoughts?
>>
>
> Could we help this series along by saying that since this BUG_ON already
> exists in the kvm_timer_update_mapped_irq function, then it just
> preserves functionality and it's up to someone else (me) to remove the
> BUG_ON from both functions later in life?
>

Sounds good to me :) Thanks!

> Thanks,
> -Christoffer
>

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

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Christoffer Dall]

On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
> wrote:
> > Now that we maintain the EL1 physical timer register states of VMs,
> > update the physical timer interrupt level along with the virtual one.
> >
> > Note that the emulated EL1 physical timer is not mapped to any hardware
> > timer, so we call a proper vgic function.
> >
> > Signed-off-by: Jintack Lim 
> > ---
> >  virt/kvm/arm/arch_timer.c | 20 
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 0f6e935..3b6bd50 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> > kvm_vcpu *vcpu, bool new_level,
> > WARN_ON(ret);
> >  }
> >  
> > +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> > +struct arch_timer_context *timer)
> > +{
> > +   int ret;
> > +
> > +   BUG_ON(!vgic_initialized(vcpu->kvm));
> 
> Although I've added my fair share of BUG_ON() in the code base, I've
> since reconsidered my position. If we get in a situation where the vgic
> is not initialized, maybe it would be better to just WARN_ON and return
> early rather than killing the whole box. Thoughts?
> 

Could we help this series along by saying that since this BUG_ON already
exists in the kvm_timer_update_mapped_irq function, then it just
preserves functionality and it's up to someone else (me) to remove the
BUG_ON from both functions later in life?

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

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On 30/01/17 19:06, Christoffer Dall wrote:
> On Mon, Jan 30, 2017 at 06:48:02PM +, Marc Zyngier wrote:
>> On 30/01/17 18:41, Christoffer Dall wrote:
>>> On Mon, Jan 30, 2017 at 05:50:03PM +, Marc Zyngier wrote:
 On 30/01/17 15:02, Christoffer Dall wrote:
> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim 
>>  wrote:
>>> Now that we maintain the EL1 physical timer register states of VMs,
>>> update the physical timer interrupt level along with the virtual one.
>>>
>>> Note that the emulated EL1 physical timer is not mapped to any hardware
>>> timer, so we call a proper vgic function.
>>>
>>> Signed-off-by: Jintack Lim 
>>> ---
>>>  virt/kvm/arm/arch_timer.c | 20 
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 0f6e935..3b6bd50 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
>>> kvm_vcpu *vcpu, bool new_level,
>>> WARN_ON(ret);
>>>  }
>>>  
>>> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>>> +struct arch_timer_context *timer)
>>> +{
>>> +   int ret;
>>> +
>>> +   BUG_ON(!vgic_initialized(vcpu->kvm));
>>
>> Although I've added my fair share of BUG_ON() in the code base, I've
>> since reconsidered my position. If we get in a situation where the vgic
>> is not initialized, maybe it would be better to just WARN_ON and return
>> early rather than killing the whole box. Thoughts?
>>
>
> The distinction to me is whether this will cause fatal crashes or
> exploits down the road if we're working on uninitialized data.  If all
> that can happen if the vgic is not initialized, is that the guest
> doesn't see interrupts, for example, then a WARN_ON is appropriate.
>
> Which is the case here?
>
> That being said, do we need this at all?  This is in the critial path
> and is actually measurable (I know this from my work on the other timer
> series), so it's better to get rid of it if we can.  Can we simply
> convince ourselves this will never happen, and is the code ever likely
> to change so that it gets called with the vgic disabled later?

 That'd be the best course of action. I remember us reworking some of
 that in the now defunct vgic-less series. Maybe we could salvage that
 code, if only for the time we spent on it...

>>> Ah, we never merged it?  Were we waiting on a userspace implementation
>>> or agreement on the ABI?
>>
>> We were waiting on the userspace side to be respun against the latest
>> API, and there were some comments from Peter (IIRC) about supporting
>> PPIs in general (the other timers and the PMU, for example).
>>
>> None of that happened, as the most vocal proponent of the series
>> apparently lost interest.
>>
>>> There was definitely a useful cleanup with the whole enabled flag thing
>>> on the timer I remember.
>>
>> Indeed. We should at least try to resurrect that bit.
>>
> 
> It's probably worth it trying to resurrect the whole thing I think,
> especially since I think the implementation ended up looking quite nice.

Indeed. My only concern is about the userspace counterpart, which hasn't
materialized when expected. Hopefully it will this time around!

> I can add a rebase of that to my list of never-ending timer rework.

We all know that you can do that while sleeping! ;-)

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 v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Christoffer Dall]

On Mon, Jan 30, 2017 at 06:48:02PM +, Marc Zyngier wrote:
> On 30/01/17 18:41, Christoffer Dall wrote:
> > On Mon, Jan 30, 2017 at 05:50:03PM +, Marc Zyngier wrote:
> >> On 30/01/17 15:02, Christoffer Dall wrote:
> >>> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
>  On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim 
>   wrote:
> > Now that we maintain the EL1 physical timer register states of VMs,
> > update the physical timer interrupt level along with the virtual one.
> >
> > Note that the emulated EL1 physical timer is not mapped to any hardware
> > timer, so we call a proper vgic function.
> >
> > Signed-off-by: Jintack Lim 
> > ---
> >  virt/kvm/arm/arch_timer.c | 20 
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 0f6e935..3b6bd50 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> > kvm_vcpu *vcpu, bool new_level,
> > WARN_ON(ret);
> >  }
> >  
> > +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> > +struct arch_timer_context *timer)
> > +{
> > +   int ret;
> > +
> > +   BUG_ON(!vgic_initialized(vcpu->kvm));
> 
>  Although I've added my fair share of BUG_ON() in the code base, I've
>  since reconsidered my position. If we get in a situation where the vgic
>  is not initialized, maybe it would be better to just WARN_ON and return
>  early rather than killing the whole box. Thoughts?
> 
> >>>
> >>> The distinction to me is whether this will cause fatal crashes or
> >>> exploits down the road if we're working on uninitialized data.  If all
> >>> that can happen if the vgic is not initialized, is that the guest
> >>> doesn't see interrupts, for example, then a WARN_ON is appropriate.
> >>>
> >>> Which is the case here?
> >>>
> >>> That being said, do we need this at all?  This is in the critial path
> >>> and is actually measurable (I know this from my work on the other timer
> >>> series), so it's better to get rid of it if we can.  Can we simply
> >>> convince ourselves this will never happen, and is the code ever likely
> >>> to change so that it gets called with the vgic disabled later?
> >>
> >> That'd be the best course of action. I remember us reworking some of
> >> that in the now defunct vgic-less series. Maybe we could salvage that
> >> code, if only for the time we spent on it...
> >>
> > Ah, we never merged it?  Were we waiting on a userspace implementation
> > or agreement on the ABI?
> 
> We were waiting on the userspace side to be respun against the latest
> API, and there were some comments from Peter (IIRC) about supporting
> PPIs in general (the other timers and the PMU, for example).
> 
> None of that happened, as the most vocal proponent of the series
> apparently lost interest.
> 
> > There was definitely a useful cleanup with the whole enabled flag thing
> > on the timer I remember.
> 
> Indeed. We should at least try to resurrect that bit.
> 

It's probably worth it trying to resurrect the whole thing I think,
especially since I think the implementation ended up looking quite nice.

I can add a rebase of that to my list of never-ending timer rework.

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

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On 30/01/17 18:41, Christoffer Dall wrote:
> On Mon, Jan 30, 2017 at 05:50:03PM +, Marc Zyngier wrote:
>> On 30/01/17 15:02, Christoffer Dall wrote:
>>> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
 On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
 wrote:
> Now that we maintain the EL1 physical timer register states of VMs,
> update the physical timer interrupt level along with the virtual one.
>
> Note that the emulated EL1 physical timer is not mapped to any hardware
> timer, so we call a proper vgic function.
>
> Signed-off-by: Jintack Lim 
> ---
>  virt/kvm/arm/arch_timer.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0f6e935..3b6bd50 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> kvm_vcpu *vcpu, bool new_level,
>   WARN_ON(ret);
>  }
>  
> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> +  struct arch_timer_context *timer)
> +{
> + int ret;
> +
> + BUG_ON(!vgic_initialized(vcpu->kvm));

 Although I've added my fair share of BUG_ON() in the code base, I've
 since reconsidered my position. If we get in a situation where the vgic
 is not initialized, maybe it would be better to just WARN_ON and return
 early rather than killing the whole box. Thoughts?

>>>
>>> The distinction to me is whether this will cause fatal crashes or
>>> exploits down the road if we're working on uninitialized data.  If all
>>> that can happen if the vgic is not initialized, is that the guest
>>> doesn't see interrupts, for example, then a WARN_ON is appropriate.
>>>
>>> Which is the case here?
>>>
>>> That being said, do we need this at all?  This is in the critial path
>>> and is actually measurable (I know this from my work on the other timer
>>> series), so it's better to get rid of it if we can.  Can we simply
>>> convince ourselves this will never happen, and is the code ever likely
>>> to change so that it gets called with the vgic disabled later?
>>
>> That'd be the best course of action. I remember us reworking some of
>> that in the now defunct vgic-less series. Maybe we could salvage that
>> code, if only for the time we spent on it...
>>
> Ah, we never merged it?  Were we waiting on a userspace implementation
> or agreement on the ABI?

We were waiting on the userspace side to be respun against the latest
API, and there were some comments from Peter (IIRC) about supporting
PPIs in general (the other timers and the PMU, for example).

None of that happened, as the most vocal proponent of the series
apparently lost interest.

> There was definitely a useful cleanup with the whole enabled flag thing
> on the timer I remember.

Indeed. We should at least try to resurrect that bit.

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 v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Christoffer Dall]

On Mon, Jan 30, 2017 at 05:50:03PM +, Marc Zyngier wrote:
> On 30/01/17 15:02, Christoffer Dall wrote:
> > On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
> >> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
> >> wrote:
> >>> Now that we maintain the EL1 physical timer register states of VMs,
> >>> update the physical timer interrupt level along with the virtual one.
> >>>
> >>> Note that the emulated EL1 physical timer is not mapped to any hardware
> >>> timer, so we call a proper vgic function.
> >>>
> >>> Signed-off-by: Jintack Lim 
> >>> ---
> >>>  virt/kvm/arm/arch_timer.c | 20 
> >>>  1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> >>> index 0f6e935..3b6bd50 100644
> >>> --- a/virt/kvm/arm/arch_timer.c
> >>> +++ b/virt/kvm/arm/arch_timer.c
> >>> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> >>> kvm_vcpu *vcpu, bool new_level,
> >>>   WARN_ON(ret);
> >>>  }
> >>>  
> >>> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> >>> +  struct arch_timer_context *timer)
> >>> +{
> >>> + int ret;
> >>> +
> >>> + BUG_ON(!vgic_initialized(vcpu->kvm));
> >>
> >> Although I've added my fair share of BUG_ON() in the code base, I've
> >> since reconsidered my position. If we get in a situation where the vgic
> >> is not initialized, maybe it would be better to just WARN_ON and return
> >> early rather than killing the whole box. Thoughts?
> >>
> > 
> > The distinction to me is whether this will cause fatal crashes or
> > exploits down the road if we're working on uninitialized data.  If all
> > that can happen if the vgic is not initialized, is that the guest
> > doesn't see interrupts, for example, then a WARN_ON is appropriate.
> > 
> > Which is the case here?
> > 
> > That being said, do we need this at all?  This is in the critial path
> > and is actually measurable (I know this from my work on the other timer
> > series), so it's better to get rid of it if we can.  Can we simply
> > convince ourselves this will never happen, and is the code ever likely
> > to change so that it gets called with the vgic disabled later?
> 
> That'd be the best course of action. I remember us reworking some of
> that in the now defunct vgic-less series. Maybe we could salvage that
> code, if only for the time we spent on it...
> 
Ah, we never merged it?  Were we waiting on a userspace implementation
or agreement on the ABI?

There was definitely a useful cleanup with the whole enabled flag thing
on the timer I remember.

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

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On 30/01/17 15:02, Christoffer Dall wrote:
> On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
>> wrote:
>>> Now that we maintain the EL1 physical timer register states of VMs,
>>> update the physical timer interrupt level along with the virtual one.
>>>
>>> Note that the emulated EL1 physical timer is not mapped to any hardware
>>> timer, so we call a proper vgic function.
>>>
>>> Signed-off-by: Jintack Lim 
>>> ---
>>>  virt/kvm/arm/arch_timer.c | 20 
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>>> index 0f6e935..3b6bd50 100644
>>> --- a/virt/kvm/arm/arch_timer.c
>>> +++ b/virt/kvm/arm/arch_timer.c
>>> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
>>> kvm_vcpu *vcpu, bool new_level,
>>> WARN_ON(ret);
>>>  }
>>>  
>>> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>>> +struct arch_timer_context *timer)
>>> +{
>>> +   int ret;
>>> +
>>> +   BUG_ON(!vgic_initialized(vcpu->kvm));
>>
>> Although I've added my fair share of BUG_ON() in the code base, I've
>> since reconsidered my position. If we get in a situation where the vgic
>> is not initialized, maybe it would be better to just WARN_ON and return
>> early rather than killing the whole box. Thoughts?
>>
> 
> The distinction to me is whether this will cause fatal crashes or
> exploits down the road if we're working on uninitialized data.  If all
> that can happen if the vgic is not initialized, is that the guest
> doesn't see interrupts, for example, then a WARN_ON is appropriate.
> 
> Which is the case here?
> 
> That being said, do we need this at all?  This is in the critial path
> and is actually measurable (I know this from my work on the other timer
> series), so it's better to get rid of it if we can.  Can we simply
> convince ourselves this will never happen, and is the code ever likely
> to change so that it gets called with the vgic disabled later?

That'd be the best course of action. I remember us reworking some of
that in the now defunct vgic-less series. Maybe we could salvage that
code, if only for the time we spent on it...

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 v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Christoffer Dall]

On Sun, Jan 29, 2017 at 03:21:06PM +, Marc Zyngier wrote:
> On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  
> wrote:
> > Now that we maintain the EL1 physical timer register states of VMs,
> > update the physical timer interrupt level along with the virtual one.
> >
> > Note that the emulated EL1 physical timer is not mapped to any hardware
> > timer, so we call a proper vgic function.
> >
> > Signed-off-by: Jintack Lim 
> > ---
> >  virt/kvm/arm/arch_timer.c | 20 
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> > index 0f6e935..3b6bd50 100644
> > --- a/virt/kvm/arm/arch_timer.c
> > +++ b/virt/kvm/arm/arch_timer.c
> > @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct 
> > kvm_vcpu *vcpu, bool new_level,
> > WARN_ON(ret);
> >  }
> >  
> > +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> > +struct arch_timer_context *timer)
> > +{
> > +   int ret;
> > +
> > +   BUG_ON(!vgic_initialized(vcpu->kvm));
> 
> Although I've added my fair share of BUG_ON() in the code base, I've
> since reconsidered my position. If we get in a situation where the vgic
> is not initialized, maybe it would be better to just WARN_ON and return
> early rather than killing the whole box. Thoughts?
> 

The distinction to me is whether this will cause fatal crashes or
exploits down the road if we're working on uninitialized data.  If all
that can happen if the vgic is not initialized, is that the guest
doesn't see interrupts, for example, then a WARN_ON is appropriate.

Which is the case here?

That being said, do we need this at all?  This is in the critial path
and is actually measurable (I know this from my work on the other timer
series), so it's better to get rid of it if we can.  Can we simply
convince ourselves this will never happen, and is the code ever likely
to change so that it gets called with the vgic disabled later?


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

Re: [RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Marc Zyngier]

On Fri, Jan 27 2017 at 01:04:56 AM, Jintack Lim  wrote:
> Now that we maintain the EL1 physical timer register states of VMs,
> update the physical timer interrupt level along with the virtual one.
>
> Note that the emulated EL1 physical timer is not mapped to any hardware
> timer, so we call a proper vgic function.
>
> Signed-off-by: Jintack Lim 
> ---
>  virt/kvm/arm/arch_timer.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 0f6e935..3b6bd50 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct kvm_vcpu 
> *vcpu, bool new_level,
>   WARN_ON(ret);
>  }
>  
> +static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> +  struct arch_timer_context *timer)
> +{
> + int ret;
> +
> + BUG_ON(!vgic_initialized(vcpu->kvm));

Although I've added my fair share of BUG_ON() in the code base, I've
since reconsidered my position. If we get in a situation where the vgic
is not initialized, maybe it would be better to just WARN_ON and return
early rather than killing the whole box. Thoughts?

> +
> + timer->irq.level = new_level;
> + trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
> +timer->irq.level);
> + ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer->irq.irq,
> +   timer->irq.level);
> + WARN_ON(ret);
> +}
> +
>  /*
>   * Check if there was a change in the timer state (should we raise or lower
>   * the line level to the GIC).
> @@ -188,6 +203,7 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
>  {
>   struct arch_timer_cpu *timer = >arch.timer_cpu;
>   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
> + struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>  
>   /*
>* If userspace modified the timer registers via SET_ONE_REG before
> @@ -201,6 +217,10 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
>   if (kvm_timer_should_fire(vcpu, vtimer) != vtimer->irq.level)
>   kvm_timer_update_mapped_irq(vcpu, !vtimer->irq.level, vtimer);
>  
> + /* The emulated EL1 physical timer irq is not mapped to hardware */

Maybe a slightly better comment would be saying that we're using a
purely virtual interrupt, unrelated to the hardware interrupt.

> + if (kvm_timer_should_fire(vcpu, ptimer) != ptimer->irq.level)
> + kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
> +
>   return 0;
>  }

Otherwise looks good.

  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

[RFC v2 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Jintack Lim]

Now that we maintain the EL1 physical timer register states of VMs,
update the physical timer interrupt level along with the virtual one.

Note that the emulated EL1 physical timer is not mapped to any hardware
timer, so we call a proper vgic function.

Signed-off-by: Jintack Lim 
---
 virt/kvm/arm/arch_timer.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 0f6e935..3b6bd50 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -180,6 +180,21 @@ static void kvm_timer_update_mapped_irq(struct kvm_vcpu 
*vcpu, bool new_level,
WARN_ON(ret);
 }
 
+static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
+struct arch_timer_context *timer)
+{
+   int ret;
+
+   BUG_ON(!vgic_initialized(vcpu->kvm));
+
+   timer->irq.level = new_level;
+   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
+  timer->irq.level);
+   ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer->irq.irq,
+ timer->irq.level);
+   WARN_ON(ret);
+}
+
 /*
  * Check if there was a change in the timer state (should we raise or lower
  * the line level to the GIC).
@@ -188,6 +203,7 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
/*
 * If userspace modified the timer registers via SET_ONE_REG before
@@ -201,6 +217,10 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
if (kvm_timer_should_fire(vcpu, vtimer) != vtimer->irq.level)
kvm_timer_update_mapped_irq(vcpu, !vtimer->irq.level, vtimer);
 
+   /* The emulated EL1 physical timer irq is not mapped to hardware */
+   if (kvm_timer_should_fire(vcpu, ptimer) != ptimer->irq.level)
+   kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
+
return 0;
 }
 
-- 
1.9.1


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