| Message ID | 20190322052308.28254-1-leo.yan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio-pci: Fix MSI IRQ forwarding for without per-vector masking | expand |
On Fri, Mar 22, 2019 at 01:23:08PM +0800, Leo Yan wrote: > If MSI doesn't support per-vector masking capability and > PCI_MSI_FLAGS_MASKBIT isn't set in message control field, the function > vfio_pci_msi_vector_write() will directly bail out for this case and > every vector's 'virt_state' keeps setting bit VFIO_PCI_MSI_STATE_MASKED. > > This results in the state maintained in 'virt_state' cannot really > reflect the MSI hardware state; finally it will mislead the function > vfio_pci_update_msi_entry() to skip IRQ forwarding with below flow: > > vfio_pci_update_msi_entry() { > > [...] > > if (msi_is_masked(entry->virt_state) == msi_is_masked(entry->phys_state)) > return 0; ==> skip IRQ forwarding > > [...] > } > > To fix this issue, when detect PCI_MSI_FLAGS_MASKBIT is not set in the > message control field, this patch simply clears bit > VFIO_PCI_MSI_STATE_MASKED for all vectors 'virt_state'; at the end > vfio_pci_update_msi_entry() can forward MSI IRQ successfully. Just remind, this patch is for kvmtool but not for kernel. Sorry I forget to add it in subject. Thanks, Leo Yan > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > vfio/pci.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/vfio/pci.c b/vfio/pci.c > index ba971eb..4fd24ac 100644 > --- a/vfio/pci.c > +++ b/vfio/pci.c > @@ -363,8 +363,18 @@ static int vfio_pci_msi_vector_write(struct kvm *kvm, struct vfio_device *vdev, > struct vfio_pci_device *pdev = &vdev->pci; > struct msi_cap_64 *msi_cap_64 = PCI_CAP(&pdev->hdr, pdev->msi.pos); > > - if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) > + if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) { > + /* > + * If MSI doesn't support per-vector masking capability, > + * simply unmask for all vectors. > + */ > + for (i = 0; i < pdev->msi.nr_entries; i++) { > + entry = &pdev->msi.entries[i]; > + msi_set_masked(entry->virt_state, false); > + } > + > return 0; > + } > > if (msi_cap_64->ctrl & PCI_MSI_FLAGS_64BIT) > mask_pos = PCI_MSI_MASK_64; > -- > 2.19.1 >
Hi Jean-Philippe, On Fri, Mar 22, 2019 at 01:29:24PM +0800, Leo Yan wrote: > On Fri, Mar 22, 2019 at 01:23:08PM +0800, Leo Yan wrote: > > If MSI doesn't support per-vector masking capability and > > PCI_MSI_FLAGS_MASKBIT isn't set in message control field, the function > > vfio_pci_msi_vector_write() will directly bail out for this case and > > every vector's 'virt_state' keeps setting bit VFIO_PCI_MSI_STATE_MASKED. > > > > This results in the state maintained in 'virt_state' cannot really > > reflect the MSI hardware state; finally it will mislead the function > > vfio_pci_update_msi_entry() to skip IRQ forwarding with below flow: > > > > vfio_pci_update_msi_entry() { > > > > [...] > > > > if (msi_is_masked(entry->virt_state) == msi_is_masked(entry->phys_state)) > > return 0; ==> skip IRQ forwarding > > > > [...] > > } > > > > To fix this issue, when detect PCI_MSI_FLAGS_MASKBIT is not set in the > > message control field, this patch simply clears bit > > VFIO_PCI_MSI_STATE_MASKED for all vectors 'virt_state'; at the end > > vfio_pci_update_msi_entry() can forward MSI IRQ successfully. > > Just remind, this patch is for kvmtool but not for kernel. Sorry I > forget to add it in subject. Gentle ping. Not sure if this patch is reasonable, could you help to give a review? [...] Thanks, Leo Yan
Hi Leo, On 22/03/2019 05:23, Leo Yan wrote: > If MSI doesn't support per-vector masking capability and > PCI_MSI_FLAGS_MASKBIT isn't set in message control field, the function > vfio_pci_msi_vector_write() will directly bail out for this case and > every vector's 'virt_state' keeps setting bit VFIO_PCI_MSI_STATE_MASKED. > > This results in the state maintained in 'virt_state' cannot really > reflect the MSI hardware state; finally it will mislead the function > vfio_pci_update_msi_entry() to skip IRQ forwarding with below flow: > > vfio_pci_update_msi_entry() { > > [...] > > if (msi_is_masked(entry->virt_state) == msi_is_masked(entry->phys_state)) > return 0; ==> skip IRQ forwarding > > [...] > } > > To fix this issue, when detect PCI_MSI_FLAGS_MASKBIT is not set in the > message control field, this patch simply clears bit > VFIO_PCI_MSI_STATE_MASKED for all vectors 'virt_state'; at the end > vfio_pci_update_msi_entry() can forward MSI IRQ successfully. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > --- > vfio/pci.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/vfio/pci.c b/vfio/pci.c > index ba971eb..4fd24ac 100644 > --- a/vfio/pci.c > +++ b/vfio/pci.c > @@ -363,8 +363,18 @@ static int vfio_pci_msi_vector_write(struct kvm *kvm, struct vfio_device *vdev, > struct vfio_pci_device *pdev = &vdev->pci; > struct msi_cap_64 *msi_cap_64 = PCI_CAP(&pdev->hdr, pdev->msi.pos); > > - if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) > + if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) { > + /* > + * If MSI doesn't support per-vector masking capability, > + * simply unmask for all vectors. > + */ > + for (i = 0; i < pdev->msi.nr_entries; i++) { > + entry = &pdev->msi.entries[i]; > + msi_set_masked(entry->virt_state, false); > + } > + This seems like the wrong place for this fix. vfio_pci_msi_vector_write() is called every time the guest pokes the MSI capability, and checks whether the access was on the Mask Bits Register. If the function doesn't support per-vector masking, then the register isn't implemented and we shouldn't do anything here. To fix the problem I think we need to set masked(virt_state) properly at init time, instead of blindly setting it to true. In fact from the guest's point of view, MSIs and MSI-X are unmasked (and disabled) at boot, so you could always set masked(virt_state) to false, but it may be safer to copy the actual state of the MSI's Mask Bits into virt_state, since for MSI, it could be non zero. If per-vector masking isn't supported, then the virt state should be false. Thanks, Jean
diff --git a/vfio/pci.c b/vfio/pci.c index ba971eb..4fd24ac 100644 --- a/vfio/pci.c +++ b/vfio/pci.c @@ -363,8 +363,18 @@ static int vfio_pci_msi_vector_write(struct kvm *kvm, struct vfio_device *vdev, struct vfio_pci_device *pdev = &vdev->pci; struct msi_cap_64 *msi_cap_64 = PCI_CAP(&pdev->hdr, pdev->msi.pos); - if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) + if (!(msi_cap_64->ctrl & PCI_MSI_FLAGS_MASKBIT)) { + /* + * If MSI doesn't support per-vector masking capability, + * simply unmask for all vectors. + */ + for (i = 0; i < pdev->msi.nr_entries; i++) { + entry = &pdev->msi.entries[i]; + msi_set_masked(entry->virt_state, false); + } + return 0; + } if (msi_cap_64->ctrl & PCI_MSI_FLAGS_64BIT) mask_pos = PCI_MSI_MASK_64;
If MSI doesn't support per-vector masking capability and PCI_MSI_FLAGS_MASKBIT isn't set in message control field, the function vfio_pci_msi_vector_write() will directly bail out for this case and every vector's 'virt_state' keeps setting bit VFIO_PCI_MSI_STATE_MASKED. This results in the state maintained in 'virt_state' cannot really reflect the MSI hardware state; finally it will mislead the function vfio_pci_update_msi_entry() to skip IRQ forwarding with below flow: vfio_pci_update_msi_entry() { [...] if (msi_is_masked(entry->virt_state) == msi_is_masked(entry->phys_state)) return 0; ==> skip IRQ forwarding [...] } To fix this issue, when detect PCI_MSI_FLAGS_MASKBIT is not set in the message control field, this patch simply clears bit VFIO_PCI_MSI_STATE_MASKED for all vectors 'virt_state'; at the end vfio_pci_update_msi_entry() can forward MSI IRQ successfully. Signed-off-by: Leo Yan <leo.yan@linaro.org> --- vfio/pci.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)