Re: [Xen-devel] [PATCH RFC v1 24/74] x86/guest: Hypercall support

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/01/18 13:53, Jan Beulich wrote:
>>>> On 04.01.18 at 14:05, <wei.liu2@xxxxxxxxxx> wrote:
>> --- /dev/null
>> +++ b/xen/arch/x86/guest/hypercall_page.S
>> @@ -0,0 +1,79 @@
>> +#include <asm/page.h>
>> +#include <asm/asm_defns.h>
>> +#include <public/xen.h>
>> +
>> +        .section ".text.page_aligned", "ax", @progbits
>> +        .p2align PAGE_SHIFT
>> +
>> +GLOBAL(hypercall_page)
>> +         /* Poisoned with `ret` for safety before hypercalls are set up. */
>> +        .fill PAGE_SIZE, 1, 0xc3
> How is RET a useful poison value? Why not 0xcc?

This was all imported basically-verbatim from XTF (which also answers
some of your lower questions).

ret over cc prevents problems when crashing early.  Turning the
preferred schedop_shutdown() into a nop stop you taking a cascade fault,
and instead try a different shutdown mechanism.

Also, before my recent patch to fix int3 behaviour, Xen will happily
execute its way (slowly) through debug traps without printing anything
useful.

>
>> +        .type hypercall_page, STT_OBJECT
> I'd rather omit the type altogether - it's not really an object (nor a
> function), the more that you produce individual entry symbols
> below anyway.
>
>> +        .size hypercall_page, PAGE_SIZE
>> +
>> +/*
>> + * Identify a specific hypercall in the hypercall page
>> + * @param name Hypercall name.
>> + */
>> +#define DECLARE_HYPERCALL(name)                                             
>>     \
>> +        .globl HYPERCALL_ ## name;                                          
>>     \
>> +        .set   HYPERCALL_ ## name, hypercall_page + __HYPERVISOR_ ## name * 
>> 32; \
>> +        .type  HYPERCALL_ ## name, STT_FUNC;                                
>>     \
>> +        .size  HYPERCALL_ ## name, 32
> This is certainly fine for now, but going forward wants to be
> machine generated directly from the header, so that it won't
> need touching when new hypercalls are being added. Until
> then I wonder whether you really need all the entries you
> enumerate below - some (like iret) are plain invalid for PVH.
>
>> --- /dev/null
>> +++ b/xen/include/asm-x86/guest/hypercall.h
>> @@ -0,0 +1,92 @@
>> +/******************************************************************************
>> + * asm-x86/guest/hypercall.h
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms and conditions 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/>.
>> + *
>> + * Copyright (c) 2017 Citrix Systems Ltd.
>> + */
>> +
>> +#ifndef __X86_XEN_HYPERCALL_H__
>> +#define __X86_XEN_HYPERCALL_H__
>> +
>> +#ifdef CONFIG_XEN_GUEST
>> +
>> +/*
>> + * Hypercall primatives for 64bit
>> + *
>> + * Inputs: %rdi, %rsi, %rdx, %r10, %r8, %r9 (arguments 1-6)
>> + */
>> +
>> +#define _hypercall64_1(type, hcall, a1)                                 \
>> +    ({                                                                  \
>> +        long res, tmp;                                                  \
> Especially for tmp I think it would be quite a bit more safe if it
> had a trailing underscore attached, so that an occasional use
> of
>
>     _hypercall64_1(..., tmp);
>
> would work as intended.

Hmm.  I'd not even considered that issue.  I'll add it to my todo list.

~Andrew

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