[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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 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()?

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

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

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

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.