Program the MSI-X vector control field for MSI-X table entries without loading the previous content of the vector control word.

[khng]

Summary

This fixes the interrupt behavior of controllers which has flakey MMIO
behavior. For instance, SM961/PM961 SSDs are observed that the offsets
starting from 0x3084 within the memory region specified by BAR0 are
all read as 0. The changes are made in accord to the MSI-X
implementation of Illumos kernel.

Two non-exported functions are modified: pci_mask_msix() and
pci_unmask_msix().

A reference to a related bug: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211713

[imp]

Reading through the standard just now, this change looks good, provided we never use the ST Table Location value of 10b (device specific mode, table 6-12 in the 3.0 standard). If we do, then the upper 16 bits are the ST values and need to be preserved.

It's unclear to me if we use them, though (seems no, but that's a quick grep, not a careful study of the code). There's a bit in the TPH Requester Capability Register (section 7.26) that indicates if ST (steering types) is supported. We don't seem to look at that register at all, via its symbolic name anyway. The core PCI code seems safe to make this change. Grep says it's unused in the rest of the tree.

[imp]

Subject to the one comment above, I like it and think it should go in

sys/dev/pci/pci.c

1673 ↗

(On Diff #59502)

/*

• Some devices (eg Samsung PM961) cannot handle RMW on this register for all its vectors.
• write the values we need directly. */

[khng]

Add comments in regards to SM961/PM961

[khng]

Add comments in regards to SM961/PM961.

[jhb]

The newest spec I have (3.0) say this about the upper 31 bits of VCTRL:

After reset, the state of these bits must be 0. However, for potential future use, software must
preserve the value of these reserved bits when modifying the value of other Vector Control bits. If software modifies the value of these reserved bits, the result is undefined.
`

Thus, a RMW seems to be required. The description of the mask bit is also:

When this bit is set, the function is prohibited from sending a message using this MSI-X Table entry.
However, any other MSI-X Table entries programmed with the same vector will still be capable of sending an equivalent message unless they are also masked.
This bit’s state after reset is 1 (entry is masked). This bit is read/write.

As such, it would seem that these devices are just not compliant. However, we could make the W portion unconditional which would satisfy the RMW and still work for these devices I think.

[jhb]

(My 3.0 is older than Warners which is somewhat unfortunate, but I think the requirement for RMW still stands)

[khng]

Update according to @jhb 's suggestions.

[imp]

Ah, the PCI vs PCIe specs.

[jhb]

One thing to consider is moving the new comment up above 'pci_mask_msix' function and just listing it once.

sys/dev/pci/pci.c

1679 ↗

(On Diff #59540)

This needs to write 'val', not the constant.

style(9) wants a blank line before comments.

I would perhaps reword the comment to say something like:

"Some devices (e.g. Samsung PM961) do not support reads of this register, so always write the new value."

1697 ↗

(On Diff #59540)

Similarly here with using 'val' instead of 0 and comment suggestions.

[khng]

Opps careless mistakes.