Re: [PATCH 3/3] selftest: Support amx selftest

[Yang Zhong <yang.zhong@intel.com>]

On Tue, Dec 21, 2021 at 06:36:17PM +0100, Paolo Bonzini wrote:
> On 12/22/21 00:15, Yang Zhong wrote:
> >This selftest do two test cases, one is to trigger #NM
> >exception and check MSR XFD_ERR value. Another case is
> >guest load tile data into tmm0 registers and trap to host
> >side to check memory data after save/restore.
> >
> >Signed-off-by: Yang Zhong <yang.zhong@intel.com>
> 
> This is a great start, mainly I'd add a lot more GUEST_SYNCs.
> 
> Basically any instruction after the initial GUEST_ASSERTs are a
> potential point for GUEST_SYNC, except right after the call to
> set_tilecfg:
> 
> GUEST_SYNC(1)
> 
> >+	/* xfd=0, enable amx */
> >+	wrmsr(MSR_IA32_XFD, 0);
> 
> GUEST_SYNC(2)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == 0);
> >+	set_tilecfg(amx_cfg);
> >+	__ldtilecfg(amx_cfg);
> 
> GUEST_SYNC(3)
 
   Paolo, with this GUEST_SYNC(3) between __ldtilecfg and __tileloadd
   instructions, the guest code generate (vector:0x6, Invalid Opcode) 
   exception, which is a strange issue. thanks!

   Yang   


> 
> >+	/* Check save/restore when trap to userspace */
> >+	__tileloadd(tiledata);
> >+	GUEST_SYNC(1);
> 
> This would become 4; here add tilerelease+GUEST_SYNC(5)+XSAVEC, and
> check that state 18 is not included in XCOMP_BV.
>
     
   Thanks Paolo, the new code like below:

    GUEST_SYNC(5);
    /* bit 18 not in the XCOMP_BV after xsavec() */
    set_xstatebv(xsave_data, XFEATURE_MASK_XTILEDATA);
     __xsavec(xsave_data, XFEATURE_MASK_XTILEDATA);
    GUEST_ASSERT((get_xstatebv(xsave_data) & XFEATURE_MASK_XTILEDATA) == 0)


    Yang

 
> >+	/* xfd=0x40000, disable amx tiledata */
> >+	wrmsr(MSR_IA32_XFD, XFEATURE_MASK_XTILEDATA);
> 
> GUEST_SYNC(6)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) == XFEATURE_MASK_XTILEDATA);
> >+	set_tilecfg(amx_cfg);
> >+	__ldtilecfg(amx_cfg);
> >+	/* Trigger #NM exception */
> >+	__tileloadd(tiledata);
> 
> GUEST_SYNC(10); this final GUEST_SYNC should also check TMM0 in the host.
> 
> >+	GUEST_DONE();
> >+}
> >+
> >+void guest_nm_handler(struct ex_regs *regs)
> >+{
> >+	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
> 
> GUEST_SYNC(7)
> 
> >+	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> >+	/* Clear xfd_err */
> 
> Same here, I'd do a GUEST_SYNC(8) and re-read MSR_IA32_XFD_ERR.
> 
> >+	wrmsr(MSR_IA32_XFD_ERR, 0);
> >+	GUEST_SYNC(2);
> 
   
    This need add regs->rip +=3 to skip __tileloadd() instruction(generate #NM) 
    when VM resume from GUEST_SYNC(9).

    Thanks!

    Yang


> This becomes GUEST_SYNC(9).
> 
> >+}
> 
> 
> >+		case UCALL_SYNC:
> >+			switch (uc.args[1]) {
> >+			case 1:
> >+				fprintf(stderr,
> >+					"Exit VM by GUEST_SYNC(1), check save/restore.\n");
> >+
> >+				/* Compacted mode, get amx offset by xsave area
> >+				 * size subtract 8K amx size.
> >+				 */
> >+				amx_offset = xsave_restore_size - NUM_TILES*TILE_SIZE;
> >+				state = vcpu_save_state(vm, VCPU_ID);
> >+				void *amx_start = (void *)state->xsave + amx_offset;
> >+				void *tiles_data = (void *)addr_gva2hva(vm, tiledata);
> >+				/* Only check TMM0 register, 1 tile */
> >+				ret = memcmp(amx_start, tiles_data, TILE_SIZE);
> >+				TEST_ASSERT(ret == 0, "memcmp failed, ret=%d\n", ret);
> >+				kvm_x86_state_cleanup(state);
> >+				break;
> 
> All GUEST_SYNCs should do save_state/load_state like state_test.c.
> Then of course you can *also* check TMM0 after __tileloadd, which
> would be cases 4 and 10.
> 

  Yes, the new code will add save_state/load_state as in the state_test.c file.

  Thanks!

  Yang



> Thanks,
> 
> Paolo
> 
> >+			case 2:
> >+				fprintf(stderr,
> >+					"Exit VM by GUEST_SYNC(2), generate #NM exception.\n");
> >+				goto done;
> >+			}
> >+			break;
> >+		case UCALL_DONE:
> >+			goto done;
> >+		default:
> >+			TEST_FAIL("Unknown ucall %lu", uc.cmd);
> >+		}
> >+	}
> >+done:
> >+	kvm_vm_free(vm);
> >+}
> >