* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 6:15 ` [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units Ganapatrao Kulkarni
@ 2017-06-21 7:09 ` Jayachandran C
2017-06-21 8:44 ` Marc Zyngier
2017-06-21 8:58 ` Marc Zyngier
2017-06-21 9:28 ` Lorenzo Pieralisi
2 siblings, 1 reply; 10+ messages in thread
From: Jayachandran C @ 2017-06-21 7:09 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> +
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
The original driver does not use pr_fmt properly, it may be worth
fixing that up rather than having prefixes, here...
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
here.
> + return -EINVAL;
> + }
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
also here.
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
The function implementation returns a value, but this does not, it is
better to be consistent here.
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
JC.
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 7:09 ` Jayachandran C
@ 2017-06-21 8:44 ` Marc Zyngier
2017-06-21 15:25 ` Jayachandran C
0 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2017-06-21 8:44 UTC (permalink / raw)
To: linux-arm-kernel
On 21/06/17 08:09, Jayachandran C wrote:
> On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>> +
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>
> The original driver does not use pr_fmt properly, it may be worth
> fixing that up rather than having prefixes, here...
Irrespective of what the "proper" usage is, this kind of cleanup doesn't
belong in this series. Feel free to submit a separate patch.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 8:44 ` Marc Zyngier
@ 2017-06-21 15:25 ` Jayachandran C
0 siblings, 0 replies; 10+ messages in thread
From: Jayachandran C @ 2017-06-21 15:25 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 09:44:58AM +0100, Marc Zyngier wrote:
> On 21/06/17 08:09, Jayachandran C wrote:
> > On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> >> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> >> Later in per device probe, ITS devices are mapped to
> >> numa node using ITS id to proximity domain mapping.
> >>
> >> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> >> ---
> >> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 79 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> >> index 45ea1933..88cfb32 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
> >>
> >> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
> >>
> >> +#ifdef CONFIG_ACPI_NUMA
> >> +struct its_srat_map {
> >> + u32 numa_node; /* numa node id */
> >> + u32 its_id; /* GIC ITS ID */
> >> +};
> >> +
> >> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> >> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> >> +
> >> +static int its_in_srat __initdata;
> >> +
> >> +static int __init
> >> +acpi_get_its_numa_node(u32 its_id)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < its_in_srat; i++) {
> >> + if (its_id == its_srat_maps[i].its_id)
> >> + return its_srat_maps[i].numa_node;
> >> + }
> >> + return NUMA_NO_NODE;
> >> +}
> >> +
> >> +static int __init
> >> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> >> + const unsigned long end)
> >> +{
> >> + int pxm, node;
> >> + struct acpi_srat_its_affinity *its_affinity;
> >> +
> >> + its_affinity = (struct acpi_srat_its_affinity *)header;
> >> + if (!its_affinity)
> >> + return -EINVAL;
> >> +
> >> + if (its_affinity->header.length <
> >> + sizeof(struct acpi_srat_its_affinity)) {
> >> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> >> + its_affinity->header.length);
> >
> > The original driver does not use pr_fmt properly, it may be worth
> > fixing that up rather than having prefixes, here...
>
> Irrespective of what the "proper" usage is
The prefixes of the prints in this driver are a bit inconsistent. And
there are a few error messages like the "Failed to allocate PROPBASE"
and "Bad LPI chunk %d\n" without any prefix at all. pr_fmt would be
helpful to clean it up.
> this kind of cleanup doesn't belong in this series.
Right. I think the printed messages in this patch can be improved quite
a bit. currently the tags and spacing are inconsistent, something like:
| pr_err("SRAT: Invalid header length %d in ITS affinity\n", ...
| pr_err("SRAT: Invalid NUMA node %d in ITS affinity\n", node);
| pr_info("SRAT: PXM %d -> ITS %d -> Node %d\n",
would be better in my opinion.
> Feel free to submit a separate patch.
The driver also prints 2 lines per CPU on boot which can get to a few
hundred lines on our test machines. So there are a few things worth
fixing up here... maybe in another kernel dev cycle.
JC.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 6:15 ` [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units Ganapatrao Kulkarni
2017-06-21 7:09 ` Jayachandran C
@ 2017-06-21 8:58 ` Marc Zyngier
2017-06-21 9:56 ` Ganapatrao Kulkarni
2017-06-21 9:28 ` Lorenzo Pieralisi
2 siblings, 1 reply; 10+ messages in thread
From: Marc Zyngier @ 2017-06-21 8:58 UTC (permalink / raw)
To: linux-arm-kernel
On 21/06/17 07:15, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
> +
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
Please keep these on the same line.
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
Same remark.
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
Same thing.
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
> + return -EINVAL;
> + }
So if you find an entry that doesn't match the current kernel
configuration, you drop all the subsequent entries? That doesn't feel right.
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
If you don't check the return value, there is no point returning it.
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 8:58 ` Marc Zyngier
@ 2017-06-21 9:56 ` Ganapatrao Kulkarni
0 siblings, 0 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2017-06-21 9:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 2:28 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 21/06/17 07:15, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>> +
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>
> Please keep these on the same line.
sure.
>
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>
> Same remark.
ok
>
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>
> Same thing.
ok
>
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>> + return -EINVAL;
>> + }
>> +
>> + if (its_in_srat >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
>> + MAX_NUMNODES);
>> + return -EINVAL;
>> + }
>> +
>> + pxm = its_affinity->proximity_domain;
>> + node = acpi_map_pxm_to_node(pxm);
>> +
>> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
>> + return -EINVAL;
>> + }
>
> So if you find an entry that doesn't match the current kernel
> configuration, you drop all the subsequent entries? That doesn't feel right.
thanks, it is reasonable to print warning to notify that
mapping is broken for some tables and continue.
anyway device which does not have mapping gets mapped default to NUMA_NO_NODE.
>
>> +
>> + its_srat_maps[its_in_srat].numa_node = node;
>> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
>> + its_in_srat++;
>> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
>> + pxm, its_affinity->its_id, node);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init acpi_table_parse_srat_its(void)
>> +{
>> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
>> + sizeof(struct acpi_table_srat),
>> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
>> + gic_acpi_parse_srat_its, 0);
>
> If you don't check the return value, there is no point returning it.
thanks, return value is don't care, i will change accordingly.
>
>> +}
>> +#else
>> +#define acpi_table_parse_srat_its() do { } while (0)
>> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
>> +#endif
>> +
>> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> goto dom_err;
>> }
>>
>> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
>> + err = its_probe_one(&res, dom_handle,
>> + acpi_get_its_numa_node(its_entry->translation_id));
>> if (!err)
>> return 0;
>>
>> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>>
>> static void __init its_acpi_probe(void)
>> {
>> + acpi_table_parse_srat_its();
>> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> gic_acpi_parse_madt_its, 0);
>> }
>>
>
> Thanks,
>
> M.
> --
> Jazz is not dead. It just smells funny...
thanks
Ganapat
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 6:15 ` [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units Ganapatrao Kulkarni
2017-06-21 7:09 ` Jayachandran C
2017-06-21 8:58 ` Marc Zyngier
@ 2017-06-21 9:28 ` Lorenzo Pieralisi
2017-06-21 10:06 ` Ganapatrao Kulkarni
2 siblings, 1 reply; 10+ messages in thread
From: Lorenzo Pieralisi @ 2017-06-21 9:28 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
> Later in per device probe, ITS devices are mapped to
> numa node using ITS id to proximity domain mapping.
>
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 45ea1933..88cfb32 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>
> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>
> +#ifdef CONFIG_ACPI_NUMA
> +struct its_srat_map {
> + u32 numa_node; /* numa node id */
> + u32 its_id; /* GIC ITS ID */
> +};
> +
> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
Nit: UINT_MAX is as valid as 0 as its_id (but the intent here is to make
it look like an invalid value), given that the its_in_srat counter
defines what entries are valid I do not necessarily see the point in
initializing with something that is a valid value != 0.
> +static int its_in_srat __initdata;
> +
> +static int __init
> +acpi_get_its_numa_node(u32 its_id)
> +{
> + int i;
> +
> + for (i = 0; i < its_in_srat; i++) {
> + if (its_id == its_srat_maps[i].its_id)
> + return its_srat_maps[i].numa_node;
> + }
> + return NUMA_NO_NODE;
> +}
> +
> +static int __init
> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + int pxm, node;
> + struct acpi_srat_its_affinity *its_affinity;
> +
> + its_affinity = (struct acpi_srat_its_affinity *)header;
> + if (!its_affinity)
> + return -EINVAL;
> +
> + if (its_affinity->header.length <
> + sizeof(struct acpi_srat_its_affinity)) {
> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
> + its_affinity->header.length);
> + return -EINVAL;
> + }
> +
> + if (its_in_srat >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
> + MAX_NUMNODES);
> + return -EINVAL;
> + }
> +
> + pxm = its_affinity->proximity_domain;
> + node = acpi_map_pxm_to_node(pxm);
> +
> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
> + return -EINVAL;
As Marc mentioned you should bail out here but continue parsing entries,
aka "return 0";
Thanks,
Lorenzo
> + }
> +
> + its_srat_maps[its_in_srat].numa_node = node;
> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
> + its_in_srat++;
> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
> + pxm, its_affinity->its_id, node);
> +
> + return 0;
> +}
> +
> +static int __init acpi_table_parse_srat_its(void)
> +{
> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
> + sizeof(struct acpi_table_srat),
> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
> + gic_acpi_parse_srat_its, 0);
> +}
> +#else
> +#define acpi_table_parse_srat_its() do { } while (0)
> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
> +#endif
> +
> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> const unsigned long end)
> {
> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
> goto dom_err;
> }
>
> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
> + err = its_probe_one(&res, dom_handle,
> + acpi_get_its_numa_node(its_entry->translation_id));
> if (!err)
> return 0;
>
> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>
> static void __init its_acpi_probe(void)
> {
> + acpi_table_parse_srat_its();
> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
> gic_acpi_parse_madt_its, 0);
> }
> --
> 1.8.1.4
>
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 2/2] acpi, gicv3-its, numa: Adding numa node mapping for gic-its units
2017-06-21 9:28 ` Lorenzo Pieralisi
@ 2017-06-21 10:06 ` Ganapatrao Kulkarni
0 siblings, 0 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2017-06-21 10:06 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 21, 2017 at 2:58 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 21, 2017 at 11:45:43AM +0530, Ganapatrao Kulkarni wrote:
>> Add code to parse SRAT ITS Affinity sub table as defined in ACPI 6.2
>> Later in per device probe, ITS devices are mapped to
>> numa node using ITS id to proximity domain mapping.
>>
>> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 80 +++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 45ea1933..88cfb32 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1833,6 +1833,82 @@ static int __init its_of_probe(struct device_node *node)
>>
>> #define ACPI_GICV3_ITS_MEM_SIZE (SZ_128K)
>>
>> +#ifdef CONFIG_ACPI_NUMA
>> +struct its_srat_map {
>> + u32 numa_node; /* numa node id */
>> + u32 its_id; /* GIC ITS ID */
>> +};
>> +
>> +static struct its_srat_map its_srat_maps[MAX_NUMNODES] __initdata = {
>> + [0 ... MAX_NUMNODES - 1] = {NUMA_NO_NODE, UINT_MAX} };
>
> Nit: UINT_MAX is as valid as 0 as its_id (but the intent here is to make
> it look like an invalid value), given that the its_in_srat counter
> defines what entries are valid I do not necessarily see the point in
> initializing with something that is a valid value != 0.
ok, this array may not need any init,
>
>> +static int its_in_srat __initdata;
>> +
>> +static int __init
>> +acpi_get_its_numa_node(u32 its_id)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < its_in_srat; i++) {
>> + if (its_id == its_srat_maps[i].its_id)
>> + return its_srat_maps[i].numa_node;
>> + }
>> + return NUMA_NO_NODE;
>> +}
>> +
>> +static int __init
>> +gic_acpi_parse_srat_its(struct acpi_subtable_header *header,
>> + const unsigned long end)
>> +{
>> + int pxm, node;
>> + struct acpi_srat_its_affinity *its_affinity;
>> +
>> + its_affinity = (struct acpi_srat_its_affinity *)header;
>> + if (!its_affinity)
>> + return -EINVAL;
>> +
>> + if (its_affinity->header.length <
>> + sizeof(struct acpi_srat_its_affinity)) {
>> + pr_err("SRAT:ITS: Invalid SRAT header length: %d\n",
>> + its_affinity->header.length);
>> + return -EINVAL;
>> + }
>> +
>> + if (its_in_srat >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: ITS devices exceeding max count[%d]\n",
>> + MAX_NUMNODES);
>> + return -EINVAL;
>> + }
>> +
>> + pxm = its_affinity->proximity_domain;
>> + node = acpi_map_pxm_to_node(pxm);
>> +
>> + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
>> + pr_err("SRAT:ITS: Invalid numa node %d\n", node);
>> + return -EINVAL;
>
> As Marc mentioned you should bail out here but continue parsing entries,
> aka "return 0";
agreed.
>
> Thanks,
> Lorenzo
>
>> + }
>> +
>> + its_srat_maps[its_in_srat].numa_node = node;
>> + its_srat_maps[its_in_srat].its_id = its_affinity->its_id;
>> + its_in_srat++;
>> + pr_info("ACPI: NUMA: SRAT: ITS: PXM %d -> ITS_ID %d -> NODE %d\n",
>> + pxm, its_affinity->its_id, node);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init acpi_table_parse_srat_its(void)
>> +{
>> + return acpi_table_parse_entries(ACPI_SIG_SRAT,
>> + sizeof(struct acpi_table_srat),
>> + ACPI_SRAT_TYPE_GIC_ITS_AFFINITY,
>> + gic_acpi_parse_srat_its, 0);
>> +}
>> +#else
>> +#define acpi_table_parse_srat_its() do { } while (0)
>> +#define acpi_get_its_numa_node(its_id) NUMA_NO_NODE
>> +#endif
>> +
>> static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> const unsigned long end)
>> {
>> @@ -1861,7 +1937,8 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>> goto dom_err;
>> }
>>
>> - err = its_probe_one(&res, dom_handle, NUMA_NO_NODE);
>> + err = its_probe_one(&res, dom_handle,
>> + acpi_get_its_numa_node(its_entry->translation_id));
>> if (!err)
>> return 0;
>>
>> @@ -1873,6 +1950,7 @@ static int __init gic_acpi_parse_madt_its(struct acpi_subtable_header *header,
>>
>> static void __init its_acpi_probe(void)
>> {
>> + acpi_table_parse_srat_its();
>> acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_TRANSLATOR,
>> gic_acpi_parse_madt_its, 0);
>> }
>> --
>> 1.8.1.4
>>
thanks
Ganapat
^ permalink raw reply [flat|nested] 10+ messages in thread