Re: [PATCH RFC REPOST 1/2] paravirt: refactor struct paravirt_ops into smaller pv_*_ops

[Rusty Russell <rusty@rustcorp.com.au>]

On Wednesday 10 October 2007 04:24:24 Jeremy Fitzhardinge wrote:
> [ I think this is a straight repost this patch, which addresses all the
> previous comments.  I'd like to submit this for .24 as the basis for a
> unified paravirt_ops.  Any objections? ]

Thanks Jeremy, I've actually taken time to finally review this in detail (I'm 
assuming you'll refactor as necessary after the x86 arch merger).

> +	OFFSET(PARAVIRT_enabled, pv_info, paravirt_enabled);

I think this gives the right answer for the wrong reasons?

> +struct paravirt_ops paravirt_ops;
> +

Do you actually need to define this?  See below...

> +DEF_NATIVE(, ud2a, "ud2a");

Hmm, that's ugly.  It was ugly before, but it's uglier now.  Maybe just 
use "unsigned char ud2a[] = { 0x0f, 0x0b };" in paravirt_patch_default?

>  }
>
>  struct paravirt_ops paravirt_ops = {
...
> +	.pv_info = {
> +		.name = "bare hardware",
> +		.paravirt_enabled = 0,
> +		.kernel_rpl = 0,
> +		.shared_kernel_pmd = 1,	/* Only used when CONFIG_X86_PAE is set */
> +	},

This is the bit I don't get.  Why not just declare struct pv_info pvinfo, etc, 
and use the declaration of struct paravirt_ops to get your unique 
offset-based identifiers for patching?

Rest looks fine...

Thanks!
Rusty.