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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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