[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
>>> 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? >> >> > --- 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |