Re: [PATCH v24 08/24] x86/sgx: Enumerate and track EPC sections

[Jarkko Sakkinen]

On Wed, 2019-12-18 at 10:18 +0100, Borislav Petkov wrote:
> On Sat, Nov 30, 2019 at 01:13:10AM +0200, Jarkko Sakkinen wrote:
> > +static bool __init sgx_alloc_epc_section(u64 addr, u64 size,
> > +					 unsigned long index,
> > +					 struct sgx_epc_section *section)
> > +{
> > +	unsigned long nr_pages = size >> PAGE_SHIFT;
> 
> I'm assuming here that size which gets communicated through CPUID -
> which is an interesting way to communicate SGX settings in itself :-) - is
> in multiples of 4K? SDM doesn't say...

Yes.

> And last time I asked:
> 
> "This size comes from CPUID but it might be prudent to sanity-check it
> nevertheless, before doing the memremap()."
> 
> but it was left uncommented.

I'm sorry about that. Not intended. I just forgot to deal with it or
missed it.

> > +/**
> > + * A section metric is concatenated in a way that @low bits 12-31 define the
> > + * bits 12-31 of the metric and @high bits 0-19 define the bits 32-51 of the
> > + * metric.
> > + */
> > +static inline u64 __init sgx_calc_section_metric(u64 low, u64 high)
> > +{
> > +	return (low & GENMASK_ULL(31, 12)) +
> > +	       ((high & GENMASK_ULL(19, 0)) << 32);
> > +}
> > +
> > +static bool __init sgx_page_cache_init(void)
> > +{
> > +	u32 eax, ebx, ecx, edx, type;
> > +	u64 pa, size;
> > +	int i;
> > +
> > +	BUILD_BUG_ON(SGX_MAX_EPC_SECTIONS > (SGX_EPC_SECTION_MASK + 1));
> > +
> > +	for (i = 0; i < (SGX_MAX_EPC_SECTIONS + 1); i++) {
> 
> Those brackets are still here from the last time. You said:
> 
> "For nothing :-)
> 
> I'll change it as:
> 
>   for (i = 0; i <= SGX_MAX_EPC_SECTIONS; i++) {"
> 
> but probably forgot...
> 
> and looking at my review comments here:
> 
> https://lkml.kernel.org/r/20191005092627.GA25699@xxxxxxx
> 
> and your reply:
> 
> https://lkml.kernel.org/r/20191007115850.GA20830@xxxxxxxxxxxxxxx
> 
> you clearly missed addressing some so I'm going to stop reviewing here.
> 
> Please have a look at those review comments again and check whether the
> apply - and then do them - or they don't and they pls explain why they
> don't.
> 
> And do that for the rest of the patchset, please, before you send it
> again.
> 
> Thx.

It is unintentional but I seriously do my best on keeping track of
things.  Sometimes when you multitask with maintaining other subsystems
and refactor huge patch set like this, it just happens, no matter how
well you try to organize your work.

I'll go through v23 comments with time before sending v25.

/Jarkko