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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 03/23/2018 06:58 AM, Julien Grall wrote:
> 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...
I think the name is quite appropriate. Also in this patch I have added 
description of the flow so this should be fairly intuitive.
Could you please let me know the specific point you dont understand, I 
can explain that.
> But you likely want to rename the function to add_fwits_node(...) or 
> something similar.
I think name is quite appropriate.
> > +                              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?
Since this list can be used multiple times even after creation of IORT 
for dom0, say thinking ahead for domUs.
> > +    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?
Yes
See a bug_on when it is used.
> > +}
> > +
> > +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?
I will change it to off, will not break anything.
> > +    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.
ok.
> > +        {
> > +            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.
Not sure if I follow your comment here.
Xen gets host IORT from firmware, how will it get updated?
> 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.
ok
> > +
> > +            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.
Could you please have a look at other patches and if there is 
functionality related change that I need to make, i can add that in my 
next rev.
> Cheers,

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