target-arm: kvm: use KVM_SET_SREGS to set target to Cortex A15

Recent kernels use this to set the cpu and features (currently, only
the A15 is supported).

Note that this causes the registers in the CPU to be initialized, so
it's important that all CPUs are created first (they are, as it turns
out).

This code ignores errors, for backwards compatibility with older
kernels.

Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
> Recent kernels use this to set the cpu and features (currently, only
> the A15 is supported).
>
> Note that this causes the registers in the CPU to be initialized, so
> it's important that all CPUs are created first (they are, as it turns
> out).
>
> This code ignores errors, for backwards compatibility with older
> kernels.
>
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>

I haven't actually been posting the ARM KVM patches to qemu-devel
thus far, so this patch is a bit of a non-sequitur for qemu-devel readers.
(We've been posting patches to kvmarm only, like the kernel patches.)

Anyway:
 * updates to qemu's kernel headers should be separate patches
and ideally the result of running the automatic 'update headers'
script. [I'm going to squash all the kernel header changes together
before we submit a patch series properly to qemu-devel, so patches
which combine header and code changes require me to untangle them]
 * any code which includes compatibility workarounds for earlier
versions of in-development kernel code should have a FIXME comment
so we can remember to undo the workaround before submitting.

thanks
-- PMM

On 13.07.2012, at 05:37, Rusty Russell <rusty.russell@linaro.org> wrote:

> Recent kernels use this to set the cpu and features (currently, only
> the A15 is supported).
> 
> Note that this causes the registers in the CPU to be initialized, so
> it's important that all CPUs are created first (they are, as it turns
> out).
> 
> This code ignores errors, for backwards compatibility with older
> kernels.
> 
> Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> 
> diff --git a/linux-headers/asm-arm/kvm.h b/linux-headers/asm-arm/kvm.h
> index 38ff1d6..988890a 100644
> --- a/linux-headers/asm-arm/kvm.h
> +++ b/linux-headers/asm-arm/kvm.h
> @@ -66,7 +66,13 @@ struct kvm_regs {
> 
> };
> 
> +/* Supported Processor Types */
> +#define KVM_ARM_TARGET_CORTEX_A15    (0xC0F)
> +
> struct kvm_sregs {
> +    __u32 target;
> +    __u32 num_features;
> +    __u32 features[14];
> };

Are you sure you want to use sregs? We did the mistake of reusing it on ppc, but that doesn't mean you need to repeat the same one :)

Basically sregs are an x86 specific struct for its segment register information. I'm quite sure that this is not what your use of them is here.

In general you have 3 sane options for API additions:

  1) ONE_REG

If you have information that is syncable in register granularity, this is what you want. We will (once necessary) add an API that allows us to sync multiple ONE_REG items at once, so it'll be like a big dynamic kvm_regs implementation. I don't think this applies here though.

2) new ioctl

Just define an ARM specific ioctl that sets the compat features. Believe me, it will make your code easier to read.

3) ENABLE_CAP

