On Thu, Aug 20, 2020 at 01:39:22PM +1000, Jordan Niethe wrote:
> There are two main places where instructions are loaded from the guest:
> * Emulate loadstore - such as when performing MMIO emulation
> triggered by an HDSI
> * After an HV emulation assistance interrupt (e40)
>
> If it is a prefixed instruction that triggers these cases, its suffix
> must be loaded. Use the SRR1_PREFIX bit to decide if a suffix needs to
> be loaded. Make sure if this bit is set inject_interrupt() also sets it
> when giving an interrupt to the guest.
>
> ISA v3.10 extends the Hypervisor Emulation Instruction Register (HEIR)
> to 64 bits long to accommodate prefixed instructions. For interrupts
> caused by a word instruction the instruction is loaded into bits 32:63
> and bits 0:31 are zeroed. When caused by a prefixed instruction the
> prefix and suffix are loaded into bits 0:63.
>
> Signed-off-by: Jordan Niethe <jniethe5@xxxxxxxxx>
> ---
> arch/powerpc/kvm/book3s.c | 15 +++++++++++++--
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 10 +++++++---
> arch/powerpc/kvm/book3s_hv_builtin.c | 3 +++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 14 ++++++++++++++
> 4 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index 70d8967acc9b..18b1928a571b 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -456,13 +456,24 @@ int kvmppc_load_last_inst(struct kvm_vcpu *vcpu,
> {
> ulong pc = kvmppc_get_pc(vcpu);
> u32 word;
> + u64 doubleword;
> int r;
>
> if (type == INST_SC)
> pc -= 4;
>
> - r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> - *inst = ppc_inst(word);
> + if ((kvmppc_get_msr(vcpu) & SRR1_PREFIXED)) {
> + r = kvmppc_ld(vcpu, &pc, sizeof(u64), &doubleword, false);
Should we also have a check here that the doubleword is not crossing a
page boundary? I can't think of a way to get this code to cross a
page boundary, assuming the hardware is working correctly, but it
makes me just a little nervous.
> +#ifdef CONFIG_CPU_LITTLE_ENDIAN
> + *inst = ppc_inst_prefix(doubleword & 0xffffffff, doubleword >> 32);
> +#else
> + *inst = ppc_inst_prefix(doubleword >> 32, doubleword & 0xffffffff);
> +#endif
Ick. Is there a cleaner way to do this?
> + } else {
> + r = kvmppc_ld(vcpu, &pc, sizeof(u32), &word, false);
> + *inst = ppc_inst(word);
> + }
> +
> if (r == EMULATE_DONE)
> return r;
> else
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 775ce41738ce..0802471f4856 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -411,9 +411,13 @@ static int instruction_is_store(struct ppc_inst instr)
> unsigned int mask;
>
> mask = 0x10000000;
> - if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> - mask = 0x100; /* major opcode 31 */
> - return (ppc_inst_val(instr) & mask) != 0;
> + if (ppc_inst_prefixed(instr)) {
> + return (ppc_inst_suffix(instr) & mask) != 0;
> + } else {
> + if ((ppc_inst_val(instr) & 0xfc000000) == 0x7c000000)
> + mask = 0x100; /* major opcode 31 */
> + return (ppc_inst_val(instr) & mask) != 0;
> + }
The way the code worked before, the mask depended on whether the
instruction was a D-form (or DS-form or other variant) instruction,
where you can tell loads and stores apart by looking at the major
opcode, or an X-form instruction, where you look at the minor opcode.
Now we are only looking at the minor opcode if it is not a prefixed
instruction. Are there no X-form prefixed loads or stores?
Paul.
