Re: [PATCH 2/3] x86: Port the basic alternative mechanism from Linux to Xen

["Jan Beulich" <JBeulich@suse.com>]

>>> On 26.05.14 at 09:27, <feng.wu@intel.com> wrote:
> +void __init arch_init_ideal_nops(void)
> +{
> +    switch (boot_cpu_data.x86_vendor)
> +    {
> +    case X86_VENDOR_INTEL:
> +        /*
> +         * Due to a decoder implementation quirk, some
> +         * specific Intel CPUs actually perform better with
> +         * the "k8_nops" than with the SDM-recommended NOPs.
> +         */
> +        if ( boot_cpu_data.x86 == 6 &&
> +            boot_cpu_data.x86_model >= 0x0f &&
> +            boot_cpu_data.x86_model != 0x1c &&
> +            boot_cpu_data.x86_model != 0x26 &&
> +            boot_cpu_data.x86_model != 0x27 &&
> +            boot_cpu_data.x86_model < 0x30 )

Indentation.

> +static void __init add_nops(void *insns, unsigned int len)
> +{
> +    while ( len > 0 )
> +    {
> +        unsigned int noplen = len;
> +        if ( noplen > ASM_NOP_MAX )
> +                noplen = ASM_NOP_MAX;

Again.

> +static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +{
> +    struct alt_instr *a;
> +    u8 *instr, *replacement;
> +    u8 insnbuf[MAX_PATCH_LEN];
> +
> +    printk("%s: alt table %p -> %p\n", __func__, start, end);

This should be at most INFO level, and likely doesn't need the __func__
prefix.

> +    /*
> +     * The scan order should be from start to end. A later scanned
> +     * alternative code can overwrite a previous scanned alternative code.
> +     * Some kernel functions (e.g. memcpy, memset, etc) use this order to
> +     * patch code.
> +     *
> +     * So be careful if you want to change the scan order to any other
> +     * order.
> +     */
> +    for ( a = start; a < end; a++ )
> +    {
> +        instr = (u8 *)&a->instr_offset + a->instr_offset;
> +        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> +        BUG_ON(a->replacementlen > a->instrlen);
> +        BUG_ON(a->instrlen > sizeof(insnbuf));
> +        BUG_ON(a->cpuid >= NCAPINTS * 32);
> +        if ( !boot_cpu_has(a->cpuid) )
> +            continue;
> +
> +        memcpy(insnbuf, replacement, a->replacementlen);
> +
> +        /* 0xe8 is a relative jump; fix the offset. */
> +        if ( *insnbuf == 0xe8 && a->replacementlen == 5 )

So as said in the conversation with Andrew already, either fix the
comment or make the code match it (by masking the low bit).

Jan