* [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset
@ 2017-12-08 6:31 Ladi Prosek
2017-12-08 14:09 ` Marcel Apfelbaum
0 siblings, 1 reply; 5+ messages in thread
From: Ladi Prosek @ 2017-12-08 6:31 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel, geoff, mst, alex.williamson
msix_mask_all() is supposed to invoke the release vector notifier if the state of the
respective vector changed from unmasked to masked. The way it's currently called from
msix_reset(), though, may result in calling the release notifier even if the vector
is already masked.
1) msix_reset() clears out the msix_cap field and the msix_table.
2) msix_mask_all() runs with was_masked=false for all vectors because of 1), which
results in calling the release notifier on all vectors.
3) if msix_reset() is subsequently called again, it goes through the same steps and
calls the release notifier on all vectors again.
This commit moves msix_mask_all() up so it runs before the device state is lost. And
it adds an assignment to msix_function_masked so that the device remembers that
MSI-X is masked.
This is likely a low impact issue, found while debugging an already broken device. It
is however easy to fix and the expectation that the use and release notifier invocations
are always balanced is very natural.
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
v1->v2:
* fixed typo in commit message "or" -> "to" (Marcel)
* directly set msix_function_masked to true instead of calling
msix_update_function_masked() (Marcel)
hw/pci/msix.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index c944c02135..d6a4dbdb6b 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
return;
}
msix_clear_all_vectors(dev);
+ msix_mask_all(dev, dev->msix_entries_nr);
dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
- msix_mask_all(dev, dev->msix_entries_nr);
+ dev->msix_function_masked = true;
}
/* PCI spec suggests that devices make it possible for software to configure
--
2.13.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset
2017-12-08 6:31 [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset Ladi Prosek
@ 2017-12-08 14:09 ` Marcel Apfelbaum
2017-12-21 14:01 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Apfelbaum @ 2017-12-08 14:09 UTC (permalink / raw)
To: Ladi Prosek; +Cc: qemu-devel, geoff, mst, alex williamson
----- Original Message -----
> From: "Ladi Prosek" <lprosek@redhat.com>
> To: qemu-devel@nongnu.org
> Cc: marcel@redhat.com, geoff@hostfission.com, mst@redhat.com, "alex williamson" <alex.williamson@redhat.com>
> Sent: Friday, December 8, 2017 8:31:13 AM
> Subject: [PATCH v2] msix: don't mask already masked vectors on reset
>
> msix_mask_all() is supposed to invoke the release vector notifier if the
> state of the
> respective vector changed from unmasked to masked. The way it's currently
> called from
> msix_reset(), though, may result in calling the release notifier even if the
> vector
> is already masked.
>
> 1) msix_reset() clears out the msix_cap field and the msix_table.
> 2) msix_mask_all() runs with was_masked=false for all vectors because of 1),
> which
> results in calling the release notifier on all vectors.
> 3) if msix_reset() is subsequently called again, it goes through the same
> steps and
> calls the release notifier on all vectors again.
>
> This commit moves msix_mask_all() up so it runs before the device state is
> lost. And
> it adds an assignment to msix_function_masked so that the device remembers
> that
> MSI-X is masked.
>
> This is likely a low impact issue, found while debugging an already broken
> device. It
> is however easy to fix and the expectation that the use and release notifier
> invocations
> are always balanced is very natural.
>
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>
> v1->v2:
> * fixed typo in commit message "or" -> "to" (Marcel)
> * directly set msix_function_masked to true instead of calling
> msix_update_function_masked() (Marcel)
>
>
> hw/pci/msix.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index c944c02135..d6a4dbdb6b 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
> return;
> }
> msix_clear_all_vectors(dev);
> + msix_mask_all(dev, dev->msix_entries_nr);
> dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
> - msix_mask_all(dev, dev->msix_entries_nr);
> + dev->msix_function_masked = true;
> }
>
> /* PCI spec suggests that devices make it possible for software to configure
> --
> 2.13.6
>
>
Thanks Ladi!
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset
2017-12-08 14:09 ` Marcel Apfelbaum
@ 2017-12-21 14:01 ` Michael S. Tsirkin
2017-12-21 15:08 ` Marcel Apfelbaum
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2017-12-21 14:01 UTC (permalink / raw)
To: Marcel Apfelbaum; +Cc: Ladi Prosek, qemu-devel, geoff, alex williamson
On Fri, Dec 08, 2017 at 09:09:06AM -0500, Marcel Apfelbaum wrote:
>
>
> ----- Original Message -----
> > From: "Ladi Prosek" <lprosek@redhat.com>
> > To: qemu-devel@nongnu.org
> > Cc: marcel@redhat.com, geoff@hostfission.com, mst@redhat.com, "alex williamson" <alex.williamson@redhat.com>
> > Sent: Friday, December 8, 2017 8:31:13 AM
> > Subject: [PATCH v2] msix: don't mask already masked vectors on reset
> >
> > msix_mask_all() is supposed to invoke the release vector notifier if the
> > state of the
> > respective vector changed from unmasked to masked. The way it's currently
> > called from
> > msix_reset(), though, may result in calling the release notifier even if the
> > vector
> > is already masked.
> >
> > 1) msix_reset() clears out the msix_cap field and the msix_table.
> > 2) msix_mask_all() runs with was_masked=false for all vectors because of 1),
> > which
> > results in calling the release notifier on all vectors.
> > 3) if msix_reset() is subsequently called again, it goes through the same
> > steps and
> > calls the release notifier on all vectors again.
> >
> > This commit moves msix_mask_all() up so it runs before the device state is
> > lost. And
> > it adds an assignment to msix_function_masked so that the device remembers
> > that
> > MSI-X is masked.
> >
> > This is likely a low impact issue, found while debugging an already broken
> > device. It
> > is however easy to fix and the expectation that the use and release notifier
> > invocations
> > are always balanced is very natural.
> >
> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> > ---
> >
> > v1->v2:
> > * fixed typo in commit message "or" -> "to" (Marcel)
> > * directly set msix_function_masked to true instead of calling
> > msix_update_function_masked() (Marcel)
> >
> >
> > hw/pci/msix.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index c944c02135..d6a4dbdb6b 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
> > return;
> > }
> > msix_clear_all_vectors(dev);
> > + msix_mask_all(dev, dev->msix_entries_nr);
> > dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
> > ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
> > memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
> > memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
> > - msix_mask_all(dev, dev->msix_entries_nr);
> > + dev->msix_function_masked = true;
> > }
> >
> > /* PCI spec suggests that devices make it possible for software to configure
> > --
> > 2.13.6
> >
> >
>
> Thanks Ladi!
>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
This breaks make check though.
Marcel - could you take a look please?
--
MST
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset
2017-12-21 14:01 ` Michael S. Tsirkin
@ 2017-12-21 15:08 ` Marcel Apfelbaum
2017-12-21 19:17 ` Marcel Apfelbaum
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Apfelbaum @ 2017-12-21 15:08 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ladi Prosek, qemu-devel, geoff, alex williamson
On 21/12/2017 16:01, Michael S. Tsirkin wrote:
> On Fri, Dec 08, 2017 at 09:09:06AM -0500, Marcel Apfelbaum wrote:
>>
>>
>> ----- Original Message -----
>>> From: "Ladi Prosek" <lprosek@redhat.com>
>>> To: qemu-devel@nongnu.org
>>> Cc: marcel@redhat.com, geoff@hostfission.com, mst@redhat.com, "alex williamson" <alex.williamson@redhat.com>
>>> Sent: Friday, December 8, 2017 8:31:13 AM
>>> Subject: [PATCH v2] msix: don't mask already masked vectors on reset
>>>
>>> msix_mask_all() is supposed to invoke the release vector notifier if the
>>> state of the
>>> respective vector changed from unmasked to masked. The way it's currently
>>> called from
>>> msix_reset(), though, may result in calling the release notifier even if the
>>> vector
>>> is already masked.
>>>
>>> 1) msix_reset() clears out the msix_cap field and the msix_table.
>>> 2) msix_mask_all() runs with was_masked=false for all vectors because of 1),
>>> which
>>> results in calling the release notifier on all vectors.
>>> 3) if msix_reset() is subsequently called again, it goes through the same
>>> steps and
>>> calls the release notifier on all vectors again.
>>>
>>> This commit moves msix_mask_all() up so it runs before the device state is
>>> lost. And
>>> it adds an assignment to msix_function_masked so that the device remembers
>>> that
>>> MSI-X is masked.
>>>
>>> This is likely a low impact issue, found while debugging an already broken
>>> device. It
>>> is however easy to fix and the expectation that the use and release notifier
>>> invocations
>>> are always balanced is very natural.
>>>
>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>> ---
>>>
>>> v1->v2:
>>> * fixed typo in commit message "or" -> "to" (Marcel)
>>> * directly set msix_function_masked to true instead of calling
>>> msix_update_function_masked() (Marcel)
>>>
>>>
>>> hw/pci/msix.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>>> index c944c02135..d6a4dbdb6b 100644
>>> --- a/hw/pci/msix.c
>>> +++ b/hw/pci/msix.c
>>> @@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
>>> return;
>>> }
>>> msix_clear_all_vectors(dev);
>>> + msix_mask_all(dev, dev->msix_entries_nr);
>>> dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>>> ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>>> memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
>>> memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
>>> - msix_mask_all(dev, dev->msix_entries_nr);
>>> + dev->msix_function_masked = true;
>>> }
>>>
>>> /* PCI spec suggests that devices make it possible for software to configure
>>> --
>>> 2.13.6
>>>
>>>
>>
>> Thanks Ladi!
>>
>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>
> This breaks make check though.
>
> Marcel - could you take a look please?
>
Sure
Thanks,
Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset
2017-12-21 15:08 ` Marcel Apfelbaum
@ 2017-12-21 19:17 ` Marcel Apfelbaum
0 siblings, 0 replies; 5+ messages in thread
From: Marcel Apfelbaum @ 2017-12-21 19:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Ladi Prosek, qemu-devel, geoff, alex williamson
On 21/12/2017 17:08, Marcel Apfelbaum wrote:
> On 21/12/2017 16:01, Michael S. Tsirkin wrote:
>> On Fri, Dec 08, 2017 at 09:09:06AM -0500, Marcel Apfelbaum wrote:
>>>
>>>
>>> ----- Original Message -----
>>>> From: "Ladi Prosek" <lprosek@redhat.com>
>>>> To: qemu-devel@nongnu.org
>>>> Cc: marcel@redhat.com, geoff@hostfission.com, mst@redhat.com, "alex williamson" <alex.williamson@redhat.com>
>>>> Sent: Friday, December 8, 2017 8:31:13 AM
>>>> Subject: [PATCH v2] msix: don't mask already masked vectors on reset
>>>>
>>>> msix_mask_all() is supposed to invoke the release vector notifier if the
>>>> state of the
>>>> respective vector changed from unmasked to masked. The way it's currently
>>>> called from
>>>> msix_reset(), though, may result in calling the release notifier even if the
>>>> vector
>>>> is already masked.
>>>>
>>>> 1) msix_reset() clears out the msix_cap field and the msix_table.
>>>> 2) msix_mask_all() runs with was_masked=false for all vectors because of 1),
>>>> which
>>>> results in calling the release notifier on all vectors.
>>>> 3) if msix_reset() is subsequently called again, it goes through the same
>>>> steps and
>>>> calls the release notifier on all vectors again.
>>>>
>>>> This commit moves msix_mask_all() up so it runs before the device state is
>>>> lost. And
>>>> it adds an assignment to msix_function_masked so that the device remembers
>>>> that
>>>> MSI-X is masked.
>>>>
>>>> This is likely a low impact issue, found while debugging an already broken
>>>> device. It
>>>> is however easy to fix and the expectation that the use and release notifier
>>>> invocations
>>>> are always balanced is very natural.
>>>>
>>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>>> ---
>>>>
>>>> v1->v2:
>>>> * fixed typo in commit message "or" -> "to" (Marcel)
>>>> * directly set msix_function_masked to true instead of calling
>>>> msix_update_function_masked() (Marcel)
>>>>
>>>>
>>>> hw/pci/msix.c | 3 ++-
>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
>>>> index c944c02135..d6a4dbdb6b 100644
>>>> --- a/hw/pci/msix.c
>>>> +++ b/hw/pci/msix.c
>>>> @@ -500,11 +500,12 @@ void msix_reset(PCIDevice *dev)
>>>> return;
>>>> }
>>>> msix_clear_all_vectors(dev);
>>>> + msix_mask_all(dev, dev->msix_entries_nr);
>>>> dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &=
>>>> ~dev->wmask[dev->msix_cap + MSIX_CONTROL_OFFSET];
>>>> memset(dev->msix_table, 0, dev->msix_entries_nr * PCI_MSIX_ENTRY_SIZE);
>>>> memset(dev->msix_pba, 0, QEMU_ALIGN_UP(dev->msix_entries_nr, 64) / 8);
>>>> - msix_mask_all(dev, dev->msix_entries_nr);
>>>> + dev->msix_function_masked = true;
>>>> }
>>>> /* PCI spec suggests that devices make it possible for software to configure
>>>> --
>>>> 2.13.6
>>>>
>>>>
>>>
>>> Thanks Ladi!
>>>
>>> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
>>
>> This breaks make check though.
>>
>> Marcel - could you take a look please?
>>
>
> Sure
We have a problem with the patch.
Moving msix_mask_all before resetting the msix_table to 0
modifies behavior since we don't set the PCI_MSIX_ENTRY_CTRL_MASKBIT
for each vector anymore (the information is lost).
What we need (I think) is to run msix_mask_all again,
this time without calling msix_handle_mask_update (only setting the bit).
(I tested it and the make check passes after it)
I think is risky to use the patch for now.
Thanks,
Marcel
>
> Thanks,
> Marcel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-21 19:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-08 6:31 [Qemu-devel] [PATCH v2] msix: don't mask already masked vectors on reset Ladi Prosek
2017-12-08 14:09 ` Marcel Apfelbaum
2017-12-21 14:01 ` Michael S. Tsirkin
2017-12-21 15:08 ` Marcel Apfelbaum
2017-12-21 19:17 ` Marcel Apfelbaum
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.