Re: [PATCH v7 01/10] iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute

[Robin Murphy]

On 22/04/16 13:00, Eric Auger wrote:
> Hi Robin,
> On 04/22/2016 01:31 PM, Robin Murphy wrote:
> > On 20/04/16 16:58, Eric Auger wrote:
> > > Hi Robin,
> > > On 04/20/2016 02:47 PM, Robin Murphy wrote:
> > > > Hi Eric,
> > > >
> > > > On 19/04/16 17:56, Eric Auger wrote:
> > > > > Introduce a new DOMAIN_ATTR_MSI_MAPPING domain attribute. If supported,
> > > > > this means the MSI addresses need to be mapped in the IOMMU.
> > > > >
> > > > > x86 IOMMUs typically don't expose the attribute since on x86, MSI write
> > > > > transaction addresses always are within the 1MB PA region [FEE0_0000h -
> > > > > FEF0_000h] window which directly targets the APIC configuration
> > > > > space and
> > > > > hence bypass the sMMU. On ARM and PowerPC however MSI transactions are
> > > > > conveyed through the IOMMU.
> > > >
> > > > What's stopping us from simply inferring this from the domain's IOMMU
> > > > not advertising interrupt remapping capabilities?
> > > My current understanding is it is not possible:
> > > on x86 CAP_INTR_REMAP is not systematically exposed (the feature can be
> > > disabled) and MSIs are never mapped in the IOMMU I think.
> >
> > Not sure I follow - if the feature is disabled such that the IOMMU
> > doesn't isolate MSIs, then it's no different a situation from the SMMU, no?
>
> sorry I understood you wanted to use IOMMU_CAP_INTR_REMAP as the sole
> criteria to detect whether MSI mapping was requested.
> > My point was that this logic:
> >
> >      if (IOMMU_CAP_INTR_REMAP)
> >          we're good
> >      else if (DOMAIN_ATTR_MSI_MAPPING)
> >          if (acquire_msi_remapping_resources(domain))
> >              we're good
> >          else
> >              oh no!
> >      else
> >          oh no!
> >
> > should be easily reducible to this:
> >
> >      if (IOMMU_CAP_INTR_REMAP)
> >          we're good
> >      else if (acquire_msi_remapping_resources(domain))
>
> But Can't we imagine a mix of smmus on the same platform, some
> requesting MSI mapping and some which don't. As soon as an smmu requires
> MSI mapping, CONFIG_IOMMU_DMA_RESERVED is set and
> acquire_msi_remapping_resources(domain) will be implemented & succeed.
> Doesn't it lead to a wrong decision. Do I miss something, or do you
> consider this situation as far-fetched?

Sorry, what was implicit there was that the imaginary 
acquire_msi_remapping_resources(*IOMMU* domian) call still involves 
going all the way down to check for MSI_FLAG_IRQ_REMAPPING in the 
relevant place.

Either way, now that I consider it further, a flag on the IOMMU domain 
is not just unnecessary, I think it's actually fundamentally incorrect: 
picture a system which for some crazy reason has both a GICv3 ITS plus 
some other dumb v2m-like MMIO-SPI bridge - whether a device's MSI domain 
targets the (safe) ITS or the (unsafe) bridge isn't a property of the 
IOMMU domain it's trying to attach to; if you can't rely on the IOMMU 
itself to isolate MSIs, then you can only know whether to allow or 
reject any given group by inspecting every device in that group to make 
sure they all have isolation provided by their MSI domains and that the 
IOMMU domain has all the relevant doorbell mappings ready.

I guess the allow_unsafe_interrupts case might complicate matters beyond 
the logic I sketched out, because then we might actually care about the 
difference between "is isolation provided?" and "are sufficient 
IOVA/descriptor resources available?", but the main point still stands.

Robin.

> Thanks
>
> Eric
>
> >          we're good
> >      else
> >          oh no!    // Don't care whether the domain ran out of
> >              // resources or simply doesn't support it,
> >              // either way we can't proceed.
> >
> > Robin.
> >
> > > Best Regards
> > >
> > > Eric
> > > > Robin.
> > > >
> > > > > Signed-off-by: Bharat Bhushan <Bharat.Bhushan@xxxxxxxxxxxxx>
> > > > > Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > > > >
> > > > > ---
> > > > >
> > > > > v4 -> v5:
> > > > > - introduce the user in the next patch
> > > > >
> > > > > RFC v1 -> v1:
> > > > > - the data field is not used
> > > > > - for this attribute domain_get_attr simply returns 0 if the
> > > > > MSI_MAPPING
> > > > >      capability if needed or <0 if not.
> > > > > - removed struct iommu_domain_msi_maps
> > > > > ---
> > > > >     include/linux/iommu.h | 1 +
> > > > >     1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > > > > index 62a5eae..b3e8c5b 100644
> > > > > --- a/include/linux/iommu.h
> > > > > +++ b/include/linux/iommu.h
> > > > > @@ -113,6 +113,7 @@ enum iommu_attr {
> > > > >         DOMAIN_ATTR_FSL_PAMU_ENABLE,
> > > > >         DOMAIN_ATTR_FSL_PAMUV1,
> > > > >         DOMAIN_ATTR_NESTING,    /* two stages of translation */
> > > > > +    DOMAIN_ATTR_MSI_MAPPING, /* Require MSIs mapping in iommu */
> > > > >         DOMAIN_ATTR_MAX,
> > > > >     };