lkml.org 
[lkml]   [2019]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] xen-pciback: optionally allow interrupt enable flag writes
On Thu, Dec 19, 2019 at 12:20:24PM +0100, Jan Beulich wrote:
> On 19.12.2019 04:49, Marek Marczykowski-Górecki wrote:
> > +enum interrupt_type xen_pcibk_get_interrupt_type(struct pci_dev *dev)
> > +{
> > + int err;
> > + u16 val;
> > +
> > + err = pci_read_config_word(dev, PCI_COMMAND, &val);
> > + if (err)
> > + return INTERRUPT_TYPE_ERR;
> > + if (!(val & PCI_COMMAND_INTX_DISABLE))
> > + return INTERRUPT_TYPE_INTX;
> > +
> > + /* Do not trust dev->msi(x)_enabled here, as enabling could be done
> > + * bypassing the pci_*msi* functions, by the qemu.
> > + */
>
> Judging from this comment, how can you assume only one of the
> three variants is actually enabled? It's against the spec, yes,
> but it's not at all impossible afaict. I think you want the
> return value here to be
> - negative errno values (no need to discard the actual error
> codes) or
> - a non-negative bitmap indicating which of the interrupt types
> is/are currently enabled.

Good idea, I'll change that.

> That way ...
>
> > +static int msi_msix_flags_write(struct pci_dev *dev, int offset, u16 new_value,
> > + void *data)
> > +{
> > + int err;
> > + u16 old_value;
> > + const struct msi_msix_field_config *field_config = data;
> > + const struct xen_pcibk_dev_data *dev_data = pci_get_drvdata(dev);
> > +
> > + if (xen_pcibk_permissive || dev_data->permissive)
> > + goto write;
> > +
> > + err = pci_read_config_word(dev, offset, &old_value);
> > + if (err)
> > + return err;
> > +
> > + if (new_value == old_value)
> > + return 0;
> > +
> > + if (!dev_data->allow_interrupt_control ||
> > + (new_value ^ old_value) & ~field_config->enable_bit)
> > + return PCIBIOS_SET_FAILED;
> > +
> > + if (new_value & field_config->enable_bit) {
> > + /* don't allow enabling together with other interrupt types */
> > + const enum interrupt_type int_type = xen_pcibk_get_interrupt_type(dev);
> > + if (int_type == INTERRUPT_TYPE_NONE ||
> > + int_type == field_config->int_type)
>
> ... equality comparisons like this one will actually become safe.
>
> Jan

--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2019-12-19 13:58    [W:0.201 / U:5.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site