Re: [PATCH v3 12/14] arm64: realm: Support nonsecure ITS emulation shared

[Catalin Marinas <catalin.marinas@arm.com>]

Hi Michael,

All good points below, thanks.

On Tue, Jun 18, 2024 at 04:04:17PM +0000, Michael Kelley wrote:
> As someone who has spent time in Linux code on the x86 side that
> manages memory shared between the guest and host, a GFP_DECRYPTED
> flag on the memory allocation calls seems very useful. Here are my
> general comments, some of which are probably obvious, but I thought
> I'd write them down anyway.
> 
> 1)  The impetus in this case is to efficiently allow sub-page allocations.
> But on x86, there's also an issue in that the x86 direct map uses large
> page mappings, which must be split if any 4K page in the large page
> is decrypted. Ideally, we could cluster such decrypted pages into the
> same large page(s) vs. just grabbing any 4K page, and reduce the
> number of splits. I haven't looked at how feasible that is to implement
> in the context of the existing allocator code, so this is aspirational.

It might be doable, though it's a lot more intrusive than just a GFP
flag. However, such memory cannot be used for any other things than
sharing data with the host.

> 2) GFP_DECRYPTED should work for the three memory allocation
> interfaces and their variants: alloc_pages(), kmalloc(), and vmalloc().

Yes. There are two main aspects: (1) the page allocation + linear map
change and (2) additional mapping attributes like in the vmalloc() case.

> 3) The current paradigm is to allocate memory and call
> set_memory_decrypted(). Then do the reverse when freeing memory.
> Using GFP_DECRYPTED on the allocation, and having the re-encryption
> done automatically when freeing certainly simplifies the call sites.
> The error handling at the call sites is additional messiness that gets
> much simpler.
> 
> 4) GFP_DECRYPTED should be silently ignored when not running
> in a CoCo VM. Drivers that need decrypted memory in a CoCo VM
> always set this flag, and work normally as if the flag isn't set when
> not in a CoCo VM. This is how set_memory_decrypted()/encrypted()
> work today. Drivers call them in all cases, and they are no-ops
> when not in a CoCo VM.

Yes, this should be straightforward. Maybe simplified further to avoid
creating additional kmalloc() caches if these functions are not
supported, though we seem to be missing some generic
arch_has_mem_encrypt() API.

> 5) The implementation of GFP_DECRYPTED must correctly handle
> errors from set_memory_decrypted()/encrypted() as I've described
> separately on this thread.

Good point, I completely missed this aspect.

I'll prepare some patches and post them next week to get feedback from
the mm folk.

Thanks.

-- 
Catalin