[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] x86/p2m: fix xenmem_add_to_physmap_one double page removal

  • To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Tue, 14 Sep 2021 16:32:53 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=KCHr+iu/Fh/OEFPHBp/vLAtGuSF7lejGesqvViIC/XM=; b=ZJ3ALdgKUk/I8+gGQ5BqaNWLaoc5AGTOXsNaM4tET342LekYnJb1mSLKRTQj8dNnz4HhNBmoWIW7WK6gwFyNqlxFV6rD/Mevmpp1gVhbiL0cCZ77wLJixSUxJBSO1wGgkC7bdHDJ2pKqzU5PaEUuXObGGK0nsF0s0vVDfoBVsmt3U4scviWsbZl77ulCBmkoPND7iHoJf3JJPxFGPE01Q6lZyMk6nQwZ26RHwp7bYZcCtrsTrp9M68MTFNuPWtT16F1nIi/lneKxCGA7fnsAiyCHfvyoKJjPokbrYMGpDD4tRmrEc977pijOaQ7XJ4cKJUbkGlNNqenZLrDNKgZ1uA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IksFiJu2gI1huiGiGxJi3JHP/lBgOU2HTS6OoVY2lDs875Beib6WvGpNR0l0OKoJt4aw1Cr6QRN6RW7BtezY8AuRx52RzujZL4XUhVL19caiyglPPmcB+KdTfYwLVhGmEJDH+Ubenf0ABYNZrJmueUmIQDvNxRQCkKmfujdage8aOK5KpEWrLHN78xg1SqLGu9lWZH/u6DOk5wCarKj+A9NQCdNPK6WvQpmuR/HOzamMAG7vbgDG1/kvTGoH+2uVHO6sGtNB+GuXycT/6CCXm3n+GO/BV4Ogx0ZKN/trpPf1JEZHzeTfVHNBJJuZvAeWz3mY4YWkvMxarV0d+robHw==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 14 Sep 2021 14:33:07 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 14.09.2021 15:34, Roger Pau Monne wrote:
> If the new gfn matches the previous one (ie: gfn == old_gpfn)

It took me a while to realize that you probably mean "gpfn" here.

> xenmem_add_to_physmap_one will issue a duplicated call to
> guest_physmap_remove_page with the same gfn, because the

Considering the local variable of this name, may I suggest to
upper-case GFN in this case?

> get_gpfn_from_mfn call has been moved by commit f8582da041 to be
> performed before the original page is removed. This leads to the
> second guest_physmap_remove_page failing, which was not the case
> before commit f8582da041.
> Fix this by adding a check that prevents a second call to
> guest_physmap_remove_page if the previous one has already removed the
> backing page from that gfn.

Same here (if this remains; see below).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2813,7 +2813,7 @@ int xenmem_add_to_physmap_one(
>      }
>      /* Unmap from old location, if any. */
> -    if ( !rc && old_gpfn != INVALID_M2P_ENTRY )
> +    if ( !rc && old_gpfn != INVALID_M2P_ENTRY && !gfn_eq(_gfn(old_gpfn), 
> gpfn) )
>          rc = guest_physmap_remove_page(d, _gfn(old_gpfn), mfn, 
>      /* Map at new location. */

It feels unbalanced to suppress the remove, but keep the add in
this case. Wouldn't we be better off skipping both, or short-
circuiting the effective no-op even earlier? I recall considering
to install a shortcut, but it didn't seem worth it. But now that
you've found actual breakage, perhaps this need reconsidering.




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.