Re: [Xen-devel] [PATCH] x86/iommu: remove usage of {set/clear}_identity_p2m_entry against PV domains

[Jan Beulich <JBeulich@xxxxxxxx>]

On 02.08.2019 11:22, Roger Pau Monne wrote:
> Switch rmrr_identity_mapping to use iommu_{un}map in order to
> establish RMRR mappings for PV domains, like it's done in
> arch_iommu_hwdom_init. This solves the issue of a PV hardware domain
> not getting RMRR mappings because {set/clear}_identity_p2m_entry was
> not properly updating the iommu page tables.

In which was this was not happening properly is still not really
understood, yet I think this (or at least a plausible theory) is
a prereq for any kind of fix. The code paths for PV and HVM are
probably different enough such that we don't need to be afraid
of there being a similar problem on the HVM side, but what if
there's a more general problem that we would better take care of,
rather perhaps than getting into a similarly puzzling situation
again later on.

> As rmrr_identity_mapping was the last user of
> {set/clear}_identity_p2m_entry against PV domains modify the function
> so it's only usable against translated domains, as the other p2m
> related functions.

Looking at the resulting code I'm actually not convinced that
this is a good move. I would, however, specifically like to
hear George's opinion here.

> @@ -1995,13 +1996,20 @@ static int rmrr_identity_mapping(struct domain *d, 
> bool_t map,
>   
>               while ( base_pfn < end_pfn )
>               {
> -                if ( clear_identity_p2m_entry(d, base_pfn) )
> -                    ret = -ENXIO;
> +                if ( paging_mode_translate(d) )
> +                    ret = clear_identity_p2m_entry(d, base_pfn);
> +                else
> +                    ret = iommu_unmap(d, _dfn(base_pfn), PAGE_ORDER_4K,
> +                                      &flush_flags);
>                   base_pfn++;
>               }
>   
>               list_del(&mrmrr->list);
>               xfree(mrmrr);
> +            /* Keep the previous error code if there's one. */
> +            err = iommu_iotlb_flush_all(d, flush_flags);
> +            if ( !ret )
> +                ret = err;
>               return ret;
>           }
>       }
> @@ -2011,8 +2019,13 @@ static int rmrr_identity_mapping(struct domain *d, 
> bool_t map,
>   
>       while ( base_pfn < end_pfn )
>       {
> -        int err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        int err;
>   
> +        if ( paging_mode_translate(d) )
> +            err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw, flag);
> +        else
> +            err = iommu_map(d, _dfn(base_pfn), _mfn(base_pfn), PAGE_ORDER_4K,
> +                            IOMMUF_readable | IOMMUF_writable, &flush_flags);
>           if ( err )
>               return err;
>           base_pfn++;
> @@ -2026,7 +2039,7 @@ static int rmrr_identity_mapping(struct domain *d, 
> bool_t map,
>       mrmrr->count = 1;
>       list_add_tail(&mrmrr->list, &hd->arch.mapped_rmrrs);
>   
> -    return 0;
> +    return iommu_iotlb_flush_all(d, flush_flags);

This is VT-d code - is there a particular reason why you go through
the generic code wrappers everywhere here?

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