| Message ID | 6468c44a-772e-45a6-b712-0732c9017234@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VT-d: set_msi_source_id() (XSA-467 follow-up) | expand |
On 13/03/2025 1:34 pm, Jan Beulich wrote: > Handling possible internal errors by just emitting a (debug-build-only) > log message can't be quite enough. Return error codes in those cases, > and have the caller propagate those up. > > Drop a pointless return path, rather than "inventing" an error code for > it. > > While touching the function declarator anyway also constify its first > parameter. > > Fixes: 476bbccc811c ("VT-d: fix MSI source-id of interrupt remapping") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > > --- a/xen/drivers/passthrough/vtd/intremap.c > +++ b/xen/drivers/passthrough/vtd/intremap.c > @@ -436,15 +436,13 @@ void cf_check io_apic_write_remap_rte( > __ioapic_write_entry(apic, pin, true, old_rte); > } > > -static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire) > +static int set_msi_source_id(const struct pci_dev *pdev, > + struct iremap_entry *ire) > { > u16 seg; > u8 bus, devfn, secbus; > int ret; > > - if ( !pdev || !ire ) > - return; > - > seg = pdev->seg; > bus = pdev->bus; > devfn = pdev->devfn; > @@ -485,16 +483,21 @@ static void set_msi_source_id(struct pci > PCI_BDF(bus, devfn)); > } > else > + { > dprintk(XENLOG_WARNING VTDPREFIX, > "d%d: no upstream bridge for %pp\n", > pdev->domain->domain_id, &pdev->sbdf); as you're doing cleanup, %pd here? Given DOM_IO for quarantine, I think it's more likely now than it used to be. > + return -ENXIO; > + } > break; > > default: > dprintk(XENLOG_WARNING VTDPREFIX, "d%d: unknown(%u): %pp\n", > pdev->domain->domain_id, pdev->type, &pdev->sbdf); Here too. Also, the "unknown(%u)" is less than ideal. "%pd: %pp unknown type %u\n" would be better. ~Andrew
On 13.03.2025 16:01, Andrew Cooper wrote: > On 13/03/2025 1:34 pm, Jan Beulich wrote: >> Handling possible internal errors by just emitting a (debug-build-only) >> log message can't be quite enough. Return error codes in those cases, >> and have the caller propagate those up. >> >> Drop a pointless return path, rather than "inventing" an error code for >> it. >> >> While touching the function declarator anyway also constify its first >> parameter. >> >> Fixes: 476bbccc811c ("VT-d: fix MSI source-id of interrupt remapping") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, Thanks. > although... > >> --- a/xen/drivers/passthrough/vtd/intremap.c >> +++ b/xen/drivers/passthrough/vtd/intremap.c >> @@ -436,15 +436,13 @@ void cf_check io_apic_write_remap_rte( >> __ioapic_write_entry(apic, pin, true, old_rte); >> } >> >> -static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire) >> +static int set_msi_source_id(const struct pci_dev *pdev, >> + struct iremap_entry *ire) >> { >> u16 seg; >> u8 bus, devfn, secbus; >> int ret; >> >> - if ( !pdev || !ire ) >> - return; >> - >> seg = pdev->seg; >> bus = pdev->bus; >> devfn = pdev->devfn; >> @@ -485,16 +483,21 @@ static void set_msi_source_id(struct pci >> PCI_BDF(bus, devfn)); >> } >> else >> + { >> dprintk(XENLOG_WARNING VTDPREFIX, >> "d%d: no upstream bridge for %pp\n", >> pdev->domain->domain_id, &pdev->sbdf); > > as you're doing cleanup, %pd here? Given DOM_IO for quarantine, I think > it's more likely now than it used to be. > >> + return -ENXIO; >> + } >> break; >> >> default: >> dprintk(XENLOG_WARNING VTDPREFIX, "d%d: unknown(%u): %pp\n", >> pdev->domain->domain_id, pdev->type, &pdev->sbdf); > > Here too. Also, the "unknown(%u)" is less than ideal. "%pd: %pp > unknown type %u\n" would be better. To be honest this feels like going too far with unrelated cleanup here, even if it's all in patch context. It would be different if at least I touched those dprintk() invocations. Jan
--- a/xen/drivers/passthrough/vtd/intremap.c +++ b/xen/drivers/passthrough/vtd/intremap.c @@ -436,15 +436,13 @@ void cf_check io_apic_write_remap_rte( __ioapic_write_entry(apic, pin, true, old_rte); } -static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire) +static int set_msi_source_id(const struct pci_dev *pdev, + struct iremap_entry *ire) { u16 seg; u8 bus, devfn, secbus; int ret; - if ( !pdev || !ire ) - return; - seg = pdev->seg; bus = pdev->bus; devfn = pdev->devfn; @@ -485,16 +483,21 @@ static void set_msi_source_id(struct pci PCI_BDF(bus, devfn)); } else + { dprintk(XENLOG_WARNING VTDPREFIX, "d%d: no upstream bridge for %pp\n", pdev->domain->domain_id, &pdev->sbdf); + return -ENXIO; + } break; default: dprintk(XENLOG_WARNING VTDPREFIX, "d%d: unknown(%u): %pp\n", pdev->domain->domain_id, pdev->type, &pdev->sbdf); - break; - } + return -EOPNOTSUPP; + } + + return 0; } static int msi_msg_to_remap_entry( @@ -509,7 +512,12 @@ static int msi_msg_to_remap_entry( bool alloc = false; if ( pdev ) - set_msi_source_id(pdev, &new_ire); + { + int rc = set_msi_source_id(pdev, &new_ire); + + if ( rc ) + return rc; + } else set_hpet_source_id(msi_desc->hpet_id, &new_ire);
Handling possible internal errors by just emitting a (debug-build-only) log message can't be quite enough. Return error codes in those cases, and have the caller propagate those up. Drop a pointless return path, rather than "inventing" an error code for it. While touching the function declarator anyway also constify its first parameter. Fixes: 476bbccc811c ("VT-d: fix MSI source-id of interrupt remapping") Signed-off-by: Jan Beulich <jbeulich@suse.com>