From: marc.zyngier@arm•com (Marc Zyngier)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH v2] ARM/KVM: save and restore generic timer registers
Date: Fri, 05 Jul 2013 15:44:28 +0100 [thread overview]
Message-ID: <51D6DBCC.9040807@arm.com> (raw)
In-Reply-To: <51D6D377.70708@linaro.org>
On 05/07/13 15:08, Andre Przywara wrote:
> Hi Marc,
>
> >> ...
>>>
>>> +int kvm_arm_num_timer_regs(void)
>>> +{
>>> + return 3;
>>> +}
>>> +
>>> +int kvm_arm_copy_timer_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> +{
>>> + if (put_user(KVM_REG_ARM_TIMER_CTL, uindices))
>>> + return -EFAULT;
>>> + uindices++;
>>> + if (put_user(KVM_REG_ARM_TIMER_CNT, uindices))
>>> + return -EFAULT;
>>> + uindices++;
>>> + if (put_user(KVM_REG_ARM_TIMER_CVAL, uindices))
>>> + return -EFAULT;
>>
>> So these macros are going to break arm64. Any chance you could introduce
>> them at the same time on both platforms? The rest of the work can be
>> delayed, but you shouldn't break arm64 (you'd expect me to say that,
>> wouldn't you? ;-).
>
> Is that just due to KVM_REG_ARM instead of KVM_REG_ARM64?
> Or do you expect the numbering to be completely different since there is
> no mrc/mcr anymore (IIRC)?
Both. The encoding is different (32bit is encoded CRn/CRm/Op1/Op2, and
64bit is Op0/Op1/CRn/CRm/Op2), and the KVM_REG_ARM64 is different too.
> Is put_user an issue here (should not, right?)
No, except for the reason outlined below.
> Is there already a document describing arch timer access on AARCH64?
No, but it is strikingly similar to the AArch32. Have a look at
arch/arm64/include/asm/arch_timer.h and arch/arm64/kvm/hyp.S for details.
> If I am thinking in a totally wrong direction, please bear with me and
> feel free to point me to the right one ;-)
> /me is now looking at getting a cross compiler to see what you mean...
That'd be a good thing! ;-)
>> Also, I'd like to keep userspace access out of the timer code itself.
>> Low level code shouldn't have to know about that. Can you create proper
>> accessors instead, and move whole userspace access to coproc.c?
>
> IIRC Christoffer recommended to keep this code completely out of
> coproc.c ;-) This also helps to keep coproc.c clean of ARCH_TIMER ifdefs
> in coproc.c (which I completely forgot in this version, btw, but that's
> already fixed).
I thought we agreed on moving the userspace access out of the timer
code. In a reply to the email you just quoted, Christoffer says:
"I'm fine with this, coproc.c or guest.c - either way.".
The man has spoken! ;-)
>>
>>> + return 0;
>>> +}
>>> +
>>> +int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>>> +{
>>> + struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>> + void __user *uaddr = (void __user *)(long)reg->addr;
>>> + u64 val;
>>> + int ret;
>>> +
>>> + ret = copy_from_user(&val, uaddr, KVM_REG_SIZE(reg->id));
>>> + if (ret != 0)
>>> + return ret;
>>> +
>>> + switch (reg->id) {
>>> + case KVM_REG_ARM_TIMER_CTL:
>>> + timer->cntv_ctl = val;
>>> + break;
>>> + case KVM_REG_ARM_TIMER_CNT:
>>> + vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - val;
>>
>> I just realized what bothers me here: You're computing cntvoff on a per
>> vcpu basis, while this is a VM property. Which means that as you're
>> restoring vcpus, you'll be changing cntvoff - sounds like a bad idea to me.
>>
>> The counter is really global. Do we have a way to handle VM-wide
>> registers? I think Christoffer was trying to some a similar thing with
>> the GIC...
>
> So the consensus of this discussion was just to block writing when the
> VCPU is running, right? Or is there something else?
Yup. No update while vcpus are running.
M.
--
Jazz is not dead. It just smells funny...
prev parent reply other threads:[~2013-07-05 14:44 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-11 15:16 [PATCH v2] ARM/KVM: save and restore generic timer registers Andre Przywara
2013-06-19 21:16 ` Christoffer Dall
2013-06-20 10:10 ` Marc Zyngier
2013-06-20 17:09 ` Christoffer Dall
2013-06-20 17:19 ` Marc Zyngier
2013-06-20 18:32 ` Christoffer Dall
2013-06-20 18:39 ` Marc Zyngier
2013-06-20 19:29 ` Peter Maydell
2013-06-20 20:37 ` Christoffer Dall
2013-06-20 21:55 ` Alexander Graf
2013-06-20 21:59 ` Peter Maydell
2013-06-20 22:02 ` Alexander Graf
2013-06-20 22:48 ` Christoffer Dall
2013-07-05 14:08 ` Andre Przywara
2013-07-05 14:44 ` Marc Zyngier [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51D6DBCC.9040807@arm.com \
--to=marc.zyngier@arm$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox