Wei Wang
2012-Jan-26 10:56 UTC
[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
Jan Beulich
2012-Feb-01 15:58 UTC
Re: [PATCH] amd iommu: disable iommu emulation on non-iommu systems
>>> 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
Wei Wang
2012-Feb-03 14:09 UTC
Re: [PATCH] amd iommu: disable iommu emulation on non-iommu systems
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 > >
Jan Beulich
2012-Feb-03 14:11 UTC
Re: [PATCH] amd iommu: disable iommu emulation on non-iommu systems
>>> 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
Wei Wang
2012-Feb-03 14:28 UTC
Re: [PATCH] amd iommu: disable iommu emulation on non-iommu systems
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 > >