Re: [kvm-unit-tests PATCH] powerpc: Add SPRs migration test

[David Gibson <dgibson@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5157 bytes --]

On Mon, 11 Apr 2016 11:23:46 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 11.04.2016 03:55, David Gibson wrote:
> > On Fri,  8 Apr 2016 13:35:29 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> This test can be used to check whether the SPR (special purpose
> >> registers) of the PowerPC CPU are migrated right. It first fills
> >> the various SPRs with some non-zero value, then reads the values
> >> back into a first array, then waits for a key (with the '-w' option)
> >> so that it is possible to migrate the VM, and finally reads the
> >> values from the SPRs back into another array and then compares it
> >> with the initial values.
> >> Currently the test only supports the SPRs from the PowerISA v2.07
> >> specification (i.e. POWER8 CPUs), but other versions should be
> >> pretty easy to add later.
> >>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>  
> > 
> > So, the main concern I have about this is that while writing an
> > arbitrary value is ok for quite a few SPRs, there's a significant
> > number where that's not safe: either because it could actually cause
> > some sort of exception interrupting the test, or because the value will
> > get masked or otherwise modifier on write.  
> 
> If a write to an SPR caused any exception or hang of the guest, I did
> not include it in the list of SPRs to be tested here. Sorry, I should
> have mentioned that somewhere in the commit message or comment of the
> sprs.c file below.

Hmm.. right, but that just means the particular value you're using
doesn't break - other values could cause guest crashes.  But as it
appears now, it looks like the value written is arbitrary.

> Anyway, working out a detailed list of values that should be written
> into each of the SPRs might get quite difficult/cumbersome since some of
> these SPRs are not that well documented in the PowerISA. So I'd like to
> keep the generic approach here instead. Anyway, if a write causes
> trouble, it's not a big issue to take that SPR out of the list again.
> And since the kvm-unit-tests are also a very isolated test (i.e. we're
> not running a Linux kernel here which might expect certain values in
> certain SPRs), it's IMHO also ok to mess up the state in the SPRs since
> the VM will get destroyed after the test anyway.

Hm, yeah, I guess so.  I'm just concerned about crashing the guest
before the test can even finish.

> About your second concern, that the values will get masked or modified:
> Yes, you're right, of course! But that's not a problem here since I read
> back the values from the SPRs before the migration - I do not use the
> original value that I wrote into the SPRs for comparison!

Ah, I missed that, sorry.  Clever approach.

> > [snip]  
> >> +/* Common SPRs for all PowerPC CPUs */
> >> +static void set_sprs_common(uint64_t val)
> >> +{
> >> +	mtspr(1, val);		/* XER */
> >> +	mtspr(9, val);		/* CTR */
> >> +	mtspr(273, val);	/* SPRG1 */
> >> +	mtspr(274, val);	/* SPRG2 */
> >> +	mtspr(275, val);	/* SPRG3 */
> >> +}
> >> +
> >> +/* SPRs from PowerISA 2.07 Book III-S */
> >> +static void set_sprs_book3s_207(uint64_t val)
> >> +{
> >> +	mtspr(3, val);		/* DSCR */
> >> +	mtspr(13, val);		/* AMR */
> >> +	mtspr(17, val);		/* DSCR */
> >> +	mtspr(18, val);		/* DSISR */
> >> +	mtspr(19, val);		/* DAR */
> >> +	mtspr(29, val);		/* AMR */  
> > 
> > AMR seems to be listed twice..  
> 
> That's because AMR is available via both SPR numbers - one time for
> userspace mode (which can be disabled by the kernel if desired), and one
> time for privileged mode.
> 
> > At a glance SPRs above where writing an arbitrary value might not be
> > safe include AMR, IAMR, UAMOR, and MMCR*.  
> 
> Well, you should not think of this test as a nice, behaving guest kernel
> that only puts valid values into safe SPRs ... rather think of it as a
> stress test for the host with a bad, misbehaving guest kernel.
> 
> So think what's the worst thing that could happen? Host crash? Right,
> this test already helped to find one of those bugs:
> 
>  http://www.spinics.net/lists/kvm/msg128989.html
> 
> ... but then we've of course got to fix the host kernel (or QEMU), not
> this test.
> 
> The other bad thing that could of course happen is a guest crash - but
> then, as mentioned above, we can simply adjust the list of SPRs that we
> test as soon as we see such a crash. With the current list, I do not get
> any guest crashes - at least not with kvm-hv. ... but kvm-pr and tcg are
> two other candidates that likely need some fixing for this test.

I'm still a little concerned about crashing the guest before the test
can complete.  But as you say generating sane values for all SPRs is
quite a lot of work, and in the meantime the SPRs which could crash the
guest (or the host) are the ones that are especially important to test
migration of.

So, you've convinced me that this is a good idea on balance.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]