qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Cc: qemu-devel@nongnu.org, "Paolo Bonzini" <pbonzini@redhat.com>,
	"David Hildenbrand" <david@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@linaro.org>
Subject: Re: [PATCH] memory: Make FlatView root references weak
Date: Wed, 5 Nov 2025 15:07:33 -0500	[thread overview]
Message-ID: <aQuuhSL6rXmyqm8x@x1.local> (raw)
In-Reply-To: <13cb4e7e-1949-4dc6-b5d6-a976f6f280e4@rsg.ci.i.u-tokyo.ac.jp>

On Mon, Nov 03, 2025 at 08:18:00PM +0900, Akihiko Odaki wrote:
> It unfortunately does not work for pci-bridge. It has the following
> function:
> 
> void pci_bridge_update_mappings(PCIBridge *br)
> {
>     PCIBridgeWindows *w = &br->windows;
> 
>     /* Make updates atomic to: handle the case of one VCPU updating the
> bridge
>      * while another accesses an unaffected region. */
>     memory_region_transaction_begin();
>     pci_bridge_region_del(br, w);
>     pci_bridge_region_cleanup(br, w);
>     pci_bridge_region_init(br);
>     memory_region_transaction_commit();
> }
> 
> object_unparent() happens in pci_bridge_region_cleanup().
> pci_bridge_region_init() reuses the storage.
> memory_region_transaction_commit() triggers flatview_unref(), but it needs
> to happen before pci_bridge_region_init().
> 
> memory_region_transaction_commit() also has an undesirable characteristic
> that its effect may be delayed due to nesting. To make sure flatview_unref()
> happens with a particular call of memory_region_transaction_commit(), you
> need to traverse the possible call graph that lead to the function.
> 
> So I'm afraid but I don't think there is a better way to ensure correctness
> without a codebase-wide audit.

Ah indeed, I missed that. :(

One way to work this around is providing a helper (abstraction from the
current memory_region_transaction_commit) to enforce a flatview reset
before reusing.  However I feel like it's an overkill too, but at least
that would also avoid weak-refs.

I think in practise I'd vote we fix pci-bridge only, either with your other
proposal to dynamically allocate the alias MRs, or something like you
posted previously:

https://lore.kernel.org/all/20250906-use-v1-3-c51caafd1eb7@rsg.ci.i.u-tokyo.ac.jp/#t

Personally, I don't mind fixing pci-bridge only even if we don't audit the
whole code base.  The audit work is time consuming, and I'd simply trust
the tests from all the QEMU users covering whatever devices are still being
used. We will always get an issue report when something was wrong.

What do you think?

Thanks,

-- 
Peter Xu



  reply	other threads:[~2025-11-05 20:08 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-27  5:56 [PATCH] memory: Make FlatView root references weak Akihiko Odaki
2025-10-28 22:09 ` Peter Xu
2025-10-29  4:06   ` Akihiko Odaki
2025-10-29  4:35     ` Akihiko Odaki
2025-10-29 15:21     ` Peter Xu
2025-11-03 11:18       ` Akihiko Odaki
2025-11-05 20:07         ` Peter Xu [this message]
2025-11-06  2:23           ` Akihiko Odaki
2025-11-06 17:50             ` Peter Xu
2025-11-07  2:16               ` Akihiko Odaki
2025-11-07 16:40                 ` Peter Xu
2025-11-08  2:07                   ` Akihiko Odaki
2025-11-12 22:39                     ` Peter Xu
2025-11-13  1:58                       ` Akihiko Odaki

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aQuuhSL6rXmyqm8x@x1.local \
    --to=peterx@redhat.com \
    --cc=david@redhat.com \
    --cc=odaki@rsg.ci.i.u-tokyo.ac.jp \
    --cc=pbonzini@redhat.com \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).