[Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Ladi Prosek]

msix_mask_all() is supposed to invoke the release vector notifier if the state of the
respective vector changed from unmasked or 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 a call to msix_update_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>
---
 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..34656de9b0 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);
+    msix_update_function_masked(dev);
 }
 
 /* PCI spec suggests that devices make it possible for software to configure
-- 
2.13.5

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[no-reply]

Hi,

This series failed automatic build test. Please find the testing commands and
their output below. If you have docker installed, you can probably reproduce it
locally.

Subject: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset
Type: series
Message-id: 20171120142216.17832-1-lprosek@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
set -e
git submodule update --init dtc
# Let docker tests dump environment info
export SHOW_ENV=1
export J=8
time make docker-test-quick@centos6
time make docker-test-build@min-glib
time make docker-test-mingw@fedora
time make docker-test-block@fedora
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
error: RPC failed; curl 18 transfer closed with outstanding read data remaining
fatal: The remote end hung up unexpectedly
error: Could not fetch 3c8cf5a9c21ff8782164d1def7f44bd888713384
Traceback (most recent call last):
  File "/usr/bin/patchew", line 442, in test_one
    git_clone_repo(clone, r["repo"], r["head"], logf)
  File "/usr/bin/patchew", line 48, in git_clone_repo
    stdout=logf, stderr=logf)
  File "/usr/lib64/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['git', 'remote', 'add', '-f', '--mirror=fetch', '3c8cf5a9c21ff8782164d1def7f44bd888713384', 'https://github.com/patchew-project/qemu']' returned non-zero exit status 1.



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Marcel Apfelbaum]

Hi Ladi,

On 20/11/2017 16:22, Ladi Prosek wrote:
> msix_mask_all() is supposed to invoke the release vector notifier if the state of the
> respective vector changed from unmasked or masked. 

You mean from unmasked "to" masked right?

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.
> 

As far as I can see in the code you are right.(very reset will trigger the release notifiers
again)

> This commit moves msix_mask_all() up so it runs before the device state is lost.

OK

> And
> it adds a call to msix_update_function_masked() so that the device remembers that
> MSI-X is masked.
> 

msix_update_function_masked checks the msix is enabled or masked-off.
You are building on the fact the msix will not be enabled to set
"msix_function_masked" to "true", right?
(I just want to be sure I understand the patch)

> 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.
> 

I would leave it (maybe) out of 2.11 because it may expose other bugs
and we are after rc2 already.

Adding Alex Williamson to see it does not affect device assignment,
other than that the patch looks OK to me.


Thanks,
Marcel

> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
>   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..34656de9b0 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);
> +    msix_update_function_masked(dev);
>   }
>   
>   /* PCI spec suggests that devices make it possible for software to configure
> 

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Ladi Prosek]

On Wed, Nov 22, 2017 at 11:46 AM, Marcel Apfelbaum <marcel@redhat.com> wrote:
> Hi Ladi,
>
> On 20/11/2017 16:22, Ladi Prosek wrote:
>>
>> msix_mask_all() is supposed to invoke the release vector notifier if the
>> state of the
>> respective vector changed from unmasked or masked.
>
>
> You mean from unmasked "to" masked right?

Yes, that's a typo.

> 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.
>>
>
> As far as I can see in the code you are right.(very reset will trigger the
> release notifiers
> again)
>
>> This commit moves msix_mask_all() up so it runs before the device state is
>> lost.
>
>
> OK
>
>> And
>> it adds a call to msix_update_function_masked() so that the device
>> remembers that
>> MSI-X is masked.
>>
>
> msix_update_function_masked checks the msix is enabled or masked-off.
> You are building on the fact the msix will not be enabled to set
> "msix_function_masked" to "true", right?
> (I just want to be sure I understand the patch)

Correct. msix_enabled() will return false because we've just reset

  dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET]

I guess we could also simply assign true to it:

  dev->msix_function_masked = true;

just like msix_init() does.

>> 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.
>>
>
> I would leave it (maybe) out of 2.11 because it may expose other bugs
> and we are after rc2 already.
>
> Adding Alex Williamson to see it does not affect device assignment,
> other than that the patch looks OK to me.
>
>
> Thanks,
> Marcel
>
>
>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> ---
>>   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..34656de9b0 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);
>> +    msix_update_function_masked(dev);
>>   }
>>     /* PCI spec suggests that devices make it possible for software to
>> configure
>>
>

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Marcel Apfelbaum]

On 22/11/2017 14:32, Ladi Prosek wrote:
> On Wed, Nov 22, 2017 at 11:46 AM, Marcel Apfelbaum <marcel@redhat.com> wrote:
>> Hi Ladi,
>>
>> On 20/11/2017 16:22, Ladi Prosek wrote:
>>>
>>> msix_mask_all() is supposed to invoke the release vector notifier if the
>>> state of the
>>> respective vector changed from unmasked or masked.
>>
>>
>> You mean from unmasked "to" masked right?
> 
> Yes, that's a typo.
> 
>> 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.
>>>
>>
>> As far as I can see in the code you are right.(very reset will trigger the
>> release notifiers
>> again)
>>
>>> This commit moves msix_mask_all() up so it runs before the device state is
>>> lost.
>>
>>
>> OK
>>
>>> And
>>> it adds a call to msix_update_function_masked() so that the device
>>> remembers that
>>> MSI-X is masked.
>>>
>>
>> msix_update_function_masked checks the msix is enabled or masked-off.
>> You are building on the fact the msix will not be enabled to set
>> "msix_function_masked" to "true", right?
>> (I just want to be sure I understand the patch)
> 
> Correct. msix_enabled() will return false because we've just reset
> 
>    dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET]
> 
> I guess we could also simply assign true to it:
> 
>    dev->msix_function_masked = true;
> 
> just like msix_init() does.

Yes, is preferable - I think.
If you intend to send V2, please wait first for Alex's remarks if he has any.

Thanks,
Marcel

> 
>>> 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.
>>>
>>
>> I would leave it (maybe) out of 2.11 because it may expose other bugs
>> and we are after rc2 already.
>>
>> Adding Alex Williamson to see it does not affect device assignment,
>> other than that the patch looks OK to me.
>>
>>
>> Thanks,
>> Marcel
>>
>>
>>> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>>> ---
>>>    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..34656de9b0 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);
>>> +    msix_update_function_masked(dev);
>>>    }
>>>      /* PCI spec suggests that devices make it possible for software to
>>> configure
>>>
>>

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Alex Williamson]

On Wed, 22 Nov 2017 12:46:45 +0200
Marcel Apfelbaum <marcel@redhat.com> wrote:

> Hi Ladi,
> 
> On 20/11/2017 16:22, Ladi Prosek wrote:
> > msix_mask_all() is supposed to invoke the release vector notifier if the state of the
> > respective vector changed from unmasked or masked.   
> 
> You mean from unmasked "to" masked right?
> 
> 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.
> >   
> 
> As far as I can see in the code you are right.(very reset will trigger the release notifiers
> again)
> 
> > This commit moves msix_mask_all() up so it runs before the device state is lost.  
> 
> OK
> 
> > And
> > it adds a call to msix_update_function_masked() so that the device remembers that
> > MSI-X is masked.
> >   
> 
> msix_update_function_masked checks the msix is enabled or masked-off.
> You are building on the fact the msix will not be enabled to set
> "msix_function_masked" to "true", right?
> (I just want to be sure I understand the patch)
> 
> > 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.
> >   
> 
> I would leave it (maybe) out of 2.11 because it may expose other bugs
> and we are after rc2 already.
> 
> Adding Alex Williamson to see it does not affect device assignment,
> other than that the patch looks OK to me.

I flip flopped around here because vfio_msix_vector_release() doesn't
care if it gets called more than once for the same vector, but then I
looked at the ordering of vfio_pci_reset() vs msix_reset().  vfio will
never leave vfio_pci_reset() with MSI-X enabled, we unset our
notifiers , release and unuse any in-use vectors, and leave with only
INTx enabled (if supported).  So I don't think the patch below has any
effect whatsoever for vfio, and probably shouldn't for most devices as
resetting back to a state of MSI-X disabled ought to be standard
procedure... but maybe other devices rely on msix_reset() for this.
Thanks,

Alex

> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> > ---
> >   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..34656de9b0 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);
> > +    msix_update_function_masked(dev);
> >   }
> >   
> >   /* PCI spec suggests that devices make it possible for software to configure
> >   
> 

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Michael S. Tsirkin]

On Wed, Nov 22, 2017 at 03:22:50PM +0200, Marcel Apfelbaum wrote:
> On 22/11/2017 14:32, Ladi Prosek wrote:
> > On Wed, Nov 22, 2017 at 11:46 AM, Marcel Apfelbaum <marcel@redhat.com> wrote:
> > > Hi Ladi,
> > > 
> > > On 20/11/2017 16:22, Ladi Prosek wrote:
> > > > 
> > > > msix_mask_all() is supposed to invoke the release vector notifier if the
> > > > state of the
> > > > respective vector changed from unmasked or masked.
> > > 
> > > 
> > > You mean from unmasked "to" masked right?
> > 
> > Yes, that's a typo.
> > 
> > > 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.
> > > > 
> > > 
> > > As far as I can see in the code you are right.(very reset will trigger the
> > > release notifiers
> > > again)
> > > 
> > > > This commit moves msix_mask_all() up so it runs before the device state is
> > > > lost.
> > > 
> > > 
> > > OK
> > > 
> > > > And
> > > > it adds a call to msix_update_function_masked() so that the device
> > > > remembers that
> > > > MSI-X is masked.
> > > > 
> > > 
> > > msix_update_function_masked checks the msix is enabled or masked-off.
> > > You are building on the fact the msix will not be enabled to set
> > > "msix_function_masked" to "true", right?
> > > (I just want to be sure I understand the patch)
> > 
> > Correct. msix_enabled() will return false because we've just reset
> > 
> >    dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET]
> > 
> > I guess we could also simply assign true to it:
> > 
> >    dev->msix_function_masked = true;
> > 
> > just like msix_init() does.
> 
> Yes, is preferable - I think.
> If you intend to send V2, please wait first for Alex's remarks if he has any.
> 
> Thanks,
> Marcel
> 
> > 
> > > > 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.
> > > > 
> > > 
> > > I would leave it (maybe) out of 2.11 because it may expose other bugs
> > > and we are after rc2 already.
> > > 
> > > Adding Alex Williamson to see it does not affect device assignment,
> > > other than that the patch looks OK to me.
> > > 
> > > 
> > > Thanks,
> > > Marcel
> > > 
> > > 
> > > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> > > > ---
> > > >    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..34656de9b0 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);
> > > > +    msix_update_function_masked(dev);
> > > >    }
> > > >      /* PCI spec suggests that devices make it possible for software to
> > > > configure
> > > > 
> > > 

Do you intend to post v2 or need Marcel to?

Re: [Qemu-devel] [PATCH] msix: don't mask already masked vectors on reset

[Ladi Prosek]

On Thu, Dec 7, 2017 at 7:27 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Nov 22, 2017 at 03:22:50PM +0200, Marcel Apfelbaum wrote:
>> On 22/11/2017 14:32, Ladi Prosek wrote:
>> > On Wed, Nov 22, 2017 at 11:46 AM, Marcel Apfelbaum <marcel@redhat.com> wrote:
>> > > Hi Ladi,
>> > >
>> > > On 20/11/2017 16:22, Ladi Prosek wrote:
>> > > >
>> > > > msix_mask_all() is supposed to invoke the release vector notifier if the
>> > > > state of the
>> > > > respective vector changed from unmasked or masked.
>> > >
>> > >
>> > > You mean from unmasked "to" masked right?
>> >
>> > Yes, that's a typo.
>> >
>> > > 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.
>> > > >
>> > >
>> > > As far as I can see in the code you are right.(very reset will trigger the
>> > > release notifiers
>> > > again)
>> > >
>> > > > This commit moves msix_mask_all() up so it runs before the device state is
>> > > > lost.
>> > >
>> > >
>> > > OK
>> > >
>> > > > And
>> > > > it adds a call to msix_update_function_masked() so that the device
>> > > > remembers that
>> > > > MSI-X is masked.
>> > > >
>> > >
>> > > msix_update_function_masked checks the msix is enabled or masked-off.
>> > > You are building on the fact the msix will not be enabled to set
>> > > "msix_function_masked" to "true", right?
>> > > (I just want to be sure I understand the patch)
>> >
>> > Correct. msix_enabled() will return false because we've just reset
>> >
>> >    dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET]
>> >
>> > I guess we could also simply assign true to it:
>> >
>> >    dev->msix_function_masked = true;
>> >
>> > just like msix_init() does.
>>
>> Yes, is preferable - I think.
>> If you intend to send V2, please wait first for Alex's remarks if he has any.
>>
>> Thanks,
>> Marcel
>>
>> >
>> > > > 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.
>> > > >
>> > >
>> > > I would leave it (maybe) out of 2.11 because it may expose other bugs
>> > > and we are after rc2 already.
>> > >
>> > > Adding Alex Williamson to see it does not affect device assignment,
>> > > other than that the patch looks OK to me.
>> > >
>> > >
>> > > Thanks,
>> > > Marcel
>> > >
>> > >
>> > > > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> > > > ---
>> > > >    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..34656de9b0 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);
>> > > > +    msix_update_function_masked(dev);
>> > > >    }
>> > > >      /* PCI spec suggests that devices make it possible for software to
>> > > > configure
>> > > >
>> > >
>
> Do you intend to post v2 or need Marcel to?

I'll post v2 tomorrow.