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 >
