Re: [PATCH v2 3/6] gnttab: allow per-domain control over transitive grants

From: Jan Beulich <jbeulich@suse.com>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Julien Grall <julien@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Anthony PERARD <anthony.perard@citrix.com>,
	Juergen Gross <jgross@suse.com>,
	Christian Lindig <christian.lindig@citrix.com>,
	David Scott <dave@recoil.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 3/6] gnttab: allow per-domain control over transitive grants
Date: Wed, 20 Oct 2021 13:51:58 +0200	[thread overview]
Message-ID: <dc514908-a1a9-7ce1-5490-16be3f8c74a2@suse.com> (raw)
In-Reply-To: <YW/r6Dk5a79myhzy@MacBook-Air-de-Roger.local>

On 20.10.2021 12:14, Roger Pau Monné wrote:
> On Fri, Oct 15, 2021 at 12:05:06PM +0200, Jan Beulich wrote:
>> On 22.09.2021 10:21, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -750,7 +750,8 @@ static struct domain *__init create_dom0(const module_t *image,
>>>          .max_evtchn_port = -1,
>>>          .max_grant_frames = -1,
>>>          .max_maptrack_frames = -1,
>>> -        .grant_opts = XEN_DOMCTL_GRANT_version_default,
>>> +        .grant_opts = XEN_DOMCTL_GRANT_version_default |
>>> +                      XEN_DOMCTL_GRANT_transitive,
>>>          .max_vcpus = dom0_max_vcpus(),
>>>          .arch = {
>>>              .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
>>
>> While I can see that you make these adjustments for retaining backwards
>> compatibility, I wonder if we need to, at least for Dom0. Dom0 doesn't
>> normally grant anything anyway and hence would even less so use
>> transitive grants. Of course then there's need to be a command line
>> control to re-enable that, just in case.
> 
> dom0=gnttab-transitive, or should it be placed somewhere else?

That sounds like the place to have it at; at least that's where I would
have suggested to put it. One question of course is how it ought to
interact with v2 being unavailable.

>>> @@ -1965,6 +1969,8 @@ int grant_table_init(struct domain *d, int max_grant_frames,
>>>      gt->max_grant_frames = max_grant_frames;
>>>      gt->max_maptrack_frames = max_maptrack_frames;
>>>      gt->max_grant_version = max_grant_version;
>>> +    gt->allow_transitive = !!(options & XEN_DOMCTL_GRANT_transitive) &&
>>> +                           opt_transitive_grants;
>>
>> No need for !! here afaics. Not even if there weren't the &&.
>>
>> As to combining with the global option: I think if an admin requested
>> them for a domain, this should overrule the command line option. Which
>> in turn suggests that the command line option could go away at this
>> point. Otherwise, if you AND both together and the guest is known to
>> not work without this functionality, domain creation would instead
>> better fail (or at the very least it should be logged by the tool
>> stack that the request wasn't satisfied, which would require a means
>> to retrieve the effective setting). IOW I would see the command line
>> turning this off to trump the per-guest enabling request.
> 
> How should we go about deprecating it? It would be a bit antisocial
> IMO to just drop the option, since people using it would then be
> exposed to guests using transient grants if they didn't realize it
> should be set in xl.conf or xl.cfg now.

So perhaps for a transitional phase fail if the command line option
says no and the request for the guest says yes? Accompanied by a
log message warning that the command line control is going to go
away, so that people will know to adjust their guest configs?

Jan