Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pci collection should always include the domain #1693

Closed
Babar opened this issue Sep 2, 2021 · 0 comments
Closed

pci collection should always include the domain #1693

Babar opened this issue Sep 2, 2021 · 0 comments
Assignees
@jaymzh

Comments

@Babar
Copy link
Contributor

Babar commented Sep 2, 2021

PCI bus addresses as collected by the PCI module do not always contain the domain

On Linux, we run lspci -vnnmk, which is missing -D.
Quoting the man page:
-D Always show PCI domain numbers. By default, lspci suppresses them on machines which have only domain 0.

Why is that important, one might wonder? Most Intel CPUs only have one domain, therefore we've all only ever seen 0000 as the domain, and ended up writing code like this:

  node['pci'].each do |bus_id, values|
    path = Pathname("/sys/bus/pci/devices/0000:#{bus_id}")
    ...
  end

This is bad as it hardcodes the domain, when it shouldn't have to.

Software Version

This is on the current version of ohai (17.5.2), on any Linux.

Replication Case

$ ohai pci | jq -c keys
["00:00.0","00:01.0","00:02.0","00:03.0", ...]

Possible Solution

Add a -D here:
https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L48

The issue is that if we then use it as a key, it will change the bus id to add the domain to it, which means it might break existing code (like the one I put as an example).
We could add the domain as a field, but then we might have duplicates on hosts which have more than one domain. To be fair, that's the current behavior, so this might work?
The safest way is probably to copy the code to a pci2 module which uses -D and have people slowly migrate over.
@jaymzh jaymzh self-assigned this Feb 15, 2022
@Babar Babar mentioned this issue Apr 21, 2024
Merged
11 tasks
Babar added a commit to Babar/ohai that referenced this issue Apr 22, 2024
@Babar
[lspci] Fix handling of more than one PCIe root port
639d84a 
Summary:
I'm guessing most machines only have one PCIe root port, but things like ARM hosts tend to have more, and that breaks as it creates collisions.

As we want to keep backwards compatibility, and `lspci` is run with the defaults that doesn't show the root port if it's `0000`, only add it when it's non-zero, and also add it as a new extra field called `root_port` to denote the fact that there is one (useful when using the BDF to find files on disk, as when there is no `root_port`, one needs to add the extra `0000:`.

