[Xen-devel,RFC,01/11] acpi: arm: Public API for populating and query based on requesterid

From: Manish Jaggi <manish.jaggi@linaro.org>

 Public API to populate and query map between requester id and
 streamId/DeviceID. IORT is parsed one time (outside this patch)
 and two lists are created one for mapping between reuesterId and streamid
 and another between requesterID and deviceID.

 These lists eliminate the need to reparse IORT for querying streamid
 or deviceid using requesterid.

 Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
 xen/drivers/acpi/Makefile     |   1 +
 xen/drivers/acpi/arm/Makefile |   1 +
 xen/drivers/acpi/arm/ridmap.c | 124 ++++++++++++++++++++++++++++++++++++++++++
 xen/include/acpi/ridmap.h     |  77 ++++++++++++++++++++++++++
 4 files changed, 203 insertions(+)

Hi Manish,

On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
>   Public API to populate and query map between requester id and

The commit message should not be indented.

>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>   and two lists are created one for mapping between reuesterId and streamid

s/reuesterId/requesterID/

Please stay consistent in the naming (including the lowercase/uppercase).

Cheers,

>   and another between requesterID and deviceID.
> 
>   These lists eliminate the need to reparse IORT for querying streamid
>   or deviceid using requesterid.
> 
>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/drivers/acpi/Makefile     |   1 +
>   xen/drivers/acpi/arm/Makefile |   1 +
>   xen/drivers/acpi/arm/ridmap.c | 124 ++++++++++++++++++++++++++++++++++++++++++
>   xen/include/acpi/ridmap.h     |  77 ++++++++++++++++++++++++++
>   4 files changed, 203 insertions(+)
> 
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 444b11d583..80a074e007 100644
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -1,6 +1,7 @@
>   subdir-y += tables
>   subdir-y += utilities
>   subdir-$(CONFIG_X86) += apei
> +subdir-$(CONFIG_ARM) += arm
>   
>   obj-bin-y += tables.init.o
>   obj-$(CONFIG_NUMA) += numa.o
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> new file mode 100644
> index 0000000000..046fad5e3d
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -0,0 +1 @@
> +obj-y = ridmap.o
> diff --git a/xen/drivers/acpi/arm/ridmap.c b/xen/drivers/acpi/arm/ridmap.c
> new file mode 100644
> index 0000000000..2c3a8876ea
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/ridmap.c
> @@ -0,0 +1,124 @@
> +/*
> + * xen/drivers/acpi/arm/ridmap.c
> + *
> + * Public API to populate and query map between requester id and
> + * streamId/DeviceID
> + *
> + * Manish Jaggi <manish.jaggi@linaro.org>
> + * Copyright (c) 2018 Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#include <acpi/ridmap.h>
> +#include <xen/iommu.h>
> +#include <xen/kernel.h>
> +#include <xen/list.h>
> +#include <xen/pci.h>
> +
> +struct list_head rid_streamid_map_list;
> +struct list_head rid_deviceid_map_list;
> +
> +void init_ridmaps(void)
> +{
> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
> +    INIT_LIST_HEAD(&rid_streamid_map_list);
> +}
> +
> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *smmu_node,
> +                         u32 input_base, u32 output_base, u32 id_count)
> +{
> +    struct rid_streamid_map *rid_map;
> +    rid_map = xzalloc(struct rid_streamid_map);
> +
> +    if (!rid_map)
> +        return -ENOMEM;
> +
> +    rid_map->idmap.input_base = input_base;
> +    rid_map->idmap.output_base = output_base;
> +    rid_map->idmap.id_count = id_count;
> +    rid_map->pcirc_node = pcirc_node;
> +    rid_map->smmu_node = smmu_node;
> +
> +    list_add_tail(&rid_map->entry, &rid_streamid_map_list);
> +    return 0;
> +}
> +
> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *its_node,
> +                         u32 input_base, u32 output_base, u32 id_count)
> +{
> +    struct rid_deviceid_map *rid_map;
> +    rid_map = xzalloc(struct rid_deviceid_map);
> +
> +    if (!rid_map)
> +        return -ENOMEM;
> +
> +    rid_map->idmap.input_base = input_base;
> +    rid_map->idmap.output_base = output_base;
> +    rid_map->idmap.id_count = id_count;
> +    rid_map->pcirc_node = pcirc_node;
> +    rid_map->its_node = its_node;
> +
> +    list_add_tail(&rid_map->entry, &rid_deviceid_map_list);
> +    return 0;
> +}
> +
> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *streamid,
> +                    struct acpi_iort_node **smmu_node)
> +{
> +    struct rid_streamid_map *rmap;
> +
> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
> +    {
> +        if (rmap->pcirc_node == pcirc_node)
> +        {
> +            if ( (rid >= rmap->idmap.input_base) &&
> +                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
> +            {
> +                *streamid = rid - rmap->idmap.input_base +
> +                            rmap->idmap.output_base;
> +                *smmu_node = rmap->smmu_node;
> +                break;
> +            }
> +        }
> +    }
> +
> +}
> +
> +void query_deviceid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *deviceid)
> +{
> +    struct rid_deviceid_map *rmap;
> +
> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
> +    {
> +        if (rmap->pcirc_node == pcirc_node)
> +        {
> +            if ( (rid >= rmap->idmap.input_base) &&
> +                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
> +            {
> +                *deviceid = rid - rmap->idmap.input_base +
> +                            rmap->idmap.output_base;
> +                break;
> +            }
> +        }
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/acpi/ridmap.h b/xen/include/acpi/ridmap.h
> new file mode 100644
> index 0000000000..806f401d89
> --- /dev/null
> +++ b/xen/include/acpi/ridmap.h
> @@ -0,0 +1,77 @@
> +/*
> + * xen/include/acpi/ridmap.h
> + *
> + * Mapping structures to hold map between requester id and streamId/DeviceID
> + * after paring the IORT table.
> + *
> + * Manish Jaggi <manish.jaggi@linaro.org>
> + * Copyright (c) 2018 Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +#ifndef RID_MAP_H
> +#define RID_MAP_H
> +
> +#include <xen/acpi.h>
> +
> +struct id_map_struct
> +{
> +    u16 input_base;
> +    u32 output_base;
> +    u16 id_count;
> +};
> +
> +struct rid_streamid_map
> +{
> +    struct acpi_iort_node *pcirc_node;
> +    struct id_map_struct idmap;
> +    struct list_head entry;
> +    struct acpi_iort_node *smmu_node;
> +};
> +
> +struct rid_deviceid_map
> +{
> +    struct acpi_iort_node *pcirc_node;
> +    struct acpi_iort_node *its_node;
> +    struct id_map_struct idmap;
> +    struct list_head entry;
> +};
> +
> +extern struct list_head rid_streamid_map_list;
> +extern struct list_head rid_deviceid_map_list;
> +
> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *smmu_node,
> +                         u32 input_base, u32 output_base, u32 id_count);
> +
> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *its_node,
> +                         u32 input_base, u32 output_base, u32 id_count);
> +
> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *streamid,
> +                    struct acpi_iort_node **smmu_node);
> +
> +void query_deviceid(struct acpi_iort_node *pcirc_node,
> +                    u16 rid, u32 *deviceid);
> +
> +void init_ridmaps(void);
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>

Hi Manish,

I sent the previous e-mail too soon.

On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 
>   Public API to populate and query map between requester id and
>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>   and two lists are created one for mapping between reuesterId and streamid
>   and another between requesterID and deviceID.
> 
>   These lists eliminate the need to reparse IORT for querying streamid
>   or deviceid using requesterid.
> 
>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/drivers/acpi/Makefile     |   1 +
>   xen/drivers/acpi/arm/Makefile |   1 +

We have a directory arch/arm/acpi/. So please move all your code there.

>   xen/drivers/acpi/arm/ridmap.c | 124 ++++++++++++++++++++++++++++++++++++++++++
>   xen/include/acpi/ridmap.h     |  77 ++++++++++++++++++++++++++

No need to make this header available in common. That should go under 
asm-arm/acpi/

>   4 files changed, 203 insertions(+)
> 
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 444b11d583..80a074e007 100644
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -1,6 +1,7 @@
>   subdir-y += tables
>   subdir-y += utilities
>   subdir-$(CONFIG_X86) += apei
> +subdir-$(CONFIG_ARM) += arm
>   
>   obj-bin-y += tables.init.o
>   obj-$(CONFIG_NUMA) += numa.o
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> new file mode 100644
> index 0000000000..046fad5e3d
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -0,0 +1 @@
> +obj-y = ridmap.o
> diff --git a/xen/drivers/acpi/arm/ridmap.c b/xen/drivers/acpi/arm/ridmap.c
> new file mode 100644
> index 0000000000..2c3a8876ea
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/ridmap.c
> @@ -0,0 +1,124 @@
> +/*
> + * xen/drivers/acpi/arm/ridmap.c
> + *
> + * Public API to populate and query map between requester id and
> + * streamId/DeviceID

I don't care whether you use deviceID or DeviceID but please stay 
consistent with the naming.

> + *
> + * Manish Jaggi <manish.jaggi@linaro.org>
> + * Copyright (c) 2018 Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Xen is GPLv2 only and hence the copyright wrong. You want to use:

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.
> + */
> +
> +#include <acpi/ridmap.h>
> +#include <xen/iommu.h>
> +#include <xen/kernel.h>
> +#include <xen/list.h>
> +#include <xen/pci.h>
> +
> +struct list_head rid_streamid_map_list;
> +struct list_head rid_deviceid_map_list;

Please drop _list. This is pointless to know that when you can discover it.

Also, can you explain the rationale of using an unsorted list over 
another structure? Along that please give an idea how often and where 
the query API will be used.

> +
> +void init_ridmaps(void)

This likely need to be __init.

> +{
> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
> +    INIT_LIST_HEAD(&rid_streamid_map_list);
> +}

This function is not necessary. Declaring 
LIST_HEAD(rid_streamid_map_list) will do the trick.

> +
> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,

Ditto.

> +                         struct acpi_iort_node *smmu_node,
> +                         u32 input_base, u32 output_base, u32 id_count)

u32 & co should not be used in new code (unless imported from Linux). 
Please use uint32_t & co.

> +{
> +    struct rid_streamid_map *rid_map;

Newline here as it should be between after declarations.


> +    rid_map = xzalloc(struct rid_streamid_map);
> +
> +    if (!rid_map)

This should be ( ... ).

> +        return -ENOMEM;

You either return -ENOMEM or 0 in this function. It sounds like to me 
that bool would be the best.

> +
> +    rid_map->idmap.input_base = input_base;
> +    rid_map->idmap.output_base = output_base;
> +    rid_map->idmap.id_count = id_count;
> +    rid_map->pcirc_node = pcirc_node;
> +    rid_map->smmu_node = smmu_node;
> +
> +    list_add_tail(&rid_map->entry, &rid_streamid_map_list);

Newline here.

> +    return 0;
> +}
> +
> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *its_node,
> +                         u32 input_base, u32 output_base, u32 id_count)

s/u*/uint_/

> +{
> +    struct rid_deviceid_map *rid_map;

Newline here.

> +    rid_map = xzalloc(struct rid_deviceid_map);
> +
> +    if (!rid_map)

Coding style.

> +        return -ENOMEM;
> +
> +    rid_map->idmap.input_base = input_base;
> +    rid_map->idmap.output_base = output_base;
> +    rid_map->idmap.id_count = id_count;
> +    rid_map->pcirc_node = pcirc_node;
> +    rid_map->its_node = its_node;
> +
> +    list_add_tail(&rid_map->entry, &rid_deviceid_map_list);

Newline here.

> +    return 0;
> +}
> +
> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *streamid,

s/u*/uint_/

But how come the rid is 16-bit here when Linux is using 32-bit?

Also, I am a bit puzzled how the caller is expected to use it. From the 
name I would expect the function to return whether a translation was 
found. But it returns void.

IHMO, this is a pretty bad idea and make more expectation on the value 
for the caller.

Lastly, I would appreciate documentation on at least the function exported.

> +                    struct acpi_iort_node **smmu_node)
> +{
> +    struct rid_streamid_map *rmap;
> +
> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
> +    {
> +        if (rmap->pcirc_node == pcirc_node)

Coding style.

> +        {
> +            if ( (rid >= rmap->idmap.input_base) &&
> +                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
> +            {
> +                *streamid = rid - rmap->idmap.input_base +
> +                            rmap->idmap.output_base;
> +                *smmu_node = rmap->smmu_node;
> +                break;
> +            }
> +        }
> +    }
> +
> +}
> +
> +void query_deviceid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *deviceid)

Ditto for everything above within this function.

