Re: [PATCH] KVM: selftests: Rename vm_handle_exception in evmcs test

[Marc Zyngier <maz@kernel.org>]

On 2021-06-05 00:11, Ricardo Koller wrote:
> On Fri, Jun 04, 2021 at 09:26:54PM +0000, Sean Christopherson wrote:
>> On Fri, Jun 04, 2021, Ricardo Koller wrote:
>> > Kernel test robot reports this:
>> >
>> > > /usr/bin/ld: tools/testing/selftests/kvm/x86_64/evmcs_test.c:157: undefined reference to `vm_handle_exception'
>> > > /usr/bin/ld: tools/testing/selftests/kvm/x86_64/evmcs_test.c:158: undefined reference to `vm_handle_exception'
>> > > collect2: error: ld returned 1 exit status
>> >
>> > Fix it by renaming vm_handle_exception to vm_install_vector_handler in
>> > evmcs_test.c.
>> >
>> > Fixes: a2bad6a990a4 ("KVM: selftests: Rename vm_handle_exception")
>> 
>> Belated code review...
> 
> Thanks for the review.
> 
>> Can we rename the helper to vm_install_exception_handler()?
>> 
>> In x86, "vector" is the number of the exception and "vectoring" is the 
>> process
>> of determining the resulting vector that gets delivered to software 
>> (e.g. when
>> dealing with contributory faults like #GP->#PF->#DF), but the thing 
>> that's being
>> handled is an exception.
> 
> Got it. What about this renaming:
> 
>   vm_handle_exception(vec) 		-> vm_install_exception_handler(vec)
>   vm_install_exception_handler(vec, ec)	-> vm_install_sync_handler(vec, 
> ec)
> 
>> 
>> arm appears to have similar terminology.  And looking at the arm code, 
>> it's very
>> confusing to have a helper vm_install_vector_handler() install into
>> exception_handlers, _not_ into vector_handlers.  Calling the 
>> vector_handlers
>> "default" handlers is also confusing, as "default" usually implies the 
>> thing can
>> be overwritten.  But in this case, the "default" handler is just 
>> another layer
>> in the routing.
>> 
>> The multiple layers of routing is also confusing and a bit hard to 
>> wade through
>> for the uninitiated.  The whole thing can be made more straightfoward 
>> by doing
>> away with the intermediate routing, whacking ~50 lines of code in the 
>> process.
>> E.g. (definitely not functional code):
> 
> I think that works and it does remove a bunch of code. Just need to 
> play
> with the idea and check that it can cover all cases.
> 
> For now, given that the build is broken, what about this series of
> patches:
> 
> 1. keep this patch to fix x86 kvm selftests
> 2. rename both arm and x86 to vm_install_exception_handler and
> vm_install_sync_handler
> 3. restructure the internals of exception handling in arm
> 
> Alternatively, I can send 1+2 together and then 3. What do you think?

This is becoming a bit messy. I'd rather drop the whole series from
-next, and get something that doesn't break in the middle. Please
resend the series tested on top of -rc4.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm