* [PATCH] IOMMU: don't BUG() on exotic hardware
@ 2016-05-06 14:24 Jan Beulich
2016-05-06 14:31 ` Wei Liu
2016-05-09 7:55 ` Xu, Quan
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2016-05-06 14:24 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu
[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]
On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
hardware. While, with our current code, that's a correct prerequisite
assumption for IOMMU presence, this is wrong on systems without IOMMU.
Hence iommu_enabled (and alike) checks should be done prior to calling
that function, not after.
Also move iommu_suspend() next to iommu_resume() - it escapes me why
iommu_do_domctl() had got put between the two.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -337,11 +337,16 @@ int __init iommu_setup(void)
return rc;
}
+void iommu_suspend()
+{
+ if ( iommu_enabled )
+ iommu_get_ops()->suspend();
+}
+
void iommu_resume()
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_enabled )
- ops->resume();
+ iommu_get_ops()->resume();
}
int iommu_do_domctl(
@@ -365,34 +370,28 @@ int iommu_do_domctl(
return ret;
}
-void iommu_suspend()
-{
- const struct iommu_ops *ops = iommu_get_ops();
- if ( iommu_enabled )
- ops->suspend();
-}
-
void iommu_share_p2m_table(struct domain* d)
{
- const struct iommu_ops *ops = iommu_get_ops();
-
if ( iommu_enabled && iommu_use_hap_pt(d) )
- ops->share_p2m(d);
+ iommu_get_ops()->share_p2m(d);
}
void iommu_crash_shutdown(void)
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_enabled )
- ops->crash_shutdown();
+ iommu_get_ops()->crash_shutdown();
iommu_enabled = iommu_intremap = iommu_intpost = 0;
}
int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
{
- const struct iommu_ops *ops = iommu_get_ops();
+ const struct iommu_ops *ops;
+
+ if ( !iommu_enabled )
+ return 0;
- if ( !iommu_enabled || !ops->get_reserved_device_memory )
+ ops = iommu_get_ops();
+ if ( !ops->get_reserved_device_memory )
return 0;
return ops->get_reserved_device_memory(func, ctxt);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1241,16 +1241,15 @@ __initcall(setup_dump_pcidevs);
int iommu_update_ire_from_msi(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
- const struct iommu_ops *ops = iommu_get_ops();
- return iommu_intremap ? ops->update_ire_from_msi(msi_desc, msg) : 0;
+ return iommu_intremap
+ ? iommu_get_ops()->update_ire_from_msi(msi_desc, msg) : 0;
}
void iommu_read_msi_from_ire(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_intremap )
- ops->read_msi_from_ire(msi_desc, msg);
+ iommu_get_ops()->read_msi_from_ire(msi_desc, msg);
}
int iommu_add_device(struct pci_dev *pdev)
[-- Attachment #2: IOMMU-defer-get-ops.patch --]
[-- Type: text/plain, Size: 2904 bytes --]
IOMMU: don't BUG() on exotic hardware
On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
hardware. While, with our current code, that's a correct prerequisite
assumption for IOMMU presence, this is wrong on systems without IOMMU.
Hence iommu_enabled (and alike) checks should be done prior to calling
that function, not after.
Also move iommu_suspend() next to iommu_resume() - it escapes me why
iommu_do_domctl() had got put between the two.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -337,11 +337,16 @@ int __init iommu_setup(void)
return rc;
}
+void iommu_suspend()
+{
+ if ( iommu_enabled )
+ iommu_get_ops()->suspend();
+}
+
void iommu_resume()
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_enabled )
- ops->resume();
+ iommu_get_ops()->resume();
}
int iommu_do_domctl(
@@ -365,34 +370,28 @@ int iommu_do_domctl(
return ret;
}
-void iommu_suspend()
-{
- const struct iommu_ops *ops = iommu_get_ops();
- if ( iommu_enabled )
- ops->suspend();
-}
-
void iommu_share_p2m_table(struct domain* d)
{
- const struct iommu_ops *ops = iommu_get_ops();
-
if ( iommu_enabled && iommu_use_hap_pt(d) )
- ops->share_p2m(d);
+ iommu_get_ops()->share_p2m(d);
}
void iommu_crash_shutdown(void)
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_enabled )
- ops->crash_shutdown();
+ iommu_get_ops()->crash_shutdown();
iommu_enabled = iommu_intremap = iommu_intpost = 0;
}
int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt)
{
- const struct iommu_ops *ops = iommu_get_ops();
+ const struct iommu_ops *ops;
+
+ if ( !iommu_enabled )
+ return 0;
- if ( !iommu_enabled || !ops->get_reserved_device_memory )
+ ops = iommu_get_ops();
+ if ( !ops->get_reserved_device_memory )
return 0;
return ops->get_reserved_device_memory(func, ctxt);
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1241,16 +1241,15 @@ __initcall(setup_dump_pcidevs);
int iommu_update_ire_from_msi(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
- const struct iommu_ops *ops = iommu_get_ops();
- return iommu_intremap ? ops->update_ire_from_msi(msi_desc, msg) : 0;
+ return iommu_intremap
+ ? iommu_get_ops()->update_ire_from_msi(msi_desc, msg) : 0;
}
void iommu_read_msi_from_ire(
struct msi_desc *msi_desc, struct msi_msg *msg)
{
- const struct iommu_ops *ops = iommu_get_ops();
if ( iommu_intremap )
- ops->read_msi_from_ire(msi_desc, msg);
+ iommu_get_ops()->read_msi_from_ire(msi_desc, msg);
}
int iommu_add_device(struct pci_dev *pdev)
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] IOMMU: don't BUG() on exotic hardware
2016-05-06 14:24 [PATCH] IOMMU: don't BUG() on exotic hardware Jan Beulich
@ 2016-05-06 14:31 ` Wei Liu
2016-05-06 15:28 ` Dario Faggioli
2016-05-09 7:55 ` Xu, Quan
1 sibling, 1 reply; 6+ messages in thread
From: Wei Liu @ 2016-05-06 14:31 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Wei Liu
On Fri, May 06, 2016 at 08:24:28AM -0600, Jan Beulich wrote:
> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
> hardware. While, with our current code, that's a correct prerequisite
> assumption for IOMMU presence, this is wrong on systems without IOMMU.
> Hence iommu_enabled (and alike) checks should be done prior to calling
> that function, not after.
>
> Also move iommu_suspend() next to iommu_resume() - it escapes me why
> iommu_do_domctl() had got put between the two.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IOMMU: don't BUG() on exotic hardware
2016-05-06 14:31 ` Wei Liu
@ 2016-05-06 15:28 ` Dario Faggioli
0 siblings, 0 replies; 6+ messages in thread
From: Dario Faggioli @ 2016-05-06 15:28 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, Jan Beulich
On Fri, May 6, 2016 at 4:31 PM, Wei Liu <wei.liu2@citrix.com> wrote:
> On Fri, May 06, 2016 at 08:24:28AM -0600, Jan Beulich wrote:
>> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
>> hardware. While, with our current code, that's a correct prerequisite
>> assumption for IOMMU presence, this is wrong on systems without IOMMU.
>> Hence iommu_enabled (and alike) checks should be done prior to calling
>> that function, not after.
>>
>> Also move iommu_suspend() next to iommu_resume() - it escapes me why
>> iommu_do_domctl() had got put between the two.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Release-acked-by: Wei Liu <wei.liu2@citrix.com>
>
FWIW,
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>
Regards,
Dario
--
<<This happens because I choose it to happen!>> (Raistlin Majere)
---------------------------------------------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IOMMU: don't BUG() on exotic hardware
2016-05-06 14:24 [PATCH] IOMMU: don't BUG() on exotic hardware Jan Beulich
2016-05-06 14:31 ` Wei Liu
@ 2016-05-09 7:55 ` Xu, Quan
2016-05-09 8:24 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Xu, Quan @ 2016-05-09 7:55 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Wei Liu
On May 06, 2016 10:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
> hardware. While, with our current code, that's a correct prerequisite
> assumption for IOMMU presence, this is wrong on systems without IOMMU.
> Hence iommu_enabled (and alike) checks should be done prior to calling that
> function, not after.
>
> Also move iommu_suspend() next to iommu_resume() - it escapes me why
> iommu_do_domctl() had got put between the two.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -337,11 +337,16 @@ int __init iommu_setup(void)
> return rc;
> }
>
> +void iommu_suspend()
> +{
> + if ( iommu_enabled )
> + iommu_get_ops()->suspend();
> +}
> +
What about this style:
+void iommu_suspend()
+{
+ if ( iommu_enabled &&
+ iommu_get_ops()->suspend )
+ iommu_get_ops()->suspend();
+}
+
At least for AMD, not all of the .callback are initialized.
> void iommu_crash_shutdown(void)
> {
> - const struct iommu_ops *ops = iommu_get_ops();
> if ( iommu_enabled )
> - ops->crash_shutdown();
> + iommu_get_ops()->crash_shutdown();
> iommu_enabled = iommu_intremap = iommu_intpost = 0;
btw, is this line still a code style issue?
}
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] IOMMU: don't BUG() on exotic hardware
2016-05-09 7:55 ` Xu, Quan
@ 2016-05-09 8:24 ` Jan Beulich
2016-05-09 8:29 ` Xu, Quan
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-05-09 8:24 UTC (permalink / raw)
To: Quan Xu; +Cc: xen-devel, Wei Liu
>>> On 09.05.16 at 09:55, <quan.xu@intel.com> wrote:
> On May 06, 2016 10:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
>> hardware. While, with our current code, that's a correct prerequisite
>> assumption for IOMMU presence, this is wrong on systems without IOMMU.
>> Hence iommu_enabled (and alike) checks should be done prior to calling that
>> function, not after.
>>
>> Also move iommu_suspend() next to iommu_resume() - it escapes me why
>> iommu_do_domctl() had got put between the two.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -337,11 +337,16 @@ int __init iommu_setup(void)
>> return rc;
>> }
>>
>> +void iommu_suspend()
>> +{
>> + if ( iommu_enabled )
>> + iommu_get_ops()->suspend();
>> +}
>> +
>
>
> What about this style:
>
> +void iommu_suspend()
> +{
> + if ( iommu_enabled &&
> + iommu_get_ops()->suspend )
> + iommu_get_ops()->suspend();
> +}
> +
Where needed - sure. But I don't see the point in adding NULL
checks when the hook is required to be there.
>> void iommu_crash_shutdown(void)
>> {
>> - const struct iommu_ops *ops = iommu_get_ops();
>> if ( iommu_enabled )
>> - ops->crash_shutdown();
>> + iommu_get_ops()->crash_shutdown();
>> iommu_enabled = iommu_intremap = iommu_intpost = 0;
>
> btw, is this line still a code style issue?
Which one - the changed one or the context one? In the latter
case, even if there were a coding style issue (which I don't see)
correcting it wouldn't belong here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] IOMMU: don't BUG() on exotic hardware
2016-05-09 8:24 ` Jan Beulich
@ 2016-05-09 8:29 ` Xu, Quan
0 siblings, 0 replies; 6+ messages in thread
From: Xu, Quan @ 2016-05-09 8:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Wei Liu
On May 09, 2016 4:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>> On 09.05.16 at 09:55, <quan.xu@intel.com> wrote:
> > On May 06, 2016 10:24 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >> On x86, iommu_get_ops() BUG()s when running on non-Intel, non-AMD
> >> hardware. While, with our current code, that's a correct prerequisite
> >> assumption for IOMMU presence, this is wrong on systems without
> IOMMU.
> >> Hence iommu_enabled (and alike) checks should be done prior to
> >> calling that function, not after.
> >>
> >> Also move iommu_suspend() next to iommu_resume() - it escapes me why
> >> iommu_do_domctl() had got put between the two.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> void iommu_crash_shutdown(void)
> >> {
> >> - const struct iommu_ops *ops = iommu_get_ops();
> >> if ( iommu_enabled )
> >> - ops->crash_shutdown();
> >> + iommu_get_ops()->crash_shutdown();
> >> iommu_enabled = iommu_intremap = iommu_intpost = 0;
> >
> > btw, is this line still a code style issue?
>
> Which one - the changed one or the context one? In the latter case, even if
> there were a coding style issue (which I don't see) correcting it wouldn't
> belong here.
>
The context one -- "iommu_enabled = iommu_intremap = iommu_intpost = 0;"
Quan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-09 8:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-06 14:24 [PATCH] IOMMU: don't BUG() on exotic hardware Jan Beulich
2016-05-06 14:31 ` Wei Liu
2016-05-06 15:28 ` Dario Faggioli
2016-05-09 7:55 ` Xu, Quan
2016-05-09 8:24 ` Jan Beulich
2016-05-09 8:29 ` Xu, Quan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.