Re: [RFC PATCH v2 08/32] x86: Use PAGE_KERNEL protection for ioremap of memory page

[Tom Lendacky]

On 3/17/2017 9:32 AM, Tom Lendacky wrote:
> On 3/16/2017 3:04 PM, Tom Lendacky wrote:
> > On 3/7/2017 8:59 AM, Borislav Petkov wrote:
> > > On Thu, Mar 02, 2017 at 10:13:32AM -0500, Brijesh Singh wrote:
> > > > From: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > > >
> > > > In order for memory pages to be properly mapped when SEV is active, we
> > > > need to use the PAGE_KERNEL protection attribute as the base
> > > > protection.
> > > > This will insure that memory mapping of, e.g. ACPI tables, receives the
> > > > proper mapping attributes.
> > > >
> > > > Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> > > > ---
> > >
> > > > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > > > index c400ab5..481c999 100644
> > > > --- a/arch/x86/mm/ioremap.c
> > > > +++ b/arch/x86/mm/ioremap.c
> > > > @@ -151,7 +151,15 @@ static void __iomem
> > > > *__ioremap_caller(resource_size_t phys_addr,
> > > >                 pcm = new_pcm;
> > > >         }
> > > >
> > > > +       /*
> > > > +        * If the page being mapped is in memory and SEV is active then
> > > > +        * make sure the memory encryption attribute is enabled in the
> > > > +        * resulting mapping.
> > > > +        */
> > > >         prot = PAGE_KERNEL_IO;
> > > > +       if (sev_active() && page_is_mem(pfn))
> > >
> > > Hmm, a resource tree walk per ioremap call. This could get expensive for
> > > ioremap-heavy workloads.
> > >
> > > __ioremap_caller() gets called here during boot 55 times so not a whole
> > > lot but I wouldn't be surprised if there were some nasty use cases which
> > > ioremap a lot.
> > >
> > > ...
> > >
> > > > diff --git a/kernel/resource.c b/kernel/resource.c
> > > > index 9b5f044..db56ba3 100644
> > > > --- a/kernel/resource.c
> > > > +++ b/kernel/resource.c
> > > > @@ -518,6 +518,46 @@ int __weak page_is_ram(unsigned long pfn)
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(page_is_ram);
> > > >
> > > > +/*
> > > > + * This function returns true if the target memory is marked as
> > > > + * IORESOURCE_MEM and IORESOUCE_BUSY and described as other than
> > > > + * IORES_DESC_NONE (e.g. IORES_DESC_ACPI_TABLES).
> > > > + */
> > > > +static int walk_mem_range(unsigned long start_pfn, unsigned long
> > > > nr_pages)
> > > > +{
> > > > +    struct resource res;
> > > > +    unsigned long pfn, end_pfn;
> > > > +    u64 orig_end;
> > > > +    int ret = -1;
> > > > +
> > > > +    res.start = (u64) start_pfn << PAGE_SHIFT;
> > > > +    res.end = ((u64)(start_pfn + nr_pages) << PAGE_SHIFT) - 1;
> > > > +    res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> > > > +    orig_end = res.end;
> > > > +    while ((res.start < res.end) &&
> > > > +        (find_next_iomem_res(&res, IORES_DESC_NONE, true) >= 0)) {
> > > > +        pfn = (res.start + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > > > +        end_pfn = (res.end + 1) >> PAGE_SHIFT;
> > > > +        if (end_pfn > pfn)
> > > > +            ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
> > > > +        if (ret)
> > > > +            break;
> > > > +        res.start = res.end + 1;
> > > > +        res.end = orig_end;
> > > > +    }
> > > > +    return ret;
> > > > +}
> > >
> > > So the relevant difference between this one and walk_system_ram_range()
> > > is this:
> > >
> > > -            ret = (*func)(pfn, end_pfn - pfn, arg);
> > > +            ret = (res.desc != IORES_DESC_NONE) ? 1 : 0;
> > >
> > > so it seems to me you can have your own *func() pointer which does that
> > > IORES_DESC_NONE comparison. And then you can define your own workhorse
> > > __walk_memory_range() which gets called by both walk_mem_range() and
> > > walk_system_ram_range() instead of almost duplicating them.
> > >
> > > And looking at walk_system_ram_res(), that one looks similar too except
> > > the pfn computation. But AFAICT the pfn/end_pfn things are computed from
> > > res.start and res.end so it looks to me like all those three functions
> > > are crying for unification...
> >
> > I'll take a look at what it takes to consolidate these with a pre-patch.
> > Then I'll add the new support.
>
> It looks pretty straight forward to combine walk_iomem_res_desc() and
> walk_system_ram_res(). The walk_system_ram_range() function would fit
> easily into this, also, except for the fact that the callback function
> takes unsigned longs vs the u64s of the other functions.  Is it worth
> modifying all of the callers of walk_system_ram_range() (which are only
> about 8 locations) to change the callback functions to accept u64s in
> order to consolidate the walk_system_ram_range() function, too?

The more I dig, the more I find that the changes keep expanding. I'll
leave walk_system_ram_range() out of the consolidation for now.

Thanks,
Tom

> Thanks,
> Tom
>
> > Thanks,
> > Tom
> >
> > >