On 2019-03-27 13:06, Sean Christopherson wrote:
> On Wed, Mar 27, 2019 at 06:38:57PM +0000, Jethro Beekman wrote:
>> On 2019-03-26 22:28, Jarkko Sakkinen wrote:
>>> On Tue, Mar 26, 2019 at 04:58:52PM -0700, Sean Christopherson wrote:
>>>> On Tue, Mar 26, 2019 at 03:26:50PM +0200, Jarkko Sakkinen wrote:
>>>>> On Thu, Mar 21, 2019 at 05:51:11PM +0200, Jarkko Sakkinen wrote:
>>>>>>> Yuck.  If we remove the driver specific Makefile then we can eliminate
>>>>>>> the "../" prefix here.  E.g. in the main SGX Makefile:
>>>>>>>
>>>>>>> obj-$(CONFIG_INTEL_SGX_DRIVER) += driver/main.o driver/ioctl.o
>>>>>>
>>>>>> I think this is a great idea.
>>>>>
>>>>> On a 2nd thought not gonna do anything to that because it would
>>>>> require to move driver.h and it is cleaner to keep all the driver
>>>>> files in the same directory (and separated from the core).
>>>>
>>>> What about collapsing driver/*.c into driver.c and moving driver.{c,h}
>>>> to the root sgx directory?  The bulk of driver/main.c is securityfs
>>>> and platform driver code, e.g. has a good chance of going away entirely
>>>> or being moved out of the "driver".  At that point there probably isn't
>>>> a strong reason to have driver/main.c and driver/ioctl.c.
>>>
>>> I think doing anything major would require to lock in whether to have
>>> the LKM for the driver at all. If we wipe out the driver, then this is
>>> just matter of moving dev management part to lets say dev.c.
>>>
>>> Unless there is some real production use I can wipe it away. For v19
>>> I wanted to fix it namely because in v18 LKM was just broken. It is
>>> always good to make decisions based on working code.
>>
>> It should be a module because things should be modules when possible. I'm
>> not sure what the "Linux policy" is here but this seems obvious to me.
>>
>> For example:
>>
>> * Modules allow users to easily disable functionality that they don't use/is
>> buggy for them/other reasons using blacklisting.
>> * Modules allow users to customize their functionality without having to
>> rebuild the entire kernel.
>> * Modules allow developers to customize their modules without having to
>> rebuild the entire kernel.
> 
> I agree with all of the above, but unfortunately blacklisting is really
> the only benefit that would be realized by modularizing the driver.  The

...

> Tying into the your comment of "things should be modules when possible",
> we've gradually come to the realization that truly modularizing the SGX
> driver isn't possible, at least not without compromising other parts of
> the design.

What do you mean? The interface that the kernel needs to provide to any 
EPC-using modules is some way to allocate/free EPC pages and some way to 
associate EPC pages with enclaves (so that the swapper can be 
intelligent). This is pretty much exactly what the API looks like in v19.

Note that you still need enclave tracking with KVM if you want the host 
kernel to be able to page out guest EPC pages. However, you probably 
wouldn't want to do this with VMAs, so maybe there's an opportunity to 
streamline the API here.

> For example, relying on the EPC ACPI entry to autoprobe the driver falls
> apart when virtualization support also wants to add an SGX device (or we
> end up with a weird split model where the uapi driver is autoprobed via
> ACPI and the virtualization device is probed by the SGX subsystem).
> 
> Relying on the EPC ACPI entry really shows its warts when systems
> with multiple EPC sections show up.  The SGS BIOS writer's guide
> (allegedly, I haven't personally read it) says that one and only one
> EPC entry should be created in the ACPI tables, i.e. software must
> use CPUID to enumerate the base+size of EPC sections.
> 
> In other words, the whole ACPI entry and platform device approach was a
> hack purely to allow SGX to be implemented as an out-of-kernel driver.
> If the darn ACPI hack had never been added in the first place, i.e.
> CPUID is the only way to enumerate/probe SGX, then odds are we wouldn't
> even be having this dicsussion and no one would bat an eye at SGX being
> implemented as an Intel-specific feature that is baked into the kernel.
> 
>> Specifically for SGX I can think of the following reasons as well:
>>
>> * Module-based hypervisors may want to make EPC allocations for their
>> guests.
>> * Easy experimentation with different EPC interfaces
>> * Easy experimentation with in-kernel LE

The only example you give here is ACPI autoprobe. I don't really care 
about this. I do care about the other things I mentioned.

--
Jethro Beekman | Fortanix