On Sun, Mar 05 2017 at 3:01:09 pm GMT, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> Hi Marc,
>
> On Wed, Feb 22, 2017 at 11:47:20AM +0000, Marc Zyngier wrote:
>> Running the following code:
>>
>> root@zomby-woof:~# cat test-pmu.c
>> int main(int argc, char *argv[])
>> {
>> unsigned int val;
>> asm ("mrc p15, 0, %0, c9, c13, 0\n" : "=r" (val));
>> return val;
>> }
>>
>> in a 32bit guest (or a 64bit guest with a 32bit userspace) results in
>> this surprising result:
>>
>> [ 120.347497] kvm [1150]: Unsupported guest CP15 access at: ab0945ae
>> [ 120.353689] kvm [1142]: { Op0( 0), Op1( 0), CRn( 9), CRm(13), Op2( 0), func_read },
>>
>> which is weird, because the guest behaves correctly:
>> root@zomby-woof:~# ./test-pmu
>> [ 16.184422] test-pmu[740]: undefined instruction: pc=00000000ab0945ae
>> [ 16.186043] Code: 00340001 b4800000 af00b085 60396078 (3f1dee19)
>> Illegal instruction
>>
>> It gets the expected UNDEF, and all is fine. So what?
>>
>> It turns out that the PMU emulation code is a bit lazy, and tells the
>> rest of KVM that the emulation has failed, so that an exception gets
>> delivered. Subtle differences in the 32bit vs 64bit handling make it
>> spit an "Unsupported..." error.
>>
>> This series tries to set things straight:
>> - Allow an exception to be injected from an emulation handler
>> - Make all PMU illegal accesses inject an UNDEF
>> - Make these illegal accesses a successful emulation w.r.t the rest of KVM.
>>
>> In the process, we also squash an interesting bug in the 64bit CP
>> access. Similar treatment could be applied to the 32bit kernel, except
>> that we don't ever inject an exception there (no PMU support yet).
>
> I'm a bit confused about this series and not too thrilled of the
> approach where we add a side-channel of the sys_reg param in the vcpu
> structure, which may or may not contain valid data at any given point.
>
> Couldn't we use a slightly bigger hammer (with cleaner semantics) and
> let all system register handling (cp on 32-bit and 64-bit sys regs
> alike) simply return true if they were emulated, in which case the
> caller should advance the PC, or false ifsomething else happened, and
> leave it up to the emulation of the individual registers to decide if
> any exceptions should be injected.
So that was my other option - changing the semantics of the return
value, and considering that an emulation never fails. At that stage, we
can repurpose the return value form the accessor to simply indicate
whether or not we should skip the current instruction.
> I don't think we have that many places where we want to inject an
> undefined exception in our handlers, and doing it explicitly might
> actually be a good idea to make it more clear that we're emulating the
> architecture properly. What do you think?
I think that'd work nicely. I'll rework the series along these lines.
Thanks,
M.
--
Jazz is not dead, it just smell funny.
