On Sat, May 11, 2013 at 01:36:30AM +0100, Christoffer Dall wrote: > On Tue, May 07, 2013 at 05:33:03PM +0100, Catalin Marinas wrote: > > On Tue, May 07, 2013 at 05:28:00PM +0100, Marc Zyngier wrote: > > > On 02/05/13 17:09, Catalin Marinas wrote: > > > > BTW, on arch/arm it looks like this is used when you get a data abort > > > > with PC as the destination register and you inject a prefetch abort in > > > > this case. Why isn't this a normal data abort? Once you get the > > > > information, you load it into PC but first you need to sort out the data > > > > abort (unless I don't understand how the kvm_inject_pabt works). > > > > > > Indeed, it should be a data abort, as we correctly fetched the > > > instruction. Now, I wonder why we even bother trying to catch this case. > > > Fetching PC from MMIO looks quite silly, but I don't think anything > > > really forbids it in the architecture. > > > > It's not forbidden and you should just treat it as any other data abort, > > no need to check whether the register is PC. If you do the PC adjustment > > further down in that function it will be overridden by the instruction > > emulation anyway. There is no optimisation in checking for PC since such > > fault is very unlikely in sane code anyway. > > > The reason is simply that it is most likely because of a bug that this > happens, and we would rather catch such an error in a meaningful way > than let this go crazy and have users track it down for a long time. It is probably a bug but it is a valid code sequence (and Peter even gave an example). > Especially, this was relevant when we did io instruction decoding and we > wanted to catch potential bugs in that when it was development code. > > That all being said, we can remove the check. I don't think, however, > that it being an unlikely thing is a good argument: if we remove the > check we need to make sure that the VM does whatever the architecture > dictates, which I assume is to not skip the MMIO instruction and support > setting the PC to the value returned from an I/O emulation device in > QEMU. > > I think the scenario sounds crazy and is more than anything a sign of a > bug somewhere, and whether it should be a PABT or a DABT is a judgement > call, but I chose a PABT because the thing that's weird is jumping to an > I/O address, it's not that the memory address for the load is > problematic. I think that's where things get confused. You are reading from an I/O location with the destination being the PC and it is trapped by KVM. This has nothing to do with PABT, it's purely a DABT on the I/O access. You should emulate it and store the result in the PC. If the value loaded in PC is wrong, you will get a subsequent PABT in the guest but you must not translate the DABT on I/O memory (which would be generated even if the destination is not PC) into a PABT which confuses the guest further. By doing this you try to convince the guest that it really branched to the I/O address (since you raise the PABT with the DFAR value) when it simply tried to load an address from I/O and branch to this new address. IOW, these two are equivalent regarding PC: ldr pc, [r0] @ r0 is an I/O address and ldr r1, [r0] @ r0 is an I/O address mov pc, r1 In the second scenario, you don't raise a PABT on the first instruction. You may raise one on the second if PC is invalid but that's different and most likely it will be a guest-only thing. So for DABT emulation, checking whether the destination register is PC is simply a minimal optimisation (if at all) of that case to avoid storing the PC twice. Injecting PABT when you get a DABT is a bug. You already catch PABT on I/O address in kvm_handle_guest_abort() and correctly inject PABT into guest. -- Catalin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
