Re: [PATCH 07/10] mini-os: move x86 specific gnttab coding into arch/x86/gnttab.c

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

Juergen Gross, le lun. 06 déc. 2021 08:23:34 +0100, a ecrit:
> Having grant table code in arch/x86/mm.c seems wrong. Move it to the
> new file arch/x86/gnttab.c, especially as the amount of code is
> expected to grow further.
> 
> No functional change.

There is the __pte fix that you'd probably want to mention.

> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>

Reviewed-by: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>

> ---
>  arch/x86/gnttab.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++
>  arch/x86/mm.c     | 47 ----------------------------
>  2 files changed, 78 insertions(+), 47 deletions(-)
>  create mode 100644 arch/x86/gnttab.c
> 
> diff --git a/arch/x86/gnttab.c b/arch/x86/gnttab.c
> new file mode 100644
> index 0000000..56e59d7
> --- /dev/null
> +++ b/arch/x86/gnttab.c
> @@ -0,0 +1,78 @@
> +/* -*-  Mode:C; c-basic-offset:4; tab-width:4 -*-
> + *
> + * (C) 2021 - Juergen Gross, SUSE Software Solutions Germany GmbH
> + *
> + * 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.
> + */
> +
> +#include <mini-os/os.h>
> +#include <mini-os/hypervisor.h>
> +#include <mini-os/gnttab.h>
> +#include <mini-os/mm.h>
> +#include <mini-os/types.h>
> +
> +grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
> +{
> +    struct gnttab_setup_table setup;
> +    unsigned long frames[nr_grant_frames];
> +
> +    setup.dom = DOMID_SELF;
> +    setup.nr_frames = nr_grant_frames;
> +    set_xen_guest_handle(setup.frame_list, frames);
> +
> +    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> +    return map_frames(frames, nr_grant_frames);
> +}
> +
> +void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> +{
> +#ifdef CONFIG_PARAVIRT
> +    int i;
> +
> +    for ( i = 0; i < nr_grant_frames; i++ )
> +    {
> +        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE 
> * i,
> +                __pte(0x0 << PAGE_SHIFT), UVMF_INVLPG);
> +    }
> +#endif
> +    return;
> +}
> +
> +void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> +{
> +    struct gnttab_setup_table setup;
> +    unsigned long frames[nr_grant_frames];
> +#ifdef CONFIG_PARAVIRT
> +    int i;
> +#endif
> +
> +    setup.dom = DOMID_SELF;
> +    setup.nr_frames = nr_grant_frames;
> +    set_xen_guest_handle(setup.frame_list, frames);
> +
> +    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> +
> +#ifdef CONFIG_PARAVIRT
> +    for ( i = 0; i < nr_grant_frames; i++ )
> +    {
> +        HYPERVISOR_update_va_mapping((unsigned long)gnttab_table + PAGE_SIZE 
> * i,
> +                __pte((frames[i] << PAGE_SHIFT) | L1_PROT), UVMF_INVLPG);
> +    }
> +#endif
> +}
> diff --git a/arch/x86/mm.c b/arch/x86/mm.c
> index c30d8bc..220c0b4 100644
> --- a/arch/x86/mm.c
> +++ b/arch/x86/mm.c
> @@ -837,53 +837,6 @@ void arch_init_mm(unsigned long* start_pfn_p, unsigned 
> long* max_pfn_p)
>  #endif
>  }
>  
> -grant_entry_v1_t *arch_init_gnttab(int nr_grant_frames)
> -{
> -    struct gnttab_setup_table setup;
> -    unsigned long frames[nr_grant_frames];
> -
> -    setup.dom = DOMID_SELF;
> -    setup.nr_frames = nr_grant_frames;
> -    set_xen_guest_handle(setup.frame_list, frames);
> -
> -    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> -    return map_frames(frames, nr_grant_frames);
> -}
> -
> -void arch_suspend_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> -{
> -#ifdef CONFIG_PARAVIRT
> -    int i;
> -
> -    for (i = 0; i < nr_grant_frames; i++) {
> -        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) 
> + PAGE_SIZE * i),
> -                (pte_t){0x0<<PAGE_SHIFT}, UVMF_INVLPG);
> -    }
> -#endif
> -    return;
> -}
> -
> -void arch_resume_gnttab(grant_entry_v1_t *gnttab_table, int nr_grant_frames)
> -{
> -    struct gnttab_setup_table setup;
> -    unsigned long frames[nr_grant_frames];
> -#ifdef CONFIG_PARAVIRT
> -    int i;
> -#endif
> -    setup.dom = DOMID_SELF;
> -    setup.nr_frames = nr_grant_frames;
> -    set_xen_guest_handle(setup.frame_list, frames);
> -
> -    HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &setup, 1);
> -
> -#ifdef CONFIG_PARAVIRT
> -    for (i = 0; i < nr_grant_frames; i++) {
> -        HYPERVISOR_update_va_mapping((unsigned long)(((char *)gnttab_table) 
> + PAGE_SIZE * i),
> -                (pte_t){(frames[i] << PAGE_SHIFT) | L1_PROT}, UVMF_INVLPG);
> -    }
> -#endif
> -}
> -
>  unsigned long alloc_virt_kernel(unsigned n_pages)
>  {
>      unsigned long addr;
> -- 
> 2.26.2
> 

-- 
Samuel
War doesn't prove who's right, just who's left.