Re: [Xen-devel] [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/08/18 10:00, Paul Durrant wrote:
> A previous patch added support for priv-mapping guest resources directly
> (rather than having to foreign-map, which requires P2M modification for
> HVM guests).
>
> This patch makes use of the new API to seed the guest grant table unless
> the underlying infrastructure (i.e. privcmd) doesn't support it, in which
> case the old scheme is used.
>
> NOTE: The call to xc_dom_gnttab_hvm_seed() in hvm_build_set_params() was
>       actually unnecessary, as the grant table has already been seeded
>       by a prior call to xc_dom_gnttab_init() made by libxl__build_dom().
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Acked-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Acked-by: Wei Liu <wei.liu2@xxxxxxxxxx>

Some minor style issues, all of which can fixed on commit (probably). 
Everything else looks fine.

> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 8a66889c68..a8a0c0da66 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -337,12 +337,8 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, 
> xen_pfn_t pfn,
>  int xc_dom_boot_image(struct xc_dom_image *dom);
>  int xc_dom_compat_check(struct xc_dom_image *dom);
>  int xc_dom_gnttab_init(struct xc_dom_image *dom);
> -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> -                           xen_pfn_t console_gmfn,
> -                           xen_pfn_t xenstore_gmfn,
> -                           uint32_t console_domid,
> -                           uint32_t xenstore_domid);
> -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> +                       bool is_hvm,
>                         xen_pfn_t console_gmfn,
>                         xen_pfn_t xenstore_gmfn,

As you're rewriting most of the functionality, can we switch to gfn to
correct the terminology?

>                         uint32_t console_domid,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 2e5681dc5d..8307ebeaf6 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -256,11 +256,29 @@ static xen_pfn_t xc_dom_gnttab_setup(xc_interface *xch, 
> uint32_t domid)
>      return gmfn;
>  }
>  
> -int xc_dom_gnttab_seed(xc_interface *xch, uint32_t domid,
> -                       xen_pfn_t console_gmfn,
> -                       xen_pfn_t xenstore_gmfn,
> -                       uint32_t console_domid,
> -                       uint32_t xenstore_domid)
> +static void xc_dom_set_gnttab_entry(xc_interface *xch,
> +                                    grant_entry_v1_t *gnttab,
> +                                    unsigned int idx,
> +                                    uint32_t guest_domid,
> +                                    uint32_t backend_domid,
> +                                    xen_pfn_t backend_gmfn)

gfn

