[PATCH 0/5] x86/vPCI: MSI/MSI-X related adjustments

[PATCH 0/5] x86/vPCI: MSI/MSI-X related adjustments

[Jan Beulich]

1: tolerate (un)masking a disabled MSI-X entry
2: check address in vpci_msi_update()
3: vPCI/MSI-X: fold clearing of entry->updated
4: vPCI/MSI-X: make use of xzalloc_flex_struct()
5: vPCI/MSI-X: tidy init_msix()

Jan

[PATCH 1/5] x86/vPCI: tolerate (un)masking a disabled MSI-X entry

[Jan Beulich]

None of the four reasons causing vpci_msix_arch_mask_entry() to get
called (there's just a single call site) are impossible or illegal prior
to an entry actually having got set up:
- the entry may remain masked (in this case, however, a prior masked ->
  unmasked transition would already not have worked),
- MSI-X may not be enabled,
- the global mask bit may be set,
- the entry may not otherwise have been updated.
Hence the function asserting that the entry was previously set up was
simply wrong. Since the caller tracks the masked state (and setting up
of an entry would only be effected when that software bit is clear),
it's okay to skip both masking and unmasking requests in this case.

Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers')
Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is a presumed alternative to Roger's "vpci/msix: exit early if
MSI-X is disabled or globally masked".

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -840,8 +840,8 @@ void vpci_msi_arch_print(const struct vp
 void vpci_msix_arch_mask_entry(struct vpci_msix_entry *entry,
                                const struct pci_dev *pdev, bool mask)
 {
-    ASSERT(entry->arch.pirq != INVALID_PIRQ);
-    vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask);
+    if ( entry->arch.pirq != INVALID_PIRQ )
+        vpci_mask_pirq(pdev->domain, entry->arch.pirq, mask);
 }
 
 int vpci_msix_arch_enable_entry(struct vpci_msix_entry *entry,

Re: [PATCH 1/5] x86/vPCI: tolerate (un)masking a disabled MSI-X entry

[Roger Pau Monné]

On Mon, Dec 07, 2020 at 11:36:38AM +0100, Jan Beulich wrote:
> None of the four reasons causing vpci_msix_arch_mask_entry() to get
> called (there's just a single call site) are impossible or illegal prior
> to an entry actually having got set up:
> - the entry may remain masked (in this case, however, a prior masked ->
>   unmasked transition would already not have worked),
> - MSI-X may not be enabled,
> - the global mask bit may be set,
> - the entry may not otherwise have been updated.
> Hence the function asserting that the entry was previously set up was
> simply wrong. Since the caller tracks the masked state (and setting up
> of an entry would only be effected when that software bit is clear),
> it's okay to skip both masking and unmasking requests in this case.

On the original approach I just added this because I convinced myself
that scenario was impossible. I think we could also do:

diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
index 64dd0a929c..509cf3962c 100644
--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
          * so that it picks the new state.
          */
         entry->masked = new_masked;
-        if ( !new_masked && msix->enabled && !msix->masked && entry->updated )
+
+        if ( !msix->enabled )
+            break;
+
+        if ( !new_masked && !msix->masked && entry->updated )
         {
             /*
              * If MSI-X is enabled, the function mask is not active, the entry
@@ -470,6 +474,7 @@ static int init_msix(struct pci_dev *pdev)
     for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
     {
         pdev->vpci->msix->entries[i].masked = true;
+        pdev->vpci->msix->entries[i].updated = true;
         vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
     }

In order to solve the issue.

As pointed out in another patch, regardless of what we end up doing
with the issue at hand we might have to consider setting updated to
true in init_msix in case we want to somehow support enabling an entry
that has it's address and data fields set to 0.

> 
> Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers')
> Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Manuel, can we get confirmation that this fixes your issue?

Thanks, Roger.

Re: [PATCH 1/5] x86/vPCI: tolerate (un)masking a disabled MSI-X entry

[Jan Beulich]

On 28.12.2020 19:24, Roger Pau Monné wrote:
> On Mon, Dec 07, 2020 at 11:36:38AM +0100, Jan Beulich wrote:
>> None of the four reasons causing vpci_msix_arch_mask_entry() to get
>> called (there's just a single call site) are impossible or illegal prior
>> to an entry actually having got set up:
>> - the entry may remain masked (in this case, however, a prior masked ->
>>   unmasked transition would already not have worked),
>> - MSI-X may not be enabled,
>> - the global mask bit may be set,
>> - the entry may not otherwise have been updated.
>> Hence the function asserting that the entry was previously set up was
>> simply wrong. Since the caller tracks the masked state (and setting up
>> of an entry would only be effected when that software bit is clear),
>> it's okay to skip both masking and unmasking requests in this case.
> 
> On the original approach I just added this because I convinced myself
> that scenario was impossible. I think we could also do:
> 
> diff --git a/xen/drivers/vpci/msix.c b/xen/drivers/vpci/msix.c
> index 64dd0a929c..509cf3962c 100644
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -357,7 +357,11 @@ static int msix_write(struct vcpu *v, unsigned long addr, unsigned int len,
>           * so that it picks the new state.
>           */
>          entry->masked = new_masked;
> -        if ( !new_masked && msix->enabled && !msix->masked && entry->updated )
> +
> +        if ( !msix->enabled )
> +            break;
> +
> +        if ( !new_masked && !msix->masked && entry->updated )
>          {
>              /*
>               * If MSI-X is enabled, the function mask is not active, the entry
> @@ -470,6 +474,7 @@ static int init_msix(struct pci_dev *pdev)
>      for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
>      {
>          pdev->vpci->msix->entries[i].masked = true;
> +        pdev->vpci->msix->entries[i].updated = true;
>          vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
>      }
> 
> In order to solve the issue.
> 
> As pointed out in another patch, regardless of what we end up doing
> with the issue at hand we might have to consider setting updated to
> true in init_msix in case we want to somehow support enabling an entry
> that has it's address and data fields set to 0.

Yes, but I view this as an orthogonal aspect to consider (down
the road).

>> Fixes: d6281be9d0145 ('vpci/msix: add MSI-X handlers')
>> Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Manuel, can we get confirmation that this fixes your issue?

I'll give it some time before committing for him to confirm,
but I guess I'd like to time out by the end of the week.

Jan

Re: [PATCH 1/5] x86/vPCI: tolerate (un)masking a disabled MSI-X entry

[Manuel Bouyer]

On Mon, Jan 04, 2021 at 05:35:23PM +0100, Jan Beulich wrote:
> Thanks.
> 
> > Manuel, can we get confirmation that this fixes your issue?
> 
> I'll give it some time before committing for him to confirm,
> but I guess I'd like to time out by the end of the week.

Yes, it works for me

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

[PATCH 2/5] x86/vPCI: check address in vpci_msi_update()

[Jan Beulich]

If the upper address bits don't match the interrupt delivery address
space window, entirely different behavior would need to be implemented.
Refuse such requests for the time being.

Replace adjacent hard tabs while introducing MSI_ADDR_BASE_MASK.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -682,6 +682,13 @@ static int vpci_msi_update(const struct
 
     ASSERT(pcidevs_locked());
 
+    if ( (address & MSI_ADDR_BASE_MASK) != MSI_ADDR_HEADER )
+    {
+        gdprintk(XENLOG_ERR, "%pp: PIRQ %u: unsupported address %lx\n",
+                 &pdev->sbdf, pirq, address);
+        return -EOPNOTSUPP;
+    }
+
     for ( i = 0; i < vectors; i++ )
     {
         uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK);
--- a/xen/include/asm-x86/msi.h
+++ b/xen/include/asm-x86/msi.h
@@ -36,8 +36,9 @@
  * Shift/mask fields for msi address
  */
 
-#define MSI_ADDR_BASE_HI	    	0
-#define MSI_ADDR_BASE_LO	    	0xfee00000
+#define MSI_ADDR_BASE_HI            0
+#define MSI_ADDR_BASE_LO            0xfee00000
+#define MSI_ADDR_BASE_MASK          (~0xfffff)
 #define MSI_ADDR_HEADER             MSI_ADDR_BASE_LO
 
 #define MSI_ADDR_DESTMODE_SHIFT     2

Re: [PATCH 2/5] x86/vPCI: check address in vpci_msi_update()

[Roger Pau Monné]

On Mon, Dec 07, 2020 at 11:37:22AM +0100, Jan Beulich wrote:
> If the upper address bits don't match the interrupt delivery address
> space window, entirely different behavior would need to be implemented.
> Refuse such requests for the time being.
> 
> Replace adjacent hard tabs while introducing MSI_ADDR_BASE_MASK.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

[PATCH 3/5] vPCI/MSI-X: fold clearing of entry->updated

[Jan Beulich]

Both call sites clear the flag after a successfull call to
update_entry(). This can be simplified by moving the clearing into the
function, onto its success path.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As a result it becomes clear that the return value of the function is of
no interest to either of the callers. I'm not sure whether ditching it
is the right thing to do, or whether this rather hints at some problem.

--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -64,6 +64,8 @@ static int update_entry(struct vpci_msix
         return rc;
     }
 
+    entry->updated = false;
+
     return 0;
 }
 
@@ -92,13 +94,8 @@ static void control_write(const struct p
     if ( new_enabled && !new_masked && (!msix->enabled || msix->masked) )
     {
         for ( i = 0; i < msix->max_entries; i++ )
-        {
-            if ( msix->entries[i].masked || !msix->entries[i].updated ||
-                 update_entry(&msix->entries[i], pdev, i) )
-                continue;
-
-            msix->entries[i].updated = false;
-        }
+            if ( !msix->entries[i].masked && msix->entries[i].updated )
+                update_entry(&msix->entries[i], pdev, i);
     }
     else if ( !new_enabled && msix->enabled )
     {
@@ -365,10 +362,7 @@ static int msix_write(struct vcpu *v, un
              * data fields Xen needs to disable and enable the entry in order
              * to pick up the changes.
              */
-            if ( update_entry(entry, pdev, vmsix_entry_nr(msix, entry)) )
-                break;
-
-            entry->updated = false;
+            update_entry(entry, pdev, vmsix_entry_nr(msix, entry));
         }
         else
             vpci_msix_arch_mask_entry(entry, pdev, entry->masked);

Re: [PATCH 3/5] vPCI/MSI-X: fold clearing of entry->updated

[Roger Pau Monné]

On Mon, Dec 07, 2020 at 11:37:51AM +0100, Jan Beulich wrote:
> Both call sites clear the flag after a successfull call to
> update_entry(). This can be simplified by moving the clearing into the
> function, onto its success path.

The point of returning a value was to set the updated field, as there
was no failure log message printed by the callers.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> As a result it becomes clear that the return value of the function is of
> no interest to either of the callers. I'm not sure whether ditching it
> is the right thing to do, or whether this rather hints at some problem.

I think you should make the function void as part of this change,
there's a log message printed by update_entry in the failure case
which IMO should be enough.

There's not much else callers can do AFAICT.

> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -64,6 +64,8 @@ static int update_entry(struct vpci_msix
>          return rc;
>      }
>  
> +    entry->updated = false;
> +
>      return 0;
>  }
>  
> @@ -92,13 +94,8 @@ static void control_write(const struct p
>      if ( new_enabled && !new_masked && (!msix->enabled || msix->masked) )
>      {
>          for ( i = 0; i < msix->max_entries; i++ )
> -        {
> -            if ( msix->entries[i].masked || !msix->entries[i].updated ||
> -                 update_entry(&msix->entries[i], pdev, i) )
> -                continue;
> -
> -            msix->entries[i].updated = false;
> -        }
> +            if ( !msix->entries[i].masked && msix->entries[i].updated )
> +                update_entry(&msix->entries[i], pdev, i);
>      }
>      else if ( !new_enabled && msix->enabled )
>      {
> @@ -365,10 +362,7 @@ static int msix_write(struct vcpu *v, un
>               * data fields Xen needs to disable and enable the entry in order
>               * to pick up the changes.
>               */
> -            if ( update_entry(entry, pdev, vmsix_entry_nr(msix, entry)) )
> -                break;
> -
> -            entry->updated = false;
> +            update_entry(entry, pdev, vmsix_entry_nr(msix, entry));
>          }

You can also drop this braces now if you feel like.

Thanks, Roger.

[PATCH 4/5] vPCI/MSI-X: make use of xzalloc_flex_struct()

[Jan Beulich]

... instead of effectively open-coding it in a type-unsafe way.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -23,8 +23,6 @@
 #include <asm/msi.h>
 #include <asm/p2m.h>
 
-#define VMSIX_SIZE(num) offsetof(struct vpci_msix, entries[num])
-
 #define VMSIX_ADDR_IN_RANGE(addr, vpci, nr)                               \
     ((addr) >= vmsix_table_addr(vpci, nr) &&                              \
      (addr) < vmsix_table_addr(vpci, nr) + vmsix_table_size(vpci, nr))
@@ -449,7 +447,8 @@ static int init_msix(struct pci_dev *pde
 
     max_entries = msix_table_size(control);
 
-    pdev->vpci->msix = xzalloc_bytes(VMSIX_SIZE(max_entries));
+    pdev->vpci->msix = xzalloc_flex_struct(struct vpci_msix, entries,
+                                           max_entries);
     if ( !pdev->vpci->msix )
         return -ENOMEM;
 

Re: [PATCH 4/5] vPCI/MSI-X: make use of xzalloc_flex_struct()

[Roger Pau Monné]

On Mon, Dec 07, 2020 at 11:38:21AM +0100, Jan Beulich wrote:
> ... instead of effectively open-coding it in a type-unsafe way.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

[PATCH 5/5] vPCI/MSI-X: tidy init_msix()

[Jan Beulich]

First of all introduce a local variable for the to be allocated struct.
The compiler can't CSE all the occurrences (I'm observing 80 bytes of
code saved with gcc 10). Additionally, while the caller can cope and
there was no memory leak, globally "announce" the struct only once done
initializing it. This also removes the dependency of the function on
the caller cleaning up after it in case of an error.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I was heavily tempted to also move up the call to vpci_add_register(),
such that there would be no pointless init done in case of an error
coming back from there.

--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -436,6 +436,7 @@ static int init_msix(struct pci_dev *pde
     uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
     unsigned int msix_offset, i, max_entries;
     uint16_t control;
+    struct vpci_msix *msix;
     int rc;
 
     msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
@@ -447,34 +448,37 @@ static int init_msix(struct pci_dev *pde
 
     max_entries = msix_table_size(control);
 
-    pdev->vpci->msix = xzalloc_flex_struct(struct vpci_msix, entries,
-                                           max_entries);
-    if ( !pdev->vpci->msix )
+    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
+    if ( !msix )
         return -ENOMEM;
 
-    pdev->vpci->msix->max_entries = max_entries;
-    pdev->vpci->msix->pdev = pdev;
+    msix->max_entries = max_entries;
+    msix->pdev = pdev;
 
-    pdev->vpci->msix->tables[VPCI_MSIX_TABLE] =
+    msix->tables[VPCI_MSIX_TABLE] =
         pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
-    pdev->vpci->msix->tables[VPCI_MSIX_PBA] =
+    msix->tables[VPCI_MSIX_PBA] =
         pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
 
-    for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
+    for ( i = 0; i < msix->max_entries; i++)
     {
-        pdev->vpci->msix->entries[i].masked = true;
-        vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
+        msix->entries[i].masked = true;
+        vpci_msix_arch_init_entry(&msix->entries[i]);
     }
 
     rc = vpci_add_register(pdev->vpci, control_read, control_write,
-                           msix_control_reg(msix_offset), 2, pdev->vpci->msix);
+                           msix_control_reg(msix_offset), 2, msix);
     if ( rc )
+    {
+        xfree(msix);
         return rc;
+    }
 
     if ( list_empty(&d->arch.hvm.msix_tables) )
         register_mmio_handler(d, &vpci_msix_table_ops);
 
-    list_add(&pdev->vpci->msix->next, &d->arch.hvm.msix_tables);
+    pdev->vpci->msix = msix;
+    list_add(&msix->next, &d->arch.hvm.msix_tables);
 
     return 0;
 }

Re: [PATCH 5/5] vPCI/MSI-X: tidy init_msix()

[Roger Pau Monné]

On Mon, Dec 07, 2020 at 11:38:42AM +0100, Jan Beulich wrote:
> First of all introduce a local variable for the to be allocated struct.
> The compiler can't CSE all the occurrences (I'm observing 80 bytes of
> code saved with gcc 10). Additionally, while the caller can cope and
> there was no memory leak, globally "announce" the struct only once done
> initializing it. This also removes the dependency of the function on
> the caller cleaning up after it in case of an error.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just a couple of comments.

> ---
> I was heavily tempted to also move up the call to vpci_add_register(),
> such that there would be no pointless init done in case of an error
> coming back from there.

Feel free to do so.

> 
> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -436,6 +436,7 @@ static int init_msix(struct pci_dev *pde
>      uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
>      unsigned int msix_offset, i, max_entries;
>      uint16_t control;
> +    struct vpci_msix *msix;
>      int rc;
>  
>      msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
> @@ -447,34 +448,37 @@ static int init_msix(struct pci_dev *pde
>  
>      max_entries = msix_table_size(control);
>  
> -    pdev->vpci->msix = xzalloc_flex_struct(struct vpci_msix, entries,
> -                                           max_entries);
> -    if ( !pdev->vpci->msix )
> +    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
> +    if ( !msix )
>          return -ENOMEM;
>  
> -    pdev->vpci->msix->max_entries = max_entries;
> -    pdev->vpci->msix->pdev = pdev;
> +    msix->max_entries = max_entries;
> +    msix->pdev = pdev;
>  
> -    pdev->vpci->msix->tables[VPCI_MSIX_TABLE] =
> +    msix->tables[VPCI_MSIX_TABLE] =
>          pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
> -    pdev->vpci->msix->tables[VPCI_MSIX_PBA] =
> +    msix->tables[VPCI_MSIX_PBA] =
>          pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
>  
> -    for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
> +    for ( i = 0; i < msix->max_entries; i++)

Feel free to just use max_entries directly here.

>      {
> -        pdev->vpci->msix->entries[i].masked = true;
> -        vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
> +        msix->entries[i].masked = true;

I think we should also set msix->entries[i].updated = true; for
correctness? Albeit this will never lead to a working configuration,
as the address field will be 0 and thus cause and error to trigger if
enabled without prior setup.

Maybe on a different patch anyway.

Thanks, Roger.

Re: [PATCH 5/5] vPCI/MSI-X: tidy init_msix()

[Jan Beulich]

On 28.12.2020 18:58, Roger Pau Monné wrote:
> On Mon, Dec 07, 2020 at 11:38:42AM +0100, Jan Beulich wrote:
>> First of all introduce a local variable for the to be allocated struct.
>> The compiler can't CSE all the occurrences (I'm observing 80 bytes of
>> code saved with gcc 10). Additionally, while the caller can cope and
>> there was no memory leak, globally "announce" the struct only once done
>> initializing it. This also removes the dependency of the function on
>> the caller cleaning up after it in case of an error.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

> Just a couple of comments.
> 
>> ---
>> I was heavily tempted to also move up the call to vpci_add_register(),
>> such that there would be no pointless init done in case of an error
>> coming back from there.
> 
> Feel free to do so.
> 
>>
>> --- a/xen/drivers/vpci/msix.c
>> +++ b/xen/drivers/vpci/msix.c
>> @@ -436,6 +436,7 @@ static int init_msix(struct pci_dev *pde
>>      uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
>>      unsigned int msix_offset, i, max_entries;
>>      uint16_t control;
>> +    struct vpci_msix *msix;
>>      int rc;
>>  
>>      msix_offset = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
>> @@ -447,34 +448,37 @@ static int init_msix(struct pci_dev *pde
>>  
>>      max_entries = msix_table_size(control);
>>  
>> -    pdev->vpci->msix = xzalloc_flex_struct(struct vpci_msix, entries,
>> -                                           max_entries);
>> -    if ( !pdev->vpci->msix )
>> +    msix = xzalloc_flex_struct(struct vpci_msix, entries, max_entries);
>> +    if ( !msix )
>>          return -ENOMEM;
>>  
>> -    pdev->vpci->msix->max_entries = max_entries;
>> -    pdev->vpci->msix->pdev = pdev;
>> +    msix->max_entries = max_entries;
>> +    msix->pdev = pdev;
>>  
>> -    pdev->vpci->msix->tables[VPCI_MSIX_TABLE] =
>> +    msix->tables[VPCI_MSIX_TABLE] =
>>          pci_conf_read32(pdev->sbdf, msix_table_offset_reg(msix_offset));
>> -    pdev->vpci->msix->tables[VPCI_MSIX_PBA] =
>> +    msix->tables[VPCI_MSIX_PBA] =
>>          pci_conf_read32(pdev->sbdf, msix_pba_offset_reg(msix_offset));
>>  
>> -    for ( i = 0; i < pdev->vpci->msix->max_entries; i++)
>> +    for ( i = 0; i < msix->max_entries; i++)
> 
> Feel free to just use max_entries directly here.
> 
>>      {
>> -        pdev->vpci->msix->entries[i].masked = true;
>> -        vpci_msix_arch_init_entry(&pdev->vpci->msix->entries[i]);
>> +        msix->entries[i].masked = true;
> 
> I think we should also set msix->entries[i].updated = true; for
> correctness? Albeit this will never lead to a working configuration,
> as the address field will be 0 and thus cause and error to trigger if
> enabled without prior setup.
> 
> Maybe on a different patch anyway.

Indeed, I'd prefer to not make such a change right here.

Jan

[PATCH v2] vPCI/MSI-X: fold clearing of entry->updated

[Jan Beulich]

Both call sites clear the flag after a successfull call to
update_entry(). This can be simplified by moving the clearing into the
function, onto its success path.

As a result of neither caller caring about update_entry()'s return value
anymore, the function gets switched to return void.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Make update_entry() return void.

--- a/xen/drivers/vpci/msix.c
+++ b/xen/drivers/vpci/msix.c
@@ -37,8 +37,8 @@ static uint32_t control_read(const struc
            (msix->masked ? PCI_MSIX_FLAGS_MASKALL : 0);
 }
 
-static int update_entry(struct vpci_msix_entry *entry,
-                        const struct pci_dev *pdev, unsigned int nr)
+static void update_entry(struct vpci_msix_entry *entry,
+                         const struct pci_dev *pdev, unsigned int nr)
 {
     int rc = vpci_msix_arch_disable_entry(entry, pdev);
 
@@ -48,7 +48,7 @@ static int update_entry(struct vpci_msix
         gprintk(XENLOG_WARNING,
                 "%pp: unable to disable entry %u for update: %d\n",
                 &pdev->sbdf, nr, rc);
-        return rc;
+        return;
     }
 
     rc = vpci_msix_arch_enable_entry(entry, pdev,
@@ -59,10 +59,10 @@ static int update_entry(struct vpci_msix
         gprintk(XENLOG_WARNING, "%pp: unable to enable entry %u: %d\n",
                 &pdev->sbdf, nr, rc);
         /* Entry is likely not properly configured. */
-        return rc;
+        return;
     }
 
-    return 0;
+    entry->updated = false;
 }
 
 static void control_write(const struct pci_dev *pdev, unsigned int reg,
@@ -90,13 +90,8 @@ static void control_write(const struct p
     if ( new_enabled && !new_masked && (!msix->enabled || msix->masked) )
     {
         for ( i = 0; i < msix->max_entries; i++ )
-        {
-            if ( msix->entries[i].masked || !msix->entries[i].updated ||
-                 update_entry(&msix->entries[i], pdev, i) )
-                continue;
-
-            msix->entries[i].updated = false;
-        }
+            if ( !msix->entries[i].masked && msix->entries[i].updated )
+                update_entry(&msix->entries[i], pdev, i);
     }
     else if ( !new_enabled && msix->enabled )
     {
@@ -363,10 +358,7 @@ static int msix_write(struct vcpu *v, un
              * data fields Xen needs to disable and enable the entry in order
              * to pick up the changes.
              */
-            if ( update_entry(entry, pdev, vmsix_entry_nr(msix, entry)) )
-                break;
-
-            entry->updated = false;
+            update_entry(entry, pdev, vmsix_entry_nr(msix, entry));
         }
         else
             vpci_msix_arch_mask_entry(entry, pdev, entry->masked);

Re: [PATCH v2] vPCI/MSI-X: fold clearing of entry->updated

[Roger Pau Monné]

On Tue, Jan 05, 2021 at 01:39:54PM +0100, Jan Beulich wrote:
> Both call sites clear the flag after a successfull call to
> update_entry(). This can be simplified by moving the clearing into the
> function, onto its success path.
> 
> As a result of neither caller caring about update_entry()'s return value
> anymore, the function gets switched to return void.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.