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

[Glauber de Oliveira Costa]

On 8/8/07, Andi Kleen <ak@xxxxxxx> wrote:
>
> Is this really synced with the latest version of the i386 code?
Roasted already commented on this. I will check out and change it here.

>
> > +#ifdef CONFIG_PARAVIRT
> > +#include <asm/paravirt.h>
> > +#endif
>
>
> > +#include <linux/errno.h>
> > +#include <linux/module.h>
> > +#include <linux/efi.h>
> > +#include <linux/bcd.h>
> > +#include <linux/start_kernel.h>
> > +
> > +#include <asm/bug.h>
> > +#include <asm/paravirt.h>
> > +#include <asm/desc.h>
> > +#include <asm/setup.h>
> > +#include <asm/irq.h>
> > +#include <asm/delay.h>
> > +#include <asm/fixmap.h>
> > +#include <asm/apic.h>
> > +#include <asm/tlbflush.h>
> > +#include <asm/msr.h>
> > +#include <asm/page.h>
> > +#include <asm/pgtable.h>
> > +#include <asm/proto.h>
> > +#include <asm/e820.h>
> > +#include <asm/time.h>
> > +#include <asm/asm-offsets.h>
> > +#include <asm/smp.h>
> > +#include <asm/irqflags.h>
>
>
> Are the includes really all needed?
delay.h is not needed anymore. Most of them, could be maybe moved to
paravirt.c , which is the one that really needs all the native_
things. Yeah, it will be better code this way, will change.

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

You mean this:
> > +#include <asm/bug.h>
?

>
> > +EXPORT_SYMBOL(paravirt_ops);
>
> Definitely _GPL at least.
Sure.

>
> Should be native_paravirt_ops I guess

makes sense.

> > +
> > + * This generates an indirect call based on the operation type number.
>
> The macros here don't
>

> > +static inline unsigned long read_msr(unsigned int msr)
> > +{
> > +     int __err;
>
> No need for __ in inlines
Right. Thanks.


> > +/* The paravirtualized I/O functions */
> > +static inline void slow_down_io(void) {
>
> I doubt this needs to be inline and it's large
In a second look, i386 have such a function in io.h because they need
slow_down_io in a bunch of I/O instructions. It seems that we do not.
Could we just get rid of it, then?

> > +     __asm__ __volatile__(paravirt_alt(PARAVIRT_CALL)
>
> No __*__ in new code please

Yup, will fix.

> > +                          : "=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.
I don't think we can do different. These functions can be patched, and
if it happens, they will put their return value in rax. So we'd better
expect it there.
Same goes for rdi, as they will expect the value to be there as an input.

I don't think it will spill in the normal case, as rdi is already the
parameter. So the compiler will just leave it there, untouched.

-- 
Glauber de Oliveira Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/