Re: [PATCH v2 1/3] x86/shadow: move dm-mmio handling code in sh_page_fault()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2023 15:26, Jan Beulich wrote:
> Do away with the partly mis-named "mmio" label there, which really is
> only about emulated MMIO. Move the code to the place where the sole
> "goto" was. Re-order steps slightly: Assertion first, perfc increment
> outside of the locked region, and "gpa" calculation closer to the first
> use of the variable. Also make the HVM conditional cover the entire
> if(), as p2m_mmio_dm isn't applicable to PV; specifically get_gfn()
> won't ever return this type for PV domains.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> v2: New.
> 
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c

I've sent a stale patch, I'm sorry. This further hunk is needed to keep
!HVM builds working:

@@ -2144,8 +2144,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;

Jan

> @@ -2588,13 +2588,33 @@ 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 )
>      {
> +        ASSERT(is_hvm_vcpu(v));
> +        if ( !guest_mode(regs) )
> +            goto not_a_shadow_fault;
> +
> +        sh_audit_gw(v, &gw);
>          gpa = guest_walk_to_gpa(&gw);
> -        goto mmio;
> +        SHADOW_PRINTK("mmio %#"PRIpaddr"\n", gpa);
> +        shadow_audit_tables(v);
> +        sh_reset_early_unshadow(v);
> +
> +        paging_unlock(d);
> +        put_gfn(d, gfn_x(gfn));
> +
> +        perfc_incr(shadow_fault_mmio);
> +        trace_shadow_gen(TRC_SHADOW_MMIO, va);
> +
> +        return handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, access)
> +               ? EXCRET_fault_fixed : 0;
>      }
>  
> +#endif /* CONFIG_HVM */
> +
>      /* Ignore attempts to write to read-only memory. */
>      if ( p2m_is_readonly(p2mt) && (ft == ft_demand_write) )
>          goto emulate_readonly; /* skip over the instruction */
> @@ -2867,25 +2887,6 @@ static int cf_check sh_page_fault(
>      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));
> -    perfc_incr(shadow_fault_mmio);
> -    sh_audit_gw(v, &gw);
> -    SHADOW_PRINTK("mmio %#"PRIpaddr"\n", gpa);
> -    shadow_audit_tables(v);
> -    sh_reset_early_unshadow(v);
> -    paging_unlock(d);
> -    put_gfn(d, gfn_x(gfn));
> -    trace_shadow_gen(TRC_SHADOW_MMIO, va);
> -    return (handle_mmio_with_translation(va, gpa >> PAGE_SHIFT, access)
> -            ? EXCRET_fault_fixed : 0);
> -#else
> -    BUG();
> -#endif
> -
>   not_a_shadow_fault:
>      sh_audit_gw(v, &gw);
>      SHADOW_PRINTK("not a shadow fault\n");
> 
>