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

[Jan Beulich <jbeulich@xxxxxxxx>]

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