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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 6/15/2017 4:41 AM, Borislav Petkov wrote:
> 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.

Ok.

> 
>>> 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.

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().

> 
>> 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;

Yup, something like that.

> 
>>> 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?

If you run checkpatch against the whole amd_iommu.c file you'll see that
same warning for the wait_on_sem() function.  The checkpatch warning
shows up now because I modified the build_completion_wait() function as
part of the support to use iommu_virt_to_phys().

Since I'm casting the arg to iommu_virt_to_phys() no matter what I can
avoid the signature change to the build_completion_wait() function and
avoid this confusion in the future.

Thanks,
Tom

>