Re: [Xen-devel] [PATCH v4 2/4] iommu: make iommu_inclusive_mapping a suboption of dom0-iommu

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.08.18 at 18:09, <roger.pau@xxxxxxxxxx> wrote:
> On Wed, Aug 08, 2018 at 06:32:00AM -0600, Jan Beulich wrote:
>> >>> On 08.08.18 at 12:07, <roger.pau@xxxxxxxxxx> wrote:
>> > Introduce a new dom0-iommu=inclusive generic option that supersedes
>> > iommu_inclusive_mapping. The previous behaviour is preserved and the
>> > option should only be enabled by default on Intel hardware.
>> 
>> Why "should" instead of "is"?
>> 
>> > @@ -1221,6 +1221,18 @@ PV Dom0:
>> >  Note that all the above options are mutually exclusive. Specifying more 
>> > than
>> >  one on the `dom0-iommu` command line will result in undefined behavior.
>> >  
>> > +The following options control whether non-RAM regions are added to the 
>> > Dom0
>> > +iommu tables. Note that they can be prefixed with `no-` to effect the 
>> > inverse
>> > +meaning:
>> 
>> I'm not particularly happy about the mentioning of "no-" here: Why is
>> this better than the also permitted "=0" etc suffixes? Keep it generic,
>> just like other options do.
> 
> Oh, I've just copied this text from the iommu option. Should I change
> this to:
> 
> "The following boolean options control whether non-RAM regions are
> added to the Dom0 iommu tables:"

Yes please, much better.

>> > +* `inclusive`: sets up DMA remapping for all the non-RAM memory below 4GB
>> > +  except for unusable ranges. Use this to work around firmware issues 
>> > providing
>> > +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for 
>> > IOMMU
>> > +  accesses for Dom0, with this option all pages up to 4GB, not marked as
>> > +  unusable in the E820 table, will get a mapping established. Note that 
>> > this
>> > +  option is only applicable to a PV Dom0 and is enabled by default on 
>> > Intel
>> > +  hardware.
>> 
>> No word at all about the interaction with none/strict/relaxed? I think,
>> as mentioned for patch 1, "none" renders this option meaningless as
>> well. But for "relaxed" it's pretty unclear, because from E820 alone
>> you can't judge whether e.g. a reserved region is RAM or MMIO. (As
>> an implication, the mentioning of RAM in patch 1's doc for "relaxed"
>> then looks symmetrically wrong, just like I've already asked to replace
>> "memory" by "RAM" for "strict".)
> 
> Hm, I'm not sure I follow. 'relaxed' refers to how the regions marked
> as RAM in the memory map (E820_RAM) will be mapped to the guest.

Hmm, it may well be that I'm (once again) confused with the
various options and their precise effects; perhaps I didn't pay
(enough) attention to the use of iommu_inclusive_mapping in
vtd_set_hwdom_mapping().

>> > @@ -202,6 +210,13 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>> >      if ( !iommu_enabled )
>> >          return;
>> >  
>> > +    if ( iommu_dom0_inclusive == true && !is_pv_domain(d) )
>> 
>> Why the "== true"? It shouldn't have its initializer value of -1 anymore
>> at this point.
> 
> It can have a value of -1, AFAICT the default values are set by
> hd->platform_ops->hwdom_init which is called later in this function.

Hmm, I see. But that's not very nice. Can't you move this check
between the ->hwdom_init() hook invocation and the call to
arch_iommu_hwdom_init() that you add? The latter is the only
place where it's actually needed.

Jan



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