Re: [Xen-devel] [PATCH resend 03/13] acpi: arm: Code to generate Hardware Domains IORT

[Julien Grall <julien.grall@xxxxxxx>]

Hi Manish,

On 03/13/2018 03:20 PM, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>
>
> Structure of Hardware domain's (hwdom) IORT
>
> hwdom's IORT will only have PCIRC nodes and ITS group nodes
> in the following order. SMMU nodes as they are hidden from hardware
> domain.
>
> [IORT Header]
> [ITS Group 1 ]
> ...
> [ITS Group n ]
> [PCIRC Node 1]
>    [PCIRC IDMAP entry 1]
>    ...
>    [PCIRC IDMAP entry m]
> ...
> [PCIRC Node p]
>    [PCIRC IDMAP entry 1]
>    ...
>    [PCIRC IDMAP entry q]
> ...
> *n,m,p are variable.
>
> requesterid-deviceid mapping list (rid_devid_list) populated by
> parsing IORT is used to generate hwdom IORT.
>
> As the rid_devid_list is populated from firmware IORT, IDMAP entry
> would have output references offsets based on firmware's IORT.
> It is required to fixup node offset of ITS Group Nodes in the PCIRC
> idmap (output_reference)
>
> First write all the ITS group nodes in the hwdom's IORT. For this
> write_hwits_nodes is called, which parses the rid_devid_list and for
> each unique its_node in firmware IORT create a its_node in hwdom's
> IORT and also creates and entry in fwits_hwits_map.
>
> fwits_hwits_map is a mapping between firmware IORT's its node
> and the node offset of the corresponding its_node stored in the
> hwdom's IORT.
>
> This map can later be used to set output reference value in hwdom's
> pcirc node's idmap entries.
>
> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/acpi/gen-iort.c        | 299 ++++++++++++++++++++++++++++++++++++
>   xen/arch/arm/domain_build.c         |  35 +++++
>   xen/include/asm-arm/acpi.h          |   1 +
>   xen/include/asm-arm/acpi/gen-iort.h |  11 ++
>   4 files changed, 346 insertions(+)
>
> diff --git a/xen/arch/arm/acpi/gen-iort.c b/xen/arch/arm/acpi/gen-iort.c
> index 687c4f18ee..251a9771e3 100644
> --- a/xen/arch/arm/acpi/gen-iort.c
> +++ b/xen/arch/arm/acpi/gen-iort.c
> @@ -19,6 +19,305 @@
>   
>   #include <asm/acpi/ridmap.h>
>   #include <xen/acpi.h>
> +#include <acpi/actables.h>
> +
> +/*
> + * Structure of Hardware domain's (hwdom) IORT
> + * -----------------------------------
> + *
> + * hwdom's IORT will only have PCIRC nodes and ITS group nodes
> + * in the following order.
> + *
> + * [IORT Header]
> + * [ITS Group 1 ]
> + * ...
> + * [ITS Group N ]
> + * [PCIRC Node 1]
> + * [PCIRC IDMAP entry 1]
> + * ...
> + * [PCIRC IDMAP entry N]
> + * ...
> + * [PCIRC Node N]
> + *
> + * requesterid-deviceid mapping list (rid_devid_list) populated by parsing IORT
> + * is used to generate hwdom IORT.
> + *
> + * One of the challanges is to fixup node offset of ITS Group Nodes
s/challanges/challenges/

> + * in the PCIRC idmap (output_reference)
> + *
> + * In rid_devid_map firmware IORT's ITS group node pointer in stored.
> + *
> + * We first write all the ITS group nodes in the hwdom's IORT. For this
> + * write_hwits_nodes is called, which parses the rid_devid_list and for
> + * each unique its_node in firmware IORT create a its_node in hwdom's IORT
> + * and also creates and entry in fwits_hwits_map.
> + *
> + * fwits_hwits_map is a mapping between firmware IORT's its node
> + * and the node offset of the corresponding its_node stored in the
> + * hwdom's IORT.
> + *
> + * This map can be later used to set output reference value in hwdom's
> + * pcirc node's idmap entries.
> + *
> + */
> +
> +/*
> + * Stores the mapping between firmware tables its group node
> + * to the offset of the equivalent its node to be stored in
> + * hwdom's IORT.
> + */
> +struct fwits_hwits_map
> +{
> +    struct acpi_iort_node *fwits_node;
> +    unsigned int hwitsnode_offset;
> +    struct list_head entry;
> +};
> +
> +LIST_HEAD(fwits_hwits_list);
As said in the previous version, I think this should be static.

> +
> +/*
> + * is_uniq_fwits_node
> + *
> + * returns 1 - if fwits_node is not already in the its_map_list
> + *         0 - if it is present already
It also returns -ENOMEM when you can't allocate memory.

> + *
> + * fwits_node - ITS Node pointer in Firmware IORT
> + * offset     - offset of the equivalent its node to be stored in
> + *              hwdom's IORT
> + */
> +static int is_uniq_fwits_node(struct acpi_iort_node *fwits_node,
The name is a bit odd given that you add the ITS node. On the previous 
version, I requested to document that behavior...
But you likely want to rename the function to add_fwits_node(...) or 
something similar.
> +                              unsigned int offset)
> +{
> +    struct fwits_hwits_map *map;
> +
> +    list_for_each_entry(map, &fwits_hwits_list, entry)
> +    {
> +        if ( map->fwits_node == fwits_node )
> +            return 0;
> +    }
> +
> +    map = xzalloc(struct fwits_hwits_map);
Where this memory is going to be freed?

> +    if ( !map )
> +        return -ENOMEM;
> +
> +    map->fwits_node = fwits_node;
> +    map->hwitsnode_offset = offset;
> +    list_add_tail(&map->entry, &fwits_hwits_list);
> +
> +    return 1;
> +}
> +
> +/*
> + * Returns the offset of corresponding its node to fwits_node
> + * written in hwdom's IORT.
> + *
> + * This function would be used when write hwdoms pcirc nodes' idmap
> + * entries.
> + */
> +static
> +unsigned int hwitsnode_offset_from_map(struct acpi_iort_node *fwits_node)
> +{
> +    struct fwits_hwits_map *map;
> +
> +    list_for_each_entry(map, &fwits_hwits_list, entry)
> +    {
> +        if ( map->fwits_node == fwits_node )
> +            return map->hwitsnode_offset;
> +    }
> +
> +    return 0;
0 could never be a valid offset, right?

> +}
> +
> +static void write_hwits_nodes(u8 *iort, unsigned int *offset,
Please avoid using u* and use uint_* instead. I expect that you fix all 
of for the next version.
Here, I think you want to use void *.

> +                              unsigned int *num_nodes)
> +{
> +    struct rid_devid_map *rmap;
> +    unsigned int of = *offset;
Please name it off. This is clearer that it is an offset. But as I said 
on the previous version, why can't you just re-use offset?
> +    int n = 0;
> +
> +    /*
> +     * rid_devid_list is iterated to get unique its group nodes
> +     * Each unique ITS group node is written in hardware domains IORT
> +     * by using some values from the firmware ITS group node.
> +     */
> +    list_for_each_entry(rmap, &rid_devid_list, entry)
> +    {
> +        struct acpi_iort_node *node;
> +        struct acpi_iort_its_group *grp;
> +        struct acpi_iort_its_group *fw_grp;
> +
> +        /* save its_node_offset_map in a list uniquely */
> +        if ( is_uniq_fwits_node(rmap->its_node, of) == 1 )
If the function is returning -ENOMEM, then you will ignore the node 
without a warning. That's going to be a real pain to find out a ITS node 
is not present if that happen.
Here, you should propagate error if something wrong is going.

> +        {
> +            node = (struct acpi_iort_node *) &iort[of];
> +            grp = (struct acpi_iort_its_group *)(&node->node_data);
> +
> +            node->type = ACPI_IORT_NODE_ITS_GROUP;
> +            node->length = sizeof(struct acpi_iort_node) +
> +                           sizeof(struct acpi_iort_its_group) -
> +                           sizeof(node->node_data);
While the substraction is good, this is odd enough to warrant a comment. 
But likely But likely you want to provide macros for defining the 
length. This will clean a lot the code.
> +
> +            node->revision = rmap->its_node->revision;
I am not sure this is right. You rewrite the IORT based on a given 
revision. Imagine the host IORT get updated to a newer spec but not Xen. 
Then you would end up to have the wrong revision number here.
> +            node->reserved = 0;
> +            node->mapping_count = 0;
> +            node->mapping_offset= 0;
> +
> +            fw_grp = (struct acpi_iort_its_group 
> *)(&rmap->its_node->node_data);
> +
> +            /* Copy its_count and identifiers from firmware iort's its_node */
> +            grp->its_count = fw_grp->its_count;
> +            grp->identifiers[0] = fw_grp->identifiers[0];
Hmmm, here you will only copy the first identifier. What if you have 
multiple one?
It would also be good that somewhere (maybe at the top of the file) that 
you rely on the number of ITS and identifiers for the hwdom is the same 
as the host.
> +
> +            of += node->length;
> +            n++;
> +        }
> +    }
> +    *offset = of;
> +    *num_nodes = n;
> +}
> +
> +static void write_hwpcirc_nodes(u8 *iort, unsigned int *pos,
> +                                unsigned int *num_nodes)
> +{
> +    struct acpi_iort_node *opcirc_node, *pcirc_node;
> +    struct acpi_iort_node *hwdom_pcirc_node = NULL;
> +    struct rid_devid_map *rmap;
> +    struct acpi_iort_id_mapping *idmap;
> +    int num_idmap = 0, n = 0;
> +    unsigned int old_pos = *pos;
> +
> +    opcirc_node = NULL;
> +    /* Iterate rid_map_devid list */
> +    list_for_each_entry(rmap, &rid_devid_list, entry)
> +    {
> +        struct acpi_iort_root_complex *rc;
> +        struct acpi_iort_root_complex *rc_fw;
> +        int add_node = 0;
This should be bool.

I am going to stop the review here because I feel I am just repeating 
all my comments from the first version and new ones. So please go 
through *all* my e-mails from the previous version, answer to my 
question, and respin it.
Cheers,

--
Julien Grall

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