Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
>>> On 09.08.18 at 12:51, wrote:
> On Thu, Aug 09, 2018 at 04:29:51AM -0600, Jan Beulich wrote:
>> >>> On 09.08.18 at 12:01, wrote:
>> > 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."
Right. I don't think the second half of the sentence is needed -
that's what happens for most other options as well.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
On Thu, Aug 09, 2018 at 04:29:51AM -0600, Jan Beulich wrote:
> >>> On 09.08.18 at 12:01, wrote:
> > On Thu, Aug 09, 2018 at 01:00:59AM -0600, Jan Beulich wrote:
> >> >>> On 08.08.18 at 17:50, wrote:
> >> > On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
> >> >> >>> On 08.08.18 at 12:07, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
>>> On 09.08.18 at 12:01, wrote:
> On Thu, Aug 09, 2018 at 01:00:59AM -0600, Jan Beulich wrote:
>> >>> On 08.08.18 at 17:50, wrote:
>> > On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
>> >> >>> On 08.08.18 at 12:07, 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
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xenproject.org] On Behalf
> Of Jan Beulich
> Sent: 09 August 2018 08:01
> To: Roger Pau Monne
> Cc: Kevin Tian ; Stefano Stabellini
> ; Wei Liu ; George Dunlap
> ; Andrew Cooper
> ; Ian Jackson ; Tim
> (Xen.org) ; Julien Grall ; Suravee
> Suthikulpanit ; xen-devel de...@lists.xenproject.org>; Brian Woods
> Subject: Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu
> option
>
> >>> On 08.08.18 at 17:50, wrote:
> > On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
> >> >>> On 08.08.18 at 12:07, 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 vers
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
On Thu, Aug 09, 2018 at 01:00:59AM -0600, Jan Beulich wrote:
> >>> On 08.08.18 at 17:50, wrote:
> > On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
> >> >>> On 08.08.18 at 12:07, 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."
Leaving out the undefined behavior part.
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.
> >> > --- 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.
> >> > +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?
The issue I see with this is that no-none or none=0 doesn't make much
sense as pointed out by Kevin in a previous review. Also adding
support for relaxed=0 or strict=0 just makes the syntax more
complicated IMO, because:
none=0 -> relaxed?
strict=0 -> relaxed?
relaxed=0 -> strict?
We would have to spell out all this combinations in the command line
document.
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
>>> On 08.08.18 at 17:50, wrote:
> On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
>> >>> On 08.08.18 at 12:07, 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?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
On Wed, Aug 08, 2018 at 06:10:39AM -0600, Jan Beulich wrote:
> >>> On 08.08.18 at 12:07, 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 1/4] iommu: introduce dom0-iommu option
>>> On 08.08.18 at 12:07, wrote:
> @@ -1198,6 +1204,23 @@ detection of systems known to misbehave upon accesses
> to that port.
>
> >> Enable IOMMU debugging code (implies `verbose`).
>
> +### dom0-iommu
This is now misplaced, as the file is (meant to be) alphabetically
sorted.
> +> `= List of [ none | strict | relaxed ]`
> +
> +* `none`: disables DMA remapping for Dom0.
> +
> +The following two options control how RAM regions are mapped in the iommu for
> +PV Dom0:
> +
> +* `strict`: sets up DMA remapping only for the memory Dom0 actually got
> + assigned.
s/memory/RAM/ ?
> +* `relaxed`: sets DMA remapping for all the host RAM except regions in use by
> + Xen. This is the default iommu behaviour.
Drop "iommu" here?
> +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.
> --- 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.
Of course I won't demand (but even less so object to) you renaming
the other related variable that is affected here.
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -22,6 +22,7 @@
> #include
>
> static int parse_iommu_param(const char *s);
> +static int parse_dom0_iommu_param(const char *s);
Please don't. Instead ...
> @@ -72,6 +71,10 @@ bool_t __read_mostly iommu_hap_pt_share = 1;
> bool_t __read_mostly iommu_debug;
> bool_t __read_mostly amd_iommu_perdev_intremap = 1;
>
> +custom_param("dom0-iommu", parse_dom0_iommu_param);
... move this immediately after (with no intervening blank line)
parse_dom0_iommu_param()'s definition.
> +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?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
