fix: write compliant bit pattern for BAR sizing

[cagatay-y]

According to the PCI spec (v6.0, p 930), "[t]o determine how much address space a Function requires, system software should write a value of all 1's to each BAR register and then read the value back." QEMU (and possibly others) mask the provided address based on the size of the address space [2], which is always larger than 128 bytes for memory BARs, so the value of the last nibble has no effect. However, cloud-hypervisor (with possibly others) is more strict in its interpretation of the specification and check for exactly the all-bits-set pattern [3]. On the latter platforms, the current pattern can be erroneously interpreted as a BAR relocation instead of sizing.

[bjorn3]

I don't have a copy of the pci spec (not worth it to pay for one and couldn't find any copies online, just a bunch of wiki pages, linux source code and some presentation slides), but I would have assumed that the writing all 1s to a BAR register would also change the BAR location on real hardware. Just to something outside of the range the parent bridge allows, hence effectively disabling the BAR until you write back the original value. I wouldn't expect there to be a special case for the literal all 1s value that keeps a shadow copy of the BAR value.

[mkroening]

Ah, that makes sense. We should definitely make sure to disable the address spaces before sizing the bar, but I think that is separate from this PR.

This PR should be a strict improvement over the status quo by closer aligning the code with the specification and would still be required regardless of disabling address spaces.

Writing back the value does happen here (unrelated to this PR):

pci_types/src/lib.rs

Line 312 in b35f742

let bar = unsafe { access.read(self.0, offset) };

pci_types/src/lib.rs

Line 319 in b35f742

let address = bar.get_bits(4..32) << 4;

pci_types/src/lib.rs

Line 326 in b35f742

access.write(self.0, offset, address);

[IsaacWoods]

Thanks for the PR.

Looking at the spec (I have the PCI Express spec rev 5.0 v1.0; relevant section is 7.5.1.2.1), this is correct - we should be writing all-ones; I'm not sure why we were manually zeroing the bottom bits before.

However, I think an implementation that misbehaves with 0xfffffff0 vs 0xffffffff is likely incorrect, with bits 0..4 being read-only. I'd imagine these are just hard-coded to the correct values on real devices, along with the bottom bits being hardcoded to zero for the sizing mechanism to work at all. I think a hypervisor relying on different behaviour is probably a bad idea.

The same section does mention disabling address decoding via the function's command register - I've interpreted this as meaning we should be clearing bit 1 of the command register before sizing the BAR and restoring it afterwards. I would be very happy to accept another PR to do so (or I'll try to remember to do so when I next work on this crate).