Re: [Minios-devel] [UNIKRAFT PATCH v2 1/3] include/xen-x86/os.h: Remove os.h on x86

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Radu,

Thanks for this cleanup. Since we agreed offline there will be a v3, I
have a couple of comments, both of them are nitpicking. Please change
the subject to: 'include/xen-x86: Remove os.h on x86'. The other comment
is inline.

On 10/24/18 4:30 PM, radunicolau102@xxxxxxxxx wrote:
> From: Radu Nicolau <radunicolau102@xxxxxxxxx>
> 
> Remove os.h from the project as it was unused.
> mm_pv.h included os.h but did not use anything from
> it. The rest of the changes have been made in sources
> which need the extern definition for the shared info
> page.
> 
> Signed-off-by: Radu Nicolau <radunicolau102@xxxxxxxxx>
> ---
>  plat/xen/gnttab.c                |  2 +-
>  plat/xen/include/xen-x86/mm_pv.h |  1 -
>  plat/xen/include/xen-x86/os.h    | 90 
> ----------------------------------------
>  plat/xen/lcpu.c                  |  1 -
>  plat/xen/x86/mm.c                |  1 +
>  plat/xen/xenbus/xs_comms.c       |  1 +
>  6 files changed, 3 insertions(+), 93 deletions(-)
>  delete mode 100644 plat/xen/include/xen-x86/os.h
> 
> diff --git a/plat/xen/gnttab.c b/plat/xen/gnttab.c
> index ddfff99..b6a8fea 100644
> --- a/plat/xen/gnttab.c
> +++ b/plat/xen/gnttab.c
> @@ -26,7 +26,7 @@
>  #include <uk/semaphore.h>
>  #include <common/gnttab.h>
>  #include <xen-x86/mm.h>
> -
> +#include <xen-x86/hypercall.h>

Nitpicking: please leave the 2 white lines distance between the includes
and the rest of definitions. The reason is to minimize the patch size
(or the number of line in the diff).

>  
>  /* NR_GRANT_FRAMES must be less than or equal to that configured in Xen */
>  #define NR_GRANT_FRAMES         4
> diff --git a/plat/xen/include/xen-x86/mm_pv.h 
> b/plat/xen/include/xen-x86/mm_pv.h
> index 21f9169..f78e62f 100644
> --- a/plat/xen/include/xen-x86/mm_pv.h
> +++ b/plat/xen/include/xen-x86/mm_pv.h
> @@ -24,7 +24,6 @@
>  #ifndef _MM_PV_H
>  #define _MM_PV_H
>  
> -#include <xen-x86/os.h>
>  #include <xen-x86/setup.h>
>  
>  #ifdef __x86_64__
> diff --git a/plat/xen/include/xen-x86/os.h b/plat/xen/include/xen-x86/os.h
> deleted file mode 100644
> index 308d91a..0000000
> --- a/plat/xen/include/xen-x86/os.h
> +++ /dev/null
> @@ -1,90 +0,0 @@
> -/* SPDX-License-Identifier: BSD-2-Clause */
> -/*
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - * 1. Redistributions of source code must retain the above copyright
> - *    notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - *    notice, this list of conditions and the following disclaimer in the
> - *    documentation and/or other materials provided with the distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> - * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> - * ARE DISCLAIMED.  IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE
> - * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> - * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> - * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> - * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> - * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> - * SUCH DAMAGE.
> - */
> -/* Taken from Mini-OS */
> -/******************************************************************************
> - * os.h
> - *
> - * random collection of macros and definition
> - */
> -
> -#ifndef _OS_H_
> -#define _OS_H_
> -
> -#include <xen-x86/smp.h>
> -
> -#include <stdint.h>
> -#include <stddef.h>
> -#include <limits.h>
> -#include <uk/arch/types.h>
> -#include <uk/arch/limits.h>
> -#include <uk/arch/time.h>
> -#include <xen/xen.h>
> -
> -#ifndef __ASSEMBLY__
> -
> -typedef __s64 quad_t;
> -typedef __u64 u_quad_t;
> -
> -typedef __sptr intptr_t;
> -typedef __uptr uintptr_t;
> -typedef __sptr ptrdiff_t;
> -
> -typedef unsigned char u_char;
> -typedef unsigned int u_int;
> -typedef unsigned long u_long;
> -
> -#include <xen-x86/hypercall.h>
> -
> -#include <xen/event_channel.h>
> -#include <xen/xsm/flask_op.h>
> -#endif
> -
> -#include <x86/cpu_defs.h>
> -
> -
> -#define LOCK_PREFIX ""
> -#define ADDR (*(volatile long *)addr)
> -
> -/* Everything below this point is not included by assembler (.S) files. */
> -#ifndef __ASSEMBLY__
> -
> -extern shared_info_t *HYPERVISOR_shared_info;
> -
> -#include <xen-x86/irq.h>
> -
> -
> -/*
> - * Make sure gcc doesn't try to be clever and move things around
> - * on us. We need to use _exactly_ the address the user gave us,
> - * not some alias that contains the same information.
> - */
> -typedef struct {
> -     volatile int counter;
> -} atomic_t;
> -
> -
> -void block_domain(__snsec until);
> -
> -#endif /* not assembly */
> -#endif /* _OS_H_ */
> diff --git a/plat/xen/lcpu.c b/plat/xen/lcpu.c
> index f0650da..d7158df 100644
> --- a/plat/xen/lcpu.c
> +++ b/plat/xen/lcpu.c
> @@ -34,7 +34,6 @@
>  
>  #include <stdint.h>
>  #if defined(__X86_32__) || defined(__x86_64__)
> -#include <xen-x86/os.h>
>  #include <xen-x86/irq.h>
>  #elif (defined __ARM_32__) || (defined __ARM_64__)
>  #include <xen-arm/os.h>
> diff --git a/plat/xen/x86/mm.c b/plat/xen/x86/mm.c
> index b7b080e..b89384f 100644
> --- a/plat/xen/x86/mm.c
> +++ b/plat/xen/x86/mm.c
> @@ -39,6 +39,7 @@
>  #include <errno.h>
>  #include <uk/alloc.h>
>  #include <uk/plat/config.h>
> +#include <common/hypervisor.h>
>  #include <xen-x86/mm.h>
>  #include <xen/memory.h>
>  #include <uk/print.h>
> diff --git a/plat/xen/xenbus/xs_comms.c b/plat/xen/xenbus/xs_comms.c
> index 274e44b..0589d48 100644
> --- a/plat/xen/xenbus/xs_comms.c
> +++ b/plat/xen/xenbus/xs_comms.c
> @@ -46,6 +46,7 @@
>  #include <uk/wait.h>
>  #include <uk/arch/spinlock.h>
>  #include <common/events.h>
> +#include <common/hypervisor.h>
>  #include <xen-x86/mm.h>
>  #include <xen-x86/setup.h>
>  #include <xenbus/client.h>
> 

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