|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 7/9] vpci/msi: add MSI handlers
>>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/30/17 5:01 PM >>>
>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.
>
>Whether Xen is going to provide this functionality to Dom0 (MSI
>emulation) is controlled by the "msi" option in the dom0 field. When
>disabling this option Xen will hide the MSI capability structure from
>Dom0.
Isn't this last paragraph stale now?
>+void vpci_msi_arch_mask(struct vpci_arch_msi *arch, struct pci_dev *pdev,
>+ unsigned int entry, bool mask)
>+{
>+ struct domain *d = pdev->domain;
>+ const struct pirq *pinfo;
>+ struct irq_desc *desc;
>+ unsigned long flags;
>+ int irq;
>+
>+ ASSERT(arch->pirq >= 0);
>+ pinfo = pirq_info(d, arch->pirq + entry);
>+ ASSERT(pinfo);
>+
>+ irq = pinfo->arch.irq;
>+ ASSERT(irq < nr_irqs && irq >= 0);
>+
>+ desc = irq_to_desc(irq);
>+ ASSERT(desc);
I know the goal is Dom0 support only at this point, but nevertheless I think
we shouldn't have ASSERT()s in place which could trigger if Dom0
misbehaves (and which would all need to be audited if we were to extend
support to DomU): I'm not convinced all of the ones above could really only
trigger depending on Xen (mis)behavior.
>+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 == -1);
Please introduce a #define for the -1 here, to allow easily matching up
producer and consumer side(s).
>+ /* Get a PIRQ. */
>+ rc = allocate_and_map_msi_pirq(pdev->domain, -1, &arch->pirq,
>+ MAP_PIRQ_TYPE_MULTI_MSI, &msi_info);
>+ if ( rc )
>+ {
>+ dprintk(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,
>+ .u.msi.gflags = msi_flags(data, address),
>+ };
>+
>+ pcidevs_lock();
>+ rc = pt_irq_create_bind(pdev->domain, &bind);
>+ if ( rc )
>+ {
>+ dprintk(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);
>+ spin_lock(&pdev->domain->event_lock);
>+ unmap_domain_pirq(pdev->domain, arch->pirq);
Don't you also need to undo the pt_irq_create_bind() calls here for all prior
successful iterations?
>+int vpci_msi_arch_disable(struct vpci_arch_msi *arch, struct pci_dev *pdev,
>+ unsigned int vectors)
>+{
>+ unsigned int i;
>+
>+ ASSERT(arch->pirq != -1);
>+
>+ for ( i = 0; i < vectors; i++ )
>+ {
>+ xen_domctl_bind_pt_irq_t bind = {
>+ .machine_irq = arch->pirq + i,
>+ .irq_type = PT_IRQ_TYPE_MSI,
>+ };
>+
>+ pcidevs_lock();
>+ pt_irq_destroy_bind(pdev->domain, &bind);
While I agree that the loop should continue of this fails, I'm not convinced
you should entirely ignore the return value here.
>+ pcidevs_unlock();
>+ }
>+
>+ pcidevs_lock();
What good does it do to acquire the lock for most of the loop body as well
as for most of the epilogue, instead of just acquiring it once ahead of the
loop?
>+int vpci_msi_arch_init(struct vpci_arch_msi *arch)
>+{
>+ arch->pirq = -1;
>+ return 0;
>+}
At this point I think the function would better return void.
>+void vpci_msi_arch_print(struct vpci_arch_msi *arch, uint16_t data,
const
>+ uint64_t addr)
>+{
>+ printk("vec=%#02x%7s%6s%3sassert%5s%7s dest_id=%lu pirq: %d\n",
>+ MASK_EXTR(data, MSI_DATA_VECTOR_MASK),
>+ data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
>+ data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
>+ data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
>+ addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
>+ addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : "cpu",
Why "cpu"? Elsewhere we call this mode "fixed".
>--- /dev/null
>+++ b/xen/drivers/vpci/msi.c
>@@ -0,0 +1,348 @@
>+/*
>+ * Handlers for accesses to the MSI capability structure.
>+ *
>+ * Copyright (C) 2017 Citrix Systems R&D
>+ *
>+ * This program is free software; you can redistribute it and/or
>+ * modify it under the terms and conditions of the GNU General Public
>+ * License, version 2, as published by the Free Software Foundation.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>+ * General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public
>+ * License along with this program; If not, see
><http://www.gnu.org/licenses/>.
>+ */
>+
>+#include <xen/sched.h>
>+#include <xen/vpci.h>
>+#include <asm/msi.h>
>+#include <xen/keyhandler.h>
Please don't mix xen/ and asm/ includes, and where possible please also
alphabetically sort each group.
>+/* Handlers for the MSI control field (PCI_MSI_FLAGS). */
>+static void vpci_msi_control_read(struct pci_dev *pdev, unsigned int reg,
>+ union vpci_val *val, void *data)
>+{
>+ const struct vpci_msi *msi = data;
>+
>+ /* Set multiple message capable. */
>+ val->u16 = MASK_INSR(fls(msi->max_vectors) - 1, PCI_MSI_FLAGS_QMASK);
The comment is somewhat misleading - whether the device is multi-message
capable depends on msi->max_vectors.
>+ if ( msi->enabled ) {
Style.
>+ val->u16 |= PCI_MSI_FLAGS_ENABLE;
>+ val->u16 |= MASK_INSR(fls(msi->vectors) - 1, PCI_MSI_FLAGS_QSIZE);
Why is reading back the proper value here dependent upon MSI being
enabled?
>+static void vpci_msi_control_write(struct pci_dev *pdev, unsigned int reg,
>+ union vpci_val val, void *data)
>+{
>+ struct vpci_msi *msi = data;
>+ unsigned int vectors = 1 << MASK_EXTR(val.u16, PCI_MSI_FLAGS_QSIZE);
>+ int ret;
>+
>+ if ( vectors > msi->max_vectors )
>+ vectors = msi->max_vectors;
>+
>+ if ( !!(val.u16 & PCI_MSI_FLAGS_ENABLE) == msi->enabled &&
>+ (vectors == msi->vectors || !msi->enabled) )
>+ return;
>+
>+ if ( val.u16 & PCI_MSI_FLAGS_ENABLE )
>+ {
>+ if ( msi->enabled )
>+ {
>+ /*
>+ * Change to the number of enabled vectors, disable and
>+ * enable MSI in order to apply it.
>+ */
At least the first part of the comment would appear to belong outside the
inner if().
>+ ret = vpci_msi_disable(pdev, msi);
>+ if ( ret )
>+ return;
Returning here without doing anything is at least strange, and hence
would call for a comment to be attached to explain the intentions.
>+static void vpci_msi_address_write(struct pci_dev *pdev, unsigned int reg,
>+ union vpci_val val, void *data)
>+{
>+ struct vpci_msi *msi = data;
>+
>+ /* Clear low part. */
>+ msi->address &= ~(uint64_t)0xffffffff;
~0xffffffffull?
>+static void vpci_msi_address_upper_write(struct pci_dev *pdev, unsigned int
>reg,
>+ union vpci_val val, void *data)
>+{
>+ struct vpci_msi *msi = data;
>+
>+ /* Clear high part. */
>+ msi->address &= ~((uint64_t)0xffffffff << 32);
Simply 0xffffffff?
+static void vpci_msi_mask_read(struct pci_dev *pdev, unsigned int reg,
Earlier read/write pairs had comments ahead of them - for consistency
one would then belong here too.
>+static void vpci_msi_mask_write(struct pci_dev *pdev, unsigned int reg,
>+ union vpci_val val, void *data)
>+{
>+ struct vpci_msi *msi = data;
>+ uint32_t dmask;
>+
>+ dmask = msi->mask ^ val.u32;
>+
>+ if ( !dmask )
>+ return;
>+
>+ if ( msi->enabled )
>+ {
>+ unsigned int i;
>+
>+ for ( i = ffs(dmask) - 1; dmask && i < msi->vectors;
>+ i = ffs(dmask) - 1 )
>+ {
>+ vpci_msi_arch_mask(&msi->arch, pdev, i, MASK_EXTR(val.u32, 1 <<
>i));
I don't think using MASK_EXTR() here is really advisable? Could be as simple
as (val.u32 >> i) & 1.
>+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 msi_offset;
Elsewhere we call such variables just "pos".
>+ uint16_t control;
>+ int ret;
>+
>+ msi_offset = pci_find_cap_offset(seg, bus, slot, func, PCI_CAP_ID_MSI);
>+ if ( !msi_offset )
>+ return 0;
>+
>+ msi = xzalloc(struct vpci_msi);
>+ if ( !msi )
>+ return -ENOMEM;
>+
>+ msi->pos = msi_offset;
>+
>+ control = pci_conf_read16(seg, bus, slot, func,
>+ msi_control_reg(msi_offset));
You don't use the value until ...
>+ ret = vpci_add_register(pdev, vpci_msi_control_read,
>+ vpci_msi_control_write,
>+ msi_control_reg(msi_offset), 2, msi);
>+ if ( ret )
>+ goto error;
>+
>+ /* Get the maximum number of vectors the device supports. */
>+ msi->max_vectors = multi_msi_capable(control);
... here. Please move the read down.
>+ ASSERT(msi->max_vectors <= 32);
>+
>+ /* No PIRQ bind yet. */
>+ vpci_msi_arch_init(&msi->arch);
s/bind/bound/ in the comment?
>...
>+ error:
>+ ASSERT(ret);
>+ xfree(msi);
>+ return ret;
>+}
Don't you also need to unregister address handlers you've registered?
>+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 guest %u\n", d->domain_id);
"... for Dom%d" or "... for d%d" please.
>...
>+ if ( msi->masking )
>+ printk("mask=%#032x\n", msi->mask);
Why 30 hex digits? And generally # should be used only when not blank or
zero padding the value (as field width includes the 0x prefix).
>--- a/xen/include/asm-x86/msi.h
>+++ b/xen/include/asm-x86/msi.h
>@@ -48,6 +48,7 @@
>#define MSI_ADDR_REDIRECTION_SHIFT 3
>#define MSI_ADDR_REDIRECTION_CPU (0 << MSI_ADDR_REDIRECTION_SHIFT)
>#define MSI_ADDR_REDIRECTION_LOWPRI (1 << MSI_ADDR_REDIRECTION_SHIFT)
>+#define MSI_ADDR_REDIRECTION_MASK 0x8
(1 << MSI_ADDR_REDIRECTION_SHIFT) please.
>+ struct vpci_msi {
>+ /* Offset of the capability in the config space. */
>+ unsigned int pos;
>+ /* Maximum number of vectors supported by the device. */
>+ unsigned int max_vectors;
>+ /* Number of vectors configured. */
>+ unsigned int vectors;
>+ /* Address and data fields. */
>+ uint64_t address;
>+ uint16_t data;
>+ /* Mask bitfield. */
>+ uint32_t mask;
>+ /* Enabled? */
>+ bool enabled;
>+ /* Supports per-vector masking? */
>+ bool masking;
>+ /* 64-bit address capable? */
>+ bool address64;
>+ /* Arch-specific data. */
>+ struct vpci_arch_msi arch;
>+ } *msi;
Please try to reduce the number/size of holes.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |