From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Jan Beulich" <JBeulich@suse.com>
Subject: Re: [PATCH 2/3] x86: Port the basic alternative
 mechanism from Linux to Xen
Date: Tue, 27 May 2014 16:35:10 +0100
Message-ID: <5384CCCE0200007800016027@mail.emea.novell.com>
References: <1401089273-16425-1-git-send-email-feng.wu@intel.com>
	<1401089273-16425-3-git-send-email-feng.wu@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1401089273-16425-3-git-send-email-feng.wu@intel.com>
Content-Disposition: inline
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Feng Wu <feng.wu@intel.com>
Cc: keir.xen@gmail.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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