Re: [PATCH 2/3] x86, hibernate: Extract the common code of 64/32 bit system

[Yu Chen <yu.c.chen@intel.com>]

On Thu, Aug 30, 2018 at 02:59:09PM +0200, Thomas Gleixner wrote:
> On Mon, 27 Aug 2018, Gu Zhimin wrote:
> > diff --git a/arch/x86/power/hibernate.c b/arch/x86/power/hibernate.c
> > new file mode 100644
> > index 0000000..6f91f7b
> > --- /dev/null
> > +++ b/arch/x86/power/hibernate.c
> > @@ -0,0 +1,255 @@
> > +/*
> > + * Hibernation support for x86
> > + *
> > + * Distribute under GPLv2
> 
> We have SPDX identifiers for that and not some randomly chosen license
> hint.
>
> > +
> > +/*
> > + *	pfn_is_nosave - check if given pfn is in the 'nosave' section
> 
> This is a half baken kernel doc header. 
> 
> > + */
> > +
> 
> Random new line.
> 
> > +int pfn_is_nosave(unsigned long pfn)
> > +{
> > +	unsigned long nosave_begin_pfn = __pa_symbol(&__nosave_begin) >> PAGE_SHIFT;
> > +	unsigned long nosave_end_pfn = PAGE_ALIGN(__pa_symbol(&__nosave_end)) >> PAGE_SHIFT;
> 
> Instead of blindly copying stuff please fix it so it matches kernel coding
> rules.
> 
> > +	return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
> 
> The brackets are pointless
> 
> > +}
> > +
> > +#ifdef CONFIG_X86_64
> > +static int relocate_restore_code(void)
> > +{
> > +	pgd_t *pgd;
> > +	p4d_t *p4d;
> > +	pud_t *pud;
> > +	pmd_t *pmd;
> > +	pte_t *pte;
> > +
> > +	relocated_restore_code = get_safe_page(GFP_ATOMIC);
> > +	if (!relocated_restore_code)
> > +		return -ENOMEM;
> > +
> > +	memcpy((void *)relocated_restore_code, core_restore_code, PAGE_SIZE);
> > +
> > +	/* Make the page containing the relocated code executable */
> > +	pgd = (pgd_t *)__va(read_cr3_pa()) +
> > +		pgd_index(relocated_restore_code);
> > +	p4d = p4d_offset(pgd, relocated_restore_code);
> > +	if (p4d_large(*p4d)) {
> > +		set_p4d(p4d, __p4d(p4d_val(*p4d) & ~_PAGE_NX));
> > +		goto out;
> > +	}
> > +	pud = pud_offset(p4d, relocated_restore_code);
> > +	if (pud_large(*pud)) {
> > +		set_pud(pud, __pud(pud_val(*pud) & ~_PAGE_NX));
> > +		goto out;
> > +	}
> > +	pmd = pmd_offset(pud, relocated_restore_code);
> > +	if (pmd_large(*pmd)) {
> > +		set_pmd(pmd, __pmd(pmd_val(*pmd) & ~_PAGE_NX));
> > +		goto out;
> > +	}
> > +	pte = pte_offset_kernel(pmd, relocated_restore_code);
> > +	set_pte(pte, __pte(pte_val(*pte) & ~_PAGE_NX));
> > +out:
> > +	__flush_tlb_all();
> > +	return 0;
> > +}
> > +
> > +#define MD5_DIGEST_SIZE 16
> > +
> > +struct restore_data_record {
> > +	unsigned long jump_address;
> > +	unsigned long jump_address_phys;
> > +	unsigned long cr3;
> > +	unsigned long magic;
> > +	u8 e820_digest[MD5_DIGEST_SIZE];
> > +};
> > +
> > +#if IS_BUILTIN(CONFIG_CRYPTO_MD5)
> > +/**
> > + * get_e820_md5 - calculate md5 according to given e820 table
> > + *
> > + * @table: the e820 table to be calculated
> > + * @buf: the md5 result to be stored to
> > + */
> > +static int get_e820_md5(struct e820_table *table, void *buf)
> > +{
> > +	struct crypto_shash *tfm;
> > +	struct shash_desc *desc;
> > +	int size;
> > +	int ret = 0;
> > +
> > +	tfm = crypto_alloc_shash("md5", 0, 0);
> > +	if (IS_ERR(tfm))
> > +		return -ENOMEM;
> > +
> > +	desc = kmalloc(sizeof(struct shash_desc) + crypto_shash_descsize(tfm),
> > +		       GFP_KERNEL);
> > +	if (!desc) {
> > +		ret = -ENOMEM;
> > +		goto free_tfm;
> > +	}
> > +
> > +	desc->tfm = tfm;
> > +	desc->flags = 0;
> > +
> > +	size = offsetof(struct e820_table, entries) +
> > +		sizeof(struct e820_entry) * table->nr_entries;
> > +
> > +	if (crypto_shash_digest(desc, (u8 *)table, size, buf))
> > +		ret = -EINVAL;
> > +
> > +	kzfree(desc);
> > +
> > +free_tfm:
> > +	crypto_free_shash(tfm);
> > +	return ret;
> > +}
> > +
> > +static void hibernation_e820_save(void *buf)
> > +{
> > +	get_e820_md5(e820_table_firmware, buf);
> 
> So if get_e820_md5() fails, then it will hibernate nevertheless. Why is
> that error code not propagated?
>
Thomas, thanks for reviewing this, we will fix them one by one.
Best,
Yu

> Thanks,
> 
> 	tglx