lkml.org 
[lkml]   [2016]   [Feb]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 3/5] irqchip/gic-v2: Parse and export virtual GIC information
    From
    Date
    On 08/02/16 18:30, Marc Zyngier wrote:
    > Julien,

    Hi Marc,

    > On 08/02/16 16:47, Julien Grall wrote:

    [...]

    >> +static void __init gic_of_setup_kvm_info(struct device_node *node)
    >> +{
    >> + int ret;
    >> + struct resource r;
    >> + unsigned int irq;
    >> +
    >> + gic_v2_kvm_info.type = GIC_V2;
    >> +
    >> + irq = irq_of_parse_and_map(node, 0);
    >> + if (!irq)
    >> + gic_v2_kvm_info.maint_irq = -1;
    >
    > Please don't do that. 0 *is* the value for an invalid interrupt, and
    > this is what you should expose here. Same for GICv3.

    I decided to use -1, because the function acpi_register_gsi is returning
    a negative value when an error occurred.

    AFAICT, returning 0 would be a valid value for acpi_register_gsi.

    >> + else
    >> + gic_v2_kvm_info.maint_irq = irq;
    >> +
    >> + ret = of_address_to_resource(node, 2, &r);
    >> + if (!ret) {
    >> + gic_v2_kvm_info.vctrl_base = r.start;
    >> + gic_v2_kvm_info.vctrl_size = resource_size(&r);
    >> + }
    >> +
    >> + ret = of_address_to_resource(node, 3, &r);
    >> + if (!ret) {
    >> + if (!PAGE_ALIGNED(r.start))
    >> + pr_warn("GICV physical address 0x%llx not page aligned\n",
    >> + (unsigned long long)r.start);
    >> + else if (!PAGE_ALIGNED(resource_size(&r)))
    >> + pr_warn("GICV size 0x%llx not a multiple of page size 0x%lx\n",
    >> + (unsigned long long)resource_size(&r),
    >> + PAGE_SIZE);
    >> + else {
    >> + gic_v2_kvm_info.vcpu_base = r.start;
    >> + gic_v2_kvm_info.vcpu_size = resource_size(&r);
    >
    > This tends to make me think that this should actually be a proper
    > resource, and not a set of arbitrary fields.

    I will give a look.

    [..]

    >> @@ -1270,6 +1338,12 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
    >> return -EINVAL;
    >>
    >> cpu_phy_base = gic_cpu_base;
    >> + acpi_data.maint_irq = processor->vgic_interrupt;
    >> + acpi_data.maint_irq_mode = (processor->flags & ACPI_MADT_VGIC_IRQ_MODE) ?
    >> + ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
    >> + acpi_data.vctrl_base = processor->gich_base_address;
    >> + acpi_data.vcpu_base = processor->gicv_base_address;
    >> +
    >
    > Maybe you can now move all the ACPI data into this acpi_data structure?
    > This would allow for slightly less clutter...

    Good idea, I will do it in the next version.

    [...]

    >
    > Thanks,

    Regards,

    --
    Julien Grall

    \
     
     \ /
      Last update: 2016-02-09 12:41    [W:4.300 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site