[Xen-devel,RFC,03/11] acpi: arm: Code to generate Hardware Domains IORT

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

Singed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
---
 xen/arch/arm/domain_build.c     |  28 +++++
 xen/drivers/acpi/arm/gen-iort.c | 253 +++++++++++++++++++++++++++++++++++++++-
 xen/include/acpi/gen-iort.h     |   1 +
 xen/include/asm-arm/acpi.h      |   1 +
 4 files changed, 282 insertions(+), 1 deletion(-)

Hi,

On 02/01/18 09:28, manish.jaggi@linaro.org wrote:
> From: Manish Jaggi <manish.jaggi@linaro.org>
> 

First of all, I would expect a commit message explaining roughly the 
logic. I am only going to skim through this patch and will wait a proper 
description to fully read it.

A general comment, please respect the coding style. Most of error can be 
avoided by using the coding style from the first line you write. And you 
are also saving bandwidth review.

> Singed-off-by: Manish Jaggi <manish.jaggi@linaro.org>
> ---
>   xen/arch/arm/domain_build.c     |  28 +++++
>   xen/drivers/acpi/arm/gen-iort.c | 253 +++++++++++++++++++++++++++++++++++++++-
>   xen/include/acpi/gen-iort.h     |   1 +
>   xen/include/asm-arm/acpi.h      |   1 +
>   4 files changed, 282 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f5d5e3d271..9831943147 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1654,6 +1654,8 @@ static int acpi_create_xsdt(struct domain *d, struct membank tbl_add[])
>                              ACPI_SIG_FADT, tbl_add[TBL_FADT].start);
>       acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
>                              ACPI_SIG_MADT, tbl_add[TBL_MADT].start);
> +    acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
> +                           ACPI_SIG_IORT, tbl_add[TBL_IORT].start);
>       xsdt->table_offset_entry[entry_count] = tbl_add[TBL_STAO].start;
>   
>       xsdt->header.length = table_size;
> @@ -1704,6 +1706,28 @@ static int acpi_create_stao(struct domain *d, struct membank tbl_add[])
>       return 0;
>   }
>   
> +static int acpi_create_iort(struct domain *d, struct membank tbl_add[])
> +{
> +    struct acpi_table_iort *hwdom_table;
> +    unsigned int size = 0;
> +
> +    tbl_add[TBL_IORT].start = d->arch.efi_acpi_gpa
> +                              + acpi_get_table_offset(tbl_add, TBL_IORT);
> +    hwdom_table = d->arch.efi_acpi_table
> +                              + acpi_get_table_offset(tbl_add, TBL_IORT);

This code (and probably other bits of this series) seems to assume that 
IORT is always present. This may not be true, think of a platform with 
no MSI controller or Xen built with no ITS support.

So you have to figure out whether you need to generate the IORT table 
for the hardware domain.

> +
> +    if ( prepare_iort(hwdom_table, &size) )
> +    {
> +        printk("Failed to write IORT table\n");
> +        return -EINVAL;
> +    }
> +    printk("%s %d %d \r\n", __func__, __LINE__, size);

This sounds like a debug message.

> +
> +    tbl_add[TBL_IORT].size = size;
> +    printk("%s %d %d \r\n", __func__, __LINE__, size);

Ditto.

Newline here.

> +    return 0;
> +}
> +
>   static int acpi_create_madt(struct domain *d, struct membank tbl_add[])
>   {
>       struct acpi_table_header *table = NULL;
> @@ -1899,6 +1923,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
>       if ( rc != 0 )
>           return rc;
>   
> +    rc = acpi_create_iort(d, tbl_add);
> +    if ( rc != 0 )
> +        return rc;
> +
>       rc = acpi_create_xsdt(d, tbl_add);
>       if ( rc != 0 )
>           return rc;
> diff --git a/xen/drivers/acpi/arm/gen-iort.c b/xen/drivers/acpi/arm/gen-iort.c
> index 3fc32959c6..f368000753 100644
> --- a/xen/drivers/acpi/arm/gen-iort.c
> +++ b/xen/drivers/acpi/arm/gen-iort.c
> @@ -21,6 +21,257 @@
>   #include <acpi/ridmap.h>
>   #include <xen/acpi.h>
>   
> +/*
> + * Structure of Hardware domain's IORT
> + * -----------------------------------
> + *
> + * Below is the structure of the IORT which this code generates.
> + *
> + * [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 poplated by parsing IORT is used

s/d-/d -/
s/poplated/populated/

> + * to create nodes and idmaps.
> + * We have one small problem, how to resolve the its grooup node offset from

s/grooup/group/

> + * the firmware iort to the ones in hardware domains IORT.

s/iort/IORT/

> + *
> + * Since the ITS group node pointer stored with the rid-devid map is used
> + * to populate the ITS Group nodes in the hardware domains' IORT.
> + * We create another map to save the offset of the ITS group node written
> + * in the hardware domain IORT with the ITS node pointer in the firmware IORT.
> + *
> + * This offset is later used when writing pcirc idmaps output_reference.
> + */
> +struct its_node_offset_map
> +{
> +    struct acpi_iort_node *its_node;
> +    unsigned int offset;
> +    struct list_head entry;
> +};

newline.

> +struct list_head its_map_list;
If you use LIST_HEAD(its_map_list) you don't need to do 
LIST_HEAD_INIT(...) below.

Also, static in front. Unless this should be exported and therefore have 
a corresponding line in the header.

> +
> +int set_its_node_offset(struct acpi_iort_node *its_node, unsigned int offset)

Ditto. I am not going to point all of them. So I expect you to fix them 
by next version.

> +{
> +    struct its_node_offset_map *its_map;

Newline.

> +    list_for_each_entry(its_map, &its_map_list, entry)
> +    {
> +        if ( its_map->its_node == its_node )
> +            return 0;
> +    }

If I get it correctly, this function will create a new ITS node if there 
are not already an ITS node with a given offset. Right? Anyhow, please 
document the function.

> +
> +    its_map = xzalloc(struct its_node_offset_map);
> +    if ( !its_map )
> +        return -ENOMEM;
> +
> +    its_map->its_node = its_node;
> +    its_map->offset = offset;
> +    list_add_tail(&its_map->entry, &its_map_list);
> +
> +    return 0;
> +}

newline.

> +/*
> + * This method would be used in write_pcirc_nodes when writing idmaps

It is not really interesting to know who use it. It is more interesting 
to know what this function does.

> + */
> +unsigned int get_its_node_offset(struct acpi_iort_node *its_node)
> +{
> +    struct its_node_offset_map *its_map;

newline.

> +    list_for_each_entry(its_map, &its_map_list, entry)
> +    {
> +        if ( its_map->its_node == its_node )
> +            return its_map->offset;
> +    }
> +
> +    return 0;
> +}
> +
> +void free_its_node_offset_list(void)
> +{
> +
> +    struct its_node_offset_map *its_map;

Newline here please.

> +    list_for_each_entry(its_map, &its_map_list, entry)
> +        xfree(its_map);

That does not sound really safe. list_for_each_entry will reuse the 
current entry to find the next. But you freed it. So I think you need to 
use list_for_each_safe here.

> +
> +    list_del(&its_map_list);

Shouldn't you remove entry one by one?

> +}
> +
> +void write_its_group(u8 *iort, unsigned int *offset, unsigned int *num_nodes)

Please use uint8_t. But I think this should be char here.

> +{
> +    struct rid_deviceid_map *rmap;
> +    unsigned int of = *offset;

Looking at the code, I don't think of is useful. You can just increment 
*offset directly.

> +    int n=0;

unsigned int n = 0;

Also, newline here please.

> +    INIT_LIST_HEAD(&its_map_list);

No need for this if you use LIST_HEAD(...) for declaring the variable as 
suggested above.

> +    /*
> +     * rid_deviceid_map_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_deviceid_map_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 ( !set_its_node_offset(rmap->its_node, of) )

I am a bit confused here. Shouldn't you return an error if you are 
unable to save the ITS node offset?

> +        {
> +            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);
> +
> +            node->revision = rmap->its_node->revision;

I am not sure this is correct. You copy the revision from the IORT but 
generate it from scratch. What if the host IORT has been upgrade to a 
newer revision? But here you still generate rev 1 (I think). So you will 
confuse the hardware domain kernel.

> +            node->reserved = 0;
> +            node->mapping_count = 0;
> +            node->mapping_offset= 0;

I single line comment would be nice here to explain why mapping_count is 0.

> +
> +            fw_grp = (struct acpi_iort_its_group *)(&rmap->its_node->node_data);
> +
> +            grp->its_count = fw_grp->its_count;
> +            grp->identifiers[0] = fw_grp->identifiers[0];
> +
> +            of += node->length;
> +            n++;
> +        }
> +    }
> +    *offset = of;
> +    *num_nodes = n;
> +}
> +
> +/* It is assumed that rid_map_devid is sorted by pcirc_nodes */
> +void write_pcirc_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_deviceid_map *rmap;
> +    struct acpi_iort_id_mapping *idmap;
> +    int num_idmap = 0, n = 0;

Any integer should be unsigned if you don't plan to use signed value.

> +    unsigned int old_pos = *pos;

Same remark as 'of' above.

> +
> +    opcirc_node = NULL;
> +    /* Iterate rid_map_devid list */
> +    list_for_each_entry(rmap, &rid_deviceid_map_list, entry)
> +    {
> +        struct acpi_iort_root_complex *rc;
> +        struct acpi_iort_root_complex *rc_fw;
> +        int add_node = 0;

bool

> +        pcirc_node = rmap->pcirc_node;
> +
> +        if ( opcirc_node == NULL ) /* First entry */
> +        {
> +            add_node = 1;
> +        }
> +        else if ( opcirc_node != pcirc_node ) /* another pci_rc_node found*/
> +        {
> +            /* All the idmaps of a pcirc are written, now update node info*/
> +            hwdom_pcirc_node->length = num_idmap *
> +                                       sizeof(struct acpi_iort_id_mapping) +
> +                                       sizeof(struct acpi_iort_node) +
> +                                       sizeof(struct acpi_iort_root_complex) -
> +                                       sizeof(pcirc_node->node_data);

Can you abstract the sizeof(...) + ... there are very similar in this 
function and make quite difficult to read the code.

> +
> +            hwdom_pcirc_node->mapping_count = num_idmap;
> +            hwdom_pcirc_node->mapping_offset = sizeof(struct acpi_iort_node) +
> +                                               sizeof(struct acpi_iort_root_complex) -
> +                                               sizeof(pcirc_node->node_data);
> +            old_pos += hwdom_pcirc_node->length;
> +            add_node = 1;
> +        }
> +
> +        if ( add_node ) /* create the pcirc node */
> +        {
> +            opcirc_node = pcirc_node;
> +            hwdom_pcirc_node = (struct acpi_iort_node *)&iort[old_pos];
> +            hwdom_pcirc_node->type = ACPI_IORT_NODE_PCI_ROOT_COMPLEX;
> +            hwdom_pcirc_node->mapping_offset = sizeof(struct acpi_iort_node) +
> +                                               sizeof(struct acpi_iort_root_complex) -
> +                                               sizeof(hwdom_pcirc_node->node_data);
> +
> +            rc = (struct acpi_iort_root_complex *)
> +                  &hwdom_pcirc_node->node_data;

This is quite difficult to read. Is there any possibility to introduce a 
macro or just break down function?

> +
> +            rc_fw = (struct acpi_iort_root_complex *)
> +                     &pcirc_node->node_data;
> +
> +            rc->pci_segment_number = rc_fw->pci_segment_number;
> +            rc->ats_attribute = rc_fw->ats_attribute;
> +            rc->memory_properties = rc_fw->memory_properties;
> +
> +            idmap = ACPI_ADD_PTR(struct acpi_iort_id_mapping,
> +                                 hwdom_pcirc_node,
> +                                 hwdom_pcirc_node->mapping_offset);
> +            n++;
> +            num_idmap = 0;
> +        }
> +
> +        idmap->input_base = rmap->idmap.input_base;
> +        idmap->id_count = rmap->idmap.id_count;
> +        idmap->output_base = rmap->idmap.output_base;
> +        idmap->output_reference = get_its_node_offset(rmap->its_node);
> +        idmap->flags = 0;
> +
> +        idmap++;
> +        num_idmap++;
> +    }
> +
> +    if ( hwdom_pcirc_node ) /* if no further PCIRC nodes found */
> +    {
> +        /* All the idmaps of a pcirc are written, now update node info*/
> +        hwdom_pcirc_node->length = num_idmap *
> +                                   sizeof(struct acpi_iort_id_mapping) +
> +                                   sizeof(struct acpi_iort_node) +
> +                                   sizeof(struct acpi_iort_root_complex) -1;
> +
> +        hwdom_pcirc_node->mapping_count = num_idmap;
> +        old_pos += hwdom_pcirc_node->length;
> +    }
> +
> +    *pos = old_pos;
> +    *num_nodes = n;
> +}
> +
> +int prepare_iort(struct acpi_table_iort *hwdom_iort, unsigned int *iort_size)
> +{
> +    struct acpi_table_iort *fw_iort;
> +    unsigned int num_nodes = 0;
> +    unsigned int pos;
> +
> +    pos = sizeof(struct acpi_table_iort);
> +
> +    if ( acpi_get_table(ACPI_SIG_IORT, 0,
> +         (struct acpi_table_header **)&fw_iort) )
> +    {
> +        printk("Failed to get IORT table\n");
> +        return -ENODEV;
> +    }
> +
> +    /* Write IORT header */
> +    ACPI_MEMCPY(hwdom_iort, fw_iort, sizeof(struct acpi_table_iort));

Please use memcpy. But is it valid to copy the header from the host one?


> +    hwdom_iort->node_offset = pos;
> +    hwdom_iort->node_count = 0;
> +
> +    /* Write its group nodes */
> +    write_its_group((u8*)hwdom_iort, &pos, &num_nodes);
> +    hwdom_iort->node_count = num_nodes;
> +    /* Write pcirc_nodes*/
> +    write_pcirc_nodes((u8*)hwdom_iort, &pos, &num_nodes);

I am pretty sure we spoke about it before. You don't handle named 
components. What is the plan for that?


> +    /* Update IORT Size in IORT header */
> +    hwdom_iort->node_count += num_nodes;
> +    hwdom_iort->header.length = pos;
> +    hwdom_iort->header.checksum = 0; /* TODO */
> +
> +    *iort_size = hwdom_iort->header.length;
> +
> +    return 0;
> +}
> +
>   /*
>    * Size of hardware domains iort is calulcated based on the number of
>    * mappings in the requesterId - deviceId mapping list.
> @@ -49,7 +300,7 @@ int estimate_iort_size(size_t *iort_size)
>       {
>           int i = 0;
>   
> -        for (i=0; i <= pcirc_count; i++)
> +        for ( i=0; i <= pcirc_count; i++ )

This belong to the patch where you add this line.

>           {
>               if ( pcirc_array[i] == (uint64_t)rmap->pcirc_node )
>                   break;
> diff --git a/xen/include/acpi/gen-iort.h b/xen/include/acpi/gen-iort.h
> index 68e666fdce..4de31b7b9f 100644
> --- a/xen/include/acpi/gen-iort.h
> +++ b/xen/include/acpi/gen-iort.h
> @@ -2,5 +2,6 @@
>   #define _GEN_IORT_H
>   
>   int estimate_iort_size(size_t *iort_size);
> +int prepare_iort(struct acpi_table_iort *hwdom_iort, unsigned int *iort_size);
>   
>   #endif
> diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
> index c183b6bb6e..f8b5254621 100644
> --- a/xen/include/asm-arm/acpi.h
> +++ b/xen/include/asm-arm/acpi.h
> @@ -36,6 +36,7 @@ typedef enum {
>       TBL_FADT,
>       TBL_MADT,
>       TBL_STAO,
> +    TBL_IORT,
>       TBL_XSDT,
>       TBL_RSDP,
>       TBL_EFIT,
> 

Cheers,