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
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,
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.
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
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 --
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
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.
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);
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.
... 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;
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.
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; }
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.
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
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);
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.
© 2016 - 2024 Red Hat, Inc.