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

["Huang, Kai" <kai.huang@xxxxxxxxxxxxxxx>]

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