[PATCH] x86/HVM: purge dubious lastpage diagnostic

Jan Beulich posted 1 patch 1 year, 1 month ago
Failed in applying to current master (apply log)
[PATCH] x86/HVM: purge dubious lastpage diagnostic
Posted by Jan Beulich 1 year, 1 month ago
Quoting b5d8b03db136 ("x86/shadow: Drop dubious lastpage diagnostic"):

"This is a global variable (actually 3, one per GUEST_PAGING_LEVEL), operated
 on using atomics only (with no regard to what else shares the same cacheline),
 which emits a diagnostic (in debug builds only) without changing any program
 behaviour.

 It is presumably left-over debugging, as it interlinks the behaviour of all
 vCPUs in chronological order.  Based on the read-only p2m types, this
 diagnostic can be tripped by entirely legitimate guest behaviour."

All the same applies here (it's only a single variable of course).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3354,16 +3354,7 @@ static enum hvm_translation_result __hvm
                 memcpy(buf, p, count);
                 hvmemul_write_cache(v, gfn_to_gaddr(gfn) | pgoff, buf, count);
             }
-            else if ( p2m_is_discard_write(p2mt) )
-            {
-                static unsigned long lastpage;
-
-                if ( xchg(&lastpage, gfn_x(gfn)) != gfn_x(gfn) )
-                    dprintk(XENLOG_G_DEBUG,
-                            "%pv attempted write to read-only gfn %#lx (mfn=%#"PRI_mfn")\n",
-                            v, gfn_x(gfn), mfn_x(page_to_mfn(page)));
-            }
-            else
+            else if ( !p2m_is_discard_write(p2mt) )
             {
                 if ( buf )
                     memcpy(p, buf, count);
Re: [PATCH] x86/HVM: purge dubious lastpage diagnostic
Posted by Andrew Cooper 1 year, 1 month ago
On 02/03/2023 12:06 pm, Jan Beulich wrote:
> Quoting b5d8b03db136 ("x86/shadow: Drop dubious lastpage diagnostic"):
>
> "This is a global variable (actually 3, one per GUEST_PAGING_LEVEL), operated
>  on using atomics only (with no regard to what else shares the same cacheline),
>  which emits a diagnostic (in debug builds only) without changing any program
>  behaviour.
>
>  It is presumably left-over debugging, as it interlinks the behaviour of all
>  vCPUs in chronological order.  Based on the read-only p2m types, this
>  diagnostic can be tripped by entirely legitimate guest behaviour."
>
> All the same applies here (it's only a single variable of course).

Just "The same ..."

It's not all, because of the single variable note, but grammatically
speaking its fine without the "All".

>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

I was aware of, but had forgotten, that we had this pattern elsewhere.