|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v21 2/2] tools/libxenctrl: use new xenforeignmemory API to seed grant table
> -----Original Message-----
> From: Andrew Cooper
> Sent: 08 August 2018 10:59
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v21 2/2] tools/libxenctrl: use new
> xenforeignmemory API to seed grant table
>
> 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.
Cool.
>
> > 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?
Sure. I'll restrict mods to the bits I'm touching though.
>
> > 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() ?
>
This is actually used in the HVM compat case too. I'll deal with the other
stylistic issues and add explicit version setting as discussed on the other
patch thread.
Paul
> > + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |