On Wed, Jun 10, 2020 at 07:55:31PM +0200, Vitaly Kuznetsov wrote:
> schedule_work() returns 'false' only when the work is already on the queue
> and this can't happen as kvm_setup_async_pf() always allocates a new one.
> Also, to avoid potential race, it makes sense to to schedule_work() at the
> very end after we've added it to the queue.
>
> While on it, do some minor cleanup. gfn_to_pfn_async() mentioned in a
> comment does not currently exist and, moreover, we can check
> kvm_is_error_hva() at the very beginning, before we try to allocate work so
> 'retry_sync' label can go away completely.
>
> Signed-off-by: Vitaly Kuznetsov <[email protected]>
> ---
> virt/kvm/async_pf.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index f1e07fae84e9..ba080088da76 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -164,7 +164,9 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> if (vcpu->async_pf.queued >= ASYNC_PF_PER_VCPU)
> return 0;
>
> - /* setup delayed work */
> + /* Arch specific code should not do async PF in this case */
> + if (unlikely(kvm_is_error_hva(hva)))
This feels like it should be changed to a WARN_ON_ONCE in a follow-up.
With the WARN, the comment could probably be dropped.
I'd also be in favor of changing the return type to a boolean. I think
you alluded to it earlier, the current semantics are quite confusing as they
invert the normal "return 0 on success".
For this patch:
Reviewed-by: Sean Christopherson <[email protected]>
On 10/06/20 20:14, Sean Christopherson wrote:
>> - /* setup delayed work */
>> + /* Arch specific code should not do async PF in this case */
>> + if (unlikely(kvm_is_error_hva(hva)))
> This feels like it should be changed to a WARN_ON_ONCE in a follow-up.
> With the WARN, the comment could probably be dropped.
I think a race is possible in principle where the memslots are changed
(for example) between s390's page fault handler and the gfn_to_hva call
in kvm_arch_setup_async_pf.
Queued both, thanks!
Paolo
Sean Christopherson <[email protected]> writes:
>
> I'd also be in favor of changing the return type to a boolean. I think
> you alluded to it earlier, the current semantics are quite confusing as they
> invert the normal "return 0 on success".
Yes, will do a follow-up.
KVM/x86 code has an intertwined mix of:
- normal 'int' functions ('0 on success')
- bool functions ('true'/'1' on success)
- 'int' exit handlers ('1'/'0' on success depending if exit to userspace
was required)
- ...
I think we can try to standardize this to:
- 'int' when error is propagated outside of KVM (userspace, other kernel
subsystem,...)
- 'bool' when the function is internal to KVM and the result is binary
('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()',
...)
- 'enum' for the rest.
And, if there's a good reason for making an exception, require a
comment. (leaving aside everything returning a pointer, of course as
these are self-explanatory -- unless it's 'void *' :-))
>
> For this patch:
>
> Reviewed-by: Sean Christopherson <[email protected]>
>
Thank you!
--
Vitaly
On Thu, Jun 11, 2020 at 10:31:08AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> writes:
>
> >
> > I'd also be in favor of changing the return type to a boolean. I think
> > you alluded to it earlier, the current semantics are quite confusing as they
> > invert the normal "return 0 on success".
>
> Yes, will do a follow-up.
>
> KVM/x86 code has an intertwined mix of:
> - normal 'int' functions ('0 on success')
> - bool functions ('true'/'1' on success)
> - 'int' exit handlers ('1'/'0' on success depending if exit to userspace
> was required)
> - ...
>
> I think we can try to standardize this to:
> - 'int' when error is propagated outside of KVM (userspace, other kernel
> subsystem,...)
> - 'bool' when the function is internal to KVM and the result is binary
> ('is_exit_required()', 'was_pf_injected()', 'will_have_another_beer()',
> ...)
> - 'enum' for the rest.
> And, if there's a good reason for making an exception, require a
> comment. (leaving aside everything returning a pointer, of course as
> these are self-explanatory -- unless it's 'void *' :-))
Agreed for 'bool' case, but 'int' versus 'enum' is less straightforward as
there are a huge number of functions that _may_ propagate an error outside
of KVM, including all of the exit handlers. As Paolo point out[*], it'd
be quite easy to end up with a mixture of enum/#define and 0/1 code, which
would be an even bigger mess than what we have today. There are
undoubtedly cases that could be converted to an enum, but they're probably
few and far between as it requires total encapsulation, e.g. the emulator.
[*] https://lkml.kernel.org/r/[email protected]