Re: [PATCH 1/4] public: Add page related definitions for accessing guests memory

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Hi Jan,

Thanks for your comments. I've just sent a v2.

Cheers,
Costin

On 8/10/21 9:41 AM, Jan Beulich wrote:
> On 09.08.2021 16:47, Costin Lupu wrote:
>> These changes introduce the page related definitions needed for mapping and
>> accessing guests memory. These values are intended to be used by any 
>> toolstack
>> component that needs to map guests memory. Until now, the values were defined
>> by the xenctrl.h header, therefore whenever a component had to use them it 
>> also
>> had to add a dependency for the xenctrl library.
>>
>> For this patch we set the same values for both x86 and ARM architectures.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>  xen/include/public/arch-arm/page.h | 34 ++++++++++++++++++++++++++
>>  xen/include/public/arch-x86/page.h | 34 ++++++++++++++++++++++++++
>>  xen/include/public/page.h          | 38 ++++++++++++++++++++++++++++++
>>  3 files changed, 106 insertions(+)
>>  create mode 100644 xen/include/public/arch-arm/page.h
>>  create mode 100644 xen/include/public/arch-x86/page.h
>>  create mode 100644 xen/include/public/page.h
> 
> I'm not convinced of these warranting introduction of new headers, but
> I'm also not meaning to say that I'm strictly opposed. I don't recall
> this aspect having had any consideration, yet.
> 
>> --- /dev/null
>> +++ b/xen/include/public/arch-arm/page.h
>> @@ -0,0 +1,34 @@
>> +/******************************************************************************
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory on ARM
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Copyright (c) 2021, Costin Lupu
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_ARCH_ARM_PAGE_H__
>> +#define __XEN_PUBLIC_ARCH_ARM_PAGE_H__
>> +
>> +#define XEN_PAGE_SHIFT           12
>> +#define XEN_PAGE_SIZE            (1UL << XEN_PAGE_SHIFT)
>> +#define XEN_PAGE_MASK            (~(XEN_PAGE_SIZE - 1))
> 
> The latter two, being identical ...
> 
>> --- /dev/null
>> +++ b/xen/include/public/arch-x86/page.h
>> @@ -0,0 +1,34 @@
>> +/******************************************************************************
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory on x86
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Copyright (c) 2021, Costin Lupu
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_ARCH_X86_PAGE_H__
>> +#define __XEN_PUBLIC_ARCH_X86_PAGE_H__
>> +
>> +#define XEN_PAGE_SHIFT           12
>> +#define XEN_PAGE_SIZE            (1UL << XEN_PAGE_SHIFT)
>> +#define XEN_PAGE_MASK            (~(XEN_PAGE_SIZE - 1))
> 
> ... not just for x86, but in general, should imo move ...
> 
>> --- /dev/null
>> +++ b/xen/include/public/page.h
>> @@ -0,0 +1,38 @@
>> +/******************************************************************************
>> + * page.h
>> + *
>> + * Page definitions for accessing guests memory
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a 
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> + * deal in the Software without restriction, including without limitation 
>> the
>> + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
>> and/or
>> + * sell copies of the Software, and to permit persons to whom the Software 
>> is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included 
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + *
>> + * Copyright (c) 2021, Costin Lupu
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_PAGE_H__
>> +#define __XEN_PUBLIC_PAGE_H__
>> +
>> +#if defined(__i386__) || defined(__x86_64__)
>> +#include "arch-x86/page.h"
>> +#elif defined (__arm__) || defined (__aarch64__)
>> +#include "arch-arm/page.h"
>> +#else
>> +#error "Unsupported architecture"
>> +#endif
> 
> ... here. I don't think though that 1UL is an appropriate construct
> to use: Imo the smallest type this should evaluate to is xen_ulong_t,
> the constant should also be usable in assembly sources, and it would
> better also suitably sign-extend when used in e.g. XEN_PAGE_MASK.
> 
> Jan
> 
>> +#endif /* __XEN_PUBLIC_PAGE_H__ */
>>
>