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: <00b07459-5512-b00b-636b-f35845ec369f@intel.com>
Date:   Wed, 20 Jul 2022 13:02:11 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH] [v2] x86/sgx: Allow enclaves to use Asynchrounous Exit
 Notification
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Dave Hansen <dave.hansen@linux.intel.com>
Cc:     dave@sr71.net, Jarkko Sakkinen <jarkko@kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "H. Peter Anvin" <hpa@zytor.com>, Kai Huang <kai.huang@intel.com>,
        Haitao Huang <haitao.huang@linux.intel.com>, x86@kernel.org,
        linux-sgx@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220720191347.1343986-1-dave.hansen@linux.intel.com>
 <YthcXSTfjoM+jjvN@google.com>
From:   Dave Hansen <dave.hansen@intel.com>
In-Reply-To: <YthcXSTfjoM+jjvN@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/20/22 12:49, Sean Christopherson wrote:
> On Wed, Jul 20, 2022, Dave Hansen wrote:
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> index 0c1ba6aa0765..96a73b5b4369 100644
>> --- a/arch/x86/kvm/cpuid.c
>> +++ b/arch/x86/kvm/cpuid.c
>> @@ -1022,9 +1022,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>>  		 * userspace.  ATTRIBUTES.XFRM is not adjusted as userspace is
>>  		 * expected to derive it from supported XCR0.
>>  		 */
>> -		entry->eax &= SGX_ATTR_DEBUG | SGX_ATTR_MODE64BIT |
>> -			      SGX_ATTR_PROVISIONKEY | SGX_ATTR_EINITTOKENKEY |
>> -			      SGX_ATTR_KSS;
>> +		entry->eax &= SGX_ATTR_PRIV_MASK | SGX_ATTR_UNPRIV_MASK;
> 
> It may seem like a maintenance burdern, and it is to some extent, but I think it's
> better for KVM to have to explicitly "enable" each flag.  There is no guarantee
> that a new feature will not require additional KVM enabling, i.e. we want the pain
> of having to manually update KVM so that we get "feature X isn't virtualized"
> complaints and not "I upgraded my kernel and my enclaves broke" bug reports.
> 
> I don't think it's likely that attribute-based features will require additional
> enabling since there aren't any virtualization controls for the ENCLU side of
> things (ENCLU is effectively disabled by blocking ENCLS[ECREATE]), but updating
> KVM isn't particularly difficult so I'd rather be paranoid.

How about something where KVM gets to keep a discrete mask, but where
it's at least defined next to the attributes, something like:

/*
 * These attributes will be advertised to KVM guests as being
 * available.  This includes privileged attributes.  Only add
 * to this list when host-side KVM does not require additional
 * enabling for the attribute.
 */
#define SGX_ATTR_KVM_MASK       (SGX_ATTR_DEBUG         | \
                                 SGX_ATTR_MODE64BIT     | \
                                 SGX_ATTR_PROVISIONKEY  | \
                                 SGX_ATTR_EINITTOKENKEY | \
                                 SGX_ATTR_KSS           | \
                                 SGX_ATTR_ASYNC_EXIT_NOTIFY)

That at least has a *chance* of someone seeing it who goes to add a new
attribute.