Re: [RFC 8/8] KVM: arm/arm64: Emulate the EL1 phys timer register access

[Jintack Lim]

On Tue, Jan 10, 2017 at 2:40 PM, Christoffer Dall
<christoffer.dall@xxxxxxxxxx> wrote:
> On Tue, Jan 10, 2017 at 12:36:36PM -0500, Jintack Lim wrote:
>> On Mon, Jan 9, 2017 at 7:16 AM, Christoffer Dall
>> <christoffer.dall@xxxxxxxxxx> wrote:
>> > On Mon, Dec 26, 2016 at 12:12:06PM -0500, Jintack Lim wrote:
>> >> Emulate read and write operations to CNTP_TVAL, CNTP_CVAL and CNTP_CTL.
>> >> Now the VM is able to use the EL1 physical timer.
>> >>
>> >> Signed-off-by: Jintack Lim <jintack@xxxxxxxxxxxxxxx>
>> >> ---
>> >>  arch/arm64/kvm/sys_regs.c    | 35 ++++++++++++++++++++++++++++++++---
>> >>  include/kvm/arm_arch_timer.h |  3 +++
>> >>  virt/kvm/arm/arch_timer.c    |  4 ++--
>> >>  3 files changed, 37 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> >> index fd9e747..7cef94f 100644
>> >> --- a/arch/arm64/kvm/sys_regs.c
>> >> +++ b/arch/arm64/kvm/sys_regs.c
>> >> @@ -824,7 +824,15 @@ static bool access_cntp_tval(struct kvm_vcpu *vcpu,
>> >>               struct sys_reg_params *p,
>> >>               const struct sys_reg_desc *r)
>> >>  {
>> >> -     kvm_inject_undefined(vcpu);
>> >> +     struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
>> >> +     cycle_t now = kvm_phys_timer_read();
>> >> +
>> >> +     if (p->is_write) {
>> >> +             ptimer->cnt_cval = p->regval + now;
>> >> +             kvm_timer_emulate(vcpu, ptimer);
>> >
>> > Hmm, do we really need those calls here?
>> >
>> > I guess I'm a little confused about exactly what the kvm_timer_emulate()
>> > function is supposed to do, and it feels to me like these handlers
>> > should just record what the guest is asking the kernel to do and the
>> > logic of handling the additional timer should be moved into the run path
>> > as much as possible.
>>
>> I think it's a design decision. As you suggested, it's simple to do
>> set up the background timer on entry to the VM, cancel it on exit, but
>> since that's on the critical path it may have some impact on the
>> performance, especially the world switch cost. To avoid
>> canceling/setting up timer every world switch, I choose to schedule
>> the physical timer here. I haven't compared the cost of the two
>> alternatives, though.
>>
>
> I'd definitely like to avoid us scheduling soft timers on the host if
> that's not even necessary in the first place, so I'd like to get that
> clear first, and as I said on the previous patch I think it's better to
> get a working solution that we understand firt, and then optimize on
> that later based on real results.

Ok, it makes sense. I'll respin!

>
> -Christoffer
>