2020-08-09 11:54:46

by Oded Gabbay

[permalink] [raw]
Subject: [PATCH 1/4] habanalabs: verify user input in cs_ioctl_signal_wait

From: Ofir Bitton <[email protected]>

User input must be validated before using it to
access internal structures.

Signed-off-by: Ofir Bitton <[email protected]>
Reviewed-by: Oded Gabbay <[email protected]>
Signed-off-by: Oded Gabbay <[email protected]>
---
drivers/misc/habanalabs/common/command_submission.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/drivers/misc/habanalabs/common/command_submission.c b/drivers/misc/habanalabs/common/command_submission.c
index b9840e368eb5..2e3fcbc794db 100644
--- a/drivers/misc/habanalabs/common/command_submission.c
+++ b/drivers/misc/habanalabs/common/command_submission.c
@@ -808,6 +808,14 @@ static int cs_ioctl_signal_wait(struct hl_fpriv *hpriv, enum hl_cs_type cs_type,

/* currently it is guaranteed to have only one chunk */
chunk = &cs_chunk_array[0];
+
+ if (chunk->queue_index >= hdev->asic_prop.max_queues) {
+ dev_err(hdev->dev, "Queue index %d is invalid\n",
+ chunk->queue_index);
+ rc = -EINVAL;
+ goto free_cs_chunk_array;
+ }
+
q_idx = chunk->queue_index;
hw_queue_prop = &hdev->asic_prop.hw_queues_props[q_idx];
q_type = hw_queue_prop->type;
--
2.17.1


2020-08-09 11:54:55

by Oded Gabbay

[permalink] [raw]
Subject: [PATCH 2/4] habanalabs: set clock gating according to mask

From: Ofir Bitton <[email protected]>

Once clock gating is set we enable clock gating according to mask,
we should also disable clock gating according to relevant bits.

Signed-off-by: Ofir Bitton <[email protected]>
Reviewed-by: Oded Gabbay <[email protected]>
Signed-off-by: Oded Gabbay <[email protected]>
---
drivers/misc/habanalabs/gaudi/gaudi.c | 44 +++++++++++++--------------
1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/misc/habanalabs/gaudi/gaudi.c b/drivers/misc/habanalabs/gaudi/gaudi.c
index 08124873483f..7e0f9f64ffcb 100644
--- a/drivers/misc/habanalabs/gaudi/gaudi.c
+++ b/drivers/misc/habanalabs/gaudi/gaudi.c
@@ -2508,6 +2508,7 @@ static void gaudi_set_clock_gating(struct hl_device *hdev)
{
struct gaudi_device *gaudi = hdev->asic_specific;
u32 qman_offset;
+ bool enable;
int i;

/* In case we are during debug session, don't enable the clock gate
@@ -2517,46 +2518,43 @@ static void gaudi_set_clock_gating(struct hl_device *hdev)
return;

for (i = GAUDI_PCI_DMA_1, qman_offset = 0 ; i < GAUDI_HBM_DMA_1 ; i++) {
- if (!(hdev->clock_gating_mask &
- (BIT_ULL(gaudi_dma_assignment[i]))))
- continue;
+ enable = !!(hdev->clock_gating_mask &
+ (BIT_ULL(gaudi_dma_assignment[i])));

qman_offset = gaudi_dma_assignment[i] * DMA_QMAN_OFFSET;
- WREG32(mmDMA0_QM_CGM_CFG1 + qman_offset, QMAN_CGM1_PWR_GATE_EN);
+ WREG32(mmDMA0_QM_CGM_CFG1 + qman_offset,
+ enable ? QMAN_CGM1_PWR_GATE_EN : 0);
WREG32(mmDMA0_QM_CGM_CFG + qman_offset,
- QMAN_UPPER_CP_CGM_PWR_GATE_EN);
+ enable ? QMAN_UPPER_CP_CGM_PWR_GATE_EN : 0);
}

for (i = GAUDI_HBM_DMA_1 ; i < GAUDI_DMA_MAX ; i++) {
- if (!(hdev->clock_gating_mask &
- (BIT_ULL(gaudi_dma_assignment[i]))))
- continue;
+ enable = !!(hdev->clock_gating_mask &
+ (BIT_ULL(gaudi_dma_assignment[i])));

qman_offset = gaudi_dma_assignment[i] * DMA_QMAN_OFFSET;
- WREG32(mmDMA0_QM_CGM_CFG1 + qman_offset, QMAN_CGM1_PWR_GATE_EN);
+ WREG32(mmDMA0_QM_CGM_CFG1 + qman_offset,
+ enable ? QMAN_CGM1_PWR_GATE_EN : 0);
WREG32(mmDMA0_QM_CGM_CFG + qman_offset,
- QMAN_COMMON_CP_CGM_PWR_GATE_EN);
+ enable ? QMAN_COMMON_CP_CGM_PWR_GATE_EN : 0);
}

- if (hdev->clock_gating_mask & (BIT_ULL(GAUDI_ENGINE_ID_MME_0))) {
- WREG32(mmMME0_QM_CGM_CFG1, QMAN_CGM1_PWR_GATE_EN);
- WREG32(mmMME0_QM_CGM_CFG, QMAN_COMMON_CP_CGM_PWR_GATE_EN);
- }
+ enable = !!(hdev->clock_gating_mask & (BIT_ULL(GAUDI_ENGINE_ID_MME_0)));
+ WREG32(mmMME0_QM_CGM_CFG1, enable ? QMAN_CGM1_PWR_GATE_EN : 0);
+ WREG32(mmMME0_QM_CGM_CFG, enable ? QMAN_COMMON_CP_CGM_PWR_GATE_EN : 0);

- if (hdev->clock_gating_mask & (BIT_ULL(GAUDI_ENGINE_ID_MME_2))) {
- WREG32(mmMME2_QM_CGM_CFG1, QMAN_CGM1_PWR_GATE_EN);
- WREG32(mmMME2_QM_CGM_CFG, QMAN_COMMON_CP_CGM_PWR_GATE_EN);
- }
+ enable = !!(hdev->clock_gating_mask & (BIT_ULL(GAUDI_ENGINE_ID_MME_2)));
+ WREG32(mmMME2_QM_CGM_CFG1, enable ? QMAN_CGM1_PWR_GATE_EN : 0);
+ WREG32(mmMME2_QM_CGM_CFG, enable ? QMAN_COMMON_CP_CGM_PWR_GATE_EN : 0);

for (i = 0, qman_offset = 0 ; i < TPC_NUMBER_OF_ENGINES ; i++) {
- if (!(hdev->clock_gating_mask &
- (BIT_ULL(GAUDI_ENGINE_ID_TPC_0 + i))))
- continue;
+ enable = !!(hdev->clock_gating_mask &
+ (BIT_ULL(GAUDI_ENGINE_ID_TPC_0 + i)));

WREG32(mmTPC0_QM_CGM_CFG1 + qman_offset,
- QMAN_CGM1_PWR_GATE_EN);
+ enable ? QMAN_CGM1_PWR_GATE_EN : 0);
WREG32(mmTPC0_QM_CGM_CFG + qman_offset,
- QMAN_COMMON_CP_CGM_PWR_GATE_EN);
+ enable ? QMAN_COMMON_CP_CGM_PWR_GATE_EN : 0);

qman_offset += TPC_QMAN_OFFSET;
}
--
2.17.1

2020-08-09 11:55:49

by Oded Gabbay

[permalink] [raw]
Subject: [PATCH 3/4] habanalabs: proper handling of alloc size in coresight

From: Ofir Bitton <[email protected]>

Allocation size can go up to 64bit but truncated to 32bit,
we should make sure it is not truncated and validate no address
overflow.

Signed-off-by: Ofir Bitton <[email protected]>
Reviewed-by: Oded Gabbay <[email protected]>
Signed-off-by: Oded Gabbay <[email protected]>
---
drivers/misc/habanalabs/common/habanalabs.h | 2 +-
drivers/misc/habanalabs/gaudi/gaudi_coresight.c | 8 +++++++-
drivers/misc/habanalabs/goya/goya_coresight.c | 8 +++++++-
3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/habanalabs/common/habanalabs.h b/drivers/misc/habanalabs/common/habanalabs.h
index 018d9d67e8e6..13c18f3d9a9b 100644
--- a/drivers/misc/habanalabs/common/habanalabs.h
+++ b/drivers/misc/habanalabs/common/habanalabs.h
@@ -1651,7 +1651,7 @@ struct hl_ioctl_desc {
*
* Return: true if the area is inside the valid range, false otherwise.
*/
-static inline bool hl_mem_area_inside_range(u64 address, u32 size,
+static inline bool hl_mem_area_inside_range(u64 address, u64 size,
u64 range_start_address, u64 range_end_address)
{
u64 end_address = address + size;
diff --git a/drivers/misc/habanalabs/gaudi/gaudi_coresight.c b/drivers/misc/habanalabs/gaudi/gaudi_coresight.c
index 5673ee49819e..881531d4d9da 100644
--- a/drivers/misc/habanalabs/gaudi/gaudi_coresight.c
+++ b/drivers/misc/habanalabs/gaudi/gaudi_coresight.c
@@ -527,7 +527,7 @@ static int gaudi_config_etf(struct hl_device *hdev,
}

static bool gaudi_etr_validate_address(struct hl_device *hdev, u64 addr,
- u32 size, bool *is_host)
+ u64 size, bool *is_host)
{
struct asic_fixed_properties *prop = &hdev->asic_prop;
struct gaudi_device *gaudi = hdev->asic_specific;
@@ -539,6 +539,12 @@ static bool gaudi_etr_validate_address(struct hl_device *hdev, u64 addr,
return false;
}

+ if (addr > (addr + size)) {
+ dev_err(hdev->dev,
+ "ETR buffer size %llu overflow\n", size);
+ return false;
+ }
+
/* PMMU and HPMMU addresses are equal, check only one of them */
if ((gaudi->hw_cap_initialized & HW_CAP_MMU) &&
hl_mem_area_inside_range(addr, size,
diff --git a/drivers/misc/habanalabs/goya/goya_coresight.c b/drivers/misc/habanalabs/goya/goya_coresight.c
index b03912483de0..4027a6a334d7 100644
--- a/drivers/misc/habanalabs/goya/goya_coresight.c
+++ b/drivers/misc/habanalabs/goya/goya_coresight.c
@@ -362,11 +362,17 @@ static int goya_config_etf(struct hl_device *hdev,
}

static int goya_etr_validate_address(struct hl_device *hdev, u64 addr,
- u32 size)
+ u64 size)
{
struct asic_fixed_properties *prop = &hdev->asic_prop;
u64 range_start, range_end;

+ if (addr > (addr + size)) {
+ dev_err(hdev->dev,
+ "ETR buffer size %llu overflow\n", size);
+ return false;
+ }
+
if (hdev->mmu_enable) {
range_start = prop->dmmu.start_addr;
range_end = prop->dmmu.end_addr;
--
2.17.1

2020-08-09 11:55:55

by Oded Gabbay

[permalink] [raw]
Subject: [PATCH 4/4] habanalabs: set max power according to card type

In Gaudi, the default max power setting is different between PCI and PMC
cards. Therefore, the driver need to set the default after knowing what is
the card type.

The current code has a bug where it limits the maximum power of the PMC
card to 200W after a reset occurs.

Signed-off-by: Oded Gabbay <[email protected]>
---
drivers/misc/habanalabs/common/device.c | 7 ++++++-
drivers/misc/habanalabs/common/habanalabs.h | 2 +-
drivers/misc/habanalabs/common/sysfs.c | 7 ++++---
drivers/misc/habanalabs/gaudi/gaudi.c | 9 ++++++++-
drivers/misc/habanalabs/gaudi/gaudiP.h | 3 ++-
5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/habanalabs/common/device.c b/drivers/misc/habanalabs/common/device.c
index be16b75bdfdb..8e34c39380a9 100644
--- a/drivers/misc/habanalabs/common/device.c
+++ b/drivers/misc/habanalabs/common/device.c
@@ -1069,7 +1069,7 @@ int hl_device_reset(struct hl_device *hdev, bool hard_reset,
goto out_err;
}

- hl_set_max_power(hdev, hdev->max_power);
+ hl_set_max_power(hdev);
} else {
rc = hdev->asic_funcs->soft_reset_late_init(hdev);
if (rc) {
@@ -1318,6 +1318,11 @@ int hl_device_init(struct hl_device *hdev, struct class *hclass)
goto out_disabled;
}

+ /* Need to call this again because the max power might change,
+ * depending on card type for certain ASICs
+ */
+ hl_set_max_power(hdev);
+
/*
* hl_hwmon_init() must be called after device_late_init(), because only
* there we get the information from the device about which
diff --git a/drivers/misc/habanalabs/common/habanalabs.h b/drivers/misc/habanalabs/common/habanalabs.h
index 13c18f3d9a9b..5864c6adfc52 100644
--- a/drivers/misc/habanalabs/common/habanalabs.h
+++ b/drivers/misc/habanalabs/common/habanalabs.h
@@ -1858,7 +1858,7 @@ int hl_get_pwm_info(struct hl_device *hdev,
void hl_set_pwm_info(struct hl_device *hdev, int sensor_index, u32 attr,
long value);
u64 hl_get_max_power(struct hl_device *hdev);
-void hl_set_max_power(struct hl_device *hdev, u64 value);
+void hl_set_max_power(struct hl_device *hdev);
int hl_set_voltage(struct hl_device *hdev,
int sensor_index, u32 attr, long value);
int hl_set_current(struct hl_device *hdev,
diff --git a/drivers/misc/habanalabs/common/sysfs.c b/drivers/misc/habanalabs/common/sysfs.c
index b3cb0ac4721c..5ae484cc84cd 100644
--- a/drivers/misc/habanalabs/common/sysfs.c
+++ b/drivers/misc/habanalabs/common/sysfs.c
@@ -81,7 +81,7 @@ u64 hl_get_max_power(struct hl_device *hdev)
return result;
}

-void hl_set_max_power(struct hl_device *hdev, u64 value)
+void hl_set_max_power(struct hl_device *hdev)
{
struct armcp_packet pkt;
int rc;
@@ -90,7 +90,7 @@ void hl_set_max_power(struct hl_device *hdev, u64 value)

pkt.ctl = cpu_to_le32(ARMCP_PACKET_MAX_POWER_SET <<
ARMCP_PKT_CTL_OPCODE_SHIFT);
- pkt.value = cpu_to_le64(value);
+ pkt.value = cpu_to_le64(hdev->max_power);

rc = hdev->asic_funcs->send_cpu_message(hdev, (u32 *) &pkt, sizeof(pkt),
0, NULL);
@@ -316,7 +316,7 @@ static ssize_t max_power_store(struct device *dev,
}

hdev->max_power = value;
- hl_set_max_power(hdev, value);
+ hl_set_max_power(hdev);

out:
return count;
@@ -422,6 +422,7 @@ int hl_sysfs_init(struct hl_device *hdev)
hdev->pm_mng_profile = PM_AUTO;
else
hdev->pm_mng_profile = PM_MANUAL;
+
hdev->max_power = hdev->asic_prop.max_power_default;

hdev->asic_funcs->add_device_attr(hdev, &hl_dev_clks_attr_group);
diff --git a/drivers/misc/habanalabs/gaudi/gaudi.c b/drivers/misc/habanalabs/gaudi/gaudi.c
index 7e0f9f64ffcb..c58343608cc7 100644
--- a/drivers/misc/habanalabs/gaudi/gaudi.c
+++ b/drivers/misc/habanalabs/gaudi/gaudi.c
@@ -456,7 +456,7 @@ static int gaudi_get_fixed_properties(struct hl_device *hdev)
prop->num_of_events = GAUDI_EVENT_SIZE;
prop->tpc_enabled_mask = TPC_ENABLED_MASK;

- prop->max_power_default = MAX_POWER_DEFAULT;
+ prop->max_power_default = MAX_POWER_DEFAULT_PMC;

prop->cb_pool_cb_cnt = GAUDI_CB_POOL_CB_CNT;
prop->cb_pool_cb_size = GAUDI_CB_POOL_CB_SIZE;
@@ -6055,6 +6055,13 @@ static int gaudi_armcp_info_get(struct hl_device *hdev)
strncpy(prop->armcp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
CARD_NAME_MAX_LEN);

+ if (prop->armcp_info.card_type == armcp_card_type_pci)
+ prop->max_power_default = MAX_POWER_DEFAULT_PCI;
+ else if (prop->armcp_info.card_type == armcp_card_type_pmc)
+ prop->max_power_default = MAX_POWER_DEFAULT_PMC;
+
+ hdev->max_power = prop->max_power_default;
+
return 0;
}

diff --git a/drivers/misc/habanalabs/gaudi/gaudiP.h b/drivers/misc/habanalabs/gaudi/gaudiP.h
index 5dc99f6f0296..182eaf6193a3 100644
--- a/drivers/misc/habanalabs/gaudi/gaudiP.h
+++ b/drivers/misc/habanalabs/gaudi/gaudiP.h
@@ -41,7 +41,8 @@

#define GAUDI_MAX_CLK_FREQ 2200000000ull /* 2200 MHz */

-#define MAX_POWER_DEFAULT 200000 /* 200W */
+#define MAX_POWER_DEFAULT_PCI 200000 /* 200W */
+#define MAX_POWER_DEFAULT_PMC 200000 /* 350W */

#define GAUDI_CPU_TIMEOUT_USEC 15000000 /* 15s */

--
2.17.1

2020-08-09 15:24:11

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH 4/4] habanalabs: set max power according to card type

Hi Oded,

I love your patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linus/master next-20200807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Oded-Gabbay/habanalabs-verify-user-input-in-cs_ioctl_signal_wait/20200809-195558
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 47ec5303d73ea344e84f46660fff693c57641386
config: x86_64-randconfig-s022-20200809 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-118-ge1578773-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>


sparse warnings: (new ones prefixed by >>)

>> drivers/misc/habanalabs/gaudi/gaudi.c:6025:34: sparse: sparse: restricted __le32 degrades to integer
drivers/misc/habanalabs/gaudi/gaudi.c:1807:26: sparse: sparse: cast truncates bits from constant value (7ffc4f4000 becomes fc4f4000)
drivers/misc/habanalabs/gaudi/gaudi.c:1811:25: sparse: sparse: cast truncates bits from constant value (7ffc4f2000 becomes fc4f2000)
drivers/misc/habanalabs/gaudi/gaudi.c:1815:26: sparse: sparse: cast truncates bits from constant value (7ffc494000 becomes fc494000)
drivers/misc/habanalabs/gaudi/gaudi.c:1819:25: sparse: sparse: cast truncates bits from constant value (7ffc492000 becomes fc492000)
drivers/misc/habanalabs/gaudi/gaudi.c:1858:17: sparse: sparse: cast truncates bits from constant value (7ffc800040 becomes fc800040)
drivers/misc/habanalabs/gaudi/gaudi.c:1896:9: sparse: sparse: cast truncates bits from constant value (7ffc800040 becomes fc800040)
drivers/misc/habanalabs/gaudi/gaudi.c:1969:23: sparse: sparse: cast truncates bits from constant value (7ffc4f4000 becomes fc4f4000)
drivers/misc/habanalabs/gaudi/gaudi.c:1973:22: sparse: sparse: cast truncates bits from constant value (7ffc4f2000 becomes fc4f2000)
drivers/misc/habanalabs/gaudi/gaudi.c:2006:17: sparse: sparse: cast truncates bits from constant value (7ffc800040 becomes fc800040)
drivers/misc/habanalabs/gaudi/gaudi.c:2079:23: sparse: sparse: cast truncates bits from constant value (7ffc4f4000 becomes fc4f4000)
drivers/misc/habanalabs/gaudi/gaudi.c:2083:22: sparse: sparse: cast truncates bits from constant value (7ffc4f2000 becomes fc4f2000)
drivers/misc/habanalabs/gaudi/gaudi.c:2118:17: sparse: sparse: cast truncates bits from constant value (7ffc800040 becomes fc800040)
drivers/misc/habanalabs/gaudi/gaudi.c:2193:23: sparse: sparse: cast truncates bits from constant value (7ffc4f4000 becomes fc4f4000)
drivers/misc/habanalabs/gaudi/gaudi.c:2197:22: sparse: sparse: cast truncates bits from constant value (7ffc4f2000 becomes fc4f2000)
drivers/misc/habanalabs/gaudi/gaudi.c:2233:17: sparse: sparse: cast truncates bits from constant value (7ffc800040 becomes fc800040)
drivers/misc/habanalabs/gaudi/gaudi.c:2728:27: sparse: sparse: cast truncates bits from constant value (7ff0000000 becomes f0000000)
drivers/misc/habanalabs/gaudi/gaudi.c:6209:9: sparse: sparse: cast truncates bits from constant value (7ffc4f2000 becomes fc4f2000)

vim +6025 drivers/misc/habanalabs/gaudi/gaudi.c

6005
6006 static int gaudi_armcp_info_get(struct hl_device *hdev)
6007 {
6008 struct gaudi_device *gaudi = hdev->asic_specific;
6009 struct asic_fixed_properties *prop = &hdev->asic_prop;
6010 int rc;
6011
6012 if (!(gaudi->hw_cap_initialized & HW_CAP_CPU_Q))
6013 return 0;
6014
6015 rc = hl_fw_armcp_info_get(hdev);
6016 if (rc)
6017 return rc;
6018
6019 if (!strlen(prop->armcp_info.card_name))
6020 strncpy(prop->armcp_info.card_name, GAUDI_DEFAULT_CARD_NAME,
6021 CARD_NAME_MAX_LEN);
6022
6023 if (prop->armcp_info.card_type == armcp_card_type_pci)
6024 prop->max_power_default = MAX_POWER_DEFAULT_PCI;
> 6025 else if (prop->armcp_info.card_type == armcp_card_type_pmc)
6026 prop->max_power_default = MAX_POWER_DEFAULT_PMC;
6027
6028 hdev->max_power = prop->max_power_default;
6029
6030 return 0;
6031 }
6032

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]


Attachments:
(No filename) (4.55 kB)
.config.gz (32.80 kB)
Download all attachments