On Thu, Jul 23, 2026 at 11:45:47AM +0100, Peter Maydell wrote: > Date: Thu, 23 Jul 2026 11:45:47 +0100 > From: Peter Maydell <[email protected]> > Subject: Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in > gpa2hva callers > > On Thu, 23 Jul 2026 at 10:46, Markus Armbruster <[email protected]> wrote: > > > > Philippe Mathieu-Daudé <[email protected]> writes: > > > > > Hi, > > > > > > On 21/5/26 16:54, [email protected] wrote: > > >> From: Marc-André Lureau <[email protected]> > > >> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter > > >> (inherited from memory_region_find()), but both SEV callers were failing > > >> to release it. > > > > > > Comment orthogonal to your fix; odd to have a non-monitor code > > > call an undocumented monitor method. > > > > Which one? > > The gpa2hva() function, presumably. It does seem a bit odd to > have the target/i386 code directly calling this. The function > in monitor.c works on the system memory AddressSpace and > assumes that there is only one kind of "guest physical address", > because that's the implicit "everything is an old i386 PC" > kind of assumption that got backed into the human monitor > command. But it's not really something we should be doing > more widely. > > The i386/sev code looks rather odd to me. It seems to be > using gpa2hva as a way to get at the host memory that is > backing the guest memory. Why can't it use the usual > address_space_map() etc APIs for this?
Yes, I agree. For sev_inject_launch_secret, it's straightforward to use address_space_map(). > Worse, it looks like > it keeps that host pointer around in a SevLaunchUpdateData > struct, and it's not immediately obvious to me that it won't > go wrong if the memory map later gets moved around and that > pointer becomes invalid. And it looks like it writes into > guest memory in a way that will bypass the dirty-tracking. For snp_populate_metadata_pages() and SevLaunchUpdateData, I think maybe we can use address_space_map() to get pointer, and add a flag to SevLaunchUpdateData marking that data entry owns a mapping. Then sev_snp_launch_finish() can unmap if that flag is set, in its loop. Thanks, Zhao
