lkml.org 
[lkml]   [2019]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH V2 2/3] HYPERV/IOMMU: Add Hyper-V stub IOMMU driver
    Date
    From: lantianyu1986@gmail.com <lantianyu1986@gmail.com> Sent: Saturday, February 2, 2019 5:15 AM
    >
    > +/*
    > + * According 82093AA IO-APIC spec , IO APIC has a 24-entry Interrupt
    > + * Redirection Table.
    > + */
    > +#define IOAPIC_REMAPPING_ENTRY 24

    The other unstated assumption here is that Hyper-v guest VMs
    have only a single IOAPIC, regardless of the size of the VM.
    Perhaps that should be stated in the comment explaining why
    there are 24 entries?

    > +
    > +static cpumask_t ioapic_max_cpumask = { CPU_BITS_NONE };
    > +static struct irq_domain *ioapic_ir_domain;
    > +
    > +static int hyperv_ir_set_affinity(struct irq_data *data,
    > + const struct cpumask *mask, bool force)
    > +{
    > + struct irq_data *parent = data->parent_data;
    > + struct irq_cfg *cfg = irqd_cfg(data);
    > + struct IO_APIC_route_entry *entry;
    > + cpumask_t cpumask;
    > + int ret;
    > +
    > + cpumask_andnot(&cpumask, mask, &ioapic_max_cpumask);
    > +
    > + /* Return error If new irq affinity is out of ioapic_max_cpumask. */
    > + if (!cpumask_empty(&cpumask))
    > + return -EINVAL;

    The above two cpumask functions can be combined in a single
    call to cpumask_subset(). This has the nice property that determining
    whether the cpus in "mask" are a subset of the cpus in "ioapic_max_cpumask"
    is exactly what this code is trying to do. :-) And it gets rid of the local
    cpumask variable and the associated compiler warnings about stack frame
    size.

    > +
    > + ret = parent->chip->irq_set_affinity(parent, mask, force);
    > + if (ret < 0 || ret == IRQ_SET_MASK_OK_DONE)
    > + return ret;
    > +
    > + entry = data->chip_data;
    > + entry->dest = cfg->dest_apicid;
    > + entry->vector = cfg->vector;
    > + send_cleanup_vector(cfg);
    > +
    > + return 0;
    > +}
    > +
    > +static struct irq_chip hyperv_ir_chip = {
    > + .name = "HYPERV-IR",
    > + .irq_ack = apic_ack_irq,
    > + .irq_set_affinity = hyperv_ir_set_affinity,
    > +};
    > +
    > +static int hyperv_irq_remapping_alloc(struct irq_domain *domain,
    > + unsigned int virq, unsigned int nr_irqs,
    > + void *arg)
    > +{
    > + struct irq_alloc_info *info = arg;
    > + struct irq_data *irq_data;
    > + struct irq_desc *desc;
    > + int ret = 0;
    > +
    > + if (!info || info->type != X86_IRQ_ALLOC_TYPE_IOAPIC || nr_irqs > 1)
    > + return -EINVAL;
    > +
    > + ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
    > + if (ret < 0)
    > + return ret;
    > +
    > + irq_data = irq_domain_get_irq_data(domain, virq);
    > + if (!irq_data) {
    > + irq_domain_free_irqs_common(domain, virq, nr_irqs);
    > + return -EINVAL;
    > + }
    > +
    > + irq_data->chip = &hyperv_ir_chip;
    > +
    > + /*
    > + * IOAPIC entry pointer is saved in chip_data to allow
    > + * hyperv_irq_remappng_activate()/hyperv_ir_set_affinity() to set
    > + * vector and dest_apicid. cfg->vector and cfg->dest_apicid are
    > + * ignorred when IRQ remapping is enabled. See ioapic_configure_entry().

    Spelling: "ignored".

    I saw Vitaly previous comments, and I still don't understand this comment. :-(
    Is IRQ remapping considered to be enabled by this IOMMU driver, such that
    cfg->vector and cfg->dest_apicid are ignored? Or is the "when IRQ remapping
    is enabled" a statement about some future enhancement?

    > + */
    > + irq_data->chip_data = info->ioapic_entry;
    > +
    > + /*
    > + * Hypver-V IO APIC irq affinity should be in the scope of
    > + * ioapic_max_cpumask because no irq remapping support.
    > + */
    > + desc = irq_data_to_desc(irq_data);
    > + cpumask_and(desc->irq_common_data.affinity,
    > + desc->irq_common_data.affinity,
    > + &ioapic_max_cpumask);
    > +
    > + return 0;
    > +}
    > +
    > +static void hyperv_irq_remapping_free(struct irq_domain *domain,
    > + unsigned int virq, unsigned int nr_irqs)
    > +{
    > + irq_domain_free_irqs_common(domain, virq, nr_irqs);
    > +}
    > +
    > +static int hyperv_irq_remappng_activate(struct irq_domain *domain,
    > + struct irq_data *irq_data, bool reserve)
    > +{
    > + struct irq_cfg *cfg = irqd_cfg(irq_data);
    > + struct IO_APIC_route_entry *entry = irq_data->chip_data;
    > +
    > + entry->dest = cfg->dest_apicid;
    > + entry->vector = cfg->vector;
    > +
    > + return 0;
    > +}
    > +
    > +static struct irq_domain_ops hyperv_ir_domain_ops = {
    > + .alloc = hyperv_irq_remapping_alloc,
    > + .free = hyperv_irq_remapping_free,
    > + .activate = hyperv_irq_remappng_activate,
    > +};
    > +
    > +static int __init hyperv_prepare_irq_remapping(void)
    > +{
    > + struct fwnode_handle *fn;
    > + u32 apic_id;
    > + int i;
    > +
    > + if (x86_hyper_type != X86_HYPER_MS_HYPERV ||

    The function hypervisor_is_type() exists for doing the above test.
    See include/asm/hypervisor.h

    > + !x2apic_supported())
    > + return -ENODEV;
    > +
    > + fn = irq_domain_alloc_named_id_fwnode("HYPERV-IR", 0);
    > + if (!fn)
    > + return -ENOMEM;
    > +
    > + ioapic_ir_domain =
    > + irq_domain_create_hierarchy(arch_get_ir_parent_domain(),
    > + 0, IOAPIC_REMAPPING_ENTRY, fn,
    > + &hyperv_ir_domain_ops, NULL);
    > +
    > + irq_domain_free_fwnode(fn);
    > +
    > + /*
    > + * Hyper-V doesn't provide irq remapping function for
    > + * IO-APIC and so IO-APIC only accepts 8-bit APIC ID.
    > + * Cpu's APIC ID is read from ACPI MADT table and APIC IDs
    > + * in the MADT table on Hyper-v are sorted monotonic increasingly.
    > + * APIC ID reflects cpu topology. There maybe some APIC ID
    > + * gaps when cpu number in a socket is not power of two. Prepare
    > + * max cpu affinity for IOAPIC irqs. Scan cpu 0-255 and set cpu
    > + * into ioapic_max_cpumask if its APIC ID is less than 256.
    > + */
    > + for (i = 0; i < 256; i++) {
    > + apic_id = cpu_physical_id(i);
    > + if (apic_id > 255)
    > + continue;
    > +
    > + cpumask_set_cpu(i, &ioapic_max_cpumask);
    > + }

    The above for loop would be more compact as:

    for (i=0; i <256; i++)
    if (cpu_physical_id(i) < 256)
    cpumask_set_cpu(i, &ioapic_max_cpumask);

    > +
    > + return 0;
    > +}
    > +
    \
     
     \ /
      Last update: 2019-02-03 22:50    [W:8.109 / U:1.280 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site