Re: [PATCH v4 4/7] x86/mm: put HVM only code under CONFIG_HVM

[Tamas K Lengyel <tamas@tklengyel.com>]

On Thu, Sep 13, 2018 at 10:38 AM Wei Liu <wei.liu2@citrix.com> wrote:
>
> Going through the code, HAP, EPT, PoD and ALTP2M depend on HVM code.
> Put these components under CONFIG_HVM. This further requires putting
> one of the vm event under CONFIG_HVM.
>
> Altp2m requires a bit more attention because its code is embedded in
> generic x86 p2m code.
>
> Also make hap_enabled evaluate to false when !CONFIG_HVM. Make sure it
> evaluate its parameter to avoid unused variable warnings in its users.
>
> Also sort items in Makefile while at it.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
> v4: provide stub for p2m_altp2m_check
>
> Razvan's ack is dropped because of the change. An ack from altp2m
> maintainers is required.
> ---
>  xen/arch/x86/mm/Makefile         | 11 ++++++-----
>  xen/arch/x86/mm/mem_access.c     | 18 +++++++++++++++++-
>  xen/arch/x86/mm/mem_sharing.c    |  2 ++
>  xen/arch/x86/mm/p2m.c            | 23 ++++++++++++-----------
>  xen/include/asm-x86/altp2m.h     | 13 ++++++++++++-
>  xen/include/asm-x86/domain.h     |  2 +-
>  xen/include/asm-x86/hvm/domain.h |  4 ++++
>  xen/include/asm-x86/p2m.h        |  8 +++++++-
>  8 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
> index 3017119..171cc74 100644
> --- a/xen/arch/x86/mm/Makefile
> +++ b/xen/arch/x86/mm/Makefile
> @@ -1,15 +1,16 @@
>  subdir-y += shadow
> -subdir-y += hap
> +subdir-$(CONFIG_HVM) += hap
>
> -obj-y += paging.o
> -obj-y += p2m.o p2m-pt.o p2m-ept.o p2m-pod.o
> -obj-y += altp2m.o
> +obj-$(CONFIG_HVM) += altp2m.o
>  obj-y += guest_walk_2.o
>  obj-y += guest_walk_3.o
>  obj-y += guest_walk_4.o
> +obj-$(CONFIG_MEM_ACCESS) += mem_access.o
>  obj-y += mem_paging.o
>  obj-y += mem_sharing.o
> -obj-y += mem_access.o
> +obj-y += p2m.o p2m-pt.o
> +obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o
> +obj-y += paging.o
>
>  guest_walk_%.o: guest_walk.c Makefile
>         $(CC) $(CFLAGS) -DGUEST_PAGING_LEVELS=$* -c $< -o $@
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index c980f17..6801841 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -246,7 +246,6 @@ bool p2m_mem_access_check(paddr_t gpa, unsigned long gla,
>      /* Return whether vCPU pause is required (aka. sync event) */
>      return (p2ma != p2m_access_n2rwx);
>  }
> -#endif
>
>  int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>                                struct p2m_domain *ap2m, p2m_access_t a,
> @@ -291,6 +290,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
>       */
>      return ap2m->set_entry(ap2m, gfn, mfn, PAGE_ORDER_4K, t, a, -1);
>  }
> +#endif
>
>  static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
>                            struct p2m_domain *ap2m, p2m_access_t a,
> @@ -298,6 +298,7 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
>  {
>      int rc = 0;
>
> +#ifdef CONFIG_HVM
>      if ( ap2m )
>      {
>          rc = p2m_set_altp2m_mem_access(d, p2m, ap2m, a, gfn);
> @@ -306,6 +307,9 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
>              rc = 0;
>      }
>      else
> +#else
> +    ASSERT(!ap2m);
> +#endif
>      {
>          mfn_t mfn;
>          p2m_access_t _a;
> @@ -367,6 +371,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>      long rc = 0;
>
>      /* altp2m view 0 is treated as the hostp2m */
> +#ifdef CONFIG_HVM
>      if ( altp2m_idx )
>      {
>          if ( altp2m_idx >= MAX_ALTP2M ||
> @@ -375,6 +380,9 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>
>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>      }
> +#else
> +    ASSERT(!altp2m_idx);
> +#endif
>
>      if ( !xenmem_access_to_p2m_access(p2m, access, &a) )
>          return -EINVAL;
> @@ -422,6 +430,7 @@ long p2m_set_mem_access_multi(struct domain *d,
>      long rc = 0;
>
>      /* altp2m view 0 is treated as the hostp2m */
> +#ifdef CONFIG_HVM
>      if ( altp2m_idx )
>      {
>          if ( altp2m_idx >= MAX_ALTP2M ||
> @@ -430,6 +439,9 @@ long p2m_set_mem_access_multi(struct domain *d,
>
>          ap2m = d->arch.altp2m_p2m[altp2m_idx];
>      }
> +#else
> +    ASSERT(!altp2m_idx);
> +#endif
>
>      p2m_lock(p2m);
>      if ( ap2m )
> @@ -483,12 +495,15 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>
>  void arch_p2m_set_access_required(struct domain *d, bool access_required)
>  {
> +#ifdef CONFIG_HVM
>      unsigned int i;
> +#endif

Perhaps this would look a little nicer with a minor restructure so
that there are no two ifdefs within this function..

>
>      ASSERT(atomic_read(&d->pause_count));
>
>      p2m_get_hostp2m(d)->access_required = access_required;
>
> +#ifdef CONFIG_HVM
>      if ( !altp2m_active(d) )

.. by changing this is into if ( altp2m_active(d) ) and moving the
unsigned int i declaration afterwards. I understand however if you
want to keep this patch mechanical.

>          return;
>
> @@ -499,6 +514,7 @@ void arch_p2m_set_access_required(struct domain *d, bool access_required)
>          if ( p2m )
>              p2m->access_required = access_required;
>      }
> +#endif
>  }

So with or without that change:
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel