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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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:"

> > +* `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.

The inclusive option OTOH refers to if/how non-RAM regions in the
memory map will be mapped to the guest. It doesn't matter if a
reserved region (E820_RESERVED) is actually backed by RAM or MMIO, it
will be mapped to the guest because it's a reserved region on the
memory map.

> > --- a/xen/drivers/passthrough/arm/iommu.c
> > +++ b/xen/drivers/passthrough/arm/iommu.c
> > @@ -73,3 +73,7 @@ int arch_iommu_populate_page_table(struct domain *d)
> >      /* The IOMMU shares the p2m with the CPU */
> >      return -ENOSYS;
> >  }
> > +
> > +void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
> > +{
> > +}
> 
> The option being in common code, I think you also need to set it for
> ARM, so it won't remain at its default of -1.
> 
> > @@ -144,16 +145,23 @@ static int __init parse_dom0_iommu_param(const char 
> > *s)
> >      int rc = 0;
> >  
> >      do {
> > +        bool val = !!strncmp(s, "no-", 3);
> 
> Oh, you do a literal comparison against "no-". Please don't, that's what
> we have parse_boolean() for.

Thanks, I will fix it. I've mostly cloned what the iommu option
currently does.

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

Thanks, Roger.

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