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 <[email protected]>
---
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 <[email protected]>
+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
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 <[email protected]>
> ---
> 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 <[email protected]>
> +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
>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> +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 ...
>>
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
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 ;-)
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
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
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
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
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
>
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
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
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
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
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?
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
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
[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?
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
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
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
>