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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Aug 09, 2018 at 01:00:59AM -0600, Jan Beulich wrote:
> >>> On 08.08.18 at 17:50, <roger.pau@xxxxxxxxxx> wrote:
> > On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
> >> >>> On 08.08.18 at 12:07, <roger.pau@xxxxxxxxxx> 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.

I'm fine with just having:

"Note that all the above options are mutually exclusive."

Leaving out the undefined behavior part.

Note that your example won't work as expected the other way around, if
you have dom0-iommu=none in the config and try to append
dom0-iommu=relaxed.

> >> > --- 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.

In fact I think this would be clearer if something like:

enum {
    NONE,
    RELAXED,
    STRICT,
} iommu_hwdom = RELAXED;

Was used instead of iommu_dom0_passthrough and iommu_dom0_strict.

> >> > +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?

The issue I see with this is that no-none or none=0 doesn't make much
sense as pointed out by Kevin in a previous review. Also adding
support for relaxed=0 or strict=0 just makes the syntax more
complicated IMO, because:

none=0 -> relaxed?
strict=0 -> relaxed?
relaxed=0 -> strict?

We would have to spell out all this combinations in the command line
document.

Thanks, Roger.

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