Fix bhyve PCIe capability emulation

[chuck]

Summary

PCIe devices starting with version 1.1 must set the Role-Based Error
Reporting bit.

And while we're in the neighborhood, generalize the code assigning the
device type.

[chuck]

added wrong diff

[imp]

usr.sbin/bhyve/pci_emul.c

956 ↗

(On Diff #55051)

this is a nop, but likely a good change none-the-less if you intended to do it.

[araujo]

LGTM!

[chuck]

usr.sbin/bhyve/pci_emul.c

956 ↗

(On Diff #55051)

Yes, I have a WIP patch to add endpoint support which then would use type.

[chuck]

One of @jhb requests was a comment on why the RBER change was needed. Here is the thought process:

qemu in their PCIe emulation (hw/pci/pcie.c) has the comment

/*
 * Windows guests will report Code 10, device cannot start, if
 * a regular Endpoint type is exposed on a root complex.  These
 * should instead be Root Complex Integrated Endpoints.
 */

If true, and Windows is actually validating PCIe capability structures to this degree, then it seemed prudent to set other bits in this area as required by the PCIe spec.

As to where to go from here, I'm conflicted. On the one hand, the change doesn't seem to fix anything from my testing. On the other hand, given this comment in qemu and the recent experience of Windows validating the size of BAR0 in NVMe devices, I'm tempted to go back through the PCIe spec and make sure that bhyve's PCIe capability structure is compliant.

[jhb]

In D19580#421672, @chuck wrote:

One of @jhb requests was a comment on why the RBER change was needed. Here is the thought process:

qemu in their PCIe emulation (hw/pci/pcie.c) has the comment

/*
 * Windows guests will report Code 10, device cannot start, if
 * a regular Endpoint type is exposed on a root complex.  These
 * should instead be Root Complex Integrated Endpoints.
 */

If true, and Windows is actually validating PCIe capability structures to this degree, then it seemed prudent to set other bits in this area as required by the PCIe spec.

As to where to go from here, I'm conflicted. On the one hand, the change doesn't seem to fix anything from my testing. On the other hand, given this comment in qemu and the recent experience of Windows validating the size of BAR0 in NVMe devices, I'm tempted to go back through the PCIe spec and make sure that bhyve's PCIe capability structure is compliant.

Hmm, due to what this bit actually does, I'd probably be inclined to not set it if we don't need it. When the role based error reporting is set, it means we also support at least one other register to configure the mask of fatal vs non-fatal errors IIRC which I'm pretty sure we don't do. I had assumed Windows was failing to boot without this bit set. If Windows is actually fine then I think I'd prefer we drop it until we find a need for it. A PCIe 1.0 device can have this bit clear and still be compliant. The other thing about being a special endpoint when directly hung off the RC would be required even for 1.0 compliance.

[chuck]

In D19580#422125, @jhb wrote:

Hmm, due to what this bit actually does, I'd probably be inclined to not set it if we don't need it. When the role based error reporting is set, it means we also support at least one other register to configure the mask of fatal vs non-fatal errors IIRC which I'm pretty sure we don't do. I had assumed Windows was failing to boot without this bit set. If Windows is actually fine then I think I'd prefer we drop it until we find a need for it. A PCIe 1.0 device can have this bit clear and still be compliant. The other thing about being a special endpoint when directly hung off the RC would be required even for 1.0 compliance.

I'm happy to revert this change but wanted to double check the "PCIe 1.0" comment. Even though the code advertises Gen1 speeds, the PCIe Capability Version field is set to 0x2 which I don't think a 1.0 device would do. Should this be changed to 0x1?

[jhb]

In D19580#422248, @chuck wrote:

In D19580#422125, @jhb wrote:

Hmm, due to what this bit actually does, I'd probably be inclined to not set it if we don't need it. When the role based error reporting is set, it means we also support at least one other register to configure the mask of fatal vs non-fatal errors IIRC which I'm pretty sure we don't do. I had assumed Windows was failing to boot without this bit set. If Windows is actually fine then I think I'd prefer we drop it until we find a need for it. A PCIe 1.0 device can have this bit clear and still be compliant. The other thing about being a special endpoint when directly hung off the RC would be required even for 1.0 compliance.

I'm happy to revert this change but wanted to double check the "PCIe 1.0" comment. Even though the code advertises Gen1 speeds, the PCIe Capability Version field is set to 0x2 which I don't think a 1.0 device would do. Should this be changed to 0x1?

Hmm, that's a good point. We probably need to document very clearly which specific PCIe version the pseudo devices are corresponding to and then follow that version of the spec. I'll have to look at it more to see what version we claim to be.