Re: [Xen-devel] [PATCH v2] xen-pciback: optionally allow interrupt enable flag writes

[Jan Beulich <jbeulich@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel