[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler
On 27/01/2022 15:48, Ayan Kumar Halder wrote: Hi Julien, Hi, On 27/01/2022 13:57, Julien Grall wrote:On 27/01/2022 13:20, Ayan Kumar Halder wrote:Hi, Asking here as well (might not have been clear on irc). On 27/01/2022 00:10, Julien Grall wrote:Hi,On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder, <ayan.kumar.halder@xxxxxxxxxx> wrote:Hi Julien, On 26/01/2022 17:22, Julien Grall wrote:Hi, On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder, <ayan.kumar.halder@xxxxxxxxxx> wrote: Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding for an exception from a Data Abort" :- ISV - ISS[23:14] holds a valid instruction syndrome When the ISV is 0, the instruction could not be decoded by the hardware (ie ISS is invalid). One should immediately return an error to the caller with an appropriate error message. That's going to be very spammy because we have use-case where it could trap without a valid ISV (e.g. when break-before-make happens). So please don't had a message.There is already a logging statement in traps.c :- inject_abt: gdprintk(XENLOG_DEBUG, "HSR=%#"PRIregister" pc=%#"PRIregister" gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n", hsr.bits, regs->pc, gva, gpa); The reason for the error is to give the user some hint that an IO abort is triggered by Xen.The main difference here is inject_dabt should only be reached when we exhausted all the possibility in I/O handling.In your case, if we can't handle as an MMIO then we should fallthrough the other method (see do_trap_stage2_abort_guest()).In fact, moving the check early and doing the decoding before find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized that earlier.Why should we care about MMIO when ISS is invalid ?Because a translation fault doesn't mean this is emulated MMIO. This may be actual RAM but with the stage-2 entry marked as invalid for tracking purpose (or else).Should we not check first if the ISS is valid or not as it will trigger IO_abort regardless of the MMIO.No. Imagine the guest decides to use a store exclusive on a RAM region that was temporally marked as invalid in the stage-2 page-table.This will generate a data abort in Xen with ISV=0. We want to try to resolve the fault first and retry the instruction.I am assuming that once Xen resolves the MMIO (p2m_resolve_translation_fault()), the guest will again try to run the same instruction on MMIO region. This will be trapped in Xen which will return IO abort as the post-indexing instruction could not be decoded.The access will not trap again in Xen if the fault was resolved.I think your words makes sense.However, I am still concerned that we might not be doing the correct thing in try_fwd_ioserv().See this :- ioreq_t p = { .type = IOREQ_TYPE_COPY, .addr = info->gpa, .size = 1 << info->dabt.size, .count = 1, .dir = !info->dabt.write, /* * On x86, df is used by 'rep' instruction to tell the direction * to iterate (forward or backward). * On Arm, all the accesses to MMIO region will do a single * memory access. So for now, we can safely always set to 0. */ .df = 0, .data = get_user_reg(regs, info->dabt.reg), .state = STATE_IOREQ_READY, };If info->dabt.valid = 0, then '.size', '.dir' and '.data' fields are invalid. Sort of. ISV=0 means that bits [23:14] are RES0. So this doesn't cover the field WnR. The validity of WnR will depend on DFSC. In our case, we will always reach this code with a translation fault. So WnR should always be valid. That said, the rest will not be valid. '.size' gets used in ioreq_server_select() to obtain the end address. It seems incorrect to me.I suppose "if ( !info->dabt.valid )" needs to be checked before "s = ioreq_server_select(v->domain, &p);". The trouble is we would need to return IO_UNHANDLED (so the rest of the code can pick it up) which I think is incorrect here. The approach I can think of is to call ioreq_server_select() with size = 0 (i.e. byte access). Then decode the size (if needed) and then check the access is correct. This is not very nice. But that at the same time, I would like to avoid dealing emulated I/O in Xen or outside differently. Stefano? Cheers, -- Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |