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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.08.18 at 09:49, <roger.pau@xxxxxxxxxx> 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.

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?

> @@ -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()?

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

Jan



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