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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.09.18 at 16:33, <paul.durrant@xxxxxxxxxx> wrote:
> v12:
>  - Fix two mis-uses of iommu_hap_pt_share().

I had hoped that with my reply to v11 you would have gone through
all uses, not just the ones your series adds or modifies. At the very
least the one in iommu_construct() is bogus too, as PV domains also
make it there.

Perhaps in the end we're better off adding is_hvm_domain() right to
hap_enabled(). Patch just sent.

> --- 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_hap_pt_share && !iommu_hwdom_strict )

Why not !iommu_use_hap_pt()? Irrespective of HAP and sharing being
available, the hardware domain may not use HAP and/or sharing. Also
the use of a the global variable here renders the PV case wrong in at
least one of the possible combinations.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1416,10 +1416,9 @@ 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) &&
> -            (d->arch.hvm.mem_sharing_enabled ||
> -             vm_event_check_ring(d->vm_event_paging) ||
> -             p2m_get_hostp2m(d)->global_logdirty)) )
> +    if ( unlikely(d->arch.hvm.mem_sharing_enabled ||
> +                  vm_event_check_ring(d->vm_event_paging) ||
> +                  p2m_get_hostp2m(d)->global_logdirty) )
>          return -EXDEV;

While as per v11 I'm in full agreement with this removal, I think it needs
calling out in the description.

Jan



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