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

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

On Tue, Jan 09, 2018 at 04:02:50PM -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>
> ---
> v1 -> v2
>  - move to using reserved memory space for grant table instead of heap
>  - use a dispatch function instead of modifying all calls

I've already commented on v2 of this patch, yet I got no reply:

https://lists.xenproject.org/archives/html/xen-devel/2018-01/msg00565.html

I'm just going to repeat the same comments.

>  xen/arch/x86/guest/vixen.c |   4 ++
>  xen/common/grant_table.c   | 101 
> +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 105 insertions(+)
> 
> diff --git a/xen/arch/x86/guest/vixen.c b/xen/arch/x86/guest/vixen.c
> index 7c886a2..2437c92 100644
> --- a/xen/arch/x86/guest/vixen.c
> +++ b/xen/arch/x86/guest/vixen.c
> @@ -22,10 +22,14 @@
>  #include <asm/guest/vixen.h>
>  #include <public/version.h>
>  #include <public/hvm/hvm_info_table.h>
> +#include <xen/grant_table.h>
> +
> +#define PCI_DEVICE_ID_XENSOURCE_PLATFORM     0x0001
>  
>  #define X86_HVM_END_SPECIAL_REGION  0xff000u
>  
>  #define SHARED_INFO_PFN              (X86_HVM_END_SPECIAL_REGION + 0)
> +#define GRANT_TABLE_PFN_0    (X86_HVM_END_SPECIAL_REGION + 1)
>  
>  static int in_vixen;
>  static int vixen_domid = 1;
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 250450b..60a7941 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 {
> @@ -1801,6 +1802,56 @@ grant_table_init(struct domain *d, struct grant_table 
> *gt,
>  }
>  
>  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;
> +    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 ( op.nr_frames > 0 ) {
> +        frame_list = xzalloc_array(xen_pfn_t, op.nr_frames);

This is kind of pointless, you should check that op.nr_frames is not
greater than the return of query_size.max_nr_frames from the L0
hypervisor.

> +        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);

Are you sure this works? HVM based guests need to use
XENMAPSPACE_grant_table so that they map the grant table somewhere in
the p2m.

My comment on v1 was the other way around: you can ditch the
GNTTABOP_setup_table call, but you need to keep the
XENMAPSPACE_grant_table one.

Maybe I'm missing something obvious, but I don't see how that's going
to work if the grant table frame is not mapped to a shim gfn.

> +static long
> +vixen_do_grant_table_op(
> +    unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
> +{
> +    long rc;
> +
> +    rc = -EFAULT;
> +    switch ( cmd )
> +    {
> +    case GNTTABOP_setup_table:
> +        rc = vixen_gnttab_setup_table(
> +            guest_handle_cast(uop, gnttab_setup_table_t), count);
> +        break;
> +
> +    case GNTTABOP_query_size:
> +        rc = vixen_gnttab_query_size(
> +            guest_handle_cast(uop, gnttab_query_size_t), count);
> +        break;
> +
> +    default:
> +        rc = -ENOSYS;
> +        break;
> +    }
> +
> +    return rc;
> + }
> +
>  long
>  do_grant_table_op(
>      unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
> @@ -3324,6 +3422,9 @@ do_grant_table_op(
>      if ( (cmd &= GNTTABOP_CMD_MASK) != GNTTABOP_cache_flush && opaque_in )
>          return -EINVAL;
>  
> +    if ( is_vixen() )
> +        return vixen_do_grant_table_op(cmd, uop, count);

You seem to be missing the compat code. You should hook this into
compat_grant_table_op and fix the implementation of
vixen_gnttab_setup_table so it can deal with 32bit guests.

Roger.

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