Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1666111 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Borislav Petkov <bp@alien8.de> - 2017-06-14 19:50 +0200
    Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Borislav Petkov <bp@alien8.de> - 2017-06-15 11:50 +0200
      Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Borislav Petkov <bp@alien8.de> - 2017-06-15 17:40 +0200
        Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with memory encryption Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> - 2017-06-15 22:20 +0200
        Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Borislav Petkov <bp@alien8.de> - 2017-06-19 19:20 +0200
      Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Joerg Roedel <joro@8bytes.org> - 2017-06-21 18:40 +0200
        Re: [PATCH v6 26/34] iommu/amd: Allow the AMD IOMMU to work with  memory encryption Borislav Petkov <bp@alien8.de> - 2017-06-21 19:10 +0200

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

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.

[toc] | [next] | [standalone]

#1666607

On Wed, Jun 14, 2017 at 03:40:28PM -0500, Tom Lendacky wrote:
> I was trying to keep all the logic for it here in the SME related files
> rather than put it in the iommu code itself. But it is easy enough to
> move if you think it's worth it.

Yes please - the less needlessly global symbols, the better.

> > 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.
> 
> I'll look into that.

And I don't think c->microcode is not set by the time we init the iommu
because, AFAICT, we do the latter in pci_iommu_init() and that's a
rootfs_initcall() which happens later then the CPU init stuff.

> I'll look into simplifying the checks.

Something like this maybe?

	if (rev >= 0x1205)
		return true;

	if (rev <= 0x11ff && rev >= 0x1126)
		return true;

	return false;

> > 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)
> > 
> 
> The semaphore area is written to by the device so the use of volatile is
> appropriate in this case.

Do you mean this is like the last exception case in that document above:

"
  - Pointers to data structures in coherent memory which might be modified
    by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
    used by a network adapter, where that adapter changes pointers to
    indicate which descriptors have been processed, is an example of this
    type of situation."

?

If so, it did work fine until now, without the volatile. Why is it
needed now, all of a sudden?

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1666810

On Thu, Jun 15, 2017 at 09:59:45AM -0500, Tom Lendacky wrote:
> Actually the detection routine, amd_iommu_detect(), is part of the
> IOMMU_INIT_FINISH macro support which is called early through mm_init()
> from start_kernel() and that routine is called before init_amd().

Ah, we do that there too:

	for (p = __iommu_table; p < __iommu_table_end; p++) {

Can't say that that code with the special section and whatnot is
obvious. :-\

Oh, well, early_init_amd() then. That is called in
start_kernel->setup_arch->early_cpu_init and thus before mm_init().

> > If so, it did work fine until now, without the volatile. Why is it
> > needed now, all of a sudden?
> 
> If you run checkpatch against the whole amd_iommu.c file you'll see that

I'm, of course, not talking about the signature change: I'm *actually*
questioning the need to make this argument volatile, all of a sudden.

If there's a need, please explain why. It worked fine until now. If it
didn't, we would've seen it.

If it is a bug, then it needs a proper explanation, a *separate* patch
and so on. But not like now, a drive-by change in an IOMMU enablement
patch.

If it is wrong, then wait_on_sem() needs to be fixed too. AFAICT,
wait_on_sem() gets called in both cases with interrupts disabled, while
holding a lock so I'd like to pls know why, even in that case, does this
variable need to be volatile.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

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

On June 15, 2017 11:33:22 AM EDT, Borislav Petkov <bp@alien8.de> wrote:
>On Thu, Jun 15, 2017 at 09:59:45AM -0500, Tom Lendacky wrote:
>> Actually the detection routine, amd_iommu_detect(), is part of the
>> IOMMU_INIT_FINISH macro support which is called early through
>mm_init()
>> from start_kernel() and that routine is called before init_amd().
>
>Ah, we do that there too:
>
>	for (p = __iommu_table; p < __iommu_table_end; p++) {
>
>Can't say that that code with the special section and whatnot is
>obvious. :-\
>

Patches to make it more obvious would be always welcome!



Thanks!

[toc] | [prev] | [next] | [standalone]

#1669522

On Thu, Jun 15, 2017 at 11:33:41AM -0500, Tom Lendacky wrote:
> Changing the signature back reverts to the original way, so this can be
> looked at separate from this patchset then.

Right, the patch which added the volatile thing was this one:

  4bf5beef578e ("iommu/amd: Don't put completion-wait semaphore on stack")

and the commit message doesn't say why the thing needs to be volatile at
all.

Joerg?

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [next] | [standalone]

#1671775

On Thu, Jun 15, 2017 at 11:41:12AM +0200, Borislav Petkov wrote:
> On Wed, Jun 14, 2017 at 03:40:28PM -0500, Tom Lendacky wrote:
> > > 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)
> > > 
> > 
> > The semaphore area is written to by the device so the use of volatile is
> > appropriate in this case.
> 
> Do you mean this is like the last exception case in that document above:
> 
> "
>   - Pointers to data structures in coherent memory which might be modified
>     by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
>     used by a network adapter, where that adapter changes pointers to
>     indicate which descriptors have been processed, is an example of this
>     type of situation."
> 
> ?

So currently (without this patch) the build_completion_wait function
does not take a volatile parameter, only wait_on_sem() does.

Wait_on_sem() needs it because its purpose is to poll a memory location
which is changed by the iommu-hardware when its done with command
processing.

But the 'volatile' in build_completion_wait() looks unnecessary, because
the function does not poll the memory location. It only uses the
pointer, converts it to a physical address and writes it to the command
to be queued.


Regards,

	Joerg

[toc] | [prev] | [next] | [standalone]

#1671825

On Wed, Jun 21, 2017 at 05:37:22PM +0200, Joerg Roedel wrote:
> > Do you mean this is like the last exception case in that document above:
> > 
> > "
> >   - Pointers to data structures in coherent memory which might be modified
> >     by I/O devices can, sometimes, legitimately be volatile.  A ring buffer
> >     used by a network adapter, where that adapter changes pointers to
> >     indicate which descriptors have been processed, is an example of this
> >     type of situation."
> > 
> > ?
> 
> So currently (without this patch) the build_completion_wait function
> does not take a volatile parameter, only wait_on_sem() does.
> 
> Wait_on_sem() needs it because its purpose is to poll a memory location
> which is changed by the iommu-hardware when its done with command
> processing.

Right, the reason above - memory modifiable by an IO device. You could
add a comment there explaining the need for the volatile.

> But the 'volatile' in build_completion_wait() looks unnecessary, because
> the function does not poll the memory location. It only uses the
> pointer, converts it to a physical address and writes it to the command
> to be queued.

Ok.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web