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
