[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 20/04/2020 10:16, Jan Beulich wrote:
On 18.04.2020 12:23, Julien Grall wrote:
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?

I'm afraid there may not be a clear line to draw until everything
got converted.

I am sorry but this doesn't add up. Here you say that we can't have a clear line to draw until everything is converted but...

I do seem to recall though that, perhaps in a
different context, Andrew recently agreed with my view here (Andrew,
please correct me if I'm wrong). It being a fuzzy thing, I guess
maintainers get to judge ...

... here you say the maintainers get to decide when to use mfn_eq() (or other typesafe construction). So basically, we would never be able to fully convert the code and therefore never draw a line.

As I am trying to convert x86 to use typesafe, I would like a bit more guidelines on your expectation for typesafe. Can you clarify it?


Julien Grall



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