Re: [PATCH v6 6/6] x86/iommu: add map-reserved dom0-iommu option to map reserved memory ranges

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Aug 21, 2018 at 07:36:17AM -0600, Jan Beulich wrote:
> >>> On 21.08.18 at 09:49, <roger.pau@citrix.com> wrote:
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -704,6 +704,15 @@ This list of booleans controls the iommu usage by Dom0:
> >    option is only applicable to a PV Dom0 and is enabled by default on Intel
> >    hardware.
> >  
> > +* `map-reserved`: sets up DMA remapping for all the reserved regions in the
> > +  memory map for Dom0. Use this to work around firmware issues providing
> > +  incorrect RMRR/IVMD entries. Rather than only mapping RAM pages for IOMMU
> > +  accesses for Dom0, all memory regions marked as reserved in the memory map
> > +  that don't overlap with any MMIO region from emulated devices will be
> > +  identity mapped. This option maps a subset of the memory that would be
> > +  mapped when using the `map-inclusive` option. This option is available to a
> > +  PVH Dom0 and is enabled by default on Intel hardware.
> 
> This sounds as if the option was meaningless for PV, but I can't seem
> to see this being the case. The places setting iommu_hwdom_reserved
> don't look at domain type afaics, and the change to the default case
> in hwdom_iommu_map()'s switch() block has the is_hvm_domain() check
> independent of the iommu_hwdom_reserved one.

The option should be functional for PV also, so that a user could
have:

dom0-iommu=map-inclusive=0,map-reserved

> I also wonder about the wording "is available to": For a domain type
> restriction, would "only takes effect on" or some such be more to the
> point?

What about:

"This option is available to all Dom0 modes and is enabled by default
on Intel hardware."

> > @@ -138,16 +139,24 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
> >                                           unsigned long pfn,
> >                                           unsigned long max_pfn)
> >  {
> > +    unsigned int i, type;
> > +
> >      /*
> >       * Set up 1:1 mapping for dom0. Default to include only conventional RAM
> >       * areas and let RMRRs include needed reserved regions. When set, the
> >       * inclusive mapping additionally maps in every pfn up to 4GB except those
> > -     * that fall in unusable ranges.
> > +     * that fall in unusable ranges for PV Dom0.
> >       */
> > -    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) )
> > +    if ( (pfn > max_pfn && !mfn_valid(_mfn(pfn))) || xen_in_range(pfn) ||
> > +         /*
> > +          * Ignore any address below 1MB, that's already identity mapped by the
> > +          * Dom0 builder for HVM.
> > +          */
> > +         (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )
> >          return false;
> >  
> > -    switch ( page_get_ram_type(pfn) )
> > +    type = page_get_ram_type(pfn);
> > +    switch ( type )
> 
> Any reason not to keep this a single line, putting the assignment inside
> the switch()?

I don't like to put assignments inside of expressions, but I can
certainly do that.

> > @@ -158,10 +167,41 @@ static bool __hwdom_init hwdom_iommu_map(const struct domain *d,
> >          break;
> >  
> >      default:
> > -        if ( !iommu_hwdom_inclusive || pfn > max_pfn )
> > +        if ( type & RAM_TYPE_RESERVED )
> > +        {
> > +            if ( !iommu_hwdom_inclusive && !iommu_hwdom_reserved )
> > +                return false;
> > +        }
> > +        else if ( is_hvm_domain(d) || !iommu_hwdom_inclusive || pfn > max_pfn )
> >              return false;
> >      }
> >  
> > +    /*
> > +     * Check that it doesn't overlap with the LAPIC
> > +     * TODO: if the guest relocates the MMIO area of the LAPIC or IO-APIC Xen
> > +     * should make sure there's nothing in the new address that would prevent
> > +     * trapping.
> > +     */
> 
> Hmm, now you even mention the IO-APIC here. Does our / qemu's
> chipset emulation allow for this in the first place?

No, I didn't recall that the IO-APIC base is changed from a register
on the chipset and not from a MSR. I will remove this.

Thanks, Roger.

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