Xen devel - Jan 2012 - [PATCH] amd iommu: disable iommu emulation on non-iommu systems

Introduce a new flag to disable iommu emulation on old iommu systems.
This patch is taken from my v4 patch queue, which is till pending, to 
make old or non-iommu system to run cleanly without interfered by 
iommuv2 codes. This might be helpful to isolate iommuv2 code in 
debugging unstable regressions. The reset part of v4 will be re-based.

Thanks,
Wei

Signed-off-by: Wei Wang <wei.wang2@amd.com>




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 26.01.12 at 11:56, Wei Wang <wei.wang2@amd.com> wrote:
>--- a/xen/drivers/passthrough/amd/iommu_guest.c	Tue Jan 24 16:46:17 2012
+0000
>+++ b/xen/drivers/passthrough/amd/iommu_guest.c	Thu Jan 26 11:50:02 2012
+0100
>@@ -805,6 +805,9 @@ int guest_iommu_set_base(struct domain *
>     p2m_type_t t;
>     struct guest_iommu *iommu = domain_iommu(d);
> 
>+    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
>+        return 0;
Is it really appropriate/correct to return 0 here, while ...
>+
>     if ( !iommu )
>         return -EACCES;
> 
... here it is -EACCES? Further, are the extra checks needed at all
(i.e. wouldn''t domain_iommu() return NULL in all of these cases
anyway due to the same checks being added to guest_iommu_init())?
If so, the checks you add to guest_iommu_destroy() are pointless
too.

Jan

On 02/01/2012 04:58 PM, Jan Beulich wrote:
>>>> On 26.01.12 at 11:56, Wei Wang<wei.wang2@amd.com>  wrote:
>> --- a/xen/drivers/passthrough/amd/iommu_guest.c	Tue Jan 24 16:46:17
2012 +0000
>> +++ b/xen/drivers/passthrough/amd/iommu_guest.c	Thu Jan 26 11:50:02
2012 +0100
>> @@ -805,6 +805,9 @@ int guest_iommu_set_base(struct domain *
>>      p2m_type_t t;
>>      struct guest_iommu *iommu = domain_iommu(d);
>>
>> +    if ( !is_hvm_domain(d) || !iommu_enabled || !iommuv2_enabled )
>> +        return 0;
>
> Is it really appropriate/correct to return 0 here, while ...

good point, will be fixed in the next try. No one use 
guest_iommu_set_base so far until remaining patches got committed.
>> +
>>      if ( !iommu )
>>          return -EACCES;
>
> ... here it is -EACCES? Further, are the extra checks needed at all
> (i.e. wouldn''t domain_iommu() return NULL in all of these cases
> anyway due to the same checks being added to guest_iommu_init())?
> If so, the checks you add to guest_iommu_destroy() are pointless
> too.
It is just to make sure those functions are not called by an unexpected 
code path since it is non-static. But I can remove it if you prefer that.


Thanks,
Wei
> Jan
>
>

>>> On 03.02.12 at 15:09, Wei Wang <wei.wang2@amd.com> wrote:
> On 02/01/2012 04:58 PM, Jan Beulich wrote:
>> ... Further, are the extra checks needed at all
>> (i.e. wouldn''t domain_iommu() return NULL in all of these
cases
>> anyway due to the same checks being added to guest_iommu_init())?
>> If so, the checks you add to guest_iommu_destroy() are pointless
>> too.
> 
> It is just to make sure those functions are not called by an unexpected 
> code path since it is non-static. But I can remove it if you prefer that.
Keir already committed the patch, but converting things like this
(where the impossible is being checked) to assertions is preferred
imo (meaning less redundant, possibly dead code in production
builds), so a follow-up patch would be appreciated.

Jan

On 02/03/2012 03:11 PM, Jan Beulich wrote:
>>>> On 03.02.12 at 15:09, Wei Wang<wei.wang2@amd.com>  wrote:
>> On 02/01/2012 04:58 PM, Jan Beulich wrote:
>>> ... Further, are the extra checks needed at all
>>> (i.e. wouldn''t domain_iommu() return NULL in all of these
cases
>>> anyway due to the same checks being added to guest_iommu_init())?
>>> If so, the checks you add to guest_iommu_destroy() are pointless
>>> too.
>>
>> It is just to make sure those functions are not called by an unexpected
>> code path since it is non-static. But I can remove it if you prefer
that.
>
> Keir already committed the patch, but converting things like this
> (where the impossible is being checked) to assertions is preferred
> imo (meaning less redundant, possibly dead code in production
> builds), so a follow-up patch would be appreciated.
No problem. Do it right away.
Thanks,
Wei
> Jan
>
>