Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with memory encryption

[Borislav Petkov <bp@alien8.de>]

On Wed, Jun 07, 2017 at 02:17:45PM -0500, Tom Lendacky wrote:
> The IOMMU is programmed with physical addresses for the various tables
> and buffers that are used to communicate between the device and the
> driver. When the driver allocates this memory it is encrypted. In order
> for the IOMMU to access the memory as encrypted the encryption mask needs
> to be included in these physical addresses during configuration.
> 
> The PTE entries created by the IOMMU should also include the encryption
> mask so that when the device behind the IOMMU performs a DMA, the DMA
> will be performed to encrypted memory.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/include/asm/mem_encrypt.h |    7 +++++++
>  arch/x86/mm/mem_encrypt.c          |   30 ++++++++++++++++++++++++++++++
>  drivers/iommu/amd_iommu.c          |   36 +++++++++++++++++++-----------------
>  drivers/iommu/amd_iommu_init.c     |   18 ++++++++++++------
>  drivers/iommu/amd_iommu_proto.h    |   10 ++++++++++
>  drivers/iommu/amd_iommu_types.h    |    2 +-
>  include/asm-generic/mem_encrypt.h  |    5 +++++
>  7 files changed, 84 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/asm/mem_encrypt.h b/arch/x86/include/asm/mem_encrypt.h
> index c7a2525..d86e544 100644
> --- a/arch/x86/include/asm/mem_encrypt.h
> +++ b/arch/x86/include/asm/mem_encrypt.h
> @@ -31,6 +31,8 @@ void __init sme_early_decrypt(resource_size_t paddr,
>  
>  void __init sme_early_init(void);
>  
> +bool sme_iommu_supported(void);
> +
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void);
>  
> @@ -62,6 +64,11 @@ static inline void __init sme_early_init(void)
>  {
>  }
>  
> +static inline bool sme_iommu_supported(void)
> +{
> +	return true;
> +}

Some more file real-estate saving:

static inline bool sme_iommu_supported(void) 	{ return true; }

> +
>  #endif	/* CONFIG_AMD_MEM_ENCRYPT */
>  
>  static inline bool sme_active(void)
> diff --git a/arch/x86/mm/mem_encrypt.c b/arch/x86/mm/mem_encrypt.c
> index 5d7c51d..018b58a 100644
> --- a/arch/x86/mm/mem_encrypt.c
> +++ b/arch/x86/mm/mem_encrypt.c
> @@ -197,6 +197,36 @@ void __init sme_early_init(void)
>  		protection_map[i] = pgprot_encrypted(protection_map[i]);
>  }
>  
> +bool sme_iommu_supported(void)

Why is this one exported with all the header file declarations if it is
going to be used in the iommu code only? IOW, you can make it a static
function there and save yourself all the exporting.

> +{
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
> +
> +	if (!sme_me_mask || (c->x86 != 0x17))

me_mask or sme_active()?

Or is the IOMMU "disabled" in a way the moment the BIOS decides that SME
can be enabled?

Also, family checks are always a bad idea for enablement. Why do we need
the family check? Because future families will work with the IOMMU? :-)

> +		return true;
> +
> +	/* For Fam17h, a specific level of support is required */
> +	switch (c->microcode & 0xf000) {

Also, you said in another mail on this subthread that c->microcode
is not yet set. Are you saying, that the iommu init gunk runs before
init_amd(), where we do set c->microcode?

If so, we can move the setting to early_init_amd() or so.

> +	case 0x0000:
> +		return false;
> +	case 0x1000:
> +		switch (c->microcode & 0x0f00) {
> +		case 0x0000:
> +			return false;
> +		case 0x0100:
> +			if ((c->microcode & 0xff) < 0x26)
> +				return false;
> +			break;
> +		case 0x0200:
> +			if ((c->microcode & 0xff) < 0x05)
> +				return false;
> +			break;
> +		}

So this is the microcode revision, why those complex compares? Can't you
simply check a range of values?

> +		break;
> +	}
> +
> +	return true;
> +}
> +
>  /* Architecture __weak replacement functions */
>  void __init mem_encrypt_init(void)
>  {
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 63cacf5..94eb130 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -544,7 +544,7 @@ static void dump_dte_entry(u16 devid)
>  
>  static void dump_command(unsigned long phys_addr)
>  {
> -	struct iommu_cmd *cmd = phys_to_virt(phys_addr);
> +	struct iommu_cmd *cmd = iommu_phys_to_virt(phys_addr);
>  	int i;
>  
>  	for (i = 0; i < 4; ++i)
> @@ -863,13 +863,15 @@ static void copy_cmd_to_buffer(struct amd_iommu *iommu,
>  	writel(tail, iommu->mmio_base + MMIO_CMD_TAIL_OFFSET);
>  }
>  
> -static void build_completion_wait(struct iommu_cmd *cmd, u64 address)
> +static void build_completion_wait(struct iommu_cmd *cmd, volatile u64 *sem)

WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst
#134: FILE: drivers/iommu/amd_iommu.c:866:
+static void build_completion_wait(struct iommu_cmd *cmd, volatile u64 *sem)

>  {
> +	u64 address = iommu_virt_to_phys((void *)sem);
> +
>  	WARN_ON(address & 0x7ULL);
>  
>  	memset(cmd, 0, sizeof(*cmd));
> -	cmd->data[0] = lower_32_bits(__pa(address)) | CMD_COMPL_WAIT_STORE_MASK;
> -	cmd->data[1] = upper_32_bits(__pa(address));
> +	cmd->data[0] = lower_32_bits(address) | CMD_COMPL_WAIT_STORE_MASK;
> +	cmd->data[1] = upper_32_bits(address);
>  	cmd->data[2] = 1;
>  	CMD_SET_TYPE(cmd, CMD_COMPL_WAIT);

<... snip stuff which Joerg needs to review... >

> diff --git a/include/asm-generic/mem_encrypt.h b/include/asm-generic/mem_encrypt.h
> index fb02ff0..bbc49e1 100644
> --- a/include/asm-generic/mem_encrypt.h
> +++ b/include/asm-generic/mem_encrypt.h
> @@ -27,6 +27,11 @@ static inline u64 sme_dma_mask(void)
>  	return 0ULL;
>  }
>  
> +static inline bool sme_iommu_supported(void)
> +{
> +	return true;
> +}

Save some more file real-estate... you get the idea by now, I'm sure.

:-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.