Re: [PATCH 01/18] arm/altp2m: Add cmd-line support for altp2m on ARM.

From: Sergej Proskurin <proskurin@sec.in.tum.de>
To: Julien Grall <julien.grall@arm.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>
Subject: Re: [PATCH 01/18] arm/altp2m: Add cmd-line support for altp2m on ARM.
Date: Mon, 4 Jul 2016 15:43:09 +0200	[thread overview]
Message-ID: <c6e3f1a5-fff9-5dd4-5d12-78fd9e2ca5d2@sec.in.tum.de> (raw)
In-Reply-To: <577A63DA.60104@arm.com>

Hello Julien,

On 07/04/2016 03:25 PM, Julien Grall wrote:
> Hello Sergej,
> 
> On 04/07/16 12:45, Sergej Proskurin wrote:
>> The Xen altp2m subsystem is currently supported only on x86-64 based
>> architectures. By utilizing ARM's virtualization extensions, we intend
>> to implement altp2m support for ARM architectures and thus further
>> extend current Virtual Machine Introspection (VMI) capabilities on ARM.
>>
>> With this commit, Xen is now able to activate altp2m support on ARM by
>> means of the command-line argument 'altp2m' (bool).
>>
>> Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
>> ---
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/hvm.c            | 22 ++++++++++++++++++++
>>   xen/include/asm-arm/hvm/hvm.h | 47
>> +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+)
>>   create mode 100644 xen/include/asm-arm/hvm/hvm.h
>>
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index d999bde..3615036 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -32,6 +32,28 @@
>>
>>   #include <asm/hypercall.h>
>>
>> +#include <asm/hvm/hvm.h>
>> +
>> +/* Xen command-line option enabling altp2m */
>> +static bool_t __initdata opt_altp2m_enabled = 0;
>> +boolean_param("altp2m", opt_altp2m_enabled);
>> +
>> +struct hvm_function_table hvm_funcs __read_mostly = {
>> +    .name = "ARM_HVM",
>> +};
> 
> I don't see any reason to introduce hvm_function_table on ARM. This
> structure is used to know whether the hardware will support altp2m.
> However, based on your implementation, this feature will this will not
> depend on the hardware for ARM.
> 

This is true: hvm_function_table is not of crucial importance. During
the implementation, we decided to pull out arch-independent parts out
the x86 and ARM implementation (still needs to be done) and hence reused
as much code as possible. However, this struct can be left out.

>> +
>> +/* Initcall enabling hvm functionality. */
>> +static int __init hvm_enable(void)
>> +{
>> +    if ( opt_altp2m_enabled )
>> +        hvm_funcs.altp2m_supported = 1;
>> +    else
>> +        hvm_funcs.altp2m_supported = 0;
>> +
>> +    return 0;
>> +}
>> +presmp_initcall(hvm_enable);
>> +
>>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>>   {
>>       long rc = 0;
>> diff --git a/xen/include/asm-arm/hvm/hvm.h
>> b/xen/include/asm-arm/hvm/hvm.h
>> new file mode 100644
>> index 0000000..96c455c
>> --- /dev/null
>> +++ b/xen/include/asm-arm/hvm/hvm.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * include/asm-arm/hvm/hvm.h
>> + *
>> + * Copyright (c) 2016, Sergej Proskurin <proskurin@sec.in.tum.de>
>> + *
>> + * 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 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 __ASM_ARM_HVM_HVM_H__
>> +#define __ASM_ARM_HVM_HVM_H__
>> +
>> +struct hvm_function_table {
>> +    char *name;
>> +
>> +    /* Necessary hardware support for alternate p2m's. */
>> +    bool_t altp2m_supported;
>> +};
>> +
>> +extern struct hvm_function_table hvm_funcs;
>> +
>> +/* Returns true if hardware supports alternate p2m's */
> 
> This comment is not true. The feature does not depend on the hardware
> for ARM.
> 

True. I will change that.

>> +static inline bool_t hvm_altp2m_supported(void)
>> +{
>> +    return hvm_funcs.altp2m_supported;
> 
> You could directly use opt_altp2m_enabled here.
> 

Ok, thank you.

>> +}
>> +
>> +#endif /* __ASM_ARM_HVM_HVM_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Regards,
> 

Thank you.

Cheers, Sergej

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