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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

>
> 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 :( )

>
>
>>
>>> +
>>> +            /* 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