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.

Based on read-only p2m types including logdirty, this diagnostic can be
tripped by entirely legitimate guest behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/mm/shadow/multi.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index 8b3e678fa0fa..3b06cfaf9a5a 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2597,14 +2597,7 @@ static int cf_check sh_page_fault(
 
     /* Ignore attempts to write to read-only memory. */
     if ( p2m_is_readonly(p2mt) && (ft == ft_demand_write) )
-    {
-        static unsigned long lastpage;
-        if ( xchg(&lastpage, va & PAGE_MASK) != (va & PAGE_MASK) )
-            gdprintk(XENLOG_DEBUG, "guest attempted write to read-only memory"
-                     " page. va page=%#lx, mfn=%#lx\n",
-                     va & PAGE_MASK, mfn_x(gmfn));
         goto emulate_readonly; /* skip over the instruction */
-    }
 
     /* In HVM guests, we force CR0.WP always to be set, so that the
      * pagetables are always write-protected.  If the guest thinks
-- 
2.11.0

On 20.01.2023 12:45, Andrew Cooper wrote:
> 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.
> 
> Based on read-only p2m types including logdirty, this diagnostic can be
> tripped by entirely legitimate guest behaviour.

Can it? At the very least shadow doesn't use p2m_ram_logdirty, but "cooks"
log-dirty handling its own way.

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

Acked-by: Jan Beulich <jbeulich@suse.com>
with the last sentence above corrected (if need be: removed).

Jan

On 20/01/2023 1:10 pm, Jan Beulich wrote:
> On 20.01.2023 12:45, Andrew Cooper wrote:
>> 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.
>>
>> Based on read-only p2m types including logdirty, this diagnostic can be
>> tripped by entirely legitimate guest behaviour.
> Can it? At the very least shadow doesn't use p2m_ram_logdirty, but "cooks"
> log-dirty handling its own way.
>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> with the last sentence above corrected (if need be: removed).

I can remove it, but I feel as if there ought to be something there.

The other RO types are ram_ro, grant_map_ro and ram_shared.  shared has
hopefully been unshared before getting to this point, while the other
two have unclear semantics (as neither exist in real systems).

Writing to something which is actually a ROM either does silent discard,
or #MC.

Read-only grants really ought to #PF, but I bet this ABI change between
PV and HVM guests wasn't noticed because I'm not aware of any common
uses of read-only grants.

~Andrew

On 20.01.2023 15:10, Andrew Cooper wrote:
> On 20/01/2023 1:10 pm, Jan Beulich wrote:
>> On 20.01.2023 12:45, Andrew Cooper wrote:
>>> 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.
>>>
>>> Based on read-only p2m types including logdirty, this diagnostic can be
>>> tripped by entirely legitimate guest behaviour.
>> Can it? At the very least shadow doesn't use p2m_ram_logdirty, but "cooks"
>> log-dirty handling its own way.
>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.
> 
>> with the last sentence above corrected (if need be: removed).
> 
> I can remove it, but I feel as if there ought to be something there.
> 
> The other RO types are ram_ro, grant_map_ro and ram_shared.  shared has
> hopefully been unshared before getting to this point, while the other
> two have unclear semantics (as neither exist in real systems).

I'd be okay as long as the "including logdirty" part isn't there. If
we're unsure, perhaps then also instead of "can" either "might" or
"can possibly"?

Jan

On 20/01/2023 2:20 pm, Jan Beulich wrote:
> On 20.01.2023 15:10, Andrew Cooper wrote:
>> On 20/01/2023 1:10 pm, Jan Beulich wrote:
>>> On 20.01.2023 12:45, Andrew Cooper wrote:
>>>> 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.
>>>>
>>>> Based on read-only p2m types including logdirty, this diagnostic can be
>>>> tripped by entirely legitimate guest behaviour.
>>> Can it? At the very least shadow doesn't use p2m_ram_logdirty, but "cooks"
>>> log-dirty handling its own way.
>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>> Thanks.
>>
>>> with the last sentence above corrected (if need be: removed).
>> I can remove it, but I feel as if there ought to be something there.
>>
>> The other RO types are ram_ro, grant_map_ro and ram_shared.  shared has
>> hopefully been unshared before getting to this point, while the other
>> two have unclear semantics (as neither exist in real systems).
> I'd be okay as long as the "including logdirty" part isn't there. If
> we're unsure, perhaps then also instead of "can" either "might" or
> "can possibly"?

I'll just delete it.  It's not important enough for the time it's taking.

~Andrew

On 20/01/2023 2:39 pm, Andrew Cooper wrote:
> On 20/01/2023 2:20 pm, Jan Beulich wrote:
>> On 20.01.2023 15:10, Andrew Cooper wrote:
>>> On 20/01/2023 1:10 pm, Jan Beulich wrote:
>>>> On 20.01.2023 12:45, Andrew Cooper wrote:
>>>>> 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.
>>>>>
>>>>> Based on read-only p2m types including logdirty, this diagnostic can be
>>>>> tripped by entirely legitimate guest behaviour.
>>>> Can it? At the very least shadow doesn't use p2m_ram_logdirty, but "cooks"
>>>> log-dirty handling its own way.
>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>> Thanks.
>>>
>>>> with the last sentence above corrected (if need be: removed).
>>> I can remove it, but I feel as if there ought to be something there.
>>>
>>> The other RO types are ram_ro, grant_map_ro and ram_shared.  shared has
>>> hopefully been unshared before getting to this point, while the other
>>> two have unclear semantics (as neither exist in real systems).
>> I'd be okay as long as the "including logdirty" part isn't there. If
>> we're unsure, perhaps then also instead of "can" either "might" or
>> "can possibly"?
> I'll just delete it.  It's not important enough for the time it's taking.

Oh, I see what you mean.  Yeah, that will work.

~Andrew