|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v6 2/3] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO handler
On Wed, 2 Feb 2022, Stefano Stabellini wrote:
> On Wed, 2 Feb 2022, Ayan Kumar Halder wrote:
> > For instructions on MMIO regions emulated by Xen, Xen reads the
> > remaining bits of the HSR. It determines if the instruction is to be
> > ignored, retried or decoded. If it gets an error while decoding the
> > instruction, then it sends an abort to the guest.
> >
> > If the instruction is valid or successfully decoded, Xen tries to
> > execute the instruction for the emulated MMIO region. If the instruction
> > was successfully executed, then Xen determines if the instruction needs
> > further processing. For eg:- In case of ldr/str post indexing on arm64,
> > the rn register needs to be updated.
> >
> > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
> > ---
> >
> > Changelog :-
> >
> > v2..v5 - Mentioned in the cover letter.
> >
> > v6 - 1. Mantained the decoding state of the instruction. This is used by the
> > caller to either abort the guest or retry or ignore or perform read/write on
> > the mmio region.
> >
> > 2. try_decode() invokes decoding for both aarch64 and thumb state.
> > (Previously
> > it used to invoke decoding only for aarch64 state). Thus, it handles all the
> > checking of the registers before invoking any decoding of instruction.
> > try_decode_instruction_invalid_iss() has thus been removed.
> >
> > xen/arch/arm/arm32/traps.c | 6 ++
> > xen/arch/arm/arm64/traps.c | 41 ++++++++++++
> > xen/arch/arm/decode.h | 12 +++-
> > xen/arch/arm/include/asm/traps.h | 2 +
> > xen/arch/arm/io.c | 108 +++++++++++++++++++++++++------
> > 5 files changed, 148 insertions(+), 21 deletions(-)
> >
> > diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> > index 9c9790a6d1..6ad9a31499 100644
> > --- a/xen/arch/arm/arm32/traps.c
> > +++ b/xen/arch/arm/arm32/traps.c
> > @@ -21,6 +21,7 @@
> >
> > #include <public/xen.h>
> >
> > +#include <asm/mmio.h>
> > #include <asm/processor.h>
> > #include <asm/traps.h>
> >
> > @@ -82,6 +83,11 @@ void do_trap_data_abort(struct cpu_user_regs *regs)
> > do_unexpected_trap("Data Abort", regs);
> > }
> >
> > +void post_increment_register(const struct instr_details *instr)
> > +{
> > + ASSERT_UNREACHABLE();
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
> > index 9113a15c7a..4de2206801 100644
> > --- a/xen/arch/arm/arm64/traps.c
> > +++ b/xen/arch/arm/arm64/traps.c
> > @@ -18,9 +18,12 @@
> >
> > #include <xen/lib.h>
> >
> > +#include <asm/current.h>
> > #include <asm/hsr.h>
> > +#include <asm/mmio.h>
> > #include <asm/system.h>
> > #include <asm/processor.h>
> > +#include <asm/regs.h>
> >
> > #include <public/xen.h>
> >
> > @@ -44,6 +47,44 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason)
> > panic("bad mode\n");
> > }
> >
> > +void post_increment_register(const struct instr_details *instr)
> > +{
> > + struct cpu_user_regs *regs = guest_cpu_user_regs();
> > + register_t val;
val needs to be initialized:
traps.c: In function ‘post_increment_register’:
traps.c:79:9: error: ‘val’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
79 | val += instr->imm9;
| ~~~~^~~~~~~~~~~~~~
> > + /*
> > + * Handle when rn = SP
> > + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register
> > selection"
> > + * t = SP_EL0
> > + * h = SP_ELx
> > + * and M[3:0] (Page - C5-474 "When exception taken from AArch64
> > state:")
> > + */
> > + if (instr->rn == 31 )
> > + {
> > + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> > + val = regs->sp_el1;
> > + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) ||
> > + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) )
> > + val = regs->sp_el0;
> > + else
> > + ASSERT_UNREACHABLE();
> > + }
> > + else
> > + val = get_user_reg(regs, instr->rn);
> > +
> > + val += instr->imm9;
> > +
> > + if ( instr->rn == 31 )
> > + {
> > + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h )
> > + regs->sp_el1 = val;
> > + else
> > + regs->sp_el0 = val;
> > + }
> > + else
> > + set_user_reg(regs, instr->rn, val);
> > +}
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> > index fe7512a053..5efd72405e 100644
> > --- a/xen/arch/arm/decode.h
> > +++ b/xen/arch/arm/decode.h
> > @@ -52,7 +52,17 @@ union instr {
> > #define POST_INDEX_FIXED_MASK 0x3B200C00
> > #define POST_INDEX_FIXED_VALUE 0x38000400
> >
> > -/* Decode an instruction from pc
> > +enum instr_decode_state
> > +{
> > + INSTR_ERROR, /* Error encountered while decoding the instruction */
> > + INSTR_VALID, /* ISS is valid, so there is no need to decode */
> > + INSTR_SUCCESS, /* Instruction is decoded successfully */
> > + INSTR_IGNORE, /* Instruction is to be ignored (similar to NOP) */
> > + INSTR_RETRY /* Instruction is to be retried */
> > +};
> > +
> > +/*
> > + * Decode an instruction from pc
> > * /!\ This function is intended to decode an instruction. It considers
> > that the
> > * instruction is valid.
> > *
> > diff --git a/xen/arch/arm/include/asm/traps.h
> > b/xen/arch/arm/include/asm/traps.h
> > index 2ed2b85c6f..95c46ad391 100644
> > --- a/xen/arch/arm/include/asm/traps.h
> > +++ b/xen/arch/arm/include/asm/traps.h
> > @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct
> > hsr_dabt dabt, register_t r)
> > return r;
> > }
> >
> > +void post_increment_register(const struct instr_details *instr);
> > +
> > #endif /* __ASM_ARM_TRAPS__ */
> > /*
> > * Local variables:
> > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > index a289d393f9..1011327058 100644
> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -95,6 +95,59 @@ static const struct mmio_handler
> > *find_mmio_handler(struct domain *d,
> > return handler;
> > }
> >
> > +enum instr_decode_state try_decode_instruction(const struct cpu_user_regs
> > *regs,
> > + mmio_info_t *info)
> > +{
> > + int rc;
> > +
> > + /*
> > + * Erratum 766422: Thumb store translation fault to Hypervisor may
> > + * not have correct HSR Rt value.
> > + */
> > + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> > + info->dabt.write )
> > + {
> > + rc = decode_instruction(regs, info);
> > + if ( rc )
> > + {
> > + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> > + return INSTR_ERROR;
> > + }
>
> It looks like we want a "return" here? But it should work either way
> because it should return with the if ( info->dabt.valid ) check right
> after anyway.
>
>
> > + }
> > +
> > + /* If ISS is valid, then no need to decode the instruction any further
> > */
> > + if (info->dabt.valid)
> > + return INSTR_VALID;
>
> code style
>
>
> > + /*
> > + * Xen should not decode the instruction when it was trapped due to
> > + * translation fault.
> > + */
> > + if ( info->dabt.s1ptw )
> > + return INSTR_RETRY;
> > +
> > + /*
> > + * If the fault occurred due to cache maintenance or address
> > translation
> > + * instructions, then Xen needs to ignore these instructions.
> > + */
> > + if ( info->dabt.cache )
> > + return INSTR_IGNORE;
> > +
> > + /*
> > + * Armv8 processor does not provide a valid syndrome for decoding some
> > + * instructions. So in order to process these instructions, Xen must
> > + * decode them.
> > + */
> > + rc = decode_instruction(regs, info);
> > + if ( rc )
> > + {
> > + gprintk(XENLOG_ERR, "Unable to decode instruction\n");
> > + return INSTR_ERROR;
> > + }
> > + else
> > + return INSTR_SUCCESS;
> > +}
> > +
> > enum io_state try_handle_mmio(struct cpu_user_regs *regs,
> > const union hsr hsr,
> > paddr_t gpa)
> > @@ -106,14 +159,14 @@ enum io_state try_handle_mmio(struct cpu_user_regs
> > *regs,
> > .gpa = gpa,
> > .dabt = dabt
> > };
> > + int rc;
> > + enum instr_decode_state state;
> >
> > ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
> >
> > handler = find_mmio_handler(v->domain, info.gpa);
> > if ( !handler )
> > {
> > - int rc;
> > -
> > rc = try_fwd_ioserv(regs, v, &info);
> > if ( rc == IO_HANDLED )
> > return handle_ioserv(regs, v);
> > @@ -121,31 +174,46 @@ enum io_state try_handle_mmio(struct cpu_user_regs
> > *regs,
> > return rc;
> > }
> >
> > - /* All the instructions used on emulated MMIO region should be valid */
> > - if ( !dabt.valid )
> > + state = try_decode_instruction(regs, &info);
>
> We still have the issue that try_fwd_ioserv (called above) doesn't work
> properly if !dabt.valid. I think we need to call try_decode_instruction
> and do the "state" checks before find_mmio_handler/try_fwd_ioserv.
>
>
> > + /*
> > + * If the instruction was to be ignored by Xen, then it should return
> > to the
> > + * caller which will increment the PC, so that the guest can execute
> > the
> > + * next instruction.
> > + */
> > + if ( state == INSTR_IGNORE )
> > + return IO_HANDLED;
> > + /*
> > + * If Xen could not decode the instruction for any reason, then it
> > should
> > + * ask the caller to abort the guest.
> > + */
> > + else if ( state == INSTR_ERROR )
> > return IO_ABORT;
> > + /* When the instruction needs to be retried by the guest */
> > + else if ( state == INSTR_RETRY )
> > + return IO_UNHANDLED;
> >
> > /*
> > - * Erratum 766422: Thumb store translation fault to Hypervisor may
> > - * not have correct HSR Rt value.
> > + * At this point, we know that the instruction is either valid or has
> > been
> > + * decoded successfully. Thus, Xen should be allowed to execute the
> > + * instruction on the emulated MMIO region.
> > */
> > - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) &&
> > - dabt.write )
> > - {
> > - int rc;
> > + if ( info.dabt.write )
> > + rc = handle_write(handler, v, &info);
> > + else
> > + rc = handle_read(handler, v, &info);
> >
> > - rc = decode_instruction(regs, &info);
> > - if ( rc )
> > - {
> > - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n");
> > - return IO_ABORT;
> > - }
> > + /*
> > + * If the instruction was decoded and has executed successfully on the
> > MMIO
> > + * region, then Xen should execute the next part of the instruction.
> > (for eg
> > + * increment the rn if it is a post-indexing instruction.
> > + */
> > + if ( (rc == IO_HANDLED) && (state == INSTR_SUCCESS) )
> > + {
> > + post_increment_register(&info.dabt_instr);
> > }
>
> We need to call post_increment_register also from arch_ioreq_complete_mmio
> for the IOREQ case.
>
>
> > - if ( info.dabt.write )
> > - return handle_write(handler, v, &info);
> > - else
> > - return handle_read(handler, v, &info);
> > + return rc;
> > }
> >
> > void register_mmio_handler(struct domain *d,
> > --
> > 2.17.1
> >
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |