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
