linux-kernel - Re: [PATCH v2 00/14] KVM: x86: Remove emulation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191106222707.GB21617@linux.intel.com>
Date:   Wed, 6 Nov 2019 14:27:07 -0800
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Alexander Graf <graf@...zon.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Liran Alon <liran.alon@...cle.com>
Subject: Re: [PATCH v2 00/14] KVM: x86: Remove emulation_result enums

On Wed, Nov 06, 2019 at 01:17:40PM +0100, Paolo Bonzini wrote:
> On 06/11/19 01:58, Sean Christopherson wrote:
> >> enum kvm_return {
> >>     KVM_RET_USER_EXIT = 0,
> >>     KVM_RET_GUEST = 1,
> >> };
> >>
> >> and then consistently use them as return values? That way anyone who has not
> >> worked on kvm before can still make sense of the code.
> > Hmm, I think it'd make more sense to use #define instead of enum to
> > hopefully make it clear that they aren't the *only* values that can be
> > returned.  That'd also prevent anyone from changing the return types from
> > 'int' to 'enum kvm_return', which IMO would hurt readability overall.
> > 
> > And maybe KVM_EXIT_TO_USERSPACE and KVM_RETURN_TO_GUEST?
> 
> That would be quite some work.  Right now there is some consistency
> between all of:
> 
> - x86_emulate_instruction and its callers
> 
> - vcpu->arch.complete_userspace_io
> 
> - vcpu_enter_guest/vcpu_block
> 
> - kvm_x86_ops->handle_exit
> 
> so it would be very easy to end up with a half-int-half-enum state that
> is more confusing than before...
 
Ya, my thought was to update the obvious cases, essentially the ones you
listed above, to use the define.  So almost intentionally end up in a
half-n-half state, at least in the short term, the thought being that the
extra annotation would do more harm than good.  But there's really no way
to determine whether or not it would actually be a net positive without
writing the code...

> I'm more worried about cases where we have functions returning either 0
> or -errno, but 0 lets you enter the guest.  I'm not sure if the only one
> is kvm_mmu_reload or there are others.

There are lots of those, e.g. basically all of the helpers for nested
consistency checks.