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

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Sun, Dec 17, 2023 at 11:55 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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>

Reviewed-by: Christopher Clark <christopher.w.clark@xxxxxxxxx>

>
> 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 for your patience and the reminders, appreciated and this patch
can be applied.

Christopher


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