Re: [PATCH 08/15] xen: x86: add SGX cpuid handling support.

["Huang, Kai" <kai.huang@linux.intel.com>]

On 7/14/2017 7:37 PM, Andrew Cooper wrote:
> On 13/07/17 07:42, Huang, Kai wrote:
>> On 7/12/2017 10:56 PM, Andrew Cooper wrote:
>>> On 09/07/17 10:10, Kai Huang wrote:
>>>
>>> Why do we need this hide_epc parameter?  If we aren't providing any 
>>> epc resource to the guest, the entire sgx union should be zero and 
>>> the SGX feature bit should be hidden.
>>
>> My intention was to hide physical EPC info for pv_max_policy and 
>> hvm_max_policy (recalculate_sgx is also called by 
>> calculate_pv_max_policy and calculate_hvm_max_policy), as they are for 
>> guest and don't need physical EPC info. But keeping physical EPC info 
>> in them does no harm so I think we can simply remove hide_epc.
> 
> It is my experience that providing half the information is worse than 
> providing none or all of it, because developers are notorious for taking 
> shortcuts when looking for features.
> 
> Patch 1 means that a PV guest will never have p->feat.sgx set. 
> Therefore, we will hit the memset() below, and zero the whole of the SGX 
> union.

Yes I'll remove hide_epc. It is not absolutely needed.

> 
>>
>> IMO we cannot check whether EPC is valid and zero sgx union in 
>> recalculate_sgx, as it is called for each CPUID. For example, it is 
>> called for SGX subleaf 0, and 1, and then 2, and when subleaf 0 and 1 
>> are called, the EPC resource is 0 (hasn't been configured).
> 
> recalculate_*() only get called when the toolstack makes updates to the 
> policy.  It is an unfortunate side effect of the current implementation, 
> but will be going away with my CPUID work.
> 
> The intended flow will be this:
> 
> At Xen boot:
> * Calculates the raw, host and max policies (as we do today)
> 
> At domain create:
> * Appropriate policy gets copied to make the default domain policy.
> * Toolstack gets the whole policy at one with a new 
> DOMCTL_get_cpuid_policy hypercall.
> * Toolstack makes all adjustments (locally) that it wants to, based on 
> configuration, etc.
> * Toolstack makes a single DOMCTL_set_cpuid_policy hypercall.
> * Xen audits the new policy proposed by the toolstack, resulting in a 
> single yes/no decision.
> ** If not, the toolstack is told to try again.  This will likely result 
> in xl asking the user to modify their .cfg file.
> ** If yes, the proposed policy becomes the actual policy.
> 
> This scheme will fix the current problem we have where the toolstack 
> blindly proposes changes (one leaf at a time), and Xen has to zero the 
> bits it doesn't like (because the toolstack has never traditionally 
> checked the return value of the hypercall :( )

This is actually what I was looking for when implementing CPUID support 
for SGX. I think I'll wait for your work to be merged to Xen and then do 
my work above your work. :)

Thanks,
-Kai

> 
>>
>>
>>>
>>>> +
>>>> +            /* Subleaf 2. */
>>>> +            uint32_t base_valid:1, :11, base_pfn_low:20;
>>>> +            uint32_t base_pfn_high:20, :12;
>>>> +            uint32_t size_valid:1, :11, npages_low:20;
>>>> +            uint32_t npages_high:20, :12;
>>>> +        };
>>>
>>> Are the {base,size}_valid fields correct?  The manual says the are 
>>> 4-bit fields rather than single bit fields.
>>
>> They are 4 bits in SDM but actually currently only bit 1 is valid 
>> (other values are reserved). I think for now bool base_valid should be 
>> enough. We can extend when new values come out. What's your suggestion?
> 
> Ok.  That can work for now.
> 
>>
>>>
>>> I would also drop the _pfn from the base names.  The fields still 
>>> need shifting to get a sensible value.
>>
>> OK. Will do.
> 
> As a further thought, what about uint64_t base:40 and size:40?  That 
> would reduce the complexity of calculating the values.
> 
> ~Andrew
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel