[SeaBIOS] [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Andy Pei posted 1 patch 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/seabios tags/patchew/1637908699-106980-1-git-send-email-andy.pei@intel.com
src/block.h         |  2 ++
src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
src/hw/virtio-blk.h |  3 +++
3 files changed, 49 insertions(+), 2 deletions(-)

[SeaBIOS] [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Posted by Andy Pei 2 months ago
according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX
and VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.

Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
Signed-off-by: Andy Pei <andy.pei@intel.com>
---
 src/block.h         |  2 ++
 src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 src/hw/virtio-blk.h |  3 +++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/src/block.h b/src/block.h
index c1b8d73..3af2d71 100644
--- a/src/block.h
+++ b/src/block.h
@@ -57,6 +57,8 @@ struct drive_s {
     u8 translation;     // type of translation
     u16 blksize;        // block size
     struct chs_s pchs;  // Physical CHS
+    u32 max_segment_size; //max_segment_size
+    u32 max_segments;   //max_segments
 };
 
 #define DISK_SECTOR_SIZE  512
diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
index 3b19896..f315839 100644
--- a/src/hw/virtio-blk.c
+++ b/src/hw/virtio-blk.c
@@ -121,12 +121,16 @@ init_virtio_blk(void *data)
         u64 version1 = 1ull << VIRTIO_F_VERSION_1;
         u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
         u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
+        u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
+        u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
+
         if (!(features & version1)) {
             dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
             goto fail;
         }
 
-        features = features & (version1 | iommu_platform | blk_size);
+        features = features & (version1 | iommu_platform | blk_size
+                        | max_segments | max_segment_size);
         vp_set_features(vp, features);
         status |= VIRTIO_CONFIG_S_FEATURES_OK;
         vp_set_status(vp, status);
@@ -135,6 +139,18 @@ init_virtio_blk(void *data)
             goto fail;
         }
 
+        if (features & max_segment_size)
+            vdrive->drive.max_segment_size =
+                vp_read(&vp->device, struct virtio_blk_config, size_max);
+        else
+            vdrive->drive.max_segment_size = 0;
+
+        if (features & max_segments)
+            vdrive->drive.max_segments =
+                vp_read(&vp->device, struct virtio_blk_config, seg_max);
+        else
+            vdrive->drive.max_segments = 0;
+
         vdrive->drive.sectors =
             vp_read(&vp->device, struct virtio_blk_config, capacity);
         if (features & blk_size) {
@@ -177,6 +193,17 @@ init_virtio_blk(void *data)
         vdrive->drive.pchs.cylinder = cfg.cylinders;
         vdrive->drive.pchs.head = cfg.heads;
         vdrive->drive.pchs.sector = cfg.sectors;
+
+        if (f & (1 << VIRTIO_BLK_F_SIZE_MAX))
+            vdrive->drive.max_segment_size = cfg.size_max;
+        else
+            vdrive->drive.max_segment_size = 0;
+
+        if (f & (1 << VIRTIO_BLK_F_SEG_MAX))
+            vdrive->drive.max_segments = cfg.seg_max;
+        else
+            vdrive->drive.max_segments = 0;
+
     }
 
     char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", pci);
@@ -218,8 +245,11 @@ init_virtio_blk_mmio(void *mmio)
     u64 features = vp_get_features(vp);
     u64 version1 = 1ull << VIRTIO_F_VERSION_1;
     u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
+    u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
+    u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
 
-    features = features & (version1 | blk_size);
+    features = features & (version1 | blk_size
+            | max_segments | max_segment_size);
     vp_set_features(vp, features);
     status |= VIRTIO_CONFIG_S_FEATURES_OK;
     vp_set_status(vp, status);
@@ -228,6 +258,18 @@ init_virtio_blk_mmio(void *mmio)
         goto fail;
     }
 
+    if (features & max_segment_size)
+        vdrive->drive.max_segment_size =
+            vp_read(&vp->device, struct virtio_blk_config, size_max);
+    else
+        vdrive->drive.max_segment_size = 0;
+
+    if (features & max_segments)
+        vdrive->drive.max_segments =
+            vp_read(&vp->device, struct virtio_blk_config, seg_max);
+    else
+        vdrive->drive.max_segments = 0;
+
     vdrive->drive.sectors =
         vp_read(&vp->device, struct virtio_blk_config, capacity);
     if (features & blk_size) {
diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h
index d20461a..0294291 100644
--- a/src/hw/virtio-blk.h
+++ b/src/hw/virtio-blk.h
@@ -16,6 +16,9 @@ struct virtio_blk_config
     u32 opt_io_size;
 } __attribute__((packed));
 
+/* Feature bits */
+#define VIRTIO_BLK_F_SIZE_MAX 1  /* Maximum size of any single segment */
+#define VIRTIO_BLK_F_SEG_MAX 2   /* Maximum number of segments in a request */
 #define VIRTIO_BLK_F_BLK_SIZE 6
 
 /* These two define direction. */
-- 
1.8.3.1

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Posted by Paul Menzel 2 months ago
Dear Andy,


Thank you for the patch.

Am 26.11.21 um 07:38 schrieb Andy Pei:
> according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX
> and VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.

How can it be tested, that the change is working?

> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>

Is it *Ding Limin*? Is she/he the author of the patch? If so, you could do

     git commit --amend --author="Ding Limin 
<dinglimin@cmss.chinamobile.com>"

and `git format-patch` should create the patch accordingly.

> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>   src/block.h         |  2 ++
>   src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>   src/hw/virtio-blk.h |  3 +++
>   3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/src/block.h b/src/block.h
> index c1b8d73..3af2d71 100644
> --- a/src/block.h
> +++ b/src/block.h
> @@ -57,6 +57,8 @@ struct drive_s {
>       u8 translation;     // type of translation
>       u16 blksize;        // block size
>       struct chs_s pchs;  // Physical CHS
> +    u32 max_segment_size; //max_segment_size
> +    u32 max_segments;   //max_segments
>   };
>   
>   #define DISK_SECTOR_SIZE  512
> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c
> index 3b19896..f315839 100644
> --- a/src/hw/virtio-blk.c
> +++ b/src/hw/virtio-blk.c
> @@ -121,12 +121,16 @@ init_virtio_blk(void *data)
>           u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>           u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>           u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> +        u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
> +        u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
> +
>           if (!(features & version1)) {
>               dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
>               goto fail;
>           }
>   
> -        features = features & (version1 | iommu_platform | blk_size);
> +        features = features & (version1 | iommu_platform | blk_size
> +                        | max_segments | max_segment_size);
>           vp_set_features(vp, features);
>           status |= VIRTIO_CONFIG_S_FEATURES_OK;
>           vp_set_status(vp, status);
> @@ -135,6 +139,18 @@ init_virtio_blk(void *data)
>               goto fail;
>           }
>   
> +        if (features & max_segment_size)
> +            vdrive->drive.max_segment_size =
> +                vp_read(&vp->device, struct virtio_blk_config, size_max);
> +        else
> +            vdrive->drive.max_segment_size = 0;
> +
> +        if (features & max_segments)
> +            vdrive->drive.max_segments =
> +                vp_read(&vp->device, struct virtio_blk_config, seg_max);
> +        else
> +            vdrive->drive.max_segments = 0;
> +

Maybe print a level 3 message with the values as it’s done for 
`drive.sectors` and `drive.blksize`?


Kind regards,

Paul


>           vdrive->drive.sectors =
>               vp_read(&vp->device, struct virtio_blk_config, capacity);
>           if (features & blk_size) {
> @@ -177,6 +193,17 @@ init_virtio_blk(void *data)
>           vdrive->drive.pchs.cylinder = cfg.cylinders;
>           vdrive->drive.pchs.head = cfg.heads;
>           vdrive->drive.pchs.sector = cfg.sectors;
> +
> +        if (f & (1 << VIRTIO_BLK_F_SIZE_MAX))
> +            vdrive->drive.max_segment_size = cfg.size_max;
> +        else
> +            vdrive->drive.max_segment_size = 0;
> +
> +        if (f & (1 << VIRTIO_BLK_F_SEG_MAX))
> +            vdrive->drive.max_segments = cfg.seg_max;
> +        else
> +            vdrive->drive.max_segments = 0;
> +
>       }
>   
>       char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", pci);
> @@ -218,8 +245,11 @@ init_virtio_blk_mmio(void *mmio)
>       u64 features = vp_get_features(vp);
>       u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>       u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> +    u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
> +    u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>   
> -    features = features & (version1 | blk_size);
> +    features = features & (version1 | blk_size
> +            | max_segments | max_segment_size);
>       vp_set_features(vp, features);
>       status |= VIRTIO_CONFIG_S_FEATURES_OK;
>       vp_set_status(vp, status);
> @@ -228,6 +258,18 @@ init_virtio_blk_mmio(void *mmio)
>           goto fail;
>       }
>   
> +    if (features & max_segment_size)
> +        vdrive->drive.max_segment_size =
> +            vp_read(&vp->device, struct virtio_blk_config, size_max);
> +    else
> +        vdrive->drive.max_segment_size = 0;
> +
> +    if (features & max_segments)
> +        vdrive->drive.max_segments =
> +            vp_read(&vp->device, struct virtio_blk_config, seg_max);
> +    else
> +        vdrive->drive.max_segments = 0;
> +
>       vdrive->drive.sectors =
>           vp_read(&vp->device, struct virtio_blk_config, capacity);
>       if (features & blk_size) {
> diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h
> index d20461a..0294291 100644
> --- a/src/hw/virtio-blk.h
> +++ b/src/hw/virtio-blk.h
> @@ -16,6 +16,9 @@ struct virtio_blk_config
>       u32 opt_io_size;
>   } __attribute__((packed));
>   
> +/* Feature bits */
> +#define VIRTIO_BLK_F_SIZE_MAX 1  /* Maximum size of any single segment */
> +#define VIRTIO_BLK_F_SEG_MAX 2   /* Maximum number of segments in a request */
>   #define VIRTIO_BLK_F_BLK_SIZE 6
>   
>   /* These two define direction. */
> 
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Posted by Pei, Andy 2 months ago
Hi Paul,

Thanks for your response.
Reply inline.

-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de> 
Sent: Friday, November 26, 2021 4:08 PM
To: Pei, Andy <andy.pei@intel.com>
Cc: dinglimin@cmss.chinamobile.com; seabios@seabios.org; Kevin O'Connor <kevin@koconnor.net>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [SeaBIOS] [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Dear Andy,


Thank you for the patch.

Am 26.11.21 um 07:38 schrieb Andy Pei:
> according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX and 
> VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.

How can it be tested, that the change is working?
Yes. Dinglimin tested in their environment.

> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>

Is it *Ding Limin*? Is she/he the author of the patch? If so, you could do

     git commit --amend --author="Ding Limin <dinglimin@cmss.chinamobile.com>"

and `git format-patch` should create the patch accordingly.

We co-work on this patch.

> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
>   src/block.h         |  2 ++
>   src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>   src/hw/virtio-blk.h |  3 +++
>   3 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/src/block.h b/src/block.h index c1b8d73..3af2d71 100644
> --- a/src/block.h
> +++ b/src/block.h
> @@ -57,6 +57,8 @@ struct drive_s {
>       u8 translation;     // type of translation
>       u16 blksize;        // block size
>       struct chs_s pchs;  // Physical CHS
> +    u32 max_segment_size; //max_segment_size
> +    u32 max_segments;   //max_segments
>   };
>   
>   #define DISK_SECTOR_SIZE  512
> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 
> 3b19896..f315839 100644
> --- a/src/hw/virtio-blk.c
> +++ b/src/hw/virtio-blk.c
> @@ -121,12 +121,16 @@ init_virtio_blk(void *data)
>           u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>           u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>           u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> +        u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
> +        u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
> +
>           if (!(features & version1)) {
>               dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
>               goto fail;
>           }
>   
> -        features = features & (version1 | iommu_platform | blk_size);
> +        features = features & (version1 | iommu_platform | blk_size
> +                        | max_segments | max_segment_size);
>           vp_set_features(vp, features);
>           status |= VIRTIO_CONFIG_S_FEATURES_OK;
>           vp_set_status(vp, status);
> @@ -135,6 +139,18 @@ init_virtio_blk(void *data)
>               goto fail;
>           }
>   
> +        if (features & max_segment_size)
> +            vdrive->drive.max_segment_size =
> +                vp_read(&vp->device, struct virtio_blk_config, size_max);
> +        else
> +            vdrive->drive.max_segment_size = 0;
> +
> +        if (features & max_segments)
> +            vdrive->drive.max_segments =
> +                vp_read(&vp->device, struct virtio_blk_config, seg_max);
> +        else
> +            vdrive->drive.max_segments = 0;
> +

Maybe print a level 3 message with the values as it’s done for `drive.sectors` and `drive.blksize`?
I think I can sent a V2 patch, with your suggest log output.

Kind regards,

Paul


>           vdrive->drive.sectors =
>               vp_read(&vp->device, struct virtio_blk_config, capacity);
>           if (features & blk_size) {
> @@ -177,6 +193,17 @@ init_virtio_blk(void *data)
>           vdrive->drive.pchs.cylinder = cfg.cylinders;
>           vdrive->drive.pchs.head = cfg.heads;
>           vdrive->drive.pchs.sector = cfg.sectors;
> +
> +        if (f & (1 << VIRTIO_BLK_F_SIZE_MAX))
> +            vdrive->drive.max_segment_size = cfg.size_max;
> +        else
> +            vdrive->drive.max_segment_size = 0;
> +
> +        if (f & (1 << VIRTIO_BLK_F_SEG_MAX))
> +            vdrive->drive.max_segments = cfg.seg_max;
> +        else
> +            vdrive->drive.max_segments = 0;
> +
>       }
>   
>       char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", pci);
> @@ -218,8 +245,11 @@ init_virtio_blk_mmio(void *mmio)
>       u64 features = vp_get_features(vp);
>       u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>       u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
> +    u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
> +    u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>   
> -    features = features & (version1 | blk_size);
> +    features = features & (version1 | blk_size
> +            | max_segments | max_segment_size);
>       vp_set_features(vp, features);
>       status |= VIRTIO_CONFIG_S_FEATURES_OK;
>       vp_set_status(vp, status);
> @@ -228,6 +258,18 @@ init_virtio_blk_mmio(void *mmio)
>           goto fail;
>       }
>   
> +    if (features & max_segment_size)
> +        vdrive->drive.max_segment_size =
> +            vp_read(&vp->device, struct virtio_blk_config, size_max);
> +    else
> +        vdrive->drive.max_segment_size = 0;
> +
> +    if (features & max_segments)
> +        vdrive->drive.max_segments =
> +            vp_read(&vp->device, struct virtio_blk_config, seg_max);
> +    else
> +        vdrive->drive.max_segments = 0;
> +
>       vdrive->drive.sectors =
>           vp_read(&vp->device, struct virtio_blk_config, capacity);
>       if (features & blk_size) {
> diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h
> index d20461a..0294291 100644
> --- a/src/hw/virtio-blk.h
> +++ b/src/hw/virtio-blk.h
> @@ -16,6 +16,9 @@ struct virtio_blk_config
>       u32 opt_io_size;
>   } __attribute__((packed));
>   
> +/* Feature bits */
> +#define VIRTIO_BLK_F_SIZE_MAX 1  /* Maximum size of any single segment */
> +#define VIRTIO_BLK_F_SEG_MAX 2   /* Maximum number of segments in a request */
>   #define VIRTIO_BLK_F_BLK_SIZE 6
>   
>   /* These two define direction. */
> 
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Posted by Paul Menzel 2 months ago
Dear Andy,


Am 26.11.21 um 09:15 schrieb Pei, Andy:

[…]

> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Friday, November 26, 2021 4:08 PM
[…]
> Subject: Re: [SeaBIOS] [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

> Am 26.11.21 um 07:38 schrieb Andy Pei:
>> according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX and
>> VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.
> 
> How can it be tested, that the change is working?
> Yes. Dinglimin tested in their environment.

[Maybe mark/tag your response somehow, or use a mailer supporting to 
quote the reply correctly. ;-)]

But how was the test exactly done?

>> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> 
> Is it *Ding Limin*? Is she/he the author of the patch? If so, you could do
> 
>       git commit --amend --author="Ding Limin <dinglimin@cmss.chinamobile.com>"
> 
> and `git format-patch` should create the patch accordingly.
> 
> We co-work on this patch.

Good to know. If the name is really one word, please spell it with a 
capital letter Dinglimin.

>> Signed-off-by: Andy Pei <andy.pei@intel.com>
>> ---
>>    src/block.h         |  2 ++
>>    src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>    src/hw/virtio-blk.h |  3 +++
>>    3 files changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/block.h b/src/block.h index c1b8d73..3af2d71 100644
>> --- a/src/block.h
>> +++ b/src/block.h
>> @@ -57,6 +57,8 @@ struct drive_s {
>>        u8 translation;     // type of translation
>>        u16 blksize;        // block size
>>        struct chs_s pchs;  // Physical CHS
>> +    u32 max_segment_size; //max_segment_size
>> +    u32 max_segments;   //max_segments
>>    };
>>    
>>    #define DISK_SECTOR_SIZE  512
>> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index
>> 3b19896..f315839 100644
>> --- a/src/hw/virtio-blk.c
>> +++ b/src/hw/virtio-blk.c
>> @@ -121,12 +121,16 @@ init_virtio_blk(void *data)
>>            u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>>            u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>>            u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
>> +        u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
>> +        u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>> +
>>            if (!(features & version1)) {
>>                dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
>>                goto fail;
>>            }
>>    
>> -        features = features & (version1 | iommu_platform | blk_size);
>> +        features = features & (version1 | iommu_platform | blk_size
>> +                        | max_segments | max_segment_size);
>>            vp_set_features(vp, features);
>>            status |= VIRTIO_CONFIG_S_FEATURES_OK;
>>            vp_set_status(vp, status);
>> @@ -135,6 +139,18 @@ init_virtio_blk(void *data)
>>                goto fail;
>>            }
>>    
>> +        if (features & max_segment_size)
>> +            vdrive->drive.max_segment_size =
>> +                vp_read(&vp->device, struct virtio_blk_config, size_max);
>> +        else
>> +            vdrive->drive.max_segment_size = 0;
>> +
>> +        if (features & max_segments)
>> +            vdrive->drive.max_segments =
>> +                vp_read(&vp->device, struct virtio_blk_config, seg_max);
>> +        else
>> +            vdrive->drive.max_segments = 0;
>> +
> 
> Maybe print a level 3 message with the values as it’s done for `drive.sectors` and `drive.blksize`?
> I think I can sent a V2 patch, with your suggest log output.

Awesome.


Kind regards,

Paul


>>            vdrive->drive.sectors =
>>                vp_read(&vp->device, struct virtio_blk_config, capacity);
>>            if (features & blk_size) {
>> @@ -177,6 +193,17 @@ init_virtio_blk(void *data)
>>            vdrive->drive.pchs.cylinder = cfg.cylinders;
>>            vdrive->drive.pchs.head = cfg.heads;
>>            vdrive->drive.pchs.sector = cfg.sectors;
>> +
>> +        if (f & (1 << VIRTIO_BLK_F_SIZE_MAX))
>> +            vdrive->drive.max_segment_size = cfg.size_max;
>> +        else
>> +            vdrive->drive.max_segment_size = 0;
>> +
>> +        if (f & (1 << VIRTIO_BLK_F_SEG_MAX))
>> +            vdrive->drive.max_segments = cfg.seg_max;
>> +        else
>> +            vdrive->drive.max_segments = 0;
>> +
>>        }
>>    
>>        char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", pci);
>> @@ -218,8 +245,11 @@ init_virtio_blk_mmio(void *mmio)
>>        u64 features = vp_get_features(vp);
>>        u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>>        u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
>> +    u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
>> +    u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>>    
>> -    features = features & (version1 | blk_size);
>> +    features = features & (version1 | blk_size
>> +            | max_segments | max_segment_size);
>>        vp_set_features(vp, features);
>>        status |= VIRTIO_CONFIG_S_FEATURES_OK;
>>        vp_set_status(vp, status);
>> @@ -228,6 +258,18 @@ init_virtio_blk_mmio(void *mmio)
>>            goto fail;
>>        }
>>    
>> +    if (features & max_segment_size)
>> +        vdrive->drive.max_segment_size =
>> +            vp_read(&vp->device, struct virtio_blk_config, size_max);
>> +    else
>> +        vdrive->drive.max_segment_size = 0;
>> +
>> +    if (features & max_segments)
>> +        vdrive->drive.max_segments =
>> +            vp_read(&vp->device, struct virtio_blk_config, seg_max);
>> +    else
>> +        vdrive->drive.max_segments = 0;
>> +
>>        vdrive->drive.sectors =
>>            vp_read(&vp->device, struct virtio_blk_config, capacity);
>>        if (features & blk_size) {
>> diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h
>> index d20461a..0294291 100644
>> --- a/src/hw/virtio-blk.h
>> +++ b/src/hw/virtio-blk.h
>> @@ -16,6 +16,9 @@ struct virtio_blk_config
>>        u32 opt_io_size;
>>    } __attribute__((packed));
>>    
>> +/* Feature bits */
>> +#define VIRTIO_BLK_F_SIZE_MAX 1  /* Maximum size of any single segment */
>> +#define VIRTIO_BLK_F_SEG_MAX 2   /* Maximum number of segments in a request */
>>    #define VIRTIO_BLK_F_BLK_SIZE 6
>>    
>>    /* These two define direction. */
>>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Posted by Pei, Andy 2 months ago
Hi Paul,

Reply inline.

-----Original Message-----
From: Paul Menzel <pmenzel@molgen.mpg.de> 
Sent: Friday, November 26, 2021 4:33 PM
To: Pei, Andy <andy.pei@intel.com>
Cc: dinglimin@cmss.chinamobile.com; seabios <seabios@seabios.org>; Kevin O'Connor <kevin@koconnor.net>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [SeaBIOS] [PATCH] src/hw/virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

Dear Andy,


Am 26.11.21 um 09:15 schrieb Pei, Andy:

[…]

> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Friday, November 26, 2021 4:08 PM
[…]
> Subject: Re: [SeaBIOS] [PATCH] src/hw/virtio-blk: add feature 
> VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX

> Am 26.11.21 um 07:38 schrieb Andy Pei:
>> according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX and 
>> VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.
> 
> How can it be tested, that the change is working?
> Yes. Dinglimin tested in their environment.

[Maybe mark/tag your response somehow, or use a mailer supporting to quote the reply correctly. ;-)]

But how was the test exactly done?

Andy:
This patch follow the VIRTIO SPEC to read virtio blk cfg register.
There will be a another patch, which will make some modification in seabois virtio blk driver.
I think there will be some discussion on that patch.
This patch just read register following the spec, which should cause no doubt.

>> Signed-off-by: dinglimin <dinglimin@cmss.chinamobile.com>
> 
> Is it *Ding Limin*? Is she/he the author of the patch? If so, you 
> could do
> 
>       git commit --amend --author="Ding Limin <dinglimin@cmss.chinamobile.com>"
> 
> and `git format-patch` should create the patch accordingly.
> 
> We co-work on this patch.

Good to know. If the name is really one word, please spell it with a capital letter Dinglimin.

Andy:
OK, thanks for your suggestion.

>> Signed-off-by: Andy Pei <andy.pei@intel.com>
>> ---
>>    src/block.h         |  2 ++
>>    src/hw/virtio-blk.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>    src/hw/virtio-blk.h |  3 +++
>>    3 files changed, 49 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/block.h b/src/block.h index c1b8d73..3af2d71 100644
>> --- a/src/block.h
>> +++ b/src/block.h
>> @@ -57,6 +57,8 @@ struct drive_s {
>>        u8 translation;     // type of translation
>>        u16 blksize;        // block size
>>        struct chs_s pchs;  // Physical CHS
>> +    u32 max_segment_size; //max_segment_size
>> +    u32 max_segments;   //max_segments
>>    };
>>    
>>    #define DISK_SECTOR_SIZE  512
>> diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index
>> 3b19896..f315839 100644
>> --- a/src/hw/virtio-blk.c
>> +++ b/src/hw/virtio-blk.c
>> @@ -121,12 +121,16 @@ init_virtio_blk(void *data)
>>            u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>>            u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM;
>>            u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
>> +        u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
>> +        u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>> +
>>            if (!(features & version1)) {
>>                dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci);
>>                goto fail;
>>            }
>>    
>> -        features = features & (version1 | iommu_platform | blk_size);
>> +        features = features & (version1 | iommu_platform | blk_size
>> +                        | max_segments | max_segment_size);
>>            vp_set_features(vp, features);
>>            status |= VIRTIO_CONFIG_S_FEATURES_OK;
>>            vp_set_status(vp, status); @@ -135,6 +139,18 @@ 
>> init_virtio_blk(void *data)
>>                goto fail;
>>            }
>>    
>> +        if (features & max_segment_size)
>> +            vdrive->drive.max_segment_size =
>> +                vp_read(&vp->device, struct virtio_blk_config, size_max);
>> +        else
>> +            vdrive->drive.max_segment_size = 0;
>> +
>> +        if (features & max_segments)
>> +            vdrive->drive.max_segments =
>> +                vp_read(&vp->device, struct virtio_blk_config, seg_max);
>> +        else
>> +            vdrive->drive.max_segments = 0;
>> +
> 
> Maybe print a level 3 message with the values as it’s done for `drive.sectors` and `drive.blksize`?
> I think I can sent a V2 patch, with your suggest log output.

Awesome.

Andy:
Thanks.


Kind regards,

Paul


>>            vdrive->drive.sectors =
>>                vp_read(&vp->device, struct virtio_blk_config, capacity);
>>            if (features & blk_size) { @@ -177,6 +193,17 @@ 
>> init_virtio_blk(void *data)
>>            vdrive->drive.pchs.cylinder = cfg.cylinders;
>>            vdrive->drive.pchs.head = cfg.heads;
>>            vdrive->drive.pchs.sector = cfg.sectors;
>> +
>> +        if (f & (1 << VIRTIO_BLK_F_SIZE_MAX))
>> +            vdrive->drive.max_segment_size = cfg.size_max;
>> +        else
>> +            vdrive->drive.max_segment_size = 0;
>> +
>> +        if (f & (1 << VIRTIO_BLK_F_SEG_MAX))
>> +            vdrive->drive.max_segments = cfg.seg_max;
>> +        else
>> +            vdrive->drive.max_segments = 0;
>> +
>>        }
>>    
>>        char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", 
>> pci); @@ -218,8 +245,11 @@ init_virtio_blk_mmio(void *mmio)
>>        u64 features = vp_get_features(vp);
>>        u64 version1 = 1ull << VIRTIO_F_VERSION_1;
>>        u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE;
>> +    u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX;
>> +    u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
>>    
>> -    features = features & (version1 | blk_size);
>> +    features = features & (version1 | blk_size
>> +            | max_segments | max_segment_size);
>>        vp_set_features(vp, features);
>>        status |= VIRTIO_CONFIG_S_FEATURES_OK;
>>        vp_set_status(vp, status);
>> @@ -228,6 +258,18 @@ init_virtio_blk_mmio(void *mmio)
>>            goto fail;
>>        }
>>    
>> +    if (features & max_segment_size)
>> +        vdrive->drive.max_segment_size =
>> +            vp_read(&vp->device, struct virtio_blk_config, size_max);
>> +    else
>> +        vdrive->drive.max_segment_size = 0;
>> +
>> +    if (features & max_segments)
>> +        vdrive->drive.max_segments =
>> +            vp_read(&vp->device, struct virtio_blk_config, seg_max);
>> +    else
>> +        vdrive->drive.max_segments = 0;
>> +
>>        vdrive->drive.sectors =
>>            vp_read(&vp->device, struct virtio_blk_config, capacity);
>>        if (features & blk_size) {
>> diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h index 
>> d20461a..0294291 100644
>> --- a/src/hw/virtio-blk.h
>> +++ b/src/hw/virtio-blk.h
>> @@ -16,6 +16,9 @@ struct virtio_blk_config
>>        u32 opt_io_size;
>>    } __attribute__((packed));
>>    
>> +/* Feature bits */
>> +#define VIRTIO_BLK_F_SIZE_MAX 1  /* Maximum size of any single segment */
>> +#define VIRTIO_BLK_F_SEG_MAX 2   /* Maximum number of segments in a request */
>>    #define VIRTIO_BLK_F_BLK_SIZE 6
>>    
>>    /* These two define direction. */
>>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org