Re: [Xen-devel] [PATCH v4 39/39] arm/xen-access: Add test of xc_altp2m_change_gfn

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 08/30/2017 09:32 PM, Sergej Proskurin wrote:
> This commit extends xen-access by a simple test of the functionality
> provided by "xc_altp2m_change_gfn". The idea is to dynamically remap a
> trapping gfn to another mfn, which holds the same content as the
> original mfn. On success, the guest will continue to run. Subsequent
> altp2m access violations will trap into Xen and be forced by xen-access
> to switch to the default view (altp2m[0]) as before. The introduced test
> can be invoked by providing the argument "altp2m_remap".
> 
> Signed-off-by: Sergej Proskurin <proskurin@xxxxxxxxxxxxx>
> ---
> Cc: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
> Cc: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> v3: Cosmetic fixes in "xenaccess_copy_gfn" and "xenaccess_change_gfn".
> 
>     Added munmap in "copy_gfn" in the second error case.
> 
>     Added option "altp2m_remap" selecting the altp2m-remap test.
> 
> v4: Dropped the COMPAT API for mapping foreign memory. Instead, we use the
>     stable library xenforeignmemory.
> 
>     Dropped the use of xc_domain_increase_reservation_exact as we do not
>     need to increase the domain's physical memory. Otherwise, remapping
>     a page via altp2m would become visible to the guest itself. As long
>     as we have additional shadow-memory for the guest domain, we do not
>     need to reserve any additional memory.
> ---
>  tools/tests/xen-access/Makefile     |   2 +-
>  tools/tests/xen-access/xen-access.c | 182 
> +++++++++++++++++++++++++++++++++++-
>  2 files changed, 179 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/tests/xen-access/Makefile b/tools/tests/xen-access/Makefile
> index e11f639ccf..ab195e233f 100644
> --- a/tools/tests/xen-access/Makefile
> +++ b/tools/tests/xen-access/Makefile
> @@ -26,6 +26,6 @@ clean:
>  distclean: clean
>  
>  xen-access: xen-access.o Makefile
> -     $(CC) -o $@ $< $(LDFLAGS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) 
> $(LDLIBS_libxenevtchn)
> +     $(CC) -o $@ $< $(LDFLAGS) $(LDLIBS_libxenctrl) $(LDLIBS_libxenguest) 
> $(LDLIBS_libxenevtchn) $(LDLIBS_libxenforeignmemory)
>  
>  -include $(DEPS)
> diff --git a/tools/tests/xen-access/xen-access.c 
> b/tools/tests/xen-access/xen-access.c
> index 481337cacd..f9b9fb6bbf 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -41,6 +41,7 @@
>  #include <xenctrl.h>
>  #include <xenevtchn.h>
>  #include <xen/vm_event.h>
> +#include <xenforeignmemory.h>
>  
>  #if defined(__arm__) || defined(__aarch64__)
>  #include <xen/arch-arm.h>
> @@ -49,6 +50,8 @@
>  #define START_PFN 0ULL
>  #endif
>  
> +#define INVALID_GFN ~(0UL)
> +
>  #define DPRINTF(a, b...) fprintf(stderr, a, ## b)
>  #define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
>  #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
> @@ -76,12 +79,19 @@ typedef struct vm_event {
>  typedef struct xenaccess {
>      xc_interface *xc_handle;
>  
> +    xenforeignmemory_handle *fmem;
> +
>      xen_pfn_t max_gpfn;
>  
>      vm_event_t vm_event;
> +
> +    unsigned int ap2m_idx;
> +    xen_pfn_t gfn_old;
> +    xen_pfn_t gfn_new;
>  } xenaccess_t;
>  
>  static int interrupted;
> +static int gfn_changed = 0;
>  bool evtchn_bind = 0, evtchn_open = 0, mem_access_enable = 0;
>  
>  static void close_handler(int sig)
> @@ -89,6 +99,104 @@ static void close_handler(int sig)
>      interrupted = sig;
>  }
>  
> +static int xenaccess_copy_gfn(xenaccess_t *xenaccess,
> +                              domid_t domain_id,
> +                              xen_pfn_t dst_gfn,
> +                              xen_pfn_t src_gfn)
> +{
> +    void *src_vaddr = NULL;
> +    void *dst_vaddr = NULL;
> +
> +    src_vaddr = xenforeignmemory_map(xenaccess->fmem, domain_id, PROT_READ,
> +                                     1, &src_gfn, NULL);
> +    if ( src_vaddr == NULL )
> +        return -1;
> +
> +    dst_vaddr = xenforeignmemory_map(xenaccess->fmem, domain_id, PROT_WRITE,
> +                                     1, &dst_gfn, NULL> +    if ( dst_vaddr 
> == NULL )
> +    {
> +        munmap(src_vaddr, XC_PAGE_SIZE);
> +        return -1;
> +    }
> +
> +    memcpy(dst_vaddr, src_vaddr, XC_PAGE_SIZE);
> +
> +    xenforeignmemory_unmap(xenaccess->fmem, src_vaddr, 1);
> +    xenforeignmemory_unmap(xenaccess->fmem, dst_vaddr, 1);
> +
> +    return 0;
> +}
> +
> +/*
> + * This function allocates and populates a page in the guest's physmap that 
> is
> + * subsequently filled with contents of the trapping address. Finally, 
> through
> + * the invocation of xc_altp2m_change_gfn, the altp2m subsystem changes the 
> gfn
> + * to mfn mapping of the target altp2m view.
> + */
> +static int xenaccess_change_gfn(xenaccess_t *xenaccess,
> +                                domid_t domain_id,
> +                                unsigned int ap2m_idx,
> +                                xen_pfn_t gfn_old,
> +                                xen_pfn_t *gfn_new)
> +{
> +    int rc;
> +
> +    /*
> +     * We perform this function only once as it is intended to be used for
> +     * testing and demonstration purposes. Thus, we signalize that further
> +     * altp2m-related traps will not change trapping gfn's.
> +     */
> +    gfn_changed = 1;
> +
> +    *gfn_new = ++(xenaccess->max_gpfn);

Unnecessary parentheses.

> +    rc = xc_domain_populate_physmap_exact(xenaccess->xc_handle, domain_id, 
> 1, 0, 0, gfn_new);
> +    if ( rc < 0 )
> +        goto err;
> +
> +    /* Copy content of the old gfn into the newly allocated gfn */
> +    rc = xenaccess_copy_gfn(xenaccess, domain_id, *gfn_new, gfn_old);
> +    if ( rc < 0 )
> +        goto err;
> +
> +    rc = xc_altp2m_change_gfn(xenaccess->xc_handle, domain_id, ap2m_idx, 
> gfn_old, *gfn_new);
> +    if ( rc < 0 )
> +        goto err;
> +
> +    return 0;
> +
> +err:
> +    xc_domain_decrease_reservation_exact(xenaccess->xc_handle, domain_id, 1, 
> 0, gfn_new);
> +
> +    (xenaccess->max_gpfn)--;

Here too.

> +
> +    return -1;
> +}
> +
> +static int xenaccess_reset_gfn(xc_interface *xch,
> +                               domid_t domain_id,
> +                               unsigned int ap2m_idx,
> +                               xen_pfn_t gfn_old,
> +                               xen_pfn_t gfn_new)
> +{
> +    int rc;
> +
> +    /* Reset previous state */
> +    xc_altp2m_change_gfn(xch, domain_id, ap2m_idx, gfn_old, INVALID_GFN);
> +
> +    /* Invalidate the new gfn */
> +    xc_altp2m_change_gfn(xch, domain_id, ap2m_idx, gfn_new, INVALID_GFN);

Do these two xc_altp2m_change_gfn() calls not require error checking?

> +
> +    rc = xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, 
> &gfn_new);
> +    if ( rc < 0 )
> +        return -1;
> +
> +    (xenaccess->max_gpfn)--;

Again, please remove the parentheses.


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel