[PATCH] vdpa_sim_blk: add `capacity` module parameter

[PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

The vDPA block simulator always allocated a 128 MiB ram-disk, but some
filesystems (e.g. XFS) may require larger minimum sizes (see
https://issues.redhat.com/browse/RHEL-45951).

So to allow us to test these filesystems, let's add a module parameter
to control the size of the simulated virtio-blk devices.
The value is mapped directly to the `capacity` field of the virtio-blk
configuration space, so it must be expressed in sector numbers of 512
bytes.

The default value (0x40000) is the same as the previous value, so the
behavior without setting `capacity` remains unchanged.

Before this patch or with this patch without setting `capacity`:
  $ modprobe vdpa-sim-blk
  $ vdpa dev add mgmtdev vdpasim_blk name blk0
  virtio_blk virtio6: 1/0/0 default/read/poll queues
  virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)

After this patch:
  $ modprobe vdpa-sim-blk capacity=614400
  $ vdpa dev add mgmtdev vdpasim_blk name blk0
  virtio_blk virtio6: 1/0/0 default/read/poll queues
  virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
index b137f3679343..18f390149836 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -33,7 +33,6 @@
 				 (1ULL << VIRTIO_BLK_F_DISCARD)  | \
 				 (1ULL << VIRTIO_BLK_F_WRITE_ZEROES))
 
-#define VDPASIM_BLK_CAPACITY	0x40000
 #define VDPASIM_BLK_SIZE_MAX	0x1000
 #define VDPASIM_BLK_SEG_MAX	32
 #define VDPASIM_BLK_DWZ_MAX_SECTORS UINT_MAX
@@ -43,6 +42,10 @@
 #define VDPASIM_BLK_AS_NUM	1
 #define VDPASIM_BLK_GROUP_NUM	1
 
+static unsigned long capacity = 0x40000;
+module_param(capacity, ulong, 0444);
+MODULE_PARM_DESC(capacity, "virtio-blk device capacity (in 512-byte sectors)");
+
 struct vdpasim_blk {
 	struct vdpasim vdpasim;
 	void *buffer;
@@ -79,10 +82,10 @@ static void vdpasim_blk_buffer_unlock(struct vdpasim_blk *blk)
 static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
 				    u64 num_sectors, u64 max_sectors)
 {
-	if (start_sector > VDPASIM_BLK_CAPACITY) {
+	if (start_sector > capacity) {
 		dev_dbg(&vdpasim->vdpa.dev,
-			"starting sector exceeds the capacity - start: 0x%llx capacity: 0x%x\n",
-			start_sector, VDPASIM_BLK_CAPACITY);
+			"starting sector exceeds the capacity - start: 0x%llx capacity: 0x%lx\n",
+			start_sector, capacity);
 	}
 
 	if (num_sectors > max_sectors) {
@@ -92,10 +95,10 @@ static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
 		return false;
 	}
 
-	if (num_sectors > VDPASIM_BLK_CAPACITY - start_sector) {
+	if (num_sectors > capacity - start_sector) {
 		dev_dbg(&vdpasim->vdpa.dev,
-			"request exceeds the capacity - start: 0x%llx num: 0x%llx capacity: 0x%x\n",
-			start_sector, num_sectors, VDPASIM_BLK_CAPACITY);
+			"request exceeds the capacity - start: 0x%llx num: 0x%llx capacity: 0x%lx\n",
+			start_sector, num_sectors, capacity);
 		return false;
 	}
 
@@ -369,7 +372,7 @@ static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
 
 	memset(config, 0, sizeof(struct virtio_blk_config));
 
-	blk_config->capacity = cpu_to_vdpasim64(vdpasim, VDPASIM_BLK_CAPACITY);
+	blk_config->capacity = cpu_to_vdpasim64(vdpasim, capacity);
 	blk_config->size_max = cpu_to_vdpasim32(vdpasim, VDPASIM_BLK_SIZE_MAX);
 	blk_config->seg_max = cpu_to_vdpasim32(vdpasim, VDPASIM_BLK_SEG_MAX);
 	blk_config->num_queues = cpu_to_vdpasim16(vdpasim, VDPASIM_BLK_VQ_NUM);
@@ -437,8 +440,7 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 	if (blk->shared_backend) {
 		blk->buffer = shared_buffer;
 	} else {
-		blk->buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
-				       GFP_KERNEL);
+		blk->buffer = kvzalloc(capacity << SECTOR_SHIFT, GFP_KERNEL);
 		if (!blk->buffer) {
 			ret = -ENOMEM;
 			goto put_dev;
@@ -495,8 +497,7 @@ static int __init vdpasim_blk_init(void)
 		goto parent_err;
 
 	if (shared_backend) {
-		shared_buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
-					 GFP_KERNEL);
+		shared_buffer = kvzalloc(capacity << SECTOR_SHIFT, GFP_KERNEL);
 		if (!shared_buffer) {
 			ret = -ENOMEM;
 			goto mgmt_dev_err;
-- 
2.45.2

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Michael S. Tsirkin]

On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> filesystems (e.g. XFS) may require larger minimum sizes (see
> https://issues.redhat.com/browse/RHEL-45951).
> 
> So to allow us to test these filesystems, let's add a module parameter
> to control the size of the simulated virtio-blk devices.
> The value is mapped directly to the `capacity` field of the virtio-blk
> configuration space, so it must be expressed in sector numbers of 512
> bytes.
> 
> The default value (0x40000) is the same as the previous value, so the
> behavior without setting `capacity` remains unchanged.
> 
> Before this patch or with this patch without setting `capacity`:
>   $ modprobe vdpa-sim-blk
>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> 
> After this patch:
>   $ modprobe vdpa-sim-blk capacity=614400
>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

What a hack. Cindy was working on adding control over config
space, why can't that be used?

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> index b137f3679343..18f390149836 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> @@ -33,7 +33,6 @@
>  				 (1ULL << VIRTIO_BLK_F_DISCARD)  | \
>  				 (1ULL << VIRTIO_BLK_F_WRITE_ZEROES))
>  
> -#define VDPASIM_BLK_CAPACITY	0x40000
>  #define VDPASIM_BLK_SIZE_MAX	0x1000
>  #define VDPASIM_BLK_SEG_MAX	32
>  #define VDPASIM_BLK_DWZ_MAX_SECTORS UINT_MAX
> @@ -43,6 +42,10 @@
>  #define VDPASIM_BLK_AS_NUM	1
>  #define VDPASIM_BLK_GROUP_NUM	1
>  
> +static unsigned long capacity = 0x40000;
> +module_param(capacity, ulong, 0444);
> +MODULE_PARM_DESC(capacity, "virtio-blk device capacity (in 512-byte sectors)");
> +
>  struct vdpasim_blk {
>  	struct vdpasim vdpasim;
>  	void *buffer;
> @@ -79,10 +82,10 @@ static void vdpasim_blk_buffer_unlock(struct vdpasim_blk *blk)
>  static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
>  				    u64 num_sectors, u64 max_sectors)
>  {
> -	if (start_sector > VDPASIM_BLK_CAPACITY) {
> +	if (start_sector > capacity) {
>  		dev_dbg(&vdpasim->vdpa.dev,
> -			"starting sector exceeds the capacity - start: 0x%llx capacity: 0x%x\n",
> -			start_sector, VDPASIM_BLK_CAPACITY);
> +			"starting sector exceeds the capacity - start: 0x%llx capacity: 0x%lx\n",
> +			start_sector, capacity);
>  	}
>  
>  	if (num_sectors > max_sectors) {
> @@ -92,10 +95,10 @@ static bool vdpasim_blk_check_range(struct vdpasim *vdpasim, u64 start_sector,
>  		return false;
>  	}
>  
> -	if (num_sectors > VDPASIM_BLK_CAPACITY - start_sector) {
> +	if (num_sectors > capacity - start_sector) {
>  		dev_dbg(&vdpasim->vdpa.dev,
> -			"request exceeds the capacity - start: 0x%llx num: 0x%llx capacity: 0x%x\n",
> -			start_sector, num_sectors, VDPASIM_BLK_CAPACITY);
> +			"request exceeds the capacity - start: 0x%llx num: 0x%llx capacity: 0x%lx\n",
> +			start_sector, num_sectors, capacity);
>  		return false;
>  	}
>  
> @@ -369,7 +372,7 @@ static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
>  
>  	memset(config, 0, sizeof(struct virtio_blk_config));
>  
> -	blk_config->capacity = cpu_to_vdpasim64(vdpasim, VDPASIM_BLK_CAPACITY);
> +	blk_config->capacity = cpu_to_vdpasim64(vdpasim, capacity);
>  	blk_config->size_max = cpu_to_vdpasim32(vdpasim, VDPASIM_BLK_SIZE_MAX);
>  	blk_config->seg_max = cpu_to_vdpasim32(vdpasim, VDPASIM_BLK_SEG_MAX);
>  	blk_config->num_queues = cpu_to_vdpasim16(vdpasim, VDPASIM_BLK_VQ_NUM);
> @@ -437,8 +440,7 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
>  	if (blk->shared_backend) {
>  		blk->buffer = shared_buffer;
>  	} else {
> -		blk->buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
> -				       GFP_KERNEL);
> +		blk->buffer = kvzalloc(capacity << SECTOR_SHIFT, GFP_KERNEL);
>  		if (!blk->buffer) {
>  			ret = -ENOMEM;
>  			goto put_dev;
> @@ -495,8 +497,7 @@ static int __init vdpasim_blk_init(void)
>  		goto parent_err;
>  
>  	if (shared_backend) {
> -		shared_buffer = kvzalloc(VDPASIM_BLK_CAPACITY << SECTOR_SHIFT,
> -					 GFP_KERNEL);
> +		shared_buffer = kvzalloc(capacity << SECTOR_SHIFT, GFP_KERNEL);
>  		if (!shared_buffer) {
>  			ret = -ENOMEM;
>  			goto mgmt_dev_err;
> -- 
> 2.45.2

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> filesystems (e.g. XFS) may require larger minimum sizes (see
>> https://issues.redhat.com/browse/RHEL-45951).
>>
>> So to allow us to test these filesystems, let's add a module parameter
>> to control the size of the simulated virtio-blk devices.
>> The value is mapped directly to the `capacity` field of the virtio-blk
>> configuration space, so it must be expressed in sector numbers of 512
>> bytes.
>>
>> The default value (0x40000) is the same as the previous value, so the
>> behavior without setting `capacity` remains unchanged.
>>
>> Before this patch or with this patch without setting `capacity`:
>>   $ modprobe vdpa-sim-blk
>>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>>
>> After this patch:
>>   $ modprobe vdpa-sim-blk capacity=614400
>>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>
>What a hack. Cindy was working on adding control over config
>space, why can't that be used?

If it can be used easily with virtio-blk device too, it will be great.
@Cindy do you plan to support that changes for a virtio-blk device too?

In the mean time, for the simulator I thought that this change was fine.
It's just used for testing and debugging...

My main question is how to use that when we have `shared_backend` set to 
true, since we use that setting to test for example live migration. In 
that case, how do we handle the size of the shared ramdisk between 
devices?

Thanks,
Stefano

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Cindy Lu]

On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
> >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> >> filesystems (e.g. XFS) may require larger minimum sizes (see
> >> https://issues.redhat.com/browse/RHEL-45951).
> >>
> >> So to allow us to test these filesystems, let's add a module parameter
> >> to control the size of the simulated virtio-blk devices.
> >> The value is mapped directly to the `capacity` field of the virtio-blk
> >> configuration space, so it must be expressed in sector numbers of 512
> >> bytes.
> >>
> >> The default value (0x40000) is the same as the previous value, so the
> >> behavior without setting `capacity` remains unchanged.
> >>
> >> Before this patch or with this patch without setting `capacity`:
> >>   $ modprobe vdpa-sim-blk
> >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> >>
> >> After this patch:
> >>   $ modprobe vdpa-sim-blk capacity=614400
> >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> >>
> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >
> >What a hack. Cindy was working on adding control over config
> >space, why can't that be used?
>
> If it can be used easily with virtio-blk device too, it will be great.
> @Cindy do you plan to support that changes for a virtio-blk device too?
>
Hi Stefano
I plan to add support to change the vdpa device's configuration after
it is created. In the first step, I want to use the vdpa tool to add
support for changing the MAC address for the network device. the next
step will also add MTU settings etc
here is the link
https://lore.kernel.org/all/20240708064820.88955-1-lulu@redhat.com/T/#t

in the device part, the device needs to implement its function of
int (*dev_set_attr)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
       const struct vdpa_dev_set_config *config);
the configuration will be passed by struct vdpa_dev_set_config. I'm
not sure if this kind of design is suitable for you? Really thanks and
any comments are welcome
thanks
Cindy


> In the mean time, for the simulator I thought that this change was fine.
> It's just used for testing and debugging...
>
> My main question is how to use that when we have `shared_backend` set to
> true, since we use that setting to test for example live migration. In
> that case, how do we handle the size of the shared ramdisk between
> devices?
>
> Thanks,
> Stefano
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Jason Wang]

On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
> > >> https://issues.redhat.com/browse/RHEL-45951).
> > >>
> > >> So to allow us to test these filesystems, let's add a module parameter
> > >> to control the size of the simulated virtio-blk devices.
> > >> The value is mapped directly to the `capacity` field of the virtio-blk
> > >> configuration space, so it must be expressed in sector numbers of 512
> > >> bytes.
> > >>
> > >> The default value (0x40000) is the same as the previous value, so the
> > >> behavior without setting `capacity` remains unchanged.
> > >>
> > >> Before this patch or with this patch without setting `capacity`:
> > >>   $ modprobe vdpa-sim-blk
> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> > >>
> > >> After this patch:
> > >>   $ modprobe vdpa-sim-blk capacity=614400
> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> > >>
> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > >
> > >What a hack. Cindy was working on adding control over config
> > >space, why can't that be used?
> >
> > If it can be used easily with virtio-blk device too, it will be great.
> > @Cindy do you plan to support that changes for a virtio-blk device too?
> >
> Hi Stefano
> I plan to add support to change the vdpa device's configuration after
> it is created.

I think for Stefano's case, we can just implement it via provisioning
parameters?

Thanks

> In the first step, I want to use the vdpa tool to add
> support for changing the MAC address for the network device. the next
> step will also add MTU settings etc
> here is the link
> https://lore.kernel.org/all/20240708064820.88955-1-lulu@redhat.com/T/#t
>
> in the device part, the device needs to implement its function of
> int (*dev_set_attr)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
>        const struct vdpa_dev_set_config *config);
> the configuration will be passed by struct vdpa_dev_set_config. I'm
> not sure if this kind of design is suitable for you? Really thanks and
> any comments are welcome
> thanks
> Cindy
>
>
> > In the mean time, for the simulator I thought that this change was fine.
> > It's just used for testing and debugging...
> >
> > My main question is how to use that when we have `shared_backend` set to
> > true, since we use that setting to test for example live migration. In
> > that case, how do we handle the size of the shared ramdisk between
> > devices?
> >
> > Thanks,
> > Stefano
> >
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

Hi Cindy, Jason,

On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
>On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
>>
>> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >
>> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
>> > >> https://issues.redhat.com/browse/RHEL-45951).
>> > >>
>> > >> So to allow us to test these filesystems, let's add a module parameter
>> > >> to control the size of the simulated virtio-blk devices.
>> > >> The value is mapped directly to the `capacity` field of the virtio-blk
>> > >> configuration space, so it must be expressed in sector numbers of 512
>> > >> bytes.
>> > >>
>> > >> The default value (0x40000) is the same as the previous value, so the
>> > >> behavior without setting `capacity` remains unchanged.
>> > >>
>> > >> Before this patch or with this patch without setting `capacity`:
>> > >>   $ modprobe vdpa-sim-blk
>> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>> > >>
>> > >> After this patch:
>> > >>   $ modprobe vdpa-sim-blk capacity=614400
>> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>> > >>
>> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> > >
>> > >What a hack. Cindy was working on adding control over config
>> > >space, why can't that be used?
>> >
>> > If it can be used easily with virtio-blk device too, it will be great.
>> > @Cindy do you plan to support that changes for a virtio-blk device too?
>> >
>> Hi Stefano
>> I plan to add support to change the vdpa device's configuration after
>> it is created.
>
>I think for Stefano's case, we can just implement it via provisioning
>parameters?

Yep, I think we don't need to change it after creation, but specifying 
while creating should be enough.

So, IIUC we can already do it, implementing something similar to 
vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?

What about when we have `shared_backend` set to true for the 
vdpa_sim_blk.ko? In this case the backend is supposed to be shared 
between all the devices to test live migration.

Maybe we can just change the size of the shared ramdisk to be reflected 
to all devices.

Suggestions?

@Cindy do you want to work on this for blk as well?
If you don't have time, I'll look at it when I can allocate some time.

>
>Thanks
>
>> In the first step, I want to use the vdpa tool to add
>> support for changing the MAC address for the network device. the next
>> step will also add MTU settings etc
>> here is the link
>> https://lore.kernel.org/all/20240708064820.88955-1-lulu@redhat.com/T/#t
>>

I'll take a look, thanks for ccing me!

Stefano

>> in the device part, the device needs to implement its function of
>> int (*dev_set_attr)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
>>        const struct vdpa_dev_set_config *config);
>> the configuration will be passed by struct vdpa_dev_set_config. I'm
>> not sure if this kind of design is suitable for you? Really thanks and
>> any comments are welcome
>> thanks
>> Cindy
>>
>>
>> > In the mean time, for the simulator I thought that this change was fine.
>> > It's just used for testing and debugging...
>> >
>> > My main question is how to use that when we have `shared_backend` set to
>> > true, since we use that setting to test for example live migration. In
>> > that case, how do we handle the size of the shared ramdisk between
>> > devices?
>> >
>> > Thanks,
>> > Stefano
>> >
>>
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Jason Wang]

On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Hi Cindy, Jason,
>
> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
> >>
> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >
> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
> >> > >> https://issues.redhat.com/browse/RHEL-45951).
> >> > >>
> >> > >> So to allow us to test these filesystems, let's add a module parameter
> >> > >> to control the size of the simulated virtio-blk devices.
> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
> >> > >> configuration space, so it must be expressed in sector numbers of 512
> >> > >> bytes.
> >> > >>
> >> > >> The default value (0x40000) is the same as the previous value, so the
> >> > >> behavior without setting `capacity` remains unchanged.
> >> > >>
> >> > >> Before this patch or with this patch without setting `capacity`:
> >> > >>   $ modprobe vdpa-sim-blk
> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> >> > >>
> >> > >> After this patch:
> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> >> > >>
> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > >
> >> > >What a hack. Cindy was working on adding control over config
> >> > >space, why can't that be used?
> >> >
> >> > If it can be used easily with virtio-blk device too, it will be great.
> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
> >> >
> >> Hi Stefano
> >> I plan to add support to change the vdpa device's configuration after
> >> it is created.
> >
> >I think for Stefano's case, we can just implement it via provisioning
> >parameters?
>
> Yep, I think we don't need to change it after creation, but specifying
> while creating should be enough.
>
> So, IIUC we can already do it, implementing something similar to
> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?

Right.

>
> What about when we have `shared_backend` set to true for the
> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
> between all the devices to test live migration.

This seems to be another topic.

>
> Maybe we can just change the size of the shared ramdisk to be reflected
> to all devices.
>
> Suggestions?

Could we specify the path to tmpfs or others during provisioning
instead?  It seems more general (but more work).

>
> @Cindy do you want to work on this for blk as well?
> If you don't have time, I'll look at it when I can allocate some time.
>
> >
> >Thanks

Thanks

> >
> >> In the first step, I want to use the vdpa tool to add
> >> support for changing the MAC address for the network device. the next
> >> step will also add MTU settings etc
> >> here is the link
> >> https://lore.kernel.org/all/20240708064820.88955-1-lulu@redhat.com/T/#t
> >>
>
> I'll take a look, thanks for ccing me!
>
> Stefano
>
> >> in the device part, the device needs to implement its function of
> >> int (*dev_set_attr)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
> >>        const struct vdpa_dev_set_config *config);
> >> the configuration will be passed by struct vdpa_dev_set_config. I'm
> >> not sure if this kind of design is suitable for you? Really thanks and
> >> any comments are welcome
> >> thanks
> >> Cindy
> >>
> >>
> >> > In the mean time, for the simulator I thought that this change was fine.
> >> > It's just used for testing and debugging...
> >> >
> >> > My main question is how to use that when we have `shared_backend` set to
> >> > true, since we use that setting to test for example live migration. In
> >> > that case, how do we handle the size of the shared ramdisk between
> >> > devices?
> >> >
> >> > Thanks,
> >> > Stefano
> >> >
> >>
> >
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
>On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> Hi Cindy, Jason,
>>
>> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
>> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
>> >>
>> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >
>> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
>> >> > >> https://issues.redhat.com/browse/RHEL-45951).
>> >> > >>
>> >> > >> So to allow us to test these filesystems, let's add a module parameter
>> >> > >> to control the size of the simulated virtio-blk devices.
>> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
>> >> > >> configuration space, so it must be expressed in sector numbers of 512
>> >> > >> bytes.
>> >> > >>
>> >> > >> The default value (0x40000) is the same as the previous value, so the
>> >> > >> behavior without setting `capacity` remains unchanged.
>> >> > >>
>> >> > >> Before this patch or with this patch without setting `capacity`:
>> >> > >>   $ modprobe vdpa-sim-blk
>> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>> >> > >>
>> >> > >> After this patch:
>> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
>> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>> >> > >>
>> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> > >
>> >> > >What a hack. Cindy was working on adding control over config
>> >> > >space, why can't that be used?
>> >> >
>> >> > If it can be used easily with virtio-blk device too, it will be great.
>> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
>> >> >
>> >> Hi Stefano
>> >> I plan to add support to change the vdpa device's configuration after
>> >> it is created.
>> >
>> >I think for Stefano's case, we can just implement it via provisioning
>> >parameters?
>>
>> Yep, I think we don't need to change it after creation, but specifying
>> while creating should be enough.
>>
>> So, IIUC we can already do it, implementing something similar to
>> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
>
>Right.
>
>>
>> What about when we have `shared_backend` set to true for the
>> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
>> between all the devices to test live migration.
>
>This seems to be another topic.

Yep, but really related. I think we need to handle that case when 
supporting the `capacity` setting.

>
>>
>> Maybe we can just change the size of the shared ramdisk to be reflected
>> to all devices.
>>
>> Suggestions?
>
>Could we specify the path to tmpfs or others during provisioning
>instead?  It seems more general (but more work).

Then it would almost become a real device, no longer just a simulator.  
It's enough work, though, as you said, but at that point we'd just have 
to specify the backend file to use for the device.

In that case what API would we need to use to allow the user to set the 
backend file?

Thanks,
Stefano

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Jason Wang]

On Tue, Jul 9, 2024 at 8:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
> >On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> Hi Cindy, Jason,
> >>
> >> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
> >> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
> >> >>
> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >> >
> >> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
> >> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> >> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> >> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
> >> >> > >> https://issues.redhat.com/browse/RHEL-45951).
> >> >> > >>
> >> >> > >> So to allow us to test these filesystems, let's add a module parameter
> >> >> > >> to control the size of the simulated virtio-blk devices.
> >> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
> >> >> > >> configuration space, so it must be expressed in sector numbers of 512
> >> >> > >> bytes.
> >> >> > >>
> >> >> > >> The default value (0x40000) is the same as the previous value, so the
> >> >> > >> behavior without setting `capacity` remains unchanged.
> >> >> > >>
> >> >> > >> Before this patch or with this patch without setting `capacity`:
> >> >> > >>   $ modprobe vdpa-sim-blk
> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> >> >> > >>
> >> >> > >> After this patch:
> >> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> >> >> > >>
> >> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> >> > >
> >> >> > >What a hack. Cindy was working on adding control over config
> >> >> > >space, why can't that be used?
> >> >> >
> >> >> > If it can be used easily with virtio-blk device too, it will be great.
> >> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
> >> >> >
> >> >> Hi Stefano
> >> >> I plan to add support to change the vdpa device's configuration after
> >> >> it is created.
> >> >
> >> >I think for Stefano's case, we can just implement it via provisioning
> >> >parameters?
> >>
> >> Yep, I think we don't need to change it after creation, but specifying
> >> while creating should be enough.
> >>
> >> So, IIUC we can already do it, implementing something similar to
> >> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
> >
> >Right.
> >
> >>
> >> What about when we have `shared_backend` set to true for the
> >> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
> >> between all the devices to test live migration.
> >
> >This seems to be another topic.
>
> Yep, but really related. I think we need to handle that case when
> supporting the `capacity` setting.

Ok, so if I was not wrong, the goal is to test migration.

>
> >
> >>
> >> Maybe we can just change the size of the shared ramdisk to be reflected
> >> to all devices.
> >>
> >> Suggestions?
> >
> >Could we specify the path to tmpfs or others during provisioning
> >instead?  It seems more general (but more work).
>
> Then it would almost become a real device, no longer just a simulator.
> It's enough work, though, as you said, but at that point we'd just have
> to specify the backend file to use for the device.
>
> In that case what API would we need to use to allow the user to set the
> backend file?

Yes, I think we can allow some vendor specific provisioning parameters.

But not sure it's an overkill for the use case here. If others are
happy with the shared_backed. I'm fine.

Thanks

>
> Thanks,
> Stefano
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

On Wed, Jul 10, 2024 at 11:08:48AM GMT, Jason Wang wrote:
>On Tue, Jul 9, 2024 at 8:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
>> >On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> Hi Cindy, Jason,
>> >>
>> >> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
>> >> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
>> >> >>
>> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >> >
>> >> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>> >> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> >> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> >> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
>> >> >> > >> https://issues.redhat.com/browse/RHEL-45951).
>> >> >> > >>
>> >> >> > >> So to allow us to test these filesystems, let's add a module parameter
>> >> >> > >> to control the size of the simulated virtio-blk devices.
>> >> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
>> >> >> > >> configuration space, so it must be expressed in sector numbers of 512
>> >> >> > >> bytes.
>> >> >> > >>
>> >> >> > >> The default value (0x40000) is the same as the previous value, so the
>> >> >> > >> behavior without setting `capacity` remains unchanged.
>> >> >> > >>
>> >> >> > >> Before this patch or with this patch without setting `capacity`:
>> >> >> > >>   $ modprobe vdpa-sim-blk
>> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>> >> >> > >>
>> >> >> > >> After this patch:
>> >> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
>> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>> >> >> > >>
>> >> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> >> > >
>> >> >> > >What a hack. Cindy was working on adding control over config
>> >> >> > >space, why can't that be used?
>> >> >> >
>> >> >> > If it can be used easily with virtio-blk device too, it will be great.
>> >> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
>> >> >> >
>> >> >> Hi Stefano
>> >> >> I plan to add support to change the vdpa device's configuration after
>> >> >> it is created.
>> >> >
>> >> >I think for Stefano's case, we can just implement it via provisioning
>> >> >parameters?
>> >>
>> >> Yep, I think we don't need to change it after creation, but specifying
>> >> while creating should be enough.
>> >>
>> >> So, IIUC we can already do it, implementing something similar to
>> >> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
>> >
>> >Right.
>> >
>> >>
>> >> What about when we have `shared_backend` set to true for the
>> >> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
>> >> between all the devices to test live migration.
>> >
>> >This seems to be another topic.
>>
>> Yep, but really related. I think we need to handle that case when
>> supporting the `capacity` setting.
>
>Ok, so if I was not wrong, the goal is to test migration.

Sorry, I was not clear, I try to rephrase:
vdpa_sim_blk already supports a module parameter called `shared_backend` 
introduced mainly to test live migration on the same host. When that 
parameter is on, all the created devices share the same backend and so 
we can easily do migration from one to another.

With that parameter on or off, the device is always 128 MB, but now 
that's a problem for testing, because it looks like XFS requires at 
least 300 MB: https://issues.redhat.com/browse/RHEL-45951

That's why I sent this patch.

When `shared_backend` is off (default), using the provisioning 
parameters seems feasible to me, but when it's on, how do I deal with 
it?

Being a simulator, we can maybe make it so that only the first device 
can change the size for example, or that all devices control the size, 
but then we would have to handle the size change at runtime, which I 
think is feasible, but it requires some work to send a notification of 
configuration change, etc.

>
>>
>> >
>> >>
>> >> Maybe we can just change the size of the shared ramdisk to be reflected
>> >> to all devices.
>> >>
>> >> Suggestions?
>> >
>> >Could we specify the path to tmpfs or others during provisioning
>> >instead?  It seems more general (but more work).
>>
>> Then it would almost become a real device, no longer just a simulator.
>> It's enough work, though, as you said, but at that point we'd just have
>> to specify the backend file to use for the device.
>>
>> In that case what API would we need to use to allow the user to set the
>> backend file?
>
>Yes, I think we can allow some vendor specific provisioning parameters.
>
>But not sure it's an overkill for the use case here. If others are
>happy with the shared_backed. I'm fine.

Yeah, maybe it's overkill and I don't have much time these days :-(

I think the easiest way is to merge this patch, but I understand that a 
module parameter is not very beautiful.

I'll try to see if I can implement provisioning parameters for 
vdpa_sim_blk. Allowing capacity to be set only to the first device if 
`shared_backend` is on.

WDYT?

Thanks,
Stefano

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Jason Wang]

On Wed, Jul 10, 2024 at 3:19 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 11:08:48AM GMT, Jason Wang wrote:
> >On Tue, Jul 9, 2024 at 8:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >>
> >> On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
> >> >On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >>
> >> >> Hi Cindy, Jason,
> >> >>
> >> >> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
> >> >> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
> >> >> >>
> >> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >> >> >> >
> >> >> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
> >> >> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
> >> >> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
> >> >> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
> >> >> >> > >> https://issues.redhat.com/browse/RHEL-45951).
> >> >> >> > >>
> >> >> >> > >> So to allow us to test these filesystems, let's add a module parameter
> >> >> >> > >> to control the size of the simulated virtio-blk devices.
> >> >> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
> >> >> >> > >> configuration space, so it must be expressed in sector numbers of 512
> >> >> >> > >> bytes.
> >> >> >> > >>
> >> >> >> > >> The default value (0x40000) is the same as the previous value, so the
> >> >> >> > >> behavior without setting `capacity` remains unchanged.
> >> >> >> > >>
> >> >> >> > >> Before this patch or with this patch without setting `capacity`:
> >> >> >> > >>   $ modprobe vdpa-sim-blk
> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> >> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
> >> >> >> > >>
> >> >> >> > >> After this patch:
> >> >> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
> >> >> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
> >> >> >> > >>
> >> >> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> >> >> >> > >
> >> >> >> > >What a hack. Cindy was working on adding control over config
> >> >> >> > >space, why can't that be used?
> >> >> >> >
> >> >> >> > If it can be used easily with virtio-blk device too, it will be great.
> >> >> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
> >> >> >> >
> >> >> >> Hi Stefano
> >> >> >> I plan to add support to change the vdpa device's configuration after
> >> >> >> it is created.
> >> >> >
> >> >> >I think for Stefano's case, we can just implement it via provisioning
> >> >> >parameters?
> >> >>
> >> >> Yep, I think we don't need to change it after creation, but specifying
> >> >> while creating should be enough.
> >> >>
> >> >> So, IIUC we can already do it, implementing something similar to
> >> >> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
> >> >
> >> >Right.
> >> >
> >> >>
> >> >> What about when we have `shared_backend` set to true for the
> >> >> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
> >> >> between all the devices to test live migration.
> >> >
> >> >This seems to be another topic.
> >>
> >> Yep, but really related. I think we need to handle that case when
> >> supporting the `capacity` setting.
> >
> >Ok, so if I was not wrong, the goal is to test migration.
>
> Sorry, I was not clear, I try to rephrase:
> vdpa_sim_blk already supports a module parameter called `shared_backend`
> introduced mainly to test live migration on the same host. When that
> parameter is on, all the created devices share the same backend and so
> we can easily do migration from one to another.
>
> With that parameter on or off, the device is always 128 MB, but now
> that's a problem for testing, because it looks like XFS requires at
> least 300 MB: https://issues.redhat.com/browse/RHEL-45951
>
> That's why I sent this patch.
>
> When `shared_backend` is off (default), using the provisioning
> parameters seems feasible to me, but when it's on, how do I deal with
> it?
>
> Being a simulator, we can maybe make it so that only the first device
> can change the size for example, or that all devices control the size,
> but then we would have to handle the size change at runtime, which I
> think is feasible, but it requires some work to send a notification of
> configuration change, etc.

Can we mandate the size parameter to be exactly the same as the first
vDPA block simulator?

>
> >
> >>
> >> >
> >> >>
> >> >> Maybe we can just change the size of the shared ramdisk to be reflected
> >> >> to all devices.
> >> >>
> >> >> Suggestions?
> >> >
> >> >Could we specify the path to tmpfs or others during provisioning
> >> >instead?  It seems more general (but more work).
> >>
> >> Then it would almost become a real device, no longer just a simulator.
> >> It's enough work, though, as you said, but at that point we'd just have
> >> to specify the backend file to use for the device.
> >>
> >> In that case what API would we need to use to allow the user to set the
> >> backend file?
> >
> >Yes, I think we can allow some vendor specific provisioning parameters.
> >
> >But not sure it's an overkill for the use case here. If others are
> >happy with the shared_backed. I'm fine.
>
> Yeah, maybe it's overkill and I don't have much time these days :-(
>
> I think the easiest way is to merge this patch, but I understand that a
> module parameter is not very beautiful
>
> I'll try to see if I can implement provisioning parameters for
> vdpa_sim_blk. Allowing capacity to be set only to the first device if
> `shared_backend` is on.
>
> WDYT?

Something like this.

When there's no block simulator, allow an arbitrary capacity. When
there is one, fail the creation when the capacity doesn't match. (when
'shared_backend' is on).

Thanks

>
> Thanks,
> Stefano
>

Re: [PATCH] vdpa_sim_blk: add `capacity` module parameter

[Stefano Garzarella]

On Wed, Jul 10, 2024 at 03:28:31PM GMT, Jason Wang wrote:
>On Wed, Jul 10, 2024 at 3:19 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Wed, Jul 10, 2024 at 11:08:48AM GMT, Jason Wang wrote:
>> >On Tue, Jul 9, 2024 at 8:41 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >>
>> >> On Tue, Jul 09, 2024 at 10:56:16AM GMT, Jason Wang wrote:
>> >> >On Mon, Jul 8, 2024 at 4:15 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >>
>> >> >> Hi Cindy, Jason,
>> >> >>
>> >> >> On Mon, Jul 08, 2024 at 03:59:34PM GMT, Jason Wang wrote:
>> >> >> >On Mon, Jul 8, 2024 at 3:06 PM Cindy Lu <lulu@redhat.com> wrote:
>> >> >> >>
>> >> >> >> On Fri, 5 Jul 2024 at 20:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >> >> >> >
>> >> >> >> > On Fri, Jul 05, 2024 at 07:30:51AM GMT, Michael S. Tsirkin wrote:
>> >> >> >> > >On Fri, Jul 05, 2024 at 01:28:21PM +0200, Stefano Garzarella wrote:
>> >> >> >> > >> The vDPA block simulator always allocated a 128 MiB ram-disk, but some
>> >> >> >> > >> filesystems (e.g. XFS) may require larger minimum sizes (see
>> >> >> >> > >> https://issues.redhat.com/browse/RHEL-45951).
>> >> >> >> > >>
>> >> >> >> > >> So to allow us to test these filesystems, let's add a module parameter
>> >> >> >> > >> to control the size of the simulated virtio-blk devices.
>> >> >> >> > >> The value is mapped directly to the `capacity` field of the virtio-blk
>> >> >> >> > >> configuration space, so it must be expressed in sector numbers of 512
>> >> >> >> > >> bytes.
>> >> >> >> > >>
>> >> >> >> > >> The default value (0x40000) is the same as the previous value, so the
>> >> >> >> > >> behavior without setting `capacity` remains unchanged.
>> >> >> >> > >>
>> >> >> >> > >> Before this patch or with this patch without setting `capacity`:
>> >> >> >> > >>   $ modprobe vdpa-sim-blk
>> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> >> > >>   virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
>> >> >> >> > >>
>> >> >> >> > >> After this patch:
>> >> >> >> > >>   $ modprobe vdpa-sim-blk capacity=614400
>> >> >> >> > >>   $ vdpa dev add mgmtdev vdpasim_blk name blk0
>> >> >> >> > >>   virtio_blk virtio6: 1/0/0 default/read/poll queues
>> >> >> >> > >>   virtio_blk virtio6: [vdb] 614400 512-byte logical blocks (315 MB/300 MiB)
>> >> >> >> > >>
>> >> >> >> > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> >> >> > >
>> >> >> >> > >What a hack. Cindy was working on adding control over config
>> >> >> >> > >space, why can't that be used?
>> >> >> >> >
>> >> >> >> > If it can be used easily with virtio-blk device too, it will be great.
>> >> >> >> > @Cindy do you plan to support that changes for a virtio-blk device too?
>> >> >> >> >
>> >> >> >> Hi Stefano
>> >> >> >> I plan to add support to change the vdpa device's configuration after
>> >> >> >> it is created.
>> >> >> >
>> >> >> >I think for Stefano's case, we can just implement it via provisioning
>> >> >> >parameters?
>> >> >>
>> >> >> Yep, I think we don't need to change it after creation, but specifying
>> >> >> while creating should be enough.
>> >> >>
>> >> >> So, IIUC we can already do it, implementing something similar to
>> >> >> vdpasim_net_setup_config() to call during vdpasim_blk_dev_add(), right?
>> >> >
>> >> >Right.
>> >> >
>> >> >>
>> >> >> What about when we have `shared_backend` set to true for the
>> >> >> vdpa_sim_blk.ko? In this case the backend is supposed to be shared
>> >> >> between all the devices to test live migration.
>> >> >
>> >> >This seems to be another topic.
>> >>
>> >> Yep, but really related. I think we need to handle that case when
>> >> supporting the `capacity` setting.
>> >
>> >Ok, so if I was not wrong, the goal is to test migration.
>>
>> Sorry, I was not clear, I try to rephrase:
>> vdpa_sim_blk already supports a module parameter called `shared_backend`
>> introduced mainly to test live migration on the same host. When that
>> parameter is on, all the created devices share the same backend and so
>> we can easily do migration from one to another.
>>
>> With that parameter on or off, the device is always 128 MB, but now
>> that's a problem for testing, because it looks like XFS requires at
>> least 300 MB: https://issues.redhat.com/browse/RHEL-45951
>>
>> That's why I sent this patch.
>>
>> When `shared_backend` is off (default), using the provisioning
>> parameters seems feasible to me, but when it's on, how do I deal with
>> it?
>>
>> Being a simulator, we can maybe make it so that only the first device
>> can change the size for example, or that all devices control the size,
>> but then we would have to handle the size change at runtime, which I
>> think is feasible, but it requires some work to send a notification of
>> configuration change, etc.
>
>Can we mandate the size parameter to be exactly the same as the first
>vDPA block simulator?
>
>>
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Maybe we can just change the size of the shared ramdisk to be reflected
>> >> >> to all devices.
>> >> >>
>> >> >> Suggestions?
>> >> >
>> >> >Could we specify the path to tmpfs or others during provisioning
>> >> >instead?  It seems more general (but more work).
>> >>
>> >> Then it would almost become a real device, no longer just a simulator.
>> >> It's enough work, though, as you said, but at that point we'd just have
>> >> to specify the backend file to use for the device.
>> >>
>> >> In that case what API would we need to use to allow the user to set the
>> >> backend file?
>> >
>> >Yes, I think we can allow some vendor specific provisioning parameters.
>> >
>> >But not sure it's an overkill for the use case here. If others are
>> >happy with the shared_backed. I'm fine.
>>
>> Yeah, maybe it's overkill and I don't have much time these days :-(
>>
>> I think the easiest way is to merge this patch, but I understand that a
>> module parameter is not very beautiful
>>
>> I'll try to see if I can implement provisioning parameters for
>> vdpa_sim_blk. Allowing capacity to be set only to the first device if
>> `shared_backend` is on.
>>
>> WDYT?
>
>Something like this.
>
>When there's no block simulator, allow an arbitrary capacity. When
>there is one, fail the creation when the capacity doesn't match. (when
>'shared_backend' is on).

Yeah, makes sense to me! I'll do it!

Thanks for the help,
Stefano