Re: [PATCH 16/18 V2]: PVH xen: elf changes to pref for dom0 PVH.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Mar 15, 2013 at 06:04:40PM -0700, Mukesh Rathor wrote:
>  This patch prepares for dom0 PVH by making some changes in the elf
>  code; add a new parameter to indicate PVH dom0 and use different copy
>  function for PVH. Also, add check in iommu.c to check for iommu
>  enabled for dom0 PVH.
> 
> Changes in V2: None
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/domain_build.c       |    2 +-
>  xen/common/libelf/libelf-loader.c |   51 ++++++++++++++++++++++++++++++++++---
>  xen/drivers/passthrough/iommu.c   |   18 +++++++++++-
>  xen/include/xen/libelf.h          |    3 +-
>  4 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index c8f435d..8c5b27a 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -766,7 +766,7 @@ int __init construct_dom0(
>  
>      /* Copy the OS image and free temporary buffer. */
>      elf.dest = (void*)vkern_start;
> -    rc = elf_load_binary(&elf);
> +    rc = elf_load_binary(&elf, 0);

Huh? Don't we want it to check for something and make it 1 or so? Or is that in the next patch?

>      if ( rc < 0 )
>      {
>          printk("Failed to load the kernel binary\n");
> diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
> index 3cf9c59..d732f75 100644
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -17,6 +17,10 @@
>   */
>  
>  #include "libelf-private.h"
> +#ifdef __XEN__
> +#include <public/xen.h>
> +#include <asm/debugger.h>
> +#endif
>  
>  /* ------------------------------------------------------------------------ */
>  
> @@ -108,7 +112,8 @@ void elf_set_log(struct elf_binary *elf, elf_log_callback *log_callback,
>      elf->verbose = verbose;
>  }
>  
> -static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t memsz)
> +static int elf_load_image(void *dst, const void *src, uint64_t filesz, 
> +                          uint64_t memsz, int not_used)
>  {
>      memcpy(dst, src, filesz);
>      memset(dst + filesz, 0, memsz - filesz);
> @@ -122,11 +127,34 @@ void elf_set_verbose(struct elf_binary *elf)
>      elf->verbose = 1;
>  }
>  
> -static int elf_load_image(void *dst, const void *src, uint64_t filesz, uint64_t memsz)
> +static int elf_load_image(void *dst, const void *src, uint64_t filesz, 
> +                          uint64_t memsz, int is_pvh_dom0)
>  {
>      int rc;
>      if ( filesz > ULONG_MAX || memsz > ULONG_MAX )
>          return -1;


> +
> +    /* raw_copy_to_guest -> copy_to_user_hvm -> __hvm_copy needs curr to
> +     * point to the hvm/pvh vcpu. Hence for PVH dom0 we can't use that. For now
> +     * just use dbg_rw_mem(). */
> +    if ( is_pvh_dom0 ) 
> +    {
> +        int j, rem; 
> +        rem = dbg_rw_mem((dbgva_t)dst, (dbgbyte_t *)src, (int)filesz, 0, 1, 0);
> +        if ( rem ) {
> +            printk("Failed to copy elf binary. len:%ld rem:%d\n", filesz, rem);
> +            return -1;
> +        }
> +        for (j=0; j < memsz - filesz; j++) {
> +            unsigned char zero=0;
> +            rem = dbg_rw_mem((dbgva_t)(dst+filesz+j), &zero, 1, 0, 1, 0);
> +            if (rem) {
> +                printk("Failed to copy to:%p rem:%d\n", dst+filesz+j, rem);
> +                return -1;
> +            }
> +        }
> +        return 0;
> +    }


Ahem!? This is all debug code. This really should not be here.

>      rc = raw_copy_to_guest(dst, src, filesz);
>      if ( rc != 0 )
>          return -1;
> @@ -260,7 +288,9 @@ void elf_parse_binary(struct elf_binary *elf)
>              __FUNCTION__, elf->pstart, elf->pend);
>  }
>  
> -int elf_load_binary(struct elf_binary *elf)
> +/* This function called from the libraries when building guests, and also for
> + * dom0 from construct_dom0().   */
> +static int _elf_load_binary(struct elf_binary *elf, int is_pvh_dom0)

Could it be a 'bool'?

>  {
>      const elf_phdr *phdr;
>      uint64_t i, count, paddr, offset, filesz, memsz;
> @@ -279,7 +309,8 @@ int elf_load_binary(struct elf_binary *elf)
>          dest = elf_get_ptr(elf, paddr);
>          elf_msg(elf, "%s: phdr %" PRIu64 " at 0x%p -> 0x%p\n",
>                  __func__, i, dest, dest + filesz);
> -        if ( elf_load_image(dest, elf->image + offset, filesz, memsz) != 0 )
> +        if ( elf_load_image(dest, elf->image + offset, filesz, memsz, 
> +                            is_pvh_dom0) != 0 )
>              return -1;
>      }
>  
> @@ -287,6 +318,18 @@ int elf_load_binary(struct elf_binary *elf)
>      return 0;
>  }
>  
> +#ifdef __XEN__
> +int elf_load_binary(struct elf_binary *elf, int is_pvh_dom0)
> +{
> +    return _elf_load_binary(elf, is_pvh_dom0);
> +}
> +#else
> +int elf_load_binary(struct elf_binary *elf)
> +{
> +    return _elf_load_binary(elf, 0);

Please add a comment right after 0 saying /* Never dom0 */

> +}
> +#endif
> +
>  void *elf_get_ptr(struct elf_binary *elf, unsigned long addr)
>  {
>      return elf->dest + addr - elf->pstart;
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index c1d3c12..9954e07 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -125,15 +125,25 @@ int iommu_domain_init(struct domain *d)
>      return hd->platform_ops->init(d);
>  }
>  
> +static inline void check_dom0_pvh_reqs(struct domain *d)
> +{
> +    if (!iommu_enabled || iommu_passthrough)
> +        panic("For pvh dom0, iommu must be enabled, dom0-passthrough must "
> +              "not be enabled \n");
> +}


Could you make it a bit smarter? Say:
		panic("For PVH dom0, %s %s\n",
		      iommu_enabled ? "" : "iommu must be enabled",
		      !iommu_passthrough ? "" : "iommu=dom0-passthrought must NOT be enabled");

> +
>  void __init iommu_dom0_init(struct domain *d)
>  {
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>  
> +    if ( is_pvh_domain(d) )
> +        check_dom0_pvh_reqs(d);
> +
>      if ( !iommu_enabled )
>          return;
>  
>      register_keyhandler('o', &iommu_p2m_table);
> -    d->need_iommu = !!iommu_dom0_strict;
> +    d->need_iommu = is_pvh_domain(d) || !!iommu_dom0_strict;
>      if ( need_iommu(d) )
>      {
>          struct page_info *page;
> @@ -146,7 +156,11 @@ void __init iommu_dom0_init(struct domain *d)
>                   ((page->u.inuse.type_info & PGT_type_mask)
>                    == PGT_writable_page) )
>                  mapping |= IOMMUF_writable;
> -            hd->platform_ops->map_page(d, mfn, mfn, mapping);
> +            if ( is_pvh_domain(d) ) {
> +                unsigned long gfn = mfn_to_gfn(d, _mfn(mfn));
> +                hd->platform_ops->map_page(d, gfn, mfn, mapping);
> +            } else
> +                hd->platform_ops->map_page(d, mfn, mfn, mapping);
>              if ( !(i++ & 0xfffff) )
>                  process_pending_softirqs();
>          }
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index 218bb18..2dc2bdb 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -192,13 +192,14 @@ int elf_phdr_is_loadable(struct elf_binary *elf, const elf_phdr * phdr);
>  int elf_init(struct elf_binary *elf, const char *image, size_t size);
>  #ifdef __XEN__
>  void elf_set_verbose(struct elf_binary *elf);
> +int elf_load_binary(struct elf_binary *elf, int is_pvh_dom0);
>  #else
>  void elf_set_log(struct elf_binary *elf, elf_log_callback*,
>                   void *log_caller_pointer, int verbose);
> +int elf_load_binary(struct elf_binary *elf);
>  #endif
>  
>  void elf_parse_binary(struct elf_binary *elf);
> -int elf_load_binary(struct elf_binary *elf);
>  
>  void *elf_get_ptr(struct elf_binary *elf, unsigned long addr);
>  uint64_t elf_lookup_addr(struct elf_binary *elf, const char *symbol);
> -- 
> 1.7.2.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>