Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

[Peter Maydell]

On 27 September 2018 at 02:13,  <mjaggi@caviumnetworks.com> wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
>
> QEMU on arm systems use -machine virt -cpu host option for a VM.
> Migration thus is limited between machines with same cpu.
>
> This is a limitation if migration is desired between cpus which are of same
> family and have only few diferences like bug fixes which have no effect on
> VM operation. They just differ in say MIDR values.
>
> This patchset introduces a command line option -skipinvariant. Invariant
> registers will be skipped from being restored from guests context on migrated
> host.
>
> Mailing list discussion on this topic:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg560043.html

Hi; thanks for this patch. The issue I see with this patch
is that the KVM/ARM QEMU approach to system registers so far
has been "the kernel knows about these and it is in control".
So we ask the kernel for the list of registers, and just save
and restore those. That would suggest that if there are sysregs
where it's OK in fact to ignore a difference between two constant
register values, it should be the kernel doing the "actually, this
mismatch is OK" behaviour...

For instance, it's probably OK to ignore a MIDR_EL1 difference
that just indicates a minor revision bump; but not to ignore
one that indicates you just tried to migrate a Cortex-A53
over to a Cavium CPU.

thanks
-- PMM

Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

[Andrew Jones]

On Tue, Oct 02, 2018 at 02:07:38PM +0100, Peter Maydell wrote:
> On 27 September 2018 at 02:13,  <mjaggi@caviumnetworks.com> wrote:
> > From: Manish Jaggi <manish.jaggi@cavium.com>
> >
> > QEMU on arm systems use -machine virt -cpu host option for a VM.
> > Migration thus is limited between machines with same cpu.
> >
> > This is a limitation if migration is desired between cpus which are of same
> > family and have only few diferences like bug fixes which have no effect on
> > VM operation. They just differ in say MIDR values.
> >
> > This patchset introduces a command line option -skipinvariant. Invariant
> > registers will be skipped from being restored from guests context on migrated
> > host.
> >
> > Mailing list discussion on this topic:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg560043.html
> 
> Hi; thanks for this patch. The issue I see with this patch
> is that the KVM/ARM QEMU approach to system registers so far
> has been "the kernel knows about these and it is in control".
> So we ask the kernel for the list of registers, and just save
> and restore those. That would suggest that if there are sysregs
> where it's OK in fact to ignore a difference between two constant
> register values, it should be the kernel doing the "actually, this
> mismatch is OK" behaviour...

I don't think the kernel should have to maintain all that logic. If a user
wants to load guest registers, then the kernel should do it, as long as
it's safe from the host's integrity/security PoV, and the hardware would
actually support it. Anything that can only break the guest, but not the
host, should be allowed. The KVM userspace can certainly ask the kernel
what it recommends first (i.e. read the invariant registers first, before
deciding what to write), but the decision of what to write should be left
up to the user.

> 
> For instance, it's probably OK to ignore a MIDR_EL1 difference
> that just indicates a minor revision bump; but not to ignore
> one that indicates you just tried to migrate a Cortex-A53
> over to a Cavium CPU.

If the user does that, then the guest will break - oh well. That's not the
host kernel's problem.

Thanks,
drew

Re: [Qemu-devel] [RFC] [PATCH 3/3] arm: Skip invariant register restore

[Andrew Jones]

