All of lore.kernel.org
 help / color / mirror / Atom feed
From: Julien Grall <julien.grall@arm.com>
To: Wei Chen <Wei.Chen@arm.com>,
	"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
Cc: Kaly Xin <Kaly.Xin@arm.com>, nd <nd@arm.com>,
	Steve Capper <Steve.Capper@arm.com>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>
Subject: Re: [PATCH v2] xen/arm32: Introduce alternative runtime patching
Date: Wed, 29 Mar 2017 15:06:55 +0100	[thread overview]
Message-ID: <4f0c6fbe-2f7b-649e-30eb-22870b2c8d1a@arm.com> (raw)
In-Reply-To: <HE1PR08MB058717E361ECA59EA0141F1D9E350@HE1PR08MB0587.eurprd08.prod.outlook.com>



On 29/03/17 10:28, Wei Chen wrote:
> Hi Julien,
>
> On 2017/3/29 16:40, Julien Grall wrote:
>> Hi Wei,
>>
>> On 28/03/2017 08:23, Wei Chen wrote:
>>> diff --git a/xen/include/asm-arm/arm32/insn.h b/xen/include/asm-arm/arm32/insn.h
>>> new file mode 100644
>>> index 0000000..4cda69e
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/arm32/insn.h
>>> @@ -0,0 +1,65 @@
>>> +/*
>>> +  * Copyright (C) 2017 ARM Ltd.
>>> +  *
>>> +  * This program is free software; you can redistribute it and/or modify
>>> +  * it under the terms of the GNU General Public License version 2 as
>>> +  * published by the Free Software Foundation.
>>> +  *
>>> +  * This program is distributed in the hope that it will be useful,
>>> +  * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +  * GNU General Public License for more details.
>>> +  *
>>> +  * You should have received a copy of the GNU General Public License
>>> +  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> +  */
>>> +#ifndef __ARCH_ARM_ARM32_INSN
>>> +#define __ARCH_ARM_ARM32_INSN
>>> +
>>> +#include <xen/types.h>
>>> +
>>> +#define __AARCH32_INSN_FUNCS(abbr, mask, val)   \
>>> +static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
>>> +{                                                                 \
>>> +    return (code & (mask)) == (val);                              \
>>> +}
>>> +
>>> +/*
>>> + * From ARM DDI 0406C.c Section A8.8.18 and A8.8.25. We can see that
>>> + * unconditional blx and conditional b have the same value field and imm
>>> + * length. And from ARM DDI 0406C.c Section A5.7 Table A5-23, we can see
>>> + * that the blx is the only one unconditional instruction has the same
>>> + * value with conditional branch instructions. So we define the b and blx
>>> + * in the same macro to check them at the same time.
>>> + */
>>
>> I don't think this is true. The encodings are:
>>       - b   cccc1010xxxxxxxxxxxxxxxxxxxxxxxx
>>       - bl  cccc1011xxxxxxxxxxxxxxxxxxxxxxxx
>>       - blx 1111101Hxxxxxxxxxxxxxxxxxxxxxxxx
>>
>> where cccc != 0b1111. So both helpers (aarch32_insn_is_{b_or_blx,bl})
>> will recognize the blx instruction depending on the value of bit H.
>>
>
> I think I had made a misunderstanding of the H bit. I always thought
> the H bit in ARM instruction set is 0.

Because Xen is only using ARM instructions, blx will always have H = 0.
But this is not what you described in your comment.

>
>> That's why I suggested to introduce a new helper checking for blx.
>>
>
> I think that's not enough. Current macro will mask the conditional bits.
> So no matter what the value of H bit, the blx will be recognized in
> aarch32_insn_is_{b, bl}.
>
> I think we should update the __AARCH32_INSN_FUNCS to cover the cond
> bits.
>
> #define __UNCONDITIONAL_INSN(code)       (((code) >> 28) == 0xF)
>
> #define __AARCH32_INSN_FUNCS(abbr, mask, val)   \
> static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
> {                                                                 \
>      return !__UNCONDITIONAL_INSN(code) && (code & (mask)) == (val);   \
> }
>
> #define __AARCH32_UNCOND_INSN_FUNCS(abbr, mask, val)   \
> static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
> {                                                                 \
>      return __UNCONDITIONAL_INSN(code) && (code & (mask)) == (val);   \
> }
>
> __AARCH32_UNCOND_INSN_FUNCS(blx,  0x0E000000, 0x0A000000)

Looking at the code you aarch32_insn_is_* helpers are only used in 
aarch32_insn_is_branch_imm. So why don't you open-code the checks in the 
latter helper?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

  reply	other threads:[~2017-03-29 14:06 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-28  7:23 [PATCH v2] xen/arm32: Introduce alternative runtime patching Wei Chen
2017-03-29  8:40 ` Julien Grall
2017-03-29  9:28   ` Wei Chen
2017-03-29 14:06     ` Julien Grall [this message]
2017-03-30  2:01       ` Wei Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4f0c6fbe-2f7b-649e-30eb-22870b2c8d1a@arm.com \
    --to=julien.grall@arm.com \
    --cc=Kaly.Xin@arm.com \
    --cc=Steve.Capper@arm.com \
    --cc=Wei.Chen@arm.com \
    --cc=nd@arm.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xen.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.