* [RFC 0/1] PCI: s390 global attribute "UID Checking" @ 2021-01-11 9:38 Niklas Schnelle 2021-01-11 9:38 ` [RFC 1/1] s390/pci: expose UID checking state in sysfs Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-11 9:38 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski Hi Bjorn, Hi Kernel Hackers, With the below patch I'm proposing to expose a global (i.e. not device bound) runtime attribute of the s390 PCI implementation (zPCI) called "UID Checking". You can find some background information on what this attribute means and why it is important at the end of this mail. The reason I'm writing to you about it however is that this is the first global PCI attribute we would like to expose to user space and I'm searching for a good place to put it. The proposed patch uses "/sys/bus/pci/zpci/uid_checking" which from our perspective would be a great choice but I realize that there currently are no platform specific attributes directly under "/sys/bus/pci" so this clearly requires some discussion. What's your thought on this and do you know of any other platform specific global PCI attributes as I couldn't find any? Best regards, Niklas Schnelle Background: On s390 OSs always run under at least a machine level hypervisor (LPAR). Simpliefied by usually running from SAN this makes VM migration possible at every level. For PCI this has created the need to allow PCI IDs to be stable across machines and to be partly user defined such that the setup of an existing VM on one machine can be recreated on another machine. In particular the Domain part of the PCI ID can be user defined by setting a per device value called UID. Since this was a late addition and isn't used by all OSs and since UIDs need to be unique if used as Domains, there is an additional global platform supplied runtime value called "UID Checking". This value indicates if UIDs are guaranteed to be unique and set which triggers Linux to use the UIDs as PCI Domains otherwise PCI Domains are simply incremented as necessary. This "UID Checking" setting is thus very important e.g. when deciding how network interface names are generated as it indicates if the domain part of the PCI ID will remain stable across reboots and migrations. Once exposed we will propose a patch to udev/systemd to use the "UID Checking" attribute to prefer network interface names which can be guaranteed to be stable and re-creatable for migration. Niklas Schnelle (1): s390/pci: expose UID checking state in sysfs Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++++ arch/s390/include/asm/pci.h | 3 +++ arch/s390/pci/pci.c | 4 +++ arch/s390/pci/pci_sysfs.c | 34 +++++++++++++++++++++++++ 4 files changed, 52 insertions(+) -- 2.17.1 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-11 9:38 [RFC 0/1] PCI: s390 global attribute "UID Checking" Niklas Schnelle @ 2021-01-11 9:38 ` Niklas Schnelle 2021-01-12 21:50 ` Bjorn Helgaas 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-11 9:38 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski We use the UID of a zPCI adapter, or the UID of the function zero if there are multiple functions in an adapter, as PCI domain if and only if UID Checking is turned on. Otherwise we automatically generate domains as devices appear. The state of UID Checking is thus essential to know if the PCI domain will be stable, yet currently there is no way to access this information from userspace. So let's solve this by showing the state of UID checking as a sysfs attribute in /sys/bus/pci/uid_checking Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++++ arch/s390/include/asm/pci.h | 3 +++ arch/s390/pci/pci.c | 4 +++ arch/s390/pci/pci_sysfs.c | 34 +++++++++++++++++++++++++ 4 files changed, 52 insertions(+) diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci index 25c9c39770c6..a174aac0ebb0 100644 --- a/Documentation/ABI/testing/sysfs-bus-pci +++ b/Documentation/ABI/testing/sysfs-bus-pci @@ -375,3 +375,14 @@ Description: The value comes from the PCI kernel device state and can be one of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold". The file is read only. +What: /sys/bus/pci/zpci/uid_checking +Date: December 2020 +Contact: Niklas Schnelle <schnelle@linux.ibm.com> +Description: + This attribute exposes the global state of UID Checking on + an s390 Linux system. If UID Checking is on this file + contains '1' otherwise '0'. If UID Checking is on the UID of + a zPCI device, or the UID of function zero for a multi-function + device will be used as its PCI Domain number. If UID Checking + is off PCI Domain numbers are generated automatically and + are not stable across reboots. diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h index 212628932ddc..3cfa6cc701ba 100644 --- a/arch/s390/include/asm/pci.h +++ b/arch/s390/include/asm/pci.h @@ -285,6 +285,9 @@ void zpci_debug_exit_device(struct zpci_dev *); /* Error reporting */ int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *); +/* Sysfs Entries */ +int zpci_sysfs_init(void); + #ifdef CONFIG_NUMA /* Returns the node based on PCI bus */ diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c index 41df8fcfddde..c16c93e5f9af 100644 --- a/arch/s390/pci/pci.c +++ b/arch/s390/pci/pci.c @@ -881,6 +881,10 @@ static int __init pci_base_init(void) if (rc) goto out_find; + rc = zpci_sysfs_init(); + if (rc) + goto out_find; + s390_pci_initialized = 1; return 0; diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c index 5c028bee91b9..d00690f73539 100644 --- a/arch/s390/pci/pci_sysfs.c +++ b/arch/s390/pci/pci_sysfs.c @@ -172,3 +172,37 @@ const struct attribute_group *zpci_attr_groups[] = { &pfip_attr_group, NULL, }; + +/* Global zPCI attributes */ +static ssize_t uid_checking_show(struct kobject *kobj, + struct kobj_attribute *attr, char *buf) +{ + return sprintf(buf, "%i\n", zpci_unique_uid); +} + +static struct kobj_attribute sys_zpci_uid_checking_attr = + __ATTR(uid_checking, 0444, uid_checking_show, NULL); + +static struct kset *zpci_global_kset; + +static struct attribute *zpci_attrs_global[] = { + &sys_zpci_uid_checking_attr.attr, + NULL, +}; + +static struct attribute_group zpci_attr_group_global = { + .attrs = zpci_attrs_global, +}; + +int __init zpci_sysfs_init(void) +{ + struct kset *pci_bus_kset; + + pci_bus_kset = bus_get_kset(&pci_bus_type); + + zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); + if (!zpci_global_kset) + return -ENOMEM; + + return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); +} -- 2.17.1 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-11 9:38 ` [RFC 1/1] s390/pci: expose UID checking state in sysfs Niklas Schnelle @ 2021-01-12 21:50 ` Bjorn Helgaas 2021-01-13 7:47 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Bjorn Helgaas @ 2021-01-12 21:50 UTC (permalink / raw) To: Niklas Schnelle Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > We use the UID of a zPCI adapter, or the UID of the function zero if > there are multiple functions in an adapter, as PCI domain if and only if > UID Checking is turned on. > Otherwise we automatically generate domains as devices appear. > > The state of UID Checking is thus essential to know if the PCI domain > will be stable, yet currently there is no way to access this information > from userspace. > So let's solve this by showing the state of UID checking as a sysfs > attribute in /sys/bus/pci/uid_checking Cosmetic: can't tell if the above is two paragraphs separated by blank lines or four separated by either blank lines or short last lines. Please separate or reflow to avoid the ambiguity. I don't have any input on the s390 issues, and I assume this will go via the s390 tree. > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > --- > Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++++ > arch/s390/include/asm/pci.h | 3 +++ > arch/s390/pci/pci.c | 4 +++ > arch/s390/pci/pci_sysfs.c | 34 +++++++++++++++++++++++++ > 4 files changed, 52 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > index 25c9c39770c6..a174aac0ebb0 100644 > --- a/Documentation/ABI/testing/sysfs-bus-pci > +++ b/Documentation/ABI/testing/sysfs-bus-pci > @@ -375,3 +375,14 @@ Description: > The value comes from the PCI kernel device state and can be one > of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold". > The file is read only. > +What: /sys/bus/pci/zpci/uid_checking > +Date: December 2020 > +Contact: Niklas Schnelle <schnelle@linux.ibm.com> > +Description: > + This attribute exposes the global state of UID Checking on > + an s390 Linux system. If UID Checking is on this file > + contains '1' otherwise '0'. If UID Checking is on the UID of > + a zPCI device, or the UID of function zero for a multi-function > + device will be used as its PCI Domain number. If UID Checking > + is off PCI Domain numbers are generated automatically and > + are not stable across reboots. > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h > index 212628932ddc..3cfa6cc701ba 100644 > --- a/arch/s390/include/asm/pci.h > +++ b/arch/s390/include/asm/pci.h > @@ -285,6 +285,9 @@ void zpci_debug_exit_device(struct zpci_dev *); > /* Error reporting */ > int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *); > > +/* Sysfs Entries */ > +int zpci_sysfs_init(void); > + > #ifdef CONFIG_NUMA > > /* Returns the node based on PCI bus */ > diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c > index 41df8fcfddde..c16c93e5f9af 100644 > --- a/arch/s390/pci/pci.c > +++ b/arch/s390/pci/pci.c > @@ -881,6 +881,10 @@ static int __init pci_base_init(void) > if (rc) > goto out_find; > > + rc = zpci_sysfs_init(); > + if (rc) > + goto out_find; > + > s390_pci_initialized = 1; > return 0; > > diff --git a/arch/s390/pci/pci_sysfs.c b/arch/s390/pci/pci_sysfs.c > index 5c028bee91b9..d00690f73539 100644 > --- a/arch/s390/pci/pci_sysfs.c > +++ b/arch/s390/pci/pci_sysfs.c > @@ -172,3 +172,37 @@ const struct attribute_group *zpci_attr_groups[] = { > &pfip_attr_group, > NULL, > }; > + > +/* Global zPCI attributes */ > +static ssize_t uid_checking_show(struct kobject *kobj, > + struct kobj_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%i\n", zpci_unique_uid); > +} > + > +static struct kobj_attribute sys_zpci_uid_checking_attr = > + __ATTR(uid_checking, 0444, uid_checking_show, NULL); Use DEVICE_ATTR_RO instead of __ATTR. > +static struct kset *zpci_global_kset; > + > +static struct attribute *zpci_attrs_global[] = { > + &sys_zpci_uid_checking_attr.attr, > + NULL, > +}; > + > +static struct attribute_group zpci_attr_group_global = { > + .attrs = zpci_attrs_global, > +}; > + > +int __init zpci_sysfs_init(void) > +{ > + struct kset *pci_bus_kset; > + > + pci_bus_kset = bus_get_kset(&pci_bus_type); > + > + zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); > + if (!zpci_global_kset) > + return -ENOMEM; > + > + return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > +} > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-12 21:50 ` Bjorn Helgaas @ 2021-01-13 7:47 ` Niklas Schnelle 2021-01-13 18:55 ` Bjorn Helgaas 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-13 7:47 UTC (permalink / raw) To: Bjorn Helgaas Cc: linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: >> We use the UID of a zPCI adapter, or the UID of the function zero if >> there are multiple functions in an adapter, as PCI domain if and only if >> UID Checking is turned on. >> Otherwise we automatically generate domains as devices appear. >> >> The state of UID Checking is thus essential to know if the PCI domain >> will be stable, yet currently there is no way to access this information >> from userspace. >> So let's solve this by showing the state of UID checking as a sysfs >> attribute in /sys/bus/pci/uid_checking > > Cosmetic: can't tell if the above is two paragraphs separated by blank > lines or four separated by either blank lines or short last lines. > Please separate or reflow to avoid the ambiguity. Thanks, you're right I split it in 3 proper paragraphs now. Also the commit message was out of sync with the documentation, cover letter and code. This version actually uses /sys/bus/pci/zpci/uid_checking sorry about that. > > I don't have any input on the s390 issues, and I assume this will go > via the s390 tree. > >> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> >> --- >> Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++++ >> arch/s390/include/asm/pci.h | 3 +++ >> arch/s390/pci/pci.c | 4 +++ >> arch/s390/pci/pci_sysfs.c | 34 +++++++++++++++++++++++++ >> 4 files changed, 52 insertions(+) >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci >> index 25c9c39770c6..a174aac0ebb0 100644 >> --- a/Documentation/ABI/testing/sysfs-bus-pci >> +++ b/Documentation/ABI/testing/sysfs-bus-pci >> @@ -375,3 +375,14 @@ Description: >> The value comes from the PCI kernel device state and can be one >> of: "unknown", "error", "D0", D1", "D2", "D3hot", "D3cold". >> The file is read only. >> +What: /sys/bus/pci/zpci/uid_checking >> +Date: December 2020 >> +Contact: Niklas Schnelle <schnelle@linux.ibm.com> >> +Description: >> + This attribute exposes the global state of UID Checking on >> + an s390 Linux system. If UID Checking is on this file >> + contains '1' otherwise '0'. If UID Checking is on the UID of >> + a zPCI device, or the UID of function zero for a multi-function >> + device will be used as its PCI Domain number. If UID Checking >> + is off PCI Domain numbers are generated automatically and >> + are not stable across reboots. >> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h >> index 212628932ddc..3cfa6cc701ba 100644 >> --- a/arch/s390/include/asm/pci.h >> +++ b/arch/s390/include/asm/pci.h >> @@ -285,6 +285,9 @@ void zpci_debug_exit_device(struct zpci_dev *); >> /* Error reporting */ >> int zpci_report_error(struct pci_dev *, struct zpci_report_error_header *); ... snip ... >> + >> +/* Global zPCI attributes */ >> +static ssize_t uid_checking_show(struct kobject *kobj, >> + struct kobj_attribute *attr, char *buf) >> +{ >> + return sprintf(buf, "%i\n", zpci_unique_uid); >> +} >> + >> +static struct kobj_attribute sys_zpci_uid_checking_attr = >> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > > Use DEVICE_ATTR_RO instead of __ATTR. It's my understanding that DEVICE_ATTR_* is only for per device attributes. This one is global for the entire Z PCI. I just tried with BUS_ATTR_RO instead and that works but only if I put the attribute at /sys/bus/pci/uid_checking instead of with a zpci subfolder. This path would work for us too, we currently don't have any other global attributes that we are planning to expose but those could of course come up in the future. > ... snip ... >> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-13 7:47 ` Niklas Schnelle @ 2021-01-13 18:55 ` Bjorn Helgaas 2021-01-14 13:20 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Bjorn Helgaas @ 2021-01-13 18:55 UTC (permalink / raw) To: Niklas Schnelle Cc: linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > > On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > >> We use the UID of a zPCI adapter, or the UID of the function zero if > >> there are multiple functions in an adapter, as PCI domain if and only if > >> UID Checking is turned on. > >> Otherwise we automatically generate domains as devices appear. > >> > >> The state of UID Checking is thus essential to know if the PCI domain > >> will be stable, yet currently there is no way to access this information > >> from userspace. > >> So let's solve this by showing the state of UID checking as a sysfs > >> attribute in /sys/bus/pci/uid_checking > >> +/* Global zPCI attributes */ > >> +static ssize_t uid_checking_show(struct kobject *kobj, > >> + struct kobj_attribute *attr, char *buf) > >> +{ > >> + return sprintf(buf, "%i\n", zpci_unique_uid); > >> +} > >> + > >> +static struct kobj_attribute sys_zpci_uid_checking_attr = > >> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > > > > Use DEVICE_ATTR_RO instead of __ATTR. > > It's my understanding that DEVICE_ATTR_* is only for > per device attributes. This one is global for the entire > Z PCI. I just tried with BUS_ATTR_RO instead > and that works but only if I put the attribute at > /sys/bus/pci/uid_checking instead of with a zpci > subfolder. This path would work for us too, we > currently don't have any other global attributes > that we are planning to expose but those could of > course come up in the future. Ah, I missed the fact that this is a kobj_attribute, not a device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but seems like it might fit? Bjorn ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-13 18:55 ` Bjorn Helgaas @ 2021-01-14 13:20 ` Niklas Schnelle 2021-01-14 13:44 ` Christian Brauner 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-14 13:20 UTC (permalink / raw) To: Bjorn Helgaas, Christian Brauner Cc: linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: >>>> We use the UID of a zPCI adapter, or the UID of the function zero if >>>> there are multiple functions in an adapter, as PCI domain if and only if >>>> UID Checking is turned on. >>>> Otherwise we automatically generate domains as devices appear. >>>> >>>> The state of UID Checking is thus essential to know if the PCI domain >>>> will be stable, yet currently there is no way to access this information >>>> from userspace. >>>> So let's solve this by showing the state of UID checking as a sysfs >>>> attribute in /sys/bus/pci/uid_checking > >>>> +/* Global zPCI attributes */ >>>> +static ssize_t uid_checking_show(struct kobject *kobj, >>>> + struct kobj_attribute *attr, char *buf) >>>> +{ >>>> + return sprintf(buf, "%i\n", zpci_unique_uid); >>>> +} >>>> + >>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = >>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); >>> >>> Use DEVICE_ATTR_RO instead of __ATTR. >> >> It's my understanding that DEVICE_ATTR_* is only for >> per device attributes. This one is global for the entire >> Z PCI. I just tried with BUS_ATTR_RO instead >> and that works but only if I put the attribute at >> /sys/bus/pci/uid_checking instead of with a zpci >> subfolder. This path would work for us too, we >> currently don't have any other global attributes >> that we are planning to expose but those could of >> course come up in the future. > > Ah, I missed the fact that this is a kobj_attribute, not a > device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but > seems like it might fit? > > Bjorn > KERNEL_ATTR_* is currently not exported in any header. After adding it to include/linuc/sysfs.h it indeed works perfectly. Adding Christian Brauner as suggested by get_maintainers for their opinion. I'm of course willing to provide a patch for that move should it be desired. @Bjorn apart from the correct macro do you have a preference for either suggested path /sys/bus/pci/zpci/uid_checking vs /sys/bus/pci/uid_checking? For completeness some further internal discussion lets us tend to rather name it to "unique_uids" but I guess that doesn't make a difference to non s390 people ;-) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 13:20 ` Niklas Schnelle @ 2021-01-14 13:44 ` Christian Brauner 2021-01-14 13:58 ` Greg Kroah-Hartman 0 siblings, 1 reply; 22+ messages in thread From: Christian Brauner @ 2021-01-14 13:44 UTC (permalink / raw) To: Niklas Schnelle, Greg Kroah-Hartman Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > > > On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > > On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > >>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > >>>> We use the UID of a zPCI adapter, or the UID of the function zero if > >>>> there are multiple functions in an adapter, as PCI domain if and only if > >>>> UID Checking is turned on. > >>>> Otherwise we automatically generate domains as devices appear. > >>>> > >>>> The state of UID Checking is thus essential to know if the PCI domain > >>>> will be stable, yet currently there is no way to access this information > >>>> from userspace. > >>>> So let's solve this by showing the state of UID checking as a sysfs > >>>> attribute in /sys/bus/pci/uid_checking > > > >>>> +/* Global zPCI attributes */ > >>>> +static ssize_t uid_checking_show(struct kobject *kobj, > >>>> + struct kobj_attribute *attr, char *buf) > >>>> +{ > >>>> + return sprintf(buf, "%i\n", zpci_unique_uid); > >>>> +} > >>>> + > >>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = > >>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > >>> > >>> Use DEVICE_ATTR_RO instead of __ATTR. > >> > >> It's my understanding that DEVICE_ATTR_* is only for > >> per device attributes. This one is global for the entire > >> Z PCI. I just tried with BUS_ATTR_RO instead > >> and that works but only if I put the attribute at > >> /sys/bus/pci/uid_checking instead of with a zpci > >> subfolder. This path would work for us too, we > >> currently don't have any other global attributes > >> that we are planning to expose but those could of > >> course come up in the future. > > > > Ah, I missed the fact that this is a kobj_attribute, not a > > device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but > > seems like it might fit? > > > > Bjorn > > > > KERNEL_ATTR_* is currently not exported in any header. After > adding it to include/linuc/sysfs.h it indeed works perfectly. > Adding Christian Brauner as suggested by get_maintainers for > their opinion. I'm of course willing to provide a patch Hey Niklas et al. :) I think this will need input from Greg. He should be best versed in sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's supposed to be kernel internal. Now, that might just be a matter of renaming the macro but let's see whether Greg has any better idea or more questions. :) Christian ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 13:44 ` Christian Brauner @ 2021-01-14 13:58 ` Greg Kroah-Hartman 2021-01-14 15:06 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-14 13:58 UTC (permalink / raw) To: Christian Brauner Cc: Niklas Schnelle, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > > > > > > On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > > > On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > > >> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > > >>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > > >>>> We use the UID of a zPCI adapter, or the UID of the function zero if > > >>>> there are multiple functions in an adapter, as PCI domain if and only if > > >>>> UID Checking is turned on. > > >>>> Otherwise we automatically generate domains as devices appear. > > >>>> > > >>>> The state of UID Checking is thus essential to know if the PCI domain > > >>>> will be stable, yet currently there is no way to access this information > > >>>> from userspace. > > >>>> So let's solve this by showing the state of UID checking as a sysfs > > >>>> attribute in /sys/bus/pci/uid_checking > > > > > >>>> +/* Global zPCI attributes */ > > >>>> +static ssize_t uid_checking_show(struct kobject *kobj, > > >>>> + struct kobj_attribute *attr, char *buf) > > >>>> +{ > > >>>> + return sprintf(buf, "%i\n", zpci_unique_uid); > > >>>> +} > > >>>> + > > >>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = > > >>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > > >>> > > >>> Use DEVICE_ATTR_RO instead of __ATTR. > > >> > > >> It's my understanding that DEVICE_ATTR_* is only for > > >> per device attributes. This one is global for the entire > > >> Z PCI. I just tried with BUS_ATTR_RO instead > > >> and that works but only if I put the attribute at > > >> /sys/bus/pci/uid_checking instead of with a zpci > > >> subfolder. This path would work for us too, we > > >> currently don't have any other global attributes > > >> that we are planning to expose but those could of > > >> course come up in the future. > > > > > > Ah, I missed the fact that this is a kobj_attribute, not a > > > device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but > > > seems like it might fit? > > > > > > Bjorn > > > > > > > KERNEL_ATTR_* is currently not exported in any header. After > > adding it to include/linuc/sysfs.h it indeed works perfectly. > > Adding Christian Brauner as suggested by get_maintainers for > > their opinion. I'm of course willing to provide a patch > > Hey Niklas et al. :) > > I think this will need input from Greg. He should be best versed in > sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's > supposed to be kernel internal. Now, that might just be a matter of > renaming the macro but let's see whether Greg has any better idea or > more questions. :) The big question is, why are you needing this? No driver or driver subsystem should EVER be messing with a "raw" kobject like this. Just use the existing DEVICE_* macros instead please. If you are using a raw kobject, please ask me how to do this properly, as that is something that should NEVER show up in the /sys/devices/* tree. Otherwise userspace tools will break. thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 13:58 ` Greg Kroah-Hartman @ 2021-01-14 15:06 ` Niklas Schnelle 2021-01-14 15:17 ` Greg Kroah-Hartman 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-14 15:06 UTC (permalink / raw) To: Greg Kroah-Hartman, Christian Brauner Cc: Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>> >>> >>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >>>>>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: >>>>>>> We use the UID of a zPCI adapter, or the UID of the function zero if >>>>>>> there are multiple functions in an adapter, as PCI domain if and only if >>>>>>> UID Checking is turned on. >>>>>>> Otherwise we automatically generate domains as devices appear. >>>>>>> >>>>>>> The state of UID Checking is thus essential to know if the PCI domain >>>>>>> will be stable, yet currently there is no way to access this information >>>>>>> from userspace. >>>>>>> So let's solve this by showing the state of UID checking as a sysfs >>>>>>> attribute in /sys/bus/pci/uid_checking >>>> >>>>>>> +/* Global zPCI attributes */ >>>>>>> +static ssize_t uid_checking_show(struct kobject *kobj, >>>>>>> + struct kobj_attribute *attr, char *buf) >>>>>>> +{ >>>>>>> + return sprintf(buf, "%i\n", zpci_unique_uid); >>>>>>> +} >>>>>>> + >>>>>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = >>>>>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); >>>>>> >>>>>> Use DEVICE_ATTR_RO instead of __ATTR. >>>>> >>>>> It's my understanding that DEVICE_ATTR_* is only for >>>>> per device attributes. This one is global for the entire >>>>> Z PCI. I just tried with BUS_ATTR_RO instead >>>>> and that works but only if I put the attribute at >>>>> /sys/bus/pci/uid_checking instead of with a zpci >>>>> subfolder. This path would work for us too, we >>>>> currently don't have any other global attributes >>>>> that we are planning to expose but those could of >>>>> course come up in the future. >>>> >>>> Ah, I missed the fact that this is a kobj_attribute, not a >>>> device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but >>>> seems like it might fit? >>>> >>>> Bjorn >>>> >>> >>> KERNEL_ATTR_* is currently not exported in any header. After >>> adding it to include/linuc/sysfs.h it indeed works perfectly. >>> Adding Christian Brauner as suggested by get_maintainers for >>> their opinion. I'm of course willing to provide a patch >> >> Hey Niklas et al. :) >> >> I think this will need input from Greg. He should be best versed in >> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's >> supposed to be kernel internal. Now, that might just be a matter of >> renaming the macro but let's see whether Greg has any better idea or >> more questions. :) > > The big question is, why are you needing this? > > No driver or driver subsystem should EVER be messing with a "raw" > kobject like this. Just use the existing DEVICE_* macros instead > please. > > If you are using a raw kobject, please ask me how to do this properly, > as that is something that should NEVER show up in the /sys/devices/* > tree. Otherwise userspace tools will break. > > thanks, > > greg k-h Hi Greg, this is for an architecture specific but global i.e. not device bound PCI attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work but only if we place the attribute directly under /sys/bus/pci/new_attr. I'm aware that this is quite unusual in fact I couldn't find anything similar. That's why this is an RFC, with a lengthy cover letter explaining our use case, that I sent to Bjorn to figure out where to even place the attribute. So I guess this is indeed me asking you how to do this properly. That said it does not show up under /sys/devices/* only /sys/bus/pci/*. Best regards, Niklas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 15:06 ` Niklas Schnelle @ 2021-01-14 15:17 ` Greg Kroah-Hartman 2021-01-14 15:51 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-14 15:17 UTC (permalink / raw) To: Niklas Schnelle Cc: Christian Brauner, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > > > On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > > On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>> > >>> > >>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > >>>>>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > >>>>>>> We use the UID of a zPCI adapter, or the UID of the function zero if > >>>>>>> there are multiple functions in an adapter, as PCI domain if and only if > >>>>>>> UID Checking is turned on. > >>>>>>> Otherwise we automatically generate domains as devices appear. > >>>>>>> > >>>>>>> The state of UID Checking is thus essential to know if the PCI domain > >>>>>>> will be stable, yet currently there is no way to access this information > >>>>>>> from userspace. > >>>>>>> So let's solve this by showing the state of UID checking as a sysfs > >>>>>>> attribute in /sys/bus/pci/uid_checking > >>>> > >>>>>>> +/* Global zPCI attributes */ > >>>>>>> +static ssize_t uid_checking_show(struct kobject *kobj, > >>>>>>> + struct kobj_attribute *attr, char *buf) > >>>>>>> +{ > >>>>>>> + return sprintf(buf, "%i\n", zpci_unique_uid); > >>>>>>> +} > >>>>>>> + > >>>>>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = > >>>>>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > >>>>>> > >>>>>> Use DEVICE_ATTR_RO instead of __ATTR. > >>>>> > >>>>> It's my understanding that DEVICE_ATTR_* is only for > >>>>> per device attributes. This one is global for the entire > >>>>> Z PCI. I just tried with BUS_ATTR_RO instead > >>>>> and that works but only if I put the attribute at > >>>>> /sys/bus/pci/uid_checking instead of with a zpci > >>>>> subfolder. This path would work for us too, we > >>>>> currently don't have any other global attributes > >>>>> that we are planning to expose but those could of > >>>>> course come up in the future. > >>>> > >>>> Ah, I missed the fact that this is a kobj_attribute, not a > >>>> device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but > >>>> seems like it might fit? > >>>> > >>>> Bjorn > >>>> > >>> > >>> KERNEL_ATTR_* is currently not exported in any header. After > >>> adding it to include/linuc/sysfs.h it indeed works perfectly. > >>> Adding Christian Brauner as suggested by get_maintainers for > >>> their opinion. I'm of course willing to provide a patch > >> > >> Hey Niklas et al. :) > >> > >> I think this will need input from Greg. He should be best versed in > >> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's > >> supposed to be kernel internal. Now, that might just be a matter of > >> renaming the macro but let's see whether Greg has any better idea or > >> more questions. :) > > > > The big question is, why are you needing this? > > > > No driver or driver subsystem should EVER be messing with a "raw" > > kobject like this. Just use the existing DEVICE_* macros instead > > please. > > > > If you are using a raw kobject, please ask me how to do this properly, > > as that is something that should NEVER show up in the /sys/devices/* > > tree. Otherwise userspace tools will break. > > > > thanks, > > > > greg k-h > > Hi Greg, > > this is for an architecture specific but global i.e. not device bound PCI > attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work > but only if we place the attribute directly under /sys/bus/pci/new_attr. Then you are doing something wrong :) Where _exactly_ are you wanting to put this attribute? > I'm aware that this is quite unusual in fact I couldn't find anything > similar. That's why this is an RFC, with a lengthy cover letter > explaining our use case, that I sent to Bjorn to figure out where to > even place the attribute. > > So I guess this is indeed me asking you how to do this properly. > That said it does not show up under /sys/devices/* only /sys/bus/pci/*. Do NOT put random kobjects under a bus subsystem. If you need that, then use BUS_ATTR_* as that is what it is there for. Again, if you are in a driver subsystem, do not use a raw kobject. Either something is already there for you, or what you want to do is not correct :) thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 15:17 ` Greg Kroah-Hartman @ 2021-01-14 15:51 ` Niklas Schnelle 2021-01-14 16:14 ` Greg Kroah-Hartman 0 siblings, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-14 15:51 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Christian Brauner, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >> >> >> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>> >>>>> >>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >>>>>>>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: >>>>>>>>> We use the UID of a zPCI adapter, or the UID of the function zero if >>>>>>>>> there are multiple functions in an adapter, as PCI domain if and only if >>>>>>>>> UID Checking is turned on. >>>>>>>>> Otherwise we automatically generate domains as devices appear. >>>>>>>>> >>>>>>>>> The state of UID Checking is thus essential to know if the PCI domain >>>>>>>>> will be stable, yet currently there is no way to access this information >>>>>>>>> from userspace. >>>>>>>>> So let's solve this by showing the state of UID checking as a sysfs >>>>>>>>> attribute in /sys/bus/pci/uid_checking >>>>>> >>>>>>>>> +/* Global zPCI attributes */ >>>>>>>>> +static ssize_t uid_checking_show(struct kobject *kobj, >>>>>>>>> + struct kobj_attribute *attr, char *buf) >>>>>>>>> +{ >>>>>>>>> + return sprintf(buf, "%i\n", zpci_unique_uid); >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = >>>>>>>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); >>>>>>>> >>>>>>>> Use DEVICE_ATTR_RO instead of __ATTR. >>>>>>> >>>>>>> It's my understanding that DEVICE_ATTR_* is only for >>>>>>> per device attributes. This one is global for the entire >>>>>>> Z PCI. I just tried with BUS_ATTR_RO instead >>>>>>> and that works but only if I put the attribute at >>>>>>> /sys/bus/pci/uid_checking instead of with a zpci >>>>>>> subfolder. This path would work for us too, we >>>>>>> currently don't have any other global attributes >>>>>>> that we are planning to expose but those could of >>>>>>> course come up in the future. >>>>>> >>>>>> Ah, I missed the fact that this is a kobj_attribute, not a >>>>>> device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but >>>>>> seems like it might fit? >>>>>> >>>>>> Bjorn >>>>>> >>>>> >>>>> KERNEL_ATTR_* is currently not exported in any header. After >>>>> adding it to include/linuc/sysfs.h it indeed works perfectly. >>>>> Adding Christian Brauner as suggested by get_maintainers for >>>>> their opinion. I'm of course willing to provide a patch >>>> >>>> Hey Niklas et al. :) >>>> >>>> I think this will need input from Greg. He should be best versed in >>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's >>>> supposed to be kernel internal. Now, that might just be a matter of >>>> renaming the macro but let's see whether Greg has any better idea or >>>> more questions. :) >>> >>> The big question is, why are you needing this? >>> >>> No driver or driver subsystem should EVER be messing with a "raw" >>> kobject like this. Just use the existing DEVICE_* macros instead >>> please. >>> >>> If you are using a raw kobject, please ask me how to do this properly, >>> as that is something that should NEVER show up in the /sys/devices/* >>> tree. Otherwise userspace tools will break. >>> >>> thanks, >>> >>> greg k-h >> >> Hi Greg, >> >> this is for an architecture specific but global i.e. not device bound PCI >> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work >> but only if we place the attribute directly under /sys/bus/pci/new_attr. > > Then you are doing something wrong :) That is very possible. > > Where _exactly_ are you wanting to put this attribute? I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using the below code and the attribute even shows up but reading it gives me two 0 bytes only. The relevant code is only a slight alteration of the original patch as follows: static ssize_t uid_checking_show(struct bus_type *bus, char *buf) { return sprintf(buf, "%i\n", zpci_unique_uid); } static BUS_ATTR_RO(uid_checking); static struct kset *zpci_global_kset; static struct attribute *zpci_attrs_global[] = { &bus_attr_uid_checking.attr, NULL, }; static struct attribute_group zpci_attr_group_global = { .attrs = zpci_attrs_global, }; int __init zpci_sysfs_init(void) { struct kset *pci_bus_kset; pci_bus_kset = bus_get_kset(&pci_bus_type); zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); if (!zpci_global_kset) return -ENOMEM; return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); } > >> I'm aware that this is quite unusual in fact I couldn't find anything >> similar. That's why this is an RFC, with a lengthy cover letter >> explaining our use case, that I sent to Bjorn to figure out where to >> even place the attribute. >> >> So I guess this is indeed me asking you how to do this properly. >> That said it does not show up under /sys/devices/* only /sys/bus/pci/*. > > Do NOT put random kobjects under a bus subsystem. If you need that, > then use BUS_ATTR_* as that is what it is there for. > > Again, if you are in a driver subsystem, do not use a raw kobject. > Either something is already there for you, or what you want to do is not > correct :) Understood and thanks for the clear advice! > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 15:51 ` Niklas Schnelle @ 2021-01-14 16:14 ` Greg Kroah-Hartman 2021-01-15 11:20 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-14 16:14 UTC (permalink / raw) To: Niklas Schnelle Cc: Christian Brauner, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > > > On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > > On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > >> > >> > >> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > >>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>>>> > >>>>> > >>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > >>>>>>>> On Mon, Jan 11, 2021 at 10:38:57AM +0100, Niklas Schnelle wrote: > >>>>>>>>> We use the UID of a zPCI adapter, or the UID of the function zero if > >>>>>>>>> there are multiple functions in an adapter, as PCI domain if and only if > >>>>>>>>> UID Checking is turned on. > >>>>>>>>> Otherwise we automatically generate domains as devices appear. > >>>>>>>>> > >>>>>>>>> The state of UID Checking is thus essential to know if the PCI domain > >>>>>>>>> will be stable, yet currently there is no way to access this information > >>>>>>>>> from userspace. > >>>>>>>>> So let's solve this by showing the state of UID checking as a sysfs > >>>>>>>>> attribute in /sys/bus/pci/uid_checking > >>>>>> > >>>>>>>>> +/* Global zPCI attributes */ > >>>>>>>>> +static ssize_t uid_checking_show(struct kobject *kobj, > >>>>>>>>> + struct kobj_attribute *attr, char *buf) > >>>>>>>>> +{ > >>>>>>>>> + return sprintf(buf, "%i\n", zpci_unique_uid); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> +static struct kobj_attribute sys_zpci_uid_checking_attr = > >>>>>>>>> + __ATTR(uid_checking, 0444, uid_checking_show, NULL); > >>>>>>>> > >>>>>>>> Use DEVICE_ATTR_RO instead of __ATTR. > >>>>>>> > >>>>>>> It's my understanding that DEVICE_ATTR_* is only for > >>>>>>> per device attributes. This one is global for the entire > >>>>>>> Z PCI. I just tried with BUS_ATTR_RO instead > >>>>>>> and that works but only if I put the attribute at > >>>>>>> /sys/bus/pci/uid_checking instead of with a zpci > >>>>>>> subfolder. This path would work for us too, we > >>>>>>> currently don't have any other global attributes > >>>>>>> that we are planning to expose but those could of > >>>>>>> course come up in the future. > >>>>>> > >>>>>> Ah, I missed the fact that this is a kobj_attribute, not a > >>>>>> device_attribute. Maybe KERNEL_ATTR_RO()? Very few uses so far, but > >>>>>> seems like it might fit? > >>>>>> > >>>>>> Bjorn > >>>>>> > >>>>> > >>>>> KERNEL_ATTR_* is currently not exported in any header. After > >>>>> adding it to include/linuc/sysfs.h it indeed works perfectly. > >>>>> Adding Christian Brauner as suggested by get_maintainers for > >>>>> their opinion. I'm of course willing to provide a patch > >>>> > >>>> Hey Niklas et al. :) > >>>> > >>>> I think this will need input from Greg. He should be best versed in > >>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's > >>>> supposed to be kernel internal. Now, that might just be a matter of > >>>> renaming the macro but let's see whether Greg has any better idea or > >>>> more questions. :) > >>> > >>> The big question is, why are you needing this? > >>> > >>> No driver or driver subsystem should EVER be messing with a "raw" > >>> kobject like this. Just use the existing DEVICE_* macros instead > >>> please. > >>> > >>> If you are using a raw kobject, please ask me how to do this properly, > >>> as that is something that should NEVER show up in the /sys/devices/* > >>> tree. Otherwise userspace tools will break. > >>> > >>> thanks, > >>> > >>> greg k-h > >> > >> Hi Greg, > >> > >> this is for an architecture specific but global i.e. not device bound PCI > >> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work > >> but only if we place the attribute directly under /sys/bus/pci/new_attr. > > > > Then you are doing something wrong :) > > That is very possible. > > > > > Where _exactly_ are you wanting to put this attribute? > > I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using > the below code and the attribute even shows up but reading > it gives me two 0 bytes only. > The relevant code is only a slight alteration of the original patch > as follows: > > static ssize_t uid_checking_show(struct bus_type *bus, char *buf) > { > return sprintf(buf, "%i\n", zpci_unique_uid); > } > static BUS_ATTR_RO(uid_checking); > > static struct kset *zpci_global_kset; > > static struct attribute *zpci_attrs_global[] = { > &bus_attr_uid_checking.attr, > NULL, > }; > > static struct attribute_group zpci_attr_group_global = { > .attrs = zpci_attrs_global, > }; Name your attribute group, and then you do not have to mess with a "raw" kobject like you are below: > > int __init zpci_sysfs_init(void) > { > struct kset *pci_bus_kset; > > pci_bus_kset = bus_get_kset(&pci_bus_type); > > zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); No, do not mess with at kset, just set the default attribute group for the bus to the above, and you should be fine. > if (!zpci_global_kset) > return -ENOMEM; > > return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, that's usually a huge clue that you are doing something wrong. Try the above again, with a simple attribute group, and name for it, and it should "just work". thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-14 16:14 ` Greg Kroah-Hartman @ 2021-01-15 11:20 ` Niklas Schnelle 2021-01-15 12:02 ` Greg Kroah-Hartman 2021-01-15 15:29 ` Bjorn Helgaas 0 siblings, 2 replies; 22+ messages in thread From: Niklas Schnelle @ 2021-01-15 11:20 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Christian Brauner, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: >> >> >> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: >>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >>>> >>>> >>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>>>> >>>>>>> >>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: ... snip ... >>>>>> >>>>>> Hey Niklas et al. :) >>>>>> >>>>>> I think this will need input from Greg. He should be best versed in >>>>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's >>>>>> supposed to be kernel internal. Now, that might just be a matter of >>>>>> renaming the macro but let's see whether Greg has any better idea or >>>>>> more questions. :) >>>>> >>>>> The big question is, why are you needing this? >>>>> >>>>> No driver or driver subsystem should EVER be messing with a "raw" >>>>> kobject like this. Just use the existing DEVICE_* macros instead >>>>> please. >>>>> >>>>> If you are using a raw kobject, please ask me how to do this properly, >>>>> as that is something that should NEVER show up in the /sys/devices/* >>>>> tree. Otherwise userspace tools will break. >>>>> >>>>> thanks, >>>>> >>>>> greg k-h >>>> >>>> Hi Greg, >>>> >>>> this is for an architecture specific but global i.e. not device bound PCI >>>> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work >>>> but only if we place the attribute directly under /sys/bus/pci/new_attr. >>> >>> Then you are doing something wrong :) >> >> That is very possible. >> >>> >>> Where _exactly_ are you wanting to put this attribute? >> >> I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using >> the below code and the attribute even shows up but reading >> it gives me two 0 bytes only. >> The relevant code is only a slight alteration of the original patch >> as follows: >> >> static ssize_t uid_checking_show(struct bus_type *bus, char *buf) >> { >> return sprintf(buf, "%i\n", zpci_unique_uid); >> } >> static BUS_ATTR_RO(uid_checking); >> >> static struct kset *zpci_global_kset; >> >> static struct attribute *zpci_attrs_global[] = { >> &bus_attr_uid_checking.attr, >> NULL, >> }; >> >> static struct attribute_group zpci_attr_group_global = { >> .attrs = zpci_attrs_global, >> }; > > Name your attribute group, and then you do not have to mess with a > "raw" kobject like you are below: Thanks for this tip and sorry for bothering you again. > >> >> int __init zpci_sysfs_init(void) >> { >> struct kset *pci_bus_kset; >> >> pci_bus_kset = bus_get_kset(&pci_bus_type); >> >> zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); > > No, do not mess with at kset, just set the default attribute group for > the bus to the above, and you should be fine. Oh ok, I got this idea from the code adding /sys/bus/pci/slots/ in drivers/pci/slot.c:pci_slot_init(). See below maybe we can clean that up too. > >> if (!zpci_global_kset) >> return -ENOMEM; >> >> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > > Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > that's usually a huge clue that you are doing something wrong. > > Try the above again, with a simple attribute group, and name for it, and > it should "just work". I'm probably missing something but I don't get how this could work in this case. If I'm seeing this right the default attribute group here is pci_bus_type.bus_groups and that is already set in drivers/pci/pci-driver.c so I don't think I should set that. I did however find bus_create_file() which does work when using the path /sys/bus/pci/uid_checking instead. This would work for us if Bjorn is okay with that path and the code is really clean and simple too. That said, I think we could also add something like bus_create_group(). Then we could use that to also clean up drivers/pci/slot.c:pci_slot_init() and get the original path /sys/bus/pci/zpci/uid_checking. I think this would also allow us to get rid of pci_bus_get_kset() which is only used in that function and seems to me like it encourages use of raw ksets. Or I'm completely off the mark and just missing something important. thanks, Niklas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-15 11:20 ` Niklas Schnelle @ 2021-01-15 12:02 ` Greg Kroah-Hartman 2021-01-15 15:29 ` Bjorn Helgaas 1 sibling, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-15 12:02 UTC (permalink / raw) To: Niklas Schnelle Cc: Christian Brauner, Bjorn Helgaas, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: > > > On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > > On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > >> > >> > >> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > >>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > >>>> > >>>> > >>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > >>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > ... snip ... > >>>>>> > >>>>>> Hey Niklas et al. :) > >>>>>> > >>>>>> I think this will need input from Greg. He should be best versed in > >>>>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's > >>>>>> supposed to be kernel internal. Now, that might just be a matter of > >>>>>> renaming the macro but let's see whether Greg has any better idea or > >>>>>> more questions. :) > >>>>> > >>>>> The big question is, why are you needing this? > >>>>> > >>>>> No driver or driver subsystem should EVER be messing with a "raw" > >>>>> kobject like this. Just use the existing DEVICE_* macros instead > >>>>> please. > >>>>> > >>>>> If you are using a raw kobject, please ask me how to do this properly, > >>>>> as that is something that should NEVER show up in the /sys/devices/* > >>>>> tree. Otherwise userspace tools will break. > >>>>> > >>>>> thanks, > >>>>> > >>>>> greg k-h > >>>> > >>>> Hi Greg, > >>>> > >>>> this is for an architecture specific but global i.e. not device bound PCI > >>>> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work > >>>> but only if we place the attribute directly under /sys/bus/pci/new_attr. > >>> > >>> Then you are doing something wrong :) > >> > >> That is very possible. > >> > >>> > >>> Where _exactly_ are you wanting to put this attribute? > >> > >> I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using > >> the below code and the attribute even shows up but reading > >> it gives me two 0 bytes only. > >> The relevant code is only a slight alteration of the original patch > >> as follows: > >> > >> static ssize_t uid_checking_show(struct bus_type *bus, char *buf) > >> { > >> return sprintf(buf, "%i\n", zpci_unique_uid); > >> } > >> static BUS_ATTR_RO(uid_checking); > >> > >> static struct kset *zpci_global_kset; > >> > >> static struct attribute *zpci_attrs_global[] = { > >> &bus_attr_uid_checking.attr, > >> NULL, > >> }; > >> > >> static struct attribute_group zpci_attr_group_global = { > >> .attrs = zpci_attrs_global, > >> }; > > > > Name your attribute group, and then you do not have to mess with a > > "raw" kobject like you are below: > > Thanks for this tip and sorry for bothering you again. > > > > >> > >> int __init zpci_sysfs_init(void) > >> { > >> struct kset *pci_bus_kset; > >> > >> pci_bus_kset = bus_get_kset(&pci_bus_type); > >> > >> zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); > > > > No, do not mess with at kset, just set the default attribute group for > > the bus to the above, and you should be fine. > > Oh ok, I got this idea from the code adding /sys/bus/pci/slots/ in > drivers/pci/slot.c:pci_slot_init(). See below maybe we can clean that up too. Slots are "interesting" and that code is really old, we know how to do things better now :) But I doubt we should change anything there, as it does work, and userspace is used to how they come/go. > >> if (!zpci_global_kset) > >> return -ENOMEM; > >> > >> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > > > > Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > > that's usually a huge clue that you are doing something wrong. > > > > Try the above again, with a simple attribute group, and name for it, and > > it should "just work". > > I'm probably missing something but I don't get how this could work in > this case. If I'm seeing this right the default attribute group here > is pci_bus_type.bus_groups and that is already set in drivers/pci/pci-driver.c > so I don't think I should set that. Yes, add your group to that list of groups and all should be good. > I did however find bus_create_file() which does work when using the path > /sys/bus/pci/uid_checking instead. This would work for us if Bjorn is okay with > that path and the code is really clean and simple too. No, use the above group you already have please. > That said, I think we could also add something like bus_create_group(). No, use the group list as show above please. > Then we could use that to also clean up drivers/pci/slot.c:pci_slot_init() > and get the original path /sys/bus/pci/zpci/uid_checking. What needs to be cleaned up there? > I think this would also allow us to get rid of pci_bus_get_kset() which is > only used in that function and seems to me like it encourages use of raw ksets. > Or I'm completely off the mark and just missing something important. Cleaning up slots is great, but they are "odd", so be careful. thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-15 11:20 ` Niklas Schnelle 2021-01-15 12:02 ` Greg Kroah-Hartman @ 2021-01-15 15:29 ` Bjorn Helgaas 2021-01-15 16:15 ` Niklas Schnelle 2021-01-21 15:31 ` Niklas Schnelle 1 sibling, 2 replies; 22+ messages in thread From: Bjorn Helgaas @ 2021-01-15 15:29 UTC (permalink / raw) To: Niklas Schnelle Cc: Greg Kroah-Hartman, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: > On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > > On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > >> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > >>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > >>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > >>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > ... snip ... > >>>>>> > >>>>>> Hey Niklas et al. :) > >>>>>> > >>>>>> I think this will need input from Greg. He should be best versed in > >>>>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's > >>>>>> supposed to be kernel internal. Now, that might just be a matter of > >>>>>> renaming the macro but let's see whether Greg has any better idea or > >>>>>> more questions. :) > >>>>> > >>>>> The big question is, why are you needing this? > >>>>> > >>>>> No driver or driver subsystem should EVER be messing with a "raw" > >>>>> kobject like this. Just use the existing DEVICE_* macros instead > >>>>> please. > >>>>> > >>>>> If you are using a raw kobject, please ask me how to do this properly, > >>>>> as that is something that should NEVER show up in the /sys/devices/* > >>>>> tree. Otherwise userspace tools will break. > >>>>> > >>>>> thanks, > >>>>> > >>>>> greg k-h > >>>> > >>>> Hi Greg, > >>>> > >>>> this is for an architecture specific but global i.e. not device bound PCI > >>>> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work > >>>> but only if we place the attribute directly under /sys/bus/pci/new_attr. > >>> > >>> Then you are doing something wrong :) > >> > >> That is very possible. > >> > >>> > >>> Where _exactly_ are you wanting to put this attribute? > >> > >> I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using > >> the below code and the attribute even shows up but reading > >> it gives me two 0 bytes only. > >> The relevant code is only a slight alteration of the original patch > >> as follows: > >> > >> static ssize_t uid_checking_show(struct bus_type *bus, char *buf) > >> { > >> return sprintf(buf, "%i\n", zpci_unique_uid); > >> } > >> static BUS_ATTR_RO(uid_checking); > >> > >> static struct kset *zpci_global_kset; > >> > >> static struct attribute *zpci_attrs_global[] = { > >> &bus_attr_uid_checking.attr, > >> NULL, > >> }; > >> > >> static struct attribute_group zpci_attr_group_global = { > >> .attrs = zpci_attrs_global, > >> }; > > > > Name your attribute group, and then you do not have to mess with a > > "raw" kobject like you are below: > > Thanks for this tip and sorry for bothering you again. > > > > >> > >> int __init zpci_sysfs_init(void) > >> { > >> struct kset *pci_bus_kset; > >> > >> pci_bus_kset = bus_get_kset(&pci_bus_type); > >> > >> zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); > > > > No, do not mess with at kset, just set the default attribute group for > > the bus to the above, and you should be fine. > > Oh ok, I got this idea from the code adding /sys/bus/pci/slots/ in > drivers/pci/slot.c:pci_slot_init(). See below maybe we can clean that up too. > > > > >> if (!zpci_global_kset) > >> return -ENOMEM; > >> > >> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > > > > Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > > that's usually a huge clue that you are doing something wrong. > > > > Try the above again, with a simple attribute group, and name for it, and > > it should "just work". > > I'm probably missing something but I don't get how this could work > in this case. If I'm seeing this right the default attribute group > here is pci_bus_type.bus_groups and that is already set in > drivers/pci/pci-driver.c so I don't think I should set that. > > I did however find bus_create_file() which does work when using the > path /sys/bus/pci/uid_checking instead. This would work for us if > Bjorn is okay with that path and the code is really clean and simple > too. > > That said, I think we could also add something like > bus_create_group(). Then we could use that to also clean up > drivers/pci/slot.c:pci_slot_init() and get the original path > /sys/bus/pci/zpci/uid_checking. I don't think "uid_checking" is quite the right name. It says something about the *implementation*, but it doesn't convey what that *means* to userspace. IIUC this file tells userspace something about whether a given PCI device always has the same PCI domain/bus/dev/fn address (or maybe just the same domain?) It sounds like this feature could be useful beyond just s390, and other arches might implement it differently, without the UID concept. If so, I'm OK with something at the /sys/bus/pci/xxx level as long as the name is not s390-specific (and "uid" sounds s390-specific). I assume it would also help with the udev/systemd end if you could make this less s390 dependent. Bjorn ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-15 15:29 ` Bjorn Helgaas @ 2021-01-15 16:15 ` Niklas Schnelle 2021-01-21 15:31 ` Niklas Schnelle 1 sibling, 0 replies; 22+ messages in thread From: Niklas Schnelle @ 2021-01-15 16:15 UTC (permalink / raw) To: Bjorn Helgaas Cc: Greg Kroah-Hartman, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/15/21 4:29 PM, Bjorn Helgaas wrote: > On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: >> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: >>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: >>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: >>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >> ... snip ... >>>>>>>> >>>>>>>> Hey Niklas et al. :) >>>>>>>> >>>>>>>> I think this will need input from Greg. He should be best versed in >>>>>>>> sysfs attributes. The problem with KERNEL_ATTR_* to me seems that it's >>>>>>>> supposed to be kernel internal. Now, that might just be a matter of >>>>>>>> renaming the macro but let's see whether Greg has any better idea or >>>>>>>> more questions. :) >>>>>>> >>>>>>> The big question is, why are you needing this? >>>>>>> >>>>>>> No driver or driver subsystem should EVER be messing with a "raw" >>>>>>> kobject like this. Just use the existing DEVICE_* macros instead >>>>>>> please. >>>>>>> >>>>>>> If you are using a raw kobject, please ask me how to do this properly, >>>>>>> as that is something that should NEVER show up in the /sys/devices/* >>>>>>> tree. Otherwise userspace tools will break. >>>>>>> >>>>>>> thanks, >>>>>>> >>>>>>> greg k-h >>>>>> >>>>>> Hi Greg, >>>>>> >>>>>> this is for an architecture specific but global i.e. not device bound PCI >>>>>> attribute. That's why DEVICE_ATTR_* does not work. BUS_ATTR_* would work >>>>>> but only if we place the attribute directly under /sys/bus/pci/new_attr. >>>>> >>>>> Then you are doing something wrong :) >>>> >>>> That is very possible. >>>> >>>>> >>>>> Where _exactly_ are you wanting to put this attribute? >>>> >>>> I'm trying for /sys/bus/pci/zpci/uid_checking, I'm using >>>> the below code and the attribute even shows up but reading >>>> it gives me two 0 bytes only. >>>> The relevant code is only a slight alteration of the original patch >>>> as follows: >>>> >>>> static ssize_t uid_checking_show(struct bus_type *bus, char *buf) >>>> { >>>> return sprintf(buf, "%i\n", zpci_unique_uid); >>>> } >>>> static BUS_ATTR_RO(uid_checking); >>>> >>>> static struct kset *zpci_global_kset; >>>> >>>> static struct attribute *zpci_attrs_global[] = { >>>> &bus_attr_uid_checking.attr, >>>> NULL, >>>> }; >>>> >>>> static struct attribute_group zpci_attr_group_global = { >>>> .attrs = zpci_attrs_global, >>>> }; >>> >>> Name your attribute group, and then you do not have to mess with a >>> "raw" kobject like you are below: >> >> Thanks for this tip and sorry for bothering you again. >> >>> >>>> >>>> int __init zpci_sysfs_init(void) >>>> { >>>> struct kset *pci_bus_kset; >>>> >>>> pci_bus_kset = bus_get_kset(&pci_bus_type); >>>> >>>> zpci_global_kset = kset_create_and_add("zpci", NULL, &pci_bus_kset->kobj); >>> >>> No, do not mess with at kset, just set the default attribute group for >>> the bus to the above, and you should be fine. >> >> Oh ok, I got this idea from the code adding /sys/bus/pci/slots/ in >> drivers/pci/slot.c:pci_slot_init(). See below maybe we can clean that up too. >> >>> >>>> if (!zpci_global_kset) >>>> return -ENOMEM; >>>> >>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); >>> >>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, >>> that's usually a huge clue that you are doing something wrong. >>> >>> Try the above again, with a simple attribute group, and name for it, and >>> it should "just work". >> >> I'm probably missing something but I don't get how this could work >> in this case. If I'm seeing this right the default attribute group >> here is pci_bus_type.bus_groups and that is already set in >> drivers/pci/pci-driver.c so I don't think I should set that. >> >> I did however find bus_create_file() which does work when using the >> path /sys/bus/pci/uid_checking instead. This would work for us if >> Bjorn is okay with that path and the code is really clean and simple >> too. >> >> That said, I think we could also add something like >> bus_create_group(). Then we could use that to also clean up >> drivers/pci/slot.c:pci_slot_init() and get the original path >> /sys/bus/pci/zpci/uid_checking. > > I don't think "uid_checking" is quite the right name. It says > something about the *implementation*, but it doesn't convey what that > *means* to userspace. IIUC this file tells userspace something about > whether a given PCI device always has the same PCI domain/bus/dev/fn > address (or maybe just the same domain?) Yes you're right, in fact we internally also started to think about something more meaning oriented like "unique_uids". This indeed results in PCI addresses which can be relied upon as stable identifiers. For us it's enough that the domain matches as the bus is always 0 and dev/fn are determined by the device and SR-IOV stride etc. so will remain the same for equivalent configurations. > > It sounds like this feature could be useful beyond just s390, and > other arches might implement it differently, without the UID concept. > If so, I'm OK with something at the /sys/bus/pci/xxx level as long as > the name is not s390-specific (and "uid" sounds s390-specific). > > I assume it would also help with the udev/systemd end if you could > make this less s390 dependent. > > Bjorn That's a very good point! I'm absolutely open to making this a common concept. I think the gist could be that the addressing/ids on the bus are reproducible, there might be some user configuration required but then they can be relied upon for stable names like network interfaces. So maybe a name like "reproducible_addressing"? I guess one non-s390 exclusive case of this could be virtualized PCI under QEMU/KVM etc. versus real hardware or even UEFI/Firmware guarantees, right? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-15 15:29 ` Bjorn Helgaas 2021-01-15 16:15 ` Niklas Schnelle @ 2021-01-21 15:31 ` Niklas Schnelle 2021-01-21 15:54 ` Bjorn Helgaas 1 sibling, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-21 15:31 UTC (permalink / raw) To: Bjorn Helgaas Cc: Greg Kroah-Hartman, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/15/21 4:29 PM, Bjorn Helgaas wrote: > On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: >> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: >>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: >>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: >>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >> ... snip ... >> >>> >>>> if (!zpci_global_kset) >>>> return -ENOMEM; >>>> >>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); >>> >>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, >>> that's usually a huge clue that you are doing something wrong. >>> >>> Try the above again, with a simple attribute group, and name for it, and >>> it should "just work". >> >> I'm probably missing something but I don't get how this could work >> in this case. If I'm seeing this right the default attribute group >> here is pci_bus_type.bus_groups and that is already set in >> drivers/pci/pci-driver.c so I don't think I should set that. >> >> I did however find bus_create_file() which does work when using the >> path /sys/bus/pci/uid_checking instead. This would work for us if >> Bjorn is okay with that path and the code is really clean and simple >> too. >> >> That said, I think we could also add something like >> bus_create_group(). Then we could use that to also clean up >> drivers/pci/slot.c:pci_slot_init() and get the original path >> /sys/bus/pci/zpci/uid_checking. > > I don't think "uid_checking" is quite the right name. It says > something about the *implementation*, but it doesn't convey what that > *means* to userspace. IIUC this file tells userspace something about > whether a given PCI device always has the same PCI domain/bus/dev/fn > address (or maybe just the same domain?) > > It sounds like this feature could be useful beyond just s390, and > other arches might implement it differently, without the UID concept. > If so, I'm OK with something at the /sys/bus/pci/xxx level as long as > the name is not s390-specific (and "uid" sounds s390-specific). > > I assume it would also help with the udev/systemd end if you could > make this less s390 dependent. > > Bjorn > Hi Bjorn, I've thought about this more and even implemented a proof of concept patch for a global attribute using a pcibios_has_reproducible_addressing() hook. However after implementing it I think as a more general and future proof concept it makes more sense to do this as a per device attribute, maybe as another flag in "stuct pci_dev" named something like "reliable_address". My reasoning behind this can be best be seen with a QEMU example. While I expect that QEMU can easily guarantee that one can always use "0000:01:00.0" for a virtio-pci NIC and thus enp1s0 interface name, the same might be harder to guarantee for a SR-IOV VF passed through with vfio-pci in that same VM and even less so if a thunderbolt controller is passed through and enumeration may depend on daisy chaining. The QEMU example also applies to s390 and maybe others will in the future. I'll send an RFC for a per device attribute but didn't want to let this discussion stand as if we had abandoned the idea and if you would prefer a global attribute I can also send an RFC for that. Besst regards, Niklas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-21 15:31 ` Niklas Schnelle @ 2021-01-21 15:54 ` Bjorn Helgaas 2021-01-21 16:11 ` Greg Kroah-Hartman 2021-01-21 17:04 ` Niklas Schnelle 0 siblings, 2 replies; 22+ messages in thread From: Bjorn Helgaas @ 2021-01-21 15:54 UTC (permalink / raw) To: Niklas Schnelle, Greg Kroah-Hartman Cc: Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski [Greg may be able to help compare/contrast this s390 UID with udev persistent names] On Thu, Jan 21, 2021 at 04:31:55PM +0100, Niklas Schnelle wrote: > On 1/15/21 4:29 PM, Bjorn Helgaas wrote: > > On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: > >> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > >>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > >>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > >>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > >>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > >>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > >> ... snip ... > >> > >>> > >>>> if (!zpci_global_kset) > >>>> return -ENOMEM; > >>>> > >>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > >>> > >>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > >>> that's usually a huge clue that you are doing something wrong. > >>> > >>> Try the above again, with a simple attribute group, and name for it, and > >>> it should "just work". > >> > >> I'm probably missing something but I don't get how this could work > >> in this case. If I'm seeing this right the default attribute group > >> here is pci_bus_type.bus_groups and that is already set in > >> drivers/pci/pci-driver.c so I don't think I should set that. > >> > >> I did however find bus_create_file() which does work when using the > >> path /sys/bus/pci/uid_checking instead. This would work for us if > >> Bjorn is okay with that path and the code is really clean and simple > >> too. > >> > >> That said, I think we could also add something like > >> bus_create_group(). Then we could use that to also clean up > >> drivers/pci/slot.c:pci_slot_init() and get the original path > >> /sys/bus/pci/zpci/uid_checking. > > > > I don't think "uid_checking" is quite the right name. It says > > something about the *implementation*, but it doesn't convey what that > > *means* to userspace. IIUC this file tells userspace something about > > whether a given PCI device always has the same PCI domain/bus/dev/fn > > address (or maybe just the same domain?) > > > > It sounds like this feature could be useful beyond just s390, and > > other arches might implement it differently, without the UID concept. > > If so, I'm OK with something at the /sys/bus/pci/xxx level as long as > > the name is not s390-specific (and "uid" sounds s390-specific). > > > > I assume it would also help with the udev/systemd end if you could > > make this less s390 dependent. > > I've thought about this more and even implemented a proof of concept > patch for a global attribute using a pcibios_has_reproducible_addressing() > hook. > > However after implementing it I think as a more general and > future proof concept it makes more sense to do this as a per device > attribute, maybe as another flag in "stuct pci_dev" named something > like "reliable_address". My reasoning behind this can be best be seen > with a QEMU example. While I expect that QEMU can easily guarantee > that one can always use "0000:01:00.0" for a virtio-pci NIC and > thus enp1s0 interface name, the same might be harder to guarantee > for a SR-IOV VF passed through with vfio-pci in that same VM and > even less so if a thunderbolt controller is passed through and > enumeration may depend on daisy chaining. The QEMU example > also applies to s390 and maybe others will in the future. I'm a little wary of using the PCI geographical address ("0000:01:00.0") as a stable name. Even if you can make a way to use that to identify a specific device instance, regardless of how it is plugged in or passed through, it sounds like we could end up with "physical PCI addresses" and "virtual PCI addresses" that look the same and would cause confusion. This concept sounds similar to the udev concept of a "persistent device name". What advantages does this s390 UID have over the udev approach? There are optional PCI device serial numbers that we currently don't really make use of. Would that be a generic way to help with this? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-21 15:54 ` Bjorn Helgaas @ 2021-01-21 16:11 ` Greg Kroah-Hartman 2021-01-21 17:04 ` Niklas Schnelle 1 sibling, 0 replies; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-21 16:11 UTC (permalink / raw) To: Bjorn Helgaas Cc: Niklas Schnelle, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 21, 2021 at 09:54:45AM -0600, Bjorn Helgaas wrote: > [Greg may be able to help compare/contrast this s390 UID with udev > persistent names] > > On Thu, Jan 21, 2021 at 04:31:55PM +0100, Niklas Schnelle wrote: > > On 1/15/21 4:29 PM, Bjorn Helgaas wrote: > > > On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: > > >> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > > >>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > > >>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > > >>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > > >>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > > >>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > > >>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > > >>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > > >>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > > >>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > > >> ... snip ... > > >> > > >>> > > >>>> if (!zpci_global_kset) > > >>>> return -ENOMEM; > > >>>> > > >>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > > >>> > > >>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > > >>> that's usually a huge clue that you are doing something wrong. > > >>> > > >>> Try the above again, with a simple attribute group, and name for it, and > > >>> it should "just work". > > >> > > >> I'm probably missing something but I don't get how this could work > > >> in this case. If I'm seeing this right the default attribute group > > >> here is pci_bus_type.bus_groups and that is already set in > > >> drivers/pci/pci-driver.c so I don't think I should set that. > > >> > > >> I did however find bus_create_file() which does work when using the > > >> path /sys/bus/pci/uid_checking instead. This would work for us if > > >> Bjorn is okay with that path and the code is really clean and simple > > >> too. > > >> > > >> That said, I think we could also add something like > > >> bus_create_group(). Then we could use that to also clean up > > >> drivers/pci/slot.c:pci_slot_init() and get the original path > > >> /sys/bus/pci/zpci/uid_checking. > > > > > > I don't think "uid_checking" is quite the right name. It says > > > something about the *implementation*, but it doesn't convey what that > > > *means* to userspace. IIUC this file tells userspace something about > > > whether a given PCI device always has the same PCI domain/bus/dev/fn > > > address (or maybe just the same domain?) > > > > > > It sounds like this feature could be useful beyond just s390, and > > > other arches might implement it differently, without the UID concept. > > > If so, I'm OK with something at the /sys/bus/pci/xxx level as long as > > > the name is not s390-specific (and "uid" sounds s390-specific). > > > > > > I assume it would also help with the udev/systemd end if you could > > > make this less s390 dependent. > > > > I've thought about this more and even implemented a proof of concept > > patch for a global attribute using a pcibios_has_reproducible_addressing() > > hook. > > > > However after implementing it I think as a more general and > > future proof concept it makes more sense to do this as a per device > > attribute, maybe as another flag in "stuct pci_dev" named something > > like "reliable_address". My reasoning behind this can be best be seen > > with a QEMU example. While I expect that QEMU can easily guarantee > > that one can always use "0000:01:00.0" for a virtio-pci NIC and > > thus enp1s0 interface name, the same might be harder to guarantee > > for a SR-IOV VF passed through with vfio-pci in that same VM and > > even less so if a thunderbolt controller is passed through and > > enumeration may depend on daisy chaining. The QEMU example > > also applies to s390 and maybe others will in the future. > > I'm a little wary of using the PCI geographical address > ("0000:01:00.0") as a stable name. Even if you can make a way to use > that to identify a specific device instance, regardless of how it is > plugged in or passed through, it sounds like we could end up with > "physical PCI addresses" and "virtual PCI addresses" that look the > same and would cause confusion. Agreed, as we all know, PCI addresses are never a stable name and can change every boot on some systems. Never rely on them, but you can use them as a "hint" for something that you have to determine is different from something else that is the same type of device. > This concept sounds similar to the udev concept of a "persistent > device name". What advantages does this s390 UID have over the udev > approach? > > There are optional PCI device serial numbers that we currently don't > really make use of. Would that be a generic way to help with this? Only if you can require that they be unique. Is there such a requirement and who enforces it? For USB, it was only "required" to have unique serial numbers for one class of devices (printers), and even then, it really wasn't enforced that much so you can't rely on it being unique at all, which makes it pretty useless :( thanks, greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-21 15:54 ` Bjorn Helgaas 2021-01-21 16:11 ` Greg Kroah-Hartman @ 2021-01-21 17:04 ` Niklas Schnelle 2021-01-21 17:28 ` Greg Kroah-Hartman 1 sibling, 1 reply; 22+ messages in thread From: Niklas Schnelle @ 2021-01-21 17:04 UTC (permalink / raw) To: Bjorn Helgaas, Greg Kroah-Hartman, Lennart Poettering Cc: Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/21/21 4:54 PM, Bjorn Helgaas wrote: > [Greg may be able to help compare/contrast this s390 UID with udev > persistent names] > > On Thu, Jan 21, 2021 at 04:31:55PM +0100, Niklas Schnelle wrote: >> On 1/15/21 4:29 PM, Bjorn Helgaas wrote: >>> On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: >>>> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: >>>>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: >>>>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: >>>>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >>>>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>>>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >>>> ... snip ... >>>> >>>>> >>>>>> if (!zpci_global_kset) >>>>>> return -ENOMEM; >>>>>> >>>>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); >>>>> >>>>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, >>>>> that's usually a huge clue that you are doing something wrong. >>>>> >>>>> Try the above again, with a simple attribute group, and name for it, and >>>>> it should "just work". >>>> >>>> I'm probably missing something but I don't get how this could work >>>> in this case. If I'm seeing this right the default attribute group >>>> here is pci_bus_type.bus_groups and that is already set in >>>> drivers/pci/pci-driver.c so I don't think I should set that. >>>> >>>> I did however find bus_create_file() which does work when using the >>>> path /sys/bus/pci/uid_checking instead. This would work for us if >>>> Bjorn is okay with that path and the code is really clean and simple >>>> too. >>>> >>>> That said, I think we could also add something like >>>> bus_create_group(). Then we could use that to also clean up >>>> drivers/pci/slot.c:pci_slot_init() and get the original path >>>> /sys/bus/pci/zpci/uid_checking. >>> >>> I don't think "uid_checking" is quite the right name. It says >>> something about the *implementation*, but it doesn't convey what that >>> *means* to userspace. IIUC this file tells userspace something about >>> whether a given PCI device always has the same PCI domain/bus/dev/fn >>> address (or maybe just the same domain?) >>> >>> It sounds like this feature could be useful beyond just s390, and >>> other arches might implement it differently, without the UID concept. >>> If so, I'm OK with something at the /sys/bus/pci/xxx level as long as >>> the name is not s390-specific (and "uid" sounds s390-specific). >>> >>> I assume it would also help with the udev/systemd end if you could >>> make this less s390 dependent. >> >> I've thought about this more and even implemented a proof of concept >> patch for a global attribute using a pcibios_has_reproducible_addressing() >> hook. >> >> However after implementing it I think as a more general and >> future proof concept it makes more sense to do this as a per device >> attribute, maybe as another flag in "stuct pci_dev" named something >> like "reliable_address". My reasoning behind this can be best be seen >> with a QEMU example. While I expect that QEMU can easily guarantee >> that one can always use "0000:01:00.0" for a virtio-pci NIC and >> thus enp1s0 interface name, the same might be harder to guarantee >> for a SR-IOV VF passed through with vfio-pci in that same VM and >> even less so if a thunderbolt controller is passed through and >> enumeration may depend on daisy chaining. The QEMU example >> also applies to s390 and maybe others will in the future. > > I'm a little wary of using the PCI geographical address > ("0000:01:00.0") as a stable name. Even if you can make a way to use > that to identify a specific device instance, regardless of how it is > plugged in or passed through, it sounds like we could end up with > "physical PCI addresses" and "virtual PCI addresses" that look the > same and would cause confusion. > > This concept sounds similar to the udev concept of a "persistent > device name". What advantages does this s390 UID have over the udev > approach? > > There are optional PCI device serial numbers that we currently don't > really make use of. Would that be a generic way to help with this? > As far as I understand systemd/udev uses the PCI geographical address by default ("enP<domain>p<bus>s<hotplug_slot_idx>...") for PCI attached network interfaces in many cases and a lot of users have already built their firewall/routing rules on these. At the same time as you said this isn't ideal. On s390 things are a bit special since it's either really unstable, if UID Checking is not enforced (the value I wanted to expose is 0) the domain part and thus the interface name may change between reboots. On the other hand when it is enforced, the Domain can be defined in the machine configuration (IOCDS, think Domain XML but formatted for punch cards) and the bus is always 00. The PCI geographical address and thus network interface names are then stable in the same way as with our CCW attached network interfaces where the CCW addresses have been stable for a long time and are regularly used for configuration. The interface would however still conform to the existing systemd/udev standard theme so the only change the user sees is that we would prefer the interface naming scheme which does not include the hotplug slot index (ehotplug slot indices are unique in the machine hypervisor and thus can't be the same in to guests on the same machine). We would thus not add a new name at all and thanks to the altname mechanism all existing rules including persistent naming via manual udev rules would keep working. Now taking this beyond s390 my idea is that under some circumstances just as with UID Uniqueness for us, the platform can tell if a PCI geographical address is a reliable identifier thus sytemd/udev has more information about the quality of existing naming schemes incorporating information from the geographical address. Looking at my personal KVM guests (Ubuntu, Arch Linux, Ubuntu ARM64) as well as my workstation (Arch Linux) all of them use a scheme with parts of the geographical address. So in essence my idea is all about either choosing the best existing default name or making sure we at least know if it may not be reliable. I hope that makes sense and I have added Lennart Poettering as he's the one everyone likes to blame for these names or at least can probably tell me what I missed. Best, Niklas ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-21 17:04 ` Niklas Schnelle @ 2021-01-21 17:28 ` Greg Kroah-Hartman 2021-01-21 17:43 ` Niklas Schnelle 0 siblings, 1 reply; 22+ messages in thread From: Greg Kroah-Hartman @ 2021-01-21 17:28 UTC (permalink / raw) To: Niklas Schnelle Cc: Bjorn Helgaas, Lennart Poettering, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On Thu, Jan 21, 2021 at 06:04:52PM +0100, Niklas Schnelle wrote: > > > On 1/21/21 4:54 PM, Bjorn Helgaas wrote: > > [Greg may be able to help compare/contrast this s390 UID with udev > > persistent names] > > > > On Thu, Jan 21, 2021 at 04:31:55PM +0100, Niklas Schnelle wrote: > >> On 1/15/21 4:29 PM, Bjorn Helgaas wrote: > >>> On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: > >>>> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: > >>>>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: > >>>>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: > >>>>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: > >>>>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: > >>>>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: > >>>>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: > >>>>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: > >>>>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: > >>>>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: > >>>> ... snip ... > >>>> > >>>>> > >>>>>> if (!zpci_global_kset) > >>>>>> return -ENOMEM; > >>>>>> > >>>>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); > >>>>> > >>>>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, > >>>>> that's usually a huge clue that you are doing something wrong. > >>>>> > >>>>> Try the above again, with a simple attribute group, and name for it, and > >>>>> it should "just work". > >>>> > >>>> I'm probably missing something but I don't get how this could work > >>>> in this case. If I'm seeing this right the default attribute group > >>>> here is pci_bus_type.bus_groups and that is already set in > >>>> drivers/pci/pci-driver.c so I don't think I should set that. > >>>> > >>>> I did however find bus_create_file() which does work when using the > >>>> path /sys/bus/pci/uid_checking instead. This would work for us if > >>>> Bjorn is okay with that path and the code is really clean and simple > >>>> too. > >>>> > >>>> That said, I think we could also add something like > >>>> bus_create_group(). Then we could use that to also clean up > >>>> drivers/pci/slot.c:pci_slot_init() and get the original path > >>>> /sys/bus/pci/zpci/uid_checking. > >>> > >>> I don't think "uid_checking" is quite the right name. It says > >>> something about the *implementation*, but it doesn't convey what that > >>> *means* to userspace. IIUC this file tells userspace something about > >>> whether a given PCI device always has the same PCI domain/bus/dev/fn > >>> address (or maybe just the same domain?) > >>> > >>> It sounds like this feature could be useful beyond just s390, and > >>> other arches might implement it differently, without the UID concept. > >>> If so, I'm OK with something at the /sys/bus/pci/xxx level as long as > >>> the name is not s390-specific (and "uid" sounds s390-specific). > >>> > >>> I assume it would also help with the udev/systemd end if you could > >>> make this less s390 dependent. > >> > >> I've thought about this more and even implemented a proof of concept > >> patch for a global attribute using a pcibios_has_reproducible_addressing() > >> hook. > >> > >> However after implementing it I think as a more general and > >> future proof concept it makes more sense to do this as a per device > >> attribute, maybe as another flag in "stuct pci_dev" named something > >> like "reliable_address". My reasoning behind this can be best be seen > >> with a QEMU example. While I expect that QEMU can easily guarantee > >> that one can always use "0000:01:00.0" for a virtio-pci NIC and > >> thus enp1s0 interface name, the same might be harder to guarantee > >> for a SR-IOV VF passed through with vfio-pci in that same VM and > >> even less so if a thunderbolt controller is passed through and > >> enumeration may depend on daisy chaining. The QEMU example > >> also applies to s390 and maybe others will in the future. > > > > I'm a little wary of using the PCI geographical address > > ("0000:01:00.0") as a stable name. Even if you can make a way to use > > that to identify a specific device instance, regardless of how it is > > plugged in or passed through, it sounds like we could end up with > > "physical PCI addresses" and "virtual PCI addresses" that look the > > same and would cause confusion. > > > > This concept sounds similar to the udev concept of a "persistent > > device name". What advantages does this s390 UID have over the udev > > approach? > > > > There are optional PCI device serial numbers that we currently don't > > really make use of. Would that be a generic way to help with this? > > > > As far as I understand systemd/udev uses the PCI geographical address > by default ("enP<domain>p<bus>s<hotplug_slot_idx>...") for PCI attached > network interfaces in many cases and a lot of users have already built > their firewall/routing rules on these. Which is fine as "normally" that does not change. But on some machines, it is quite volatile so users pick a different naming scheme. And this is all done in userspace, I really don't understand what you want to do in the kernel here. If you want to expose another unique thing that the hardware knows about, wonderful, userspace can then use that if it wants to in how it names specific devices. But don't put that naming in the kernel, that's not where it belongs. > Now taking this beyond s390 my idea is that under some circumstances > just as with UID Uniqueness for us, the platform can tell if a PCI > geographical address is a reliable identifier thus sytemd/udev > has more information about the quality of existing naming schemes > incorporating information from the geographical address. The platform does not "know" if this is reliable or not, sorry. That's not how PCI or UEFI works. > Looking at my personal KVM guests (Ubuntu, Arch Linux, Ubuntu ARM64) > as well as my workstation (Arch Linux) all of them use a scheme > with parts of the geographical address. Because for the most part, yes, this works. Until you plug another device into the system. Or remove one. Or plug a hotplug device in and then cold boot with it plugged in (or removed). Or, my favorite system, just decide to renumber the PCI bus every other boot "just because". None of that variability can be known by the kernel, that's only known by the user of that system, so again, they can make the best decision as to how to name their devices. If you want to use the systemd default, wonderful, but know that it does not work for everyone, so systemd allows you to do whatever you want. > So in essence my idea is all about either choosing the best existing > default name or making sure we at least know if it may not be reliable. There is no reliability "score" here, sorry. Hardware is fun :) good luck! greg k-h ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC 1/1] s390/pci: expose UID checking state in sysfs 2021-01-21 17:28 ` Greg Kroah-Hartman @ 2021-01-21 17:43 ` Niklas Schnelle 0 siblings, 0 replies; 22+ messages in thread From: Niklas Schnelle @ 2021-01-21 17:43 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Bjorn Helgaas, Lennart Poettering, Christian Brauner, linux-pci, linux-kernel, linux-s390, Pierre Morel, Peter Oberparleiter, Viktor Mihajlovski On 1/21/21 6:28 PM, Greg Kroah-Hartman wrote: > On Thu, Jan 21, 2021 at 06:04:52PM +0100, Niklas Schnelle wrote: >> >> >> On 1/21/21 4:54 PM, Bjorn Helgaas wrote: >>> [Greg may be able to help compare/contrast this s390 UID with udev >>> persistent names] >>> >>> On Thu, Jan 21, 2021 at 04:31:55PM +0100, Niklas Schnelle wrote: >>>> On 1/15/21 4:29 PM, Bjorn Helgaas wrote: >>>>> On Fri, Jan 15, 2021 at 12:20:59PM +0100, Niklas Schnelle wrote: >>>>>> On 1/14/21 5:14 PM, Greg Kroah-Hartman wrote: >>>>>>> On Thu, Jan 14, 2021 at 04:51:17PM +0100, Niklas Schnelle wrote: >>>>>>>> On 1/14/21 4:17 PM, Greg Kroah-Hartman wrote: >>>>>>>>> On Thu, Jan 14, 2021 at 04:06:11PM +0100, Niklas Schnelle wrote: >>>>>>>>>> On 1/14/21 2:58 PM, Greg Kroah-Hartman wrote: >>>>>>>>>>> On Thu, Jan 14, 2021 at 02:44:53PM +0100, Christian Brauner wrote: >>>>>>>>>>>> On Thu, Jan 14, 2021 at 02:20:10PM +0100, Niklas Schnelle wrote: >>>>>>>>>>>>> On 1/13/21 7:55 PM, Bjorn Helgaas wrote: >>>>>>>>>>>>>> On Wed, Jan 13, 2021 at 08:47:58AM +0100, Niklas Schnelle wrote: >>>>>>>>>>>>>>> On 1/12/21 10:50 PM, Bjorn Helgaas wrote: >>>>>> ... snip ... >>>>>> >>>>>>> >>>>>>>> if (!zpci_global_kset) >>>>>>>> return -ENOMEM; >>>>>>>> >>>>>>>> return sysfs_create_group(&zpci_global_kset->kobj, &zpci_attr_group_global); >>>>>>> >>>>>>> Huge hint, if in a driver, or bus subsystem, and you call sysfs_*, >>>>>>> that's usually a huge clue that you are doing something wrong. >>>>>>> >>>>>>> Try the above again, with a simple attribute group, and name for it, and >>>>>>> it should "just work". >>>>>> >>>>>> I'm probably missing something but I don't get how this could work >>>>>> in this case. If I'm seeing this right the default attribute group >>>>>> here is pci_bus_type.bus_groups and that is already set in >>>>>> drivers/pci/pci-driver.c so I don't think I should set that. >>>>>> >>>>>> I did however find bus_create_file() which does work when using the >>>>>> path /sys/bus/pci/uid_checking instead. This would work for us if >>>>>> Bjorn is okay with that path and the code is really clean and simple >>>>>> too. >>>>>> >>>>>> That said, I think we could also add something like >>>>>> bus_create_group(). Then we could use that to also clean up >>>>>> drivers/pci/slot.c:pci_slot_init() and get the original path >>>>>> /sys/bus/pci/zpci/uid_checking. >>>>> >>>>> I don't think "uid_checking" is quite the right name. It says >>>>> something about the *implementation*, but it doesn't convey what that >>>>> *means* to userspace. IIUC this file tells userspace something about >>>>> whether a given PCI device always has the same PCI domain/bus/dev/fn >>>>> address (or maybe just the same domain?) >>>>> >>>>> It sounds like this feature could be useful beyond just s390, and >>>>> other arches might implement it differently, without the UID concept. >>>>> If so, I'm OK with something at the /sys/bus/pci/xxx level as long as >>>>> the name is not s390-specific (and "uid" sounds s390-specific). >>>>> >>>>> I assume it would also help with the udev/systemd end if you could >>>>> make this less s390 dependent. >>>> >>>> I've thought about this more and even implemented a proof of concept >>>> patch for a global attribute using a pcibios_has_reproducible_addressing() >>>> hook. >>>> >>>> However after implementing it I think as a more general and >>>> future proof concept it makes more sense to do this as a per device >>>> attribute, maybe as another flag in "stuct pci_dev" named something >>>> like "reliable_address". My reasoning behind this can be best be seen >>>> with a QEMU example. While I expect that QEMU can easily guarantee >>>> that one can always use "0000:01:00.0" for a virtio-pci NIC and >>>> thus enp1s0 interface name, the same might be harder to guarantee >>>> for a SR-IOV VF passed through with vfio-pci in that same VM and >>>> even less so if a thunderbolt controller is passed through and >>>> enumeration may depend on daisy chaining. The QEMU example >>>> also applies to s390 and maybe others will in the future. >>> >>> I'm a little wary of using the PCI geographical address >>> ("0000:01:00.0") as a stable name. Even if you can make a way to use >>> that to identify a specific device instance, regardless of how it is >>> plugged in or passed through, it sounds like we could end up with >>> "physical PCI addresses" and "virtual PCI addresses" that look the >>> same and would cause confusion. >>> >>> This concept sounds similar to the udev concept of a "persistent >>> device name". What advantages does this s390 UID have over the udev >>> approach? >>> >>> There are optional PCI device serial numbers that we currently don't >>> really make use of. Would that be a generic way to help with this? >>> >> >> As far as I understand systemd/udev uses the PCI geographical address >> by default ("enP<domain>p<bus>s<hotplug_slot_idx>...") for PCI attached >> network interfaces in many cases and a lot of users have already built >> their firewall/routing rules on these. > > Which is fine as "normally" that does not change. But on some machines, > it is quite volatile so users pick a different naming scheme. > > And this is all done in userspace, I really don't understand what you > want to do in the kernel here. If you want to expose another unique > thing that the hardware knows about, wonderful, userspace can then use > that if it wants to in how it names specific devices. But don't put > that naming in the kernel, that's not where it belongs. Oh no I definitely don't want to put any naming in the kernel. Rather this is very much "a thing that the hardware knows". The thing being: We're a virtual platform and this PCI devices' address is generated from user configuration and we guarantee it's stable. > >> Now taking this beyond s390 my idea is that under some circumstances >> just as with UID Uniqueness for us, the platform can tell if a PCI >> geographical address is a reliable identifier thus sytemd/udev >> has more information about the quality of existing naming schemes >> incorporating information from the geographical address. > > The platform does not "know" if this is reliable or not, sorry. That's > not how PCI or UEFI works. Yes I assumed as much which is why my examples were all about virtualized platforms/hypervisors. I would say QEMU very much knows if the PCI address is coming from its fluffy virtual sandbox or real wild hardware (pass-through). > >> Looking at my personal KVM guests (Ubuntu, Arch Linux, Ubuntu ARM64) >> as well as my workstation (Arch Linux) all of them use a scheme >> with parts of the geographical address. > > Because for the most part, yes, this works. Until you plug another > device into the system. Or remove one. Or plug a hotplug device in and > then cold boot with it plugged in (or removed). Or, my favorite system, > just decide to renumber the PCI bus every other boot "just because". > > None of that variability can be known by the kernel, that's only known by > the user of that system, so again, they can make the best decision as to > how to name their devices. If you want to use the systemd default, > wonderful, but know that it does not work for everyone, so systemd > allows you to do whatever you want. > >> So in essence my idea is all about either choosing the best existing >> default name or making sure we at least know if it may not be reliable. > > There is no reliability "score" here, sorry. Hardware is fun :) Well as said above, at the very least there is real hardware, emulated hardware and our always virtualizing machines that keep things cozy and reliable enough that you can keep running OSs from the 70s largely unchanged on >5 GHz CPUs. So I think at least our platform has a pretty good track record in this regard. > > good luck! > > greg k-h > ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2021-01-21 17:45 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-11 9:38 [RFC 0/1] PCI: s390 global attribute "UID Checking" Niklas Schnelle 2021-01-11 9:38 ` [RFC 1/1] s390/pci: expose UID checking state in sysfs Niklas Schnelle 2021-01-12 21:50 ` Bjorn Helgaas 2021-01-13 7:47 ` Niklas Schnelle 2021-01-13 18:55 ` Bjorn Helgaas 2021-01-14 13:20 ` Niklas Schnelle 2021-01-14 13:44 ` Christian Brauner 2021-01-14 13:58 ` Greg Kroah-Hartman 2021-01-14 15:06 ` Niklas Schnelle 2021-01-14 15:17 ` Greg Kroah-Hartman 2021-01-14 15:51 ` Niklas Schnelle 2021-01-14 16:14 ` Greg Kroah-Hartman 2021-01-15 11:20 ` Niklas Schnelle 2021-01-15 12:02 ` Greg Kroah-Hartman 2021-01-15 15:29 ` Bjorn Helgaas 2021-01-15 16:15 ` Niklas Schnelle 2021-01-21 15:31 ` Niklas Schnelle 2021-01-21 15:54 ` Bjorn Helgaas 2021-01-21 16:11 ` Greg Kroah-Hartman 2021-01-21 17:04 ` Niklas Schnelle 2021-01-21 17:28 ` Greg Kroah-Hartman 2021-01-21 17:43 ` Niklas Schnelle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).