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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 26/03/2020 15:39, Jan Beulich wrote:
> On 22.03.2020 17:14, julien@xxxxxxx wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -952,25 +952,27 @@ int arch_set_info_guest(
> >       }
> >       else
> >       {
> > -        unsigned long pfn = pagetable_get_pfn(v->arch.guest_table);
> > +        mfn_t mfn = pagetable_get_mfn(v->arch.guest_table);
> >           bool fail;
> >   
> >           if ( !compat )
> >           {
> > -            fail = xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[3];
> > +            fail = mfn_to_cr3(mfn) != c.nat->ctrlreg[3];
> The patch, besides a few other comments further down, looks fine
> on its own, but I don't think it can be acked without seeing the
> effects of the adjustments pending to the patch introducing
> mfn_to_cr3() and friends.
>
> > @@ -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.

> > @@ -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.

> > @@ -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.
> > @@ -2351,11 +2351,11 @@ int sh_safe_not_to_sync(struct vcpu *v, mfn_t gl1mfn)
> >       ASSERT(mfn_valid(smfn));
> >   #endif
> >   
> > -    if ( pagetable_get_pfn(v->arch.shadow_table[0]) == mfn_x(smfn)
> > +    if ( mfn_eq(pagetable_get_mfn(v->arch.shadow_table[0]), smfn)
> >   #if (SHADOW_PAGING_LEVELS == 3)
> > -         || pagetable_get_pfn(v->arch.shadow_table[1]) == mfn_x(smfn)
> > -         || pagetable_get_pfn(v->arch.shadow_table[2]) == mfn_x(smfn)
> > -         || pagetable_get_pfn(v->arch.shadow_table[3]) == mfn_x(smfn)
> > +         || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[1]), smfn)
> > +         || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[2]), smfn)
> > +         || mfn_eq(pagetable_get_mfn(v->arch.shadow_table[3]), smfn)
> >   #endif
> >           )
> While here moving the || to their designated places would make
> the code look worse overall, ...
>
> > @@ -3707,7 +3707,7 @@ sh_update_linear_entries(struct vcpu *v)
> >   
> >       /* Don't try to update the monitor table if it doesn't exist */
> >       if ( shadow_mode_external(d)
> > -         && pagetable_get_pfn(v->arch.monitor_table) == 0 )
> > +         && pagetable_is_null(v->arch.monitor_table) )
> ... could I talk you into moving the && here to the end of the
> previous line, as you're touching this anyway?
I will do.

> Also, seeing there's quite a few conversions to pagetable_is_null()
> and also seeing that this patch is quite big - could this
> conversion be split out?
I will have a look.

> > @@ -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.
Cheers,

--
Julien Grall