Re: [Xen-devel] [PATCH v11 9/9] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.09.18 at 12:56, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -783,7 +783,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, 
> hvm_load_mtrr_msr, 1,
>  
>  void memory_type_changed(struct domain *d)
>  {
> -    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
> +    if ( (has_iommu_pt(d) || iommu_use_hap_pt(d)) &&
> +         d->vcpu && d->vcpu[0] )
>      {
>          p2m_memory_type_changed(d);
>          flush_all(FLUSH_CACHE);
> @@ -831,7 +832,7 @@ int epte_get_entry_emt(struct domain *d, unsigned long 
> gfn, mfn_t mfn,
>          return MTRR_TYPE_UNCACHABLE;
>      }
>  
> -    if ( !need_iommu(d) && !cache_flush_permitted(d) )
> +    if ( !has_iommu_pt(d) && !cache_flush_permitted(d) )
>      {
>          *ipat = 1;
>          return MTRR_TYPE_WRBACK;

Considering how closely the two functions are related I'm struggling to
understand why the conditions are no longer the inverse of one another.
With iommu_use_hap_pt() including a has_iommu_pt() check I think the
former can and should be simplified.

> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1426,8 +1426,13 @@ int memory_add(unsigned long spfn, unsigned long epfn, 
> unsigned int pxm)
>      if ( ret )
>          goto destroy_m2p;
>  
> -    if ( iommu_enabled && !iommu_hwdom_passthrough &&
> -         !need_iommu(hardware_domain) )
> +    /*
> +     * If hardware domain has IOMMU mappings but page tables are not
> +     * shared, and are not being kept in sync (which is the case when
> +     * in strict mode) then newly added memory needs to be mapped here.
> +     */
> +    if ( has_iommu_pt(hardware_domain) &&
> +         !iommu_use_hap_pt(hardware_domain) && !iommu_hwdom_strict )

iommu_use_hap_pt() includes a hap_enabled() check, but that is valid
to be used on HVM domains only. It looks like there are other similar
improper uses elsewhere - all new and pre-existing uses need auditing.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1416,7 +1416,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>  
>      /* Prevent device assign if mem paging or mem sharing have been 
>       * enabled for this domain */
> -    if ( unlikely(!need_iommu(d) &&
> +    if ( unlikely(!has_iommu_pt(d) &&
>              (d->arch.hvm.mem_sharing_enabled ||
>               vm_event_check_ring(d->vm_event_paging) ||
>               p2m_get_hostp2m(d)->global_logdirty)) )

This need_iommu() check looks rather unmotivated to me - wouldn't
you better delete it instead of finding a suitable replacement?

Jan



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