[Skiboot] [RESEND PATCH v1 02/11]ibm-fsp/firenze: nest feature detection

[Madhavan Srinivasan]

On Thursday 16 July 2015 07:26 AM, Joel Stanley wrote:
> On Sun, 2015-07-05 at 22:25 +0530, Madhavan Srinivasan wrote:
>> diff --git a/hw/nest.c b/hw/nest.c
>> new file mode 100644
>> index 0000000..fd22a0e
>> --- /dev/null
>> +++ b/hw/nest.c
>> +struct ima_catalog_page_0 *page0_ptr;
>> +struct page0_offsets *pg0_offsets;
>> +
>> +int load_catalogue_lid()
>> +{
>> +	struct proc_chip *chip = get_chip(pir_to_chip_id(this_cpu()->pir));
> I think get_chip(this_cpu()->chip_id) will get you the same thing with
> one less function call?

Correct. Will replace it.

>> +	size_t size = NEST_CATALOGUE_SIZE;
>> +	int rc=0, loaded;
>> +
>> +	pg0_offsets = (struct page0_offsets *)malloc(
>> +					sizeof(struct page0_offsets));
> You don't need to cast the void * that malloc returns (same goes for all
> the other times you call malloc in these patches).

ok. Will watch for any warning.

>> +	if (!pg0_offsets) {
>> +		prerror("Nest_IMA: No mem for pg0_offsets structure\n");;
>> +		rc = OPAL_NO_MEM;
>> +		return rc;
> you could just return OPAL_NO_MEM?
>
>> +	}
>> +
>> +	pg0_offsets->page0 = (char *)malloc(NEST_CATALOGUE_SIZE);
>> +	if (!pg0_offsets->page0) {
>> +		prerror("Nest_IMA: No mem for catalogue lid \n");
>> +		rc = OPAL_NO_MEM;
>> +		return rc;
> Here too.
>
>> +	}
>> +
>> +	loaded = start_preload_resource (RESOURCE_ID_CATALOGUE,
>> +						RESOURCE_SUBID_NONE,
>> +						pg0_offsets->page0, &size);
>> +	if (loaded == OPAL_SUCCESS)
>> +		loaded = wait_for_resource_loaded(RESOURCE_ID_CATALOGUE,
>> +							RESOURCE_SUBID_NONE);
>> +
>> +	if (loaded != OPAL_SUCCESS) {
>> +		prerror("Nest_IMA: Error loading catalogue lid. index=%x\n",
>> +							(int)chip->type);
> You shouldn't need to cast to int.
>
> Why are you printing the chip type here?

Nice catch. My bad. I added that for when debugging. Will remove it.

>
>> +		rc = OPAL_RESOURCE;
>> +		free(pg0_offsets->page0);
>> +		free(pg0_offsets);
>> +		return rc;
> return OPAL_RESOURCE.
>
>> +	}
>> +
>> +	/*
>> +	 * Now that we have loaded the catalogue. Check for Chip event support
>> +	 */
>> +	page0_ptr = (struct ima_catalog_page_0 *) PAGE0(pg0_offsets);
>> +	if (page0_ptr->chip_group_offset == 0) {
>> +		prerror("Nest_IMA: Not Supported \n");
>> +		return OPAL_UNSUPPORTED;
>> +	}
>> +
>> +	/*
>> +	 * Lets save some entry points to help out search
>> +	 */
>> +	GROUP_ENTRY(pg0_offsets) = PAGE0(pg0_offsets) +
>> +					(page0_ptr->group_data_offs * 4096);
>> +	EVENT_ENTRY(pg0_offsets) = PAGE0(pg0_offsets) +
>> +					(page0_ptr->event_data_offs * 4096);
>> +	CHIP_EVENT_ENTRY(pg0_offsets) = EVENT_ENTRY(pg0_offsets) +
>> +					(page0_ptr->chip_event_offset);
>> +	CHIP_GROUP_ENTRY(pg0_offsets) = GROUP_ENTRY(pg0_offsets) +
>> +					(page0_ptr->chip_group_offset);
>> +
>> +	return rc;
> This is the only time you return RC, and you haven't changed it from the
> initalised value. Make it return OPAL_SUCCESS. Now you can drop the 'rc'
> variable.

Yes. Correct. I will remove the rc.

>> +}
>> +
>> +/*
>> + * powerpc Nest instrumentation support
>> + */
>> +void nest_ima_init(void)
>> +{
>> +	if (load_catalogue_lid()) {
>> +		printf("IMA Catalog lid failed to load, Exiting \n");
>> +		return;
>> +	}
>> +
>> +	return;
>> +}