Page MenuHomeFreeBSD

pci_user: Report NUMA domain
Needs ReviewPublic

Authored by jfree on Mar 10 2024, 3:00 AM.
Tags
None
Referenced Files
F103048401: D44289.id141547.diff
Wed, Nov 20, 6:22 AM
F103041594: D44289.diff
Wed, Nov 20, 4:22 AM
Unknown Object (File)
Wed, Nov 13, 9:42 PM
Unknown Object (File)
Mon, Nov 11, 5:58 PM
Unknown Object (File)
Sun, Oct 27, 9:20 PM
Unknown Object (File)
Sep 26 2024, 11:50 PM
Unknown Object (File)
Sep 21 2024, 8:36 PM
Unknown Object (File)
Sep 20 2024, 8:17 PM
Subscribers

Details

Reviewers
markj
imp
Summary
pci_user: Report NUMA domain

A PCI device's NUMA domain is now accessible via the pd_numa_domain
member of struct pci_conf when using the PCIOCGETCONF ioctl.

A new ioctl number has been assigned to PCIOCGETCONF to preserve
compatibility with binaries compiled on FreeBSD versions 7 through 14.
Such binaries can continue to use the PCIOCGETCONF ioctl number
that they were compiled with and experience no ABI repercussions.

Sponsored by:   NIKSUN, Inc.
Test Plan

Compiled the following program on FreeBSD 13 and FreeBSD 15.

https://reviews.freebsd.org/P644

The FreeBSD 13 version had the pd_numa_domain bit commented out, since it is unavailable.

Ran them both on FreeBSD 15 and diff'd the outputs. There was no difference in output besides numa domain.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 58881
Build 55768: arc lint + arc unit

Event Timeline

jfree requested review of this revision.Mar 10 2024, 3:00 AM

Don't forget to update .Dd when content of manual page changed. :-)

Update manual page .Dd date

imp requested changes to this revision.Mar 17 2024, 10:49 PM

These changes aren't backward compatible. You likely need to keep the old struct to reply to the old ioctl...

And you can't change the current old structure. Old binaries will have buffer overflows

This revision now requires changes to proceed.Mar 17 2024, 10:49 PM

Do not update pci_conf_old and pci_conf_old32 structures. These
should stay static for compatibility with binaries that were
compiled to use the PCIOCGETCONF_OLD and PCIOCGETCONF_OLD32 ioctls.

Thanks for removing this from all the old versions...

However, we still have some compatibility issue. I think we need to rename all the _old names to _freebsd7 to make it comply with more-modern conventions. This can likely be a separate commit.

Then, we need to take the current structures and make _freebsd14 compat versions for them. Since the layout is the same, we'd likely be able to do this by just not setting pd_numa_domain if the IOCTL (which encodes the lenght) is the _freebsd14 version I'd like you to create. It sounds like a lot, but I think most of it is cut and paste and isn't that bad since it's an extra case statement and an extra if for the normal and compat32 branches.

So something like https://reviews.freebsd.org/D45867 for the rename (I can commit this if you'd like)

And something like https://reviews.freebsd.org/D45868 for the compat stuff... It's a little different than I originally thought, so there will need to be a new ioctl to cover this case since the sizes stay the same, we have to change the number so we don't overflow the receiver's conf buffer if it's run against a new kernel. I suspect the new ioctl# and the freebsd14 ioctl parts from that differential belong here, and then you can take mine for compat, make sure it works and then commit it maybe (or if you want me to, let me know)

jfree retitled this revision from PCIOCGETCONF: Report NUMA domain to pci_user: Report NUMA domain.Jul 29 2024, 10:44 PM
jfree edited the summary of this revision. (Show Details)
jfree edited the test plan for this revision. (Show Details)

Address compatibility concerns pointed out by Warner

sys/sys/pciio.h
80

Does it make sense to add some spare fields here, so that you can add additional fields later without needing compat shims?

sys/sys/pciio.h
80

Does it make sense to add some spare fields here, so that you can add additional fields later without needing compat shims?

I was thinking the same thing - this is a lot of compat cruft for a single field. It would be unfortunate if we needed to do it all again for another field in the next major version. I do not know how this should be handled. Maybe some simple char padding would do?

sys/sys/pciio.h
80

For most 'compat' items, the length is just appended and it's just an extra line or two.
The older structures were more of a re-arrangement.
So I'm not sure that the copying element by element we do for the FreeBSD14 section is strictly necessary. That's why one normally doesn't do extra fields... We just selectively copy the first bits for older ioctl numbers.

Also... just realized I likely wrote the field by f8eld copy... let me try again