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

[Baoquan He]

On 06/21/18 at 08:12am, Tom Lendacky wrote:
> On 6/21/2018 3:39 AM, Baoquan He wrote:
> > 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@xxxxxxxxxx>
> >>>> ---
> >>>> 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."
> 
> Even simpler, there's no need to even check for is_kdump_kernel().  The
> __sme_clr() should always be done if the physical address is going to be
> used for some form of io or memory remapping.
> 
> So you could just change the existing:
> 
> 	old_devtb_phys = entry & PAGE_MASK;
> 
> to:
> 
> 	old_devtb_phys = __sme_clr(entry) & PAGE_MASK;

Agree, this is even better.

> 
> >>
> >> 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@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 
> _______________________________________________
> kexec mailing list
> kexec@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/kexec