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

[Jan Beulich <jbeulich@xxxxxxxx>]

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__ */
>