More detailed explanation I wrote on the slack channel:
On Linux, we run `lspci -vnnmk`, which will show the BDF in the first `Device:` field. So the first bug is that the regular expression on https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L54 is missing a `\` before the `.`, therefore will will match `0001:02:03.4` as `01:02:03` which is plain broken. But fixing that we end up with `02:03.4` which is also plain wrong as it loses the root complex.
I see a few ways we could fix this:

1. Highly disruptive: we add -D to lspci, which means we'll always get the root complex even when it's 0000 . This is terrible 'cause it will basically break everything
2. We do this but only with an option in ohai. Less terrible, but means people have to know about that option
3. We do this but only when we notice more than one root complex. That's not really doable
4. We add a new field, root_complex in the PCI structure, which is 0000 by default and the actual root complex for everything else

I like 4 the most, as it mirrors closely what lspci does (https://github.com/pciutils/pciutils/blob/master/lspci.c#L285)
or maybe 5. We do it the same way lspci does it: if the root complex is 0, we just store the BDF as the ID, if it's not, we store the root complex + BDF
we can't really do 4 'cause we might have collisions... Like if you have 0000:01:02.3 and 0001:01:02.3 then they have to be stored differently. So we can either do:
Option A: 01:02.3 and 0001:01:02.3 (this can co-exist with everything today)
Option B: 00000:1:02.3 and 0001:01:02.3 (this most likely needs to become pci2)

Test Plan:
Before:
```
$ ohai | jq .pci\|keys
[
  "00:00.0",
  "01:00.0",
  "01:00.1",
  "02:00:0",
  "04:00:0",
  "08:00:0",
  "08:01:0",
  "08:02:0",
  "08:03:0",
  "08:04:0",
  "08:05:0",
  "09:00:0",
  "09:01:0"
]
```

After:
```
$ ohai | jq .pci\|keys
[
  "0002:00:00.0",
  "0004:00:00.0",
  "0008:00:00.0",
  "0008:01:00.0",
  "0008:02:01.0",
  "0008:02:02.0",
  "0008:03:00.0",
  "0008:04:00.0",
  "0008:05:00.0",
  "0009:00:00.0",
  "0009:01:00.0",
  "00:00.0",
  "01:00.0",
  "01:00.1"
]
```

Reviewers: jaymzh

Closes: chef#1693
Babar added a commit to Babar/ohai that referenced this issue Apr 22, 2024
@Babar
[lspci] Fix handling of more than one PCIe root port
be979bc 
Summary:
I'm guessing most machines only have one PCIe root port, but things like ARM hosts tend to have more, and that breaks as it creates collisions.

As we want to keep backwards compatibility, and `lspci` is run with the defaults that doesn't show the root port if it's `0000`, only add it when it's non-zero, and also add it as a new extra field called `root_port` to denote the fact that there is one (useful when using the BDF to find files on disk, as when there is no `root_port`, one needs to add the extra `0000:`.

More detailed explanation I wrote on the slack channel:
On Linux, we run `lspci -vnnmk`, which will show the BDF in the first `Device:` field. So the first bug is that the regular expression on https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L54 is missing a `\` before the `.`, therefore will will match `0001:02:03.4` as `01:02:03` which is plain broken. But fixing that we end up with `02:03.4` which is also plain wrong as it loses the root complex.
I see a few ways we could fix this:

1. Highly disruptive: we add -D to lspci, which means we'll always get the root complex even when it's 0000 . This is terrible 'cause it will basically break everything
2. We do this but only with an option in ohai. Less terrible, but means people have to know about that option
3. We do this but only when we notice more than one root complex. That's not really doable
4. We add a new field, root_complex in the PCI structure, which is 0000 by default and the actual root complex for everything else

I like 4 the most, as it mirrors closely what lspci does (https://github.com/pciutils/pciutils/blob/master/lspci.c#L285)
or maybe 5. We do it the same way lspci does it: if the root complex is 0, we just store the BDF as the ID, if it's not, we store the root complex + BDF
we can't really do 4 'cause we might have collisions... Like if you have 0000:01:02.3 and 0001:01:02.3 then they have to be stored differently. So we can either do:
Option A: 01:02.3 and 0001:01:02.3 (this can co-exist with everything today)
Option B: 00000:1:02.3 and 0001:01:02.3 (this most likely needs to become pci2)

Test Plan:
Before:
```
$ ohai | jq .pci\|keys
[
  "00:00.0",
  "01:00.0",
  "01:00.1",
  "02:00:0",
  "04:00:0",
  "08:00:0",
  "08:01:0",
  "08:02:0",
  "08:03:0",
  "08:04:0",
  "08:05:0",
  "09:00:0",
  "09:01:0"
]
```

After:
```
$ ohai | jq .pci\|keys
[
  "0002:00:00.0",
  "0004:00:00.0",
  "0008:00:00.0",
  "0008:01:00.0",
  "0008:02:01.0",
  "0008:02:02.0",
  "0008:03:00.0",
  "0008:04:00.0",
  "0008:05:00.0",
  "0009:00:00.0",
  "0009:01:00.0",
  "00:00.0",
  "01:00.0",
  "01:00.1"
]
```

Reviewers: jaymzh

Closes: chef#1693
Babar added a commit to Babar/ohai that referenced this issue Apr 22, 2024
@Babar
[lspci] Fix handling of more than one PCIe root port
656eb32 
Summary:
I'm guessing most machines only have one PCIe root port, but things like ARM hosts tend to have more, and that breaks as it creates collisions.

As we want to keep backwards compatibility, and `lspci` is run with the defaults that doesn't show the root port if it's `0000`, only add it when it's non-zero, and also add it as a new extra field called `root_port` to denote the fact that there is one (useful when using the BDF to find files on disk, as when there is no `root_port`, one needs to add the extra `0000:`.

More detailed explanation I wrote on the slack channel:
On Linux, we run `lspci -vnnmk`, which will show the BDF in the first `Device:` field. So the first bug is that the regular expression on https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L54 is missing a `\` before the `.`, therefore will will match `0001:02:03.4` as `01:02:03` which is plain broken. But fixing that we end up with `02:03.4` which is also plain wrong as it loses the root complex.
I see a few ways we could fix this:

1. Highly disruptive: we add -D to lspci, which means we'll always get the root complex even when it's 0000 . This is terrible 'cause it will basically break everything
2. We do this but only with an option in ohai. Less terrible, but means people have to know about that option
3. We do this but only when we notice more than one root complex. That's not really doable
4. We add a new field, root_complex in the PCI structure, which is 0000 by default and the actual root complex for everything else

I like 4 the most, as it mirrors closely what lspci does (https://github.com/pciutils/pciutils/blob/master/lspci.c#L285)
or maybe 5. We do it the same way lspci does it: if the root complex is 0, we just store the BDF as the ID, if it's not, we store the root complex + BDF
we can't really do 4 'cause we might have collisions... Like if you have 0000:01:02.3 and 0001:01:02.3 then they have to be stored differently. So we can either do:
Option A: 01:02.3 and 0001:01:02.3 (this can co-exist with everything today)
Option B: 00000:1:02.3 and 0001:01:02.3 (this most likely needs to become pci2)

Test Plan:
Before:
```
$ ohai | jq .pci\|keys
[
  "00:00.0",
  "01:00.0",
  "01:00.1",
  "02:00:0",
  "04:00:0",
  "08:00:0",
  "08:01:0",
  "08:02:0",
  "08:03:0",
  "08:04:0",
  "08:05:0",
  "09:00:0",
  "09:01:0"
]
```

After:
```
$ ohai | jq .pci\|keys
[
  "0002:00:00.0",
  "0004:00:00.0",
  "0008:00:00.0",
  "0008:01:00.0",
  "0008:02:01.0",
  "0008:02:02.0",
  "0008:03:00.0",
  "0008:04:00.0",
  "0008:05:00.0",
  "0009:00:00.0",
  "0009:01:00.0",
  "00:00.0",
  "01:00.0",
  "01:00.1"
]
```

Reviewers: jaymzh

Closes: chef#1693
Signed-off-by: Olivier Raginel <babar@meta.com>
Babar added a commit to Babar/ohai that referenced this issue Apr 26, 2024
@Babar
[lspci] Fix handling of more than one PCIe root port
8d49639 
Summary:
I'm guessing most machines only have one PCIe root port, but things like ARM hosts tend to have more, and that breaks as it creates collisions.

As we want to keep backwards compatibility, and `lspci` is run with the defaults that doesn't show the root port if it's `0000`, only add it when it's non-zero, and also add it as a new extra field called `root_port` to denote the fact that there is one (useful when using the BDF to find files on disk, as when there is no `root_port`, one needs to add the extra `0000:`.

More detailed explanation I wrote on the slack channel:
On Linux, we run `lspci -vnnmk`, which will show the BDF in the first `Device:` field. So the first bug is that the regular expression on https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L54 is missing a `\` before the `.`, therefore will will match `0001:02:03.4` as `01:02:03` which is plain broken. But fixing that we end up with `02:03.4` which is also plain wrong as it loses the root complex.
I see a few ways we could fix this:

1. Highly disruptive: we add -D to lspci, which means we'll always get the root complex even when it's 0000 . This is terrible 'cause it will basically break everything
2. We do this but only with an option in ohai. Less terrible, but means people have to know about that option
3. We do this but only when we notice more than one root complex. That's not really doable
4. We add a new field, root_complex in the PCI structure, which is 0000 by default and the actual root complex for everything else

I like 4 the most, as it mirrors closely what lspci does (https://github.com/pciutils/pciutils/blob/master/lspci.c#L285)
or maybe 5. We do it the same way lspci does it: if the root complex is 0, we just store the BDF as the ID, if it's not, we store the root complex + BDF
we can't really do 4 'cause we might have collisions... Like if you have 0000:01:02.3 and 0001:01:02.3 then they have to be stored differently. So we can either do:
Option A: 01:02.3 and 0001:01:02.3 (this can co-exist with everything today)
Option B: 00000:1:02.3 and 0001:01:02.3 (this most likely needs to become pci2)

Test Plan:
Before:
```
$ ohai | jq .pci\|keys
[
  "00:00.0",
  "01:00.0",
  "01:00.1",
  "02:00:0",
  "04:00:0",
  "08:00:0",
  "08:01:0",
  "08:02:0",
  "08:03:0",
  "08:04:0",
  "08:05:0",
  "09:00:0",
  "09:01:0"
]
```

After:
```
$ ohai | jq .pci\|keys
[
  "0002:00:00.0",
  "0004:00:00.0",
  "0008:00:00.0",
  "0008:01:00.0",
  "0008:02:01.0",
  "0008:02:02.0",
  "0008:03:00.0",
  "0008:04:00.0",
  "0008:05:00.0",
  "0009:00:00.0",
  "0009:01:00.0",
  "00:00.0",
  "01:00.0",
  "01:00.1"
]
```

Reviewers: jaymzh

Closes: chef#1693
Signed-off-by: Olivier Raginel <babar@meta.com>
Babar added a commit to Babar/ohai that referenced this issue Apr 26, 2024
@Babar
[lspci] Fix handling of more than one PCIe root port
e6a4242 
Summary:
I'm guessing most machines only have one PCIe root port, but things like ARM hosts tend to have more, and that breaks as it creates collisions.

As we want to keep backwards compatibility, and `lspci` is run with the defaults that doesn't show the root port if it's `0000`, only add it when it's non-zero, and also add it as a new extra field called `root_port` to denote the fact that there is one (useful when using the BDF to find files on disk, as when there is no `root_port`, one needs to add the extra `0000:`.

More detailed explanation I wrote on the slack channel:
On Linux, we run `lspci -vnnmk`, which will show the BDF in the first `Device:` field. So the first bug is that the regular expression on https://github.com/chef/ohai/blob/main/lib/ohai/plugins/linux/lspci.rb#L54 is missing a `\` before the `.`, therefore will will match `0001:02:03.4` as `01:02:03` which is plain broken. But fixing that we end up with `02:03.4` which is also plain wrong as it loses the root complex.
I see a few ways we could fix this:

1. Highly disruptive: we add -D to lspci, which means we'll always get the root complex even when it's 0000 . This is terrible 'cause it will basically break everything
2. We do this but only with an option in ohai. Less terrible, but means people have to know about that option
3. We do this but only when we notice more than one root complex. That's not really doable
4. We add a new field, root_complex in the PCI structure, which is 0000 by default and the actual root complex for everything else

I like 4 the most, as it mirrors closely what lspci does (https://github.com/pciutils/pciutils/blob/master/lspci.c#L285)
or maybe 5. We do it the same way lspci does it: if the root complex is 0, we just store the BDF as the ID, if it's not, we store the root complex + BDF
we can't really do 4 'cause we might have collisions... Like if you have 0000:01:02.3 and 0001:01:02.3 then they have to be stored differently. So we can either do:
Option A: 01:02.3 and 0001:01:02.3 (this can co-exist with everything today)
Option B: 00000:1:02.3 and 0001:01:02.3 (this most likely needs to become pci2)

Test Plan:
Before:
```
$ ohai | jq .pci\|keys
[
  "00:00.0",
  "01:00.0",
  "01:00.1",
  "02:00:0",
  "04:00:0",
  "08:00:0",
  "08:01:0",
  "08:02:0",
  "08:03:0",
  "08:04:0",
  "08:05:0",
  "09:00:0",
  "09:01:0"
]
```

After:
```
$ ohai | jq .pci\|keys
[
  "0002:00:00.0",
  "0004:00:00.0",
  "0008:00:00.0",
  "0008:01:00.0",
  "0008:02:01.0",
  "0008:02:02.0",
  "0008:03:00.0",
  "0008:04:00.0",
  "0008:05:00.0",
  "0009:00:00.0",
  "0009:01:00.0",
  "00:00.0",
  "01:00.0",
  "01:00.1"
]
```

Reviewers: jaymzh

Closes: chef#1693
Signed-off-by: Olivier Raginel <babar@meta.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jaymzh jaymzh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Babar @jaymzh