Re: [PATCH v8 4/5] x86/mm: Add noalias variants of set_memory_*crypted() functions

[Sathyanarayanan Kuppuswamy]

On 7/19/22 9:13 AM, Kirill A. Shutemov wrote:
> On Mon, Jul 18, 2022 at 07:22:52AM -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi Dave,
>>
>> On 7/5/22 8:29 AM, Kirill A. Shutemov wrote:
>>>>> I still don't like the idea of using the DMA API itself.  But, maybe we
>>>>> need some common infrastructure that the DMA API and this code use which
>>>>> says, "get me some pages that I can safely make shared".
>>>> Right.  For instance any KVM PV feature would require shared memory, and DMA API
>>>> normally doesn't fit (taking 'struct kvm_steal_time' as example).
>>>>
>>>> Maybe we can reserve a CMA for this purpose.
>>> CMA for couple low traffic users sounds like an overkill. It will create
>>> an separate pool just for them.
>>>
>>> I think the best way is to add an dummy device and couple of helpers
>>> around DMA API that would allow to tap into swiotlb.
>>>
>>> Maybe hide it inside CC infrastructure. Like cc_decrypted_alloc() and
>>> cc_decrypted_free().
>>
>> I also think creating a generic device in the CC layer, and using it to allocate
>> memory via DMA APIs is a better approach for this issue. Following is the sample
>> implementation to give you an idea on how it looks. Please let me know
>> your comments.
>>
>> cc_shmem_alloc/free() APIs can be used by CC users to allocate and
>> free shared memory.
> 
> We usually use 'decrypted' term in kernel. cc_decrypted_alloc()/_free().

Fine with me. I will use it.

> 
>> Other vendors can define their own shared memory allocation and free
>> logic via struct shmem_priv alloc/free/init hooks.
>>
>> --- a/arch/x86/coco/Makefile
>> +++ b/arch/x86/coco/Makefile
>> @@ -3,6 +3,6 @@ CFLAGS_REMOVE_core.o    = -pg
>>  KASAN_SANITIZE_core.o  := n
>>  CFLAGS_core.o          += -fno-stack-protector
>>  
>> -obj-y += core.o
>> +obj-y += core.o shmem.o
> 
> Rename shmem.o -> mem.o ?

Ok.

> 
>> diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
>> index 49b44f881484..62fe68d1f60a 100644
>> --- a/arch/x86/coco/core.c
>> +++ b/arch/x86/coco/core.c
>> @@ -13,7 +13,7 @@
>>  #include <asm/coco.h>
>>  #include <asm/processor.h>
>>  
>> -static enum cc_vendor vendor __ro_after_init;
>> +enum cc_vendor vendor __ro_after_init;
>>  static u64 cc_mask __ro_after_init;
>>  
>>  static bool intel_cc_platform_has(enum cc_attr attr)
>> @@ -23,6 +23,7 @@ static bool intel_cc_platform_has(enum cc_attr attr)
>>         case CC_ATTR_HOTPLUG_DISABLED:
>>         case CC_ATTR_GUEST_MEM_ENCRYPT:
>>         case CC_ATTR_MEM_ENCRYPT:
>> +       case CC_ATTR_SHMEM:
>>                 return true;
>>         default:
>>                 return false;
> 
> I don't think we need a new attribute. CC_ATTR_MEM_ENCRYPT has to be
> enough.

Ok. Make sense.

> 
>> @@ -134,6 +135,11 @@ __init void cc_set_vendor(enum cc_vendor v)
>>         vendor = v;
>>  }
>>  
>> +enum cc_vendor cc_get_vendor(void)
>> +{
>> +       return vendor;
>> +}
>> +
>>  __init void cc_set_mask(u64 mask)
>>  {
>>         cc_mask = mask;
>>
>> +++ b/arch/x86/coco/shmem.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Confidential Computing Shared Memory Allocator
>> + *
>> + * Copyright (C) 2022 Intel Corporation, Inc.
>> + *
>> + */
>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt)     "CC: " fmt
>> +
>> +#include <linux/export.h>
>> +#include <linux/cma.h>
>> +#include <linux/mm.h>
>> +#include <linux/cc_platform.h>
>> +#include <linux/set_memory.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dma-mapping.h>
>> +
>> +#include <asm/coco.h>
>> +#include <asm/processor.h>
>> +
>> +#define CC_SHMEM_DRIVER                "cc-shmem"
>> +
>> +struct platform_device *shmem_pdev;
>> +
>> +struct shmem_priv
>> +{
>> +       int (*init)(struct platform_device *dev);
>> +       void* (*alloc)(size_t size, gfp_t gfp, struct shmem_priv *priv);
>> +       void (*free)(void *addr, size_t size, struct shmem_priv *priv);
>> +       struct platform_device *pdev;
>> +       void *data;
>> +};
>> +
>> +void *cc_shmem_alloc(size_t size, gfp_t gfp)
>> +{
>> +       struct shmem_priv *priv;
>> +
>> +       if (!shmem_pdev)
>> +               return NULL;
>> +
>> +       priv = platform_get_drvdata(shmem_pdev);
>> +
>> +       return priv->alloc(size, gfp, priv);
>> +}
>> +
>> +void cc_shmem_free(void *addr, size_t size)
>> +{
>> +       struct shmem_priv *priv;
>> +
>> +       if (!shmem_pdev)
>> +               return;
>> +
>> +       priv = platform_get_drvdata(shmem_pdev);
>> +
>> +       priv->free(addr, size, priv);
>> +}
>> +
>> +static int intel_shmem_init(struct platform_device *pdev)
>> +{
>> +       struct shmem_priv *priv;
>> +       dma_addr_t *handle;
>> +
>> +       handle = devm_kmalloc(&pdev->dev, sizeof(*handle), GFP_KERNEL);
>> +       if (!handle)
>> +               return -ENOMEM;
>> +
>> +       priv = platform_get_drvdata(pdev);
>> +
>> +       priv->data = handle;
>> +
>> +       return dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
>> +}
>> +
>> +static void *intel_shmem_alloc(size_t size, gfp_t gfp, struct shmem_priv *priv)
>> +{
>> +       dma_addr_t *handle = (dma_addr_t *) priv->data;
>> +
>> +       return dma_alloc_coherent(&priv->pdev->dev, size, handle, gfp);
>> +}
>> +
>> +static void intel_shmem_free(void *addr, size_t size, struct shmem_priv *priv)
>> +{
>> +       dma_addr_t *handle = (dma_addr_t *) priv->data;
>> +
>> +       return dma_free_coherent(&priv->pdev->dev, size, addr, *handle);
>> +}
>> +
>> +static struct shmem_priv intel_shmem = {
>> +       .init  = intel_shmem_init,
>> +       .alloc = intel_shmem_alloc,
>> +       .free  = intel_shmem_free,
>> +};
> 
> Hm. What is Intel-specific here. Looks like a generic thing, no?
> 
> Maybe just drop all vendor stuff. CC_ATTR_MEM_ENCRYPT should be enough.

I thought that not all CC vendors would want to use DMA APIs for shared
buffer allocation. So adding a vendor layer would give them a way to implement
their own model.

> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer