Re: [PATCH v4 1/4] iommu: introduce dom0-iommu option

["Jan Beulich" <JBeulich@suse.com>]

>>> On 08.08.18 at 17:50, <roger.pau@citrix.com> wrote:
> On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
>> >>> On 08.08.18 at 12:07, <roger.pau@citrix.com> wrote:
>> > +Note that all the above options are mutually exclusive. Specifying more than
>> > +one on the `dom0-iommu` command line will result in undefined behavior.
>> 
>> Isn't this more strict than it needs to be? "none", afaict, always takes
>> precedence if enabled. What color a bike shed is simply doesn't matter
>> when it doesn't exist.
> 
> Right, that's due to the current implementation and the way this is
> stored, but I don't think we want to spell out any of this in order to
> not give any guarantees. For example:
> 
> dom0-iommu=none,relaxed
> 
> Shouldn't be used, albeit with the current implementation relaxed will
> be basically ignored I don't think we want to write this down
> anywhere because people shouldn't rely on this behavior.

Well, there's one very particular case to be considered: In a number
of environments you can (easily) append to the command line, but
you can't (easily) alter what has been put there e.g. in some config
file. If the config file says "dom0-iommu=relaxed" but for the current
run you want "dom0-iommu=none", with your restrictions you'd be
unable to (legitimately) do so.

Therefore I think we should try to avoid spelling out undefined
behavior for command line option combinations wherever we can.

>> > --- a/xen/arch/x86/x86_64/mm.c
>> > +++ b/xen/arch/x86/x86_64/mm.c
>> > @@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned long epfn, unsigned int pxm)
>> >      if ( ret )
>> >          goto destroy_m2p;
>> >  
>> > -    if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) )
>> > +    if ( iommu_enabled && !iommu_dom0_passthrough &&
>> > +         !need_iommu(hardware_domain) )
>> 
>> This makes already clear that you need to better distinguish Dom0 and
>> hwdom here, but it's not immediately clear to me which direction the
>> changes should be made: Do you mean truly only Dom0 throughout
>> this patch, or hwdom? While the doc and command line option name can
>> perhaps left as is, internal variable names should not say Dom0 when
>> they don't mean Dom0. Otoh if you mean only Dom0, then the use of
>> hardware_domain above (and elsewhere) is now wrong.
> 
> Hm, everything is kind of mixed here. Existing variables already use
> _dom0_ (iommu_dom0_strict for example). I can rename them to
> iommu_hwdom_, because AFAICT this applies to the hardware domain.

Well, as said - I'd like you to do so for ones you rename anyway.
I'd appreciate (but won't demand) you to also do so for others.

>> > +static int __init parse_dom0_iommu_param(const char *s)
>> > +{
>> > +    const char *ss;
>> > +    int rc = 0;
>> > +
>> > +    do {
>> > +        ss = strchr(s, ',');
>> > +        if ( !ss )
>> > +            ss = strchr(s, '\0');
>> > +
>> > +        if ( !strncmp(s, "none", ss - s) )
>> > +            iommu_dom0_passthrough = true;
>> > +        else if ( !strncmp(s, "strict", ss - s) )
>> > +            iommu_dom0_strict = true;
>> > +        else if ( !strncmp(s, "relaxed", ss - s) )
>> > +            iommu_dom0_strict = false;
>> 
>> Perhaps better just have one of the two, and make them truly
>> boolean? Or would that conflict with further patches / plans?
> 
> I don't think this will cause a lot of conflicts, some rebasing
> issues but no big deal. I've used this syntax as discussed
> in a previous version and agreed with Paul and Kevin. I'm OK with
> this, and I think it's clear, but I don't have a strong opinion so if
> you think this is not clear I can change it.

Well, I'm certainly of the pretty strong opinion that inverse
options should be specifiable by a boolean mechanism, not
(only) by entirely distinct names. I wouldn't mind you retaining
both "relaxed" and "strict", as long as "relaxed=0" means
"strict" and vice versa. Paul, Kevin?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel