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

[Pei, Andy]

Hi Paul,

Reply inline.

-----Original Message-----
From: Paul Menzel  
Sent: Friday, November 26, 2021 4:33 PM
To: Pei, Andy 
Cc: [e-mail]; seabios ; Kevin O'Connor ; Gerd Hoffmann 
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 
> 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
>
> Is it *Ding Limin*? Is she/he the author of the patch? If so, you 
> could do
>   git commit --amend --author="Ding Limin <[e-mail]>"
>
> 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
> >
> > 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. */