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.
