Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization

From: "Huang, Kai" <kai.huang@intel.com>
To: "linux-sgx@vger.kernel.org" <linux-sgx@vger.kernel.org>,
	"Luck, Tony" <tony.luck@intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@intel.com>,
	"Hansen, Dave" <dave.hansen@intel.com>,
	"jarkko@kernel.org" <jarkko@kernel.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
	"Du, Fan" <fan.du@intel.com>, "Christopherson,,
	Sean" <seanjc@google.com>, "Zhang, Cathy" <cathy.zhang@intel.com>,
	"bp@suse.de" <bp@suse.de>
Subject: Re: [PATCH v9 3/3] x86/sgx: Fine grained SGX MCA behavior for virtualization
Date: Wed, 12 Oct 2022 11:01:49 +0000	[thread overview]
Message-ID: <188a334629319e430e16d4caf5e4a421b80c357a.camel@intel.com> (raw)
In-Reply-To: <14fabc80-a9fc-05b4-eda1-62f2b265e66b@intel.com>

On Wed, 2022-10-12 at 13:09 +0800, Zhiquan Li wrote:
> > > SGX virtual EPC driver doesn't explicitly prevent virtual EPC instance
> > > being shared by multiple VMs via fork().  However KVM doesn't support
> > > running a VM across multiple mm structures, and the de facto userspace
> > > hypervisor (Qemu) doesn't use fork() to create a new VM, so in practice
> > > this should not happen.
> > 
> > This is out of the blue.  Why is this here?
> > 
> > What happens if a hypervisor *DOES* fork()?  What's the fallout?
> 
> This part originates from below discussion:
> 
> https://lore.kernel.org/linux-sgx/52dc7f50b68c99cecb9e1c3383d9c6d88734cd67.camel@intel.com/#t
> 
> It intents to answer the question:
> 
>     Do you think the processes sharing the same enclave need to be
>     killed, even they had not touched the EPC page with hardware error?

Sharing virtual EPC instance will very likely unexpectedly break enclaves in all
VMs.  Whether kernel should explicitly prevent is another topic. To me I don't
see strong reason to enforce in the kernel.  For instance, multiple VMs can map
the same file as memory backend with MAP_SHARED, in which case they can all
break.  Userspace should use virtual EPC in the right way.

But the point is above is not directly related to your patch.  On host where
multiple processes can share one enclave legally, it does the same thing.  I
think you can just remove that paragraph from changelog.

-- 
Thanks,
-Kai