| Message ID | 20171018140339.13888-3-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Ross Lagerwall > Sent: 18 October 2017 15:04 > To: Xen-devel <xen-devel@lists.xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu > <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > (Xen.org) <tim@xen.org>; Ross Lagerwall <ross.lagerwall@citrix.com>; Jan > Beulich <jbeulich@suse.com> > Subject: [Xen-devel] [PATCH v1 2/5] xen: Provide > XEN_DMOP_add_to_physmap > > Provide XEN_DMOP_add_to_physmap, a limited version of > XENMEM_add_to_physmap to allow a deprivileged QEMU to move VRAM > when a > guest programs its BAR. It is equivalent to XENMEM_add_to_physmap with > space == XENMAPSPACE_gmfn. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > --- > xen/arch/x86/hvm/dm.c | 17 +++++++++++++++++ > xen/include/public/hvm/dm_op.h | 14 ++++++++++++++ > xen/include/xlat.lst | 1 + > 3 files changed, 32 insertions(+) > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > index 32ade95..432a863 100644 > --- a/xen/arch/x86/hvm/dm.c > +++ b/xen/arch/x86/hvm/dm.c > @@ -640,6 +640,22 @@ static int dm_op(const struct dmop_args *op_args) > break; > } > > + case XEN_DMOP_add_to_physmap: > + { > + const struct xen_dm_op_add_to_physmap *data = > + &op.u.add_to_physmap; > + struct xen_add_to_physmap xatp = { > + .domid = op_args->domid, > + .space = XENMAPSPACE_gmfn, > + .idx = data->idx, > + .gpfn = data->gpfn, > + }; > + Where does xatp.size get set? Looks like you're missing a parameter. Paul > + rc = xenmem_add_to_physmap(d, &xatp, > + XENMEM_add_to_physmap >> MEMOP_EXTENT_SHIFT); > + break; > + } > + > default: > rc = -EOPNOTSUPP; > break; > @@ -669,6 +685,7 @@ CHECK_dm_op_set_mem_type; > CHECK_dm_op_inject_event; > CHECK_dm_op_inject_msi; > CHECK_dm_op_remote_shutdown; > +CHECK_dm_op_add_to_physmap; > > int compat_dm_op(domid_t domid, > unsigned int nr_bufs, > diff --git a/xen/include/public/hvm/dm_op.h > b/xen/include/public/hvm/dm_op.h > index e173085..88aace7 100644 > --- a/xen/include/public/hvm/dm_op.h > +++ b/xen/include/public/hvm/dm_op.h > @@ -368,6 +368,19 @@ struct xen_dm_op_remote_shutdown { > /* (Other reason values are not blocked) */ > }; > > +/* > + * XEN_DMOP_add_to_physmap : Sets the GPFN at which a particular page > appears > + * in the specified guest's pseudophysical address > + * space. Identical to XENMEM_add_to_physmap with > + * space == XENMAPSPACE_gmfn. > + */ > +#define XEN_DMOP_add_to_physmap 17 > + > +struct xen_dm_op_add_to_physmap { > + uint64_aligned_t idx; /* Index into GMFN space. */ > + uint64_aligned_t gpfn; /* GPFN in domid where the GMFN should > appear. */ > +}; > + > struct xen_dm_op { > uint32_t op; > uint32_t pad; > @@ -389,6 +402,7 @@ struct xen_dm_op { > struct xen_dm_op_map_mem_type_to_ioreq_server > map_mem_type_to_ioreq_server; > struct xen_dm_op_remote_shutdown remote_shutdown; > + struct xen_dm_op_add_to_physmap add_to_physmap; > } u; > }; > > diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst > index 4346cbe..d40bac6 100644 > --- a/xen/include/xlat.lst > +++ b/xen/include/xlat.lst > @@ -57,6 +57,7 @@ > ? grant_entry_v2 grant_table.h > ? gnttab_swap_grant_ref grant_table.h > ! dm_op_buf hvm/dm_op.h > +? dm_op_add_to_physmap hvm/dm_op.h > ? dm_op_create_ioreq_server hvm/dm_op.h > ? dm_op_destroy_ioreq_server hvm/dm_op.h > ? dm_op_get_ioreq_server_info hvm/dm_op.h > -- > 2.9.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
> -----Original Message----- > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > Paul Durrant > Sent: 20 October 2017 10:16 > To: Ross Lagerwall <ross.lagerwall@citrix.com>; Xen-devel <xen- > devel@lists.xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu > <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > Andrew Cooper <Andrew.Cooper3@citrix.com>; Tim (Xen.org) > <tim@xen.org>; George Dunlap <George.Dunlap@citrix.com>; Ross > Lagerwall <ross.lagerwall@citrix.com>; Jan Beulich <jbeulich@suse.com>; Ian > Jackson <Ian.Jackson@citrix.com> > Subject: Re: [Xen-devel] [PATCH v1 2/5] xen: Provide > XEN_DMOP_add_to_physmap > > > -----Original Message----- > > From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of > > Ross Lagerwall > > Sent: 18 October 2017 15:04 > > To: Xen-devel <xen-devel@lists.xen.org> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu > > <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > > (Xen.org) <tim@xen.org>; Ross Lagerwall <ross.lagerwall@citrix.com>; Jan > > Beulich <jbeulich@suse.com> > > Subject: [Xen-devel] [PATCH v1 2/5] xen: Provide > > XEN_DMOP_add_to_physmap > > > > Provide XEN_DMOP_add_to_physmap, a limited version of > > XENMEM_add_to_physmap to allow a deprivileged QEMU to move VRAM > > when a > > guest programs its BAR. It is equivalent to XENMEM_add_to_physmap with > > space == XENMAPSPACE_gmfn. > > > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > --- > > xen/arch/x86/hvm/dm.c | 17 +++++++++++++++++ > > xen/include/public/hvm/dm_op.h | 14 ++++++++++++++ > > xen/include/xlat.lst | 1 + > > 3 files changed, 32 insertions(+) > > > > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > > index 32ade95..432a863 100644 > > --- a/xen/arch/x86/hvm/dm.c > > +++ b/xen/arch/x86/hvm/dm.c > > @@ -640,6 +640,22 @@ static int dm_op(const struct dmop_args *op_args) > > break; > > } > > > > + case XEN_DMOP_add_to_physmap: > > + { > > + const struct xen_dm_op_add_to_physmap *data = > > + &op.u.add_to_physmap; > > + struct xen_add_to_physmap xatp = { > > + .domid = op_args->domid, > > + .space = XENMAPSPACE_gmfn, > > + .idx = data->idx, > > + .gpfn = data->gpfn, > > + }; > > + > > Where does xatp.size get set? Looks like you're missing a parameter. > > Paul > > > + rc = xenmem_add_to_physmap(d, &xatp, > > + XENMEM_add_to_physmap >> > MEMOP_EXTENT_SHIFT); ... Also looking at this slightly odd argument, assuming that the additional size parameter could be arbitrarily large, you're going to need to handle continuations. Paul > > + break; > > + } > > + > > default: > > rc = -EOPNOTSUPP; > > break; > > @@ -669,6 +685,7 @@ CHECK_dm_op_set_mem_type; > > CHECK_dm_op_inject_event; > > CHECK_dm_op_inject_msi; > > CHECK_dm_op_remote_shutdown; > > +CHECK_dm_op_add_to_physmap; > > > > int compat_dm_op(domid_t domid, > > unsigned int nr_bufs, > > diff --git a/xen/include/public/hvm/dm_op.h > > b/xen/include/public/hvm/dm_op.h > > index e173085..88aace7 100644 > > --- a/xen/include/public/hvm/dm_op.h > > +++ b/xen/include/public/hvm/dm_op.h > > @@ -368,6 +368,19 @@ struct xen_dm_op_remote_shutdown { > > /* (Other reason values are not blocked) */ > > }; > > > > +/* > > + * XEN_DMOP_add_to_physmap : Sets the GPFN at which a particular > page > > appears > > + * in the specified guest's pseudophysical address > > + * space. Identical to XENMEM_add_to_physmap with > > + * space == XENMAPSPACE_gmfn. > > + */ > > +#define XEN_DMOP_add_to_physmap 17 > > + > > +struct xen_dm_op_add_to_physmap { > > + uint64_aligned_t idx; /* Index into GMFN space. */ > > + uint64_aligned_t gpfn; /* GPFN in domid where the GMFN should > > appear. */ > > +}; > > + > > struct xen_dm_op { > > uint32_t op; > > uint32_t pad; > > @@ -389,6 +402,7 @@ struct xen_dm_op { > > struct xen_dm_op_map_mem_type_to_ioreq_server > > map_mem_type_to_ioreq_server; > > struct xen_dm_op_remote_shutdown remote_shutdown; > > + struct xen_dm_op_add_to_physmap add_to_physmap; > > } u; > > }; > > > > diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst > > index 4346cbe..d40bac6 100644 > > --- a/xen/include/xlat.lst > > +++ b/xen/include/xlat.lst > > @@ -57,6 +57,7 @@ > > ? grant_entry_v2 grant_table.h > > ? gnttab_swap_grant_ref grant_table.h > > ! dm_op_buf hvm/dm_op.h > > +? dm_op_add_to_physmap hvm/dm_op.h > > ? dm_op_create_ioreq_server hvm/dm_op.h > > ? dm_op_destroy_ioreq_server hvm/dm_op.h > > ? dm_op_get_ioreq_server_info hvm/dm_op.h > > -- > > 2.9.5 > > > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > https://lists.xen.org/xen-devel > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On 10/20/2017 10:15 AM, Paul Durrant wrote: >> -----Original Message----- snip>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c >> index 32ade95..432a863 100644 >> --- a/xen/arch/x86/hvm/dm.c >> +++ b/xen/arch/x86/hvm/dm.c >> @@ -640,6 +640,22 @@ static int dm_op(const struct dmop_args *op_args) >> break; >> } >> >> + case XEN_DMOP_add_to_physmap: >> + { >> + const struct xen_dm_op_add_to_physmap *data = >> + &op.u.add_to_physmap; >> + struct xen_add_to_physmap xatp = { >> + .domid = op_args->domid, >> + .space = XENMAPSPACE_gmfn, >> + .idx = data->idx, >> + .gpfn = data->gpfn, >> + }; >> + > > Where does xatp.size get set? Looks like you're missing a parameter. > xatp.size is only used for XENMAPSPACE_gmfn_range which is not supported by this interface. size gets set to 0 by the C99 designated initializer. Based on your other comments, would it make sense to instead use XENMAPSPACE_gmfn_range and have the caller set the size? As it is currently, QEMU does only populate VRAM one page at a time (using xen_xc_domain_add_to_physmap) so it is already slow but it could be improved.
> -----Original Message----- > From: Ross Lagerwall [mailto:ross.lagerwall@citrix.com] > Sent: 20 October 2017 10:37 > To: Paul Durrant <Paul.Durrant@citrix.com>; Xen-devel <xen- > devel@lists.xen.org> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Wei Liu > <wei.liu2@citrix.com>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; > George Dunlap <George.Dunlap@citrix.com>; Andrew Cooper > <Andrew.Cooper3@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Tim > (Xen.org) <tim@xen.org>; Jan Beulich <jbeulich@suse.com> > Subject: Re: [Xen-devel] [PATCH v1 2/5] xen: Provide > XEN_DMOP_add_to_physmap > > On 10/20/2017 10:15 AM, Paul Durrant wrote: > >> -----Original Message----- > snip>> diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c > >> index 32ade95..432a863 100644 > >> --- a/xen/arch/x86/hvm/dm.c > >> +++ b/xen/arch/x86/hvm/dm.c > >> @@ -640,6 +640,22 @@ static int dm_op(const struct dmop_args > *op_args) > >> break; > >> } > >> > >> + case XEN_DMOP_add_to_physmap: > >> + { > >> + const struct xen_dm_op_add_to_physmap *data = > >> + &op.u.add_to_physmap; > >> + struct xen_add_to_physmap xatp = { > >> + .domid = op_args->domid, > >> + .space = XENMAPSPACE_gmfn, > >> + .idx = data->idx, > >> + .gpfn = data->gpfn, > >> + }; > >> + > > > > Where does xatp.size get set? Looks like you're missing a parameter. > > > xatp.size is only used for XENMAPSPACE_gmfn_range which is not > supported > by this interface. size gets set to 0 by the C99 designated initializer. > > Based on your other comments, would it make sense to instead use > XENMAPSPACE_gmfn_range and have the caller set the size? Yes... my eyes had read XENMAPSPACE_gmfn_range in the first place, hence my confusion over the size parameter. > > As it is currently, QEMU does only populate VRAM one page at a time > (using xen_xc_domain_add_to_physmap) Ouch, yes, I'd forgotten that. > so it is already slow but it could > be improved. Indeed. I think we should shoot for a better semantic given that it's a new op. Paul > > -- > Ross Lagerwall
diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c index 32ade95..432a863 100644 --- a/xen/arch/x86/hvm/dm.c +++ b/xen/arch/x86/hvm/dm.c @@ -640,6 +640,22 @@ static int dm_op(const struct dmop_args *op_args) break; } + case XEN_DMOP_add_to_physmap: + { + const struct xen_dm_op_add_to_physmap *data = + &op.u.add_to_physmap; + struct xen_add_to_physmap xatp = { + .domid = op_args->domid, + .space = XENMAPSPACE_gmfn, + .idx = data->idx, + .gpfn = data->gpfn, + }; + + rc = xenmem_add_to_physmap(d, &xatp, + XENMEM_add_to_physmap >> MEMOP_EXTENT_SHIFT); + break; + } + default: rc = -EOPNOTSUPP; break; @@ -669,6 +685,7 @@ CHECK_dm_op_set_mem_type; CHECK_dm_op_inject_event; CHECK_dm_op_inject_msi; CHECK_dm_op_remote_shutdown; +CHECK_dm_op_add_to_physmap; int compat_dm_op(domid_t domid, unsigned int nr_bufs, diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index e173085..88aace7 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -368,6 +368,19 @@ struct xen_dm_op_remote_shutdown { /* (Other reason values are not blocked) */ }; +/* + * XEN_DMOP_add_to_physmap : Sets the GPFN at which a particular page appears + * in the specified guest's pseudophysical address + * space. Identical to XENMEM_add_to_physmap with + * space == XENMAPSPACE_gmfn. + */ +#define XEN_DMOP_add_to_physmap 17 + +struct xen_dm_op_add_to_physmap { + uint64_aligned_t idx; /* Index into GMFN space. */ + uint64_aligned_t gpfn; /* GPFN in domid where the GMFN should appear. */ +}; + struct xen_dm_op { uint32_t op; uint32_t pad; @@ -389,6 +402,7 @@ struct xen_dm_op { struct xen_dm_op_map_mem_type_to_ioreq_server map_mem_type_to_ioreq_server; struct xen_dm_op_remote_shutdown remote_shutdown; + struct xen_dm_op_add_to_physmap add_to_physmap; } u; }; diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index 4346cbe..d40bac6 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -57,6 +57,7 @@ ? grant_entry_v2 grant_table.h ? gnttab_swap_grant_ref grant_table.h ! dm_op_buf hvm/dm_op.h +? dm_op_add_to_physmap hvm/dm_op.h ? dm_op_create_ioreq_server hvm/dm_op.h ? dm_op_destroy_ioreq_server hvm/dm_op.h ? dm_op_get_ioreq_server_info hvm/dm_op.h
Provide XEN_DMOP_add_to_physmap, a limited version of XENMEM_add_to_physmap to allow a deprivileged QEMU to move VRAM when a guest programs its BAR. It is equivalent to XENMEM_add_to_physmap with space == XENMAPSPACE_gmfn. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- xen/arch/x86/hvm/dm.c | 17 +++++++++++++++++ xen/include/public/hvm/dm_op.h | 14 ++++++++++++++ xen/include/xlat.lst | 1 + 3 files changed, 32 insertions(+)