Re: [Xen-devel] [PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> On AArch64, the encoding 31 could be used to represent {x,w}sp or
> {x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how the
> register field is interpreted by the instruction.
> 
> All the instructions trapped by Xen (either via a sysreg access or data
> abort) interprets the encoding 31 as zr. Therefore we don't have to
> worry about decoding the instruction.

Is decoding the only way we could tell? i.e. there's no indication in the
syndrome reg?

Is there no way a data abort could result from an access which involved SP
rather than XZ? e.g. if the OS was to (stupidly) point SP at an MMIO area
and then try a stp x0, x1, [sp] for example? Ah, perhaps in that case we
would get a trap with x0 or x1 but sp would be in the FAR and not in the
HSR?

Or maybe a less lunatic case, could xenaccess not result in a faulting
stack access? I suppose the answer there is that xenaccess faults are
handled without actually caring which register it was and then the
instruction is replayed in guest context?

Aside: if an stp traps, does the h/v get two exits, one for each register?
I suppose it must.

> The register zr reprents the zero register, i.e it will always

"represents"

> return 0 and write to it is ignored. To handle properly this properties,

"these properties"

> 2 new helpers have been introduced {get,set}_user_reg to read/write a
> value from/to a register. All the call to select_user_reg have been

calls

> replaced by these 2 helpers.
> 
> Furthermore, the code to emulate encoding 31 in select_user_reg has been
> dropped because it was invalid. For Aarch64 context, the encoding is
> used for sp or zr. For AArch32 context, the ISS won't be valid for data
> abort from AArch32 using r15 (i.e pc) as source/destination (See D7-1881
> ARM DDI 0487A.d, note the validity is more restrictive than on ARMv7).
> It's also not possible to use r15 in co-processor instructions.

Does this "Just Work" for arm32 Xen then?

What happens if an aarch32 guest accesses r15 in one of these ways on an
aarch64 hypervisor? Is it verboten in ARM v8 or something else?ÂE1.2.3
describes it as deprecated, but not forbidden, but I suspect that "and the
source address of the ldr is in MMIO space" is not covered there or
something.

> @@ -207,16 +214,42 @@ register_t *select_user_reg(struct cpu_user_regs
> *regs, int reg)
> ÂÂÂÂÂ}
> Â#undef REGOFFS
> Â#else
> -ÂÂÂÂ/* In 64 bit the syndrome register contains the AArch64 register
> -ÂÂÂÂÂ* number even if the trap was from AArch32 mode. Except that
> -ÂÂÂÂÂ* AArch32 R15 (PC) is encoded as 0b11111.
> +ÂÂÂÂ/*
> +ÂÂÂÂÂ* On 64-bit the syndrome register contains the register index as
> +ÂÂÂÂÂ* viewed in AArch64 state even if the trap was from AArch32 mode.
> ÂÂÂÂÂÂ*/
> -ÂÂÂÂif ( reg == 0x1f /* && is aarch32 guest */)
> -ÂÂÂÂÂÂÂÂreturn &regs->pc;
> ÂÂÂÂÂreturn &regs->x0 + reg;

If reg were == 0x1f here then it would end up accessing regs->sp, which is
only valid for traps from Xen to Xen (i.e. interrupt stack frames), whichif
it were to somehow happen would lead to some rather interesting behaviour I
fear. I'm pretty sure it can't happen, but I'd be happier with an explicit
assert/BUG_ON.

Alternatively we could return NULL here to represent XZR and handle that
appropriately in the {get,set}_user_reg, replacing the check for reg == 31 in 
both places.

(FWIW the actual guest stack pointer state is in either SP_EL{0,1} or the
aarch sp_* in x13, x17 etc.)

> Â#endif
> Â}
> Â
> +register_t get_user_reg(struct cpu_user_regs *regs, int reg)
> +{
> +#ifdef CONFIG_ARM_64
> +ÂÂÂÂ/*
> +ÂÂÂÂÂ* For store/load and sysreg instruction, the encoding 31 always
> +ÂÂÂÂÂ* correspond to {w,x}zr which is the zero register.
> +ÂÂÂÂÂ*/
> +ÂÂÂÂif ( reg == 31 )
> +ÂÂÂÂÂÂÂÂreturn 0;
> +#endif
> +
> +ÂÂÂÂreturn *select_user_reg(regs, reg);
> +}
> +
> +void set_user_reg(struct cpu_user_regs *regs, int reg, register_t value)
> +{
> +#ifdef CONFIG_ARM_64
> +ÂÂÂÂ/*
> +ÂÂÂÂÂ* For store/load and sysreg instruction, the encoding 31 always
> +ÂÂÂÂÂ* correspond to {w,x}zr which is the zero register.
> +ÂÂÂÂÂ*/
> +ÂÂÂÂif ( reg == 31 )
> +ÂÂÂÂÂÂÂÂreturn;
> +#endif
> +
> +ÂÂÂÂ*select_user_reg(regs, reg) = value;
> +}
> +
> [...]

All of the other mostly mechanical changes look OK from a quick glance.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel