| Message ID | f16872889aed9a7b042a7e53722b9ae83430a126.1682615447.git.reinette.chatre@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Support dynamic allocation of MSI-X interrupts | expand |
> From: Chatre, Reinette <reinette.chatre@intel.com> > Sent: Friday, April 28, 2023 1:36 AM > > @@ -419,11 +419,9 @@ static int vfio_msi_set_vector_signal(struct > vfio_pci_core_device *vdev, > } > > /* > - * The MSIx vector table resides in device memory which may be > cleared > - * via backdoor resets. We don't allow direct access to the vector > - * table so even if a userspace driver attempts to save/restore > around > - * such a reset it would be unsuccessful. To avoid this, restore the > - * cached value of the message prior to enabling. > + * If the vector was previously allocated, refresh the on-device > + * message data before enabling in case it had been cleared or > + * corrupted since writing. > */ > cmd = vfio_pci_memory_lock_and_enable(vdev); > if (msix) { What about keeping backdoor reset as an example, e.g. "in case it had been cleared or corrupted (e.g. due to backdoor resets) ..." otherwise one may doubt whether it is a more severe problem causing the corruption inadvertently w/o userspace driver's awareness and then whether just restoring the data is sufficient to move on.
Hi Kevin, On 4/27/2023 11:42 PM, Tian, Kevin wrote: >> From: Chatre, Reinette <reinette.chatre@intel.com> >> Sent: Friday, April 28, 2023 1:36 AM >> >> @@ -419,11 +419,9 @@ static int vfio_msi_set_vector_signal(struct >> vfio_pci_core_device *vdev, >> } >> >> /* >> - * The MSIx vector table resides in device memory which may be >> cleared >> - * via backdoor resets. We don't allow direct access to the vector >> - * table so even if a userspace driver attempts to save/restore >> around >> - * such a reset it would be unsuccessful. To avoid this, restore the >> - * cached value of the message prior to enabling. >> + * If the vector was previously allocated, refresh the on-device >> + * message data before enabling in case it had been cleared or >> + * corrupted since writing. >> */ >> cmd = vfio_pci_memory_lock_and_enable(vdev); >> if (msix) { > > What about keeping backdoor reset as an example, e.g. > > "in case it had been cleared or corrupted (e.g. due to backdoor resets) ..." > > otherwise one may doubt whether it is a more severe problem causing > the corruption inadvertently w/o userspace driver's awareness and then > whether just restoring the data is sufficient to move on. Will do. Thank you very much. Reinette
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c index 5e3de004f4cb..bdda7f46c2be 100644 --- a/drivers/vfio/pci/vfio_pci_intrs.c +++ b/drivers/vfio/pci/vfio_pci_intrs.c @@ -419,11 +419,9 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_core_device *vdev, } /* - * The MSIx vector table resides in device memory which may be cleared - * via backdoor resets. We don't allow direct access to the vector - * table so even if a userspace driver attempts to save/restore around - * such a reset it would be unsuccessful. To avoid this, restore the - * cached value of the message prior to enabling. + * If the vector was previously allocated, refresh the on-device + * message data before enabling in case it had been cleared or + * corrupted since writing. */ cmd = vfio_pci_memory_lock_and_enable(vdev); if (msix) {
In preparation for surrounding code change it is helpful to ensure that existing comments are accurate. Remove inaccurate comment about direct access and update the rest of the comment to reflect the purpose of writing the cached MSI message to the device. Suggested-by: Alex Williamson <alex.williamson@redhat.com> Link: https://lore.kernel.org/lkml/20230330164050.0069e2a5.alex.williamson@redhat.com/ Signed-off-by: Reinette Chatre <reinette.chatre@intel.com> --- No changes since V3. Changes since V2: - New patch. drivers/vfio/pci/vfio_pci_intrs.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)