[v8,2/3] x86/sgx: Introduce union with vepc_vaddr field for virtualization case

Commit Message

Zhiquan Li Sept. 13, 2022, 2:53 p.m. UTC

When a page triggers a machine check, it only reports the PFN.  But in
order to inject #MC into hypervisor, the virtual address is required.
The 'encl_owner' field is useless in virtualization case, then
repurpose it as 'vepc_vaddr' - the virtual address of the virtual EPC
page for such case so that arch_memory_failure() can easily retrieve it.

Introduce a union to prevent adding a new dedicated structure to
track the virtual address of virtual EPC page.  And it can also prevent
playing the casting games while using it.

Add a new EPC page flag - SGX_EPC_PAGE_KVM_GUEST to interpret the
meaning of the field.

Co-developed-by: Cathy Zhang <[e-mail]>
Signed-off-by: Cathy Zhang <[e-mail]>
Signed-off-by: Zhiquan Li <[e-mail]>
Acked-by: Kai Huang <[e-mail]>
Reviewed-by: Jarkko Sakkinen <[e-mail]>
Acked-by: Jarkko Sakkinen <[e-mail]>

---
Changes since V7:
- Add Acked-by from Jarkko.

No changes since V6.

Changes since V5:
- To prevent casting the 'encl_owner' field, introduce a union with
  another field - 'vepc_vaddr', sugguested by Dave Hansen.
- Add Reviewed-by from Jarkko.
  Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#m379d00fc7f1d43726a42b3884637532061a8c0d1

Changes since V4:
- Add Co-developed-by and Signed-off-by from Cathy Zhang, as she had
  fully discussed the flag name with Jarkko.
  Link: https://lore.kernel.org/all/[e-mail]/
- Add Acked-by from Kai Huang
  Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#mccfb11df30698dbd060f2b6f06383cda7f154ef3

Changes since V3:
- Take the definition of EPC page flag SGX_EPC_PAGE_KVM_GUEST from
  Cathy Zhang's third patch of SGX rebootless recovery patch set but
  discard irrelevant portion, since it might need some time to
  re-forge and these are two different features.
  Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#m9782d23496cacecb7da07a67daa79f4b322ae170

Changes since V2:
- Remove struct sgx_vepc_page and relevant code.
- Rework the patch suggested by Jarkko.
- Remove new EPC page flag SGX_EPC_PAGE_IS_VEPC definition as it is
  duplicated to SGX_EPC_PAGE_KVM_GUEST.
  Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#u

Changes since V1:
- Add documentation suggested by Jarkko.
---
 arch/x86/kernel/cpu/sgx/main.c | 4 ++++
 arch/x86/kernel/cpu/sgx/sgx.h  | 8 +++++++-
 arch/x86/kernel/cpu/sgx/virt.c | 4 +++-
 3 files changed, 14 insertions(+), 2 deletions(-)

Comments

Huang, Kai Sept. 13, 2022, 4:40 p.m. UTC | #1

On Tue, 2022-09-13 at 22:53 +0800, Zhiquan Li wrote:
> Reviewed-by: Jarkko Sakkinen <[e-mail]>
> Acked-by: Jarkko Sakkinen <[e-mail]>

Nit: you can delete someone's Acked-by if you already have the Reviewed-by.

(The same to your patch 3).

Zhiquan Li Sept. 14, 2022, 12:29 a.m. UTC | #2

On 2022/9/14 00:40, Huang, Kai wrote:
> On Tue, 2022-09-13 at 22:53 +0800, Zhiquan Li wrote:
>> Reviewed-by: Jarkko Sakkinen <[e-mail]>
>> Acked-by: Jarkko Sakkinen <[e-mail]>
> Nit: you can delete someone's Acked-by if you already have the Reviewed-by.
> 
> (The same to your patch 3).

Thanks, good to know that ;-)

> 
> -- Thanks, -Kai

Haitao Huang Sept. 14, 2022, 3:46 p.m. UTC | #3

Hi Zhiquan

On Tue, 13 Sep 2022 09:53:29 -0500, Zhiquan Li <[e-mail]>  
wrote:

> When a page triggers a machine check, it only reports the PFN.  But in
> order to inject #MC into hypervisor, the virtual address is required.
> The 'encl_owner' field is useless in virtualization case, then
> repurpose it as 'vepc_vaddr' - the virtual address of the virtual EPC
> page for such case so that arch_memory_failure() can easily retrieve it.
>
> Introduce a union to prevent adding a new dedicated structure to
> track the virtual address of virtual EPC page.  And it can also prevent
> playing the casting games while using it.
>
> Add a new EPC page flag - SGX_EPC_PAGE_KVM_GUEST to interpret the
> meaning of the field.
>
> Co-developed-by: Cathy Zhang <[e-mail]>
> Signed-off-by: Cathy Zhang <[e-mail]>
> Signed-off-by: Zhiquan Li <[e-mail]>
> Acked-by: Kai Huang <[e-mail]>
> Reviewed-by: Jarkko Sakkinen <[e-mail]>
> Acked-by: Jarkko Sakkinen <[e-mail]>
>
> ---
> Changes since V7:
> - Add Acked-by from Jarkko.
>
> No changes since V6.
>
> Changes since V5:
> - To prevent casting the 'encl_owner' field, introduce a union with
>   another field - 'vepc_vaddr', sugguested by Dave Hansen.
> - Add Reviewed-by from Jarkko.
>   Link:  
> https://lore.kernel.org/linux-sgx/[e-mail]/T/#m379d00fc7f1d43726a42b3884637532061a8c0d1
>
> Changes since V4:
> - Add Co-developed-by and Signed-off-by from Cathy Zhang, as she had
>   fully discussed the flag name with Jarkko.
>   Link:  
> https://lore.kernel.org/all/[e-mail]/
> - Add Acked-by from Kai Huang
>   Link:  
> https://lore.kernel.org/linux-sgx/[e-mail]/T/#mccfb11df30698dbd060f2b6f06383cda7f154ef3
>
> Changes since V3:
> - Take the definition of EPC page flag SGX_EPC_PAGE_KVM_GUEST from
>   Cathy Zhang's third patch of SGX rebootless recovery patch set but
>   discard irrelevant portion, since it might need some time to
>   re-forge and these are two different features.
>   Link:  
> https://lore.kernel.org/linux-sgx/[e-mail]/T/#m9782d23496cacecb7da07a67daa79f4b322ae170
>
> Changes since V2:
> - Remove struct sgx_vepc_page and relevant code.
> - Rework the patch suggested by Jarkko.
> - Remove new EPC page flag SGX_EPC_PAGE_IS_VEPC definition as it is
>   duplicated to SGX_EPC_PAGE_KVM_GUEST.
>   Link:  
> https://lore.kernel.org/linux-sgx/[e-mail]/T/#u
>
> Changes since V1:
> - Add documentation suggested by Jarkko.
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 4 ++++
>  arch/x86/kernel/cpu/sgx/sgx.h  | 8 +++++++-
>  arch/x86/kernel/cpu/sgx/virt.c | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/sgx/main.c  
> b/arch/x86/kernel/cpu/sgx/main.c
> index 1315c69a733e..b319bedcaf1e 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -549,6 +549,10 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page  
> *page)
>   * Finally, wake up ksgxd when the number of pages goes below the  
> watermark
>   * before returning back to the caller.
>   *
> + * When an EPC page is assigned to KVM guest, repurpose the  
> 'encl_owner' field
> + * as the virtual address of virtual EPC page, since it is useless in  
> such
> + * scenario, so 'owner' is assigned to 'vepc_vaddr'.
> + *
>   * Return:
>   *   an EPC page,
>   *   -errno on error
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h  
> b/arch/x86/kernel/cpu/sgx/sgx.h
> index 4d88abccd12e..d16a8baa28d4 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -28,12 +28,18 @@
> /* Pages on free list */
>  #define SGX_EPC_PAGE_IS_FREE		BIT(1)
> +/* Pages allocated for KVM guest */
> +#define SGX_EPC_PAGE_KVM_GUEST		BIT(2)
> struct sgx_epc_page {
>  	unsigned int section;
>  	u16 flags;
>  	u16 poison;
> -	struct sgx_encl_page *encl_owner;
> +	union {
> +		struct sgx_encl_page *encl_owner;
> +		/* Use when SGX_EPC_PAGE_KVM_GUEST set in ->flags: */
> +		void __user *vepc_vaddr;
> +	};

Maybe it's just me missing some prior knowledge. It's not obvious to me  
why you don't need any guard accessing the encl_owner field in ksgxd  
thread. Is it because all vepc pages are never put in the active list and  
encl_owner would never be null for all pages in that list?
Regardless, could you add a few sentence here to to make the rule explicit?

Thanks
Haitao

Zhiquan Li Sept. 15, 2022, 2:22 a.m. UTC | #4

On 2022/9/14 23:46, Haitao Huang wrote:
> Maybe it's just me missing some prior knowledge. It's not obvious to me
> why you don't need any guard accessing the encl_owner field in ksgxd
> thread. Is it because all vepc pages are never put in the active list
> and encl_owner would never be null for all pages in that list?

Yes, all vepc pages are never put in the active list. The SGX core page
reclaimer doesn’t support reclaiming EPC pages allocated to KVM guests
through the virtual EPC driver.

> Regardless, could you add a few sentence here to to make the rule explicit?

More details please see the section "Virtual EPC" at
Documentation/x86/sgx.rst.

Thanks and Best Regards,
Zhiquan

> 
> Thanks
> Haitao

Haitao Huang Sept. 15, 2022, 8 p.m. UTC | #5

On Wed, 14 Sep 2022 21:22:33 -0500, Zhiquan Li <[e-mail]>  
wrote:

>
> On 2022/9/14 23:46, Haitao Huang wrote:
>> Maybe it's just me missing some prior knowledge. It's not obvious to me
>> why you don't need any guard accessing the encl_owner field in ksgxd
>> thread. Is it because all vepc pages are never put in the active list
>> and encl_owner would never be null for all pages in that list?
>
> Yes, all vepc pages are never put in the active list. The SGX core page
> reclaimer doesn’t support reclaiming EPC pages allocated to KVM guests
> through the virtual EPC driver.
>
>> Regardless, could you add a few sentence here to to make the rule  
>> explicit?
>
> More details please see the section "Virtual EPC" at
> Documentation/x86/sgx.rst.
>
Thanks for pointing to that.
Haitao

Jarkko Sakkinen Sept. 20, 2022, 4:51 a.m. UTC | #6

On Tue, Sep 13, 2022 at 10:53:29PM +0800, Zhiquan Li wrote:
> When a page triggers a machine check, it only reports the PFN.  But in
> order to inject #MC into hypervisor, the virtual address is required.
> The 'encl_owner' field is useless in virtualization case, then
> repurpose it as 'vepc_vaddr' - the virtual address of the virtual EPC
> page for such case so that arch_memory_failure() can easily retrieve it.
> 
> Introduce a union to prevent adding a new dedicated structure to
> track the virtual address of virtual EPC page.  And it can also prevent
> playing the casting games while using it.
> 
> Add a new EPC page flag - SGX_EPC_PAGE_KVM_GUEST to interpret the
> meaning of the field.
> 
> Co-developed-by: Cathy Zhang <[e-mail]>
> Signed-off-by: Cathy Zhang <[e-mail]>
> Signed-off-by: Zhiquan Li <[e-mail]>
> Acked-by: Kai Huang <[e-mail]>
> Reviewed-by: Jarkko Sakkinen <[e-mail]>
> Acked-by: Jarkko Sakkinen <[e-mail]>

Remove acked-by

> 
> ---
> Changes since V7:
> - Add Acked-by from Jarkko.
> 
> No changes since V6.
> 
> Changes since V5:
> - To prevent casting the 'encl_owner' field, introduce a union with
>   another field - 'vepc_vaddr', sugguested by Dave Hansen.
> - Add Reviewed-by from Jarkko.
>   Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#m379d00fc7f1d43726a42b3884637532061a8c0d1
> 
> Changes since V4:
> - Add Co-developed-by and Signed-off-by from Cathy Zhang, as she had
>   fully discussed the flag name with Jarkko.
>   Link: https://lore.kernel.org/all/[e-mail]/
> - Add Acked-by from Kai Huang
>   Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#mccfb11df30698dbd060f2b6f06383cda7f154ef3
> 
> Changes since V3:
> - Take the definition of EPC page flag SGX_EPC_PAGE_KVM_GUEST from
>   Cathy Zhang's third patch of SGX rebootless recovery patch set but
>   discard irrelevant portion, since it might need some time to
>   re-forge and these are two different features.
>   Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#m9782d23496cacecb7da07a67daa79f4b322ae170
> 
> Changes since V2:
> - Remove struct sgx_vepc_page and relevant code.
> - Rework the patch suggested by Jarkko.
> - Remove new EPC page flag SGX_EPC_PAGE_IS_VEPC definition as it is
>   duplicated to SGX_EPC_PAGE_KVM_GUEST.
>   Link: https://lore.kernel.org/linux-sgx/[e-mail]/T/#u
> 
> Changes since V1:
> - Add documentation suggested by Jarkko.
> ---
>  arch/x86/kernel/cpu/sgx/main.c | 4 ++++
>  arch/x86/kernel/cpu/sgx/sgx.h  | 8 +++++++-
>  arch/x86/kernel/cpu/sgx/virt.c | 4 +++-
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
> index 1315c69a733e..b319bedcaf1e 100644
> --- a/arch/x86/kernel/cpu/sgx/main.c
> +++ b/arch/x86/kernel/cpu/sgx/main.c
> @@ -549,6 +549,10 @@ int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
>   * Finally, wake up ksgxd when the number of pages goes below the watermark
>   * before returning back to the caller.
>   *
> + * When an EPC page is assigned to KVM guest, repurpose the 'encl_owner' field
> + * as the virtual address of virtual EPC page, since it is useless in such
> + * scenario, so 'owner' is assigned to 'vepc_vaddr'.
> + *
>   * Return:
>   *   an EPC page,
>   *   -errno on error
> diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
> index 4d88abccd12e..d16a8baa28d4 100644
> --- a/arch/x86/kernel/cpu/sgx/sgx.h
> +++ b/arch/x86/kernel/cpu/sgx/sgx.h
> @@ -28,12 +28,18 @@
>  
>  /* Pages on free list */
>  #define SGX_EPC_PAGE_IS_FREE		BIT(1)
> +/* Pages allocated for KVM guest */
> +#define SGX_EPC_PAGE_KVM_GUEST		BIT(2)
>  
>  struct sgx_epc_page {
>  	unsigned int section;
>  	u16 flags;
>  	u16 poison;
> -	struct sgx_encl_page *encl_owner;
> +	union {
> +		struct sgx_encl_page *encl_owner;
> +		/* Use when SGX_EPC_PAGE_KVM_GUEST set in ->flags: */
> +		void __user *vepc_vaddr;
> +	};
>  	struct list_head list;
>  };
>  
> diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
> index 6a77a14eee38..776ae5c1c032 100644
> --- a/arch/x86/kernel/cpu/sgx/virt.c
> +++ b/arch/x86/kernel/cpu/sgx/virt.c
> @@ -46,10 +46,12 @@ static int __sgx_vepc_fault(struct sgx_vepc *vepc,
>  	if (epc_page)
>  		return 0;
>  
> -	epc_page = sgx_alloc_epc_page(vepc, false);
> +	epc_page = sgx_alloc_epc_page((void *)addr, false);
>  	if (IS_ERR(epc_page))
>  		return PTR_ERR(epc_page);
>  
> +	epc_page->flags |= SGX_EPC_PAGE_KVM_GUEST;
> +
>  	ret = xa_err(xa_store(&vepc->page_array, index, epc_page, GFP_KERNEL));
>  	if (ret)
>  		goto err_free;
> -- 
> 2.25.1
> 

BR, Jarkko

Patch

diff --git a/arch/x86/kernel/cpu/sgx/main.c b/arch/x86/kernel/cpu/sgx/main.c
index 1315c69a733e..b319bedcaf1e 100644
--- a/arch/x86/kernel/cpu/sgx/main.c
+++ b/arch/x86/kernel/cpu/sgx/main.c
@@ -549,6 +549,10 @@  int sgx_unmark_page_reclaimable(struct sgx_epc_page *page)
  * Finally, wake up ksgxd when the number of pages goes below the watermark
  * before returning back to the caller.
  *
+ * When an EPC page is assigned to KVM guest, repurpose the 'encl_owner' field
+ * as the virtual address of virtual EPC page, since it is useless in such
+ * scenario, so 'owner' is assigned to 'vepc_vaddr'.
+ *
  * Return:
  *   an EPC page,
  *   -errno on error
diff --git a/arch/x86/kernel/cpu/sgx/sgx.h b/arch/x86/kernel/cpu/sgx/sgx.h
index 4d88abccd12e..d16a8baa28d4 100644
--- a/arch/x86/kernel/cpu/sgx/sgx.h
+++ b/arch/x86/kernel/cpu/sgx/sgx.h
@@ -28,12 +28,18 @@ 
 
 /* Pages on free list */
 #define SGX_EPC_PAGE_IS_FREE		BIT(1)
+/* Pages allocated for KVM guest */
+#define SGX_EPC_PAGE_KVM_GUEST		BIT(2)
 
 struct sgx_epc_page {
 	unsigned int section;
 	u16 flags;
 	u16 poison;
-	struct sgx_encl_page *encl_owner;
+	union {
+		struct sgx_encl_page *encl_owner;
+		/* Use when SGX_EPC_PAGE_KVM_GUEST set in ->flags: */
+		void __user *vepc_vaddr;
+	};
 	struct list_head list;
 };
 
diff --git a/arch/x86/kernel/cpu/sgx/virt.c b/arch/x86/kernel/cpu/sgx/virt.c
index 6a77a14eee38..776ae5c1c032 100644
--- a/arch/x86/kernel/cpu/sgx/virt.c
+++ b/arch/x86/kernel/cpu/sgx/virt.c
@@ -46,10 +46,12 @@  static int __sgx_vepc_fault(struct sgx_vepc *vepc,
 	if (epc_page)
 		return 0;
 
-	epc_page = sgx_alloc_epc_page(vepc, false);
+	epc_page = sgx_alloc_epc_page((void *)addr, false);
 	if (IS_ERR(epc_page))
 		return PTR_ERR(epc_page);
 
+	epc_page->flags |= SGX_EPC_PAGE_KVM_GUEST;
+
 	ret = xa_err(xa_store(&vepc->page_array, index, epc_page, GFP_KERNEL));
 	if (ret)
 		goto err_free;