> +{
> +    struct rid_deviceid_map *rmap;
> +
> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
> +    {
> +        if (rmap->pcirc_node == pcirc_node)
> +        {
> +            if ( (rid >= rmap->idmap.input_base) &&
> +                 (rid < rmap->idmap.input_base + rmap->idmap.id_count) )
> +            {
> +                *deviceid = rid - rmap->idmap.input_base +
> +                            rmap->idmap.output_base;
> +                break;
> +            }
> +        }
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/acpi/ridmap.h b/xen/include/acpi/ridmap.h
> new file mode 100644
> index 0000000000..806f401d89
> --- /dev/null
> +++ b/xen/include/acpi/ridmap.h
> @@ -0,0 +1,77 @@
> +/*
> + * xen/include/acpi/ridmap.h
> + *
> + * Mapping structures to hold map between requester id and streamId/DeviceID
> + * after paring the IORT table.

s/paring/parsing/

I think it would be clearer if you say:

Defitions for structure holding mapping between a requesterID and 
streamID/DeviceID.

> + *
> + * Manish Jaggi <manish.jaggi@linaro.org>
> + * Copyright (c) 2018 Linaro.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

Wrong license.

> + *
> + * 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.
> + */
> +
> +#ifndef RID_MAP_H

We usually had the directory name in the guard to prevent potential

> +#define RID_MAP_H
> +
> +#include <xen/acpi.h>
> +
> +struct id_map_struct

Saying struct in the name is a bit pointless given you will always use 
it with "struct ..." before.

I would also appreciate some documentation within this header.


> +{
> +    u16 input_base;

uint_ same below.

> +    u32 output_base;
> +    u16 id_count;
> +};
> +
> +struct rid_streamid_map
> +{
> +    struct acpi_iort_node *pcirc_node;
> +    struct id_map_struct idmap;
> +    struct list_head entry;
> +    struct acpi_iort_node *smmu_node; > +};
> +
> +struct rid_deviceid_map
> +{
> +    struct acpi_iort_node *pcirc_node;
> +    struct acpi_iort_node *its_node;
> +    struct id_map_struct idmap;
> +    struct list_head entry;
> +};
> +
> +extern struct list_head rid_streamid_map_list;
> +extern struct list_head rid_deviceid_map_list;

I am not a big fan of exporting those 2 maps. But I will see how you use 
it before commenting.

For the rest of the code, see my comments in the patch.

> +
> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *smmu_node,
> +                         u32 input_base, u32 output_base, u32 id_count);
> +
> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
> +                         struct acpi_iort_node *its_node,
> +                         u32 input_base, u32 output_base, u32 id_count);
> +
> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 *streamid,
> +                    struct acpi_iort_node **smmu_node);
> +
> +void query_deviceid(struct acpi_iort_node *pcirc_node,
> +                    u16 rid, u32 *deviceid);
> +
> +void init_ridmaps(void);
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> 

Cheers,

On 01/17/2018 12:01 AM, Julien Grall wrote:
> Hi Manish,
>
Hi Julien,
Thanks for reviewing the patch.

> I sent the previous e-mail too soon.
>
> On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>
>>   Public API to populate and query map between requester id and
>>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>>   and two lists are created one for mapping between reuesterId and 
>> streamid
>>   and another between requesterID and deviceID.
>>
>>   These lists eliminate the need to reparse IORT for querying streamid
>>   or deviceid using requesterid.
>>
>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>> ---
>>   xen/drivers/acpi/Makefile     |   1 +
>>   xen/drivers/acpi/arm/Makefile |   1 +
>
> We have a directory arch/arm/acpi/. So please move all your code there.
The current files in arch/arm/acpi hold only boot time/low level code.
IMHO creating drivers/acpi/arm makes more sense.
Linux also has iort code in drivers/acpi/arm.

>
>>   xen/drivers/acpi/arm/ridmap.c | 124 
>> ++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/acpi/ridmap.h     |  77 ++++++++++++++++++++++++++
>
> No need to make this header available in common. That should go under 
> asm-arm/acpi/
ok
>
>>   4 files changed, 203 insertions(+)
>>
>> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
>> index 444b11d583..80a074e007 100644
>> --- a/xen/drivers/acpi/Makefile
>> +++ b/xen/drivers/acpi/Makefile
>> @@ -1,6 +1,7 @@
>>   subdir-y += tables
>>   subdir-y += utilities
>>   subdir-$(CONFIG_X86) += apei
>> +subdir-$(CONFIG_ARM) += arm
>>     obj-bin-y += tables.init.o
>>   obj-$(CONFIG_NUMA) += numa.o
>> diff --git a/xen/drivers/acpi/arm/Makefile 
>> b/xen/drivers/acpi/arm/Makefile
>> new file mode 100644
>> index 0000000000..046fad5e3d
>> --- /dev/null
>> +++ b/xen/drivers/acpi/arm/Makefile
>> @@ -0,0 +1 @@
>> +obj-y = ridmap.o
>> diff --git a/xen/drivers/acpi/arm/ridmap.c 
>> b/xen/drivers/acpi/arm/ridmap.c
>> new file mode 100644
>> index 0000000000..2c3a8876ea
>> --- /dev/null
>> +++ b/xen/drivers/acpi/arm/ridmap.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * xen/drivers/acpi/arm/ridmap.c
>> + *
>> + * Public API to populate and query map between requester id and
>> + * streamId/DeviceID
>
> I don't care whether you use deviceID or DeviceID but please stay 
> consistent with the naming.
ok
>
>> + *
>> + * Manish Jaggi <manish.jaggi@linaro.org>
>> + * Copyright (c) 2018 Linaro.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> Xen is GPLv2 only and hence the copyright wrong. You want to use:
>
> 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.

I picked this copyright from xen/arch/arm/traps.c.

  * xen/arch/arm/traps.c
  *
  * ARM Trap handlers
  *
  * Copyright (c) 2011 Citrix Systems.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the Licese, or
  * (at your option) any later version.

So IIUYC, traps.c copyright is also wrong.
How do we plan to fix all other files in xen code which use the same 
copyright.
>> + *
>> + * 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.
>> + */
>> +
>> +#include <acpi/ridmap.h>
>> +#include <xen/iommu.h>
>> +#include <xen/kernel.h>
>> +#include <xen/list.h>
>> +#include <xen/pci.h>
>> +
>> +struct list_head rid_streamid_map_list;
>> +struct list_head rid_deviceid_map_list;
>
> Please drop _list. This is pointless to know that when you can 
> discover it.

