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

[Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Feb 28, 2019 at 03:58:37AM -0700, Jan Beulich wrote:
> >>> 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.

Ok.

> >> > +    /* 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?

I put 3 flags there for easier extending it in the future.
But maybe indeed two flags + error on any other bit set would be enough
too.

-- 
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?

Attachment: signature.asc
Description: PGP signature

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