Re: [Xen-devel] [PATCH 16/22] vixen: pass grant table operations through to the outer Xen

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Sat, Jan 06, 2018 at 02:54:31PM -0800, Anthony Liguori wrote:
> From: Anthony Liguori <aliguori@xxxxxxxxxx>
> 
> The grant table is a region of guest memory that contains GMFNs
> which in PV are MFNs but are PFNs in HVM.  Since a Vixen guest MFN
> is an HVM PFN, we can pass this table directly through to the outer
> Xen which cuts down considerably on overhead.
> 
> We do not forward most of the hypercalls since we only intend on
> Vixen to be used for normal guests, not driver domains.
> 
> Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>
> ---
>  xen/common/grant_table.c | 131 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 131 insertions(+)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 250450b..b302fd0 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -39,6 +39,7 @@
>  #include <xen/vmap.h>
>  #include <xsm/xsm.h>
>  #include <asm/flushtlb.h>
> +#include <asm/guest.h>
>  
>  /* Per-domain grant information. */
>  struct grant_table {
> @@ -1199,6 +1200,9 @@ gnttab_map_grant_ref(
>      int i;
>      struct gnttab_map_grant_ref op;
>  
> +    if ( is_vixen() )
> +        return -ENOSYS;

Here and below: instead of adding all those is_vixen calls in a bunch
of gnttab functions, why don't you just replace the whole
do_grant_table_op function? That's cleaner and less intrusive.

>  static long
> +vixen_gnttab_setup_table(
> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
> +{
> +    long rc;
> +
> +    struct gnttab_setup_table op;
> +    xen_pfn_t *frame_list = NULL;
> +    static void *grant_table;
> +    XEN_GUEST_HANDLE(xen_pfn_t) old_frame_list;
> +
> +    if ( count != 1 )
> +        return -EINVAL;
> +
> +    if ( unlikely(copy_from_guest(&op, uop, 1) != 0) )
> +    {
> +        gdprintk(XENLOG_INFO, "Fault while reading gnttab_setup_table_t.\n");
> +        return -EFAULT;
> +    }
> +
> +    if ( grant_table == NULL ) {
> +        struct xen_add_to_physmap xatp;
> +        struct domain *d;
> +        int i;
> +
> +        for ( i = 0; i < max_grant_frames; i++ )
> +        {
> +             grant_table = alloc_xenheap_page();

This is wasting one memory page, grant table frames don't need to be
populated.

> +             BUG_ON(grant_table == NULL);
> +             xatp.domid = DOMID_SELF;
> +             xatp.idx = i;
> +             xatp.space = XENMAPSPACE_grant_table;
> +             xatp.gpfn = virt_to_mfn(grant_table);
> +             rc = HYPERVISOR_memory_op(XENMEM_add_to_physmap, &xatp);
> +             if ( rc != 0 )
> +                 printk("Add to physmap failed! %ld\n", rc);
> +
> +             d = rcu_lock_current_domain();
> +             share_xen_page_with_guest(mfn_to_page(xatp.gpfn), d, 
> XENSHARE_writable);
> +             rcu_unlock_domain(d);
> +        }
> +    }
> +
> +    if ( op.nr_frames > 0 ) {
> +        frame_list = xzalloc_array(xen_pfn_t, op.nr_frames);
> +        if ( frame_list == NULL )
> +            return -ENOMEM;
> +    }
> +
> +    old_frame_list = op.frame_list;
> +    op.frame_list.p = frame_list;
> +
> +    rc = HYPERVISOR_grant_table_op(GNTTABOP_setup_table, &op, count);

On HVM you don't need to use the GNTTABOP_setup_table hypercall,
XENMEM_add_to_physmap already does all the needed setup AFAICT.

> +    op.frame_list = old_frame_list;
> +
> +    if ( rc >= 0 ) {
> +        if ( op.status == 0 && op.nr_frames &&
> +             copy_to_guest(old_frame_list, frame_list, op.nr_frames) != 0 ) {
> +            rc = -EFAULT;
> +            goto out;
> +        }
> +
> +        if ( unlikely(copy_to_guest(uop, &op, 1)) != 0 ) {
> +            rc = -EFAULT;
> +            goto out;
> +        }
> +    }
> +
> + out:
> +    xfree(frame_list);
> +
> +    return rc;
> +}
> +
> +static long
>  gnttab_setup_table(
>      XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>      unsigned int limit_max)
> @@ -1811,6 +1895,9 @@ gnttab_setup_table(
>      struct grant_table *gt;
>      unsigned int i;
>  
> +    if ( is_vixen() )
> +        return vixen_gnttab_setup_table(uop, count);
> +
>      if ( count != 1 )
>          return -EINVAL;
>  
> @@ -1892,6 +1979,26 @@ gnttab_setup_table(
>  }
>  
>  static long
> +vixen_gnttab_query_size(
> +    XEN_GUEST_HANDLE_PARAM(gnttab_query_size_t) uop, unsigned int count)
> +{
> +    struct gnttab_query_size op;
> +    int rc;
> +
> +    if ( count != 1 )
> +        return -EINVAL;
> +
> +    if ( unlikely(copy_from_guest(&op, uop, 1)) != 0)
> +        return -EFAULT;
> +
> +    rc = HYPERVISOR_grant_table_op(GNTTABOP_query_size, &op, count);
> +    if (rc == 0 && unlikely(__copy_to_guest(uop, &op, 1)) )
           ^ nit: missing space

Thanks, Roger.

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