| Message ID | 20241209185429.54054-29-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.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. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> Acked-by: Edward Cree <ecree.xilinx@gmail.com> > @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) > iounmap(efx->membase); > efx->membase = membase; > > - /* Set up the WC mapping if needed */ > - if (wc_mem_map_size) { > + if (!wc_mem_map_size) > + goto out; Maybe this label ought to be called something else; it's not really an 'early exit', it's a 'skip over mapping and linking PIO', which just _happens_ to be almost the last thing in the function. > @@ -24,9 +24,10 @@ int efx_cxl_init(struct efx_probe_data *probe_data) > DECLARE_BITMAP(expected, CXL_MAX_CAPS); > DECLARE_BITMAP(found, CXL_MAX_CAPS); > struct pci_dev *pci_dev; > + resource_size_t max; > struct efx_cxl *cxl; > struct resource res; > - resource_size_t max; Why does 'max' have to move? Weird churn.
On 12/11/24 02:39, Edward Cree wrote: > On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.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. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Acked-by: Edward Cree <ecree.xilinx@gmail.com> > >> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) >> iounmap(efx->membase); >> efx->membase = membase; >> >> - /* Set up the WC mapping if needed */ >> - if (wc_mem_map_size) { >> + if (!wc_mem_map_size) >> + goto out; > Maybe this label ought to be called something else; it's not really an > 'early exit', it's a 'skip over mapping and linking PIO', which just > _happens_ to be almost the last thing in the function. I do not know if I follow your point here. This was added following Martin's previous review for keeping the debugging when PIO is not needed. It is formally skipping now because the change what I think is good for keeping indentation simpler with the additional conditional added. Anyway, I can change the label to something like "out_through_debug "which adds, IMO, unnecessary name complexity. Just using "debug" could give the wrong idea ... Any naming suggestion? >> @@ -24,9 +24,10 @@ int efx_cxl_init(struct efx_probe_data *probe_data) >> DECLARE_BITMAP(expected, CXL_MAX_CAPS); >> DECLARE_BITMAP(found, CXL_MAX_CAPS); >> struct pci_dev *pci_dev; >> + resource_size_t max; >> struct efx_cxl *cxl; >> struct resource res; >> - resource_size_t max; > Why does 'max' have to move? Weird churn. A previous version (not sure if an official one) had two resource_size_t variables defined, and I moved it there for preserving the reverse christmas tree, and when removed one it did not look bad. I will remove the move. Thanks!
On 11/12/2024 09:38, Alejandro Lucero Palau wrote: > On 12/11/24 02:39, Edward Cree wrote: >> On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: >>> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) >>> iounmap(efx->membase); >>> efx->membase = membase; >>> - /* Set up the WC mapping if needed */ >>> - if (wc_mem_map_size) { >>> + if (!wc_mem_map_size) >>> + goto out; >> Maybe this label ought to be called something else; it's not really an >> 'early exit', it's a 'skip over mapping and linking PIO', which just >> _happens_ to be almost the last thing in the function. > > > I do not know if I follow your point here. This was added following Martin's previous review for keeping the debugging when PIO is not needed. > > It is formally skipping now because the change what I think is good for keeping indentation simpler with the additional conditional added. Yeah, I agree that additional indentation is undesirable here. (Although that does suggest that the *ideal* approach would be some refactoring into smaller functions, but I'm not going to ask you to take on that extra work just to get your change in.) > Anyway, I can change the label to something like "out_through_debug "which adds, IMO, unnecessary name complexity. Just using "debug" could give the wrong idea ... > > Any naming suggestion? I was thinking something like "skip_pio:" or "no_piobufs:". That way if someone later adds another bit of code to this function (to do something not PIO-related) it'll be obvious it should go after the label (& hence not be skipped), whereas with an "out:" label normally that means "something went wrong, we're exiting early" and thus additional functionality gets added before it.
On 12/11/24 10:11, Edward Cree wrote: > On 11/12/2024 09:38, Alejandro Lucero Palau wrote: >> On 12/11/24 02:39, Edward Cree wrote: >>> On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: >>>> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) >>>> iounmap(efx->membase); >>>> efx->membase = membase; >>>> - /* Set up the WC mapping if needed */ >>>> - if (wc_mem_map_size) { >>>> + if (!wc_mem_map_size) >>>> + goto out; >>> Maybe this label ought to be called something else; it's not really an >>> 'early exit', it's a 'skip over mapping and linking PIO', which just >>> _happens_ to be almost the last thing in the function. >> >> I do not know if I follow your point here. This was added following Martin's previous review for keeping the debugging when PIO is not needed. >> >> It is formally skipping now because the change what I think is good for keeping indentation simpler with the additional conditional added. > Yeah, I agree that additional indentation is undesirable here. > (Although that does suggest that the *ideal* approach would be > some refactoring into smaller functions, but I'm not going to > ask you to take on that extra work just to get your change in.) I appreciate it. :-) >> Anyway, I can change the label to something like "out_through_debug "which adds, IMO, unnecessary name complexity. Just using "debug" could give the wrong idea ... >> >> Any naming suggestion? > I was thinking something like "skip_pio:" or "no_piobufs:". > That way if someone later adds another bit of code to this function > (to do something not PIO-related) it'll be obvious it should go > after the label (& hence not be skipped), whereas with an "out:" > label normally that means "something went wrong, we're exiting > early" and thus additional functionality gets added before it. I like skip_pio. I'll use it. Thanks!
On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.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. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/net/ethernet/sfc/ef10.c | 48 +++++++++++++++++++++++---- > drivers/net/ethernet/sfc/efx_cxl.c | 19 ++++++++++- > drivers/net/ethernet/sfc/net_driver.h | 2 ++ > drivers/net/ethernet/sfc/nic.h | 3 ++ > 4 files changed, 65 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c > index 452009ed7a43..4587ca884c03 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); > } Hi Alejandro, Earlier in efx_ef10_init_datapath_caps, outbuf is declared using: MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); This will result in the following declaration: efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)] Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78. So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements. > > + 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; > } > MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148. And the above will result in an access to element 148 / 4 == 37 of outbuf. A buffer overflow. Flagged by gcc-14 W=1 allmodconfig builds as: In file included from drivers/net/ethernet/sfc/net_driver.h:33, from drivers/net/ethernet/sfc/ef10.c:7: drivers/net/ethernet/sfc/ef10.c: In function 'efx_ef10_init_datapath_caps': drivers/net/ethernet/sfc/bitfield.h:167:35: warning: array subscript 37 is above array bounds of 'efx_dword_t[20]' {aka 'union efx_dword[20]'} [-Warray-bounds=] 167 | (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) & \ drivers/net/ethernet/sfc/bitfield.h:129:11: note: in definition of macro 'EFX_EXTRACT_NATIVE' 129 | (native_element) << ((min) - (low))) | ^~~~~~~~~~~~~~ ./include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__le32_to_cpu' 89 | #define le32_to_cpu __le32_to_cpu | ^~~~~~~~~~~~~ drivers/net/ethernet/sfc/bitfield.h:167:10: note: in expansion of macro 'EFX_EXTRACT32' 167 | (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) & \ | ^~~~~~~~~~~~~ drivers/net/ethernet/sfc/bitfield.h:187:9: note: in expansion of macro 'EFX_EXTRACT_DWORD' 187 | EFX_EXTRACT_DWORD(dword, EFX_LOW_BIT(field), \ | ^~~~~~~~~~~~~~~~~ drivers/net/ethernet/sfc/mcdi.h:257:9: note: in expansion of macro 'EFX_DWORD_FIELD' 257 | EFX_DWORD_FIELD(*_MCDI_DWORD(_buf, _field), EFX_DWORD_0) | ^~~~~~~~~~~~~~~ drivers/net/ethernet/sfc/ef10.c:184:44: note: in expansion of macro 'MCDI_DWORD' 184 | nic_data->datapath_caps3 = MCDI_DWORD(outbuf, | ^~~~~~~~~~ In file included from drivers/net/ethernet/sfc/ef10.c:12: drivers/net/ethernet/sfc/ef10.c:110:26: note: while referencing 'outbuf' 110 | MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); | ^~~~~~ drivers/net/ethernet/sfc/mcdi.h:187:21: note: in definition of macro '_MCDI_DECLARE_BUF' 187 | efx_dword_t _name[DIV_ROUND_UP(_len, 4)] | ^~~~~ drivers/net/ethernet/sfc/ef10.c:110:9: note: in expansion of macro 'MCDI_DECLARE_BUF' 110 | MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); | ^~~~~~~~~~~~~~~~ ...
On 12/12/24 21:22, Simon Horman wrote: > On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.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. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/net/ethernet/sfc/ef10.c | 48 +++++++++++++++++++++++---- >> drivers/net/ethernet/sfc/efx_cxl.c | 19 ++++++++++- >> drivers/net/ethernet/sfc/net_driver.h | 2 ++ >> drivers/net/ethernet/sfc/nic.h | 3 ++ >> 4 files changed, 65 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c >> index 452009ed7a43..4587ca884c03 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); >> } > Hi Alejandro, > > Earlier in efx_ef10_init_datapath_caps, outbuf is declared using: > > MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); > > This will result in the following declaration: > > efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)] > > Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78. > So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements. > >> >> + 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; >> } >> > MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148. > And the above will result in an access to element 148 / 4 == 37 of > outbuf. A buffer overflow. Hi Simon, This is, obviously, quite serious, although being the first and only flag in that MCDI extension explains why has gone hidden and harmless (as it is a read). I'll definitely fix it. Thanks! > > Flagged by gcc-14 W=1 allmodconfig builds as: > > In file included from drivers/net/ethernet/sfc/net_driver.h:33, > from drivers/net/ethernet/sfc/ef10.c:7: > drivers/net/ethernet/sfc/ef10.c: In function 'efx_ef10_init_datapath_caps': > drivers/net/ethernet/sfc/bitfield.h:167:35: warning: array subscript 37 is above array bounds of 'efx_dword_t[20]' {aka 'union efx_dword[20]'} [-Warray-bounds=] > 167 | (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) & \ > drivers/net/ethernet/sfc/bitfield.h:129:11: note: in definition of macro 'EFX_EXTRACT_NATIVE' > 129 | (native_element) << ((min) - (low))) > | ^~~~~~~~~~~~~~ > ./include/linux/byteorder/generic.h:89:21: note: in expansion of macro '__le32_to_cpu' > 89 | #define le32_to_cpu __le32_to_cpu > | ^~~~~~~~~~~~~ > drivers/net/ethernet/sfc/bitfield.h:167:10: note: in expansion of macro 'EFX_EXTRACT32' > 167 | (EFX_EXTRACT32((dword).u32[0], 0, 31, low, high) & \ > | ^~~~~~~~~~~~~ > drivers/net/ethernet/sfc/bitfield.h:187:9: note: in expansion of macro 'EFX_EXTRACT_DWORD' > 187 | EFX_EXTRACT_DWORD(dword, EFX_LOW_BIT(field), \ > | ^~~~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/mcdi.h:257:9: note: in expansion of macro 'EFX_DWORD_FIELD' > 257 | EFX_DWORD_FIELD(*_MCDI_DWORD(_buf, _field), EFX_DWORD_0) > | ^~~~~~~~~~~~~~~ > drivers/net/ethernet/sfc/ef10.c:184:44: note: in expansion of macro 'MCDI_DWORD' > 184 | nic_data->datapath_caps3 = MCDI_DWORD(outbuf, > | ^~~~~~~~~~ > In file included from drivers/net/ethernet/sfc/ef10.c:12: > drivers/net/ethernet/sfc/ef10.c:110:26: note: while referencing 'outbuf' > 110 | MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); > | ^~~~~~ > drivers/net/ethernet/sfc/mcdi.h:187:21: note: in definition of macro '_MCDI_DECLARE_BUF' > 187 | efx_dword_t _name[DIV_ROUND_UP(_len, 4)] > | ^~~~~ > drivers/net/ethernet/sfc/ef10.c:110:9: note: in expansion of macro 'MCDI_DECLARE_BUF' > 110 | MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); > | ^~~~~~~~~~~~~~~~ > > ... >
On Fri, Dec 13, 2024 at 10:20:30AM +0000, Alejandro Lucero Palau wrote: > > On 12/12/24 21:22, Simon Horman wrote: > > On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote: > > > From: Alejandro Lucero <alucerop@amd.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. > > > > > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > > > --- > > > drivers/net/ethernet/sfc/ef10.c | 48 +++++++++++++++++++++++---- > > > drivers/net/ethernet/sfc/efx_cxl.c | 19 ++++++++++- > > > drivers/net/ethernet/sfc/net_driver.h | 2 ++ > > > drivers/net/ethernet/sfc/nic.h | 3 ++ > > > 4 files changed, 65 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c > > > index 452009ed7a43..4587ca884c03 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); > > > } > > Hi Alejandro, > > > > Earlier in efx_ef10_init_datapath_caps, outbuf is declared using: > > > > MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); > > > > This will result in the following declaration: > > > > efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)] > > > > Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78. > > So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements. > > > > > + 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; > > > } > > MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148. > > And the above will result in an access to element 148 / 4 == 37 of > > outbuf. A buffer overflow. > > > Hi Simon, > > > This is, obviously, quite serious, although being the first and only flag in > that MCDI extension explains why has gone hidden and harmless (as it is a > read). > > > I'll definitely fix it. > > > Thanks! Likewise, thanks. Please to look at my analysis with a sceptical eye. It is my understanding based on looking at the code in the context of the compiler warnings.
On 12/13/24 10:24, Simon Horman wrote: > On Fri, Dec 13, 2024 at 10:20:30AM +0000, Alejandro Lucero Palau wrote: >> On 12/12/24 21:22, Simon Horman wrote: >>> On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote: >>>> From: Alejandro Lucero <alucerop@amd.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. >>>> >>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >>>> --- >>>> drivers/net/ethernet/sfc/ef10.c | 48 +++++++++++++++++++++++---- >>>> drivers/net/ethernet/sfc/efx_cxl.c | 19 ++++++++++- >>>> drivers/net/ethernet/sfc/net_driver.h | 2 ++ >>>> drivers/net/ethernet/sfc/nic.h | 3 ++ >>>> 4 files changed, 65 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c >>>> index 452009ed7a43..4587ca884c03 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); >>>> } >>> Hi Alejandro, >>> >>> Earlier in efx_ef10_init_datapath_caps, outbuf is declared using: >>> >>> MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); >>> >>> This will result in the following declaration: >>> >>> efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)] >>> >>> Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78. >>> So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements. >>> >>>> + 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; >>>> } >>> MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148. >>> And the above will result in an access to element 148 / 4 == 37 of >>> outbuf. A buffer overflow. >> >> Hi Simon, >> >> >> This is, obviously, quite serious, although being the first and only flag in >> that MCDI extension explains why has gone hidden and harmless (as it is a >> read). >> >> >> I'll definitely fix it. >> >> >> Thanks! > Likewise, thanks. > > Please to look at my analysis with a sceptical eye. > It is my understanding based on looking at the code in > the context of the compiler warnings. > Yes, I need to confirm this, but it looks a problem. BTW, I can not get the same warning/error with gcc 11.4. Just for being sure, are you just compiling with make W=1 or applying some other gcc param or kernel config option?
On Fri, Dec 13, 2024 at 11:45:41AM +0000, Alejandro Lucero Palau wrote: > > On 12/13/24 10:24, Simon Horman wrote: > > On Fri, Dec 13, 2024 at 10:20:30AM +0000, Alejandro Lucero Palau wrote: > > > On 12/12/24 21:22, Simon Horman wrote: > > > > On Mon, Dec 09, 2024 at 06:54:29PM +0000, alejandro.lucero-palau@amd.com wrote: > > > > > From: Alejandro Lucero <alucerop@amd.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. > > > > > > > > > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > > > > > --- > > > > > drivers/net/ethernet/sfc/ef10.c | 48 +++++++++++++++++++++++---- > > > > > drivers/net/ethernet/sfc/efx_cxl.c | 19 ++++++++++- > > > > > drivers/net/ethernet/sfc/net_driver.h | 2 ++ > > > > > drivers/net/ethernet/sfc/nic.h | 3 ++ > > > > > 4 files changed, 65 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c > > > > > index 452009ed7a43..4587ca884c03 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); > > > > > } > > > > Hi Alejandro, > > > > > > > > Earlier in efx_ef10_init_datapath_caps, outbuf is declared using: > > > > > > > > MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CAPABILITIES_V4_OUT_LEN); > > > > > > > > This will result in the following declaration: > > > > > > > > efx_dword_t _name[DIV_ROUND_UP(MC_CMD_GET_CAPABILITIES_V4_OUT_LEN, 4)] > > > > > > > > Where MC_CMD_GET_CAPABILITIES_V4_OUT_LEN is defined as 78. > > > > So outbuf will be an array with DIV_ROUND_UP(78, 4) == 20 elements. > > > > > > > > > + 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; > > > > > } > > > > MC_CMD_GET_CAPABILITIES_V7_OUT_FLAGS3_OFST is defined as 148. > > > > And the above will result in an access to element 148 / 4 == 37 of > > > > outbuf. A buffer overflow. > > > > > > Hi Simon, > > > > > > > > > This is, obviously, quite serious, although being the first and only flag in > > > that MCDI extension explains why has gone hidden and harmless (as it is a > > > read). > > > > > > > > > I'll definitely fix it. > > > > > > > > > Thanks! > > Likewise, thanks. > > > > Please to look at my analysis with a sceptical eye. > > It is my understanding based on looking at the code in > > the context of the compiler warnings. > > > > Yes, I need to confirm this, but it looks a problem. > > > BTW, I can not get the same warning/error with gcc 11.4. Just for being > sure, are you just compiling with make W=1 or applying some other gcc param > or kernel config option? Hi Alejandro, Sorry about the incomplete information. I checked and with gcc 14.2 -Warray-bounds is needed to for it to flag this problem. make EXTRA_CFLAGS="-Warray-bounds" ...
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 452009ed7a43..4587ca884c03 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; } @@ -919,6 +926,9 @@ static void efx_ef10_forget_old_piobufs(struct efx_nic *efx) static void efx_ef10_remove(struct efx_nic *efx) { struct efx_ef10_nic_data *nic_data = efx->nic_data; +#ifdef CONFIG_SFC_CXL + struct efx_probe_data *probe_data; +#endif int rc; #ifdef CONFIG_SFC_SRIOV @@ -949,7 +959,12 @@ static void efx_ef10_remove(struct efx_nic *efx) efx_mcdi_rx_free_indir_table(efx); +#ifdef CONFIG_SFC_CXL + probe_data = container_of(efx, struct efx_probe_data, efx); + if (nic_data->wc_membase && !probe_data->cxl_pio_in_use) +#else if (nic_data->wc_membase) +#endif iounmap(nic_data->wc_membase); rc = efx_mcdi_free_vis(efx); @@ -1140,6 +1155,9 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) unsigned int channel_vis, pio_write_vi_base, max_vis; struct efx_ef10_nic_data *nic_data = efx->nic_data; unsigned int uc_mem_map_size, wc_mem_map_size; +#ifdef CONFIG_SFC_CXL + struct efx_probe_data *probe_data; +#endif void __iomem *membase; int rc; @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) iounmap(efx->membase); efx->membase = membase; - /* Set up the WC mapping if needed */ - if (wc_mem_map_size) { + if (!wc_mem_map_size) + goto out; + + /* Set up the WC mapping */ + +#ifdef CONFIG_SFC_CXL + probe_data = container_of(efx, struct efx_probe_data, efx); + if ((nic_data->datapath_caps3 & + (1 << MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN)) && + probe_data->cxl_pio_initialised) { + /* Using PIO through CXL mapping? */ + nic_data->pio_write_base = probe_data->cxl->ctpio_cxl + + (pio_write_vi_base * efx->vi_stride + + ER_DZ_TX_PIOBUF - uc_mem_map_size); + probe_data->cxl_pio_in_use = true; + } else +#endif + { + /* Using legacy PIO BAR mapping */ nic_data->wc_membase = ioremap_wc(efx->membase_phys + uc_mem_map_size, wc_mem_map_size); @@ -1279,12 +1314,13 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) nic_data->wc_membase + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - uc_mem_map_size); - - rc = efx_ef10_link_piobufs(efx); - if (rc) - efx_ef10_free_piobufs(efx); } + rc = efx_ef10_link_piobufs(efx); + if (rc) + efx_ef10_free_piobufs(efx); + +out: netif_dbg(efx, probe, efx->net_dev, "memory BAR at %pa (virtual %p+%x UC, %p+%x WC)\n", &efx->membase_phys, efx->membase, uc_mem_map_size, diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index e7c121368b0a..2de0edda873c 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -24,9 +24,10 @@ int efx_cxl_init(struct efx_probe_data *probe_data) DECLARE_BITMAP(expected, CXL_MAX_CAPS); DECLARE_BITMAP(found, CXL_MAX_CAPS); struct pci_dev *pci_dev; + resource_size_t max; struct efx_cxl *cxl; struct resource res; - resource_size_t max; + struct range range; u16 dvsec; int rc; @@ -135,10 +136,25 @@ int efx_cxl_init(struct efx_probe_data *probe_data) goto err_region; } + rc = cxl_get_region_range(cxl->efx_region, &range); + if (rc) { + pci_err(pci_dev, "CXL getting regions params failed"); + goto err_region_params; + } + + cxl->ctpio_cxl = ioremap(range.start, range.end - range.start); + if (!cxl->ctpio_cxl) { + pci_err(pci_dev, "CXL ioremap region (%pra) pfailed", &range); + goto err_region_params; + } + probe_data->cxl = cxl; + probe_data->cxl_pio_initialised = true; return 0; +err_region_params: + cxl_accel_region_detach(cxl->cxled); err_region: cxl_dpa_free(cxl->cxled); err3: @@ -153,6 +169,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data) void efx_cxl_exit(struct efx_probe_data *probe_data) { if (probe_data->cxl) { + iounmap(probe_data->cxl->ctpio_cxl); cxl_accel_region_detach(probe_data->cxl->cxled); cxl_dpa_free(probe_data->cxl->cxled); cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h index 7f11ff200c25..90b884039058 100644 --- a/drivers/net/ethernet/sfc/net_driver.h +++ b/drivers/net/ethernet/sfc/net_driver.h @@ -1209,6 +1209,7 @@ struct efx_cxl; * @efx: Efx NIC details * @cxl: details of related cxl objects * @cxl_pio_initialised: cxl initialization outcome. + * @cxl_pio_in_use: PIO using CXL mapping */ struct efx_probe_data { struct pci_dev *pci_dev; @@ -1216,6 +1217,7 @@ struct efx_probe_data { #ifdef CONFIG_SFC_CXL struct efx_cxl *cxl; bool cxl_pio_initialised; + bool cxl_pio_in_use; #endif }; diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h index 9fa5c4c713ab..c87cc9214690 100644 --- a/drivers/net/ethernet/sfc/nic.h +++ b/drivers/net/ethernet/sfc/nic.h @@ -152,6 +152,8 @@ enum { * %MC_CMD_GET_CAPABILITIES response) * @datapath_caps2: Further Capabilities of datapath firmware (FLAGS2 field of * %MC_CMD_GET_CAPABILITIES response) + * @datapath_caps3: Further Capabilities of datapath firmware (FLAGS3 field of + * %MC_CMD_GET_CAPABILITIES response) * @rx_dpcpu_fw_id: Firmware ID of the RxDPCPU * @tx_dpcpu_fw_id: Firmware ID of the TxDPCPU * @must_probe_vswitching: Flag: vswitching has yet to be setup after MC reboot @@ -186,6 +188,7 @@ struct efx_ef10_nic_data { bool must_check_datapath_caps; u32 datapath_caps; u32 datapath_caps2; + u32 datapath_caps3; unsigned int rx_dpcpu_fw_id; unsigned int tx_dpcpu_fw_id; bool must_probe_vswitching;