On Wed, Sep 2, 2020 at 4:18 PM Paul Mackerras <pau...@ozlabs.org> wrote:
>
> 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 <jniet...@gmail.com>
> > ---
> > 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.
I didn't think it could happen but I will add a check to be safe.
>
> > +#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?
Would it be nicer to read the prefix as u32 then the suffix as a u32 too?
>
> > + } 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?
I could not see an X-form load/stores so I went with just that.
But checking the ISA it does mention "..X-form instructions that are
preceded by an MLS-form or MMLS-form prefix..." so I shall use the
other mask too.
>
> Paul.
Thank you for the comments and suggestions.
