[RFC] [PATCH] kvm: arm: Introduce error code KVM_EINVARIANT

[RFC] [PATCH] kvm: arm: Introduce error code KVM_EINVARIANT

[Manish Jaggi]

CCing a larger audience.
Please review.

On 10/23/2018 03:51 PM, Jaggi, Manish wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
>
> This patch introduces an error code KVM_EINVARIANT which is returned
> by KVM when userland tries to set an invariant register.
>
> The need for this error code is in VM Migration for arm64.
> ARM64 systems use mainly -machine virt -cpu host as parameter to qemu.
> Migration requires both Source and destination machines to have same
> physical cpu. There are cases where the overall architecture of CPU is
> same but the next version of the chip with some bug fixes which have no
> effect on qemu operation. In such cases invariant registers like MIDR
> have a different value.
> Currently Migration fails in such cases.
>
> Rather than sending a EINVAL, a specifc error code will help
> userland program the guest invariant register by querying the migrated
> host machines invariant registers.
>
> Qemu will have a parameter -hostinvariant along with checking of this
> error code. So it can be safely assumed that the feature is opt-in
>
> Corresponding Qemu patchset can be found at :
> https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05048.html
>
>
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   arch/arm64/kvm/sys_regs.c     | 9 ++++-----
>   include/uapi/linux/kvm_para.h | 1 +
>   2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22fbbdb..78ffc02 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1111,7 +1111,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
>   
>   	/* This is what we mean by invariant: you can't change it. */
>   	if (val != read_id_reg(rd, raz))
> -		return -EINVAL;
> +		return -KVM_EINVARIANT;
>   
>   	return 0;
>   }
> @@ -2254,9 +2254,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
>   		return err;
>   
>   	/* This is what we mean by invariant: you can't change it. */
> -	if (r->val != val)
> -		return -EINVAL;
> -
> +	if (r->val != val)	
> +		return -KVM_EINVARIANT;
>   	return 0;
>   }
>   
> @@ -2335,7 +2334,7 @@ static int demux_c15_set(u64 id, void __user *uaddr)
>   
>   		/* This is also invariant: you can't change it. */
>   		if (newval != get_ccsidr(val))
> -			return -EINVAL;
> +			return -KVM_EINVARIANT;
>   		return 0;
>   	default:
>   		return -ENOENT;
> diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
> index 6c0ce49..4358669 100644
> --- a/include/uapi/linux/kvm_para.h
> +++ b/include/uapi/linux/kvm_para.h
> @@ -17,6 +17,7 @@
>   #define KVM_E2BIG		E2BIG
>   #define KVM_EPERM		EPERM
>   #define KVM_EOPNOTSUPP		95
> +#define KVM_EINVARIANT          96
>   
>   #define KVM_HC_VAPIC_POLL_IRQ		1
>   #define KVM_HC_MMU_OP			2

[RFC] [PATCH] kvm: arm: Introduce error code KVM_EINVARIANT

[Marc Zyngier]

On Sat, 10 Nov 2018 22:18:47 +0000,
Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
> 
> 
> CCing a larger audience.
> Please review.
> 
> On 10/23/2018 03:51 PM, Jaggi, Manish wrote:
> > From: Manish Jaggi <manish.jaggi@cavium.com>
> >
> > This patch introduces an error code KVM_EINVARIANT which is returned
> > by KVM when userland tries to set an invariant register.
> >
> > The need for this error code is in VM Migration for arm64.
> > ARM64 systems use mainly -machine virt -cpu host as parameter to qemu.
> > Migration requires both Source and destination machines to have same
> > physical cpu. There are cases where the overall architecture of CPU is
> > same but the next version of the chip with some bug fixes which have no
> > effect on qemu operation. In such cases invariant registers like MIDR
> > have a different value.
> > Currently Migration fails in such cases.
> >
> > Rather than sending a EINVAL, a specifc error code will help
> > userland program the guest invariant register by querying the migrated
> > host machines invariant registers.

But failing migration is a good thing, right? How do you expect that
the guest will be happy to see a new CPU revision right in the middle
of its execution? Do you also propose that QEMU starts doing that for
big-little systems? After all, if ignoring the differences in some
registers is harmless for migration, surely that should be the case in
a single system, right?

> >
> > Qemu will have a parameter -hostinvariant along with checking of this
> > error code. So it can be safely assumed that the feature is opt-in

You're changing the ABI without any buy in from userspace, which is
not acceptable.

As it stands, this patch creates a number of issues without solving
any. Things to think about:

- How does errata management works when migrating to a different
  system?
- big-little, as mentioned above
- Are all invariant registers equal? A different MIDR has the same
  effect as a different MMFR0?

Instead of papering over architectural constants i a system, how about
allowing the relevant ID registers to be overloaded when not
incompatible?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

[Qemu-devel] [RFC] [PATCH] kvm: arm: Introduce error code KVM_EINVARIANT

[Andrew Jones]

On Sun, Nov 11, 2018 at 11:36:44AM +0000, Marc Zyngier wrote:
> On Sat, 10 Nov 2018 22:18:47 +0000,
> Manish Jaggi <mjaggi@caviumnetworks.com> wrote:
> > 
> > 
> > CCing a larger audience.
> > Please review.
> > 
> > On 10/23/2018 03:51 PM, Jaggi, Manish wrote:
> > > From: Manish Jaggi <manish.jaggi@cavium.com>
> > >
> > > This patch introduces an error code KVM_EINVARIANT which is returned
> > > by KVM when userland tries to set an invariant register.
> > >
> > > The need for this error code is in VM Migration for arm64.
> > > ARM64 systems use mainly -machine virt -cpu host as parameter to qemu.
> > > Migration requires both Source and destination machines to have same
> > > physical cpu. There are cases where the overall architecture of CPU is
> > > same but the next version of the chip with some bug fixes which have no
> > > effect on qemu operation. In such cases invariant registers like MIDR
> > > have a different value.
> > > Currently Migration fails in such cases.
> > >
> > > Rather than sending a EINVAL, a specifc error code will help
> > > userland program the guest invariant register by querying the migrated
> > > host machines invariant registers.
> 
> But failing migration is a good thing, right? How do you expect that
> the guest will be happy to see a new CPU revision right in the middle
> of its execution? Do you also propose that QEMU starts doing that for
> big-little systems? After all, if ignoring the differences in some
> registers is harmless for migration, surely that should be the case in
> a single system, right?
> 
> > >
> > > Qemu will have a parameter -hostinvariant along with checking of this
> > > error code. So it can be safely assumed that the feature is opt-in
> 
> You're changing the ABI without any buy in from userspace, which is
> not acceptable.
> 
> As it stands, this patch creates a number of issues without solving
> any. Things to think about:
> 
> - How does errata management works when migrating to a different
>   system?
> - big-little, as mentioned above
> - Are all invariant registers equal? A different MIDR has the same
>   effect as a different MMFR0?
> 
> Instead of papering over architectural constants i a system, how about
> allowing the relevant ID registers to be overloaded when not
> incompatible?
> 

In QEMU, I think we need nail down how we define '-cpu host' for kvmarm.
IMO, '-cpu host' is what libvirt calls "host-passthrough"[*], and indeed
migrating a guest using host-passthrough is super risky. You need to
know what you're doing, and likely what you'll want to do is migrate to
an _identical_ host. We should start looking at building cpu models to
better support migration, but errata complicates that. However, for the
sake of argument, let's assume we solved those problems and completed the
implementation of cpu models - so we would no longer be using '-cpu host'
for a "typical" config. So what would '-cpu host' mean? It appears the
current semantics are "source host passthrough", similar to
"host-model"[*]. Rather than "whatever host I'm running on host
passthrough", which is what I think we want and what this patch and the
QEMU counterpart changes seem to be aiming at implementing. Anyway,
whichever of those two semantics we choose for '-cpu host', the user will
still need to know what they're doing and to assume the risks. Without
cpu models, I'm not even sure discussing migration safety makes sense.

(Yeah, I know I ignored big-little. IMO, big-little is an upper layer
problem, as it should just be a vcpu thread to cpu set affinity
assignment issue.)

Thanks,
drew

[*] https://wiki.openstack.org/wiki/LibvirtXMLCPUModel

[RFC] [PATCH] kvm: arm: Introduce error code KVM_EINVARIANT

[Christoffer Dall]

Hi Manish,

On Sat, Nov 10, 2018 at 10:18:47PM +0000, Manish Jaggi wrote:
> 
> CCing a larger audience.
> Please review.
> 
> On 10/23/2018 03:51 PM, Jaggi, Manish wrote:
> > From: Manish Jaggi <manish.jaggi@cavium.com>
> >
> > This patch introduces an error code KVM_EINVARIANT which is returned
> > by KVM when userland tries to set an invariant register.
> >
> > The need for this error code is in VM Migration for arm64.
> > ARM64 systems use mainly -machine virt -cpu host as parameter to qemu.
> > Migration requires both Source and destination machines to have same
> > physical cpu. There are cases where the overall architecture of CPU is
> > same but the next version of the chip with some bug fixes which have no
> > effect on qemu operation. In such cases invariant registers like MIDR
> > have a different value.
> > Currently Migration fails in such cases.
> >
> > Rather than sending a EINVAL, a specifc error code will help
> > userland program the guest invariant register by querying the migrated
> > host machines invariant registers.
> >
> > Qemu will have a parameter -hostinvariant along with checking of this
> > error code. So it can be safely assumed that the feature is opt-in
> >
> > Corresponding Qemu patchset can be found at :
> > https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg05048.html
> >
> >
> > Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> > ---
> >   arch/arm64/kvm/sys_regs.c     | 9 ++++-----
> >   include/uapi/linux/kvm_para.h | 1 +
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 22fbbdb..78ffc02 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1111,7 +1111,7 @@ static int __set_id_reg(const struct sys_reg_desc *rd, void __user *uaddr,
> >   
> >   	/* This is what we mean by invariant: you can't change it. */
> >   	if (val != read_id_reg(rd, raz))
> > -		return -EINVAL;
> > +		return -KVM_EINVARIANT;
> >   
> >   	return 0;
> >   }
> > @@ -2254,9 +2254,8 @@ static int set_invariant_sys_reg(u64 id, void __user *uaddr)
> >   		return err;
> >   
> >   	/* This is what we mean by invariant: you can't change it. */
> > -	if (r->val != val)
> > -		return -EINVAL;
> > -
> > +	if (r->val != val)	
> > +		return -KVM_EINVARIANT;
> >   	return 0;
> >   }
> >   
> > @@ -2335,7 +2334,7 @@ static int demux_c15_set(u64 id, void __user *uaddr)
> >   
> >   		/* This is also invariant: you can't change it. */
> >   		if (newval != get_ccsidr(val))
> > -			return -EINVAL;
> > +			return -KVM_EINVARIANT;
> >   		return 0;
> >   	default:
> >   		return -ENOENT;
> > diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h
> > index 6c0ce49..4358669 100644
> > --- a/include/uapi/linux/kvm_para.h
> > +++ b/include/uapi/linux/kvm_para.h
> > @@ -17,6 +17,7 @@
> >   #define KVM_E2BIG		E2BIG
> >   #define KVM_EPERM		EPERM
> >   #define KVM_EOPNOTSUPP		95
> > +#define KVM_EINVARIANT          96
> >   
> >   #define KVM_HC_VAPIC_POLL_IRQ		1
> >   #define KVM_HC_MMU_OP			2
> 

Eh, unless I'm severely missing something, I think you're confusing how
these error codes are used.

The error codes in include/uapi/linux/kvm_para.h are for hypercalls from
guests, not return values to userspace.  For the latter, we don't
have a habit of inventing new constants for something subsystem
specific, but instead we use the existing error codes which can be
understood by userspace software.

What's more, changing an error code is an ABI change and not something
you can just do at will.  If we are overloading EINVAL with the
KVM_SET_ONE_REG syscall (are we?), then you need to find some way to
communicate the additional information you need to userspace without
interfering with legacy userspace software's interaction with the
kernel.


Thanks,

    Christoffer