Re: [PATCH 11/16] x86/shadow: drop is_hvm_...() where easily possible

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/03/2023 9:35 am, Jan Beulich wrote:
> Emulation related functions are involved in HVM handling only, and in
> some cases they even invoke such checks after having already done things
> which are valid for HVM domains only. OOS active also implies HVM. In
> sh_remove_all_mappings() one of the two checks is redundant with an
> earlier paging_mode_external() one (the other, however, needs to stay).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain
>                 && (page->count_info & PGC_count_mask) <= 3
>                 && ((page->u.inuse.type_info & PGT_count_mask)
>                     == (is_special_page(page) ||
> -                       (is_hvm_domain(d) && is_ioreq_server_page(d, 
> page))))) )
> +                       is_ioreq_server_page(d, page)))) )
>              printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
>                     " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
>                     mfn_x(gmfn), gfn_x(gfn),

Out of context here needs an equivalent adjustment.

But in this case, I'm not sure the commit message covers the relevant
details.  ioreq servers have been made fully common since this code was
written, and *that* is a better reason for dropping the predicates IMO
than the redundancy with paging_mode_external().

That said...  I'm not sure the logic here is correct any more.  It used
to be the case that ioreq pages were in the p2m, but they're outside of
the p2m these days, so don't see how there can be any interaction with
unexpected refcounts any more.

I suspect that one way or another, this change wants to be in a separate
patch.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3441,7 +3441,7 @@ int sh_rm_write_access_from_sl1p(struct
>  
>  #ifdef CONFIG_HVM
>      /* Remember if we've been told that this process is being torn down */
> -    if ( curr->domain == d && is_hvm_domain(d) )
> +    if ( curr->domain == d )
>          curr->arch.paging.shadow.pagetable_dying
>              = mfn_to_page(gmfn)->pagetable_dying;
>  #endif

This one is dangerous.

After tracing, I can see that sh_rm_write_access_from_sl1p() is only
called from OOS functions, but this function itself does its very best
to look like it has mixed PV + HVM usage, and dropping this conditional
means that pagetable_dying can, in principle at least, become non-NULL
for a PV guest.

I think this function needs to be made far more obviously HVM-only first.

> --- a/xen/arch/x86/mm/shadow/oos.c
> +++ b/xen/arch/x86/mm/shadow/oos.c
> @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn
>      if ( pg->shadow_flags &
>           ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync)
>           || sh_page_has_multiple_shadows(pg)
> -         || !is_hvm_vcpu(v)
>           || !v->domain->arch.paging.shadow.oos_active )

This is reachable for PV guests as far as I can see.  What am I missing ?

The changes in hvm.c are all fine, and for those alone, consider it R-by
if you end up splitting the patch.

~Andrew