RE: [PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -Original Message-
> From: Alex Williamson [mailto:alex.william...@redhat.com]
> Sent: Saturday, October 2, 2021 7:04 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> 
> Cc: phi...@redhat.com; pbonz...@redhat.com; marcel.apfelb...@gmail.com;
> m...@redhat.com; qemu-devel@nongnu.org; Gonglei (Arei)
> ; chenjiashang 
> Subject: Re: [PATCH v3 4/9] msix: simplify the conditional in
> msix_set/unset_vector_notifiers
> 
> On Tue, 21 Sep 2021 07:01:57 +0800
> "Longpeng(Mike)"  wrote:
> 
> > 'msix_function_masked' is synchronized with the device's config,
> > we can use it to replace the complex conditional statementis in
> > msix_set/unset_vector_notifiers.
> >
> > Signed-off-by: Longpeng(Mike) 
> > ---
> >  hw/pci/msix.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> > index ae9331cd0b..67682289af 100644
> > --- a/hw/pci/msix.c
> > +++ b/hw/pci/msix.c
> > @@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
> >  dev->msix_vector_release_notifier = release_notifier;
> >  dev->msix_vector_poll_notifier = poll_notifier;
> >
> > -if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > -(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> > +if (!dev->msix_function_masked) {
> >  for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> >  ret = msix_set_notifier_for_vector(dev, vector);
> >  if (ret < 0) {
> > @@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
> >  assert(dev->msix_vector_use_notifier &&
> > dev->msix_vector_release_notifier);
> >
> > -if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> > -(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> > +if (!dev->msix_function_masked) {
> >  for (vector = 0; vector < dev->msix_entries_nr; vector++) {
> >  msix_unset_notifier_for_vector(dev, vector);
> >  }
> 
> This appears to be a cleanup that's not required for the functionality
> of this series.  I'd suggest proposing it separately.  Same for the
> patch 5/9 in this series.  If it makes a functional difference it
> should be described in the commit log.  Thanks,
> 

OK, will remove these two patches in the v4, thanks.

> Alex

Re: [PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Alex Williamson]

On Tue, 21 Sep 2021 07:01:57 +0800
"Longpeng(Mike)"  wrote:

> 'msix_function_masked' is synchronized with the device's config,
> we can use it to replace the complex conditional statementis in
> msix_set/unset_vector_notifiers.
> 
> Signed-off-by: Longpeng(Mike) 
> ---
>  hw/pci/msix.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index ae9331cd0b..67682289af 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
>  dev->msix_vector_release_notifier = release_notifier;
>  dev->msix_vector_poll_notifier = poll_notifier;
>  
> -if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> -(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> +if (!dev->msix_function_masked) {
>  for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>  ret = msix_set_notifier_for_vector(dev, vector);
>  if (ret < 0) {
> @@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
>  assert(dev->msix_vector_use_notifier &&
> dev->msix_vector_release_notifier);
>  
> -if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
> -(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
> +if (!dev->msix_function_masked) {
>  for (vector = 0; vector < dev->msix_entries_nr; vector++) {
>  msix_unset_notifier_for_vector(dev, vector);
>  }

This appears to be a cleanup that's not required for the functionality
of this series.  I'd suggest proposing it separately.  Same for the
patch 5/9 in this series.  If it makes a functional difference it
should be described in the commit log.  Thanks,

Alex

[PATCH v3 4/9] msix: simplify the conditional in msix_set/unset_vector_notifiers

[Longpeng(Mike)]

'msix_function_masked' is synchronized with the device's config,
we can use it to replace the complex conditional statementis in
msix_set/unset_vector_notifiers.

Signed-off-by: Longpeng(Mike) 
---
 hw/pci/msix.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index ae9331cd0b..67682289af 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -592,8 +592,7 @@ int msix_set_vector_notifiers(PCIDevice *dev,
 dev->msix_vector_release_notifier = release_notifier;
 dev->msix_vector_poll_notifier = poll_notifier;
 
-if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
-(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
+if (!dev->msix_function_masked) {
 for (vector = 0; vector < dev->msix_entries_nr; vector++) {
 ret = msix_set_notifier_for_vector(dev, vector);
 if (ret < 0) {
@@ -622,8 +621,7 @@ void msix_unset_vector_notifiers(PCIDevice *dev)
 assert(dev->msix_vector_use_notifier &&
dev->msix_vector_release_notifier);
 
-if ((dev->config[dev->msix_cap + MSIX_CONTROL_OFFSET] &
-(MSIX_ENABLE_MASK | MSIX_MASKALL_MASK)) == MSIX_ENABLE_MASK) {
+if (!dev->msix_function_masked) {
 for (vector = 0; vector < dev->msix_entries_nr; vector++) {
 msix_unset_notifier_for_vector(dev, vector);
 }
-- 
2.23.0