| Message ID | 572CC53C02000078000E9182@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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
>>> 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
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
--- 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)