Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The newly composed MSI message on affinity change is never written to the PCI device because `data->chip->irq_write_msi_msg` is NULL. - [High] VMBus recomposition is permanently skipped when `mapped_vector` is 0, causing the device to use a stale vector. - [High] Multi-MSI interrupts leak hypervisor mappings on every affinity change. --
--- Patch [3]: [PATCH 3/3] PCI: hv: fix interrupt affinity change on the nested root partition --- commit b9428909b0f32d968c5c2cd7f2d85948c6d7893a Author: Wei Liu <[email protected]> PCI: hv: fix interrupt affinity change on the nested root partition This commit updates interrupt handling on affinity changes in the nested root partition. It recomposes the VMBus interrupt with the new vector and programs the device with the resulting message before mapping the new interrupt. > diff --git a/drivers/pci/controller/pci-hyperv.c > b/drivers/pci/controller/pci-hyperv.c > index 5a36382742bf7..5acb8e41567e6 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c [ ... ] > @@ -752,9 +755,17 @@ static void hv_arch_irq_unmask(struct irq_data *data) > * RETARGET_INTERRUPT. > * > * Keep the returned entry so the mapping can be removed again > - * when the interrupt is torn down. > + * when the interrupt is re-targeted or torn down. > + * > + * This is also the re-target point. The core calls us from > + * __irq_move_irq() with the interrupt masked once the new > + * vector has been assigned, so if the vector changed the vmbus > + * interrupt is re-composed for it first -- PCI_CREATE_INTERRUPT > + * carries the vector, so the device would otherwise keep > + * signalling the one it was created with. > */ > struct hv_msi_int_entry *ie = data->chip_data; > + unsigned int vec = hv_msi_get_int_vector(data); > > /* > * A NULL chip_data means hv_compose_msi_msg() failed and the > @@ -763,8 +774,29 @@ static void hv_arch_irq_unmask(struct irq_data *data) > if (!ie) > return; > > - if (hv_map_msi_interrupt(data, &ie->hv_entry)) > + /* Already mapped for this vector, nothing changed. */ > + if (ie->mapped_vector == vec && ie->hv_entry.source) > + return; > + > + if (ie->mapped_vector && ie->mapped_vector != vec) { [Severity: High] If hv_map_msi_interrupt() fails previously, ie->mapped_vector is set to 0. On the next unmask, if the vector has changed due to an affinity change, will this ie->mapped_vector != 0 check skip recomposing the message? It appears this could leave the device programmed with the old vector while the hypervisor is programmed with the new one. > + struct msi_msg msg; > + > + hv_compose_msi_msg(data, &msg); > + > + ie = data->chip_data; > + if (!ie) > + return; > + > + if (data->chip->irq_write_msi_msg) > + data->chip->irq_write_msi_msg(data, &msg); [Severity: High] Does hv_msi_irq_chip actually implement the irq_write_msi_msg callback? Looking at the hv_msi_irq_chip definition, it seems to lack this callback. If so, would this check fail and prevent the new MSI message from ever being written to the device, causing it to continue signalling the old unmapped vector? > + } > + > + if (hv_map_msi_interrupt(data, &ie->hv_entry)) { [Severity: High] For multi-MSI devices, does this overwrite ie->hv_entry and leak hypervisor mappings on every affinity change? When hv_compose_msi_msg() is called for a multi-MSI device, it hits an early return and bypasses the hv_vmbus_unmap_msi_interrupt() cleanup: hv_compose_msi_msg() { ... if (data->chip_data && multi_msi) { ... return; } ... } Then hv_map_msi_interrupt() is called here, which issues a new HVCALL_MAP_DEVICE_INTERRUPT and overwrites ie->hv_entry. Since the previous mapping wasn't unmapped, is it permanently leaked? > memset(&ie->hv_entry, 0, sizeof(ie->hv_entry)); > + ie->mapped_vector = 0; > + return; > + } > + ie->mapped_vector = vec; > } else { > hv_irq_retarget_interrupt(data); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
