netdev - Re: [PATCH v2 15/15] efx: support pio mapping based on cxl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e17a0f9-ef84-5ce1-3574-5d609525b7f1@amd.com>
Date: Mon, 19 Aug 2024 17:28:46 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, richard.hughes@....com
Subject: Re: [PATCH v2 15/15] efx: support pio mapping based on cxl


On 8/4/24 19:13, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:35 +0100
> alejandro.lucero-palau@....com wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> With a device supporting CXL and successfully initialised, use the cxl
>> region to map the memory range and use this mapping for PIO buffers.
> This explains why you weren't worried about any step of the CXL
> code failing and why that wasn't a 'bug' as such.
>
> I'd argue that you should still have the cxl intialization return
> an error code and cleanup any state it if hits an error.


Ideally, but with devm* being used, this is not easy to do if the error 
is not fatal.


> Then the top level driver can of course elect to use an alternative
> path given that failure.  Logically it belongs there rather than relying
> on a buffer being mapped or not.
>

Same driver needs to support same functionality which relies on those 
specific hardware buffers.

The functionality is expected to be there with or without CXL. If the 
hardware has no CXL, the system or the device, the functionality will be 
there with legacy PCIe BAR regions. The green light for CXL use comes 
from two sources: the firmware and the kernel. Both need to give the 
thumbs up. If not, legacy PCIe BAR regions will be used.


>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> ---
>>   drivers/net/ethernet/sfc/ef10.c      | 25 +++++++++++++++++++++----
>>   drivers/net/ethernet/sfc/efx_cxl.c   | 12 +++++++++++-
>>   drivers/net/ethernet/sfc/mcdi_pcol.h |  3 +++
>>   drivers/net/ethernet/sfc/nic.h       |  1 +
>>   4 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index 8fa6c0e9195b..3924076d2628 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/wait.h>
>>   #include <linux/workqueue.h>
>>   #include <net/udp_tunnel.h>
>> +#include "efx_cxl.h"
>>   
>>   /* Hardware control for EF10 architecture including 'Huntington'. */
>>   
>> @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx)
>>   			  efx->num_mac_stats);
>>   	}
>>   
>> +	if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN)
>> +		nic_data->datapath_caps3 = 0;
>> +	else
>> +		nic_data->datapath_caps3 = MCDI_DWORD(outbuf,
>> +						      GET_CAPABILITIES_V7_OUT_FLAGS3);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   			return -ENOMEM;
>>   		}
>>   		nic_data->pio_write_vi_base = pio_write_vi_base;
>> -		nic_data->pio_write_base =
>> -			nic_data->wc_membase +
>> -			(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> -			 uc_mem_map_size);
>> +
>> +		if ((nic_data->datapath_caps3 &
>> +		    (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) &&
>> +		    efx->cxl->ctpio_cxl)
> As per comment at the top, I'd prefer to see some clean handling of the an
> error passed up to the caller of the cxl init that then sets a flag that
> we can clearly see is all about whether we have CXL or not.
>
> Using this buffer mapping is a it too much of a detail in my opinion.


Yes, maybe that is clearer than relying on the pointer from the CXL 
mapping.

I will do it.

Thanks!


>> +		{
>> +			nic_data->pio_write_base =
>> +				efx->cxl->ctpio_cxl +
>> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> +				 uc_mem_map_size);
>> +		} else {
>> +			nic_data->pio_write_base =nic_data->wc_membase +
>> +				(pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF -
>> +				 uc_mem_map_size);
>> +		}
>