[PATCH 0/6] gnttab: add per-domain controls

Roger Pau Monne posted 6 patches 2 years, 7 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
docs/man/xl.cfg.5.pod.in                |  12 +++
docs/man/xl.conf.5.pod.in               |  14 +++
tools/console/Makefile                  |   4 +-
tools/console/daemon/io.c               |  25 ++++-
tools/helpers/init-xenstore-domain.c    |   1 +
tools/include/libxl.h                   |  14 +++
tools/libs/light/libxl_create.c         |   4 +
tools/libs/light/libxl_dm.c             |   2 +
tools/libs/light/libxl_dom.c            |   2 +-
tools/libs/light/libxl_types.idl        |   2 +
tools/ocaml/libs/xc/xenctrl.ml          |   5 +
tools/ocaml/libs/xc/xenctrl.mli         |   5 +
tools/ocaml/libs/xc/xenctrl_stubs.c     |  12 ++-
tools/xenstore/Makefile                 |   4 +-
tools/xenstore/include/xenstore_state.h |   1 +
tools/xenstore/xenstored_core.h         |   2 +-
tools/xenstore/xenstored_domain.c       | 120 +++++++++++++++---------
tools/xl/xl.c                           |  15 +++
tools/xl/xl.h                           |   2 +
tools/xl/xl_parse.c                     |  13 +++
xen/arch/arm/domain_build.c             |   4 +
xen/arch/x86/setup.c                    |   2 +
xen/common/domain.c                     |   4 +-
xen/common/grant_table.c                | 119 ++++++++++++++++++++++-
xen/include/public/domctl.h             |  13 ++-
xen/include/xen/grant_table.h           |   7 +-
26 files changed, 341 insertions(+), 67 deletions(-)
[PATCH 0/6] gnttab: add per-domain controls
Posted by Roger Pau Monne 2 years, 7 months ago
Hello,

The first two patches of this series allows setting the preisoutly host
wide command line `gnttab` option on a per domain basis. That means
selecting the max allowed grant table version and whether transitive
grants are allowed.

The last 4 patches attempt to implement support for creating guests
without grant table support at all. This requires some changes to
xenstore in order to map shared ring using foreign memory instead of
grant table.

Note that patch 5 will break the save format for xenstore records, and
should not be applied.

Thanks, Roger.

Roger Pau Monne (6):
  gnttab: allow setting max version per-domain
  grant: allow per-domain control over transitive grants
  tools/console: use xenforeigmemory to map console ring
  tools/xenstored: use atexit to close interfaces
  tools/xenstored: restore support for mapping ring as foreign memory
  gnttab: allow disabling grant table per-domain

 docs/man/xl.cfg.5.pod.in                |  12 +++
 docs/man/xl.conf.5.pod.in               |  14 +++
 tools/console/Makefile                  |   4 +-
 tools/console/daemon/io.c               |  25 ++++-
 tools/helpers/init-xenstore-domain.c    |   1 +
 tools/include/libxl.h                   |  14 +++
 tools/libs/light/libxl_create.c         |   4 +
 tools/libs/light/libxl_dm.c             |   2 +
 tools/libs/light/libxl_dom.c            |   2 +-
 tools/libs/light/libxl_types.idl        |   2 +
 tools/ocaml/libs/xc/xenctrl.ml          |   5 +
 tools/ocaml/libs/xc/xenctrl.mli         |   5 +
 tools/ocaml/libs/xc/xenctrl_stubs.c     |  12 ++-
 tools/xenstore/Makefile                 |   4 +-
 tools/xenstore/include/xenstore_state.h |   1 +
 tools/xenstore/xenstored_core.h         |   2 +-
 tools/xenstore/xenstored_domain.c       | 120 +++++++++++++++---------
 tools/xl/xl.c                           |  15 +++
 tools/xl/xl.h                           |   2 +
 tools/xl/xl_parse.c                     |  13 +++
 xen/arch/arm/domain_build.c             |   4 +
 xen/arch/x86/setup.c                    |   2 +
 xen/common/domain.c                     |   4 +-
 xen/common/grant_table.c                | 119 ++++++++++++++++++++++-
 xen/include/public/domctl.h             |  13 ++-
 xen/include/xen/grant_table.h           |   7 +-
 26 files changed, 341 insertions(+), 67 deletions(-)

-- 
2.33.0


Re: [PATCH 0/6] gnttab: add per-domain controls
Posted by Christian Lindig 2 years, 7 months ago

> On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> Hello,
> 
> The first two patches of this series allows setting the preisoutly host
> wide command line `gnttab` option on a per domain basis. That means
> selecting the max allowed grant table version and whether transitive
> grants are allowed.
> 
> The last 4 patches attempt to implement support for creating guests
> without grant table support at all. This requires some changes to
> xenstore in order to map shared ring using foreign memory instead of
> grant table.
> 
> Note that patch 5 will break the save format for xenstore records, and
> should not be applied.

Has this relevance for the format used by oxenstored?

