Re: [Xen-devel] [PATCH v6 10/14] mm / iommu: split need_iommu() into has_iommu_pt() and need_iommu_pt_sync()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.08.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/mtrr.c
> +++ b/xen/arch/x86/hvm/mtrr.c
> @@ -793,7 +793,8 @@ HVM_REGISTER_SAVE_RESTORE(MTRR, hvm_save_mtrr_msr, 
> hvm_load_mtrr_msr,
>  
>  void memory_type_changed(struct domain *d)
>  {
> -    if ( need_iommu(d) && d->vcpu && d->vcpu[0] )
> +    if ( (need_iommu_pt_sync(d) || iommu_use_hap_pt(d)) &&
> +         d->vcpu && d->vcpu[0] )
>      {
>          p2m_memory_type_changed(d);
>          flush_all(FLUSH_CACHE);

How does need_iommu_pt_sync() come into play here? To me,
has_iommu_pt() would seem to be a better match. The question
here solely is (iirc) whether memory types take any effect in the
first place (or else we can skip adjusting them), which in turn
solely depends on whether there are any pass-through devices
in the domain. This is along the lines of ...

> @@ -841,7 +842,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;

... this.

> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -1426,7 +1426,8 @@ int memory_add(unsigned long spfn, unsigned long epfn, 
> unsigned int pxm)
>      if ( ret )
>          goto destroy_m2p;
>  
> -    if ( iommu_enabled && !iommu_passthrough && !need_iommu(hardware_domain) 
> )
> +    if ( iommu_enabled && !iommu_passthrough &&
> +         !need_iommu_pt_sync(hardware_domain) )
>      {
>          for ( i = spfn; i < epfn; i++ )
>              if ( iommu_map_page(hardware_domain, _bfn(i), _mfn(i),

I'm confused - the condition you change looks to be inverted. Wouldn't
we better fix this?

And then I again wonder whether you've chosen the right predicate:
Where would the equivalent mappings come from in the opposite case?

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -805,8 +805,8 @@ int xenmem_add_to_physmap(struct domain *d, struct 
> xen_add_to_physmap *xatp,
>      xatp->size -= start;
>  
>  #ifdef CONFIG_HAS_PASSTHROUGH
> -    if ( need_iommu(d) )
> -        this_cpu(iommu_dont_flush_iotlb) = 1;
> +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
> +       this_cpu(iommu_dont_flush_iotlb) = 1;
>  #endif

Rather than making the conditional more complicated, perhaps
simply drop it (and move the reset-to-false code out of ...

> @@ -828,7 +828,7 @@ int xenmem_add_to_physmap(struct domain *d, struct 
> xen_add_to_physmap *xatp,
>      }
>  
>  #ifdef CONFIG_HAS_PASSTHROUGH
> -    if ( need_iommu(d) )
> +    if ( need_iommu_pt_sync(d) || iommu_use_hap_pt(d) )
>      {
>          int ret;

... this if())?

Also it looks to me as if here you've got confused by the meaning
you've assigned to need_iommu_pt_sync(): According to the
description, it is about sync-ing of page tables. Here, however,
we're checking whether to flush TLBs.

> @@ -179,8 +179,10 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          return;
>  
>      register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 
> 0);
> -    d->need_iommu = !!iommu_dom0_strict;
> -    if ( need_iommu(d) && !iommu_use_hap_pt(d) )
> +
> +    hd->status = IOMMU_STATUS_initializing;
> +    hd->need_sync = !!iommu_dom0_strict && !iommu_use_hap_pt(d);

Pointless !! afaict.

>  int iommu_construct(struct domain *d)
>  {
> -    if ( need_iommu(d) > 0 )
> +    struct domain_iommu *hd = dom_iommu(d);
> +
> +    if ( hd->status == IOMMU_STATUS_initialized )
>          return 0;
>  
> -    if ( !iommu_use_hap_pt(d) )
> +    if ( !iommu_use_hap_pt(d) && hd->status < IOMMU_STATUS_initialized )

Isn't the addition here redundant with the earlier if()?

> @@ -889,9 +886,11 @@ void watchdog_domain_destroy(struct domain *d);
>  #define is_pinned_vcpu(v) ((v)->domain->is_pinned || \
>                             cpumask_weight((v)->cpu_hard_affinity) == 1)
>  #ifdef CONFIG_HAS_PASSTHROUGH
> -#define need_iommu(d)    ((d)->need_iommu)
> +#define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> +#define need_iommu_pt_sync(d) (dom_iommu(d)->need_sync)
>  #else
> -#define need_iommu(d)    (0)
> +#define has_iommu_pt(d) (0)
> +#define need_iommu_pt_sync(d) (0)

Please use false here (and no need for the parentheses).

Jan


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