Ping: [PATCH] Argo: drop meaningless mfn_valid() check

[Jan Beulich <jbeulich@xxxxxxxx>]

Christopher,

On 27.11.2023 14:55, Jan Beulich wrote:
> Holding a valid struct page_info * in hands already means the referenced
> MFN is valid; there's no need to check that again. Convert the checking
> logic to a switch(), to help keeping the extra (and questionable) x86-
> only check in somewhat tidy shape.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

much like "Argo: don't obtain excess page references" (with which the one
here actually also conflicts), this one is awaiting your ack or otherwise.
Note that the other one has now been pending for quite a bit more than a
year. I hope the same isn't going to happen here ...

Thanks, Jan

> ---
> Initially I had this (with less code churn) as
> 
> #ifdef CONFIG_X86
>     if ( p2mt == p2m_ram_logdirty )
>         ret = -EAGAIN;
>     else
> #endif
>     if ( (p2mt != p2m_ram_rw) ||
>          !get_page_type(page, PGT_writable_page) )
>         ret = -EINVAL;
> 
> But the "else" placement seemed too ugly to me. Otoh there better
> wouldn't be any special casing of log-dirty here (and instead such a
> page be converted, perhaps right in check_get_page_from_gfn() when
> readonly=false), at which point the odd "else" would go away, and the
> if() likely again be preferable over the switch().
> 
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -1421,15 +1421,24 @@ find_ring_mfn(struct domain *d, gfn_t gf
>          return ret;
>  
>      *mfn = page_to_mfn(page);
> -    if ( !mfn_valid(*mfn) )
> -        ret = -EINVAL;
> +
> +    switch ( p2mt )
> +    {
> +    case p2m_ram_rw:
> +        if ( !get_page_and_type(page, d, PGT_writable_page) )
> +            ret = -EINVAL;
> +        break;
> +
>  #ifdef CONFIG_X86
> -    else if ( p2mt == p2m_ram_logdirty )
> +    case p2m_ram_logdirty:
>          ret = -EAGAIN;
> +        break;
>  #endif
> -    else if ( (p2mt != p2m_ram_rw) ||
> -              !get_page_and_type(page, d, PGT_writable_page) )
> +
> +    default:
>          ret = -EINVAL;
> +        break;
> +    }
>  
>      put_page(page);
>  
>