Re: [PATCH 3/4 V3] Remap the device table of IOMMU in encrypted manner for kdump

[Baoquan He <bhe@redhat.com>]

On 06/21/18 at 01:42pm, lijiang wrote:
> 在 2018年06月21日 00:42, Tom Lendacky 写道:
> > On 6/16/2018 3:27 AM, Lianbo Jiang wrote:
> >> In kdump mode, it will copy the device table of IOMMU from the old
> >> device table, which is encrypted when SME is enabled in the first
> >> kernel. So we must remap it in encrypted manner in order to be
> >> automatically decrypted when we read.
> >>
> >> Signed-off-by: Lianbo Jiang <lijiang@redhat.com>
> >> ---
> >> Some changes:
> >> 1. add some comments
> >> 2. clean compile warning.
> >>
> >>  drivers/iommu/amd_iommu_init.c | 15 ++++++++++++++-
> >>  1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> >> index 904c575..a20af4c 100644
> >> --- a/drivers/iommu/amd_iommu_init.c
> >> +++ b/drivers/iommu/amd_iommu_init.c
> >> @@ -889,11 +889,24 @@ static bool copy_device_table(void)
> >>  	}
> >>  
> >>  	old_devtb_phys = entry & PAGE_MASK;
> >> +
> >> +	/*
> >> +	 *  When sme enable in the first kernel, old_devtb_phys includes the
> >> +	 *  memory encryption mask(sme_me_mask), we must remove the memory
> >> +	 *  encryption mask to obtain the true physical address in kdump mode.
> >> +	 */
> >> +	if (mem_encrypt_active() && is_kdump_kernel())
> >> +		old_devtb_phys = __sme_clr(old_devtb_phys);
> >> +
> > 
> > You can probably just use "if (is_kdump_kernel())" here, since memory
> > encryption is either on in both the first and second kernel or off in
> > both the first and second kernel.  At which point __sme_clr() will do
> > the proper thing.
> > 
> > Actually, this needs to be done no matter what.  When doing either the
> > ioremap_encrypted() or the memremap(), the physical address should not
> > include the encryption bit/mask.
> > 
> > Thanks,
> > Tom
> > 
> Thanks for your comments. If we don't remove the memory encryption mask, it will
> return false because the 'old_devtb_phys >= 0x100000000ULL' may become true.

Lianbo, you may not get what Tom suggested. Tom means no matter what it
is, encrypted or not in 1st kernel, we need get pure physicall address,
and using below code is always right for both cases.

	if (is_kdump_kernel())
		old_devtb_phys = __sme_clr(old_devtb_phys);

And this is simpler. You even can add one line of code comment to say
like "Physical address w/o encryption mask is needed here."
> 
> Lianbo
> >>  	if (old_devtb_phys >= 0x100000000ULL) {
> >>  		pr_err("The address of old device table is above 4G, not trustworthy!\n");
> >>  		return false;
> >>  	}
> >> -	old_devtb = memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);
> >> +	old_devtb = (mem_encrypt_active() && is_kdump_kernel())
> >> +		    ? (__force void *)ioremap_encrypted(old_devtb_phys,
> >> +							dev_table_size)
> >> +		    : memremap(old_devtb_phys, dev_table_size, MEMREMAP_WB);> +
> >>  	if (!old_devtb)
> >>  		return false;
> >>  
> >>
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec