Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f708d769-5d93-351f-ea24-8fa7deb9f689@redhat.com>
Date:   Wed, 28 Sep 2022 13:41:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.1
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org
References: <20220927152241.194900-1-pbonzini@redhat.com>
 <YzM55hqavzENQq7I@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] KVM: allow compiling out SMM support
In-Reply-To: <YzM55hqavzENQq7I@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/27/22 19:59, Sean Christopherson wrote:
>> 	The patch isn't pretty.  I could skip all the changes to add WARNs
>> 	to called functions, but the point of adding the config symbol is
>> 	to make sure that those functions, and all the baggage they bring,
>> 	are dead.
> 
> I would much rather we go even further and completely kill off those functions
> at compile time.

Ok, but then we should go all the way and move as much as possible to a 
separate file.  This also means moving the out-of-SMM flow away from the 
emulator, which in turn enables using ctxt only for the GPRs and not for 
ctxt->ops.

I have already done all that and it's quite a bit nicer; I'll send it 
once I've tested it with more than just smm_test.  I left a couple stubs 
behind where the balance seemed to be better that way (mostly for use in 
kvm_vcpu_ioctl_x86_set_vcpu_events), but most of the code is compiled out.

> There are side effects that should also be eliminated, e.g. x86 should not define
> __KVM_VCPU_MULTIPLE_ADDRESS_SPACE so that usersepace can't create memslots for
> SMM.  Dropping the functions entirely wrapping those #defines in #ifdef as well,
> and so makes it all but impossible for KVM to do anything SMM related.
> 
> Eliminating those at compile time requires a bit more #ifdeffery, but it's not
> awful, and IMO it's better than sprinkling WARNs in a bunch of paths.  KVM_REQ_SMI
> in particular might be going too far, but even for that one I vote to kill it.

Sounds good, though of course some of the various cleanups are best done 
in separate patches.

>>  static int kvm_vcpu_ioctl_smi(struct kvm_vcpu *vcpu)
>>  {
>> -	kvm_make_request(KVM_REQ_SMI, vcpu);
>> -
>> +	if (IS_ENABLED(CONFIG_KVM_SMM))
>> +		kvm_make_request(KVM_REQ_SMI, vcpu);
>>  	return 0;
> 
> This should return -EINVAL, not 0.

I'm a bit wary of changing this in case userspace is relying on it not 
failing, because the paths that lead to the failing ioctl are most 
likely controlled by the guest.

Paolo