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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.08.18 at 15:43, <roger.pau@xxxxxxxxxx> wrote:
> @@ -138,13 +139,20 @@ static bool __hwdom_init hwdom_iommu_map(const struct 
> domain *d,
>                                           unsigned long pfn,
>                                           unsigned long max_pfn)
>  {
> +    unsigned int i;
> +
>      /*
>       * 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
> +          * domain builder for HVM.
> +          */
> +         (!d->domain_id && is_hvm_domain(d) && pfn < PFN_DOWN(MB(1))) )

Would you mind clarifying the comment by saying "Dom0 builder"?
The respective source files have been renamed to dom0_build.c
for the same reason, quite some time ago.

> +    /* Check that it doesn't overlap with the LAPIC */
> +    if ( has_vlapic(d) )
> +    {
> +        const struct vcpu *v;
> +
> +        for_each_vcpu(d, v)
> +            if ( pfn == PFN_DOWN(vlapic_base_address(vcpu_vlapic(v))) )
> +                return false;
> +    }
> +    /* ... or the IO-APIC */
> +    for ( i = 0; has_vioapic(d) && i < d->arch.hvm_domain.nr_vioapics; i++ )
> +        if ( pfn == PFN_DOWN(domain_vioapic(d, i)->base_address) )
> +            return false;
> +    /*
> +     * ... or the PCIe MCFG regions.
> +     * TODO: runtime added MMCFG regions are not checked to make sure they
> +     * don't overlap with already mapped regions, thus preventing trapping.
> +     */
> +    if ( has_vpci(d) && vpci_is_mmcfg_address(d, pfn_to_paddr(pfn)) )
> +        return false;

Didn't we agree to have such a TODO also in the LAPIC loop?

> @@ -185,7 +219,13 @@ void __hwdom_init arch_iommu_hwdom_init(struct domain *d)
>          if ( !hwdom_iommu_map(d, pfn, max_pfn) )
>              continue;
>  
> -        rc = iommu_map_page(d, pfn, pfn, IOMMUF_readable|IOMMUF_writable);
> +        if ( iommu_use_hap_pt(d) )
> +        {
> +            ASSERT(is_hvm_domain(d));
> +            rc = set_identity_p2m_entry(d, pfn, p2m_access_rw, 0);
> +        }
> +        else
> +            rc = iommu_map_page(d, pfn, pfn, 
> IOMMUF_readable|IOMMUF_writable);

Why iommu_use_hap_pt()? Shouldn't HAP with or without shared
page tables as well as shadow all get the same in-sync p2m and
IOMMU mappings?

Jan



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