[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v9 2/4] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
Hi Ayan, On 01/03/2022 12:40, Ayan Kumar Halder wrote: +void post_increment_register(const struct instr_details *instr) +{ + struct cpu_user_regs *regs = guest_cpu_user_regs(); + register_t val = 0; + + /* Currently, we handle only ldr/str post indexing instructions */ + if ( instr->state != INSTR_LDR_STR_POSTINDEXING ) + return; + + /* + * 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) ) You are using 3 times regs->cpsr & PSR_MODE_MASK. Can you introduce a temporary variable? Alternatively, a switch could be used here. [...] diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h index 3354d9c635..ef2c57a2d5 100644 --- a/xen/arch/arm/include/asm/mmio.h +++ b/xen/arch/arm/include/asm/mmio.h @@ -26,12 +26,24 @@#define MAX_IO_HANDLER 16 +enum instr_decode_state+{ + INSTR_ERROR, /* Error encountered while decoding instr */ + INSTR_VALID, /* ISS is valid, so no need to decode */ + /* + * Instruction is decoded successfully. It is a ldr/str post indexing + * instruction. + */ + INSTR_LDR_STR_POSTINDEXING NIT: Please add ',' even for the last item. This would reduce the diff if we add new one. +}; + typedef struct { struct hsr_dabt dabt; struct instr_details { unsigned long rn:5; signed int imm9:9; + enum instr_decode_state state; } dabt_instr; paddr_t gpa; } mmio_info_t; @@ -69,14 +81,15 @@ struct vmmio { };enum io_state try_handle_mmio(struct cpu_user_regs *regs,- const union hsr hsr, - paddr_t gpa); + mmio_info_t *info); void register_mmio_handler(struct domain *d, const struct mmio_handler_ops *ops, paddr_t addr, paddr_t size, void *priv); int domain_io_init(struct domain *d, int max_count); void domain_io_free(struct domain *d);+void try_decode_instruction(const struct cpu_user_regs *regs,+ mmio_info_t *info);#endif /* __ASM_ARM_MMIO_H__ */ diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.hindex 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 fad103bdbd..bea69ffb08 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -102,57 +102,79 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, return handler; }+void try_decode_instruction(const struct cpu_user_regs *regs,+ mmio_info_t *info) +{ + int rc; + + if ( info->dabt.valid ) + { + info->dabt_instr.state = INSTR_VALID; + + /* + * 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 ) This change is not explained in the commit message. TBH, I think it should be separate but I am not going to request that at v9. + { + rc = decode_instruction(regs, info); + if ( rc ) + { + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); + info->dabt_instr.state = INSTR_ERROR; + } + return; + } + + /* + * 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"); + info->dabt_instr.state = INSTR_ERROR; + } +} + enum io_state try_handle_mmio(struct cpu_user_regs *regs, - const union hsr hsr, - paddr_t gpa) + mmio_info_t *info) { struct vcpu *v = current; const struct mmio_handler *handler = NULL; - const struct hsr_dabt dabt = hsr.dabt; - mmio_info_t info = { - .gpa = gpa, - .dabt = dabt - }; + int rc;- ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);+ ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);- handler = find_mmio_handler(v->domain, info.gpa);- if ( !handler ) + if ( !((info->dabt_instr.state == INSTR_VALID) || (info->dabt_instr.state == INSTR_LDR_STR_POSTINDEXING)) ) This check will become quite large if we decode more class. I would instead set the dabt.valid bit whenever we successfully decoded the instruction and check that if dabt.valid here. { - int rc; + ASSERT_UNREACHABLE(); + return IO_ABORT; + } [...] @@ -1982,21 +2030,18 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, case IO_UNHANDLED: /* IO unhandled, try another way to handle it. */ break; - } }/*- * First check if the translation fault can be resolved by the - * P2M subsystem. If that's the case nothing else to do. + * If the instruction syndrome was invalid, then we already checked if + * this was due to a P2M fault. So no point to check again as the result + * will be the same. */ - if ( p2m_resolve_translation_fault(current->domain, - gaddr_to_gfn(gpa)) ) - return; - - if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) ) + if ( info.dabt.valid && check_p2m(is_data, gpa) ) This check would need to be adjusted to check the instruction state is INSTR_VALID. Cheers, -- Julien Grall
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |