Xen devel - Mar 2013 - [PATCH 16/18 V2]: PVH xen: elf changes to pref for dom0 PVH.

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);
     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;
+    }
     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)
 {
     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);
+}
+#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");
+}
+
 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

>>> On 16.03.13 at 02:04, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> +    /* 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(). */
Again - definitely not outside of an RFC patch.
> +    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;
> +    }
And with that I put under question the rest of the changes don in
this patch.

Jan

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
>

On Mon, 18 Mar 2013 12:43:33 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:
> >>> On 16.03.13 at 02:04, Mukesh Rathor
<mukesh.rathor@oracle.com>
> >>> wrote:
> > +    /* 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(). */
> 
> Again - definitely not outside of an RFC patch.
What, the "for now" comment, or the use of dbg_rw_mem()? There are
fixme''s in
xen already. dbg_rw_mem() is perfectly fine to use IMO, but in future we
may look at a faster copy, hence the comment.

Thanks,
M-

>>> On 19.03.13 at 02:13, Mukesh Rathor
<mukesh.rathor@oracle.com> wrote:
> On Mon, 18 Mar 2013 12:43:33 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 16.03.13 at 02:04, Mukesh Rathor
<mukesh.rathor@oracle.com>
>> >>> wrote:
>> > +    /* 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(). */
>> 
>> Again - definitely not outside of an RFC patch.
> 
> What, the "for now" comment, or the use of dbg_rw_mem()? There
are fixme''s
> in
> xen already. dbg_rw_mem() is perfectly fine to use IMO, but in future we
> may look at a faster copy, hence the comment.
No, this is a debugger interface, and should hence only be used
for that purpose. And actually I would have expected that
debugger code to only be built conditionally only anyway, the
more that we have a HAS_GDBSX construct. That not being the
case is no excuse imo, as much as pointing at other fixme
comments elsewhere isn''t.

The fundamental problem here is that you''d have to prove that
an interface that isn''t intended to be secure is now being used
in a way that doesn''t open security holes.

Jan