|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 09/11] vpci/msi: add MSI handlers
>>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> wrote:
> +static unsigned int msi_flags(uint16_t data, uint64_t addr)
> +{
> + unsigned int rh, dm, dest_id, deliv_mode, trig_mode;
> +
> + rh = MASK_EXTR(addr, MSI_ADDR_REDIRECTION_MASK);
> + dm = MASK_EXTR(addr, MSI_ADDR_DESTMODE_MASK);
> + dest_id = MASK_EXTR(addr, MSI_ADDR_DEST_ID_MASK);
> + deliv_mode = MASK_EXTR(data, MSI_DATA_DELIVERY_MODE_MASK);
> + trig_mode = MASK_EXTR(data, MSI_DATA_TRIGGER_MASK);
> +
> + return (dest_id << GFLAGS_SHIFT_DEST_ID) | (rh << GFLAGS_SHIFT_RH) |
> + (dm << GFLAGS_SHIFT_DM) | (deliv_mode << GFLAGS_SHIFT_DELIV_MODE)
> |
> + (trig_mode << GFLAGS_SHIFT_TRG_MODE);
This would need re-basing over the removal of those GFLAGS_*
values anyway, but do you really mean those? There's no domctl
involved here I think, and hence I think you rather mean the x86
architecture mandated macros found in asm-x86/msi.h. Or wait,
no, I've just found the caller - you instead want to name the
function msi_gflags() and perhaps add comment on why you
need to use the domctl constants here.
> +void vpci_msi_arch_mask(struct vpci_arch_msi *arch, struct pci_dev *pdev,
Presumably constification of pdev here and elsewhere will come as
a result of doing so at the callback layer. If not, helper functions
not needing to alter it should have it const nevertheless.
> + unsigned int entry, bool mask)
> +{
> + struct domain *d = pdev->domain;
This one probably can be const too.
> +int vpci_msi_arch_enable(struct vpci_arch_msi *arch, struct pci_dev *pdev,
> + uint64_t address, uint32_t data, unsigned int
> vectors)
> +{
> + struct msi_info msi_info = {
> + .seg = pdev->seg,
> + .bus = pdev->bus,
> + .devfn = pdev->devfn,
> + .entry_nr = vectors,
> + };
> + unsigned int i;
> + int rc;
> +
> + ASSERT(arch->pirq == INVALID_PIRQ);
> +
> + /* Get a PIRQ. */
> + rc = allocate_and_map_msi_pirq(pdev->domain, -1, &arch->pirq,
> + MAP_PIRQ_TYPE_MULTI_MSI, &msi_info);
> + if ( rc )
> + {
> + gdprintk(XENLOG_ERR, "%04x:%02x:%02x.%u: failed to map PIRQ: %d\n",
> + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), rc);
> + return rc;
> + }
> +
> + for ( i = 0; i < vectors; i++ )
> + {
> + xen_domctl_bind_pt_irq_t bind = {
> + .machine_irq = arch->pirq + i,
> + .irq_type = PT_IRQ_TYPE_MSI,
> + .u.msi.gvec = msi_vector(data) + i,
Isn't that rather msi_vector(data + i), i.e. wouldn't you better
increment data together with i?
> + .u.msi.gflags = msi_flags(data, address),
> + };
> +
> + pcidevs_lock();
> + rc = pt_irq_create_bind(pdev->domain, &bind);
> + if ( rc )
> + {
> + gdprintk(XENLOG_ERR,
> + "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n",
> + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), arch->pirq + i, rc);
> + while ( bind.machine_irq-- )
> + pt_irq_destroy_bind(pdev->domain, &bind);
> + spin_lock(&pdev->domain->event_lock);
> + unmap_domain_pirq(pdev->domain, arch->pirq);
> + spin_unlock(&pdev->domain->event_lock);
> + pcidevs_unlock();
> + arch->pirq = -1;
INVALID_PIRQ ?
> +int vpci_msi_arch_disable(struct vpci_arch_msi *arch, struct pci_dev *pdev,
> + unsigned int vectors)
> +{
> + unsigned int i;
> +
> + ASSERT(arch->pirq != INVALID_PIRQ);
> +
> + pcidevs_lock();
> + for ( i = 0; i < vectors; i++ )
> + {
> + xen_domctl_bind_pt_irq_t bind = {
> + .machine_irq = arch->pirq + i,
> + .irq_type = PT_IRQ_TYPE_MSI,
> + };
> + int rc;
> +
> + rc = pt_irq_destroy_bind(pdev->domain, &bind);
> + gdprintk(XENLOG_ERR,
> + "%04x:%02x:%02x.%u: failed to unbind PIRQ %u: %d\n",
> + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), arch->pirq + i, rc);
Couldn't this be ASSERT(!rc)? Afaics all error paths in that function
are due to passing in invalid state or information.
> +static int vpci_msi_disable(struct pci_dev *pdev, struct vpci_msi *msi)
> +{
> + int ret;
> +
> + ASSERT(msi->enabled);
> + __msi_set_enable(pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
> + PCI_FUNC(pdev->devfn), msi->pos, 0);
> +
> + ret = vpci_msi_arch_disable(&msi->arch, pdev, msi->vectors);
> + if ( ret )
> + return ret;
> +
> + msi->enabled = false;
> +
> + return 0;
> +}
Please invert the if() condition and have both cases reach the final
return.
> +static void vpci_msi_control_write(struct pci_dev *pdev, unsigned int reg,
> + uint32_t val, void *data)
> +{
> + struct vpci_msi *msi = data;
> + unsigned int vectors = 1 << MASK_EXTR(val, PCI_MSI_FLAGS_QSIZE);
> + bool new_enabled = val & PCI_MSI_FLAGS_ENABLE;
> +
> + if ( vectors > msi->max_vectors )
> + vectors = msi->max_vectors;
> +
> + /*
> + * No change in the enable field and the number of vectors is
s/ in / if / ?
> +static int vpci_init_msi(struct pci_dev *pdev)
> +{
> + uint8_t seg = pdev->seg, bus = pdev->bus;
> + uint8_t slot = PCI_SLOT(pdev->devfn), func = PCI_FUNC(pdev->devfn);
> + struct vpci_msi *msi;
> + unsigned int pos;
> + uint16_t control;
> + int ret;
> +
> + pos = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
> + if ( !pos )
> + return 0;
> +
> + msi = xzalloc(struct vpci_msi);
> + if ( !msi )
> + return -ENOMEM;
> +
> + msi->pos = pos;
> +
> + ret = vpci_add_register(pdev, vpci_msi_control_read,
> + vpci_msi_control_write,
> + msi_control_reg(pos), 2, msi);
> + if ( ret )
> + {
> + xfree(msi);
> + return ret;
> + }
> +
> + /* Get the maximum number of vectors the device supports. */
> + control = pci_conf_read16(seg, 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->arch);
I think it is generally a good idea to hand the entire thing to the
arch hoot, not just the ->arch portion. The hook may want to
read other fields in order to establish the per-arch ones. Just
think of something depending on the maximum vector count.
> + msi->address64 = is_64bit_address(control) ? true : false;
> + msi->masking = is_mask_bit_support(control) ? true : false;
What are these conditional operators good for?
> +}
> +
> +REGISTER_VPCI_INIT(vpci_init_msi);
Btw, it's certainly a matter of taste, but would you mind leaving out
the blank line between the function and such registration macro
invocations? We commonly (but not fully uniformly) do so e.g. for
custom_param() invocations, too.
> +void vpci_dump_msi(void)
> +{
> + struct domain *d;
> +
> + for_each_domain ( d )
> + {
> + const struct pci_dev *pdev;
> +
> + if ( !has_vpci(d) )
> + continue;
> +
> + printk("vPCI MSI information for d%d\n", d->domain_id);
> +
> + if ( !vpci_tryrlock(d) )
> + {
> + printk("Unable to get vPCI lock, skipping\n");
> + continue;
> + }
> +
> + list_for_each_entry ( pdev, &d->arch.pdev_list, domain_list )
> + {
> + uint8_t seg = pdev->seg, bus = pdev->bus;
> + uint8_t slot = PCI_SLOT(pdev->devfn), func =
> PCI_FUNC(pdev->devfn);
> + const struct vpci_msi *msi = pdev->vpci->msi;
> +
> + if ( msi )
> + {
> + printk("Device %04x:%02x:%02x.%u\n", seg, bus, slot, func);
> +
> + printk(" Enabled: %u Supports masking: %u 64-bit addresses:
> %u\n",
> + msi->enabled, msi->masking, msi->address64);
> + printk(" Max vectors: %u enabled vectors: %u\n",
> + msi->max_vectors, msi->vectors);
> +
> + vpci_msi_arch_print(&msi->arch, msi->data, msi->address);
> +
> + if ( msi->masking )
> + printk(" mask=%08x\n", msi->mask);
> + }
> + }
> + vpci_runlock(d);
> + }
> +}
There no process_pending_softirqs() in here, despite the nested
loops potentially being long lasting.
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -133,6 +133,7 @@ struct pirq {
> struct arch_pirq arch;
> };
>
> +#define INVALID_PIRQ -1
This needs parentheses.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |