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

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

On Thu, Aug 09, 2018 at 04:29:51AM -0600, Jan Beulich wrote:
> >>> On 09.08.18 at 12:01, <roger.pau@xxxxxxxxxx> wrote:
> > 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."
> 
> But they aren't.
> 
> > 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.
> 
> Indeed, which means there would need to be an opposite of
> "none". I'm hesitant to suggest "no-none". Perhaps "strict"
> and "relaxed" could also clear that other flag?

See below.

> >> >> > --- 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.
> 
> Fine with me.

Switching to a single enum also means that the last dom0-iommu = [
none | strict | relaxed ] will be the one enforced, so I could keep
them as-is, and add:

"Note that all the above options are mutually exclusive and the last
one found on the command line will be the one to take effect."

This will also deal with your request to be able to override previous
options on the command line while keeping the options as they are.

Thanks, Roger.

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