Re: [PATCH v2 1/2] xen/passthrough: Provide stub functions when !HAS_PASSTHROUGH

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> 
>>>> --- a/xen/include/xen/iommu.h
>>>> +++ b/xen/include/xen/iommu.h
>>>> @@ -110,6 +110,8 @@ extern int8_t iommu_hwdom_reserved;
>>>> 
>>>> extern unsigned int iommu_dev_iotlb_timeout;
>>>> 
>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>> +
>>>> int iommu_setup(void);
>>>> int iommu_hardware_setup(void);
>>>> 
>>>> @@ -122,6 +124,24 @@ int arch_iommu_domain_init(struct domain *d);
>>>> void arch_iommu_check_autotranslated_hwdom(struct domain *d);
>>>> void arch_iommu_hwdom_init(struct domain *d);
>>>> 
>>>> +#else
>>>> +
>>>> +static inline int iommu_setup(void)
>>>> +{
>>>> +    return -ENODEV;
>>>> +}
>>>> +
>>>> +static inline int iommu_domain_init(struct domain *d, unsigned int opts)
>>>> +{
>>>> +    return 0;
>>> 
>>> Shouldn't this fail when is_iommu_enabled(d) is true? (The use of the
>>> predicate here as well as in the real function is slightly strange, but
>>> that's the way it is.)
>> 
>> Right, probably you know better this code than me, I started from the 
>> assumption
>> that when !HAS_PASSTHROUGH, 'iommu_enabled' is false.
>> 
>> is_iommu_enabled(d) checks if the domain structure ‘options’ field has
>> XEN_DOMCTL_CDF_iommu, this flag is set on domain creation when 
>> ‘iommu_enabled'
>> is true on arm and x86.
>> 
>> So when !HAS_PASSTHROUGH can we assume is_iommu_enabled(d) give false?
>> Or shall we return for example the value of is_iommu_enabled(d)?
> 
> Since HAS_PASSTHROUGH being selected conditionally a (pretty) new, I
> fear that assumptions shouldn't be made. It's possible the stub could
> remain as is, yet even then - if only for documentation purposes - I'd
> suggest to have some ASSERT() there. In the end it all depends on how
> XEN_DOMCTL_CDF_iommu is handled when !HAS_PASSTHROUGH.

I’ve tried to add an ASSERT(!is_iommu_enabled(d)); but it’s not building, I’m 
starting to think there
is some reason why I can’t do that but I didn’t figure out why, I’ve added the 
inclusion for xen/sched.h,
but it still says implicit declaration of function ‘is_iommu_enabled’…

But I could assert for !iommu_enabled: I checked into common/domain.c, 
sanitise_domain_config,
if a domain is called with XEN_DOMCTL_CDF_iommu set, the function would fail if 
!iommu_enabled,
so I would say that the stub returns the expected value (0) since for sure 
iommu_enabled is false and
there cannot be a domain with that flag set that has the iommu_enabled=true 
under !HAS_PASSTHROUGH.

But would it be ok to add this assert (ASSERT(!iommu_enabled);) even if we know 
that iommu_enabled
is false, since !HAS_PASSTHROUGH ?

Please let me know your thoughts on this.

Cheers,
Luca