> +{
> +    if ( guest_domid == backend_domid || backend_gmfn == -1)

Space at the end.

> +        return;
> +
> +    xc_dom_printf(xch, "%s: [%u] -> 0x%"PRI_xen_pfn,
> +                  __FUNCTION__, idx, backend_gmfn);

__func__ rather than __FUNCTION__.  Also, "[%u] => d%d 0x"PRI_xen_pfn
would be more helpful for diagnostics.  (I do realise that backend
domid's were retrofitted without adjusting the printf().)

> +
> +    gnttab[idx].flags = GTF_permit_access;
> +    gnttab[idx].domid = backend_domid;
> +    gnttab[idx].frame = backend_gmfn;
> +}
> +
> +static int compat_gnttab_seed(xc_interface *xch, uint32_t domid,

compat_gnttab_pv_seed() ?

> +                              xen_pfn_t console_gmfn,
> +                              xen_pfn_t xenstore_gmfn,

gfn

> +                              uint32_t console_domid,
> +                              uint32_t xenstore_domid)
>  {
>  
>      xen_pfn_t gnttab_gmfn;
> @@ -313,11 +323,11 @@ int xc_dom_gnttab_seed(xc_interface *xch, uint32_t 
> domid,
>      return 0;
>  }
>  
> -int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> -                           xen_pfn_t console_gpfn,
> -                           xen_pfn_t xenstore_gpfn,
> -                           uint32_t console_domid,
> -                           uint32_t xenstore_domid)
> +static int compat_gnttab_hvm_seed(xc_interface *xch, uint32_t domid,
> +                                  xen_pfn_t console_gpfn,
> +                                  xen_pfn_t xenstore_gpfn,

gfn.

> +                                  uint32_t console_domid,
> +                                  uint32_t xenstore_domid)
>  {
>      int rc;
>      xen_pfn_t scratch_gpfn;
> @@ -356,7 +366,7 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t 
> domid,
>          return -1;
>      }
>  
> -    rc = xc_dom_gnttab_seed(xch, domid,
> +    rc = compat_gnttab_seed(xch, domid,
>                              console_gpfn, xenstore_gpfn,
>                              console_domid, xenstore_domid);
>      if (rc != 0)
> @@ -381,18 +391,56 @@ int xc_dom_gnttab_hvm_seed(xc_interface *xch, uint32_t 
> domid,
>      return 0;
>  }
>  
> -int xc_dom_gnttab_init(struct xc_dom_image *dom)
> +int xc_dom_gnttab_seed(xc_interface *xch, uint32_t guest_domid,
> +                       bool is_hvm, xen_pfn_t console_gmfn,
> +                       xen_pfn_t xenstore_gmfn, uint32_t console_domid,

gfn.

> +                       uint32_t xenstore_domid)
>  {
> -    if ( xc_dom_translated(dom) ) {
> -        return xc_dom_gnttab_hvm_seed(dom->xch, dom->guest_domid,
> -                                      dom->console_pfn, dom->xenstore_pfn,
> -                                      dom->console_domid, 
> dom->xenstore_domid);
> -    } else {
> -        return xc_dom_gnttab_seed(dom->xch, dom->guest_domid,
> -                                  xc_dom_p2m(dom, dom->console_pfn),
> -                                  xc_dom_p2m(dom, dom->xenstore_pfn),
> -                                  dom->console_domid, dom->xenstore_domid);
> +    xenforeignmemory_handle* fmem = xch->fmem;
> +    xenforeignmemory_resource_handle *fres;
> +    void *addr = NULL;
> +
> +    fres = xenforeignmemory_map_resource(
> +        fmem, guest_domid, XENMEM_resource_grant_table,
> +        XENMEM_resource_grant_table_id_shared, 0, 1, &addr,
> +        PROT_READ | PROT_WRITE, 0);
> +    if ( !fres )
> +    {
> +        if ( errno == EOPNOTSUPP )
> +            return is_hvm ?
> +                compat_gnttab_hvm_seed(xch, guest_domid,
> +                                       console_gmfn, xenstore_gmfn,
> +                                       console_domid, xenstore_domid) :
> +                compat_gnttab_seed(xch, guest_domid,
> +                                   console_gmfn, xenstore_gmfn,
> +                                   console_domid, xenstore_domid);
> +
> +        xc_dom_panic(xch, XC_INTERNAL_ERROR,
> +                     "%s: failed to acquire grant table "
> +                     "[errno=%d]\n",
> +                     __FUNCTION__, errno);

__func__, and guest domid in the printk()?

> +        return -1;
>      }
> +
> +    xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_CONSOLE,
> +                            guest_domid, console_domid, console_gmfn);
> +    xc_dom_set_gnttab_entry(xch, addr, GNTTAB_RESERVED_XENSTORE,
> +                            guest_domid, xenstore_domid, xenstore_gmfn);
> +
> +    xenforeignmemory_unmap_resource(fmem, fres);
> +
> +    return 0;
> +}
> +
> +int xc_dom_gnttab_init(struct xc_dom_image *dom)
> +{
> +    bool is_hvm = xc_dom_translated(dom);
> +    xen_pfn_t console_gmfn = xc_dom_p2m(dom, dom->console_pfn);
> +    xen_pfn_t xenstore_gmfn = xc_dom_p2m(dom, dom->xenstore_pfn);

I've still got a pending fix to rename dom->*_pfn to gfn, but the local
variables should at least be correct.

> +
> +    return xc_dom_gnttab_seed(dom->xch, dom->guest_domid, is_hvm,
> +                              console_gmfn, xenstore_gmfn,
> +                              dom->console_domid, dom->xenstore_domid);
>  }
>  
>  /*
> diff --git a/tools/libxc/xc_sr_restore_x86_hvm.c 
> b/tools/libxc/xc_sr_restore_x86_hvm.c
> index 227c48553e..4765a52f33 100644
> --- a/tools/libxc/xc_sr_restore_x86_hvm.c
> +++ b/tools/libxc/xc_sr_restore_x86_hvm.c
> @@ -216,11 +216,11 @@ static int x86_hvm_stream_complete(struct xc_sr_context 
> *ctx)
>          return rc;
>      }
>  
> -    rc = xc_dom_gnttab_hvm_seed(xch, ctx->domid,
> -                                ctx->restore.console_gfn,
> -                                ctx->restore.xenstore_gfn,
> -                                ctx->restore.console_domid,
> -                                ctx->restore.xenstore_domid);
> +    rc = xc_dom_gnttab_seed(xch, ctx->domid, true,
> +                            ctx->restore.console_gfn,
> +                            ctx->restore.xenstore_gfn,
> +                            ctx->restore.console_domid,
> +                            ctx->restore.xenstore_domid);

Hmm.  This is now common with the pv side, and will similarly be wanted
on the ARM side eventually.  This patch shouldn't change from how it is
now, but I'll try and find some copious free time to pull together a
common stream_complete() function.

~Andrew

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