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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Manish,

I sent the previous e-mail too soon.

On 02/01/18 09:27, manish.jaggi@xxxxxxxxxx wrote:
> From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
>
>   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@xxxxxxxxxx>
> ---
>   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@xxxxxxxxxx>
> + * 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@xxxxxxxxxx>
> + * 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,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel