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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf
> Of Jan Beulich
> Sent: 09 August 2018 08:01
> To: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George Dunlap
> <George.Dunlap@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Tim
> (Xen.org) <tim@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>; Suravee
> Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>; Brian Woods <brian.woods@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu
> option
> 
> >>> 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.
> 
> >> > --- 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?
> 

Well, the problem is that the options relating to RAM mappings are not boolean. 
I guess they could be made boolean if we went with a scheme that tried to call 
out specifically what areas of RAM are mapped. I think the areas in question 
are just 'guest-ram' and 'non-guest-ram'.

Relaxed -> guest-ram+non-guest-ram
Strict -> guest-ram
None -> neither

  Paul

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