| Message ID | 5076ed2c486ac33bfd87dc0e17047a1673692b53.1611634586.git.kai.huang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM SGX virtualization support | expand |
On 1/26/21 1:31 AM, Kai Huang wrote: > Modify sgx_init() to always try to initialize the virtual EPC driver, > even if the bare-metal SGX driver is disabled. The bare-metal driver > might be disabled if SGX Launch Control is in locked mode, or not > supported in the hardware at all. This allows (non-Linux) guests that > support non-LC configurations to use SGX. One thing worth calling out *somewhere* (which is entirely my fault): "bare-metal" in the context of this patch set refers to true bare-metal, but *ALSO* covers the plain SGX driver running inside a guest. So, perhaps "bare-metal" isn't the best term to use. Again, my bad. Better nomenclature suggestions are welcome.
> On Jan 26, 2021, at 9:03 AM, Dave Hansen <dave.hansen@intel.com> wrote: > > On 1/26/21 1:31 AM, Kai Huang wrote: >> Modify sgx_init() to always try to initialize the virtual EPC driver, >> even if the bare-metal SGX driver is disabled. The bare-metal driver >> might be disabled if SGX Launch Control is in locked mode, or not >> supported in the hardware at all. This allows (non-Linux) guests that >> support non-LC configurations to use SGX. > > One thing worth calling out *somewhere* (which is entirely my fault): > "bare-metal" in the context of this patch set refers to true bare-metal, > but *ALSO* covers the plain SGX driver running inside a guest. > > So, perhaps "bare-metal" isn't the best term to use. Again, my bad. > Better nomenclature suggestions are welcome. How about just SGX? We can have an SGX driver and a virtual EPC driver.
On Tue, 2021-01-26 at 10:10 -0800, Andy Lutomirski wrote: > > > On Jan 26, 2021, at 9:03 AM, Dave Hansen <dave.hansen@intel.com> wrote: > > > > On 1/26/21 1:31 AM, Kai Huang wrote: > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > might be disabled if SGX Launch Control is in locked mode, or not > > > supported in the hardware at all. This allows (non-Linux) guests that > > > support non-LC configurations to use SGX. > > > > One thing worth calling out *somewhere* (which is entirely my fault): > > "bare-metal" in the context of this patch set refers to true bare-metal, > > but *ALSO* covers the plain SGX driver running inside a guest. > > > > So, perhaps "bare-metal" isn't the best term to use. Again, my bad. > > Better nomenclature suggestions are welcome. > > > How about just SGX? We can have an SGX driver and a virtual EPC driver. Thanks. If no one has better idea, I'll change 'bare-metal' driver to SGX driver, in the whole series.
On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > Modify sgx_init() to always try to initialize the virtual EPC driver, > even if the bare-metal SGX driver is disabled. The bare-metal driver > might be disabled if SGX Launch Control is in locked mode, or not > supported in the hardware at all. This allows (non-Linux) guests that > support non-LC configurations to use SGX. > > Signed-off-by: Kai Huang <kai.huang@intel.com> > --- > v2->v3: > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > --- > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > index 21c2ffa13870..93d249f7bff3 100644 > --- a/arch/x86/kernel/cpu/sgx/main.c > +++ b/arch/x86/kernel/cpu/sgx/main.c > @@ -12,6 +12,7 @@ > #include "driver.h" > #include "encl.h" > #include "encls.h" > +#include "virt.h" > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > static int sgx_nr_epc_sections; > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > goto err_page_cache; > } > > - ret = sgx_drv_init(); > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); If would create more dumb code and just add ret = sgx_vepc_init() if (ret) goto err_kthread; > if (ret) > goto err_kthread; > > -- > 2.29.2 > /Jarkko >
On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > might be disabled if SGX Launch Control is in locked mode, or not > > supported in the hardware at all. This allows (non-Linux) guests that > > support non-LC configurations to use SGX. > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > --- > > v2->v3: > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > --- > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > index 21c2ffa13870..93d249f7bff3 100644 > > --- a/arch/x86/kernel/cpu/sgx/main.c > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > @@ -12,6 +12,7 @@ > > #include "driver.h" > > #include "encl.h" > > #include "encls.h" > > +#include "virt.h" > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > static int sgx_nr_epc_sections; > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > goto err_page_cache; > > } > > > > - ret = sgx_drv_init(); > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > If would create more dumb code and just add > > ret = sgx_vepc_init() > if (ret) > goto err_kthread; Do you mean you want below? ret = sgx_drv_init(); ret = sgx_vepc_init(); if (ret) goto err_kthread; This was Sean's original code, but Dave didn't like it. Sean/Dave, Please let me know which way you prefer. > > > if (ret) > > goto err_kthread; > > > > -- > > 2.29.2 > > > > /Jarkko > >
On 1/31/21 9:40 PM, Kai Huang wrote: >>> - ret = sgx_drv_init(); >>> + /* Success if the native *or* virtual EPC driver initialized cleanly. */ >>> + ret = !!sgx_drv_init() & !!sgx_vepc_init(); >> If would create more dumb code and just add >> >> ret = sgx_vepc_init() >> if (ret) >> goto err_kthread; Jarkko, I'm not sure I understand this suggestion. > Do you mean you want below? > > ret = sgx_drv_init(); > ret = sgx_vepc_init(); > if (ret) > goto err_kthread; > > This was Sean's original code, but Dave didn't like it. Are you sure? I remember the !!&!! abomination being Sean's doing. :) > Sean/Dave, > > Please let me know which way you prefer. Kai, I don't really know you are saying here. In the end, sgx_vepc_init() has to run regardless of whether sgx_drv_init() is successful or not. Also, we only want to 'goto err_kthraed' if *BOTH* fail. The code you have above will, for instance, 'goto err_kthread' if sgx_drv_init() succeeds but sgx_vepc_init() fails. It entirely disregards the sgx_drv_init() error code.
On Mon, Feb 01, 2021, Dave Hansen wrote: > On 1/31/21 9:40 PM, Kai Huang wrote: > >>> - ret = sgx_drv_init(); > >>> + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > >>> + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > >> If would create more dumb code and just add > >> > >> ret = sgx_vepc_init() > >> if (ret) > >> goto err_kthread; > > Jarkko, I'm not sure I understand this suggestion. > > > Do you mean you want below? > > > > ret = sgx_drv_init(); > > ret = sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > > > This was Sean's original code, but Dave didn't like it. The problem is it's wrong. That snippet would incorrectly bail if drv_init() succeeds but vepc_init() fails. The alternative to the bitwise AND is to snapshot the result in two separate variables: ret = sgx_drv_init(); ret2 = sgx_vepc_init(); if (ret && ret2) goto err_kthread; or check the return from drv_init() _after_ vepc_init(): ret = sgx_drv_init(); if (sgx_vepc_init() && ret) goto err_kthread; As evidenced by this thread, the behavior is subtle and easy to get wrong. I deliberately chose the option that was the weirdest specifically to reduce the probability of someone incorrectly "cleaning up" the code. > Are you sure? I remember the !!&!! abomination being Sean's doing. :) Yep! That 100% functionally correct horror is my doing. > > Sean/Dave, > > > > Please let me know which way you prefer. > > Kai, I don't really know you are saying here. In the end, > sgx_vepc_init() has to run regardless of whether sgx_drv_init() is > successful or not. Also, we only want to 'goto err_kthraed' if *BOTH* > fail. The code you have above will, for instance, 'goto err_kthread' if > sgx_drv_init() succeeds but sgx_vepc_init() fails. It entirely > disregards the sgx_drv_init() error code.
On Mon, 1 Feb 2021 09:23:18 -0800 Sean Christopherson wrote: > On Mon, Feb 01, 2021, Dave Hansen wrote: > > On 1/31/21 9:40 PM, Kai Huang wrote: > > >>> - ret = sgx_drv_init(); > > >>> + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > >>> + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > >> If would create more dumb code and just add > > >> > > >> ret = sgx_vepc_init() > > >> if (ret) > > >> goto err_kthread; > > > > Jarkko, I'm not sure I understand this suggestion. > > > > > Do you mean you want below? > > > > > > ret = sgx_drv_init(); > > > ret = sgx_vepc_init(); > > > if (ret) > > > goto err_kthread; > > > > > > This was Sean's original code, but Dave didn't like it. > > The problem is it's wrong. That snippet would incorrectly bail if drv_init() > succeeds but vepc_init() fails. > > The alternative to the bitwise AND is to snapshot the result in two separate > variables: > > ret = sgx_drv_init(); > ret2 = sgx_vepc_init(); > if (ret && ret2) > goto err_kthread; > > or check the return from drv_init() _after_ vepc_init(): > > ret = sgx_drv_init(); > if (sgx_vepc_init() && ret) > goto err_kthread; > > > As evidenced by this thread, the behavior is subtle and easy to get wrong. I > deliberately chose the option that was the weirdest specifically to reduce the > probability of someone incorrectly "cleaning up" the code. > > > Are you sure? I remember the !!&!! abomination being Sean's doing. :) > > Yep! That 100% functionally correct horror is my doing. > > > > Sean/Dave, > > > > > > Please let me know which way you prefer. > > > > Kai, I don't really know you are saying here. In the end, > > sgx_vepc_init() has to run regardless of whether sgx_drv_init() is > > successful or not. Also, we only want to 'goto err_kthraed' if *BOTH* > > fail. The code you have above will, for instance, 'goto err_kthread' if > > sgx_drv_init() succeeds but sgx_vepc_init() fails. It entirely > > disregards the sgx_drv_init() error code. > Hi Dave, Sean, Yeah sorry my bad. The example I provided won't work. So I'd like keep the !! &!! :) Thanks. Jarkko, please let us know if you still have concern.
On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > might be disabled if SGX Launch Control is in locked mode, or not > > > supported in the hardware at all. This allows (non-Linux) guests that > > > support non-LC configurations to use SGX. > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > --- > > > v2->v3: > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > --- > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > index 21c2ffa13870..93d249f7bff3 100644 > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > @@ -12,6 +12,7 @@ > > > #include "driver.h" > > > #include "encl.h" > > > #include "encls.h" > > > +#include "virt.h" > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > static int sgx_nr_epc_sections; > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > goto err_page_cache; > > > } > > > > > > - ret = sgx_drv_init(); > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > If would create more dumb code and just add > > > > ret = sgx_vepc_init() > > if (ret) > > goto err_kthread; > > Do you mean you want below? > > ret = sgx_drv_init(); > ret = sgx_vepc_init(); > if (ret) > goto err_kthread; > > This was Sean's original code, but Dave didn't like it. I think it should be like: ret = sgx_drv_init(); if (ret) pr_warn("Driver initialization failed with %d\n", ret); ret = sgx_vepc_init(); if (ret) goto err_kthread; There is problem here anyhow. I.e. -ENODEV's from sgx_drv_init(). I think how driver.c should be changed would be just to return 0 in the places where it now return -ENODEV. Consider "not initialized" as a successful initialization. /Jarkko
On Tue, Feb 02, 2021, Jarkko Sakkinen wrote: > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > > might be disabled if SGX Launch Control is in locked mode, or not > > > > supported in the hardware at all. This allows (non-Linux) guests that > > > > support non-LC configurations to use SGX. > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > --- > > > > v2->v3: > > > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > > > --- > > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > index 21c2ffa13870..93d249f7bff3 100644 > > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > > @@ -12,6 +12,7 @@ > > > > #include "driver.h" > > > > #include "encl.h" > > > > #include "encls.h" > > > > +#include "virt.h" > > > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > > static int sgx_nr_epc_sections; > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > > goto err_page_cache; > > > > } > > > > > > > > - ret = sgx_drv_init(); > > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > > > If would create more dumb code and just add > > > > > > ret = sgx_vepc_init() > > > if (ret) > > > goto err_kthread; > > > > Do you mean you want below? > > > > ret = sgx_drv_init(); > > ret = sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > > > This was Sean's original code, but Dave didn't like it. > > I think it should be like: > > ret = sgx_drv_init(); > if (ret) > pr_warn("Driver initialization failed with %d\n", ret); > > ret = sgx_vepc_init(); > if (ret) > goto err_kthread; And that's wrong, it doesn't correctly handle the case where sgx_drv_init() succeeds but sgx_vepc_init() fails. > There is problem here anyhow. I.e. -ENODEV's from sgx_drv_init(). I think > how driver.c should be changed would be just to return 0 in the places > where it now return -ENODEV. Consider "not initialized" as a successful > initialization.
On Tue, 2 Feb 2021 19:32:30 +0200 Jarkko Sakkinen wrote: > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > > might be disabled if SGX Launch Control is in locked mode, or not > > > > supported in the hardware at all. This allows (non-Linux) guests that > > > > support non-LC configurations to use SGX. > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > --- > > > > v2->v3: > > > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > > > --- > > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > index 21c2ffa13870..93d249f7bff3 100644 > > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > > @@ -12,6 +12,7 @@ > > > > #include "driver.h" > > > > #include "encl.h" > > > > #include "encls.h" > > > > +#include "virt.h" > > > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > > static int sgx_nr_epc_sections; > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > > goto err_page_cache; > > > > } > > > > > > > > - ret = sgx_drv_init(); > > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > > > If would create more dumb code and just add > > > > > > ret = sgx_vepc_init() > > > if (ret) > > > goto err_kthread; > > > > Do you mean you want below? > > > > ret = sgx_drv_init(); > > ret = sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > > > This was Sean's original code, but Dave didn't like it. > > I think it should be like: > > ret = sgx_drv_init(); > if (ret) > pr_warn("Driver initialization failed with %d\n", ret); > > ret = sgx_vepc_init(); > if (ret) > goto err_kthread; > > There is problem here anyhow. I.e. -ENODEV's from sgx_drv_init(). I think > how driver.c should be changed would be just to return 0 in the places > where it now return -ENODEV. Consider "not initialized" as a successful > initialization. Hi Jarkko, Dave already pointed out above code won't work. The problem is failure to initialize vepc will just goto err_kthread, no matter whether driver has been initialized successfully or not. I am sticking to the original way (!! & !!). > > /Jarkko
On Mon, Feb 01, 2021 at 07:25:41AM -0800, Dave Hansen wrote: > On 1/31/21 9:40 PM, Kai Huang wrote: > >>> - ret = sgx_drv_init(); > >>> + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > >>> + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > >> If would create more dumb code and just add > >> > >> ret = sgx_vepc_init() > >> if (ret) > >> goto err_kthread; > > Jarkko, I'm not sure I understand this suggestion. I refined it in my 2nd response to Kai: https://lore.kernel.org/linux-sgx/YBmMrqxlTxClg9Eb@kernel.org/ /Jarkko
On Tue, Feb 02, 2021 at 01:12:07PM +1300, Kai Huang wrote: > On Mon, 1 Feb 2021 09:23:18 -0800 Sean Christopherson wrote: > > On Mon, Feb 01, 2021, Dave Hansen wrote: > > > On 1/31/21 9:40 PM, Kai Huang wrote: > > > >>> - ret = sgx_drv_init(); > > > >>> + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > >>> + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > >> If would create more dumb code and just add > > > >> > > > >> ret = sgx_vepc_init() > > > >> if (ret) > > > >> goto err_kthread; > > > > > > Jarkko, I'm not sure I understand this suggestion. > > > > > > > Do you mean you want below? > > > > > > > > ret = sgx_drv_init(); > > > > ret = sgx_vepc_init(); > > > > if (ret) > > > > goto err_kthread; > > > > > > > > This was Sean's original code, but Dave didn't like it. > > > > The problem is it's wrong. That snippet would incorrectly bail if drv_init() > > succeeds but vepc_init() fails. > > > > The alternative to the bitwise AND is to snapshot the result in two separate > > variables: > > > > ret = sgx_drv_init(); > > ret2 = sgx_vepc_init(); > > if (ret && ret2) > > goto err_kthread; > > > > or check the return from drv_init() _after_ vepc_init(): > > > > ret = sgx_drv_init(); > > if (sgx_vepc_init() && ret) > > goto err_kthread; > > > > > > As evidenced by this thread, the behavior is subtle and easy to get wrong. I > > deliberately chose the option that was the weirdest specifically to reduce the > > probability of someone incorrectly "cleaning up" the code. > > > > > Are you sure? I remember the !!&!! abomination being Sean's doing. :) > > > > Yep! That 100% functionally correct horror is my doing. > > > > > > Sean/Dave, > > > > > > > > Please let me know which way you prefer. > > > > > > Kai, I don't really know you are saying here. In the end, > > > sgx_vepc_init() has to run regardless of whether sgx_drv_init() is > > > successful or not. Also, we only want to 'goto err_kthraed' if *BOTH* > > > fail. The code you have above will, for instance, 'goto err_kthread' if > > > sgx_drv_init() succeeds but sgx_vepc_init() fails. It entirely > > > disregards the sgx_drv_init() error code. > > > > Hi Dave, Sean, > > Yeah sorry my bad. The example I provided won't work. So I'd like keep the !! > &!! :) > > Thanks. > > Jarkko, please let us know if you still have concern. I'll just review the next version. I disagree with this packing though. /Jarkko
On Tue, Feb 02, 2021 at 10:20:47AM -0800, Sean Christopherson wrote: > On Tue, Feb 02, 2021, Jarkko Sakkinen wrote: > > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > > > might be disabled if SGX Launch Control is in locked mode, or not > > > > > supported in the hardware at all. This allows (non-Linux) guests that > > > > > support non-LC configurations to use SGX. > > > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > > --- > > > > > v2->v3: > > > > > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > > > > > --- > > > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > index 21c2ffa13870..93d249f7bff3 100644 > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > > > @@ -12,6 +12,7 @@ > > > > > #include "driver.h" > > > > > #include "encl.h" > > > > > #include "encls.h" > > > > > +#include "virt.h" > > > > > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > > > static int sgx_nr_epc_sections; > > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > > > goto err_page_cache; > > > > > } > > > > > > > > > > - ret = sgx_drv_init(); > > > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > > > > > If would create more dumb code and just add > > > > > > > > ret = sgx_vepc_init() > > > > if (ret) > > > > goto err_kthread; > > > > > > Do you mean you want below? > > > > > > ret = sgx_drv_init(); > > > ret = sgx_vepc_init(); > > > if (ret) > > > goto err_kthread; > > > > > > This was Sean's original code, but Dave didn't like it. > > > > I think it should be like: > > > > ret = sgx_drv_init(); > > if (ret) > > pr_warn("Driver initialization failed with %d\n", ret); > > > > ret = sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > And that's wrong, it doesn't correctly handle the case where sgx_drv_init() > succeeds but sgx_vepc_init() fails. After reading all of this, I think that the only acceptable way to to manage this is to ret = sgx_drv_init(); if (ret && ret != -ENODEV) goto err_kthread; ret = sgx_vepc_init(); if (ret) goto err_kthread; Anything else would be a bad idea. We do support allowing KVM when the driver does not *support* SGX, not when something is working incorrectly. In that case it is a bad idea to allow any SGX related initialization to continue. Agreed that my earlier example is incorrect but so is the condition in the original patch. /Jarkko
On Wed, Feb 03, 2021 at 07:49:45AM +1300, Kai Huang wrote: > On Tue, 2 Feb 2021 19:32:30 +0200 Jarkko Sakkinen wrote: > > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > > > might be disabled if SGX Launch Control is in locked mode, or not > > > > > supported in the hardware at all. This allows (non-Linux) guests that > > > > > support non-LC configurations to use SGX. > > > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > > --- > > > > > v2->v3: > > > > > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > > > > > --- > > > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > index 21c2ffa13870..93d249f7bff3 100644 > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > > > @@ -12,6 +12,7 @@ > > > > > #include "driver.h" > > > > > #include "encl.h" > > > > > #include "encls.h" > > > > > +#include "virt.h" > > > > > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > > > static int sgx_nr_epc_sections; > > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > > > goto err_page_cache; > > > > > } > > > > > > > > > > - ret = sgx_drv_init(); > > > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > > > > > If would create more dumb code and just add > > > > > > > > ret = sgx_vepc_init() > > > > if (ret) > > > > goto err_kthread; > > > > > > Do you mean you want below? > > > > > > ret = sgx_drv_init(); > > > ret = sgx_vepc_init(); > > > if (ret) > > > goto err_kthread; > > > > > > This was Sean's original code, but Dave didn't like it. > > > > I think it should be like: > > > > ret = sgx_drv_init(); > > if (ret) > > pr_warn("Driver initialization failed with %d\n", ret); > > > > ret = sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > > > There is problem here anyhow. I.e. -ENODEV's from sgx_drv_init(). I think > > how driver.c should be changed would be just to return 0 in the places > > where it now return -ENODEV. Consider "not initialized" as a successful > > initialization. > > Hi Jarkko, > > Dave already pointed out above code won't work. The problem is failure to > initialize vepc will just goto err_kthread, no matter whether driver has been > initialized successfully or not. > > I am sticking to the original way (!! & !!). I think it is wrong, as it is not in line with the conditions when KVM SGX support is allowed. It's exactly allowed when SGX is not supported by the driver. Not when things are not behaving right. Would be insane to allow anything to initialize in that situation. /Jarkko
On Wed, 3 Feb 2021 01:16:20 +0200 Jarkko Sakkinen wrote: > On Tue, Feb 02, 2021 at 10:20:47AM -0800, Sean Christopherson wrote: > > On Tue, Feb 02, 2021, Jarkko Sakkinen wrote: > > > On Mon, Feb 01, 2021 at 06:40:40PM +1300, Kai Huang wrote: > > > > On Sat, 30 Jan 2021 16:45:43 +0200 Jarkko Sakkinen wrote: > > > > > On Tue, Jan 26, 2021 at 10:31:00PM +1300, Kai Huang wrote: > > > > > > Modify sgx_init() to always try to initialize the virtual EPC driver, > > > > > > even if the bare-metal SGX driver is disabled. The bare-metal driver > > > > > > might be disabled if SGX Launch Control is in locked mode, or not > > > > > > supported in the hardware at all. This allows (non-Linux) guests that > > > > > > support non-LC configurations to use SGX. > > > > > > > > > > > > Signed-off-by: Kai Huang <kai.huang@intel.com> > > > > > > --- > > > > > > v2->v3: > > > > > > > > > > > > - Changed from sgx_virt_epc_init() to sgx_vepc_init(). > > > > > > > > > > > > --- > > > > > > arch/x86/kernel/cpu/sgx/main.c | 4 +++- > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c > > > > > > index 21c2ffa13870..93d249f7bff3 100644 > > > > > > --- a/arch/x86/kernel/cpu/sgx/main.c > > > > > > +++ b/arch/x86/kernel/cpu/sgx/main.c > > > > > > @@ -12,6 +12,7 @@ > > > > > > #include "driver.h" > > > > > > #include "encl.h" > > > > > > #include "encls.h" > > > > > > +#include "virt.h" > > > > > > > > > > > > struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; > > > > > > static int sgx_nr_epc_sections; > > > > > > @@ -712,7 +713,8 @@ static int __init sgx_init(void) > > > > > > goto err_page_cache; > > > > > > } > > > > > > > > > > > > - ret = sgx_drv_init(); > > > > > > + /* Success if the native *or* virtual EPC driver initialized cleanly. */ > > > > > > + ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > > > > > > > > > If would create more dumb code and just add > > > > > > > > > > ret = sgx_vepc_init() > > > > > if (ret) > > > > > goto err_kthread; > > > > > > > > Do you mean you want below? > > > > > > > > ret = sgx_drv_init(); > > > > ret = sgx_vepc_init(); > > > > if (ret) > > > > goto err_kthread; > > > > > > > > This was Sean's original code, but Dave didn't like it. > > > > > > I think it should be like: > > > > > > ret = sgx_drv_init(); > > > if (ret) > > > pr_warn("Driver initialization failed with %d\n", ret); > > > > > > ret = sgx_vepc_init(); > > > if (ret) > > > goto err_kthread; > > > > And that's wrong, it doesn't correctly handle the case where sgx_drv_init() > > succeeds but sgx_vepc_init() fails. > > After reading all of this, I think that the only acceptable way to > to manage this is to > > ret = sgx_drv_init(); > if (ret && ret != -ENODEV) > goto err_kthread; Why? From SGX virtualization's perspective, it doesn't care what error code caused driver not being initialized properly. Actually it even doesn't care about whether driver initialization is successful or not. > > ret = sgx_vepc_init(); > if (ret) > goto err_kthread; > > Anything else would be a bad idea. > > We do support allowing KVM when the driver does not *support* SGX, > not when something is working incorrectly. What working *incorrectly* thing is related to SGX virtualization? The things SGX virtualization requires (basically just raw EPC allocation) are all in sgx/main.c. In that case it is a bad > idea to allow any SGX related initialization to continue. > > Agreed that my earlier example is incorrect but so is the condition > in the original patch. > > /Jarkko
On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > What working *incorrectly* thing is related to SGX virtualization? The things > SGX virtualization requires (basically just raw EPC allocation) are all in > sgx/main.c. States: A. SGX driver is unsupported. B. SGX driver is supported and initialized correctly. C. SGX driver is supported and failed to initialize. I just thought that KVM should support SGX when we are either in states A or B. Even the short summary implies this. It is expected that SGX driver initializes correctly if it is supported in the first place. If it doesn't, something is probaly seriously wrong. That is something we don't expect in a legit system behavior. /Jarkko
On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > What working *incorrectly* thing is related to SGX virtualization? The things > > SGX virtualization requires (basically just raw EPC allocation) are all in > > sgx/main.c. > > States: > > A. SGX driver is unsupported. > B. SGX driver is supported and initialized correctly. > C. SGX driver is supported and failed to initialize. > > I just thought that KVM should support SGX when we are either in states A > or B. Even the short summary implies this. It is expected that SGX driver > initializes correctly if it is supported in the first place. If it doesn't, > something is probaly seriously wrong. That is something we don't expect in > a legit system behavior. It's legit behavior, and something we (you?) explicitly want to support. See patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support.
On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > sgx/main.c. > > > > States: > > > > A. SGX driver is unsupported. > > B. SGX driver is supported and initialized correctly. > > C. SGX driver is supported and failed to initialize. > > > > I just thought that KVM should support SGX when we are either in states A > > or B. Even the short summary implies this. It is expected that SGX driver > > initializes correctly if it is supported in the first place. If it doesn't, > > something is probaly seriously wrong. That is something we don't expect in > > a legit system behavior. > > It's legit behavior, and something we (you?) explicitly want to support. See > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. What I think would be a sane behavior, would be to allow KVM when sgx_drv_init() returns -ENODEV (case A). This happens when LC is not enabled: if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) return -ENODEV; /Jarkko
On Thu, 2021-02-04 at 03:39 +0200, Jarkko Sakkinen wrote: > On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > > sgx/main.c. > > > > > > States: > > > > > > A. SGX driver is unsupported. > > > B. SGX driver is supported and initialized correctly. > > > C. SGX driver is supported and failed to initialize. > > > > > > I just thought that KVM should support SGX when we are either in states A > > > or B. Even the short summary implies this. It is expected that SGX driver > > > initializes correctly if it is supported in the first place. If it doesn't, > > > something is probaly seriously wrong. That is something we don't expect in > > > a legit system behavior. > > > > It's legit behavior, and something we (you?) explicitly want to support. See > > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. > > What I think would be a sane behavior, would be to allow KVM when > sgx_drv_init() returns -ENODEV (case A). This happens when LC is > not enabled: > > if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) > return -ENODEV; > > /Jarkko I really don't understand what's the difference between A and C. When "SGX driver is supported and failed to initialize" happens, it just means "SGX driver is unsupported". If it is not the case, can you explicitly point out what will be the problem?
On Thu, Feb 04, 2021 at 03:59:20PM +1300, Kai Huang wrote: > On Thu, 2021-02-04 at 03:39 +0200, Jarkko Sakkinen wrote: > > On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > > > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > > > sgx/main.c. > > > > > > > > States: > > > > > > > > A. SGX driver is unsupported. > > > > B. SGX driver is supported and initialized correctly. > > > > C. SGX driver is supported and failed to initialize. > > > > > > > > I just thought that KVM should support SGX when we are either in states A > > > > or B. Even the short summary implies this. It is expected that SGX driver > > > > initializes correctly if it is supported in the first place. If it doesn't, > > > > something is probaly seriously wrong. That is something we don't expect in > > > > a legit system behavior. > > > > > > It's legit behavior, and something we (you?) explicitly want to support. See > > > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. > > > > What I think would be a sane behavior, would be to allow KVM when > > sgx_drv_init() returns -ENODEV (case A). This happens when LC is > > not enabled: > > > > if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) > > return -ENODEV; > > > > /Jarkko > > I really don't understand what's the difference between A and C. When "SGX driver is > supported and failed to initialize" happens, it just means "SGX driver is > unsupported". If it is not the case, can you explicitly point out what will be the > problem? ret != 0 && ret != -ENODEV /Jarkko
On Thu, Feb 04, 2021 at 05:05:56AM +0200, Jarkko Sakkinen wrote: > On Thu, Feb 04, 2021 at 03:59:20PM +1300, Kai Huang wrote: > > On Thu, 2021-02-04 at 03:39 +0200, Jarkko Sakkinen wrote: > > > On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > > > > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > > > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > > > > sgx/main.c. > > > > > > > > > > States: > > > > > > > > > > A. SGX driver is unsupported. > > > > > B. SGX driver is supported and initialized correctly. > > > > > C. SGX driver is supported and failed to initialize. > > > > > > > > > > I just thought that KVM should support SGX when we are either in states A > > > > > or B. Even the short summary implies this. It is expected that SGX driver > > > > > initializes correctly if it is supported in the first place. If it doesn't, > > > > > something is probaly seriously wrong. That is something we don't expect in > > > > > a legit system behavior. > > > > > > > > It's legit behavior, and something we (you?) explicitly want to support. See > > > > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. > > > > > > What I think would be a sane behavior, would be to allow KVM when > > > sgx_drv_init() returns -ENODEV (case A). This happens when LC is > > > not enabled: > > > > > > if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) > > > return -ENODEV; > > > > > > /Jarkko > > > > I really don't understand what's the difference between A and C. When "SGX driver is > > supported and failed to initialize" happens, it just means "SGX driver is > > unsupported". If it is not the case, can you explicitly point out what will be the > > problem? This is as explicit as I can ever possibly get: A: ret == -ENODEV B: ret == 0 C: ret != 0 && ret != -ENODEV /Jarkko
On Thu, 2021-02-04 at 05:09 +0200, Jarkko Sakkinen wrote: > On Thu, Feb 04, 2021 at 05:05:56AM +0200, Jarkko Sakkinen wrote: > > On Thu, Feb 04, 2021 at 03:59:20PM +1300, Kai Huang wrote: > > > On Thu, 2021-02-04 at 03:39 +0200, Jarkko Sakkinen wrote: > > > > On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > > > > > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > > > > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > > > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > > > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > > > > > sgx/main.c. > > > > > > > > > > > > States: > > > > > > > > > > > > A. SGX driver is unsupported. > > > > > > B. SGX driver is supported and initialized correctly. > > > > > > C. SGX driver is supported and failed to initialize. > > > > > > > > > > > > I just thought that KVM should support SGX when we are either in states A > > > > > > or B. Even the short summary implies this. It is expected that SGX driver > > > > > > initializes correctly if it is supported in the first place. If it doesn't, > > > > > > something is probaly seriously wrong. That is something we don't expect in > > > > > > a legit system behavior. > > > > > > > > > > It's legit behavior, and something we (you?) explicitly want to support. See > > > > > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. > > > > > > > > What I think would be a sane behavior, would be to allow KVM when > > > > sgx_drv_init() returns -ENODEV (case A). This happens when LC is > > > > not enabled: > > > > > > > > if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) > > > > return -ENODEV; > > > > > > > > /Jarkko > > > > > > I really don't understand what's the difference between A and C. When "SGX driver is > > > supported and failed to initialize" happens, it just means "SGX driver is > > > unsupported". If it is not the case, can you explicitly point out what will be the > > > problem? > > This is as explicit as I can ever possibly get: > > A: ret == -ENODEV > B: ret == 0 > C: ret != 0 && ret != -ENODEV Let me try again: Why A and C should be treated differently? What will behave incorrectly, in case of C? > > /Jarkko
On Thu, Feb 04, 2021 at 04:20:49PM +1300, Kai Huang wrote: > On Thu, 2021-02-04 at 05:09 +0200, Jarkko Sakkinen wrote: > > On Thu, Feb 04, 2021 at 05:05:56AM +0200, Jarkko Sakkinen wrote: > > > On Thu, Feb 04, 2021 at 03:59:20PM +1300, Kai Huang wrote: > > > > On Thu, 2021-02-04 at 03:39 +0200, Jarkko Sakkinen wrote: > > > > > On Wed, Feb 03, 2021 at 02:59:47PM -0800, Sean Christopherson wrote: > > > > > > On Thu, Feb 04, 2021, Jarkko Sakkinen wrote: > > > > > > > On Wed, Feb 03, 2021 at 01:49:06PM +1300, Kai Huang wrote: > > > > > > > > What working *incorrectly* thing is related to SGX virtualization? The things > > > > > > > > SGX virtualization requires (basically just raw EPC allocation) are all in > > > > > > > > sgx/main.c. > > > > > > > > > > > > > > States: > > > > > > > > > > > > > > A. SGX driver is unsupported. > > > > > > > B. SGX driver is supported and initialized correctly. > > > > > > > C. SGX driver is supported and failed to initialize. > > > > > > > > > > > > > > I just thought that KVM should support SGX when we are either in states A > > > > > > > or B. Even the short summary implies this. It is expected that SGX driver > > > > > > > initializes correctly if it is supported in the first place. If it doesn't, > > > > > > > something is probaly seriously wrong. That is something we don't expect in > > > > > > > a legit system behavior. > > > > > > > > > > > > It's legit behavior, and something we (you?) explicitly want to support. See > > > > > > patch 05, x86/cpu/intel: Allow SGX virtualization without Launch Control support. > > > > > > > > > > What I think would be a sane behavior, would be to allow KVM when > > > > > sgx_drv_init() returns -ENODEV (case A). This happens when LC is > > > > > not enabled: > > > > > > > > > > if (!cpu_feature_enabled(X86_FEATURE_SGX_LC)) > > > > > return -ENODEV; > > > > > > > > > > /Jarkko > > > > > > > > I really don't understand what's the difference between A and C. When "SGX driver is > > > > supported and failed to initialize" happens, it just means "SGX driver is > > > > unsupported". If it is not the case, can you explicitly point out what will be the > > > > problem? > > > > This is as explicit as I can ever possibly get: > > > > A: ret == -ENODEV > > B: ret == 0 > > C: ret != 0 && ret != -ENODEV > > Let me try again: > > Why A and C should be treated differently? What will behave incorrectly, in case of > C? So you don't know what different error codes mean? /Jarkko
On 2/4/21 6:51 AM, Jarkko Sakkinen wrote: >>> A: ret == -ENODEV >>> B: ret == 0 >>> C: ret != 0 && ret != -ENODEV >> Let me try again: >> >> Why A and C should be treated differently? What will behave incorrectly, in case of >> C? > So you don't know what different error codes mean? How about we just leave the check in place as Sean wrote it, and add a nice comment to explain what it is doing: /* * Always try to initialize the native *and* KVM drivers. * The KVM driver is less picky than the native one and * can function if the native one is not supported on the * current system or fails to initialize. * * Error out only if both fail to initialize. */ ret = !!sgx_drv_init() & !!sgx_vepc_init(); if (ret) goto err_kthread;
On Thu, 2021-02-04 at 14:41 -0800, Dave Hansen wrote: > On 2/4/21 6:51 AM, Jarkko Sakkinen wrote: > > > > A: ret == -ENODEV > > > > B: ret == 0 > > > > C: ret != 0 && ret != -ENODEV > > > Let me try again: > > > > > > Why A and C should be treated differently? What will behave incorrectly, in case of > > > C? > > So you don't know what different error codes mean? > > How about we just leave the check in place as Sean wrote it, and add a > nice comment to explain what it is doing: > > /* > * Always try to initialize the native *and* KVM drivers. > * The KVM driver is less picky than the native one and > * can function if the native one is not supported on the > * current system or fails to initialize. > * > * Error out only if both fail to initialize. > */ > ret = !!sgx_drv_init() & !!sgx_vepc_init(); > if (ret) > goto err_kthread; > Perfect to me. Thanks.
On Thu, Feb 04, 2021 at 02:41:57PM -0800, Dave Hansen wrote: > On 2/4/21 6:51 AM, Jarkko Sakkinen wrote: > >>> A: ret == -ENODEV > >>> B: ret == 0 > >>> C: ret != 0 && ret != -ENODEV > >> Let me try again: > >> > >> Why A and C should be treated differently? What will behave incorrectly, in case of > >> C? > > So you don't know what different error codes mean? > > How about we just leave the check in place as Sean wrote it, and add a > nice comment to explain what it is doing: > > /* > * Always try to initialize the native *and* KVM drivers. > * The KVM driver is less picky than the native one and > * can function if the native one is not supported on the > * current system or fails to initialize. > * > * Error out only if both fail to initialize. > */ > ret = !!sgx_drv_init() & !!sgx_vepc_init(); > if (ret) > goto err_kthread; WFM, I can go along, as long as there is a remark. There is a semantical difference between "not supported" and "failure to initialize". The driving point is that this should not be hidden. I was first thinking a note in the commit message, but inline comment is actually a better idea. Thanks! I can ack the next version, as long as this comment is included. /Jarkko
> On Thu, Feb 04, 2021 at 02:41:57PM -0800, Dave Hansen wrote: > > On 2/4/21 6:51 AM, Jarkko Sakkinen wrote: > > >>> A: ret == -ENODEV > > >>> B: ret == 0 > > >>> C: ret != 0 && ret != -ENODEV > > >> Let me try again: > > >> > > >> Why A and C should be treated differently? What will behave > > >> incorrectly, in case of C? > > > So you don't know what different error codes mean? > > > > How about we just leave the check in place as Sean wrote it, and add a > > nice comment to explain what it is doing: > > > > /* > > * Always try to initialize the native *and* KVM drivers. > > * The KVM driver is less picky than the native one and > > * can function if the native one is not supported on the > > * current system or fails to initialize. > > * > > * Error out only if both fail to initialize. > > */ > > ret = !!sgx_drv_init() & !!sgx_vepc_init(); > > if (ret) > > goto err_kthread; > > WFM, I can go along, as long as there is a remark. There is a semantical > difference between "not supported" and "failure to initialize". The driving point > is that this should not be hidden. I was first thinking a note in the commit > message, but inline comment is actually a better idea. Thanks! > > I can ack the next version, as long as this comment is included. Sure. Thanks Dave and Jarkko. > > /Jarkko
diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c index 21c2ffa13870..93d249f7bff3 100644 --- a/arch/x86/kernel/cpu/sgx/main.c +++ b/arch/x86/kernel/cpu/sgx/main.c @@ -12,6 +12,7 @@ #include "driver.h" #include "encl.h" #include "encls.h" +#include "virt.h" struct sgx_epc_section sgx_epc_sections[SGX_MAX_EPC_SECTIONS]; static int sgx_nr_epc_sections; @@ -712,7 +713,8 @@ static int __init sgx_init(void) goto err_page_cache; } - ret = sgx_drv_init(); + /* Success if the native *or* virtual EPC driver initialized cleanly. */ + ret = !!sgx_drv_init() & !!sgx_vepc_init(); if (ret) goto err_kthread;
Modify sgx_init() to always try to initialize the virtual EPC driver, even if the bare-metal SGX driver is disabled. The bare-metal driver might be disabled if SGX Launch Control is in locked mode, or not supported in the hardware at all. This allows (non-Linux) guests that support non-LC configurations to use SGX. Signed-off-by: Kai Huang <kai.huang@intel.com> --- v2->v3: - Changed from sgx_virt_epc_init() to sgx_vepc_init(). --- arch/x86/kernel/cpu/sgx/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)