Message-ID: <53B57B1D.8090308@amd.com>
Date:	Thu, 3 Jul 2014 10:47:41 -0500
From:	Suravee Suthikulanit <suravee.suthikulpanit@....com>
To:	Mark Rutland <mark.rutland@....com>
CC:	Marc Zyngier <Marc.Zyngier@....com>,
	"jason@...edaemon.net" <jason@...edaemon.net>,
	Pawel Moll <Pawel.Moll@....com>,
	Catalin Marinas <Catalin.Marinas@....com>,
	Will Deacon <Will.Deacon@....com>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"Harish.Kasiviswanathan@....com" <Harish.Kasiviswanathan@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Tom Lendacky <thomas.lendacky@....com>
Subject: Re: [PATCH 3/3 V2] irqchip: gic: Add supports for ARM GICv2m MSI(-X)

On 7/3/2014 3:46 AM, Mark Rutland wrote:
> On Wed, Jul 02, 2014 at 09:04:09PM +0100, Suravee Suthikulanit wrote:
>> Thanks again for the review. Please see my comments below.
>>
>> On 7/2/2014 11:39 AM, Mark Rutland wrote:
>>> On Wed, Jul 02, 2014 at 04:22:23PM +0100, suravee.suthikulpanit@....com wrote:
>>>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
>>>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>>>> index 5573c08..9e46f7f 100644
>>>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>>>> @@ -12,11 +12,13 @@ Main node required properties:
>>>>
>>>>    - compatible : should be one of:
>>>>           "arm,gic-400"
>>>> +       "arm,gic-400-plus"
>>>
>>> I am not keen on this name.
>>
>> Ok, I'll change it. Any suggestion on name?  I'm not sure what is the
>> "official" product name. I've seen this in some slides from ARM. What
>> about "gic-400-v2m"??.
>
> I'll query this internally.
Thanks.

>>>> @@ -37,9 +39,16 @@ Main node required properties:
>>>>           the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
>>>>           the interrupt is wired to that CPU.  Only valid for PPI interrupts.
>>>>
>>>> -- reg : Specifies base physical address(s) and size of the GIC registers. The
>>>> -  first region is the GIC distributor register base and size. The 2nd region is
>>>> -  the GIC cpu interface register base and size.
>>>> +- reg : Specifies base physical address(s) and size of the GIC register frames.
>>>> +
>>>> +  Region | Description
>>>> +  Index  |
>>>> +  -------------------------------------------------------------------
>>>> +     0   | GIC distributor register base and size
>>>> +     1   | GIC cpu interface register base and size
>>>> +     2   | VGIC interface control register base and size (Optional)
>>>> +     3   | VGIC CPU interface register base and size (Optional)
>>>> +     4   | GICv2m MSI interface register base and size (Optional)
>>>
>>> As far as I am aware, the MSI interface is completely orthogonal to
>>> having a GICH and GICV.
>>
>> Agree. I'm not doing anything with it. I'm just listing them here since
>> they are also mentioned in the gic.txt
>>
>>>
>>> We should figure out how we expect to handle that (zero-sized reg
>>> entries? reg-names?).
>>
>> I'm not sure how VGIC stuff handles reg/size = 0.
>
> Neither am I.
>
> However, what I said was that we should figure out how we _expect_ to
> handle that case. If we have to make changes to handle it, that's fine
> given we already have to make changes to support GICv2m.

Looking through the code in virt/kvm/arm/vgic.c, I believe this would 
ended up returning -EINVAL or -ENXIO from kvm_vgic_hyp_init().

>>>
>>>>
>>>>    Optional
>>>>    - interrupts   : Interrupt source of the parent interrupt controller on
>>>> @@ -55,6 +64,10 @@ Optional
>>>>                     by a crossbar/multiplexer preceding the GIC. The GIC irq
>>>>                     input line is assigned dynamically when the corresponding
>>>>                     peripheral's crossbar line is mapped.
>>>> +
>>>> +- msi-controller : Identifies the node as an MSI controller.  This requires
>>>> +  the register region index 4.
>>>
>>> That last clarifying comment is more confusing than helpful.
>>
>> If you are referring to the table, I added that since it was easier to
>> see than scanning the text.
>
> I was referring to "This requires the register region index 4".
>
> How about:
>
> - msi-controller : Identifies the node as an MSI controller. Requried
>    for GICv2m.

OK

>>
>>> [...]
>>>
>>>> +#define GIC_V2M_MIN_SPI                        32
>>>> +#define GIC_V2M_MAX_SPI                        1024
>>>
>>> GIC interrupt IDs 1020 and above are reserved, no?
>>>
>>> Surely the max ID an SPI can take is 1019?
>>
>> Right, thanks for catching. But the spec says that the SPI ID value must
>> be in the range of 32 to 1020. I guess it was a bit unclear, but
>> definitely not 1024 :)
>
> Which spec?

This was in the "Server Base System Architecture v1.0".
>
> In the GICv2 spec I see:
>
> * "Interrupt numbers ID32-ID1019 are used for SPIs." in
>    2.2.1 Interrupt IDs.
>
> * "The GIC architecture reserves interrupt ID numbers 1020-1023 for
>    special purposes." in 3.2.5 Special interrupt numbers.
>
>>
>>>> +#define GIC_OF_MSIV2M_RANGE_INDEX      4
>>>> +
>>>> +/**
>>>> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
>>>> + * @data: Pointer to v2m_data
>>>> + * @nvec: Number of interrupts to allocate
>>>> + * @irq: Pointer to the allocated irq
>>>> + *
>>>> + * Allocates interrupts only if the contiguous range of MSIs
>>>> + * with specified nvec are available. Otherwise return the number
>>>> + * of available interrupts. If none are available, then returns -ENOENT.
>>>> + */
>>>
>>> This function is overly complicated, and pointlessly so.
>>>
>>> It doesn't achieve anything useful as it returns the size of the _last_
>>> contiguous block rather than the _largest_ contiguous block, and the
>>> only caller (gicv2m_setup_msi_irq) doesn't even care.
>>>
>>> Simplify this to just return an error code if allocation is impossible.
>>
>> Actually, I made another mistake in the gicv2m_setup_msi_irq when
>> returning from the alloc_msi_irq().
>
> Ok.
>
>> My understanding is the arch_setup_msi_irqs() is supposed to return the
>> number of available vectors if the requested amount could not be
>> allocated. I notice that the current "drivers/pci/msi.c:
>> arch_setup_msi_irqs()" does not do so, which is okay.
>>
>> However, We are also working on adding support for multi-MSI support
>> since some of the devices we have are using it, which means we will need
>> to provide a different version of the "arch_setup_msi_irqs()" as the
>> current one does not allow (PCI_CAP_ID_MSI && nvec > 1).
>>
>> Therefore, I implemented the "alloc_msi_irq" this way to account for
>> future changes.
>
> Per my comments, I still think the function is broken given it returns
> the size of the last contiguous block of available interrupts.

Actually, the current logic for alloc_msi_irq() should do the following:
     - Return the next available contiguous block of nvec.
     - In case the requested nvec is not available, return the size of 
of the largest available range, since the for loop always call the 
"bitmap_find_next_zero_area()" from the first index of the bitmap.

Actually, let me just add the code for multiple MSIs, which should be 
making use of this function also in the V3 patch set.  That way, it 
would be clear.

Thanks,

Suravee

Suravee

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists