spec inconsistency: Device Configuration Interrupt bit in ISR status

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

Quoted 7 lines · Michael S. Tsirkin

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

I agree that it would make sense to be more explicit, especially if people have already fallen into this trap.

My main question is: do we still have time to do this for 1.2? By the proposed timeline, we need to have a vote open by Jan 17th (next Monday), which is cutting it awfully close.

[We seem to have quite some last minute stuff, sigh.]

The spec says (v1.1 4.1.4.5 ISR status capability):

The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR status field to be used for INT#x interrupt handling.

and

to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the interrupt.
In this way, driver read of ISR status causes the device to de­assert an interrupt.

See sections 4.1.5.3 and 4.1.5.4 for how this is used.

and in 4.1.5.4 Notification of Device Configuration Changes

it says:

• If MSI­X capability is disabled:
1. Set the second lower bit of the ISR Status field for the device.
2. Send the appropriate PCI interrupt for the device.

If MSI­X capability is enabled:
1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for the device, config_msix_vector sets the MSI­X Table entry number.

all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is unused with MSIX.

This was actually the way the spec was understood by Zhu Lingshan from Intel (Cc'd).

However, looking at the conformance statements, one finds out this is not the case:

4.1.4.5.1 Device Requirements: ISR status capability

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­ ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a virtqueue notification to the driver.

which implies that the Device Configuration Interrupt bit is set unconditionally.

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

Quoted 47 lines · Michael S. Tsirkin

The spec says (v1.1 4.1.4.5 ISR status capability):

The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR status field to be used for INT#x interrupt handling.

and

to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the interrupt.
In this way, driver read of ISR status causes the device to de­assert an interrupt.

See sections 4.1.5.3 and 4.1.5.4 for how this is used.

and in 4.1.5.4 Notification of Device Configuration Changes

it says:

• If MSI­X capability is disabled:
1. Set the second lower bit of the ISR Status field for the device.
2. Send the appropriate PCI interrupt for the device.

If MSI­X capability is enabled:
1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for the device, config_msix_vector sets the MSI­X Table entry number.

all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is unused with MSIX.

This was actually the way the spec was understood by Zhu Lingshan from Intel (Cc'd).

However, looking at the conformance statements, one finds out this is not the case:

4.1.4.5.1 Device Requirements: ISR status capability

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­ ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a virtqueue notification to the driver.

which implies that the Device Configuration Interrupt bit is set unconditionally.

Quoted 7 lines

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

I wonder do we want

1) mandate ISR bit

or

2) remove the ISR bit set for MSI mode?

1) is the current Qemu behavior but seems a little bit contradict with the goal of MSI.

Thanks

Quoted 66 lines · Jason Wang

Quoted 47 lines · Michael S. Tsirkin

The spec says (v1.1 4.1.4.5 ISR status capability):

The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR status field to be used for INT#x interrupt handling.

and

to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the interrupt.
In this way, driver read of ISR status causes the device to de­assert an interrupt.

See sections 4.1.5.3 and 4.1.5.4 for how this is used.

and in 4.1.5.4 Notification of Device Configuration Changes

it says:

• If MSI­X capability is disabled:
1. Set the second lower bit of the ISR Status field for the device.
2. Send the appropriate PCI interrupt for the device.

If MSI­X capability is enabled:
1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for the device, config_msix_vector sets the MSI­X Table entry number.

all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is unused with MSIX.

This was actually the way the spec was understood by Zhu Lingshan from Intel (Cc'd).

However, looking at the conformance statements, one finds out this is not the case:

4.1.4.5.1 Device Requirements: ISR status capability

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­ ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a virtqueue notification to the driver.

which implies that the Device Configuration Interrupt bit is set unconditionally.

Quoted 7 lines

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

I wonder do we want

1) mandate ISR bit

or

2) remove the ISR bit set for MSI mode?

Quoted 6 lines

1) is the current Qemu behavior but seems a little bit contradict with the goal of MSI.

Thanks

I think we want the ISR bit

since otherwise we need to go read
a ton of config space fields to check whether anything changed.

--
MST

Quoted 54 lines · Jason Wang

Quoted 53 lines · Michael S. Tsirkin

Quoted 52 lines · Jason Wang

Quoted 51 lines · Michael S. Tsirkin

The spec says (v1.1 4.1.4.5 ISR status capability):

The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR status field to be used for INT#x interrupt handling.

and

to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the interrupt.
In this way, driver read of ISR status causes the device to de­assert an interrupt.

See sections 4.1.5.3 and 4.1.5.4 for how this is used.

and in 4.1.5.4 Notification of Device Configuration Changes

it says:

• If MSI­X capability is disabled:
1. Set the second lower bit of the ISR Status field for the device.
2. Send the appropriate PCI interrupt for the device.

If MSI­X capability is enabled:
1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for the device, config_msix_vector sets the MSI­X Table entry number.

all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is unused with MSIX.

This was actually the way the spec was understood by Zhu Lingshan from Intel (Cc'd).

However, looking at the conformance statements, one finds out this is not the case:

4.1.4.5.1 Device Requirements: ISR status capability

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­ ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a virtqueue notification to the driver.

which implies that the Device Configuration Interrupt bit is set unconditionally.

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

I agree to expand ISR cap usage to MSI(MSIX) usage with clear descriptions,
and remove the limitations to MSIX. E.g,:
4.1.4.5.2 Driver Requirements: ISR status capability
If MSI-X capability is enabled, the driver SHOULD NOT access ISR status upon detecting a Queue Interrupt.

Thanks

Quoted 26 lines

Quoted 8 lines

Quoted 8 lines

I wonder do we want

1) mandate ISR bit

or

2) remove the ISR bit set for MSI mode?

Quoted 7 lines

Quoted 6 lines

1) is the current Qemu behavior but seems a little bit contradict with the goal of MSI.

Thanks

I think we want the ISR bit

This means there's no chance to use fast irq path but since it's a less frequent operation. It should be fine.

Thanks

Quoted 6 lines

since otherwise we need to go read
a ton of config space fields to check whether anything changed.

--
MST

On 1/18/2022 11:01 AM, Jason Wang wrote:

On Mon, Jan 17, 2022 at 3:57 PM Michael S. Tsirkin <[email protected]> wrote:

On Mon, Jan 17, 2022 at 02:15:55PM +0800, Jason Wang wrote:

在 2022/1/14 下午9:39, Michael S. Tsirkin 写道:

The spec says (v1.1 4.1.4.5 ISR status capability):

The VIRTIO_PCI_CAP_ISR_CFG capability refers to at least a single byte, which contains the 8­bit ISR status field to be used for INT#x interrupt handling.

and

to avoid an extra access, simply reading this register resets it to 0 and causes the device to de­assert the interrupt.
In this way, driver read of ISR status causes the device to de­assert an interrupt.

See sections 4.1.5.3 and 4.1.5.4 for how this is used.

and in 4.1.5.4 Notification of Device Configuration Changes

it says:

• If MSI­X capability is disabled:
1. Set the second lower bit of the ISR Status field for the device.
2. Send the appropriate PCI interrupt for the device.

If MSI­X capability is enabled:
1. If config_msix_vector is not NO_VECTOR, request the appropriate MSI­X interrupt message for the device, config_msix_vector sets the MSI­X Table entry number.

all of the above make it looks like VIRTIO_PCI_CAP_ISR_CFG capability is unused with MSIX.

This was actually the way the spec was understood by Zhu Lingshan from Intel (Cc'd).

However, looking at the conformance statements, one finds out this is not the case:

4.1.4.5.1 Device Requirements: ISR status capability

The device MUST present at least one VIRTIO_PCI_CAP_ISR_CFG capability.
The device MUST set the Device Configuration Interrupt bit in ISR status before sending a device configu­ ration change notification to the driver.
If MSI­X capability is disabled, the device MUST set the Queue Interrupt bit in ISR status before sending a virtqueue notification to the driver.

which implies that the Device Configuration Interrupt bit is set unconditionally.

It is unfortunate that it does not copy this requirement in more places, and that the non-conformance text is incomplete and does not mention the MSI-X usage at all.

I propose to extend 4.1.4.5 ISR status capability and 4.1.5.4 Notification of Device Configuration Changes to mention the MSI use.

I agree to expand ISR cap usage to MSI(MSIX) usage with clear descriptions, and remove the limitations to MSIX. E.g,:
4.1.4.5.2 Driver Requirements: ISR status capability
If MSI-X capability is enabled, the driver SHOULD NOT access ISR status upon detecting a Queue Interrupt.

Thanks

I wonder do we want

1) mandate ISR bit

or

2) remove the ISR bit set for MSI mode?

1) is the current Qemu behavior but seems a little bit contradict with the goal of MSI.

Thanks

I think we want the ISR bit

This means there's no chance to use fast irq path but since it's a less frequent operation. It should be fine.

Thanks

since otherwise we need to go read
a ton of config space fields to check whether anything changed.

--
MST