Re: [PATCH 25/25] [PATCH] add paravirtualization support for x86_64

Andi Kleen wrote:

quoted

+	if (opfunc == NULL)
+		/* If there's no function, patch it with a ud2a (BUG) */
+		ret = paravirt_patch_insns(site, len, start_ud2a, end_ud2a);
    

This will actually give corrupted BUGs because you don't supply
the full inline BUG header. Perhaps another trap would be better.
  

The BUG handler will still report it as a normal illegal instruction. 
It should never happen; the main thing is that it clearly points out
where the problem is (as opposed to jumping to a NULL pointer and
getting the unhelpful "oh, eip is zero" symptom).

quoted

+EXPORT_SYMBOL(paravirt_ops);
    

Definitely _GPL at least.
  

No, for the same reason as i386.

quoted

+extern struct paravirt_ops paravirt_ops;
    

Should be native_paravirt_ops I guess

  

No, because its the current set of pv_ops.  It starts all native, but it
is either completely or partially overwritten by hypervisor-specific ops.

quoted

+
+ * This generates an indirect call based on the operation type number.
    

The macros here don't
  

Yes, PARAVIRT_CALL does: "call *(paravirt_ops+%c[paravirt_typenum]*8);"

quoted

+			     : "=a"(f)
+			     : paravirt_type(save_fl),
+			       paravirt_clobber(CLBR_RAX)
+			     : "memory", "cc");
+	return f;
+}
+
+static inline void raw_local_irq_restore(unsigned long f)
+{
+	__asm__ __volatile__(paravirt_alt(PARAVIRT_CALL)
+			     :
+			     : "D" (f),
    

Have you investigated if a different input register generates better/smaller 
code? I would assume rdi to be usually used already for the caller's 
arguments so it will produce spilling

Similar for the rax return in the other functions.
  

This has to match the normal C calling convention though, doesn't it?

    J