Add option -W that allows to write into a BAR region.
Also, add an option -R that allows to read from a BAR region that works similar to -r that reads from the PCI config space.
Details
Details
For example, a VirtIO PCI device
% pciconf -lb pci0:0:10:0
virtio_pci0@pci0:0:10:0: class=0x020000 rev=0x00 hdr=0x00 vendor=0x1af4 device=0x1000 subvendor=0x1af4 subdevice=0x0001
bar [10] = type I/O Port, range 32, base 0, size 32, enabled
bar [18] = type Memory, range 32, base 0xe1852000, size 8192, enabledThe second 4 bytes of BAR 0x18 specifies the device's features based on the first 4 bytes.
% sudo pciconf -D pci0:0:10:0 0x18 0x0 2 | hexdump -C 00000000 01 00 00 00 01 00 00 00 |........|
By writing a 0 in the first 4 bytes the device changes the second 4 bytes.
% sudo pciconf -W pci0:0:10:0 0x18 0 0 % sudo pciconf -D pci0:0:10:0 0x18 0x0 2 | hexdump -C 00000000 00 00 00 00 a3 dd 2f 00 |....../.|
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
I think rather than trying to overload -B, it would be better to just use a different getopt() letter for this mode. I would suggest using -R for reading from a BAR but keep 'D' as an alias for backwards compatibility, and using -W for writing into a BAR.
Comment Actions
You only write a single value into the specified offset, right?
This should be clearly explained in the man page addition.
Or consider adding a possibility of writing the series of values starting at the given offset, of the specified width for each value (IMO it is fine as is, but needs to be properly documented).
Comment Actions
I thought about that too but decided against it because:
- As a user, I probably would find it less clear that a capital letter operates on a BAR region instead of the PCI config space.
- Once I understand that, I would expect that -R (which would do the same as -D) reads from a BAR region as -r reads from the PCI config space. However, these modes a quite different.
After I saw that -b is already overloaded, I decided to use -B.
As a compromise, I could add a -W and leave the rest as it is (no -R). What do you think?
Comment Actions
In my use case, I only had to write a single value. Do you know use cases where a series of values needs to be written to a BAR region?
I tried to make it similar to -w (write to config space). This is the man page part about -w:
| The -w option writes the value into a configuration space register at byte offset addr of device selector. |
Do you find this more clear?
If so, I could try to write the part of -Bw more like that.
If not, we should change that too.
BTW: Do you know what a selector is? It is not a pciconf argument as I would expect since it is underlined.
Comment Actions
Yes
If so, I could try to write the part of -Bw more like that.
If not, we should change that too.
BTW: Do you know what a selector is? It is not a pciconf argument as I would expect since it is underlined.
A selector is the device specification, basically 'pci:b:f:d' for pciconf.
Comment Actions
OK. This is the synopsis:
pciconf -Bw [-b | -h | -x] device bar offset value
and this is the current text:
| The -Bw option writes to the BAR specified by its offset in config space bar. The value is written at byte offset offset in the BAR region. |
How about this?
| The -Bw option writes the value into a BAR region of a PCI device at the given byte offset. It uses the BAR specified by its offset in the config space bar of the PCI device specified by the selector device. |
If so, I could try to write the part of -Bw more like that.
If not, we should change that too.
BTW: Do you know what a selector is? It is not a pciconf argument as I would expect since it is underlined.A selector is the device specification, basically 'pci:b:f:d' for pciconf.
OK, my question was misleading. I mean, why is selector underlined? It is not a pciconf argument. The name of the argument it refers to is device. Shouldn't end the sentence like that:
| ... of device selector device. |
Comment Actions
Ok
If so, I could try to write the part of -Bw more like that.
If not, we should change that too.
BTW: Do you know what a selector is? It is not a pciconf argument as I would expect since it is underlined.A selector is the device specification, basically 'pci:b:f:d' for pciconf.
OK, my question was misleading. I mean, why is selector underlined? It is not a pciconf argument. The name of the argument it refers to is device. Shouldn't end the sentence like that:
... of device selector device.
I think this was some kind of typo. There are several instances of it in the man page. I suggest providing a preparational patch that would fix this first.
Comment Actions
I'm fine with just -W. It's mostly the overloading of -B to set a mode I was not a fan of (where as -b never sets a mode, it is just a modifier).
At this point, one might argue that pciconf should move to a pciconf [global options] <command> [command-specific options> somewhat similar to procstat, etc. but I'm not sure it's worth the work to do that.
Comment Actions
A small manpage fix, the rest looks OK.
| usr.sbin/pciconf/pciconf.8 | ||
|---|---|---|
| 348–350 | s/request/requests/ | |
| usr.sbin/pciconf/pciconf.8 | ||
|---|---|---|
| 348–350 | Thanks! | |
Comment Actions
Thanks for fixing the command line usage as I had asked. In general this looks good. There are some style things most of which are just broken in the existing code. I will try to fix those in the existing code in a separate review so there is a better pattern to copy from in the future.
| usr.sbin/pciconf/pciconf.c | ||
|---|---|---|
| 1388 | There's not really a need to return the fd here. It can be closed after the ioctl() before returning as none of the callers use it. I would also be tempted to just name this function map_bar. | |
| 1427 | Maybe do the memset() in the helper function instead so it isn't duplicated in all of the callers? | |
| 1497 | This cast is redundant. The structure member is already a void *. Looks to be a pre-existing bug however. I would suggest adding a second helper function to do the munmap() (without the cast) that all of the callers can use so that each of the callers becomes something like: foo_bar()
{
structure pci_bar_mmap pbm;
map_bar(..., &pbm);
/* Do work */
unmap_bar(&pbm);
} | |
| 1518 | style(9) prefers explicit comparisons against NULL rather than treating pointers as implicit booleans. I see though that this fix and the one below are just copy/pasted from the readit() functions, so not really your fault. | |
| 1520 | ||
| 1528 | Instead of the union (and the aliasing it entails), just use a uint64_t t and make the width in the printf variable using the same scheme as readone(). uintptr_t addr;
uint64_t t;
addr = ...;
switch (width) {
case 1:
t = *(uint8_t *)addr;
break;
....
}
printf("%0*jx", width * 2, (uintmax_t)t); | |
| 1551 | There are some other style things here as well (spaces around operators in the 16/width, etc.) and treating integer values as booleans instead of explicitly comparing against 0, but those are also all pre-existing bugs in readit(). You can leave these as-is or adopt my suggestions below. I'm going to upload a review to fix readit() to follow my suggestions below. If I were writing this loop (or the one in readit()) from scratch, I would probably define a new local variable for 16/width, something like: int items_per_line = 16 / width; The logic to add a double space only when using a width of 1 is also a bit odd since it can output random extra spaces at the ends of lines for other widths. :( That is an existing bug in readit() though. Probably that should be rewritten as something like: /* Use a double space in the middle when outputting bytes. */
if (width == 1 && (i % 16) == 8)
putchar(' ');
putchar(i % items_per_line == 0 ' ? '\n' : ' '); | |
| 1578 | Ah, this is why you did the memset() out in the callers. I would instead add a flags argument to the helper function. | |
| 1584 | You don't need the casts on value. | |
| usr.sbin/pciconf/pciconf.c | ||
|---|---|---|
| 1518 | Sorry, there shouldn't even be a check against NULL here actually (and none of the other callers of strto* in this file do this except for the busted code in readit()) | |
| usr.sbin/pciconf/pciconf.c | ||
|---|---|---|
| 1497 | Good idea. Thanks! | |
| 1528 |
I don't like the solution with the union either. My intention was to have only one (multi-byte) read operation from the memory location and print the bytes as they are in the memory location. For example, if the memory location contains these four bytes 01 02 03 04 and pciconf reads the first two bytes, it should print 0102. If I store the first two bytes in a uint64_t and print it at once with printf("%0*jx", width * 2, (uintmax_t)t);I get 0201 on a little-endian system. If I print byte by byte from the uint64_t variable, I get 0102 on a little-endian system, but 0000 on a big-endian system. | |
| 1551 |
Did I address all your style comments? | |
Comment Actions
Yes, this looks good to me. I'm about to land my style fixes change and can land this one next. Do you have a git commit I can cherry-pick? Alternatively, is "Timo Völker <timo.voelker_fh@muenster.de>" the correct author identity to use for the commit?
Comment Actions
I don't have a public git commit with these changes.
Alternatively, is "Timo Völker <timo.voelker_fh@muenster.de>" the correct author identity to use for the commit?
Almost, it's "Timo Völker <timo.voelker@fh-muenster.de>".