Re: [PATCH v2 4/4] xen/pci: solve compilation error on ARM with HAS_PCI enabled.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.11.2020 16:59, Rahul Singh wrote:
> If mem-sharing, mem-paging and log-dirty functionality is not enabled
> for architecture when HAS_PCI is enabled, compiler will throw an error.

Nit: Is it really "and", not "or"?

> @@ -1418,12 +1417,7 @@ static int assign_device(struct domain *d, u16 seg, u8 
> bus, u8 devfn, u32 flag)
>      if ( !is_iommu_enabled(d) )
>          return 0;
>  
> -    /* Prevent device assign if mem paging or mem sharing have been 
> -     * enabled for this domain */
> -    if ( d != dom_io &&
> -         unlikely(mem_sharing_enabled(d) ||
> -                  vm_event_check_ring(d->vm_event_paging) ||
> -                  p2m_get_hostp2m(d)->global_logdirty) )
> +    if( !arch_iommu_usable(d) )
>          return -EXDEV;

While iirc I did suggest this name, seeing it used here leaves me
somewhat unhappy with the name, albeit I also can't think of any
better alternative right now. Maybe arch_iommu_use_permitted()?

> @@ -315,6 +316,18 @@ int iommu_update_ire_from_msi(
>             ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>  }
>  
> +bool_t arch_iommu_usable(struct domain *d)

Just bool please and I very much hope the parameter can be const.

> +{
> +
> +    /* Prevent device assign if mem paging or mem sharing have been
> +     * enabled for this domain */

Please correct comment style as you move it.

> +    if ( d != dom_io && unlikely(mem_sharing_enabled(d) ||
> +                        vm_event_check_ring(d->vm_event_paging) ||
> +                        p2m_get_hostp2m(d)->global_logdirty) )

You've screwed up indentation, and I don't see why ...

> +        return false;
> +    else
> +        return true;
> +}

... this can't be a simple single return statement anyway:

    return d == dom_io ||
           likely(!mem_sharing_enabled(d) &&
                  !vm_event_check_ring(d->vm_event_paging) &&
                  !p2m_get_hostp2m(d)->global_logdirty);

In the course of moving I'd also suggest dropping the use of
likely() here: The way it's used (on an && expression) isn't
normally having much effect anyway. If anything it should imo
be

    return d == dom_io ||
           (likely(!mem_sharing_enabled(d)) &&
            likely(!vm_event_check_ring(d->vm_event_paging)) &&
            likely(!p2m_get_hostp2m(d)->global_logdirty));

Any transformation to this effect wants mentioning in the
description, though.

Jan