Size of ring buffer, as defined in uio_hv_generic driver, is no longer
fixed to 16 KB. This creates a problem in fcopy, since this size was
hardcoded. With the change in place to make ring sysfs node actually
reflect the size of underlying ring buffer, it is safe to get the size
of ring sysfs file and use it for ring buffer size in fcopy daemon.
Fix the issue of disparity in ring buffer size, by making it dynamic
in fcopy uio daemon.

Cc: stable@vger.kernel.org
Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
---
 tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
 1 file changed, 75 insertions(+), 7 deletions(-)

diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
index 0198321d14a2..5388ee1ebf4d 100644
--- a/tools/hv/hv_fcopy_uio_daemon.c
+++ b/tools/hv/hv_fcopy_uio_daemon.c
@@ -36,6 +36,7 @@
 #define WIN8_SRV_VERSION	(WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
 
 #define FCOPY_UIO		"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
+#define FCOPY_CHANNELS_PATH	"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/channels"
 
 #define FCOPY_VER_COUNT		1
 static const int fcopy_versions[] = {
@@ -47,9 +48,62 @@ static const int fw_versions[] = {
 	UTIL_FW_VERSION
 };
 
-#define HV_RING_SIZE		0x4000 /* 16KB ring buffer size */
+static uint32_t get_ring_buffer_size(void)
+{
+	char ring_path[PATH_MAX];
+	DIR *dir;
+	struct dirent *entry;
+	struct stat st;
+	uint32_t ring_size = 0;
+	int retry_count = 0;
 
-static unsigned char desc[HV_RING_SIZE];
+	/* Find the channel directory */
+	dir = opendir(FCOPY_CHANNELS_PATH);
+	if (!dir) {
+		usleep(100 * 1000); /* Avoid race with kernel, wait 100ms and retry once */
+		dir = opendir(FCOPY_CHANNELS_PATH);
+		if (!dir) {
+			syslog(LOG_ERR, "Failed to open channels directory: %s", strerror(errno));
+			return 0;
+		}
+	}
+
+retry_once:
+	while ((entry = readdir(dir)) != NULL) {
+		if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
+		    strcmp(entry->d_name, "..") != 0) {
+			snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
+				 FCOPY_CHANNELS_PATH, entry->d_name);
+
+			if (stat(ring_path, &st) == 0) {
+				/*
+				 * stat returns size of Tx, Rx rings combined,
+				 * so take half of it for individual ring size.
+				 */
+				ring_size = (uint32_t)st.st_size / 2;
+				syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
+				       ring_path, ring_size);
+				break;
+			}
+		}
+	}
+
+	if (!ring_size && retry_count == 0) {
+		retry_count = 1;
+		rewinddir(dir);
+		usleep(100 * 1000); /* Wait 100ms and retry once */
+		goto retry_once;
+	}
+
+	closedir(dir);
+
+	if (!ring_size)
+		syslog(LOG_ERR, "Could not determine ring size");
+
+	return ring_size;
+}
+
+static unsigned char *desc;
 
 static int target_fd;
 static char target_fname[PATH_MAX];
@@ -406,7 +460,7 @@ int main(int argc, char *argv[])
 	int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
 	struct vmbus_br txbr, rxbr;
 	void *ring;
-	uint32_t len = HV_RING_SIZE;
+	uint32_t ring_size, len;
 	char uio_name[NAME_MAX] = {0};
 	char uio_dev_path[PATH_MAX] = {0};
 
@@ -437,6 +491,20 @@ int main(int argc, char *argv[])
 	openlog("HV_UIO_FCOPY", 0, LOG_USER);
 	syslog(LOG_INFO, "starting; pid is:%d", getpid());
 
+	ring_size = get_ring_buffer_size();
+	if (!ring_size) {
+		ret = -ENODEV;
+		goto exit;
+	}
+
+	len = ring_size;
+	desc = malloc(ring_size * sizeof(unsigned char));
+	if (!desc) {
+		syslog(LOG_ERR, "malloc failed for desc buffer");
+		ret = -ENOMEM;
+		goto exit;
+	}
+
 	fcopy_get_first_folder(FCOPY_UIO, uio_name);
 	snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
 	fcopy_fd = open(uio_dev_path, O_RDWR);
@@ -448,14 +516,14 @@ int main(int argc, char *argv[])
 		goto exit;
 	}
 
-	ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
+	ring = vmbus_uio_map(&fcopy_fd, ring_size);
 	if (!ring) {
 		ret = errno;
 		syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
 		goto close;
 	}
-	vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
-	vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
+	vmbus_br_setup(&txbr, ring, ring_size);
+	vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
 
 	rxbr.vbr->imask = 0;
 
@@ -472,7 +540,7 @@ int main(int argc, char *argv[])
 			goto close;
 		}
 
-		len = HV_RING_SIZE;
+		len = ring_size;
 		ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
 		if (unlikely(ret <= 0)) {
 			/* This indicates a failure to communicate (or worse) */

base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
-- 
2.34.1

On Tue, Jul 08, 2025 at 01:33:19PM +0530, Naman Jain wrote:
> Size of ring buffer, as defined in uio_hv_generic driver, is no longer
> fixed to 16 KB. This creates a problem in fcopy, since this size was
> hardcoded. With the change in place to make ring sysfs node actually
> reflect the size of underlying ring buffer, it is safe to get the size
> of ring sysfs file and use it for ring buffer size in fcopy daemon.
> Fix the issue of disparity in ring buffer size, by making it dynamic
> in fcopy uio daemon.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---
>  tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
>  1 file changed, 75 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
> index 0198321d14a2..5388ee1ebf4d 100644
> --- a/tools/hv/hv_fcopy_uio_daemon.c
> +++ b/tools/hv/hv_fcopy_uio_daemon.c
> @@ -36,6 +36,7 @@
>  #define WIN8_SRV_VERSION	(WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
>  
>  #define FCOPY_UIO		"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
> +#define FCOPY_CHANNELS_PATH	"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/channels"

We can use a single path up to the device ID and then append either 'uio' or 'channels' using
two separate variables.

>  
>  #define FCOPY_VER_COUNT		1
>  static const int fcopy_versions[] = {
> @@ -47,9 +48,62 @@ static const int fw_versions[] = {
>  	UTIL_FW_VERSION
>  };
>  
> -#define HV_RING_SIZE		0x4000 /* 16KB ring buffer size */
> +static uint32_t get_ring_buffer_size(void)
> +{
> +	char ring_path[PATH_MAX];
> +	DIR *dir;
> +	struct dirent *entry;
> +	struct stat st;
> +	uint32_t ring_size = 0;
> +	int retry_count = 0;
>  
> -static unsigned char desc[HV_RING_SIZE];
> +	/* Find the channel directory */
> +	dir = opendir(FCOPY_CHANNELS_PATH);
> +	if (!dir) {
> +		usleep(100 * 1000); /* Avoid race with kernel, wait 100ms and retry once */
> +		dir = opendir(FCOPY_CHANNELS_PATH);
> +		if (!dir) {
> +			syslog(LOG_ERR, "Failed to open channels directory: %s", strerror(errno));
> +			return 0;
> +		}
> +	}
> +
> +retry_once:
> +	while ((entry = readdir(dir)) != NULL) {
> +		if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
> +		    strcmp(entry->d_name, "..") != 0) {
> +			snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
> +				 FCOPY_CHANNELS_PATH, entry->d_name);
> +
> +			if (stat(ring_path, &st) == 0) {
> +				/*
> +				 * stat returns size of Tx, Rx rings combined,
> +				 * so take half of it for individual ring size.
> +				 */
> +				ring_size = (uint32_t)st.st_size / 2;
> +				syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
> +				       ring_path, ring_size);
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (!ring_size && retry_count == 0) {
> +		retry_count = 1;
> +		rewinddir(dir);
> +		usleep(100 * 1000); /* Wait 100ms and retry once */
> +		goto retry_once;

		Is this retry solving any real problem ?

> +	}
> +
> +	closedir(dir);
> +
> +	if (!ring_size)
> +		syslog(LOG_ERR, "Could not determine ring size");
> +
> +	return ring_size;
> +}
> +
> +static unsigned char *desc;
>  
>  static int target_fd;
>  static char target_fname[PATH_MAX];
> @@ -406,7 +460,7 @@ int main(int argc, char *argv[])
>  	int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
>  	struct vmbus_br txbr, rxbr;
>  	void *ring;
> -	uint32_t len = HV_RING_SIZE;
> +	uint32_t ring_size, len;
>  	char uio_name[NAME_MAX] = {0};
>  	char uio_dev_path[PATH_MAX] = {0};
>  
> @@ -437,6 +491,20 @@ int main(int argc, char *argv[])
>  	openlog("HV_UIO_FCOPY", 0, LOG_USER);
>  	syslog(LOG_INFO, "starting; pid is:%d", getpid());
>  
> +	ring_size = get_ring_buffer_size();
> +	if (!ring_size) {
> +		ret = -ENODEV;
> +		goto exit;
> +	}
> +
> +	len = ring_size;

	Do we need this ?

> +	desc = malloc(ring_size * sizeof(unsigned char));
> +	if (!desc) {
> +		syslog(LOG_ERR, "malloc failed for desc buffer");
> +		ret = -ENOMEM;
> +		goto exit;
> +	}

	This memory is not being freed anywhere. While I agree that freeing memory at
	program exit may not have much practical value, we can easily address
	this by adding a goto label for cleanup, this will keep all the static code
	analyzers happy.

> +
>  	fcopy_get_first_folder(FCOPY_UIO, uio_name);
>  	snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
>  	fcopy_fd = open(uio_dev_path, O_RDWR);
> @@ -448,14 +516,14 @@ int main(int argc, char *argv[])
>  		goto exit;
>  	}
>  
> -	ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
> +	ring = vmbus_uio_map(&fcopy_fd, ring_size);
>  	if (!ring) {
>  		ret = errno;
>  		syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
>  		goto close;
>  	}
> -	vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
> -	vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
> +	vmbus_br_setup(&txbr, ring, ring_size);
> +	vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
>  
>  	rxbr.vbr->imask = 0;
>  
> @@ -472,7 +540,7 @@ int main(int argc, char *argv[])
>  			goto close;
>  		}
>  
> -		len = HV_RING_SIZE;
> +		len = ring_size;
>  		ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
>  		if (unlikely(ret <= 0)) {
>  			/* This indicates a failure to communicate (or worse) */
> 
> base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
> -- 
> 2.34.1

On 7/9/2025 4:52 PM, Saurabh Singh Sengar wrote:
> On Tue, Jul 08, 2025 at 01:33:19PM +0530, Naman Jain wrote:
>> Size of ring buffer, as defined in uio_hv_generic driver, is no longer
>> fixed to 16 KB. This creates a problem in fcopy, since this size was
>> hardcoded. With the change in place to make ring sysfs node actually
>> reflect the size of underlying ring buffer, it is safe to get the size
>> of ring sysfs file and use it for ring buffer size in fcopy daemon.
>> Fix the issue of disparity in ring buffer size, by making it dynamic
>> in fcopy uio daemon.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>> ---
>>   tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
>>   1 file changed, 75 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/hv_fcopy_uio_daemon.c
>> index 0198321d14a2..5388ee1ebf4d 100644
>> --- a/tools/hv/hv_fcopy_uio_daemon.c
>> +++ b/tools/hv/hv_fcopy_uio_daemon.c
>> @@ -36,6 +36,7 @@
>>   #define WIN8_SRV_VERSION	(WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
>>   
>>   #define FCOPY_UIO		"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
>> +#define FCOPY_CHANNELS_PATH	"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/channels"
> 
> We can use a single path up to the device ID and then append either 'uio' or 'channels' using
> two separate variables.

I am planning to use it like this, please let me know if it is OK.

+#define FCOPY_DEVICE_PATH(subdir) 
"/sys/bus/vmbus/devices/eb765408-105f-49b6-b4aa-c123b64d17d4/"#subdir
+#define FCOPY_UIO_PATH          FCOPY_DEVICE_PATH(uio)
+#define FCOPY_CHANNELS_PATH     FCOPY_DEVICE_PATH(channels)

> 
>>   
>>   #define FCOPY_VER_COUNT		1
>>   static const int fcopy_versions[] = {
>> @@ -47,9 +48,62 @@ static const int fw_versions[] = {
>>   	UTIL_FW_VERSION
>>   };
>>   
>> -#define HV_RING_SIZE		0x4000 /* 16KB ring buffer size */
>> +static uint32_t get_ring_buffer_size(void)
>> +{
>> +	char ring_path[PATH_MAX];
>> +	DIR *dir;
>> +	struct dirent *entry;
>> +	struct stat st;
>> +	uint32_t ring_size = 0;
>> +	int retry_count = 0;
>>   
>> -static unsigned char desc[HV_RING_SIZE];
>> +	/* Find the channel directory */
>> +	dir = opendir(FCOPY_CHANNELS_PATH);
>> +	if (!dir) {
>> +		usleep(100 * 1000); /* Avoid race with kernel, wait 100ms and retry once */
>> +		dir = opendir(FCOPY_CHANNELS_PATH);
>> +		if (!dir) {
>> +			syslog(LOG_ERR, "Failed to open channels directory: %s", strerror(errno));
>> +			return 0;
>> +		}
>> +	}
>> +
>> +retry_once:
>> +	while ((entry = readdir(dir)) != NULL) {
>> +		if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 0 &&
>> +		    strcmp(entry->d_name, "..") != 0) {
>> +			snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
>> +				 FCOPY_CHANNELS_PATH, entry->d_name);
>> +
>> +			if (stat(ring_path, &st) == 0) {
>> +				/*
>> +				 * stat returns size of Tx, Rx rings combined,
>> +				 * so take half of it for individual ring size.
>> +				 */
>> +				ring_size = (uint32_t)st.st_size / 2;
>> +				syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
>> +				       ring_path, ring_size);
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (!ring_size && retry_count == 0) {
>> +		retry_count = 1;
>> +		rewinddir(dir);
>> +		usleep(100 * 1000); /* Wait 100ms and retry once */
>> +		goto retry_once;
> 
> 		Is this retry solving any real problem ?

Yes, these two retry mechanism are added to avoid race conditions with 
creation of channels dir, numbered channels inside channels directory.
More in patch 1 comment by Michael.
https://lore.kernel.org/all/SN6PR02MB41574C54FFDE0D3F3B7A5649D47CA@SN6PR02MB4157.namprd02.prod.outlook.com/

> 
>> +	}
>> +
>> +	closedir(dir);
>> +
>> +	if (!ring_size)
>> +		syslog(LOG_ERR, "Could not determine ring size");
>> +
>> +	return ring_size;
>> +}
>> +
>> +static unsigned char *desc;
>>   
>>   static int target_fd;
>>   static char target_fname[PATH_MAX];
>> @@ -406,7 +460,7 @@ int main(int argc, char *argv[])
>>   	int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
>>   	struct vmbus_br txbr, rxbr;
>>   	void *ring;
>> -	uint32_t len = HV_RING_SIZE;
>> +	uint32_t ring_size, len;
>>   	char uio_name[NAME_MAX] = {0};
>>   	char uio_dev_path[PATH_MAX] = {0};
>>   
>> @@ -437,6 +491,20 @@ int main(int argc, char *argv[])
>>   	openlog("HV_UIO_FCOPY", 0, LOG_USER);
>>   	syslog(LOG_INFO, "starting; pid is:%d", getpid());
>>   
>> +	ring_size = get_ring_buffer_size();
>> +	if (!ring_size) {
>> +		ret = -ENODEV;
>> +		goto exit;
>> +	}
>> +
>> +	len = ring_size;
> 
> 	Do we need this ?

Yes, because len is being used as a temporary variable for storing
ring_size, and it is modified when we pass it with reference in
rte_vmbus_chan_recv_raw. In order to avoid calculating ring sizes again,
we need to keep ring_size separate.

> 
>> +	desc = malloc(ring_size * sizeof(unsigned char));
>> +	if (!desc) {
>> +		syslog(LOG_ERR, "malloc failed for desc buffer");
>> +		ret = -ENOMEM;
>> +		goto exit;
>> +	}
> 
> 	This memory is not being freed anywhere. While I agree that freeing memory at
> 	program exit may not have much practical value, we can easily address
> 	this by adding a goto label for cleanup, this will keep all the static code
> 	analyzers happy.
> 

Sure. Will add it.

>> +
>>   	fcopy_get_first_folder(FCOPY_UIO, uio_name);
>>   	snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
>>   	fcopy_fd = open(uio_dev_path, O_RDWR);
>> @@ -448,14 +516,14 @@ int main(int argc, char *argv[])
>>   		goto exit;
>>   	}
>>   
>> -	ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
>> +	ring = vmbus_uio_map(&fcopy_fd, ring_size);
>>   	if (!ring) {
>>   		ret = errno;
>>   		syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", ret, strerror(ret));
>>   		goto close;
>>   	}
>> -	vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
>> -	vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
>> +	vmbus_br_setup(&txbr, ring, ring_size);
>> +	vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
>>   
>>   	rxbr.vbr->imask = 0;
>>   
>> @@ -472,7 +540,7 @@ int main(int argc, char *argv[])
>>   			goto close;
>>   		}
>>   
>> -		len = HV_RING_SIZE;
>> +		len = ring_size;
>>   		ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
>>   		if (unlikely(ret <= 0)) {
>>   			/* This indicates a failure to communicate (or worse) */
>>
>> base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
>> -- 
>> 2.34.1

Regards,
Naman

On 7/10/2025 11:54 AM, Naman Jain wrote:
> 
> 
> On 7/9/2025 4:52 PM, Saurabh Singh Sengar wrote:
>> On Tue, Jul 08, 2025 at 01:33:19PM +0530, Naman Jain wrote:
>>> Size of ring buffer, as defined in uio_hv_generic driver, is no longer
>>> fixed to 16 KB. This creates a problem in fcopy, since this size was
>>> hardcoded. With the change in place to make ring sysfs node actually
>>> reflect the size of underlying ring buffer, it is safe to get the size
>>> of ring sysfs file and use it for ring buffer size in fcopy daemon.
>>> Fix the issue of disparity in ring buffer size, by making it dynamic
>>> in fcopy uio daemon.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
>>> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
>>> ---
>>>   tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
>>>   1 file changed, 75 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/tools/hv/hv_fcopy_uio_daemon.c b/tools/hv/ 
>>> hv_fcopy_uio_daemon.c
>>> index 0198321d14a2..5388ee1ebf4d 100644
>>> --- a/tools/hv/hv_fcopy_uio_daemon.c
>>> +++ b/tools/hv/hv_fcopy_uio_daemon.c
>>> @@ -36,6 +36,7 @@
>>>   #define WIN8_SRV_VERSION    (WIN8_SRV_MAJOR << 16 | WIN8_SRV_MINOR)
>>>   #define FCOPY_UIO        "/sys/bus/vmbus/devices/ 
>>> eb765408-105f-49b6-b4aa-c123b64d17d4/uio"
>>> +#define FCOPY_CHANNELS_PATH    "/sys/bus/vmbus/devices/ 
>>> eb765408-105f-49b6-b4aa-c123b64d17d4/channels"
>>
>> We can use a single path up to the device ID and then append either 
>> 'uio' or 'channels' using
>> two separate variables.
> 
> I am planning to use it like this, please let me know if it is OK.
> 
> +#define FCOPY_DEVICE_PATH(subdir) "/sys/bus/vmbus/devices/ 
> eb765408-105f-49b6-b4aa-c123b64d17d4/"#subdir
> +#define FCOPY_UIO_PATH          FCOPY_DEVICE_PATH(uio)
> +#define FCOPY_CHANNELS_PATH     FCOPY_DEVICE_PATH(channels)

As per your suggestion, using it like this avoids the need to change 
hard coded device path at two places, and it looks better from code 
re-usability POV. No functional changes.

I will make this change in next version.


> 
>>
>>>   #define FCOPY_VER_COUNT        1
>>>   static const int fcopy_versions[] = {
>>> @@ -47,9 +48,62 @@ static const int fw_versions[] = {
>>>       UTIL_FW_VERSION
>>>   };
>>> -#define HV_RING_SIZE        0x4000 /* 16KB ring buffer size */
>>> +static uint32_t get_ring_buffer_size(void)
>>> +{
>>> +    char ring_path[PATH_MAX];
>>> +    DIR *dir;
>>> +    struct dirent *entry;
>>> +    struct stat st;
>>> +    uint32_t ring_size = 0;
>>> +    int retry_count = 0;
>>> -static unsigned char desc[HV_RING_SIZE];
>>> +    /* Find the channel directory */
>>> +    dir = opendir(FCOPY_CHANNELS_PATH);
>>> +    if (!dir) {
>>> +        usleep(100 * 1000); /* Avoid race with kernel, wait 100ms 
>>> and retry once */
>>> +        dir = opendir(FCOPY_CHANNELS_PATH);
>>> +        if (!dir) {
>>> +            syslog(LOG_ERR, "Failed to open channels directory: %s", 
>>> strerror(errno));
>>> +            return 0;
>>> +        }
>>> +    }
>>> +
>>> +retry_once:
>>> +    while ((entry = readdir(dir)) != NULL) {
>>> +        if (entry->d_type == DT_DIR && strcmp(entry->d_name, ".") != 
>>> 0 &&
>>> +            strcmp(entry->d_name, "..") != 0) {
>>> +            snprintf(ring_path, sizeof(ring_path), "%s/%s/ring",
>>> +                 FCOPY_CHANNELS_PATH, entry->d_name);
>>> +
>>> +            if (stat(ring_path, &st) == 0) {
>>> +                /*
>>> +                 * stat returns size of Tx, Rx rings combined,
>>> +                 * so take half of it for individual ring size.
>>> +                 */
>>> +                ring_size = (uint32_t)st.st_size / 2;
>>> +                syslog(LOG_INFO, "Ring buffer size from %s: %u bytes",
>>> +                       ring_path, ring_size);
>>> +                break;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if (!ring_size && retry_count == 0) {
>>> +        retry_count = 1;
>>> +        rewinddir(dir);
>>> +        usleep(100 * 1000); /* Wait 100ms and retry once */
>>> +        goto retry_once;
>>
>>         Is this retry solving any real problem ?
> 
> Yes, these two retry mechanism are added to avoid race conditions with 
> creation of channels dir, numbered channels inside channels directory.
> More in patch 1 comment by Michael.
> https://lore.kernel.org/all/ 
> SN6PR02MB41574C54FFDE0D3F3B7A5649D47CA@SN6PR02MB4157.namprd02.prod.outlook.com/
> 
>>
>>> +    }
>>> +
>>> +    closedir(dir);
>>> +
>>> +    if (!ring_size)
>>> +        syslog(LOG_ERR, "Could not determine ring size");
>>> +
>>> +    return ring_size;
>>> +}
>>> +
>>> +static unsigned char *desc;
>>>   static int target_fd;
>>>   static char target_fname[PATH_MAX];
>>> @@ -406,7 +460,7 @@ int main(int argc, char *argv[])
>>>       int daemonize = 1, long_index = 0, opt, ret = -EINVAL;
>>>       struct vmbus_br txbr, rxbr;
>>>       void *ring;
>>> -    uint32_t len = HV_RING_SIZE;
>>> +    uint32_t ring_size, len;
>>>       char uio_name[NAME_MAX] = {0};
>>>       char uio_dev_path[PATH_MAX] = {0};
>>> @@ -437,6 +491,20 @@ int main(int argc, char *argv[])
>>>       openlog("HV_UIO_FCOPY", 0, LOG_USER);
>>>       syslog(LOG_INFO, "starting; pid is:%d", getpid());
>>> +    ring_size = get_ring_buffer_size();
>>> +    if (!ring_size) {
>>> +        ret = -ENODEV;
>>> +        goto exit;
>>> +    }
>>> +
>>> +    len = ring_size;
>>
>>     Do we need this ?
> 
> Yes, because len is being used as a temporary variable for storing
> ring_size, and it is modified when we pass it with reference in
> rte_vmbus_chan_recv_raw. In order to avoid calculating ring sizes again,
> we need to keep ring_size separate.

I misinterpreted your query. We can remove this line, since len is
reinitialized to ring_size before using again in main() function.

> 
>>
>>> +    desc = malloc(ring_size * sizeof(unsigned char));
>>> +    if (!desc) {
>>> +        syslog(LOG_ERR, "malloc failed for desc buffer");
>>> +        ret = -ENOMEM;
>>> +        goto exit;
>>> +    }
>>
>>     This memory is not being freed anywhere. While I agree that 
>> freeing memory at
>>     program exit may not have much practical value, we can easily address
>>     this by adding a goto label for cleanup, this will keep all the 
>> static code
>>     analyzers happy.
>>
> 
> Sure. Will add it.
> 
>>> +
>>>       fcopy_get_first_folder(FCOPY_UIO, uio_name);
>>>       snprintf(uio_dev_path, sizeof(uio_dev_path), "/dev/%s", uio_name);
>>>       fcopy_fd = open(uio_dev_path, O_RDWR);
>>> @@ -448,14 +516,14 @@ int main(int argc, char *argv[])
>>>           goto exit;
>>>       }
>>> -    ring = vmbus_uio_map(&fcopy_fd, HV_RING_SIZE);
>>> +    ring = vmbus_uio_map(&fcopy_fd, ring_size);
>>>       if (!ring) {
>>>           ret = errno;
>>>           syslog(LOG_ERR, "mmap ringbuffer failed; error: %d %s", 
>>> ret, strerror(ret));
>>>           goto close;
>>>       }
>>> -    vmbus_br_setup(&txbr, ring, HV_RING_SIZE);
>>> -    vmbus_br_setup(&rxbr, (char *)ring + HV_RING_SIZE, HV_RING_SIZE);
>>> +    vmbus_br_setup(&txbr, ring, ring_size);
>>> +    vmbus_br_setup(&rxbr, (char *)ring + ring_size, ring_size);
>>>       rxbr.vbr->imask = 0;
>>> @@ -472,7 +540,7 @@ int main(int argc, char *argv[])
>>>               goto close;
>>>           }
>>> -        len = HV_RING_SIZE;
>>> +        len = ring_size;
>>>           ret = rte_vmbus_chan_recv_raw(&rxbr, desc, &len);
>>>           if (unlikely(ret <= 0)) {
>>>               /* This indicates a failure to communicate (or worse) */
>>>
>>> base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
>>> -- 
>>> 2.34.1
> 
> Regards,
> Naman

On 7/8/2025 1:33 PM, Naman Jain wrote:
> Size of ring buffer, as defined in uio_hv_generic driver, is no longer
> fixed to 16 KB. This creates a problem in fcopy, since this size was
> hardcoded. With the change in place to make ring sysfs node actually
> reflect the size of underlying ring buffer, it is safe to get the size
> of ring sysfs file and use it for ring buffer size in fcopy daemon.
> Fix the issue of disparity in ring buffer size, by making it dynamic
> in fcopy uio daemon.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0315fef2aff9 ("uio_hv_generic: Align ring size to system page")
> Signed-off-by: Naman Jain <namjain@linux.microsoft.com>
> ---

Noticed that I missed adding change logs. Adding them now.

Changes since v2:
https://lore.kernel.org/all/20250701104837.3006-1-namjain@linux.microsoft.com/
* Removed fallback mechanism to default size, to keep fcopy behavior
consistent (Long's suggestion). If ring sysfs file is not present for
some reason, things are already bad and its the right thing for fcopy to
abort.

Changes since v1:
https://lore.kernel.org/all/20250620070618.3097-1-namjain@linux.microsoft.com/

* Removed unnecessary type casting in malloc for desc variable (Olaf)
* Added retry mechanisms to avoid potential race conditions (Michael)
* Moved the logic to fetch ring size to a later part in main (Michael)


>   tools/hv/hv_fcopy_uio_daemon.c | 82 +++++++++++++++++++++++++++++++---
>   1 file changed, 75 insertions(+), 7 deletions(-)
> 


Regards,
Naman