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

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


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)

Zero has not special meaning on Arm, so we could limit to x86.

@@ -3560,19 +3561,18 @@ long do_mmuext_op(
               if ( unlikely(rc) )
   -            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 
                * previous attempt got preempted in the put-old-MFN phase.
-            if ( old_mfn == op.arg1.mfn )
+            if ( mfn_eq(old_mfn, new_mfn) )
   -            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 )
                                    "Error %d installing new mfn %" PRI_mfn 
-                                 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

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?


Julien Grall



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