Re: [Xen-devel] [PATCH v8 09/11] vpci/msi: add MSI handlers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.01.18 at 16:07, <roger.pau@xxxxxxxxxx> wrote:
> Add handlers for the MSI control, address, data and mask fields in
> order to detect accesses to them and setup the interrupts as requested
> by the guest.
> 
> Note that the pending register is not trapped, and the guest can
> freely read/write to it.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Paul Durrant <paul.durrant@xxxxxxxxxx>
> ---
> Changes since v7:
>  - Don't store pci segment/bus on local variables.
>  - Add an error label to init_msi.
>  - Don't trap accesses to the PBA.
>  - Fix msi_pending_bits_reg macro so it matches coding style.
>  - Move the position of vectors in the vpci_msi struct.
>  - Add a comment to clarify the expected state of vectors after
>    pt_irq_create_bind and use XEN_DOMCTL_VMSI_X86_UNMASKED.

And this long list did not invalidate Paul's R-b?

> NB: I've only been able to test this with devices using a single MSI interrupt
> and no mask register. I will try to find hardware that supports the mask
> register and more than one vector, but I cannot make any promises.
> 
> If there are doubts about the untested parts we could always force Xen to
> report no per-vector masking support and only 1 available vector, but I would
> rather avoid doing it.

Has this become stale meanwhile, or is this still untested? In any
event, I agree we shouldn't add hacks, but there should be a
record somewhere of things needing to be done (here: tested)
before the whole Dom0 thing can be marked supported. Perhaps
just a fixme-like comment in the code.

> +static int init_msi(struct pci_dev *pdev)
> +{
> +    uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> +    struct vpci_msi *msi;
> +    unsigned int pos = pci_find_cap_offset(pdev->seg, pdev->bus, slot, func,
> +                                           PCI_CAP_ID_MSI);
> +    uint16_t control;
> +    int ret;
> +
> +    if ( !pos )
> +        return 0;
> +
> +    msi = xzalloc(struct vpci_msi);
> +    if ( !msi )
> +        return -ENOMEM;
> +
> +    ret = vpci_add_register(pdev->vpci, control_read, control_write,
> +                            msi_control_reg(pos), 2, msi);
> +    if ( ret )
> +        goto error;
> +
> +    /* Get the maximum number of vectors the device supports. */
> +    control = pci_conf_read16(pdev->seg, pdev->bus, slot, func,
> +                              msi_control_reg(pos));
> +    msi->max_vectors = multi_msi_capable(control);
> +    ASSERT(msi->max_vectors <= 32);
> +
> +    /* The multiple message enable is 0 after reset (1 message enabled). */
> +    msi->vectors = 1;
> +
> +    /* No PIRQ bound yet. */
> +    vpci_msi_arch_init(msi);
> +
> +    msi->address64 = is_64bit_address(control);
> +    msi->masking = is_mask_bit_support(control);
> +
> +    ret = vpci_add_register(pdev->vpci, address_read, address_write,
> +                            msi_lower_address_reg(pos), 4, msi);
> +    if ( ret )
> +        goto error;
> +
> +    ret = vpci_add_register(pdev->vpci, data_read, data_write,
> +                            msi_data_reg(pos, msi->address64), 2,
> +                            msi);
> +    if ( ret )
> +        goto error;
> +
> +    if ( msi->address64 )
> +    {
> +        ret = vpci_add_register(pdev->vpci, address_hi_read, 
> address_hi_write,
> +                                msi_upper_address_reg(pos), 4, msi);
> +        if ( ret )
> +            goto error;
> +    }
> +
> +    if ( msi->masking )
> +    {
> +        ret = vpci_add_register(pdev->vpci, mask_read, mask_write,
> +                                msi_mask_bits_reg(pos, msi->address64), 4,
> +                                msi);
> +        if ( ret )
> +            goto error;
> +        /*
> +         * NB: do not add any handler for the pending bits for the hardware
> +         * domain, which means direct access. This will be revisited when
> +         * adding unprivileged domain support.
> +         */
> +    }
> +
> +    pdev->vpci->msi = msi;
> +
> +    return 0;
> +
> + error:
> +    ASSERT(ret);
> +    xfree(msi);
> +
> +    return ret;

Can you please add a short comment here making clear why all the
vpci_add_register() above don't need undoing here? Otherwise,
just like me every time I come across this, readers will get the
impression that there's something being leaked here.

With both of the comment additions taken care of
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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