Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
Hi Joerg, [auto build test ERROR on s390/features] [also build test ERROR on v4.11-rc8 next-20170428] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Joerg-Roedel/iommu-s390-Fix-iommu-groups-and-add-sysfs-support/20170429-003440 base: https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features config: s390-default_defconfig (attached as .config) compiler: s390x-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=s390 All errors (new ones prefixed by >>): In file included from include/trace/define_trace.h:95:0, from include/trace/events/iommu.h:167, from include/linux/iommu.h:27, from arch/s390/include/asm/pci.h:11, from include/linux/pci.h:1627, from include/trace/events/iommu.h:14, from drivers/iommu/iommu-traces.c:12: include/trace/events/iommu.h: In function 'ftrace_test_probe_add_device_to_group': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_add_device_to_group' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/events/iommu.h:39:1: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(iommu_group_event, add_device_to_group, ^~~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_remove_device_from_group': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_remove_device_from_group' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/events/iommu.h:47:1: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(iommu_group_event, remove_device_from_group, ^~~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_attach_device_to_domain': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_attach_device_to_domain' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/events/iommu.h:72:1: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(iommu_device_event, attach_device_to_domain, ^~~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_detach_device_from_domain': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_detach_device_from_domain' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/events/iommu.h:79:1: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(iommu_device_event, detach_device_from_domain, ^~~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_map': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_map' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/trace_events.h:66:2: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args)); ^~~~ include/trace/events/iommu.h:86:1: note: in expansion of macro 'TRACE_EVENT' TRACE_EVENT(map, ^~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_unmap': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_unmap' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/trace_events.h:66:2: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args)); ^~~~ include/trace/events/iommu.h:109:1: note: in expansion of macro 'TRACE_EVENT' TRACE_EVENT(unmap, ^~~ include/trace/events/iommu.h: In function 'ftrace_test_probe_io_page_fault': include/trace/trace_events.h:716:2: error: implicit declaration of function 'check_trace_callback_type_io_page_fault' [-Werror=implicit-function-declaration] check_trace_callback_type_##call(trace_event_raw_event_##template); \ ^ include/trace/events/iommu.h:158:1: note: in expansion of macro 'DEFINE_EVENT' DEFINE_EVENT(iommu_error, io_page_fault, ^~~~ In file included from arch/s390/include/asm/pci.h:11:0, from include/linux/pci.h:1627, from
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Fri, Apr 28, 2017 at 03:20:17PM +0200, Gerald Schaefer wrote: > On Thu, 27 Apr 2017 23:12:32 +0200 > Joerg Roedelwrote: > > This is the way to free an iommu-group. It was missing before probably > > because it was unclear whether the add_device function allocated a group > > or not. So there was no way to know if it needs to be put again in the > > remove_device function. > > Hmm, for the reference count it should not matter whether a new group was > allocated or an existing group found with iommu_group_get(). Our add_device > callback always gets one reference either from iommu_group_get or _alloc, > and then another one from iommu_group_add_device(), after which the first > reference is put again. So there should always be one reference more after > a successful add_device. Right, my statement above is wrong. The current code is fine, it gets a reference to the group with iommu_group_get/iommu_group_alloc, attaches the device to the group (which takes a reference to the group of its own), and in the end it drops its local reference. When the device->group link is broken up in the remove_device function, that reference is also dropped. So everything is fine. The additional iommu_group_put() in my patch is wrong. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Thu, 27 Apr 2017 23:12:32 +0200 Joerg Roedelwrote: > On Thu, Apr 27, 2017 at 08:11:42PM +0200, Gerald Schaefer wrote: > > > +void zpci_destroy_iommu(struct zpci_dev *zdev) > > > +{ > > > + iommu_group_put(zdev->group); > > > + zdev->group = NULL; > > > +} > > > > While the rest of this patch doesn't seem to make much of a difference to > > the current behavior, I'm wondering where this extra iommu_group_put() > > comes from. It either was erroneously missing before this patch, or it > > is erroneously introduced by this patch. > > This is the way to free an iommu-group. It was missing before probably > because it was unclear whether the add_device function allocated a group > or not. So there was no way to know if it needs to be put again in the > remove_device function. Hmm, for the reference count it should not matter whether a new group was allocated or an existing group found with iommu_group_get(). Our add_device callback always gets one reference either from iommu_group_get or _alloc, and then another one from iommu_group_add_device(), after which the first reference is put again. So there should always be one reference more after a successful add_device. Now I'm wondering where this one reference is put again, and I thought that happened in the remove_device callback, where we call iommu_group_remove_device(). Is this not correct? Just want to make sure that we don't have a refcount issue in the current code. Regards, Gerald ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Thu, Apr 27, 2017 at 08:11:42PM +0200, Gerald Schaefer wrote: > > +void zpci_destroy_iommu(struct zpci_dev *zdev) > > +{ > > + iommu_group_put(zdev->group); > > + zdev->group = NULL; > > +} > > While the rest of this patch doesn't seem to make much of a difference to > the current behavior, I'm wondering where this extra iommu_group_put() > comes from. It either was erroneously missing before this patch, or it > is erroneously introduced by this patch. This is the way to free an iommu-group. It was missing before probably because it was unclear whether the add_device function allocated a group or not. So there was no way to know if it needs to be put again in the remove_device function. With this patch the iommu-group is explicitly allocated when the zpci_dev is created and destroyed again with it. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/2] iommu/s390: Fix IOMMU groups
On Thu, 27 Apr 2017 17:28:24 +0200 Joerg Roedelwrote: > From: Joerg Roedel > > Currently the s390 iommu driver allocates an iommu-group for > every device that is added. But that is wrong, as there is > only one dma-table per pci-root-bus. Make all devices behind > one dma-table share one iommu-group. On s390, each PCI device has its own zpci_dev structure, and also its own DMA address space. Even with this patch, you'll still allocate an iommu_group for every device that is added, see below, so I do not really see the benefit of this (but my knowledge got a little rusty, so I may be missing something). The only reason why we allow the theoretical option to attach more than one device to the same IOMMU domain (and thus address space), is because it was a requirement by you at that time (IIRC). We have no use-case for this, and even in this theoretical scenario you would still have separate zpci_dev structures for the affected devices that share the same IOMMU domain (DMA address space), and thus also separate IOMMU groups, at least after this patch. Before this patch, you could in theory have different PCI devices in the same IOMMU group, by having iommu_group_get() in s390_iommu_add_device() return a group != NULL. With this patch, you will enforce that a new iommu_group is allocated for every device, which would be the contrary of what the description says. > > Signed-off-by: Joerg Roedel > --- > arch/s390/include/asm/pci.h | 7 +++ > arch/s390/pci/pci.c | 10 +- > drivers/iommu/s390-iommu.c | 43 --- > 3 files changed, 48 insertions(+), 12 deletions(-) > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h > index 4e31866..045665d 100644 > --- a/arch/s390/include/asm/pci.h > +++ b/arch/s390/include/asm/pci.h > @@ -8,6 +8,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -123,6 +124,8 @@ struct zpci_dev { > unsigned long iommu_pages; > unsigned intnext_bit; > > + struct iommu_group *group; /* IOMMU group for all devices behind > this zdev */ There is always only one device behind a zpci_dev, so this comment makes no sense. > + > char res_name[16]; > struct zpci_bar_struct bars[PCI_BAR_COUNT]; > > @@ -173,6 +176,10 @@ static inline bool zdev_enabled(struct zpci_dev *zdev) > int clp_enable_fh(struct zpci_dev *, u8); > int clp_disable_fh(struct zpci_dev *); > > +/* IOMMU Interface */ > +int zpci_init_iommu(struct zpci_dev *zdev); > +void zpci_destroy_iommu(struct zpci_dev *zdev); > + > #ifdef CONFIG_PCI > /* Error handling and recovery */ > void zpci_event_error(void *); > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c > index 364b9d8..3178e4d 100644 > --- a/arch/s390/pci/pci.c > +++ b/arch/s390/pci/pci.c > @@ -754,6 +754,7 @@ void pcibios_remove_bus(struct pci_bus *bus) > > zpci_exit_slot(zdev); > zpci_cleanup_bus_resources(zdev); > + zpci_destroy_iommu(zdev); > zpci_free_domain(zdev); > > spin_lock(_list_lock); > @@ -825,11 +826,15 @@ int zpci_create_device(struct zpci_dev *zdev) > if (rc) > goto out; > > + rc = zpci_init_iommu(zdev); > + if (rc) > + goto out_free; This will get called for every device that is added, and every device will get its own zpci_dev, so this will still result in allocating an IOMMU group for every device. > + > mutex_init(>lock); > if (zdev->state == ZPCI_FN_STATE_CONFIGURED) { > rc = zpci_enable_device(zdev); > if (rc) > - goto out_free; > + goto out_iommu; > } > rc = zpci_scan_bus(zdev); > if (rc) > @@ -846,6 +851,9 @@ int zpci_create_device(struct zpci_dev *zdev) > out_disable: > if (zdev->state == ZPCI_FN_STATE_ONLINE) > zpci_disable_device(zdev); > +out_iommu: > + zpci_destroy_iommu(zdev); > + > out_free: > zpci_free_domain(zdev); > out: > diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c > index 179e636..cad3ad0 100644 > --- a/drivers/iommu/s390-iommu.c > +++ b/drivers/iommu/s390-iommu.c > @@ -163,22 +163,22 @@ static void s390_iommu_detach_device(struct > iommu_domain *domain, > } > } > > -static int s390_iommu_add_device(struct device *dev) > +static struct iommu_group *s390_iommu_device_group(struct device *dev) > { > - struct iommu_group *group; > - int rc; > + struct zpci_dev *zdev = to_pci_dev(dev)->sysdata; > > - group = iommu_group_get(dev); > - if (!group) { > - group = iommu_group_alloc(); > - if (IS_ERR(group)) > - return PTR_ERR(group); > - } > + return zdev->group; > +} > + > +static int s390_iommu_add_device(struct device *dev) > +{ > + struct iommu_group *group = iommu_group_get_for_dev(dev); > + if
[PATCH 1/2] iommu/s390: Fix IOMMU groups
From: Joerg RoedelCurrently the s390 iommu driver allocates an iommu-group for every device that is added. But that is wrong, as there is only one dma-table per pci-root-bus. Make all devices behind one dma-table share one iommu-group. Signed-off-by: Joerg Roedel --- arch/s390/include/asm/pci.h | 7 +++ arch/s390/pci/pci.c | 10 +- drivers/iommu/s390-iommu.c | 43 --- 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 4e31866..045665d 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -123,6 +124,8 @@ struct zpci_dev { unsigned long iommu_pages; unsigned intnext_bit; + struct iommu_group *group; /* IOMMU group for all devices behind this zdev */ + char res_name[16]; struct zpci_bar_struct bars[PCI_BAR_COUNT]; @@ -173,6 +176,10 @@ static inline bool zdev_enabled(struct zpci_dev *zdev) int clp_enable_fh(struct zpci_dev *, u8); int clp_disable_fh(struct zpci_dev *); +/* IOMMU Interface */ +int zpci_init_iommu(struct zpci_dev *zdev); +void zpci_destroy_iommu(struct zpci_dev *zdev); + #ifdef CONFIG_PCI /* Error handling and recovery */ void zpci_event_error(void *); diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c index 364b9d8..3178e4d 100644 --- a/arch/s390/pci/pci.c +++ b/arch/s390/pci/pci.c @@ -754,6 +754,7 @@ void pcibios_remove_bus(struct pci_bus *bus) zpci_exit_slot(zdev); zpci_cleanup_bus_resources(zdev); + zpci_destroy_iommu(zdev); zpci_free_domain(zdev); spin_lock(_list_lock); @@ -825,11 +826,15 @@ int zpci_create_device(struct zpci_dev *zdev) if (rc) goto out; + rc = zpci_init_iommu(zdev); + if (rc) + goto out_free; + mutex_init(>lock); if (zdev->state == ZPCI_FN_STATE_CONFIGURED) { rc = zpci_enable_device(zdev); if (rc) - goto out_free; + goto out_iommu; } rc = zpci_scan_bus(zdev); if (rc) @@ -846,6 +851,9 @@ int zpci_create_device(struct zpci_dev *zdev) out_disable: if (zdev->state == ZPCI_FN_STATE_ONLINE) zpci_disable_device(zdev); +out_iommu: + zpci_destroy_iommu(zdev); + out_free: zpci_free_domain(zdev); out: diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c index 179e636..cad3ad0 100644 --- a/drivers/iommu/s390-iommu.c +++ b/drivers/iommu/s390-iommu.c @@ -163,22 +163,22 @@ static void s390_iommu_detach_device(struct iommu_domain *domain, } } -static int s390_iommu_add_device(struct device *dev) +static struct iommu_group *s390_iommu_device_group(struct device *dev) { - struct iommu_group *group; - int rc; + struct zpci_dev *zdev = to_pci_dev(dev)->sysdata; - group = iommu_group_get(dev); - if (!group) { - group = iommu_group_alloc(); - if (IS_ERR(group)) - return PTR_ERR(group); - } + return zdev->group; +} + +static int s390_iommu_add_device(struct device *dev) +{ + struct iommu_group *group = iommu_group_get_for_dev(dev); + if (IS_ERR(group)) + return PTR_ERR(group); - rc = iommu_group_add_device(group, dev); iommu_group_put(group); - return rc; + return 0; } static void s390_iommu_remove_device(struct device *dev) @@ -333,6 +333,26 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain, return size; } +int zpci_init_iommu(struct zpci_dev *zdev) +{ + int rc = 0; + + zdev->group = iommu_group_alloc(); + + if (IS_ERR(zdev->group)) { + rc = PTR_ERR(zdev->group); + zdev->group = NULL; + } + + return rc; +} + +void zpci_destroy_iommu(struct zpci_dev *zdev) +{ + iommu_group_put(zdev->group); + zdev->group = NULL; +} + static struct iommu_ops s390_iommu_ops = { .capable = s390_iommu_capable, .domain_alloc = s390_domain_alloc, @@ -344,6 +364,7 @@ static size_t s390_iommu_unmap(struct iommu_domain *domain, .iova_to_phys = s390_iommu_iova_to_phys, .add_device = s390_iommu_add_device, .remove_device = s390_iommu_remove_device, + .device_group = s390_iommu_device_group, .pgsize_bitmap = S390_IOMMU_PGSIZES, }; -- 1.9.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu