Re: [PATCH 05/17] xen/x86: Remove the non-typesafe version of pagetable_* helpers

[Julien Grall <julien@xxxxxxx>]

Hi,

On 30/03/2020 08:52, Jan Beulich wrote:
> On 28.03.2020 11:52, Julien Grall wrote:
> > On 26/03/2020 15:39, Jan Beulich wrote:
> > > On 22.03.2020 17:14, julien@xxxxxxx wrote:
> > > > @@ -3116,24 +3116,24 @@ int vcpu_destroy_pagetables(struct vcpu *v)
> > > >          /* Free that page if non-zero */
> > > >        do {
> > > > -        if ( mfn )
> > > > +        if ( !mfn_eq(mfn, _mfn(0)) )
> > > I admit I'm not fully certain either, but at the first glance
> > >
> > >           if ( mfn_x(mfn) )
> > >
> > > would seem more in line with the original code to me (and then
> > > also elsewhere).
> > It is doing *exactly* the same things. The whole point of typesafe
> > is to use typesafe helper not open-coding test everywhere.
> >
> > It is also easier to spot any use of MFN 0 within the code as you
> > know could grep "_mfn(0)".
> >
> > Therefore I will insist to the code as-is.
> What I insit on is that readability of the result of such changes be
> also kept in mind. The mfn_eq() construct is (I think) clearly less
> easy to read and recognize than the simpler alternative suggested.
If mfn_eq() is less clear, then where do you draw the line when the 
macro should or not be used?
> If you want to avoid mfn_x(), how about introducing (if possible
> limited to x86, assuming that MFN 0 has no special meaning on Arm)
> mfn_zero()?
Zero has not special meaning on Arm, so we could limit to x86.

> > > > @@ -3560,19 +3561,18 @@ long do_mmuext_op(
> > > >                if ( unlikely(rc) )
> > > >                    break;
> > > >    -            old_mfn = pagetable_get_pfn(curr->arch.guest_table_user);
> > > > +            old_mfn = pagetable_get_mfn(curr->arch.guest_table_user);
> > > >                /*
> > > >                 * This is particularly important when getting restarted after 
> > > > the
> > > >                 * previous attempt got preempted in the put-old-MFN phase.
> > > >                 */
> > > > -            if ( old_mfn == op.arg1.mfn )
> > > > +            if ( mfn_eq(old_mfn, new_mfn) )
> > > >                    break;
> > > >    -            if ( op.arg1.mfn != 0 )
> > > > +            if ( !mfn_eq(new_mfn, _mfn(0)) )
> > > At least here I would clearly prefer the old code to be kept.
> > See above.
> I don't agree - here you're evaluating an aspect of the public
> interface. MFN 0 internally having a special meaning is, while
> connected to this aspect, still an implementation detail.
Fair enough.

> > > > @@ -3580,19 +3580,19 @@ long do_mmuext_op(
> > > >                        else if ( rc != -ERESTART )
> > > >                            gdprintk(XENLOG_WARNING,
> > > >                                     "Error %d installing new mfn %" PRI_mfn 
> > > > "\n",
> > > > -                                 rc, op.arg1.mfn);
> > > > +                                 rc, mfn_x(new_mfn));
> > > Here I'm also not sure I see the point of the conversion.
> > op.arg1.mfn and mfn are technically not the same type. The
> > former is a xen_pfn_t, whilst the latter is mfn_t.
> >
> > In practice they are both unsigned long on x86, so it should
> > be fine to use PRI_mfn. However, I think this is an abuse
> > and we should aim to use the proper PRI_* for a type.
> I'd be fine with switching to PRI_xen_pfn here, yes. But
> especially with the "not the same type" argument what should
> be logged is imo what was specified, not what we converted it
> to.
Fair point. I will switch back to op.arg1.mfn.

> > > > @@ -213,17 +214,17 @@ static inline l4_pgentry_t l4e_from_paddr(paddr_t pa, 
> > > > unsigned int flags)
> > > >    #ifndef __ASSEMBLY__
> > > >      /* Page-table type. */
> > > > -typedef struct { u64 pfn; } pagetable_t;
> > > > -#define pagetable_get_paddr(x)  ((paddr_t)(x).pfn << PAGE_SHIFT)
> > > > +typedef struct { mfn_t mfn; } pagetable_t;
> > > > +#define PAGETABLE_NULL_MFN      _mfn(0)
> > > I'd prefer to get away without this constant.
> > I would rather keep the constant as it makes easier to
> > understand what _mfn(0) means in the context of the pagetable.
> If this was used outside of the accessor definitions, I'd
> probably agree. But the accessor definitions exist specifically
> to abstract away such things from use sites. Hence, bike-
> shedding or not, if Andrew was clearly agreeing with your view,
> I'd accept it. If he's indifferent, I'd prefer the #define to
> be dropped.
Andrew, do you have any opinion?

Cheers,

--
Julien Grall