On Thu, Sep 27, 2018 at 01:13:54AM +0000, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> Invariant registers will be skipped from being restored from
> guests' context on migrated host.
> 
> Signed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> 
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 65f867d..2d89600 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -451,6 +451,9 @@ bool write_list_to_kvmstate(ARMCPU *cpu, int level)
>          default:
>              abort();
>          }
> +        if (skip_invariant && kvm_arm_is_invariant(&r)) {
> +            continue;
> +        }
>          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &r);
>          if (ret) {
>              /* We might fail for "unknown register" and also for
> -- 
> 1.8.3.1
> 
>

I think we should compare the invariants we're going to skip restoring
with their saved state and output messages when they don't match to the
migration log. That way when things go wrong we have a clue as to why.

Thanks,
drew

Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

[Peter Maydell]

On 4 October 2018 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
> On Tue, Oct 02, 2018 at 02:07:38PM +0100, Peter Maydell wrote:
>> Hi; thanks for this patch. The issue I see with this patch
>> is that the KVM/ARM QEMU approach to system registers so far
>> has been "the kernel knows about these and it is in control".
>> So we ask the kernel for the list of registers, and just save
>> and restore those. That would suggest that if there are sysregs
>> where it's OK in fact to ignore a difference between two constant
>> register values, it should be the kernel doing the "actually, this
>> mismatch is OK" behaviour...
>
> I don't think the kernel should have to maintain all that logic. If a user
> wants to load guest registers, then the kernel should do it, as long as
> it's safe from the host's integrity/security PoV, and the hardware would
> actually support it. Anything that can only break the guest, but not the
> host, should be allowed. The KVM userspace can certainly ask the kernel
> what it recommends first (i.e. read the invariant registers first, before
> deciding what to write), but the decision of what to write should be left
> up to the user.

I can see the logic of that approach. But right now QEMU
userspace knows basically nothing about the system registers
when we're using KVM: all that knowledge is in the kernel.
So we don't have a place really to put policy info (and
not really anywhere to put policy info that depends on the
host CPU type when mostly we let the kernel care about that).

>> For instance, it's probably OK to ignore a MIDR_EL1 difference
>> that just indicates a minor revision bump; but not to ignore
>> one that indicates you just tried to migrate a Cortex-A53
>> over to a Cavium CPU.
>
> If the user does that, then the guest will break - oh well. That's not the
> host kernel's problem.

Patching QEMU to ignore all attempts to write wrong values
to read-only registers would be easy. It just wouldn't
be very helpful to the user...

thanks
-- PMM

Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 4 October 2018 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
> > On Tue, Oct 02, 2018 at 02:07:38PM +0100, Peter Maydell wrote:
> >> Hi; thanks for this patch. The issue I see with this patch
> >> is that the KVM/ARM QEMU approach to system registers so far
> >> has been "the kernel knows about these and it is in control".
> >> So we ask the kernel for the list of registers, and just save
> >> and restore those. That would suggest that if there are sysregs
> >> where it's OK in fact to ignore a difference between two constant
> >> register values, it should be the kernel doing the "actually, this
> >> mismatch is OK" behaviour...
> >
> > I don't think the kernel should have to maintain all that logic. If a user
> > wants to load guest registers, then the kernel should do it, as long as
> > it's safe from the host's integrity/security PoV, and the hardware would
> > actually support it. Anything that can only break the guest, but not the
> > host, should be allowed. The KVM userspace can certainly ask the kernel
> > what it recommends first (i.e. read the invariant registers first, before
> > deciding what to write), but the decision of what to write should be left
> > up to the user.
> 
> I can see the logic of that approach. But right now QEMU
> userspace knows basically nothing about the system registers
> when we're using KVM: all that knowledge is in the kernel.
> So we don't have a place really to put policy info (and
> not really anywhere to put policy info that depends on the
> host CPU type when mostly we let the kernel care about that).

It would seem wrong to put this logic in the kernel because the
priviliged code should be as small as possible.

Dave

> >> For instance, it's probably OK to ignore a MIDR_EL1 difference
> >> that just indicates a minor revision bump; but not to ignore
> >> one that indicates you just tried to migrate a Cortex-A53
> >> over to a Cavium CPU.
> >
> > If the user does that, then the guest will break - oh well. That's not the
> > host kernel's problem.
> 
> Patching QEMU to ignore all attempts to write wrong values
> to read-only registers would be easy. It just wouldn't
> be very helpful to the user...
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC] [PATCH 0/3] qemu: arm: Migration between machines with different MIDR values

[Peter Maydell]

On 5 October 2018 at 09:48, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> On 4 October 2018 at 16:05, Andrew Jones <drjones@redhat.com> wrote:
>> > On Tue, Oct 02, 2018 at 02:07:38PM +0100, Peter Maydell wrote:
>> >> Hi; thanks for this patch. The issue I see with this patch
>> >> is that the KVM/ARM QEMU approach to system registers so far
>> >> has been "the kernel knows about these and it is in control".
>> >> So we ask the kernel for the list of registers, and just save
>> >> and restore those. That would suggest that if there are sysregs
>> >> where it's OK in fact to ignore a difference between two constant
>> >> register values, it should be the kernel doing the "actually, this
>> >> mismatch is OK" behaviour...
>> >
>> > I don't think the kernel should have to maintain all that logic. If a user
>> > wants to load guest registers, then the kernel should do it, as long as
>> > it's safe from the host's integrity/security PoV, and the hardware would
>> > actually support it. Anything that can only break the guest, but not the
>> > host, should be allowed. The KVM userspace can certainly ask the kernel
>> > what it recommends first (i.e. read the invariant registers first, before
>> > deciding what to write), but the decision of what to write should be left
>> > up to the user.
>>
>> I can see the logic of that approach. But right now QEMU
>> userspace knows basically nothing about the system registers
>> when we're using KVM: all that knowledge is in the kernel.
>> So we don't have a place really to put policy info (and
>> not really anywhere to put policy info that depends on the
>> host CPU type when mostly we let the kernel care about that).
>
> It would seem wrong to put this logic in the kernel because the
> priviliged code should be as small as possible.

Agreed. But if we want to move to "QEMU knows about all the
possible host CPUs" that's a fair-sized design change and
needs more than the minimalist approach this patch has.

thanks
-- PMM