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

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

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.

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

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

I would just like to reach a consensus on the nomenclature of the
option ASAP, so the bikeshed can be as small as possible :).

Thanks, Roger.

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