I think it is not pointless.
There is a point here. :)
  _list is added to show that it is a list to make it more verbose.
Without _list the name could mean a single mapping as well.

If you care to see
xen/common/rangeset.c:27:    struct list_head range_list;

I hope you can appreciate the point.
>
> Also, can you explain the rationale of using an unsorted list over 
> another structure? 
Since rid - streamId mapping also requires pcirc_node so it would 
require two level of sorting.
First sort based on pcirc_node and next on basis of rid.
Does it makes sense to have all that complex code here ?
  as this API will be used only once per pci device
> Along that please give an idea how often and where the query API will 
> be used.
ok
BTW, this is called from pci_for_each_dma_alias code flow.
>
>> +
>> +void init_ridmaps(void)
>
> This likely need to be __init.
ok.
>
>> +{
>> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
>> +    INIT_LIST_HEAD(&rid_streamid_map_list);
>> +}
>
> This function is not necessary. Declaring 
> LIST_HEAD(rid_streamid_map_list) will do the trick.
ok.
>> +
>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>
> Ditto.
Ditto for? Sorry didnt catch your point here.
>
>> +                         struct acpi_iort_node *smmu_node,
>> +                         u32 input_base, u32 output_base, u32 id_count)
>
> u32 & co should not be used in new code (unless imported from Linux). 
> Please use uint32_t & co.
I couldn't find this in xen coding style document.
Could you please point to the section which says u32 should not be used.

>
>> +{
>> +    struct rid_streamid_map *rid_map;
>
> Newline here as it should be between after declarations.
>
ok
>
>> +    rid_map = xzalloc(struct rid_streamid_map);
>> +
>> +    if (!rid_map)
>
> This should be ( ... ).
>
>> +        return -ENOMEM;
>
> You either return -ENOMEM or 0 in this function. It sounds like to me 
> that bool would be the best.
I think ENOMEM should be used here. The error code is designed 
specifically for this purpose.
>
>> +
>> +    rid_map->idmap.input_base = input_base;
>> +    rid_map->idmap.output_base = output_base;
>> +    rid_map->idmap.id_count = id_count;
>> +    rid_map->pcirc_node = pcirc_node;
>> +    rid_map->smmu_node = smmu_node;
>> +
>> +    list_add_tail(&rid_map->entry, &rid_streamid_map_list);
>
> Newline here.
>
>> +    return 0;
>> +}
>> +
>> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
>> +                         struct acpi_iort_node *its_node,
>> +                         u32 input_base, u32 output_base, u32 id_count)
>
> s/u*/uint_/
See above
>
>> +{
>> +    struct rid_deviceid_map *rid_map;
>
> Newline here.
>
>> +    rid_map = xzalloc(struct rid_deviceid_map);
>> +
>> +    if (!rid_map)
>
> Coding style.
>
>> +        return -ENOMEM;
>> +
>> +    rid_map->idmap.input_base = input_base;
>> +    rid_map->idmap.output_base = output_base;
>> +    rid_map->idmap.id_count = id_count;
>> +    rid_map->pcirc_node = pcirc_node;
>> +    rid_map->its_node = its_node;
>> +
>> +    list_add_tail(&rid_map->entry, &rid_deviceid_map_list);
>
> Newline here.
>
>> +    return 0;
>> +}
>> +
>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 
>> *streamid,
>
> s/u*/uint_/
>
> But how come the rid is 16-bit here when Linux is using 32-bit?
IIUC rid is 16bit only. Dont know why linux is using 32bit.

rid = bus - 8bits , devfn 8bits.

 From PCI Express specification
The Requester ID is a 16-bit value that is unique for every PCI Express 
Function within a Hierarchy..

If you think it is a 32bit value please let me know how to use upper 16 
bits.

>
> Also, I am a bit puzzled how the caller is expected to use it. 
I thought it would be self explanatory query streamid based on rid.
But if it is not verbose enough for you, I will add this explicitly.