> 
> Thanks, Roger.
> 
> Roger Pau Monne (6):
>  gnttab: allow setting max version per-domain
>  grant: allow per-domain control over transitive grants
>  tools/console: use xenforeigmemory to map console ring
>  tools/xenstored: use atexit to close interfaces
>  tools/xenstored: restore support for mapping ring as foreign memory
>  gnttab: allow disabling grant table per-domain
> 
> docs/man/xl.cfg.5.pod.in                |  12 +++
> docs/man/xl.conf.5.pod.in               |  14 +++
> tools/console/Makefile                  |   4 +-
> tools/console/daemon/io.c               |  25 ++++-
> tools/helpers/init-xenstore-domain.c    |   1 +
> tools/include/libxl.h                   |  14 +++
> tools/libs/light/libxl_create.c         |   4 +
> tools/libs/light/libxl_dm.c             |   2 +
> tools/libs/light/libxl_dom.c            |   2 +-
> tools/libs/light/libxl_types.idl        |   2 +
> tools/ocaml/libs/xc/xenctrl.ml          |   5 +
> tools/ocaml/libs/xc/xenctrl.mli         |   5 +
> tools/ocaml/libs/xc/xenctrl_stubs.c     |  12 ++-
> tools/xenstore/Makefile                 |   4 +-
> tools/xenstore/include/xenstore_state.h |   1 +
> tools/xenstore/xenstored_core.h         |   2 +-
> tools/xenstore/xenstored_domain.c       | 120 +++++++++++++++---------
> tools/xl/xl.c                           |  15 +++
> tools/xl/xl.h                           |   2 +
> tools/xl/xl_parse.c                     |  13 +++
> xen/arch/arm/domain_build.c             |   4 +
> xen/arch/x86/setup.c                    |   2 +
> xen/common/domain.c                     |   4 +-
> xen/common/grant_table.c                | 119 ++++++++++++++++++++++-
> xen/include/public/domctl.h             |  13 ++-
> xen/include/xen/grant_table.h           |   7 +-
> 26 files changed, 341 insertions(+), 67 deletions(-)
> 
> -- 
> 2.33.0
> 


Re: [PATCH 0/6] gnttab: add per-domain controls
Posted by Roger Pau Monné 2 years, 7 months ago
On Fri, Sep 17, 2021 at 06:06:42PM +0200, Christian Lindig wrote:
> 
> 
> > On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > 
> > Hello,
> > 
> > The first two patches of this series allows setting the preisoutly host
> > wide command line `gnttab` option on a per domain basis. That means
> > selecting the max allowed grant table version and whether transitive
> > grants are allowed.
> > 
> > The last 4 patches attempt to implement support for creating guests
> > without grant table support at all. This requires some changes to
> > xenstore in order to map shared ring using foreign memory instead of
> > grant table.
> > 
> > Note that patch 5 will break the save format for xenstore records, and
> > should not be applied.
> 
> Has this relevance for the format used by oxenstored?

I'm no expert on oxenstored, but I think it has always mapped the
shared ring as foreign memory, and hence no changes are needed there.
AFAICT it also stores the mfn on the save format, so I think this is
all fine.

Should have mentioned it on the cover letter.

Thanks, Roger.

Re: [PATCH 0/6] gnttab: add per-domain controls
Posted by Edwin Torok 2 years, 7 months ago

> On 20 Sep 2021, at 08:26, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> On Fri, Sep 17, 2021 at 06:06:42PM +0200, Christian Lindig wrote:
>> 
>> 
>>> On 17 Sep 2021, at 16:46, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> 
>>> Hello,
>>> 
>>> The first two patches of this series allows setting the preisoutly host
>>> wide command line `gnttab` option on a per domain basis. That means
>>> selecting the max allowed grant table version and whether transitive
>>> grants are allowed.
>>> 
>>> The last 4 patches attempt to implement support for creating guests
>>> without grant table support at all. This requires some changes to
>>> xenstore in order to map shared ring using foreign memory instead of
>>> grant table.
>>> 
>>> Note that patch 5 will break the save format for xenstore records, and
>>> should not be applied.
>> 
>> Has this relevance for the format used by oxenstored?
> 
> I'm no expert on oxenstored, but I think it has always mapped the
> shared ring as foreign memory, and hence no changes are needed there.
> AFAICT it also stores the mfn on the save format, so I think this is
> all fine.
> 
> Should have mentioned it on the cover letter.
>  


There is a patch series from last year to make oxenstored use gnttab instead of map_foreign_range.
https://patchwork.kernel.org/project/xen-devel/cover/cover.1598548832.git.edvin.torok@citrix.com/
This got lost/forgotten amid all the oxenstored XSA work.

Later on I discovered and fixed some bugs in it, and is part of this refreshed patch series (part of which got committed, part of which didn't):
https://patchwork.kernel.org/project/xen-devel/list/?series=480623
https://github.com/edwintorok/xen/pull/2

I think the current status is:
* there was an objection that the commit vendoring the external dependencies for the unit tests was too big, and should be replaced by just an opam and lockfile telling 'opam' or 'opam monorepo' where to download it from
* I've discovered some bugs while testing this code together with other code, and need to retest with just this code alone to check that the bug was not in this code


As for the save format, that is part of this patch series too, and we don't store the mfn anymore. Do we need to go back to storing the mfn?

What do I need to change here? The reason to move away from foreign memory was that we could avoid relying on xenctrl for that function (and thus having one less unstable interface to link to). If we need to conditionally use foreign memory mapping then we're back to using unstable interfaces, unless there is a stable interface equivalent to mapping foreign pages?
I see there is a libs/foreignmemory (it has no OCaml bindings though). If we wrote OCaml bindings would the API/ABI of libs/foreignmemory be stable?
In which case we should probably replace the commit introducing the use of gnttab with the one using foreignmemory and always use foreignmemory instead of gnttab libs.

What do you think?

Best regards,
--Edwin