Messages in this thread | | | Date | Thu, 29 Jan 2009 01:26:53 -0800 | From | Jeremy Fitzhardinge <> | Subject | Re: [PATCH 2 of 7] x86/pvops: add a paravirt_ident functions to allow special patching |
| |
Rusty Russell wrote: > On Thursday 29 January 2009 09:05:02 Jeremy Fitzhardinge wrote: > >> void _paravirt_nop(void); >> +u32 _paravirt_ident_32(u32); >> +u64 _paravirt_ident_64(u64); >> + >> #define paravirt_nop ((void *)_paravirt_nop) >> > > So, we used a void * cast for the paravirt_nop case, but you decided to use explicit types for the ident cases? >
Yes. Partly because any nop function is going to be basically equivalent to (void (*)(void)), but the concrete types for the ident function will vary (pointer, scalar, structure, etc).
>> if (opfunc == NULL) >> /* If there's no function, patch it with a ud2a (BUG) */ >> ret = paravirt_patch_insns(insnbuf, len, ud2a, ud2a+sizeof(ud2a)); >> - else if (opfunc == paravirt_nop) >> + else if (opfunc == _paravirt_nop) >> /* If the operation is a nop, then nop the callsite */ >> ret = paravirt_patch_nop(); >> > > Gratuitous change? >
To make it consistent with the newly added lines following. And its slightly more correct.
> >> +typedef pte_t make_pte_t(pteval_t); >> +typedef pmd_t make_pmd_t(pmdval_t); >> +typedef pud_t make_pud_t(pudval_t); >> +typedef pgd_t make_pgd_t(pgdval_t); >> + >> +typedef pteval_t pte_val_t(pte_t); >> +typedef pmdval_t pmd_val_t(pmd_t); >> +typedef pudval_t pud_val_t(pud_t); >> +typedef pgdval_t pgd_val_t(pgd_t); >> + >> + >> +#if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE) >> +/* 32-bit pagetable entries */ >> +#define paravirt_native_make_pte (make_pte_t *)_paravirt_ident_32 >> +#define paravirt_native_pte_val (pte_val_t *)_paravirt_ident_32 >> + >> +#define paravirt_native_make_pmd (make_pmd_t *)_paravirt_ident_32 >> +#define paravirt_native_pmd_val (pmd_val_t *)_paravirt_ident_32 >> + >> +#define paravirt_native_make_pud (make_pud_t *)_paravirt_ident_32 >> +#define paravirt_native_pud_val (pud_val_t *)_paravirt_ident_32 >> + >> +#define paravirt_native_make_pgd (make_pgd_t *)_paravirt_ident_32 >> +#define paravirt_native_pgd_val (pgd_val_t *)_paravirt_ident_32 >> +#else >> +/* 64-bit pagetable entries */ >> +#define paravirt_native_make_pte (make_pte_t *)_paravirt_ident_64 >> +#define paravirt_native_pte_val (pte_val_t *)_paravirt_ident_64 >> + >> +#define paravirt_native_make_pmd (make_pmd_t *)_paravirt_ident_64 >> +#define paravirt_native_pmd_val (pmd_val_t *)_paravirt_ident_64 >> + >> +#define paravirt_native_make_pud (make_pud_t *)_paravirt_ident_64 >> +#define paravirt_native_pud_val (pud_val_t *)_paravirt_ident_64 >> + >> +#define paravirt_native_make_pgd (make_pgd_t *)_paravirt_ident_64 >> +#define paravirt_native_pgd_val (pgd_val_t *)_paravirt_ident_64 >> +#endif >> > > I think I prefer: > > /* make_pte etc and pgd_val etc are identity functions. */ > #define paravirt_native_page_op \ > (sizeof(pte_t) == sizeof(u64) ? paravirt_ident_64 : paravirt_ident_32) > > Then use that everywhere rather than these defines? >
They disappear later in the series anyway.
> But it's a minor point; the code seems perfectly sound. >
Great, thanks for reviewing.
J
| |