Re: [Xen-devel] [PATCH v4 5/6] xen/x86: add PHYSDEVOP_msi_set_enable

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.02.19 at 16:05, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 27, 2019 at 04:41:37AM -0700, Jan Beulich wrote:
>> >>> On 07.02.19 at 01:07, <marmarek@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>> > +int msi_msix_set_enable(struct pci_dev *pdev, int mode, int enable)
>> > +{
>> > +    int ret;
>> > +
>> > +    ret = xsm_msi_set_enable(XSM_DM_PRIV, pdev->domain,
>> > +                             (pdev->seg << 16) | (pdev->bus << 8) | 
>> > pdev->devfn,
>> > +                             mode, enable);
>> > +    if ( ret )
>> > +        return ret;
>> > +
>> > +    switch ( mode )
>> > +    {
>> > +    case PHYSDEVOP_MSI_SET_ENABLE_MSI:
>> > +        msi_set_enable(pdev, enable);
>> > +        break;
>> > +
>> > +    case PHYSDEVOP_MSI_SET_ENABLE_MSIX:
>> > +        msix_set_enable(pdev, enable);
>> > +        break;
>> > +    }
>> 
>> What about a call to pci_intx()? 
> 
> Should pci_intx(dev, !enable) be called in all those cases?

Well, that depends whether Dom0 is involved, which is where the
operation would normally be done. But since this is about bypassing
pciback, I think it may be needed.

>> And what about invocations for
>> the wrong device (e.g. MSI-X request for MSI-X incapable device)?
> 
> Looking at msi(x)_set_enable(), it is no-op for incapable devices, but
> if the function would do anything else, indeed such check should be
> added. Is pci_find_cap_offset(..., PCI_CAP_ID_MSI(X)) the correct way
> of doing that?

Well, for MSI-X you could simply check pdev->msix to be non-NULL.
For MSI I think looking for the capability is your only choice.

Another thing: You're also bypassing the MSI{,-X}-already-enabled
checks that __pci_enable_msi{,x}() do, yet allowing to enable both
on a device would be a security issue.

>> > +    /* IN */
>> > +    uint16_t seg;
>> > +    uint8_t bus;
>> > +    uint8_t devfn;
>> > +    uint8_t mode;
>> > +    uint8_t enable;
>> 
>> "mode" and "enable" don't really make clear which of the two is the
>> boolean, and which is the operation. I'd anyway prefer a single
>> flags field with descriptive #define-s, which will also make more
>> obvious how to extend this if need be.
> 
> You mean:
> 
> #define PHYSDEVOP_MSI_CONTROL_ENABLE 1
> #define PHYSDEVOP_MSI_CONTROL_MSI    2 
> #define PHYSDEVOP_MSI_CONTROL_MSIX   4

Not exactly - you need just two flags: One selecting between
enable and disable, and a second selecting between MSI and
MSI-X. Otherwise, in your model, what do 0 or ENABLE alone
mean?

Jan



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