[PATCH v2 3/3] ACPI: MADT: add in compliance checks for the GIC ITS subtable

[Colin Ian King]

On 11/01/16 23:23, Al Stone wrote:
> Having previously added the proper structs for the GIC ITS subtable
> of the MADT, add in test to make sure the content is reasonably correct
> if one is being used.
> 
> Signed-off-by: Al Stone <al.stone at linaro.org>
> ---
>  src/acpi/compliance/madt.c | 83 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 81 insertions(+), 2 deletions(-)
> 
> diff --git a/src/acpi/compliance/madt.c b/src/acpi/compliance/madt.c
> index 78cf4ac..8ba16ed 100644
> --- a/src/acpi/compliance/madt.c
> +++ b/src/acpi/compliance/madt.c
> @@ -202,6 +202,7 @@ static fwts_acpi_table_info *mtable;
>  static fwts_acpi_table_info *ftable;
>  
>  static fwts_list msi_frame_ids;
> +static fwts_list its_ids;
>  
>  static int spec_madt_init(fwts_framework *fw)
>  {
> @@ -253,8 +254,12 @@ static int spec_madt_init(fwts_framework *fw)
>  		ms++;
>  	}
>  
> -	/* initialize the MSI frame ID list should we need it later */
> +	/*
> +	 * Initialize the MSI frame ID and ITS ID lists should we need
> +	 * them later
> +	 */
>  	fwts_list_init(&msi_frame_ids);
> +	fwts_list_init(&its_ids);
>  
>  	return (!spec_data) ? FWTS_ERROR : FWTS_OK;
>  }
> @@ -1090,6 +1095,75 @@ static int spec_madt_gicr(fwts_framework *fw,
>  	return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header));
>  }
>  
> +static int spec_madt_gic_its(fwts_framework *fw,
> +			     fwts_acpi_madt_sub_table_header *hdr,
> +			     const uint8_t *data)
> +{
> +	/* specific checks for subtable type 0xf: GIC ITS */
> +	fwts_acpi_madt_gic_its *gic_its = (fwts_acpi_madt_gic_its *)data;
> +	fwts_list_link *item;
> +	bool found;
> +
> +	if (gic_its->reserved)
> +		fwts_failed(fw, LOG_LEVEL_LOW,
> +			    "SPECMADTGICITSReservedNonZero",
> +			    "MADT %s first reserved field should be zero, "
> +			    "instead got 0x%" PRIx32 ".",
> +			    spec_madt_sub_names[hdr->type], gic_its->reserved);
> +	else
> +		fwts_passed(fw,
> +			    "MADT %s first reserved field is properly set "
> +			    "to zero.",
> +			    spec_madt_sub_names[hdr->type]);
> +
> +	/*
> +	 * Check ITS ID against previously found IDs to see if it
> +	 * is unique.  According to the spec, they must be.
> +	 */
> +	found = false;
> +	fwts_list_foreach(item, &its_ids) {
> +		uint32_t *its_id = fwts_list_data(uint32_t *, item);
> +
> +		if (*its_id == gic_its->its_id)
> +			found = true;
> +	}
> +	if (found) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM,
> +			    "SPECMADTGICITSNonUniqueId",
> +			    "MADT %s ITS ID 0x%" PRIx32 " is not unique "
> +			    "and has already be defined in a previous %s.",
> +			    spec_madt_sub_names[hdr->type],
> +			    gic_its->its_id,
> +			    spec_madt_sub_names[hdr->type]);
> +	} else {
> +		fwts_list_append(&its_ids, &(gic_its->its_id));
> +		fwts_passed(fw,
> +			    "MADT %s ITS ID 0x%" PRIx32 " is unique "
> +			    "as is required.",
> +			    spec_madt_sub_names[hdr->type],
> +			    gic_its->its_id);
> +	}
> +
> +	/*
> +	 * TODO: can the physical base address be tested, or is zero
> +	 * allowed?
> +	 */
> +
> +	if (gic_its->reserved2)
> +		fwts_failed(fw, LOG_LEVEL_LOW,
> +			    "SPECMADTGICITSReserved2NonZero",
> +			    "MADT %s second reserved field should be zero, "
> +			    "instead got 0x%" PRIx32 ".",
> +			    spec_madt_sub_names[hdr->type], gic_its->reserved2);
> +	else
> +		fwts_passed(fw,
> +			    "MADT %s second reserved field is properly set "
> +			    "to zero.",
> +			    spec_madt_sub_names[hdr->type]);
> +
> +	return (hdr->length - sizeof(fwts_acpi_madt_sub_table_header));
> +}
> +
>  static int spec_madt_subtables(fwts_framework *fw)
>  {
>  	fwts_acpi_table_madt *madt = (fwts_acpi_table_madt *)mtable->data;
> @@ -1238,6 +1312,10 @@ static int spec_madt_subtables(fwts_framework *fw)
>  			skip = spec_madt_gicr(fw, hdr, data);
>  			break;
>  
> +		case FWTS_ACPI_MADT_GIC_ITS:
> +			skip = spec_madt_gic_its(fw, hdr, data);
> +			break;
> +
>  		default:
>  			if (hdr->type >= 0x10 && hdr->type <= 0x7f)
>  				fwts_failed(fw, LOG_LEVEL_MEDIUM,
> @@ -1268,8 +1346,9 @@ static int spec_madt_subtables(fwts_framework *fw)
>  
>  static int spec_madt_deinit(fwts_framework *fw)
>  {
> -	/* only one minor clean up needed */
> +	/* only minor clean up needed */
>  	fwts_list_free_items(&msi_frame_ids, NULL);
> +	fwts_list_free_items(&its_ids, NULL);
>  
>  	return (fw) ? FWTS_ERROR : FWTS_OK;
>  }
> 

Let's review this on the next spin of this patch set if that's OK

Colin