Re: [PATCH 2/7] x86/alt: Clean up struct alt_instr and its users

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/18 17:18, Wei Liu wrote:
> On Mon, Feb 12, 2018 at 04:52:21PM +0000, Roger Pau Monné wrote:
>> On Mon, Feb 12, 2018 at 11:23:02AM +0000, Andrew Cooper wrote:
>>>  * Rename some fields for consistency and clarity, and use standard types.
>>>  * Don't opencode the use of ALT_{ORIG,REPL}_PTR().
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  xen/arch/x86/alternative.c        | 24 ++++++++++++------------
>>>  xen/include/asm-x86/alternative.h | 12 ++++++------
>>>  2 files changed, 18 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
>>> index 5c8b6f6..f8ddab5 100644
>>> --- a/xen/arch/x86/alternative.c
>>> +++ b/xen/arch/x86/alternative.c
>>> @@ -163,8 +163,6 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>>                                            const struct alt_instr *end)
>>>  {
>>>      const struct alt_instr *a;
>>> -    u8 *instr, *replacement;
>>> -    u8 insnbuf[MAX_PATCH_LEN];
>>>  
>>>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>>>  
>>> @@ -179,23 +177,25 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>>>       */
>>>      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));
>>> +        uint8_t *orig = ALT_ORIG_PTR(a);
>>> +        uint8_t *repl = ALT_REPL_PTR(a);
>>> +        uint8_t buf[MAX_PATCH_LEN];
>>> +
>>> +        BUG_ON(a->repl_len > a->orig_len);
>>> +        BUG_ON(a->orig_len > sizeof(buf));
>> Would be nice to have an assembly BUILD_BUG_ON equivalent so that this
>> could be turned into a compile time check and added to ALTINSTR_ENTRY.
>  This function is also used to livepatch the hypervisor.
>
>  I would actually suggest not use BUG_ON here instead but I didn't want
>  to add noise to the problem at hand. :-)

We've got some assembler time checks already, and I've introduced more
in patch 5 which include this sizeof(buf) one.

As for BUG_ON()s, I considered dropping them as well, but the only thing
which will happen if these are skipped is that we will crash in less
obvious ways in the patch points.

~Andrew

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