Re: [PATCH v2 06/18] IOMMU/x86: restrict IO-APIC mappings for PV Dom0

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

On Wed, Dec 01, 2021 at 12:45:12PM +0100, Jan Beulich wrote:
> On 01.12.2021 11:32, Roger Pau Monné wrote:
> > On Wed, Dec 01, 2021 at 10:27:21AM +0100, Jan Beulich wrote:
> >> On 01.12.2021 10:09, Roger Pau Monné wrote:
> >>> On Fri, Sep 24, 2021 at 11:46:57AM +0200, Jan Beulich wrote:
> >>>> @@ -267,44 +267,60 @@ static bool __hwdom_init hwdom_iommu_map
> >>>>       * that fall in unusable ranges for PV Dom0.
> >>>>       */
> >>>>      if ( (pfn > max_pfn && !mfn_valid(mfn)) || xen_in_range(pfn) )
> >>>> -        return false;
> >>>> +        return 0;
> >>>>  
> >>>>      switch ( type = page_get_ram_type(mfn) )
> >>>>      {
> >>>>      case RAM_TYPE_UNUSABLE:
> >>>> -        return false;
> >>>> +        return 0;
> >>>>  
> >>>>      case RAM_TYPE_CONVENTIONAL:
> >>>>          if ( iommu_hwdom_strict )
> >>>> -            return false;
> >>>> +            return 0;
> >>>>          break;
> >>>>  
> >>>>      default:
> >>>>          if ( type & RAM_TYPE_RESERVED )
> >>>>          {
> >>>>              if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> >>>> -                return false;
> >>>> +                perms = 0;
> >>>>          }
> >>>> -        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > 
> >>>> max_pfn )
> >>>> -            return false;
> >>>> +        else if ( is_hvm_domain(d) )
> >>>> +            return 0;
> >>>> +        else if ( !iommu_hwdom_inclusive || pfn > max_pfn )
> >>>> +            perms = 0;
> >>>
> >>> I'm confused about the reason to set perms = 0 instead of just
> >>> returning here. AFAICT perms won't be set to any other value below,
> >>> so you might as well just return 0.
> >>
> >> This is so that ...
> >>
> >>>>      }
> >>>>  
> >>>>      /* Check that it doesn't overlap with the Interrupt Address Range. 
> >>>> */
> >>>>      if ( pfn >= 0xfee00 && pfn <= 0xfeeff )
> >>>> -        return false;
> >>>> +        return 0;
> >>>>      /* ... or the IO-APIC */
> >>>> -    for ( i = 0; has_vioapic(d) && i < d->arch.hvm.nr_vioapics; i++ )
> >>>> -        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>>> -            return false;
> >>>> +    if ( has_vioapic(d) )
> >>>> +    {
> >>>> +        for ( i = 0; i < d->arch.hvm.nr_vioapics; i++ )
> >>>> +            if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> >>>> +                return 0;
> >>>> +    }
> >>>> +    else if ( is_pv_domain(d) )
> >>>> +    {
> >>>> +        /*
> >>>> +         * Be consistent with CPU mappings: Dom0 is permitted to 
> >>>> establish r/o
> >>>> +         * ones there, so it should also have such established for 
> >>>> IOMMUs.
> >>>> +         */
> >>>> +        for ( i = 0; i < nr_ioapics; i++ )
> >>>> +            if ( pfn == PFN_DOWN(mp_ioapics[i].mpc_apicaddr) )
> >>>> +                return rangeset_contains_singleton(mmio_ro_ranges, pfn)
> >>>> +                       ? IOMMUF_readable : 0;
> >>>> +    }
> >>
> >> ... this return, as per the comment, takes precedence over returning
> >> zero.
> > 
> > I see. This is because you want to map those in the IOMMU page tables
> > even if the IO-APIC ranges are outside of a reserved region.
> > 
> > I have to admit this is kind of weird, because the purpose of this
> > function is to add mappings for all memory below 4G, and/or for all
> > reserved regions.
> 
> Well, that was what it started out as. The purpose here is to be consistent
> about IO-APICs: Either have them all mapped, or none of them. Since we map
> them in the CPU page tables and since Andrew asked for the two mappings to
> be consistent, this is the only way to satisfy the requests. Personally I'd
> be okay with not mapping IO-APICs here (but then regardless of whether they
> are covered by a reserved region).

I'm unsure of the best way to deal with this, it seems like both
the CPU and the IOMMU page tables would never be equal for PV dom0,
because we have no intention to map the MSI-X tables in RO mode in the
IOMMU page tables.

I'm not really opposed to having the IO-APIC mapped RO in the IOMMU
page tables, but I also don't see much benefit of doing it unless we
have a user-case for it. The IO-APIC handling in PV is already
different from native, so I would be fine if we add a comment noting
that while the IO-APIC is mappable to the CPU page tables as RO it's
not present in the IOMMU page tables (and then adjust hwdom_iommu_map
to prevent it's mapping).

I think we should also prevent mapping of the LAPIC, the HPET and the
HyperTransport range in case they fall into a reserved region?

TBH looks like we should be using iomem_access_permitted in
arch_iommu_hwdom_init? (not just for the IO-APIC, but for other device
ranges)

> >>>> @@ -346,15 +362,19 @@ void __hwdom_init arch_iommu_hwdom_init(
> >>>>      for ( ; i < top; i++ )
> >>>>      {
> >>>>          unsigned long pfn = pdx_to_pfn(i);
> >>>> +        unsigned int perms = hwdom_iommu_map(d, pfn, max_pfn);
> >>>>          int rc;
> >>>>  
> >>>> -        if ( !hwdom_iommu_map(d, pfn, max_pfn) )
> >>>> +        if ( !perms )
> >>>>              rc = 0;
> >>>>          else if ( paging_mode_translate(d) )
> >>>> -            rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
> >>>> +            rc = set_identity_p2m_entry(d, pfn,
> >>>> +                                        perms & IOMMUF_writable ? 
> >>>> p2m_access_rw
> >>>> +                                                                : 
> >>>> p2m_access_r,
> >>>> +                                        0);
> >>>>          else
> >>>>              rc = iommu_map(d, _dfn(pfn), _mfn(pfn), 1ul << 
> >>>> PAGE_ORDER_4K,
> >>>> -                           IOMMUF_readable | IOMMUF_writable, 
> >>>> &flush_flags);
> >>>> +                           perms, &flush_flags);
> >>>
> >>> You could just call set_identity_p2m_entry uniformly here. It will
> >>> DTRT for non-translated guests also, and then hwdom_iommu_map could
> >>> perhaps return a p2m_access_t?
> >>
> >> That's an orthogonal change imo, i.e. could be done as a prereq change,
> >> but I'd prefer to leave it as is for now. Furthermore see "x86/mm: split
> >> set_identity_p2m_entry() into PV and HVM parts": In v2 I'm now also
> >> adjusting the code here 
> > 
> > I would rather adjust the code here to just call
> > set_identity_p2m_entry instead of differentiating between PV and
> > HVM.
> 
> I'm a little hesitant, in particular considering your suggestion to
> then have hwdom_iommu_map() return p2m_access_t. Andrew has made quite
> clear that the use of p2m_access_* here and in a number of other places
> is actually an abuse.
> 
> Plus - forgot about this in my earlier reply - there would also be a
> conflict with the next patch in this series, where larger orders will
> get passed to iommu_map(), while set_identity_p2m_entry() has no
> respective parameter yet (and imo it isn't urgent for it to gain one).

I've seen now as the iommu_map path is modified to handle ranges
instead of single pages. Long term we also want to expand this range
handling to the HVM branch.

Thanks, Roger.