Re: [Xen-devel] [PATCH v6 5/6] xen/x86: add PHYSDEVOP_interrupt_control

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.09.2019 17:37, Marek Marczykowski-Górecki  wrote:
> Allow device model running in stubdomain to enable/disable INTx/MSI(-X),
> bypassing pciback. While pciback is still used to access config space
> from within stubdomain, it refuse to write to
> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE/PCI_COMMAND_INTX_DISABLE
> in non-permissive mode. Which is the right thing to do for PV domain
> (the main use case for pciback), as PV domain should use XEN_PCI_OP_*
> commands for that. Unfortunately those commands are not good for
> stubdomain use, as they configure MSI in dom0's kernel too, which should
> not happen for HVM domain.

Why the "for HVM domain" here? I.e. why would this be correct for
a PV domain? Besides my dislike for such a bypass (imo all of the
handling should go through pciback, or none of it) I continue to
wonder whether the problem can't be addressed by a pciback change.
And even if not, I'd still wonder whether the request shouldn't go
through pciback, to retain proper layering. Ultimately it may be
better to have even the map/unmap go through pciback (it's at
least an apparent violation of the original physdev-op model that
these two are XSM_DM_PRIV).

Irrespective of this a couple of comments on the patch itself:

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1443,6 +1443,51 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int msi_control(struct pci_dev *pdev, bool msix, bool enable)
> +{
> +    unsigned int cap = msix ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
> +    unsigned int other_cap = msix ? PCI_CAP_ID_MSI : PCI_CAP_ID_MSIX;
> +    uint16_t cmd;
> +
> +    if ( !use_msi )
> +        return -EOPNOTSUPP;
> +
> +    if ( !pci_find_cap_offset(pdev->seg,
> +                              pdev->bus,
> +                              PCI_SLOT(pdev->devfn),
> +                              PCI_FUNC(pdev->devfn),

Please don't use PCI_SLOT() and PCI_FUNC() anymore, now that we
have pdev->dev and pdev->fn. And please put multiple arguments
on one line, as many as will fit.

> +                              cap) )
> +        return -ENODEV;
> +
> +    cmd = pci_conf_read16(pdev->sbdf, PCI_COMMAND);
> +
> +    /* don't allow enabling MSI(-X) and INTx at the same time */
> +    if ( enable && ! (cmd & PCI_COMMAND_INTX_DISABLE) )

Stray blank after ! .

> +        return -EBUSY;
> +
> +    /* don't allow enabling both MSI and MSI-X at the same time */
> +    if ( enable && find_msi_entry(pdev, -1, other_cap) )
> +        return -EBUSY;

Combine both if()-s, since they both check "enable"?

Also - comment style again (should start with capital letter);
more instances elsewhere.

> +int intx_control(struct pci_dev *pdev, bool enable)
> +{
> +    /* don't allow enabling INTx if MSI(-X) is already enabled */
> +    if ( enable && find_msi_entry(pdev, -1, PCI_CAP_ID_MSI) )
> +        return -EBUSY;
> +    if ( enable && find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX) )
> +        return -EBUSY;

Here even more so you want to combine both if()-s.

> +    pci_intx(pdev, enable);
> +    return 0;
> +}

Blank line ahead of main return statement please, and I guess
another blank line ahead of the pci_intx() invocation wouldn't
hurt either.

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -662,6 +662,59 @@ ret_t do_physdev_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>  
> +    case PHYSDEVOP_interrupt_control: {
> +        struct physdev_interrupt_control op;
> +        struct pci_dev *pdev;
> +        int intr_type;
> +        bool enable;
> +
> +        ret = -EFAULT;
> +        if ( copy_from_guest(&op, arg, 1) )
> +            break;
> +
> +        ret = -EINVAL;
> +        if ( op.flags & ~(PHYSDEVOP_INTERRUPT_CONTROL_TYPE_MASK |
> +                          PHYSDEVOP_INTERRUPT_CONTROL_ENABLE) )
> +            break;
> +
> +        intr_type = op.flags & PHYSDEVOP_INTERRUPT_CONTROL_TYPE_MASK;
> +        enable = op.flags & PHYSDEVOP_INTERRUPT_CONTROL_ENABLE;
> +
> +        pcidevs_lock();
> +        pdev = pci_get_pdev(op.seg, op.bus, op.devfn);
> +        ret = -ENODEV;
> +        /* explicitly exclude hidden devices */
> +        if ( !pdev || pdev->domain == dom_xen )

The right side should be avoided by reducing the scope of the device
lookup right away, through use of pci_get_pdev_by_domain(). This
will also ensure we don't exclusively rely on the XSM check below to
prevent abuse of this operation. (FAOD, while
pci_get_pdev_by_domain() doesn't assert that the pcidevs lock is
held, you should still acquire it here. That missing ASSERT() should
get added as soon as other violators of the locking requirement have
been taken care of.)

> +            goto pci_unlock;
> +
> +        ret = xsm_interrupt_control(XSM_DM_PRIV,
> +                                    pdev->domain,
> +                                    pdev->sbdf.sbdf,
> +                                    intr_type,
> +                                    enable);
> +        if ( ret )
> +            goto pci_unlock;
> +
> +        switch ( intr_type )
> +        {
> +            case PHYSDEVOP_INTERRUPT_CONTROL_INTX:
> +                ret = intx_control(pdev, enable);
> +                break;
> +            case PHYSDEVOP_INTERRUPT_CONTROL_MSI:
> +                ret = msi_control(pdev, false, enable);
> +                break;
> +            case PHYSDEVOP_INTERRUPT_CONTROL_MSIX:
> +                ret = msi_control(pdev, true, enable);
> +                break;
> +            default:
> +                ret = -EINVAL;
> +                break;

Indentation and blank lines between independent case blocks please.

> +        }
> +pci_unlock:

Labels indented by at least one blank, please.

> --- a/xen/include/public/physdev.h
> +++ b/xen/include/public/physdev.h
> @@ -345,6 +345,29 @@ typedef struct physdev_dbgp_op physdev_dbgp_op_t;
>  DEFINE_XEN_GUEST_HANDLE(physdev_dbgp_op_t);
>  
>  /*
> + * Choose which interrupt type to control. If neither MSI nor MSI-X is 
> chosen,
> + * will apply to INTx - for convenience define 
> PHYSDEVOP_INTERRUPT_CONTROL_INTX
> + * and PHYSDEVOP_INTERRUPT_CONTROL_TYPE_MASK
> + */
> +#define PHYSDEVOP_INTERRUPT_CONTROL_TYPE_MASK 3
> +#define PHYSDEVOP_INTERRUPT_CONTROL_INTX      0
> +#define PHYSDEVOP_INTERRUPT_CONTROL_MSI       1
> +#define PHYSDEVOP_INTERRUPT_CONTROL_MSIX      2
> +/* when PHYSDEVOP_INTERRUPT_CONTROL_ENABLE not set, disable */
> +#define PHYSDEVOP_INTERRUPT_CONTROL_ENABLE    4
> +
> +#define PHYSDEVOP_interrupt_control   32
> +struct physdev_interrupt_control {
> +    /* IN */
> +    uint16_t seg;
> +    uint8_t bus;
> +    uint8_t devfn;

Please re-use struct physdev_pci_device for these.

Jan

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