[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c0e27fbe-e2f7-22ca-c3f4-bafb252c7bcc@suse.com>
Date: Thu, 19 Dec 2019 12:20:24 +0100
From: Jan Beulich <jbeulich@...e.com>
To: Marek Marczykowski-Górecki
<marmarek@...isiblethingslab.com>
Cc: xen-devel@...ts.xenproject.org,
Roger Pau Monné <roger.pau@...rix.com>,
YueHaibing <yuehaibing@...wei.com>,
Simon Gaiser <simon@...isiblethingslab.com>,
Stefano Stabellini <sstabellini@...nel.org>,
Boris Ostrovsky <boris.ostrovsky@...cle.com>,
Juergen Gross <jgross@...e.com>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] xen-pciback: optionally allow interrupt enable flag
writes
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.
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
Powered by blists - more mailing lists