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

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

>>> On 09.08.18 at 12:01, <roger.pau@citrix.com> wrote:
> On Thu, Aug 09, 2018 at 01:00:59AM -0600, Jan Beulich wrote:
>> >>> 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.
> 
> 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?

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

Jan



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