linux-kernel - Re: [PATCH 01/22] KVM: x86: Drop unnecessary and confusing KVM_X86_OP

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: <YfgS7VIp8fpGJvyD@google.com>
Date:   Mon, 31 Jan 2022 16:48:45 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     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, Like Xu <like.xu.linux@...il.com>
Subject: Re: [PATCH 01/22] KVM: x86: Drop unnecessary and confusing
 KVM_X86_OP_NULL macro

On Mon, Jan 31, 2022, Paolo Bonzini wrote:
> On 1/28/22 16:42, Sean Christopherson wrote:
> All in all I totally agree with patches 2-11 and will apply them (patch 2 to
> 5.17 even, as a prerequisite to fix the AVIC race).  Several of patches
> 13-21 are also mostly useful as it clarifies the code, and the others I
> guess are okay in the context of a coherent series though probably they
> would have been rejected as one-offs.

Yeah, the SEV changes in particular are a bit forced.  The only one I care deeply
about is mem_enc_op() => mem_enc_ioctl().  If the macro shenanigans are rejected,
I'd say drop patches 20 and 21, drop most of 19, and maybe give 18 (svm=>avic) the
boot as well.  I'd prefer to keep patch 17 (TLB tweak) to clarify the scope of
SVM's TLB flush.  Many of the changelogs would need to be tweaked as well, i.e. a
v2 is in order.

> However, patches 12 and 22 are unnecessary uses of the C preprocessor in my
> opinion.  

And 14 :-)

I don't have a super strong opinion.  I mostly worked on this because the idea
had been discussed multiple times in the past.  And because I wanted an excuse to
rename vmx_free_vcpu => vmx_vcpu_free, which for some reason I can never find :-)

I was/am concerned that the macro approach will make it more difficult to find a
vendor's implementation, though forcing a conforming name will mitigate that to
some degree.

The pros, in order of importance (IMO)

  1. Mostly forces vendor implementation name to match hook name
  2. Forces new hooks to get an entry in kvm-x86-ops.h
  3. Provides a bit of documentation for specialized hooks (APICv, etc...)
  4. Forces vendors to explicitly define something for non-conforming hooks