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

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 20 Sep 2021, at 08:26, Roger Pau Monne <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx/
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