[PATCH] irqchip/gic-v3-its: Mark some in-memory data structures as 'decrypted'

[Will Deacon]

Hi Ard,

On Wed, Dec 08, 2021 at 05:56:59PM +0100, Ard Biesheuvel wrote:
> On Wed, 8 Dec 2021 at 17:00, Will Deacon <will at kernel.org> wrote:
> >
> > The GICv3 ITS driver allocates memory for its tables using alloc_pages()
> > and performs explicit cache maintenance if necessary. On systems such
> > as those running pKVM, where the memory encryption API is implemented,
> > memory shared with the ITS must first be transitioned to the "decrypted"
> > state, as it would be if allocated via the DMA API.
> >
> > Allow pKVM guests to interact with an ITS emulation by ensuring that the
> > shared pages are decrypted at the point of allocation and encrypted
> > again upon free().
> >
> > Cc: Thomas Gleixner <tglx at linutronix.de>
> > Cc: Marc Zyngier <maz at kernel.org>
> > Signed-off-by: Will Deacon <will at kernel.org>
> > ---
> >
> > Although pKVM doesn't yet expose share/unshare hypercalls to the guest,
> > this change is agnostic of the hypervisor and could be queued
> > independently as it has no functional impact when the memory encryption
> > API is not implemented.
> >
> >  drivers/irqchip/irq-gic-v3-its.c | 40 ++++++++++++++++++++++++++------
> >  1 file changed, 33 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index eb0882d15366..4559b8dfb9bc 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/of_pci.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/percpu.h>
> > +#include <linux/set_memory.h>
> >  #include <linux/slab.h>
> >  #include <linux/syscore_ops.h>
> >
> > @@ -2166,6 +2167,7 @@ static void gic_reset_prop_table(void *va)
> >
> >         /* Make sure the GIC will observe the written configuration */
> >         gic_flush_dcache_to_poc(va, LPI_PROPBASE_SZ);
> > +       set_memory_decrypted((unsigned long)va, LPI_PROPBASE_SZ >> PAGE_SHIFT);
> >  }
> >
> >  static struct page *its_allocate_prop_table(gfp_t gfp_flags)
> > @@ -2183,8 +2185,10 @@ static struct page *its_allocate_prop_table(gfp_t gfp_flags)
> >
> >  static void its_free_prop_table(struct page *prop_page)
> >  {
> > -       free_pages((unsigned long)page_address(prop_page),
> > -                  get_order(LPI_PROPBASE_SZ));
> > +       unsigned long va = (unsigned long)page_address(prop_page);
> > +
> > +       set_memory_encrypted(va, LPI_PROPBASE_SZ >> PAGE_SHIFT);
> 
> I take it this call is there to return the freed pages to the state
> they are assumed to have been in when they were allocated?
> 
> This seems like a maintenance nightmare in the making, to be honest.
> This driver should be able to stop caring about the
> encrypted/decrypted state of the pages as soon as it frees them, and
> on systems that actually implement the difference, I would expect the
> memory management layer to implement a reasonable default that always
> applies to newly allocated pages, regardless of how they may have been
> used in the past.

This currently happens above the mm layer and is instead handled by the DMA
layer which may use the page allocator, swiotlb, CMA etc under the hood and
ensures that whatever memory is allocated ends up being in the decrypted
state while allocated if needed by the device. If we could use the normal
DMA APIs here then we wouldn't have to worry about the *crypted nature of
the memory at all, but since we're forced to roll our own routines then
we're exposed to the horror of it all.

It's a similar story to the cache maintenance, which we also end up having
to open-code here.

Will