Re: [PATCH v7 1/2] ice: reduce time to read Option ROM CIVD data

* Re: [PATCH v7 1/2] ice: reduce time to read Option ROM CIVD data
       [not found] <20211027232255.669167-1-jacob.e.keller@intel.com>
@ 2021-10-28  6:43 ` Paul Menzel
  2021-10-28 17:56   ` Keller, Jacob E
  0 siblings, 1 reply; 2+ messages in thread
From: Paul Menzel @ 2021-10-28  6:43 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Anthony Nguyen, intel-wired-lan, linux-pci

[Cc: +linux-pci for ideas how to work with Option ROMs]

Dear Jacob,

On 28.10.21 01:22, Jacob Keller wrote:
> During probe and device reset, the ice driver reads some data from the
> NVM image as part of ice_init_nvm. Part of this data includes a section
> of the Option ROM which contains version information.
> 
> The function ice_get_orom_civd_data is used to locate the '$CIV' data
> section of the Option ROM.
> 
> Timing of ice_probe and ice_rebuild indicate that the
> ice_get_orom_civd_data function takes about 10 seconds to finish
> executing.
> 
> The function locates the section by scanning the Option ROM every 512
> bytes. This requires a significant number of NVM read accesses, since
> the Option ROM bank is 500KB. In the worst case it would take about 1000
> reads. Worse, all PFs serialize this operation during reload because of
> acquiring the NVM semaphore.
> 
> The CIVD section is located at the end of the Option ROM image data.
> Unfortunately, the driver has no easy method to determine the offset
> manually. Practical experiments have shown that the data could be at
> a variety of locations, so simply reversing the scanning order is not
> sufficient to reduce the overall read time.
> 
> Instead, copy the entire contents of the Option ROM into memory. This
> allows reading the data using 4Kb pages instead of 512 bytes at a time.
> This reduces the total number of firmware commands by a factor of 8. In
> addition, reading the whole section together at once allows better
> indication to firmware of when we're "done".
> 
> Re-write ice_get_orom_civd_data to allocate virtual memory to store the
> Option ROM data. Copy the entire OptionROM contents at once using

Option ROM

> ice_read_flash_module. Finally, use this memory copy to scan for the
> '$CIV' section.
> 
> This change significantly reduces the time to read the Option ROM CIVD
> section from ~10 seconds down to ~1 second. This has a significant
> impact on the total time to complete a driver rebuild or probe.

Thank you for taking up the challenge and looking into this, and for 
decreasing the time.

OT: Out of curiosity, how does it work on UEFI systems without using 
Compatibility System Mode (CSM) and just “EFI drivers”?

> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
> Changes since v6
> * fix a memory leak
> 
> Changes since v5
> * new patch
> 
>   drivers/net/ethernet/intel/ice/ice_nvm.c | 48 ++++++++++++++++++------
>   1 file changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 941bfce97bf4..c5a39aa8ac94 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -619,7 +619,7 @@ static int
>   ice_get_orom_civd_data(struct ice_hw *hw, enum ice_bank_select bank,
>   		       struct ice_orom_civd_info *civd)
>   {
> -	struct ice_orom_civd_info tmp;
> +	u8 *orom_data;

I know the function names use orom, but oprom is already used in other 
subsystems.

>   	int status;
>   	u32 offset;
>   
> @@ -627,36 +627,60 @@ ice_get_orom_civd_data(struct ice_hw *hw, enum ice_bank_select bank,
>   	 * The first 4 bytes must contain the ASCII characters "$CIV".
>   	 * A simple modulo 256 sum of all of the bytes of the structure must
>   	 * equal 0.
> +	 *
> +	 * The exact location is unknown and varies between images but is
> +	 * usually somewhere in the middle of the bank. We need to scan the
> +	 * Option ROM bank to locate it.
> +	 *
> +	 * It's significantly faster to read the entire Option ROM up front
> +	 * using the maximum page size, than to read each possible location
> +	 * with a separate firmware command.
>   	 */
> +	orom_data = vzalloc(hw->flash.banks.orom_size);
> +	if (!orom_data)
> +		return -ENOMEM;
> +
> +	status = ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR, 0,
> +				       orom_data, hw->flash.banks.orom_size);
> +	if (status) {
> +		ice_debug(hw, ICE_DBG_NVM, "Unable to read Option ROM data\n");
> +		return status;
> +	}
> +
> +	/* Scan the memory buffer to locate the CIVD data section */
>   	for (offset = 0; (offset + 512) <= hw->flash.banks.orom_size; offset += 512) {
> +		struct ice_orom_civd_info *tmp;
>   		u8 sum = 0, i;
>   
> -		status = ice_read_flash_module(hw, bank, ICE_SR_1ST_OROM_BANK_PTR,
> -					       offset, (u8 *)&tmp, sizeof(tmp));
> -		if (status) {
> -			ice_debug(hw, ICE_DBG_NVM, "Unable to read Option ROM CIVD data\n");
> -			return status;
> -		}
> +		tmp = (struct ice_orom_civd_info *)&orom_data[offset];
>   
>   		/* Skip forward until we find a matching signature */
> -		if (memcmp("$CIV", tmp.signature, sizeof(tmp.signature)) != 0)
> +		if (memcmp("$CIV", tmp->signature, sizeof(tmp->signature)) != 0)
>   			continue;
>   
> +		ice_debug(hw, ICE_DBG_NVM, "Found CIVD section at offset %u\n",
> +			  offset);
> +
>   		/* Verify that the simple checksum is zero */
> -		for (i = 0; i < sizeof(tmp); i++)
> +		for (i = 0; i < sizeof(*tmp); i++)
>   			/* cppcheck-suppress objectIndex */
> -			sum += ((u8 *)&tmp)[i];
> +			sum += ((u8 *)tmp)[i];
>   
>   		if (sum) {
>   			ice_debug(hw, ICE_DBG_NVM, "Found CIVD data with invalid checksum of %u\n",
>   				  sum);
> -			return -EIO;
> +			goto err_invalid_checksum;
>   		}
>   
> -		*civd = tmp;
> +		*civd = *tmp;
> +		vfree(orom_data);
>   		return 0;
>   	}
>   
> +	ice_debug(hw, ICE_DBG_NVM, "Unable to locate CIVD data within the Option ROM\n");
> +
> +err_invalid_checksum:
> +	vfree(orom_data);
>   	return -EIO;
>   }

Please excuse my ignorance. I would have though, that the system 
firmware already put that Option ROM into memory. There is a function 
`pci_map_biosrom()` declared in `arch/x86/include/asm/probe_roms.h` and 
implemented in `arch/x86/kernel/probe_roms.c`, which might be used?

Kind regards,

Paul

^ permalink raw reply	[flat|nested] 2+ messages in thread