[PATCH 0/2] x86/shadow: sh_page_fault() adjustments

[PATCH 0/2] x86/shadow: sh_page_fault() adjustments

[Jan Beulich]

1: fix PAE check for top-level table unshadowing
2: mark more of sh_page_fault() HVM-only

Jan

[PATCH 1/2] x86/shadow: fix PAE check for top-level table unshadowing

[Jan Beulich]

Clearly within the for_each_vcpu() the vCPU of this loop is meant, not
the (loop invariant) one the fault occurred on.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Quitle likely this mistake would have been avoided if the function scope
variable was named "curr", leaving "v" available for purposes likethe
one here. Doing the rename now would, however, be quite a bit of code
churn.

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2672,10 +2672,10 @@ static int cf_check sh_page_fault(
 #if GUEST_PAGING_LEVELS == 3
             unsigned int i;
 
-            for_each_shadow_table(v, i)
+            for_each_shadow_table(tmp, i)
             {
                 mfn_t smfn = pagetable_get_mfn(
-                                 v->arch.paging.shadow.shadow_table[i]);
+                                 tmp->arch.paging.shadow.shadow_table[i]);
 
                 if ( mfn_x(smfn) )
                 {

[PATCH 2/2] x86/shadow: mark more of sh_page_fault() HVM-only

[Jan Beulich]

Neither p2m_mmio_dm nor the types p2m_is_readonly() checks for are
applicable to PV; specifically get_gfn() won't ever return such a type
for PV domains. Adjacent to those checks is yet another is_hvm_...()
one. With that block made conditional, another conditional block near
the end of the function can be widened.

Furthermore the shadow_mode_refcounts() check immediately ahead of the
aforementioned newly inserted #ifdef renders redundant two subsequent
is_hvm_domain() checks (the latter of which was already redundant with
the former).

Also guest_mode() checks are pointless when we already know we're
dealing with a HVM domain.

Finally style-adjust a comment which otherwise would be fully visible as
patch context anyway.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I'm not convinced of the usefulness of the ASSERT() immediately after
the "mmio" label. Additionally I think the code there would better move
to the single place where we presently have "goto mmio", bringing things
more in line with the other handle_mmio_with_translation() invocation
site.

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -2158,8 +2158,8 @@ static int cf_check sh_page_fault(
     gfn_t gfn = _gfn(0);
     mfn_t gmfn, sl1mfn = _mfn(0);
     shadow_l1e_t sl1e, *ptr_sl1e;
-    paddr_t gpa;
 #ifdef CONFIG_HVM
+    paddr_t gpa;
     struct sh_emulate_ctxt emul_ctxt;
     const struct x86_emulate_ops *emul_ops;
     int r;
@@ -2583,6 +2583,7 @@ static int cf_check sh_page_fault(
         goto emulate;
     }
 
+#ifdef CONFIG_HVM
     /* Need to hand off device-model MMIO to the device model */
     if ( p2mt == p2m_mmio_dm )
     {
@@ -2614,13 +2615,14 @@ static int cf_check sh_page_fault(
         perfc_incr(shadow_fault_emulate_wp);
         goto emulate;
     }
+#endif
 
     perfc_incr(shadow_fault_fixed);
     d->arch.paging.log_dirty.fault_count++;
     sh_reset_early_unshadow(v);
 
     trace_shadow_fixup(gw.l1e, va);
- done:
+ done: __maybe_unused;
     sh_audit_gw(v, &gw);
     SHADOW_PRINTK("fixed\n");
     shadow_audit_tables(v);
@@ -2629,9 +2631,10 @@ static int cf_check sh_page_fault(
     return EXCRET_fault_fixed;
 
  emulate:
-    if ( !shadow_mode_refcounts(d) || !guest_mode(regs) )
+    if ( !shadow_mode_refcounts(d) )
         goto not_a_shadow_fault;
 
+#ifdef CONFIG_HVM
     /*
      * We do not emulate user writes. Instead we use them as a hint that the
      * page is no longer a page table. This behaviour differs from native, but
@@ -2653,17 +2656,11 @@ static int cf_check sh_page_fault(
      * caught by user-mode page-table check above.
      */
  emulate_readonly:
-    if ( !is_hvm_domain(d) )
-    {
-        ASSERT_UNREACHABLE();
-        goto not_a_shadow_fault;
-    }
-
-#ifdef CONFIG_HVM
-    /* Unshadow if we are writing to a toplevel pagetable that is
-     * flagged as a dying process, and that is not currently used. */
-    if ( sh_mfn_is_a_page_table(gmfn) && is_hvm_domain(d) &&
-         mfn_to_page(gmfn)->pagetable_dying )
+    /*
+     * Unshadow if we are writing to a toplevel pagetable that is
+     * flagged as a dying process, and that is not currently used.
+     */
+    if ( sh_mfn_is_a_page_table(gmfn) && mfn_to_page(gmfn)->pagetable_dying )
     {
         int used = 0;
         struct vcpu *tmp;
@@ -2867,13 +2864,9 @@ static int cf_check sh_page_fault(
  emulate_done:
     SHADOW_PRINTK("emulated\n");
     return EXCRET_fault_fixed;
-#endif /* CONFIG_HVM */
 
  mmio:
-    if ( !guest_mode(regs) )
-        goto not_a_shadow_fault;
-#ifdef CONFIG_HVM
-    ASSERT(is_hvm_vcpu(v));
+    ASSERT(is_hvm_domain(d));
     perfc_incr(shadow_fault_mmio);
     sh_audit_gw(v, &gw);
     SHADOW_PRINTK("mmio %#"PRIpaddr"\n", gpa);
@@ -2884,9 +2877,7 @@ static int cf_check sh_page_fault(
     trace_shadow_gen(TRC_SHADOW_MMIO, va);
     return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, access)
             ? EXCRET_fault_fixed : 0);
-#else
-    BUG();
-#endif
+#endif /* CONFIG_HVM */
 
  not_a_shadow_fault:
     sh_audit_gw(v, &gw);

Re: [PATCH 1/2] x86/shadow: fix PAE check for top-level table unshadowing

[Andrew Cooper]

On 19/01/2023 1:19 pm, Jan Beulich wrote:
> Clearly within the for_each_vcpu() the vCPU of this loop is meant, not
> the (loop invariant) one the fault occurred on.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Wow that's been broken for the entire lifetime of the pagetable_dying op
0 3d5e6a3ff38 from 2010, but it still deserves a fixes tag.

Preferably with that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Quitle likely this mistake would have been avoided if the function scope
> variable was named "curr", leaving "v" available for purposes likethe
> one here. Doing the rename now would, however, be quite a bit of code
> churn.

Perhaps, but that pattern was far less prevalent back then, and the real
cause of this bug is sh_page_fault() being far too big and sprawling.

~Andrew

Re: [PATCH 1/2] x86/shadow: fix PAE check for top-level table unshadowing

[Jan Beulich]

On 19.01.2023 19:19, Andrew Cooper wrote:
> On 19/01/2023 1:19 pm, Jan Beulich wrote:
>> Clearly within the for_each_vcpu() the vCPU of this loop is meant, not
>> the (loop invariant) one the fault occurred on.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Wow that's been broken for the entire lifetime of the pagetable_dying op
> 0 3d5e6a3ff38 from 2010, but it still deserves a fixes tag.

Oh, yes, of course. But then it'll really need two, as ef3b0d8d2c39
("x86/shadow: shadow_table[] needs only one entry for PV-only configs")
was also flawed, and I guess I really should have spotted the issue
back then already.

> Preferably with that, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan

Re: [PATCH 2/2] x86/shadow: mark more of sh_page_fault() HVM-only

[Andrew Cooper]

On 19/01/2023 1:19 pm, Jan Beulich wrote:
> Neither p2m_mmio_dm nor the types p2m_is_readonly() checks for are
> applicable to PV; specifically get_gfn() won't ever return such a type
> for PV domains. Adjacent to those checks is yet another is_hvm_...()
> one. With that block made conditional, another conditional block near
> the end of the function can be widened.

Why?  I presume you're referring to the emulate_readonly label?

Could I request "with that block made condition, the emulate_readonly
label becomes conditional too."

"widened" is actually widened in both direction, AFAICT. to include both
the emulate_readonly and mmio labels.

But looking at the code, I think we mean emulated mmio only, because it
comes from p2m_mmio_dm only ?

>
> Furthermore the shadow_mode_refcounts() check immediately ahead of the
> aforementioned newly inserted #ifdef

This would be far easier to follow if you said the emulate label.

>  renders redundant two subsequent
> is_hvm_domain() checks (the latter of which was already redundant with
> the former).
>
> Also guest_mode() checks are pointless when we already know we're
> dealing with a HVM domain.
>
> Finally style-adjust a comment which otherwise would be fully visible as
> patch context anyway.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

So I think this is all ok, but honestly it would be far easier to review
if it was split into two patches - first the #ifdef rearranging, and the
cleanup second.

> ---
> I'm not convinced of the usefulness of the ASSERT() immediately after
> the "mmio" label. Additionally I think the code there would better move
> to the single place where we presently have "goto mmio", bringing things
> more in line with the other handle_mmio_with_translation() invocation
> site.

That would remove the poorly named label, and make it clearly emulated
mmio only.  So 3 patches with this movement as the first one?

~Andrew