If you only need to enable a feature and care about backwards compatibility of the API (which you don't yet), this is a good one. it basically allows you to enable new features in newer kernel versions which would otherwise break compatibility. You can also pass arbitrary data to ENABLE_CAP to pass in additional information.


Usually in released KVM versions, I'd use ENABLE_CAP for a feature like this. Since you can still define and break the API as you wish, option 2 works as well :).

Alex

> 
> struct kvm_fpu {
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 29bb51f..67d005f 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -34,7 +34,13 @@ int kvm_arch_init(KVMState *s)
> 
> int kvm_arch_init_vcpu(CPUARMState *env)
> {
> -    return 0;
> +    struct kvm_sregs sregs;
> +
> +    sregs.target = KVM_ARM_TARGET_CORTEX_A15;
> +    sregs.num_features = 0;
> +
> +    /* Ignore failure for compatibility with old kvm versions. */
> +    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;
> }
> 
> int kvm_arch_put_registers(CPUARMState *env, int level)
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
> > struct kvm_sregs {
> > +    __u32 target;
> > +    __u32 num_features;
> > +    __u32 features[14];
> > };
> 
> Are you sure you want to use sregs? We did the mistake of reusing it
> on ppc, but that doesn't mean you need to repeat the same one :)
> 
> Basically sregs are an x86 specific struct for its segment register
> information. I'm quite sure that this is not what your use of them is
> here.

Since each arch is given a hook already, I just abused it.  I'll change
this to a fresh KVM_ARM_SET_TARGET ioctl.  

> 3) ENABLE_CAP
> 
> If you only need to enable a feature and care about backwards
> compatibility of the API (which you don't yet), this is a good one. it
> basically allows you to enable new features in newer kernel versions
> which would otherwise break compatibility. You can also pass arbitrary
> data to ENABLE_CAP to pass in additional information.

Hmm, it's not quite a clean fit: this bitmap is for guest features, not
kvm ones.  Which ones you can enable depends on the target CPU, at least
in theory.

eg. FP/NEON support and debug register support are (in theory) optional
features for an implementation.  There may be more in future, I guess.

And you really want to initialize this all at the same time; eg. the cpu
identification registers need to be initialized depending on the
presence of various features.  It's also possible that various features
may be related, so you can't turn a single one off at a time.

Currently, it's a bit theoretical, since we don't have any guest
features, but it was suggested that we'll want them in future.

Cheers,
Rusty.

On Fri, 13 Jul 2012 09:06:16 +0100, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
> > Recent kernels use this to set the cpu and features (currently, only
> > the A15 is supported).
> >
> > Note that this causes the registers in the CPU to be initialized, so
> > it's important that all CPUs are created first (they are, as it turns
> > out).
> >
> > This code ignores errors, for backwards compatibility with older
> > kernels.
> >
> > Signed-off-by: Rusty Russell <rusty.russell@linaro.org>
> 
> I haven't actually been posting the ARM KVM patches to qemu-devel
> thus far, so this patch is a bit of a non-sequitur for qemu-devel readers.
> (We've been posting patches to kvmarm only, like the kernel patches.)

OK; given the wider API implications I thought I'd add them.

> Anyway:
>  * updates to qemu's kernel headers should be separate patches
> and ideally the result of running the automatic 'update headers'
> script. [I'm going to squash all the kernel header changes together
> before we submit a patch series properly to qemu-devel, so patches
> which combine header and code changes require me to untangle them]

OK.  This is totally a hack until we know how we're going to transition.

>  * any code which includes compatibility workarounds for earlier
> versions of in-development kernel code should have a FIXME comment
> so we can remember to undo the workaround before submitting.

Sure.  It's up to you whether you want a flag day: tell me and I'll
re-spin the patches?

Thanks,
Rusty.

On 16.07.2012, at 09:19, Rusty Russell wrote:

> On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
>>> struct kvm_sregs {
>>> +    __u32 target;
>>> +    __u32 num_features;
>>> +    __u32 features[14];
>>> };
>> 
>> Are you sure you want to use sregs? We did the mistake of reusing it
>> on ppc, but that doesn't mean you need to repeat the same one :)
>> 
>> Basically sregs are an x86 specific struct for its segment register
>> information. I'm quite sure that this is not what your use of them is
>> here.
> 
> Since each arch is given a hook already, I just abused it.  I'll change
> this to a fresh KVM_ARM_SET_TARGET ioctl.  
> 
>> 3) ENABLE_CAP
>> 
>> If you only need to enable a feature and care about backwards
>> compatibility of the API (which you don't yet), this is a good one. it
>> basically allows you to enable new features in newer kernel versions
>> which would otherwise break compatibility. You can also pass arbitrary
>> data to ENABLE_CAP to pass in additional information.
> 
> Hmm, it's not quite a clean fit: this bitmap is for guest features, not
> kvm ones.  Which ones you can enable depends on the target CPU, at least
> in theory.

Sure. You'd do (pseudo-code):

  kvm_ioctl(fd, KVM_ENABLE_CAP, KVM_CAP_ARM_CPU_TARGET, ARM_CPU_NEON | ARM_CPU_VFP16 | ARM_CPU_V7);

> 
> eg. FP/NEON support and debug register support are (in theory) optional
> features for an implementation.  There may be more in future, I guess.
> 
> And you really want to initialize this all at the same time; eg. the cpu
> identification registers need to be initialized depending on the
> presence of various features.  It's also possible that various features
> may be related, so you can't turn a single one off at a time.

That argument does hold true. We are in that mess with ppc, where we don't have a proper "init" ioctl. So you'd definitely be better off defining an extensible init handshake now, so you can easily configure the kernel side early on.

Currently for PPC, we just try to either make things stateless or put additional constraints on certain CAPs, like "may only be set before the first vcpu run". Not as pretty as doing it cleanly.

> Currently, it's a bit theoretical, since we don't have any guest
> features, but it was suggested that we'll want them in future.

Yes, hence I'm trying to get you guys to something where you won't be stuck in 6 months from now with a stable ABI that totally doesn't fit you :)


Alex

On 07/16/2012 10:19 AM, Rusty Russell wrote:
> On Fri, 13 Jul 2012 12:06:26 +0200, Alexander Graf <agraf@suse.de> wrote:
>> > struct kvm_sregs {
>> > +    __u32 target;
>> > +    __u32 num_features;
>> > +    __u32 features[14];
>> > };
>> 
>> Are you sure you want to use sregs? We did the mistake of reusing it
>> on ppc, but that doesn't mean you need to repeat the same one :)
>> 
>> Basically sregs are an x86 specific struct for its segment register
>> information. I'm quite sure that this is not what your use of them is
>> here.
> 
> Since each arch is given a hook already, I just abused it.  I'll change
> this to a fresh KVM_ARM_SET_TARGET ioctl.  

I guess this is equivalent to KVM_SET_CPUID2 on x86.  Note that's a vcpu
ioctl.

On 13 July 2012 04:37, Rusty Russell <rusty.russell@linaro.org> wrote:
>  int kvm_arch_init_vcpu(CPUARMState *env)
>  {
> -    return 0;
> +    struct kvm_sregs sregs;
> +
> +    sregs.target = KVM_ARM_TARGET_CORTEX_A15;
> +    sregs.num_features = 0;

We need to add a check somewhere that if we're not emulating
an A15 then we either fail noisily or silently drop back to
TCG. Then we should have an assert in here that env refers
to an A15 I guess. [not yet figured out how to do that, I
don't want to look at the cpu->midr, we only just finished
cleaning out all the references to that. Maybe there should
be a field in the struct ARMCPU which gives the KVM_ARM_TARGET_*
value to use?]

Unfortunately I can't find a good place to put the CPU type
check -- ideally we would fail kvm_init() as this would make
us fall back to TCG in the usual way, but in kvm_init() you
don't yet know what guest CPU you're going to be emulating...

> +
> +    /* Ignore failure for compatibility with old kvm versions. */
> +    return kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs) ? 0 : 0;

I assume this weird "? 0 : 0" construct is going to go away
when we drop the back-compat?

Since this back-compat code is going to go away within a
few weeks, it would make my life easier if all the back
compat code was flagged with ifdefs or something, so we
don't leave it in by mistake (and so I know what not to
worry about reviewing in patches).

-- PMM

On 17 July 2012 18:31, Peter Maydell <peter.maydell@linaro.org> wrote:
> We need to add a check somewhere that if we're not emulating
> an A15 then we either fail noisily or silently drop back to
> TCG. Then we should have an assert in here that env refers
> to an A15 I guess. [not yet figured out how to do that, I
> don't want to look at the cpu->midr, we only just finished
> cleaning out all the references to that. Maybe there should
> be a field in the struct ARMCPU which gives the KVM_ARM_TARGET_*
> value to use?]
>
> Unfortunately I can't find a good place to put the CPU type
> check -- ideally we would fail kvm_init() as this would make
> us fall back to TCG in the usual way, but in kvm_init() you
> don't yet know what guest CPU you're going to be emulating...

Having thought more about this, I don't think there is anywhere
we can do a check that would give the failure semantics we'd
like (ie respecting whatever the user's accel= options were).
We don't know what the guest CPU is until we get to trying to
construct the CPU in the machine. But other devices the machine
constructs might want to know kvm vs tcg (eg irqchip), and we
can't enforce ordering restrictions on device construction really.
(That's ignoring the question of whether we could even postpone
kvm_init() at all, which I'm not convinced wouldn't break something.)

So I can't see anything better than "complain and return failure
from kvm_arch_init_vcpu() if the guest CPU isn't going to work".

-- PMM