Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Zhao Liu <zhao1.liu@intel.com>]

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 <peter.maydell@linaro.org>
> Subject: Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in
>  gpa2hva callers
> 
> On Thu, 23 Jul 2026 at 10:46, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:
> >
> > > Hi,
> > >
> > > On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
> > >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >> 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