> From the name I would expect the function to return whether a 
> translation was found. But it returns void.
>
> IHMO, this is a pretty bad idea and make more expectation on the value 
> for the caller.
>
> Lastly, I would appreciate documentation on at least the function 
> exported.
ok.
>
>> +                    struct acpi_iort_node **smmu_node)
>> +{
>> +    struct rid_streamid_map *rmap;
>> +
>> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
>> +    {
>> +        if (rmap->pcirc_node == pcirc_node)
>
> Coding style.
Can we have a checkpatch.pl for xen.
This would help in cases when code has a mix of files with linux coding 
style and xen coding style.
>
>> +        {
>> +            if ( (rid >= rmap->idmap.input_base) &&
>> +                 (rid < rmap->idmap.input_base + 
>> rmap->idmap.id_count) )
>> +            {
>> +                *streamid = rid - rmap->idmap.input_base +
>> +                            rmap->idmap.output_base;
>> +                *smmu_node = rmap->smmu_node;
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +}
>> +
>> +void query_deviceid(struct acpi_iort_node *pcirc_node, u16 rid, u32 
>> *deviceid)
>
> Ditto for everything above within this function.
>
>> +{
>> +    struct rid_deviceid_map *rmap;
>> +
>> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
>> +    {
>> +        if (rmap->pcirc_node == pcirc_node)
>> +        {
>> +            if ( (rid >= rmap->idmap.input_base) &&
>> +                 (rid < rmap->idmap.input_base + 
>> rmap->idmap.id_count) )
>> +            {
>> +                *deviceid = rid - rmap->idmap.input_base +
>> +                            rmap->idmap.output_base;
>> +                break;
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/acpi/ridmap.h b/xen/include/acpi/ridmap.h
>> new file mode 100644
>> index 0000000000..806f401d89
>> --- /dev/null
>> +++ b/xen/include/acpi/ridmap.h
>> @@ -0,0 +1,77 @@
>> +/*
>> + * xen/include/acpi/ridmap.h
>> + *
>> + * Mapping structures to hold map between requester id and 
>> streamId/DeviceID
>> + * after paring the IORT table.
>
> s/paring/parsing/
>
> I think it would be clearer if you say:
>
> Defitions for structure holding mapping between a requesterID and 
> streamID/DeviceID.
If that makes you happy, I will change it :)
>
>> + *
>> + * Manish Jaggi <manish.jaggi@linaro.org>
>> + * Copyright (c) 2018 Linaro.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>
> Wrong license.
See above.
>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef RID_MAP_H
>
> We usually had the directory name in the guard to prevent potential
ok will change it
>
>> +#define RID_MAP_H
>> +
>> +#include <xen/acpi.h>
>> +
>> +struct id_map_struct
>
> Saying struct in the name is a bit pointless given you will always use 
> it with "struct ..." before.
>
> I would also appreciate some documentation within this header.
ok
>
>
>> +{
>> +    u16 input_base;
>
> uint_ same below.
>
>> +    u32 output_base;
>> +    u16 id_count;
>> +};
>> +
>> +struct rid_streamid_map
>> +{
>> +    struct acpi_iort_node *pcirc_node;
>> +    struct id_map_struct idmap;
>> +    struct list_head entry;
>> +    struct acpi_iort_node *smmu_node; > +};
>> +
>> +struct rid_deviceid_map
>> +{
>> +    struct acpi_iort_node *pcirc_node;
>> +    struct acpi_iort_node *its_node;
>> +    struct id_map_struct idmap;
>> +    struct list_head entry;
>> +};
>> +
>> +extern struct list_head rid_streamid_map_list;
>> +extern struct list_head rid_deviceid_map_list;
>
> I am not a big fan of exporting those 2 maps. But I will see how you 
> use it before commenting.
Lot of xen code using the same way. Is that all wrong?
>
> For the rest of the code, see my comments in the patch.
>
>> +
>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>> +                         struct acpi_iort_node *smmu_node,
>> +                         u32 input_base, u32 output_base, u32 
>> id_count);
>> +
>> +int add_rid_deviceid_map(struct acpi_iort_node *pcirc_node,
>> +                         struct acpi_iort_node *its_node,
>> +                         u32 input_base, u32 output_base, u32 
>> id_count);
>> +
>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 
>> *streamid,
>> +                    struct acpi_iort_node **smmu_node);
>> +
>> +void query_deviceid(struct acpi_iort_node *pcirc_node,
>> +                    u16 rid, u32 *deviceid);
>> +
>> +void init_ridmaps(void);
>> +
>> +#endif
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
>
> Cheers,
>

On 19/01/18 06:05, Manish Jaggi wrote:
> 
> On 01/17/2018 12:01 AM, Julien Grall wrote:
>> Hi Manish,
>>
> Hi Julien,
> Thanks for reviewing the patch.
> 
>> I sent the previous e-mail too soon.
>>
>> On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
>>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>>
>>>   Public API to populate and query map between requester id and
>>>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>>>   and two lists are created one for mapping between reuesterId and 
>>> streamid
>>>   and another between requesterID and deviceID.
>>>
>>>   These lists eliminate the need to reparse IORT for querying streamid
>>>   or deviceid using requesterid.
>>>
>>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>>> ---
>>>   xen/drivers/acpi/Makefile     |   1 +
>>>   xen/drivers/acpi/arm/Makefile |   1 +
>>
>> We have a directory arch/arm/acpi/. So please move all your code there.
> The current files in arch/arm/acpi hold only boot time/low level code.
> IMHO creating drivers/acpi/arm makes more sense.
> Linux also has iort code in drivers/acpi/arm.

drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is 
AFAICT Arm specific. So arch/arm/acpi is a better place.

[...]

>>
>>> + *
>>> + * Manish Jaggi <manish.jaggi@linaro.org>
>>> + * Copyright (c) 2018 Linaro.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>
>> Xen is GPLv2 only and hence the copyright wrong. You want to use:
>>
>> 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.
> 
> I picked this copyright from xen/arch/arm/traps.c.
> 
>   * xen/arch/arm/traps.c
>   *
>   * ARM Trap handlers
>   *
>   * Copyright (c) 2011 Citrix Systems.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
>   * the Free Software Foundation; either version 2 of the Licese, or
>   * (at your option) any later version.
> 
> So IIUYC, traps.c copyright is also wrong.
> How do we plan to fix all other files in xen code which use the same 
> copyright.

https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html

>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <acpi/ridmap.h>
>>> +#include <xen/iommu.h>
>>> +#include <xen/kernel.h>
>>> +#include <xen/list.h>
>>> +#include <xen/pci.h>
>>> +
>>> +struct list_head rid_streamid_map_list;
>>> +struct list_head rid_deviceid_map_list;
>>
>> Please drop _list. This is pointless to know that when you can 
>> discover it.
> 
> I think it is not pointless.
> There is a point here. :)
>   _list is added to show that it is a list to make it more verbose.
> Without _list the name could mean a single mapping as well.

When I read, rid_streamid_map. I understand it is a map of rid/streamid. 
Not a single mapping. But ...

> 
> If you care to see
> xen/common/rangeset.c:27:    struct list_head range_list;
> 
> I hope you can appreciate the point.

... look at the name length here, 10 characters. Yours is 22 characters. 
This is 1/4 of a line. That's just stupid.

>>
>> Also, can you explain the rationale of using an unsorted list over 
>> another structure? 
> Since rid - streamId mapping also requires pcirc_node so it would 
> require two level of sorting.
> First sort based on pcirc_node and next on basis of rid.
> Does it makes sense to have all that complex code here ?
>   as this API will be used only once per pci device
>> Along that please give an idea how often and where the query API will 
>> be used.
> ok
> BTW, this is called from pci_for_each_dma_alias code flow.

The document the rationale.

>>
>>> +
>>> +void init_ridmaps(void)
>>
>> This likely need to be __init.
> ok.
>>
>>> +{
>>> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
>>> +    INIT_LIST_HEAD(&rid_streamid_map_list);
>>> +}
>>
>> This function is not necessary. Declaring 
>> LIST_HEAD(rid_streamid_map_list) will do the trick.
> ok.
>>> +
>>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>>
>> Ditto.
> Ditto for? Sorry didnt catch your point here.

__init.

>>
>>> +                         struct acpi_iort_node *smmu_node,
>>> +                         u32 input_base, u32 output_base, u32 id_count)
>>
>> u32 & co should not be used in new code (unless imported from Linux). 
>> Please use uint32_t & co.
> I couldn't find this in xen coding style document.
> Could you please point to the section which says u32 should not be used.

It is not in the coding style but Xen is phasing out from u*. What's the 
problem?

> 
>>
>>> +{
>>> +    struct rid_streamid_map *rid_map;
>>
>> Newline here as it should be between after declarations.
>>
> ok
>>
>>> +    rid_map = xzalloc(struct rid_streamid_map);
>>> +
>>> +    if (!rid_map)
>>
>> This should be ( ... ).
>>
>>> +        return -ENOMEM;
>>
>> You either return -ENOMEM or 0 in this function. It sounds like to me 
>> that bool would be the best.
> I think ENOMEM should be used here. The error code is designed 
> specifically for this purpose.

Fair enough.

>>> +    return 0;
>>> +}
>>> +
>>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, u32 
>>> *streamid,
>>
>> s/u*/uint_/
>>
>> But how come the rid is 16-bit here when Linux is using 32-bit?
> IIUC rid is 16bit only. Dont know why linux is using 32bit.
> 
> rid = bus - 8bits , devfn 8bits.
> 
>  From PCI Express specification
> The Requester ID is a 16-bit value that is unique for every PCI Express 
> Function within a Hierarchy..
> 
> If you think it is a 32bit value please let me know how to use upper 16 
> bits.

Well AFAICT, the IORT stores 32-bit. So it is probably best to stick 
with it.

> 
>>
>> Also, I am a bit puzzled how the caller is expected to use it. 
> I thought it would be self explanatory query streamid based on rid.
> But if it is not verbose enough for you, I will add this explicitly.

Not at all. More than this function is returning void. A query function 
is usually return a bool/int.

> 
>> From the name I would expect the function to return whether a 
>> translation was found. But it returns void.
>>
>> IHMO, this is a pretty bad idea and make more expectation on the value 
>> for the caller.
>>
>> Lastly, I would appreciate documentation on at least the function 
>> exported.
> ok.
>>
>>> +                    struct acpi_iort_node **smmu_node)
>>> +{
>>> +    struct rid_streamid_map *rmap;
>>> +
>>> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
>>> +    {
>>> +        if (rmap->pcirc_node == pcirc_node)
>>
>> Coding style.
> Can we have a checkpatch.pl for xen.
> This would help in cases when code has a mix of files with linux coding 
> style and xen coding style.

This is been worked on.

[...]

>> I am not a big fan of exporting those 2 maps. But I will see how you 
>> use it before commenting.
> Lot of xen code using the same way. Is that all wrong?

Second sentence: "I will see how you use it before commenting".

Cheers,

On 01/19/2018 05:33 PM, Julien Grall wrote:
>
>
> On 19/01/18 06:05, Manish Jaggi wrote:
>>
>> On 01/17/2018 12:01 AM, Julien Grall wrote:
>>> Hi Manish,
>>>
>> Hi Julien,
>> Thanks for reviewing the patch.
>>
>>> I sent the previous e-mail too soon.
>>>
>>> On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
>>>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>>>
>>>>   Public API to populate and query map between requester id and
>>>>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>>>>   and two lists are created one for mapping between reuesterId and 
>>>> streamid
>>>>   and another between requesterID and deviceID.
>>>>
>>>>   These lists eliminate the need to reparse IORT for querying streamid
>>>>   or deviceid using requesterid.
>>>>
>>>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>>>> ---
>>>>   xen/drivers/acpi/Makefile     |   1 +
>>>>   xen/drivers/acpi/arm/Makefile |   1 +
>>>
>>> We have a directory arch/arm/acpi/. So please move all your code there.
>> The current files in arch/arm/acpi hold only boot time/low level code.
>> IMHO creating drivers/acpi/arm makes more sense.
>> Linux also has iort code in drivers/acpi/arm.
>
> drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is 
> AFAICT Arm specific. So arch/arm/acpi is a better place.
ok, I will move the code.
What about iort.c ?
Should it be in drivers/acpi/arm
>
> [...]
>
>>>
>>>> + *
>>>> + * Manish Jaggi <manish.jaggi@linaro.org>
>>>> + * Copyright (c) 2018 Linaro.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or 
>>>> modify
>>>> + * it under the terms of the GNU General Public License as 
>>>> published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>
>>> Xen is GPLv2 only and hence the copyright wrong. You want to use:
>>>
>>> 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.
>>
>> I picked this copyright from xen/arch/arm/traps.c.
>>
>>   * xen/arch/arm/traps.c
>>   *
>>   * ARM Trap handlers
>>   *
>>   * Copyright (c) 2011 Citrix Systems.
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License as published by
>>   * the Free Software Foundation; either version 2 of the Licese, or
>>   * (at your option) any later version.
>>
>> So IIUYC, traps.c copyright is also wrong.
>> How do we plan to fix all other files in xen code which use the same 
>> copyright.
>
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html 
>
I was not aware of this patch, will change the copyright.
Please add what copyright to be used in Xen coding guidelines document.
Moreover the patch to fix copyright is still not merged, any specific 
reason?
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#include <acpi/ridmap.h>
>>>> +#include <xen/iommu.h>
>>>> +#include <xen/kernel.h>
>>>> +#include <xen/list.h>
>>>> +#include <xen/pci.h>
>>>> +
>>>> +struct list_head rid_streamid_map_list;
>>>> +struct list_head rid_deviceid_map_list;
>>>
>>> Please drop _list. This is pointless to know that when you can 
>>> discover it.
>>
>> I think it is not pointless.
>> There is a point here. :)
>>   _list is added to show that it is a list to make it more verbose.
>> Without _list the name could mean a single mapping as well.
>
> When I read, rid_streamid_map. I understand it is a map of 
> rid/streamid. Not a single mapping. But ...
But In code the rid_streamid_map is used for a single mapping, so there 
has to be a proper naming.
Thats why I used a _list.

range_t and range_list. Same logic here.
>
>>
>> If you care to see
>> xen/common/rangeset.c:27:    struct list_head range_list;
>>
>> I hope you can appreciate the point.
>
> ... look at the name length here, 10 characters. Yours is 22 
> characters. This is 1/4 of a line. That's just stupid.
So your reservation is not against _list but 22 characters ?
I dont consider this stupid at all. The list is accessed in few 
functions only and it makes code more readable.
Anyway, how about rid_sid_map_list? or rid_sid_list ? Which one you prefer.
>>>
>>> Also, can you explain the rationale of using an unsorted list over 
>>> another structure? 
>> Since rid - streamId mapping also requires pcirc_node so it would 
>> require two level of sorting.
>> First sort based on pcirc_node and next on basis of rid.
>> Does it makes sense to have all that complex code here ?
>>   as this API will be used only once per pci device
>>> Along that please give an idea how often and where the query API 
>>> will be used.
>> ok
>> BTW, this is called from pci_for_each_dma_alias code flow.
>
> The document the rationale.
The/Then?
Yes I will do in v2.
>
>>>
>>>> +
>>>> +void init_ridmaps(void)
>>>
>>> This likely need to be __init.
>> ok.
>>>
>>>> +{
>>>> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
>>>> +    INIT_LIST_HEAD(&rid_streamid_map_list);
>>>> +}
>>>
>>> This function is not necessary. Declaring 
>>> LIST_HEAD(rid_streamid_map_list) will do the trick.
>> ok.
>>>> +
>>>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>>>
>>> Ditto.
>> Ditto for? Sorry didnt catch your point here.
>
> __init.
_init might not be valid, I will add more documentation and you can see 
the rationale.
>
>>>
>>>> +                         struct acpi_iort_node *smmu_node,
>>>> +                         u32 input_base, u32 output_base, u32 
>>>> id_count)
>>>
>>> u32 & co should not be used in new code (unless imported from 
>>> Linux). Please use uint32_t & co. /.
>> I couldn't find this in xen coding style document.
>> Could you please point to the section which says u32 should not be used.
>
> It is not in the coding style but Xen is phasing out from u*. What's 
> the problem?
There is no problem, but It is not written anywhere so I was not sure of 
your comment.
I suggest it should be _mentioned_ in xen coding guidelines.
This will avoid all unwritten standards not being followed issues.
>
>>
>>>
>>>> +{
>>>> +    struct rid_streamid_map *rid_map;
>>>
>>> Newline here as it should be between after declarations.
>>>
>> ok
>>>
>>>> +    rid_map = xzalloc(struct rid_streamid_map);
>>>> +
>>>> +    if (!rid_map)
>>>
>>> This should be ( ... ).
>>>
>>>> +        return -ENOMEM;
>>>
>>> You either return -ENOMEM or 0 in this function. It sounds like to 
>>> me that bool would be the best.
>> I think ENOMEM should be used here. The error code is designed 
>> specifically for this purpose.
>
> Fair enough.
Thanks it is one of rare instances where you agree to my point.
>
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, 
>>>> u32 *streamid,
>>>
>>> s/u*/uint_/
>>>
>>> But how come the rid is 16-bit here when Linux is using 32-bit?
>> IIUC rid is 16bit only. Dont know why linux is using 32bit.
>>
>> rid = bus - 8bits , devfn 8bits.
>>
>>  From PCI Express specification
>> The Requester ID is a 16-bit value that is unique for every PCI 
>> Express Function within a Hierarchy..
>>
>> If you think it is a 32bit value please let me know how to use upper 
>> 16 bits.
>
> Well AFAICT, the IORT stores 32-bit. So it is probably best to stick 
> with it.
IORT uses 32bit and refers as input_base.
That is generic in acpi_iort_id_mapping.
Does IORT spec uses term requesterID and defines its width as 32bit?

I can keep 32bit anyways.
>
>>
>>>
>>> Also, I am a bit puzzled how the caller is expected to use it. 
>> I thought it would be self explanatory query streamid based on rid.
>> But if it is not verbose enough for you, I will add this explicitly.
>
> Not at all. More than this function is returning void. A query 
> function is usually return a bool/int.
ok will change.
>
>>
>>> From the name I would expect the function to return whether a 
>>> translation was found. But it returns void.
>>>
>>> IHMO, this is a pretty bad idea and make more expectation on the 
>>> value for the caller.
>>>
>>> Lastly, I would appreciate documentation on at least the function 
>>> exported.
>> ok.
>>>
>>>> +                    struct acpi_iort_node **smmu_node)
>>>> +{
>>>> +    struct rid_streamid_map *rmap;
>>>> +
>>>> +    list_for_each_entry(rmap, &rid_streamid_map_list, entry)
>>>> +    {
>>>> +        if (rmap->pcirc_node == pcirc_node)
>>>
>>> Coding style.
>> Can we have a checkpatch.pl for xen.
>> This would help in cases when code has a mix of files with linux 
>> coding style and xen coding style.
>
> This is been worked on.
Great!
>
> [...]
>
>>> I am not a big fan of exporting those 2 maps. But I will see how you 
>>> use it before commenting.
>> Lot of xen code using the same way. Is that all wrong?
>
> Second sentence: "I will see how you use it before commenting".
>
In that case, the comment was unwanted..
> Cheers,
>

Hi,

On 22/01/18 05:07, Manish Jaggi wrote:
> On 01/19/2018 05:33 PM, Julien Grall wrote:
>>
>>
>> On 19/01/18 06:05, Manish Jaggi wrote:
>>>
>>> On 01/17/2018 12:01 AM, Julien Grall wrote:
>>>> Hi Manish,
>>>>
>>> Hi Julien,
>>> Thanks for reviewing the patch.
>>>
>>>> I sent the previous e-mail too soon.
>>>>
>>>> On 02/01/18 09:27, manish.jaggi@linaro.org wrote:
>>>>> From: Manish Jaggi <manish.jaggi@linaro.org>
>>>>>
>>>>>   Public API to populate and query map between requester id and
>>>>>   streamId/DeviceID. IORT is parsed one time (outside this patch)
>>>>>   and two lists are created one for mapping between reuesterId and 
>>>>> streamid
>>>>>   and another between requesterID and deviceID.
>>>>>
>>>>>   These lists eliminate the need to reparse IORT for querying streamid
>>>>>   or deviceid using requesterid.
>>>>>
>>>>>   Signed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
>>>>> ---
>>>>>   xen/drivers/acpi/Makefile     |   1 +
>>>>>   xen/drivers/acpi/arm/Makefile |   1 +
>>>>
>>>> We have a directory arch/arm/acpi/. So please move all your code there.
>>> The current files in arch/arm/acpi hold only boot time/low level code.
>>> IMHO creating drivers/acpi/arm makes more sense.
>>> Linux also has iort code in drivers/acpi/arm.
>>
>> drivers/acpi mostly contain generic ACPI code. ridmap.c and iort.c is 
>> AFAICT Arm specific. So arch/arm/acpi is a better place.
> ok, I will move the code.
> What about iort.c ?
> Should it be in drivers/acpi/arm

I think they should be both in arch/arm/acpi. Stefano do you have any 
opinion?

>>
>> [...]
>>
>>>>
>>>>> + *
>>>>> + * Manish Jaggi <manish.jaggi@linaro.org>
>>>>> + * Copyright (c) 2018 Linaro.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or 
>>>>> modify
>>>>> + * it under the terms of the GNU General Public License as 
>>>>> published by
>>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>>> + * (at your option) any later version.
>>>>
>>>> Xen is GPLv2 only and hence the copyright wrong. You want to use:
>>>>
>>>> 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.
>>>
>>> I picked this copyright from xen/arch/arm/traps.c.
>>>
>>>   * xen/arch/arm/traps.c
>>>   *
>>>   * ARM Trap handlers
>>>   *
>>>   * Copyright (c) 2011 Citrix Systems.
>>>   *
>>>   * This program is free software; you can redistribute it and/or modify
>>>   * it under the terms of the GNU General Public License as published by
>>>   * the Free Software Foundation; either version 2 of the Licese, or
>>>   * (at your option) any later version.
>>>
>>> So IIUYC, traps.c copyright is also wrong.
>>> How do we plan to fix all other files in xen code which use the same 
>>> copyright.
>>
>> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg02899.html 
>>
> I was not aware of this patch, will change the copyright.
> Please add what copyright to be used in Xen coding guidelines document.

Patch are welcomed for coding style improvement.

> Moreover the patch to fix copyright is still not merged, any specific 
> reason?

Because it is unclear how you can modify the header in current code. 
Whether you need an ack from everyone who has touched the code. See the 
thread.

New code should use the GPLv2 copyright to avoid such dilemma.

>>>>> + *
>>>>> + * 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.
>>>>> + */
>>>>> +
>>>>> +#include <acpi/ridmap.h>
>>>>> +#include <xen/iommu.h>
>>>>> +#include <xen/kernel.h>
>>>>> +#include <xen/list.h>
>>>>> +#include <xen/pci.h>
>>>>> +
>>>>> +struct list_head rid_streamid_map_list;
>>>>> +struct list_head rid_deviceid_map_list;
>>>>
>>>> Please drop _list. This is pointless to know that when you can 
>>>> discover it.
>>>
>>> I think it is not pointless.
>>> There is a point here. :)
>>>   _list is added to show that it is a list to make it more verbose.
>>> Without _list the name could mean a single mapping as well.
>>
>> When I read, rid_streamid_map. I understand it is a map of 
>> rid/streamid. Not a single mapping. But ...
> But In code the rid_streamid_map is used for a single mapping, so there 
> has to be a proper naming.
> Thats why I used a _list.
> 
> range_t and range_list. Same logic here.
>>
>>>
>>> If you care to see
>>> xen/common/rangeset.c:27:    struct list_head range_list;
>>>
>>> I hope you can appreciate the point.
>>
>> ... look at the name length here, 10 characters. Yours is 22 
>> characters. This is 1/4 of a line. That's just stupid.
> So your reservation is not against _list but 22 characters ?
> I dont consider this stupid at all. The list is accessed in few 
> functions only and it makes code more readable.
> Anyway, how about rid_sid_map_list? or rid_sid_list ? Which one you prefer.

I prefer rid_sid_list.

>>>>
>>>> Also, can you explain the rationale of using an unsorted list over 
>>>> another structure? 
>>> Since rid - streamId mapping also requires pcirc_node so it would 
>>> require two level of sorting.
>>> First sort based on pcirc_node and next on basis of rid.
>>> Does it makes sense to have all that complex code here ?
>>>   as this API will be used only once per pci device
>>>> Along that please give an idea how often and where the query API 
>>>> will be used.
>>> ok
>>> BTW, this is called from pci_for_each_dma_alias code flow.
>>
>> The document the rationale.
> The/Then?

Yes then...

> Yes I will do in v2.
>>
>>>>
>>>>> +
>>>>> +void init_ridmaps(void)
>>>>
>>>> This likely need to be __init.
>>> ok.
>>>>
>>>>> +{
>>>>> +    INIT_LIST_HEAD(&rid_deviceid_map_list);
>>>>> +    INIT_LIST_HEAD(&rid_streamid_map_list);
>>>>> +}
>>>>
>>>> This function is not necessary. Declaring 
>>>> LIST_HEAD(rid_streamid_map_list) will do the trick.
>>> ok.
>>>>> +
>>>>> +int add_rid_streamid_map(struct acpi_iort_node *pcirc_node,
>>>>
>>>> Ditto.
>>> Ditto for? Sorry didnt catch your point here.
>>
>> __init.
> _init might not be valid, I will add more documentation and you can see 
> the rationale.

Do you mind giving the rationale now? Why would you call this function 
after boot?

>>
>>>>
>>>>> +                         struct acpi_iort_node *smmu_node,
>>>>> +                         u32 input_base, u32 output_base, u32 
>>>>> id_count)
>>>>
>>>> u32 & co should not be used in new code (unless imported from 
>>>> Linux). Please use uint32_t & co. /.
>>> I couldn't find this in xen coding style document.
>>> Could you please point to the section which says u32 should not be used.
>>
>> It is not in the coding style but Xen is phasing out from u*. What's 
>> the problem?
> There is no problem, but It is not written anywhere so I was not sure of 
> your comment.
> I suggest it should be _mentioned_ in xen coding guidelines.
> This will avoid all unwritten standards not being followed issues.

Patch for coding style improvement are welcomed.

>>
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +void query_streamid(struct acpi_iort_node *pcirc_node, u16 rid, 
>>>>> u32 *streamid,
>>>>
>>>> s/u*/uint_/
>>>>
>>>> But how come the rid is 16-bit here when Linux is using 32-bit?
>>> IIUC rid is 16bit only. Dont know why linux is using 32bit.
>>>
>>> rid = bus - 8bits , devfn 8bits.
>>>
>>>  From PCI Express specification
>>> The Requester ID is a 16-bit value that is unique for every PCI 
>>> Express Function within a Hierarchy..
>>>
>>> If you think it is a 32bit value please let me know how to use upper 
>>> 16 bits.
>>
>> Well AFAICT, the IORT stores 32-bit. So it is probably best to stick 
>> with it.
> IORT uses 32bit and refers as input_base.
> That is generic in acpi_iort_id_mapping.
> Does IORT spec uses term requesterID and defines its width as 32bit?

The IORT spec does not seem to say anything about the width of 
requesterID. So I prefer to stay on the safe side on use 32-bit.

[...]

>>>> I am not a big fan of exporting those 2 maps. But I will see how you 
>>>> use it before commenting.
>>> Lot of xen code using the same way. Is that all wrong?
>>
>> Second sentence: "I will see how you use it before commenting".
>>
> In that case, the comment was unwanted..
Probably less than "Thanks it is one of rare instances where you agree 
to my point." ;).

Cheers,