[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.