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

Commit Message

Jan Beulich Jan. 19, 2023, 1:19 p.m. UTC

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 <[e-mail]>
---
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.

Comments

Andrew Cooper Jan. 20, 2023, 3:24 p.m. UTC | #1

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 <[e-mail]>

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

Patch

--- 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);