[PATCH] crypto: ccp/sfs - Use DIV_ROUND_UP for set_memory_uc() size calculation

[PATCH] crypto: ccp/sfs - Use DIV_ROUND_UP for set_memory_uc() size calculation

[kfatyuip]

From: Kieran Moy <kfatyuip@gmail.com>

The SFS driver allocates a 2MB command buffer using snp_alloc_hv_fixed_pages(),
but set_memory_uc() is called with SFS_NUM_PAGES_CMDBUF which assumes 4KB pages.
This mismatch could lead to incomplete cache attribute updates on the buffer if
the payload size is not strictly page-aligned.

Switch to using DIV_ROUND_UP(SFS_MAX_PAYLOAD_SIZE, PAGE_SIZE) to calculate the
number of pages required for the attribute update. This approach follows kernel
coding best practices, improves code robustness, and ensures that all buffer
regions are properly covered regardless of current or future PAGE_SIZE values.

Using DIV_ROUND_UP is also consistent with Linux kernel style for page counting,
which avoids hidden bugs in case the payload size ever changes and is not a
multiple of PAGE_SIZE, or if the kernel is built with a non-default page size.

Signed-off-by: Kieran Moy <kfatyuip@gmail.com>
---
 drivers/crypto/ccp/sfs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/sfs.c b/drivers/crypto/ccp/sfs.c
index 2f4beaafe7ec..3397895160c0 100644
--- a/drivers/crypto/ccp/sfs.c
+++ b/drivers/crypto/ccp/sfs.c
@@ -277,7 +277,8 @@ int sfs_dev_init(struct psp_device *psp)
 	/*
 	* SFS command buffer must be mapped as non-cacheable.
 	*/
-	ret = set_memory_uc((unsigned long)sfs_dev->command_buf, SFS_NUM_PAGES_CMDBUF);
+	ret = set_memory_uc((unsigned long)sfs_dev->command_buf,
+			   DIV_ROUND_UP(SFS_MAX_PAYLOAD_SIZE, PAGE_SIZE));
 	if (ret) {
 		dev_dbg(dev, "Set memory uc failed\n");
 		goto cleanup_cmd_buf;
-- 
2.51.0

Re: [PATCH] crypto: ccp/sfs - Use DIV_ROUND_UP for set_memory_uc() size calculation

[Ashish Kalra]

>From: Kieran Moy <kfatyuip@gmail.com>

>The SFS driver allocates a 2MB command buffer using snp_alloc_hv_fixed_pages(),
>but set_memory_uc() is called with SFS_NUM_PAGES_CMDBUF which assumes 4KB pages.
>This mismatch could lead to incomplete cache attribute updates on the buffer if
>the payload size is not strictly page-aligned.

>Switch to using DIV_ROUND_UP(SFS_MAX_PAYLOAD_SIZE, PAGE_SIZE) to calculate the
>number of pages required for the attribute update. This approach follows kernel
>coding best practices, improves code robustness, and ensures that all buffer
>regions are properly covered regardless of current or future PAGE_SIZE values.

>Using DIV_ROUND_UP is also consistent with Linux kernel style for page counting,
>which avoids hidden bugs in case the payload size ever changes and is not a
>multiple of PAGE_SIZE, or if the kernel is built with a non-default page size.

>Signed-off-by: Kieran Moy <kfatyuip@gmail.com>
>---
> drivers/crypto/ccp/sfs.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

>diff --git a/drivers/crypto/ccp/sfs.c b/drivers/crypto/ccp/sfs.c
>index 2f4beaafe7ec..3397895160c0 100644
>--- a/drivers/crypto/ccp/sfs.c
>+++ b/drivers/crypto/ccp/sfs.c
>@@ -277,7 +277,8 @@ int sfs_dev_init(struct psp_device *psp)
> 	/*
> 	* SFS command buffer must be mapped as non-cacheable.
> 	*/
>-	ret = set_memory_uc((unsigned long)sfs_dev->command_buf, SFS_NUM_PAGES_CMDBUF);
>+	ret = set_memory_uc((unsigned long)sfs_dev->command_buf,
>+			   DIV_ROUND_UP(SFS_MAX_PAYLOAD_SIZE, PAGE_SIZE));

I am not sure, i find the maths quite straightforward: 

The command buffer is allocated using snp_alloc_hv_fixed_pages() or alloc_pages() using
an order of 9, which should be allocating 512 pages and returning a 2MB aligned address 
and then set_memory_uc() is (SFS_MAX_PAYLOAD_SIZE / PAGE_SIZE) which computes 512 pages.
Also the payload size here is page-aligned and is a constant.

Thanks,
Ashish

> 	if (ret) {
> 		dev_dbg(dev, "Set memory uc failed\n");
> 		goto cleanup_cmd_buf;

Re: [PATCH] crypto: ccp/sfs - Use DIV_ROUND_UP for set_memory_uc() size calculation

[kfatyuip]

From: Kieran Moy <kfatyuip@gmail.com>

Signed-off-by: Kieran Moy <kfatyuip@gmail.com>

Thank you for your review and for pointing this out. You are absolutely right - the math is indeed straightforward in this case, and the existing calculation is correct. I apologize for my unnecessary change.

Thanks again for your attention to this and for the clarification.

Best regards,  
Kieran Moy