[PATCH 0/5] block/vvfat: introduce "size" option

[PATCH 0/5] block/vvfat: introduce "size" option

[Clément Chigot]

The main goal of this series is to introduce a new option "size" within
the vvfat backend (patch 5). It allows more control over SD cards' size.
The value for "Number of Heads" and "Sectors per track" are based on SD
specifications Part 2.

This series also includes minor patches:
 - patch 1 introduces another option to remove the Master Boot Record
   (this is mandatory for QNX)
 - patch 2-4 are minor improvements easing the introducing of "size"
   option

This was tested on with a aarch64-linux kernel taken from
functional/aarch64/test-virt and on aarch64-qnx over raspi4b with a
workaround, not included here (the SD bus must be associated to the EMMC2
port instead of through GPIOs).

Clément Chigot (5):
  vvfat: introduce no-mbr option
  vvfat: move fat_type check prior to size setup
  vvfat: add a define for SECTOR_SIZE
  vvfat: move size parameters within driver structure
  vvfat: add support for "size" options

 block/vvfat.c | 279 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 213 insertions(+), 66 deletions(-)

-- 
2.34.1

[PATCH 1/5] vvfat: introduce no-mbr option

[Clément Chigot]

This option when set prevents a master boot record (MBR) to be
initialized. This is mandatory as some operating system don't recognized
mounted disks if a MBR is present.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 814796d918..0220dd828b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1082,6 +1082,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Make the image writable",
         },
+        {
+            .name = "no-mbr",
+            .type = QEMU_OPT_BOOL,
+            .help = "Do not add a Master Boot Record on this disk",
+        },
         { /* end of list */ }
     },
 };
@@ -1092,6 +1097,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     int fat_type = 0;
     bool floppy = false;
     bool rw = false;
+    bool no_mbr = false;
     int i;
 
     if (!strstart(filename, "fat:", NULL)) {
@@ -1116,6 +1122,10 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
         rw = true;
     }
 
+    if (strstr(filename, ":no-mbr:")) {
+        no_mbr = true;
+    }
+
     /* Get the directory name without options */
     i = strrchr(filename, ':') - filename;
     assert(i >= 3);
@@ -1131,6 +1141,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put_int(options, "fat-type", fat_type);
     qdict_put_bool(options, "floppy", floppy);
     qdict_put_bool(options, "rw", rw);
+    qdict_put_bool(options, "no-mbr", no_mbr);
 }
 
 static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
@@ -1196,7 +1207,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         if (!s->fat_type) {
             s->fat_type = 16;
         }
-        s->offset_to_bootsector = 0x3f;
+        /* Reserver space for MBR */
+        if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
+            s->offset_to_bootsector = 0x3f;
+        }
         cyls = s->fat_type == 12 ? 64 : 1024;
         heads = 16;
         secs = 63;
-- 
2.34.1

Re: [PATCH 1/5] vvfat: introduce no-mbr option

[Kevin Wolf]

Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> This option when set prevents a master boot record (MBR) to be
> initialized. This is mandatory as some operating system don't recognized
> mounted disks if a MBR is present.
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>

Can we actually give an example of such an OS in the commit message?

> ---
>  block/vvfat.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 814796d918..0220dd828b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1082,6 +1082,11 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "Make the image writable",
>          },
> +        {
> +            .name = "no-mbr",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Do not add a Master Boot Record on this disk",
> +        },

Let's keep option names positive to avoid double negations like
'no-mbr=false'. We can have an 'mbr' option that defaults to true. Or in
fact, maybe calling it 'partitioned' would be easier to understand.

You need to update BlockdevOptionsVVFAT in qapi/block-core.json, too, to
make the new option work with -blockdev. You should update the
description for @floppy there, too, because it says that hard disks are
always partitioned.

It should also be added to vvfat_strong_runtime_opts because the value
of this option changes the data that the guest sees.

>          { /* end of list */ }
>      },
>  };
> @@ -1092,6 +1097,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
>      int fat_type = 0;
>      bool floppy = false;
>      bool rw = false;
> +    bool no_mbr = false;
>      int i;
>  
>      if (!strstart(filename, "fat:", NULL)) {
> @@ -1116,6 +1122,10 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
>          rw = true;
>      }
>  
> +    if (strstr(filename, ":no-mbr:")) {

In the string, the negative form can stay (because the positive one
doesn't exist here).

> +        no_mbr = true;
> +    }
> +
>      /* Get the directory name without options */
>      i = strrchr(filename, ':') - filename;
>      assert(i >= 3);
> @@ -1131,6 +1141,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
>      qdict_put_int(options, "fat-type", fat_type);
>      qdict_put_bool(options, "floppy", floppy);
>      qdict_put_bool(options, "rw", rw);
> +    qdict_put_bool(options, "no-mbr", no_mbr);
>  }
>  
>  static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> @@ -1196,7 +1207,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          if (!s->fat_type) {
>              s->fat_type = 16;
>          }
> -        s->offset_to_bootsector = 0x3f;
> +        /* Reserver space for MBR */
> +        if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
> +            s->offset_to_bootsector = 0x3f;
> +        }
>          cyls = s->fat_type == 12 ? 64 : 1024;
>          heads = 16;
>          secs = 63;

Kevin

Re: [PATCH 1/5] vvfat: introduce no-mbr option

[Clément Chigot]

On Thu, Oct 23, 2025 at 8:21 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > This option when set prevents a master boot record (MBR) to be
> > initialized. This is mandatory as some operating system don't recognized
> > mounted disks if a MBR is present.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> Can we actually give an example of such an OS in the commit message?
>
> > ---
> >  block/vvfat.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 814796d918..0220dd828b 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -1082,6 +1082,11 @@ static QemuOptsList runtime_opts = {
> >              .type = QEMU_OPT_BOOL,
> >              .help = "Make the image writable",
> >          },
> > +        {
> > +            .name = "no-mbr",
> > +            .type = QEMU_OPT_BOOL,
> > +            .help = "Do not add a Master Boot Record on this disk",
> > +        },
>
> Let's keep option names positive to avoid double negations like
> 'no-mbr=false'. We can have an 'mbr' option that defaults to true. Or in
> fact, maybe calling it 'partitioned' would be easier to understand.
>
> You need to update BlockdevOptionsVVFAT in qapi/block-core.json, too, to
> make the new option work with -blockdev. You should update the
> description for @floppy there, too, because it says that hard disks are
> always partitioned.
>
> It should also be added to vvfat_strong_runtime_opts because the value
> of this option changes the data that the guest sees.

Just to keep you updated, I've seen your comments. I'm just waiting to
see where the discussion of patch 5 leads before pushing v2.

> >          { /* end of list */ }
> >      },
> >  };
> > @@ -1092,6 +1097,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
> >      int fat_type = 0;
> >      bool floppy = false;
> >      bool rw = false;
> > +    bool no_mbr = false;
> >      int i;
> >
> >      if (!strstart(filename, "fat:", NULL)) {
> > @@ -1116,6 +1122,10 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
> >          rw = true;
> >      }
> >
> > +    if (strstr(filename, ":no-mbr:")) {
>
> In the string, the negative form can stay (because the positive one
> doesn't exist here).
>
> > +        no_mbr = true;
> > +    }
> > +
> >      /* Get the directory name without options */
> >      i = strrchr(filename, ':') - filename;
> >      assert(i >= 3);
> > @@ -1131,6 +1141,7 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
> >      qdict_put_int(options, "fat-type", fat_type);
> >      qdict_put_bool(options, "floppy", floppy);
> >      qdict_put_bool(options, "rw", rw);
> > +    qdict_put_bool(options, "no-mbr", no_mbr);
> >  }
> >
> >  static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> > @@ -1196,7 +1207,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
> >          if (!s->fat_type) {
> >              s->fat_type = 16;
> >          }
> > -        s->offset_to_bootsector = 0x3f;
> > +        /* Reserver space for MBR */
> > +        if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
> > +            s->offset_to_bootsector = 0x3f;
> > +        }
> >          cyls = s->fat_type == 12 ? 64 : 1024;
> >          heads = 16;
> >          secs = 63;
>
> Kevin
>

Re: [PATCH 1/5] vvfat: introduce no-mbr option

[Kevin Wolf]

Am 29.10.2025 um 09:37 hat Clément Chigot geschrieben:
> On Thu, Oct 23, 2025 at 8:21 PM Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > > This option when set prevents a master boot record (MBR) to be
> > > initialized. This is mandatory as some operating system don't recognized
> > > mounted disks if a MBR is present.
> > >
> > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> >
> > Can we actually give an example of such an OS in the commit message?
> >
> > > ---
> > >  block/vvfat.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index 814796d918..0220dd828b 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -1082,6 +1082,11 @@ static QemuOptsList runtime_opts = {
> > >              .type = QEMU_OPT_BOOL,
> > >              .help = "Make the image writable",
> > >          },
> > > +        {
> > > +            .name = "no-mbr",
> > > +            .type = QEMU_OPT_BOOL,
> > > +            .help = "Do not add a Master Boot Record on this disk",
> > > +        },
> >
> > Let's keep option names positive to avoid double negations like
> > 'no-mbr=false'. We can have an 'mbr' option that defaults to true. Or in
> > fact, maybe calling it 'partitioned' would be easier to understand.
> >
> > You need to update BlockdevOptionsVVFAT in qapi/block-core.json, too, to
> > make the new option work with -blockdev. You should update the
> > description for @floppy there, too, because it says that hard disks are
> > always partitioned.
> >
> > It should also be added to vvfat_strong_runtime_opts because the value
> > of this option changes the data that the guest sees.
> 
> Just to keep you updated, I've seen your comments. I'm just waiting to
> see where the discussion of patch 5 leads before pushing v2.

Yes, that makes sense. So this means that for all the other patches, you
agree with my comments and it's clear to you what to change in v2?

Kevin

Re: [PATCH 1/5] vvfat: introduce no-mbr option

[Clément Chigot]

On Wed, Oct 29, 2025 at 11:56 AM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 29.10.2025 um 09:37 hat Clément Chigot geschrieben:
> > On Thu, Oct 23, 2025 at 8:21 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > > > This option when set prevents a master boot record (MBR) to be
> > > > initialized. This is mandatory as some operating system don't recognized
> > > > mounted disks if a MBR is present.
> > > >
> > > > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > >
> > > Can we actually give an example of such an OS in the commit message?
> > >
> > > > ---
> > > >  block/vvfat.c | 16 +++++++++++++++-
> > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > > index 814796d918..0220dd828b 100644
> > > > --- a/block/vvfat.c
> > > > +++ b/block/vvfat.c
> > > > @@ -1082,6 +1082,11 @@ static QemuOptsList runtime_opts = {
> > > >              .type = QEMU_OPT_BOOL,
> > > >              .help = "Make the image writable",
> > > >          },
> > > > +        {
> > > > +            .name = "no-mbr",
> > > > +            .type = QEMU_OPT_BOOL,
> > > > +            .help = "Do not add a Master Boot Record on this disk",
> > > > +        },
> > >
> > > Let's keep option names positive to avoid double negations like
> > > 'no-mbr=false'. We can have an 'mbr' option that defaults to true. Or in
> > > fact, maybe calling it 'partitioned' would be easier to understand.
> > >
> > > You need to update BlockdevOptionsVVFAT in qapi/block-core.json, too, to
> > > make the new option work with -blockdev. You should update the
> > > description for @floppy there, too, because it says that hard disks are
> > > always partitioned.
> > >
> > > It should also be added to vvfat_strong_runtime_opts because the value
> > > of this option changes the data that the guest sees.
> >
> > Just to keep you updated, I've seen your comments. I'm just waiting to
> > see where the discussion of patch 5 leads before pushing v2.
>
> Yes, that makes sense. So this means that for all the other patches, you
> agree with my comments and it's clear to you what to change in v2?

Globally yes. I'm still unsure about patch 2 (I'll answer directly on
this patch). But for patch 1 and, yes clearly.

[PATCH 2/5] vvfat: move fat_type check prior to size setup

[Clément Chigot]

This allows to handle the default FAT size in a single place and make the
following part taking care only about size parameters. It will be later
moved away in a specific function.

Setting a floppy disk of 1MB is no longer possible as it was a side
effect of passing "fat-type=12". To be precise there were three cases:
 - fat-type undefined (aka default): a fat12 2MB disk
 - fat-type=16: a fat16 2Mb disk
 - fat-type=12: a fat12 1Mb disk

Now, that fat-type undefined means fat-type=2, it's no longer possible
to make that size distinction. It will be added back a bit later,
through the size parameter.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 48 +++++++++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 0220dd828b..91e4ad3158 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1190,45 +1190,51 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         memcpy(s->volume_label, "QEMU VVFAT", 10);
     }
 
-    if (floppy) {
-        /* 1.44MB or 2.88MB floppy.  2.88MB can be FAT12 (default) or FAT16. */
-        if (!s->fat_type) {
+    /* Verify FAT type  */
+    switch (s->fat_type) {
+    case 32:
+        warn_report("FAT32 has not been tested. You are welcome to do so!");
+        break;
+    case 16:
+    case 12:
+        break;
+    case 0:
+        /* Set a default type */
+        if (floppy) {
             s->fat_type = 12;
+        } else {
+            s->fat_type = 16;
+        }
+        break;
+    default:
+        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+
+    if (floppy) {
+        /* 2.88MB floppy */
+        if (s->fat_type == 12) {
             secs = 36;
             s->sectors_per_cluster = 2;
         } else {
-            secs = s->fat_type == 12 ? 18 : 36;
+            secs = 36;
             s->sectors_per_cluster = 1;
         }
         cyls = 80;
         heads = 2;
     } else {
-        /* 32MB or 504MB disk*/
-        if (!s->fat_type) {
-            s->fat_type = 16;
-        }
         /* Reserver space for MBR */
         if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
             s->offset_to_bootsector = 0x3f;
         }
+        /* 32MB or 504MB disk*/
         cyls = s->fat_type == 12 ? 64 : 1024;
         heads = 16;
         secs = 63;
     }
 
-    switch (s->fat_type) {
-    case 32:
-        warn_report("FAT32 has not been tested. You are welcome to do so!");
-        break;
-    case 16:
-    case 12:
-        break;
-    default:
-        error_setg(errp, "Valid FAT types are only 12, 16 and 32");
-        ret = -EINVAL;
-        goto fail;
-    }
-
 
     s->bs = bs;
 
-- 
2.34.1

Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup

[Kevin Wolf]

Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> This allows to handle the default FAT size in a single place and make the
> following part taking care only about size parameters. It will be later
> moved away in a specific function.
> 
> Setting a floppy disk of 1MB is no longer possible as it was a side
> effect of passing "fat-type=12". To be precise there were three cases:
>  - fat-type undefined (aka default): a fat12 2MB disk
>  - fat-type=16: a fat16 2Mb disk
>  - fat-type=12: a fat12 1Mb disk

That's quite a strange interface!

If we're touching it anyway, I would change it to make the more common
format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
temporarily unavailable and later require an explicit size. This way
both sizes would still be available using the fat-type.

Please say 1.44 MB and 2.88 MB in the commit message rather than 1MB (or
even 1Mb, which might mean megabit). There were other sizes like 1.2 MB
that are closer to 1 MB, so it's better to avoid that confusion.

> Now, that fat-type undefined means fat-type=2, it's no longer possible

s/2/12/

> to make that size distinction. It will be added back a bit later,
> through the size parameter.
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>

The code looks good, apart from the change I suggested above (making
1.44 MB the default for FAT12).

Kevin

Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup

[Clément Chigot]

On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > This allows to handle the default FAT size in a single place and make the
> > following part taking care only about size parameters. It will be later
> > moved away in a specific function.
> >
> > Setting a floppy disk of 1MB is no longer possible as it was a side
> > effect of passing "fat-type=12". To be precise there were three cases:
> >  - fat-type undefined (aka default): a fat12 2MB disk
> >  - fat-type=16: a fat16 2Mb disk
> >  - fat-type=12: a fat12 1Mb disk
>
> That's quite a strange interface!
>
> If we're touching it anyway, I would change it to make the more common
> format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
> temporarily unavailable and later require an explicit size. This way
> both sizes would still be available using the fat-type.

I'm a bit hesitant to change the default behavior as people might be
using it without clear knowledge of it. True, "floppy" is probably not
a widely used feature but still.
Do QEMU have some specific guidelines when changing such default
behavior ? Adding a warning ? Or just a comment in the changelog would
be enough ?

> Please say 1.44 MB and 2.88 MB in the commit message rather than 1MB (or
> even 1Mb, which might mean megabit). There were other sizes like 1.2 MB
> that are closer to 1 MB, so it's better to avoid that confusion.
>
> > Now, that fat-type undefined means fat-type=2, it's no longer possible
>
> s/2/12/
>
> > to make that size distinction. It will be added back a bit later,
> > through the size parameter.
> >
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
>
> The code looks good, apart from the change I suggested above (making
> 1.44 MB the default for FAT12).
>
> Kevin
>

Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

On Wed, 29 Oct 2025, Clément Chigot wrote:
> On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
>>
>> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
>>> This allows to handle the default FAT size in a single place and make the
>>> following part taking care only about size parameters. It will be later
>>> moved away in a specific function.
>>>
>>> Setting a floppy disk of 1MB is no longer possible as it was a side
>>> effect of passing "fat-type=12". To be precise there were three cases:
>>>  - fat-type undefined (aka default): a fat12 2MB disk
>>>  - fat-type=16: a fat16 2Mb disk
>>>  - fat-type=12: a fat12 1Mb disk
>>
>> That's quite a strange interface!
>>
>> If we're touching it anyway, I would change it to make the more common
>> format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
>> temporarily unavailable and later require an explicit size. This way
>> both sizes would still be available using the fat-type.
>
> I'm a bit hesitant to change the default behavior as people might be
> using it without clear knowledge of it. True, "floppy" is probably not
> a widely used feature but still.
> Do QEMU have some specific guidelines when changing such default
> behavior ? Adding a warning ? Or just a comment in the changelog would
> be enough ?

https://www.qemu.org/docs/master/about/deprecated.html

Usually you'd add a warning and list it in the docs as deprecated then can 
make the change after it was deperecated for two releases.

Regards,
BALATON Zoltan

Re: [PATCH 2/5] vvfat: move fat_type check prior to size setup

[Kevin Wolf]

Am 29.10.2025 um 14:58 hat BALATON Zoltan geschrieben:
> On Wed, 29 Oct 2025, Clément Chigot wrote:
> > On Thu, Oct 23, 2025 at 8:40 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > > 
> > > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> > > > This allows to handle the default FAT size in a single place and make the
> > > > following part taking care only about size parameters. It will be later
> > > > moved away in a specific function.
> > > > 
> > > > Setting a floppy disk of 1MB is no longer possible as it was a side
> > > > effect of passing "fat-type=12". To be precise there were three cases:
> > > >  - fat-type undefined (aka default): a fat12 2MB disk
> > > >  - fat-type=16: a fat16 2Mb disk
> > > >  - fat-type=12: a fat12 1Mb disk
> > > 
> > > That's quite a strange interface!
> > > 
> > > If we're touching it anyway, I would change it to make the more common
> > > format (1.44 MB) the default for FAT12 and make the 2.88 MB FAT12 floppy
> > > temporarily unavailable and later require an explicit size. This way
> > > both sizes would still be available using the fat-type.
> > 
> > I'm a bit hesitant to change the default behavior as people might be
> > using it without clear knowledge of it. True, "floppy" is probably not
> > a widely used feature but still.
> > Do QEMU have some specific guidelines when changing such default
> > behavior ? Adding a warning ? Or just a comment in the changelog would
> > be enough ?
> 
> https://www.qemu.org/docs/master/about/deprecated.html
> 
> Usually you'd add a warning and list it in the docs as deprecated then can
> make the change after it was deperecated for two releases.

We're not removing any functionality, just changing the default. So I
don't think the deprecation period applies.

In general, our stance is that we can change defaults whenever we want
to, and if you don't want to be surprised by changing defaults, you need
to specify the option explicitly. What's a bit strange about the vvfat
interface is that the default actually represents a configuration that
can't even be expressed explicitly at the moment.

So it is a special case in a way, but given that this is vvfat, which is
known to be unstable, not widely used outside of the occasional manual
use and not supported by libvirt, I'm willing to just make the change.

Kevin

[PATCH 3/5] vvfat: add a define for SECTOR_SIZE

[Clément Chigot]

This makes those 0x200 far clearer.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 60 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 91e4ad3158..6b6d158a18 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -75,6 +75,8 @@ static void checkpoint(void);
  */
 #define BOOTSECTOR_OEM_NAME "MSWIN4.1"
 
+#define SECTOR_SIZE 0x200
+
 #define DIR_DELETED 0xe5
 #define DIR_KANJI DIR_DELETED
 #define DIR_KANJI_FAKE 0x05
@@ -300,7 +302,7 @@ static void print_mapping(const struct mapping_t* mapping);
 typedef struct BDRVVVFATState {
     CoMutex lock;
     BlockDriverState* bs; /* pointer to parent */
-    unsigned char first_sectors[0x40*0x200];
+    unsigned char first_sectors[0x40 * SECTOR_SIZE];
 
     int fat_type; /* 16 or 32 */
     array_t fat,directory,mapping;
@@ -689,11 +691,11 @@ static inline void init_fat(BDRVVVFATState* s)
     if (s->fat_type == 12) {
         array_init(&(s->fat),1);
         array_ensure_allocated(&(s->fat),
-                s->sectors_per_fat * 0x200 * 3 / 2 - 1);
+                s->sectors_per_fat * SECTOR_SIZE * 3 / 2 - 1);
     } else {
         array_init(&(s->fat),(s->fat_type==32?4:2));
         array_ensure_allocated(&(s->fat),
-                s->sectors_per_fat * 0x200 / s->fat.item_size - 1);
+                s->sectors_per_fat * SECTOR_SIZE / s->fat.item_size - 1);
     }
     memset(s->fat.pointer,0,s->fat.size);
 
@@ -901,19 +903,19 @@ static int init_directories(BDRVVVFATState* s,
     unsigned int i;
     unsigned int cluster;
 
-    memset(&(s->first_sectors[0]),0,0x40*0x200);
+    memset(&(s->first_sectors[0]), 0 , 0x40 * SECTOR_SIZE);
 
-    s->cluster_size=s->sectors_per_cluster*0x200;
+    s->cluster_size = s->sectors_per_cluster * SECTOR_SIZE;
     s->cluster_buffer=g_malloc(s->cluster_size);
 
     /*
-     * The formula: sc = spf+1+spf*spc*(512*8/fat_type),
+     * The formula: sc = spf+1+spf*spc*(SECTOR_SIZE*8/fat_type),
      * where sc is sector_count,
      * spf is sectors_per_fat,
      * spc is sectors_per_clusters, and
      * fat_type = 12, 16 or 32.
      */
-    i = 1+s->sectors_per_cluster*0x200*8/s->fat_type;
+    i = 1 + s->sectors_per_cluster * SECTOR_SIZE * 8 / s->fat_type;
     s->sectors_per_fat=(s->sector_count+i)/i; /* round up */
 
     s->offset_to_fat = s->offset_to_bootsector + 1;
@@ -1011,12 +1013,12 @@ static int init_directories(BDRVVVFATState* s,
     s->current_mapping = NULL;
 
     bootsector = (bootsector_t *)(s->first_sectors
-                                  + s->offset_to_bootsector * 0x200);
+                                  + s->offset_to_bootsector * SECTOR_SIZE);
     bootsector->jump[0]=0xeb;
     bootsector->jump[1]=0x3e;
     bootsector->jump[2]=0x90;
     memcpy(bootsector->name, BOOTSECTOR_OEM_NAME, 8);
-    bootsector->sector_size=cpu_to_le16(0x200);
+    bootsector->sector_size = cpu_to_le16(SECTOR_SIZE);
     bootsector->sectors_per_cluster=s->sectors_per_cluster;
     bootsector->reserved_sectors=cpu_to_le16(1);
     bootsector->number_of_fats=0x2; /* number of FATs */
@@ -1513,7 +1515,7 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
                              " allocated\n", sector_num,
                              n >> BDRV_SECTOR_BITS));
                 if (bdrv_co_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE, n,
-                                  buf + i * 0x200, 0) < 0) {
+                                  buf + i * SECTOR_SIZE, 0) < 0) {
                     return -1;
                 }
                 i += (n >> BDRV_SECTOR_BITS) - 1;
@@ -1525,19 +1527,19 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
         }
         if (sector_num < s->offset_to_root_dir) {
             if (sector_num < s->offset_to_fat) {
-                memcpy(buf + i * 0x200,
-                       &(s->first_sectors[sector_num * 0x200]),
-                       0x200);
+                memcpy(buf + i * SECTOR_SIZE,
+                       &(s->first_sectors[sector_num * SECTOR_SIZE]),
+                       SECTOR_SIZE);
             } else if (sector_num < s->offset_to_fat + s->sectors_per_fat) {
-                memcpy(buf + i * 0x200,
+                memcpy(buf + i * SECTOR_SIZE,
                        &(s->fat.pointer[(sector_num
-                                       - s->offset_to_fat) * 0x200]),
-                       0x200);
+                                       - s->offset_to_fat) * SECTOR_SIZE]),
+                       SECTOR_SIZE);
             } else if (sector_num < s->offset_to_root_dir) {
-                memcpy(buf + i * 0x200,
+                memcpy(buf + i * SECTOR_SIZE,
                        &(s->fat.pointer[(sector_num - s->offset_to_fat
-                                       - s->sectors_per_fat) * 0x200]),
-                       0x200);
+                                       - s->sectors_per_fat) * SECTOR_SIZE]),
+                       SECTOR_SIZE);
             }
         } else {
             uint32_t sector = sector_num - s->offset_to_root_dir,
@@ -1545,10 +1547,12 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
             cluster_num=sector/s->sectors_per_cluster;
             if(cluster_num > s->cluster_count || read_cluster(s, cluster_num) != 0) {
                 /* LATER TODO: strict: return -1; */
-                memset(buf+i*0x200,0,0x200);
+                memset(buf + i * SECTOR_SIZE, 0, SECTOR_SIZE);
                 continue;
             }
-            memcpy(buf+i*0x200,s->cluster+sector_offset_in_cluster*0x200,0x200);
+            memcpy(buf + i * SECTOR_SIZE,
+                   s->cluster + sector_offset_in_cluster * SECTOR_SIZE,
+                   SECTOR_SIZE);
         }
     }
     return 0;
@@ -2176,7 +2180,7 @@ DLOG(checkpoint());
      * - if all is fine, return number of used clusters
      */
     if (s->fat2 == NULL) {
-        int size = 0x200 * s->sectors_per_fat;
+        int size = SECTOR_SIZE * s->sectors_per_fat;
         s->fat2 = g_malloc(size);
         memcpy(s->fat2, s->fat.pointer, size);
     }
@@ -2573,7 +2577,8 @@ commit_one_file(BDRVVVFATState* s, int dir_index, uint32_t offset)
                 (size > offset && c >=2 && !fat_eof(s, c)));
 
         ret = vvfat_read(s->bs, cluster2sector(s, c),
-            (uint8_t*)cluster, DIV_ROUND_UP(rest_size, 0x200));
+                         (uint8_t *)cluster,
+                         DIV_ROUND_UP(rest_size, SECTOR_SIZE));
 
         if (ret < 0) {
             qemu_close(fd);
@@ -2952,7 +2957,7 @@ static int coroutine_fn GRAPH_RDLOCK do_commit(BDRVVVFATState* s)
     }
 
     /* copy FAT (with bdrv_pread) */
-    memcpy(s->fat.pointer, s->fat2, 0x200 * s->sectors_per_fat);
+    memcpy(s->fat.pointer, s->fat2, SECTOR_SIZE * s->sectors_per_fat);
 
     /* recurse direntries from root (using bs->bdrv_pread) */
     ret = commit_direntries(s, 0, -1);
@@ -3016,14 +3021,14 @@ DLOG(checkpoint());
          * used to mark volume dirtiness
          */
         unsigned char *bootsector = s->first_sectors
-                                    + s->offset_to_bootsector * 0x200;
+                                    + s->offset_to_bootsector * SECTOR_SIZE;
         /*
          * LATER TODO: if FAT32, this is wrong (see init_directories(),
          * which always creates a FAT16 bootsector)
          */
         const int reserved1_offset = offsetof(bootsector_t, u.fat16.reserved1);
 
-        for (i = 0; i < 0x200; i++) {
+        for (i = 0; i < SECTOR_SIZE; i++) {
             if (i != reserved1_offset && bootsector[i] != buf[i]) {
                 fprintf(stderr, "Tried to write to protected bootsector\n");
                 return -1;
@@ -3078,7 +3083,8 @@ DLOG(checkpoint());
                     end = sector_num + nb_sectors;
                 dir_index  = mapping->dir_index +
                     0x10 * (begin - mapping->begin * s->sectors_per_cluster);
-                direntries = (direntry_t*)(buf + 0x200 * (begin - sector_num));
+                direntries =
+                    (direntry_t *)(buf + SECTOR_SIZE * (begin - sector_num));
 
                 for (k = 0; k < (end - begin) * 0x10; k++) {
                     /* no access to the direntry of a read-only file */
-- 
2.34.1

Re: [PATCH 3/5] vvfat: add a define for SECTOR_SIZE

[Kevin Wolf]

Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> This makes those 0x200 far clearer.
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>  block/vvfat.c | 60 ++++++++++++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 27 deletions(-)

> @@ -1513,7 +1515,7 @@ vvfat_read(BlockDriverState *bs, int64_t sector_num, uint8_t *buf, int nb_sector
>                               " allocated\n", sector_num,
>                               n >> BDRV_SECTOR_BITS));
>                  if (bdrv_co_pread(s->qcow, sector_num * BDRV_SECTOR_SIZE, n,
> -                                  buf + i * 0x200, 0) < 0) {
> +                                  buf + i * SECTOR_SIZE, 0) < 0) {

We get a nasty mix of BDRV_SECTOR_SIZE (the QEMU block layer's sector
size) and the new SECTOR_SIZE (the FAT file system's sector size) here.
I think both of these actually refer to FAT.

Should we also change those instances of BDRV_SECTOR_SIZE that really
should be SECTOR_SIZE?

Kevin

[PATCH 4/5] vvfat: move size parameters within driver structure

[Clément Chigot]

At the same time, rename them to match bootsector fields.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 6b6d158a18..6526c585a2 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -310,7 +310,10 @@ typedef struct BDRVVVFATState {
 
     uint32_t offset_to_bootsector; /* 0 for floppy, 0x3f for disk */
 
+    unsigned int cylinders;
     unsigned int cluster_size;
+    unsigned int number_of_heads;
+    unsigned int sectors_per_track;
     unsigned int sectors_per_cluster;
     unsigned int sectors_per_fat;
     uint32_t last_cluster_of_root_directory;
@@ -364,7 +367,7 @@ static int sector2CHS(mbr_chs_t *chs, int spos, int cyls, int heads, int secs)
     return 0;
 }
 
-static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
+static void init_mbr(BDRVVVFATState *s)
 {
     /* TODO: if the files mbr.img and bootsect.img exist, use them */
     mbr_t* real_mbr=(mbr_t*)s->first_sectors;
@@ -380,9 +383,9 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
 
     /* LBA is used when partition is outside the CHS geometry */
     lba  = sector2CHS(&partition->start_CHS, s->offset_to_bootsector,
-                     cyls, heads, secs);
+                      s->cylinders, s->number_of_heads, s->sectors_per_track);
     lba |= sector2CHS(&partition->end_CHS,   s->bs->total_sectors - 1,
-                     cyls, heads, secs);
+                      s->cylinders, s->number_of_heads, s->sectors_per_track);
 
     /*LBA partitions are identified only by start/length_sector_long not by CHS*/
     partition->start_sector_long  = cpu_to_le32(s->offset_to_bootsector);
@@ -894,8 +897,7 @@ static inline off_t cluster2sector(BDRVVVFATState* s, uint32_t cluster_num)
     return s->offset_to_root_dir + s->sectors_per_cluster * cluster_num;
 }
 
-static int init_directories(BDRVVVFATState* s,
-                            const char *dirname, int heads, int secs,
+static int init_directories(BDRVVVFATState *s, const char *dirname,
                             Error **errp)
 {
     bootsector_t* bootsector;
@@ -1028,8 +1030,8 @@ static int init_directories(BDRVVVFATState* s,
     bootsector->media_type = (s->offset_to_bootsector > 0 ? 0xf8 : 0xf0);
     s->fat.pointer[0] = bootsector->media_type;
     bootsector->sectors_per_fat=cpu_to_le16(s->sectors_per_fat);
-    bootsector->sectors_per_track = cpu_to_le16(secs);
-    bootsector->number_of_heads = cpu_to_le16(heads);
+    bootsector->sectors_per_track = cpu_to_le16(s->sectors_per_track);
+    bootsector->number_of_heads = cpu_to_le16(s->number_of_heads);
     bootsector->hidden_sectors = cpu_to_le32(s->offset_to_bootsector);
     bootsector->total_sectors=cpu_to_le32(s->sector_count>0xffff?s->sector_count:0);
 
@@ -1150,7 +1152,6 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
-    int cyls, heads, secs;
     bool floppy;
     const char *dirname, *label;
     QemuOpts *opts;
@@ -1218,23 +1219,23 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     if (floppy) {
         /* 2.88MB floppy */
         if (s->fat_type == 12) {
-            secs = 36;
+            s->sectors_per_track = 36;
             s->sectors_per_cluster = 2;
         } else {
-            secs = 36;
+            s->sectors_per_track = 36;
             s->sectors_per_cluster = 1;
         }
-        cyls = 80;
-        heads = 2;
+        s->cylinder = 80;
+        s->number_of_heads = 2;
     } else {
         /* Reserver space for MBR */
         if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
             s->offset_to_bootsector = 0x3f;
         }
         /* 32MB or 504MB disk*/
-        cyls = s->fat_type == 12 ? 64 : 1024;
-        heads = 16;
-        secs = 63;
+        s->cylinders = s->fat_type == 12 ? 64 : 1024;
+        s->number_of_heads = 16;
+        s->sectors_per_track = 63;
     }
 
 
@@ -1251,10 +1252,13 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     s->downcase_short_names = 1;
 
     DLOG(fprintf(stderr, "vvfat %s chs %d,%d,%d\n",
-                 dirname, cyls, heads, secs));
+                 dirname, s->cylinders, s->number_of_heads,
+                 s->sectors_per_track));
 
-    s->sector_count = cyls * heads * secs - s->offset_to_bootsector;
-    bs->total_sectors = cyls * heads * secs;
+    s->sector_count = s->cylinders * s->number_of_heads *
+        s->sectors_per_track - s->offset_to_bootsector;
+    bs->total_sectors = s->cylinders * s->number_of_heads *
+        s->sectors_per_track;
 
     if (qemu_opt_get_bool(opts, "rw", false)) {
         if (!bdrv_is_read_only(bs)) {
@@ -1275,7 +1279,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    if (init_directories(s, dirname, heads, secs, errp)) {
+    if (init_directories(s, dirname, errp)) {
         ret = -EIO;
         goto fail;
     }
@@ -1296,7 +1300,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     if (s->offset_to_bootsector > 0) {
-        init_mbr(s, cyls, heads, secs);
+        init_mbr(s);
     }
 
     qemu_co_mutex_init(&s->lock);
-- 
2.34.1

[PATCH 5/5] vvfat: add support for "size" options

[Clément Chigot]

This allows more flexibility to vvfat backend. The value for "Number of
Heads" and "Sectors per track" are based on SD specifications Part 2.

Some limitations remains, the size parameter is recognized only when
"format=vvfat" is passed. In particular, "format=raw,size=xxx" is
keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
FAT12. FAT32 has not been adjusted and thus still default to 504MB.

Moreover, for flopyy, size=1M is creating a disk 1.44 MB, and size=2M a
disk of 2.88 MB. This avoids having to worry about float operations.

Signed-off-by: Clément Chigot <chigot@adacore.com>
---
 block/vvfat.c | 165 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 141 insertions(+), 24 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 6526c585a2..4537c39d5c 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1091,6 +1091,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Do not add a Master Boot Record on this disk",
         },
+        {
+            .name = BLOCK_OPT_SIZE,
+            .type = QEMU_OPT_SIZE,
+            .help = "Virtual disk size"
+        },
         { /* end of list */ }
     },
 };
@@ -1148,10 +1153,141 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put_bool(options, "no-mbr", no_mbr);
 }
 
+static void vvfat_get_size_parameters(uint64_t size, BDRVVVFATState *s,
+                                      bool floppy, Error **errp)
+{
+    if (floppy) {
+        /*
+         * Floppy emulation only supports 1.44 MB or 2.88 MB (default).
+         * In order to avoid floating operations ambiguity, 1 MB is
+         * recognized for 1.44 MB and 2 MB for 2.88 MB.
+         */
+        if (!size) {
+            size = 2 * 1024 * 1024;
+        } else {
+            if (size == 1024 * 1024 && s->fat_type == 16) {
+                error_setg(errp,
+                           "floppy FAT16 unsupported size; only support 2M "
+                           "(for an effective size of 2.88 MB)");
+            } else if (size != 2 * 1024 * 1024 && size != 1024 * 1024) {
+                error_setg(errp,
+                           "floppy unsupported size; should be 1MB (for "
+                           "an effective size of 1.44 MB) or 2.88M (for "
+                           "2.88MB)");
+            }
+        }
+
+        if (s->fat_type == 12) {
+            if (size == 2 * 1024 * 1024) {
+                s->sectors_per_cluster = 2;
+            } else {
+                s->sectors_per_cluster = 1;
+            }
+        } else {
+            s->sectors_per_cluster = 1;
+        }
+
+        s->sectors_per_track = 36;
+        s->cylinders = 80;
+        s->number_of_heads = 2;
+    } else {
+        /* LATER TODO: if FAT32, adjust */
+        s->sectors_per_cluster = 0x10;
+
+        switch (s->fat_type) {
+        case 12:
+
+            /* Default is 32 MB */
+            if (!size) {
+                size = 32 * 1024 * 1024;
+            } else if (size > 32 * 1024 * 1024) {
+                error_setg(errp, "FAT12 unsupported size; higher than 32Mb");
+            }
+
+            s->cylinders = 64;
+
+            /*
+             * Based on CHS Recommandation table:
+             *  Card Capacity | Number of Headers | Sectors per track
+             *     ~ 2 MB     |         4         |       16
+             *     ~ 4 MB     |         8         |       16
+             *     ~ 8 MB     |        16         |       16
+             *     ~ 16 MB    |         2         |       32
+             *     ~ 32 MB    |         4         |       32
+             *
+             * For 2 MB, SD is recommending heads = 2 and sectors = 16, but
+             * this requires a different number of cylinders. Thus, it was
+             * adjusted to keep this number constant.
+             */
+            if (size <= 8 * 1024 * 1024) {
+                s->sectors_per_track = 16;
+            } else {
+                s->sectors_per_track = 32;
+            }
+
+            /*
+             * The formula between the size (in bytes) and the parameters are:
+             *  size = SECTOR_SIZE * sectors_per_track * number_of_headers *
+             *         cylinders
+             */
+            s->number_of_heads = size / s->sectors_per_track /
+                SECTOR_SIZE / s->cylinders;
+            return;
+
+        case 16:
+            /* Default is 504 MB */
+            if (!size) {
+                size = 504 * 1024 * 1024;
+            } else if (size / 1024 > 4 * 1024 * 1024) {
+                error_setg(errp, "FAT16 unsupported size; higher than 4Gb");
+            }
+
+            s->cylinders = 1024;
+
+            /*
+             * Based on CHS Recommandation table:
+             *  Card Capacity | Number of Headers | Sectors per track
+             *     ~64 MB     |         4         |       32
+             *     ~128 MB    |         8         |       32
+             *     ~256 MB    |        16         |       32
+             *     ~504 MB    |        16         |       63
+             *    ~1008 MB    |        32         |       63
+             *    ~2016 MB    |        64         |       63
+             */
+            if (size <= 256 * 1024 * 1024) {
+                s->sectors_per_track = 32;
+            } else {
+                s->sectors_per_track = 63;
+            }
+
+            /*
+             * The formula between the size (in bytes) and the parameters are:
+             *  size = SECTOR_SIZE * sectors_per_track * number_of_headers *
+             *         cylinders
+             */
+            s->number_of_heads = size / s->sectors_per_track /
+                SECTOR_SIZE / s->cylinders;
+            return;
+
+        case 32:
+            /* TODO FAT32 adjust  */
+            if (size) {
+                warn_report("size parameters not supported with FAT32;"
+                            "default to 504MB.");
+            }
+            s->cylinders = 1024;
+            s->number_of_heads = 16;
+            s->sectors_per_track = 63;
+            return;
+        }
+    }
+}
+
 static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
+    uint64_t size;
     bool floppy;
     const char *dirname, *label;
     QemuOpts *opts;
@@ -1178,6 +1314,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->fat_type = qemu_opt_get_number(opts, "fat-type", 0);
     floppy = qemu_opt_get_bool(opts, "floppy", false);
+    size = qemu_opt_get_size_del(opts, "size", 0);
 
     memset(s->volume_label, ' ', sizeof(s->volume_label));
     label = qemu_opt_get(opts, "label");
@@ -1215,35 +1352,15 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    vvfat_get_size_parameters(size, s, floppy, errp);
 
-    if (floppy) {
-        /* 2.88MB floppy */
-        if (s->fat_type == 12) {
-            s->sectors_per_track = 36;
-            s->sectors_per_cluster = 2;
-        } else {
-            s->sectors_per_track = 36;
-            s->sectors_per_cluster = 1;
-        }
-        s->cylinder = 80;
-        s->number_of_heads = 2;
-    } else {
-        /* Reserver space for MBR */
-        if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
-            s->offset_to_bootsector = 0x3f;
-        }
-        /* 32MB or 504MB disk*/
-        s->cylinders = s->fat_type == 12 ? 64 : 1024;
-        s->number_of_heads = 16;
-        s->sectors_per_track = 63;
+    /* Reserver space for MBR */
+    if (!floppy && !qemu_opt_get_bool(opts, "no-mbr", false)) {
+        s->offset_to_bootsector = 0x3f;
     }
 
-
     s->bs = bs;
 
-    /* LATER TODO: if FAT32, adjust */
-    s->sectors_per_cluster=0x10;
-
     s->current_cluster=0xffffffff;
 
     s->qcow = NULL;
-- 
2.34.1

Re: [PATCH 5/5] vvfat: add support for "size" options

[Kevin Wolf]

Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> This allows more flexibility to vvfat backend. The value for "Number of
> Heads" and "Sectors per track" are based on SD specifications Part 2.
> 
> Some limitations remains, the size parameter is recognized only when
> "format=vvfat" is passed. In particular, "format=raw,size=xxx" is
> keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
> FAT12. FAT32 has not been adjusted and thus still default to 504MB.
> 
> Moreover, for flopyy, size=1M is creating a disk 1.44 MB, and size=2M a
> disk of 2.88 MB. This avoids having to worry about float operations.
> 
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> ---
>  block/vvfat.c | 165 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 141 insertions(+), 24 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 6526c585a2..4537c39d5c 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1091,6 +1091,11 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_BOOL,
>              .help = "Do not add a Master Boot Record on this disk",
>          },
> +        {
> +            .name = BLOCK_OPT_SIZE,
> +            .type = QEMU_OPT_SIZE,
> +            .help = "Virtual disk size"
> +        },
>          { /* end of list */ }
>      },
>  };

Like in patch 1, you need additional changes, in particular to add the
option to the QAPI schema in qapi/block-core.json.

> @@ -1148,10 +1153,141 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
>      qdict_put_bool(options, "no-mbr", no_mbr);
>  }
>  
> +static void vvfat_get_size_parameters(uint64_t size, BDRVVVFATState *s,
> +                                      bool floppy, Error **errp)
> +{
> +    if (floppy) {
> +        /*
> +         * Floppy emulation only supports 1.44 MB or 2.88 MB (default).
> +         * In order to avoid floating operations ambiguity, 1 MB is
> +         * recognized for 1.44 MB and 2 MB for 2.88 MB.
> +         */
> +        if (!size) {
> +            size = 2 * 1024 * 1024;
> +        } else {
> +            if (size == 1024 * 1024 && s->fat_type == 16) {
> +                error_setg(errp,
> +                           "floppy FAT16 unsupported size; only support 2M "
> +                           "(for an effective size of 2.88 MB)");
> +            } else if (size != 2 * 1024 * 1024 && size != 1024 * 1024) {
> +                error_setg(errp,
> +                           "floppy unsupported size; should be 1MB (for "
> +                           "an effective size of 1.44 MB) or 2.88M (for "
> +                           "2.88MB)");
> +            }
> +        }

This is horrible. To be fair, it's pretty hard to do something not
horrible when the usual units to describe floppy sizes are already
horrible. :-)

But I'd still like us to do better here.

To me it looks a bit like what we really want is an enum for floppy
sizes (though is there any real reason why we have only those two?), but
an arbitrary size for hard disks.

Without the enum, obviously, users could specify 1440k and that would do
the right thing. Maybe special casing whatever 1.44M and 2.88M result
in and translating them into 1440k and 2880k could be more justifiable
than special casing 1M and 2M, but it would still be ugly.

Markus, do you have any advice how this should be represented in QAPI?

Kevin

> +
> +        if (s->fat_type == 12) {
> +            if (size == 2 * 1024 * 1024) {
> +                s->sectors_per_cluster = 2;
> +            } else {
> +                s->sectors_per_cluster = 1;
> +            }
> +        } else {
> +            s->sectors_per_cluster = 1;
> +        }
> +
> +        s->sectors_per_track = 36;
> +        s->cylinders = 80;
> +        s->number_of_heads = 2;
> +    } else {
> +        /* LATER TODO: if FAT32, adjust */
> +        s->sectors_per_cluster = 0x10;
> +
> +        switch (s->fat_type) {
> +        case 12:
> +
> +            /* Default is 32 MB */
> +            if (!size) {
> +                size = 32 * 1024 * 1024;
> +            } else if (size > 32 * 1024 * 1024) {
> +                error_setg(errp, "FAT12 unsupported size; higher than 32Mb");
> +            }
> +
> +            s->cylinders = 64;
> +
> +            /*
> +             * Based on CHS Recommandation table:
> +             *  Card Capacity | Number of Headers | Sectors per track
> +             *     ~ 2 MB     |         4         |       16
> +             *     ~ 4 MB     |         8         |       16
> +             *     ~ 8 MB     |        16         |       16
> +             *     ~ 16 MB    |         2         |       32
> +             *     ~ 32 MB    |         4         |       32
> +             *
> +             * For 2 MB, SD is recommending heads = 2 and sectors = 16, but
> +             * this requires a different number of cylinders. Thus, it was
> +             * adjusted to keep this number constant.
> +             */
> +            if (size <= 8 * 1024 * 1024) {
> +                s->sectors_per_track = 16;
> +            } else {
> +                s->sectors_per_track = 32;
> +            }
> +
> +            /*
> +             * The formula between the size (in bytes) and the parameters are:
> +             *  size = SECTOR_SIZE * sectors_per_track * number_of_headers *
> +             *         cylinders
> +             */
> +            s->number_of_heads = size / s->sectors_per_track /
> +                SECTOR_SIZE / s->cylinders;
> +            return;
> +
> +        case 16:
> +            /* Default is 504 MB */
> +            if (!size) {
> +                size = 504 * 1024 * 1024;
> +            } else if (size / 1024 > 4 * 1024 * 1024) {
> +                error_setg(errp, "FAT16 unsupported size; higher than 4Gb");
> +            }
> +
> +            s->cylinders = 1024;
> +
> +            /*
> +             * Based on CHS Recommandation table:
> +             *  Card Capacity | Number of Headers | Sectors per track
> +             *     ~64 MB     |         4         |       32
> +             *     ~128 MB    |         8         |       32
> +             *     ~256 MB    |        16         |       32
> +             *     ~504 MB    |        16         |       63
> +             *    ~1008 MB    |        32         |       63
> +             *    ~2016 MB    |        64         |       63
> +             */
> +            if (size <= 256 * 1024 * 1024) {
> +                s->sectors_per_track = 32;
> +            } else {
> +                s->sectors_per_track = 63;
> +            }
> +
> +            /*
> +             * The formula between the size (in bytes) and the parameters are:
> +             *  size = SECTOR_SIZE * sectors_per_track * number_of_headers *
> +             *         cylinders
> +             */
> +            s->number_of_heads = size / s->sectors_per_track /
> +                SECTOR_SIZE / s->cylinders;
> +            return;
> +
> +        case 32:
> +            /* TODO FAT32 adjust  */
> +            if (size) {
> +                warn_report("size parameters not supported with FAT32;"
> +                            "default to 504MB.");
> +            }
> +            s->cylinders = 1024;
> +            s->number_of_heads = 16;
> +            s->sectors_per_track = 63;
> +            return;
> +        }
> +    }
> +}
> +
>  static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>                        Error **errp)
>  {
>      BDRVVVFATState *s = bs->opaque;
> +    uint64_t size;
>      bool floppy;
>      const char *dirname, *label;
>      QemuOpts *opts;
> @@ -1178,6 +1314,7 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>  
>      s->fat_type = qemu_opt_get_number(opts, "fat-type", 0);
>      floppy = qemu_opt_get_bool(opts, "floppy", false);
> +    size = qemu_opt_get_size_del(opts, "size", 0);
>  
>      memset(s->volume_label, ' ', sizeof(s->volume_label));
>      label = qemu_opt_get(opts, "label");
> @@ -1215,35 +1352,15 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>          goto fail;
>      }
>  
> +    vvfat_get_size_parameters(size, s, floppy, errp);
>  
> -    if (floppy) {
> -        /* 2.88MB floppy */
> -        if (s->fat_type == 12) {
> -            s->sectors_per_track = 36;
> -            s->sectors_per_cluster = 2;
> -        } else {
> -            s->sectors_per_track = 36;
> -            s->sectors_per_cluster = 1;
> -        }
> -        s->cylinder = 80;
> -        s->number_of_heads = 2;
> -    } else {
> -        /* Reserver space for MBR */
> -        if (!qemu_opt_get_bool(opts, "no-mbr", false)) {
> -            s->offset_to_bootsector = 0x3f;
> -        }
> -        /* 32MB or 504MB disk*/
> -        s->cylinders = s->fat_type == 12 ? 64 : 1024;
> -        s->number_of_heads = 16;
> -        s->sectors_per_track = 63;
> +    /* Reserver space for MBR */
> +    if (!floppy && !qemu_opt_get_bool(opts, "no-mbr", false)) {
> +        s->offset_to_bootsector = 0x3f;
>      }
>  
> -
>      s->bs = bs;
>  
> -    /* LATER TODO: if FAT32, adjust */
> -    s->sectors_per_cluster=0x10;
> -
>      s->current_cluster=0xffffffff;
>  
>      s->qcow = NULL;
> -- 
> 2.34.1
> 

Re: [PATCH 5/5] vvfat: add support for "size" options

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
>> This allows more flexibility to vvfat backend. The value for "Number of
>> Heads" and "Sectors per track" are based on SD specifications Part 2.

This is too terse to remind me of how vvfat picks cylinders, heads, and
sectors before this patch, so I need to go dig through the source code.
I figure it depends on configuration parameters @floppy and @fat-type
like this:

    floppy  fat-type    cyls heads secs   cyls*heads*secs*512
    false      12         64    16   63         31.5 MiB
    false      16       1024    16   63        504   MiB
    false      32       1024    16   63        504   MiB
    true       12         80     2   18       1440   KiB
    true       16         80     2   36       2880   KiB
    true       32         80     2   36       2880   KiB

How exactly does the new parameter @size change this?

>> Some limitations remains, the size parameter is recognized only when
>> "format=vvfat" is passed. In particular, "format=raw,size=xxx" is
>> keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
>> FAT12. FAT32 has not been adjusted and thus still default to 504MB.

31.5MiB unless I'm mistaken.

I'm not sure what you're trying to convey in this paragraph.  As far as
I can tell, you're adding a @size parameter to vvfat, so of course it
doesn't affect raw.

>> Moreover, for flopyy, size=1M is creating a disk 1.44 MB, and size=2M a

floppy

>> disk of 2.88 MB. This avoids having to worry about float operations.

More on this part below.

>> Signed-off-by: Clément Chigot <chigot@adacore.com>
>> ---
>>  block/vvfat.c | 165 ++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 141 insertions(+), 24 deletions(-)
>> 
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 6526c585a2..4537c39d5c 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1091,6 +1091,11 @@ static QemuOptsList runtime_opts = {
>>              .type = QEMU_OPT_BOOL,
>>              .help = "Do not add a Master Boot Record on this disk",
>>          },
>> +        {
>> +            .name = BLOCK_OPT_SIZE,
>> +            .type = QEMU_OPT_SIZE,
>> +            .help = "Virtual disk size"
>> +        },
>>          { /* end of list */ }
>>      },
>>  };
>
> Like in patch 1, you need additional changes, in particular to add the
> option to the QAPI schema in qapi/block-core.json.
>
>> @@ -1148,10 +1153,141 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
>>      qdict_put_bool(options, "no-mbr", no_mbr);
>>  }
>>  
>> +static void vvfat_get_size_parameters(uint64_t size, BDRVVVFATState *s,
>> +                                      bool floppy, Error **errp)
>> +{
>> +    if (floppy) {
>> +        /*
>> +         * Floppy emulation only supports 1.44 MB or 2.88 MB (default).
>> +         * In order to avoid floating operations ambiguity, 1 MB is
>> +         * recognized for 1.44 MB and 2 MB for 2.88 MB.
>> +         */
>> +        if (!size) {
>> +            size = 2 * 1024 * 1024;
>> +        } else {
>> +            if (size == 1024 * 1024 && s->fat_type == 16) {
>> +                error_setg(errp,
>> +                           "floppy FAT16 unsupported size; only support 2M "
>> +                           "(for an effective size of 2.88 MB)");
>> +            } else if (size != 2 * 1024 * 1024 && size != 1024 * 1024) {
>> +                error_setg(errp,
>> +                           "floppy unsupported size; should be 1MB (for "
>> +                           "an effective size of 1.44 MB) or 2.88M (for "
>> +                           "2.88MB)");
>> +            }
>> +        }
>
> This is horrible. To be fair, it's pretty hard to do something not
> horrible when the usual units to describe floppy sizes are already
> horrible. :-)

Yes :)

> But I'd still like us to do better here.
>
> To me it looks a bit like what we really want is an enum for floppy
> sizes (though is there any real reason why we have only those two?), but
> an arbitrary size for hard disks.
>
> Without the enum, obviously, users could specify 1440k and that would do
> the right thing. Maybe special casing whatever 1.44M and 2.88M result
> in and translating them into 1440k and 2880k could be more justifiable
> than special casing 1M and 2M, but it would still be ugly.
>
> Markus, do you have any advice how this should be represented in QAPI?

Maybe, but first I'd like to understand what @size does.

Re: [PATCH 5/5] vvfat: add support for "size" options

[Clément Chigot]

On Fri, Oct 24, 2025 at 10:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> >> This allows more flexibility to vvfat backend. The value for "Number of
> >> Heads" and "Sectors per track" are based on SD specifications Part 2.
>
> This is too terse to remind me of how vvfat picks cylinders, heads, and
> sectors before this patch, so I need to go dig through the source code.
> I figure it depends on configuration parameters @floppy and @fat-type
> like this:
>
>     floppy  fat-type    cyls heads secs   cyls*heads*secs*512
>     false      12         64    16   63         31.5 MiB
>     false      16       1024    16   63        504   MiB
>     false      32       1024    16   63        504   MiB
>     true       12         80     2   18       1440   KiB
>     true       16         80     2   36       2880   KiB
>     true       32         80     2   36       2880   KiB
>
> How exactly does the new parameter @size change this?

My prime goal was to create a 256 Mib VVFAT disk. As you can see,
today for hard-disks there are only two possibilities: 31.5 Mib or 504
Mib. Hence, I've introduced the option `size=xxx` to allow more
granular choices.
This option changes how cyls, heads and secs parameters are computed
to be as closed as possible of its value.

I did try to keep it simple. I could have introduced options to select
cylinders, heads, etc. But I think "size=xxx" would be more intuitive.
There are also approximations made, as not all sizes can be reached. I
didn't add errors or warnings for them. I'm fine adding them.

> >> Some limitations remains, the size parameter is recognized only when
> >> "format=vvfat" is passed. In particular, "format=raw,size=xxx" is
> >> keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
> >> FAT12. FAT32 has not been adjusted and thus still default to 504MB.
>
> 31.5MiB unless I'm mistaken.

True, I will fix it.

> I'm not sure what you're trying to convey in this paragraph.  As far as
> I can tell, you're adding a @size parameter to vvfat, so of course it
> doesn't affect raw.

Yes, but AFAICT, `if=sd,format=raw` will result in vvfat backend being
called. I didn't manage to make the new option work with
`if=sd,format=raw,size=256Mb`. Thus, when the "size" option is not
provided, I keep the previous value (those for your above comment).
Hence this paragraph to mostly warn people about the current
limitation.

> >> Moreover, for flopyy, size=1M is creating a disk 1.44 MB, and size=2M a
>
> floppy
>
> >> disk of 2.88 MB. This avoids having to worry about float operations.
>
> More on this part below.
>
> >> Signed-off-by: Clément Chigot <chigot@adacore.com>
> >> ---
> >>  block/vvfat.c | 165 ++++++++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 141 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/block/vvfat.c b/block/vvfat.c
> >> index 6526c585a2..4537c39d5c 100644
> >> --- a/block/vvfat.c
> >> +++ b/block/vvfat.c
> >> @@ -1091,6 +1091,11 @@ static QemuOptsList runtime_opts = {
> >>              .type = QEMU_OPT_BOOL,
> >>              .help = "Do not add a Master Boot Record on this disk",
> >>          },
> >> +        {
> >> +            .name = BLOCK_OPT_SIZE,
> >> +            .type = QEMU_OPT_SIZE,
> >> +            .help = "Virtual disk size"
> >> +        },
> >>          { /* end of list */ }
> >>      },
> >>  };
> >
> > Like in patch 1, you need additional changes, in particular to add the
> > option to the QAPI schema in qapi/block-core.json.
> >
> >> @@ -1148,10 +1153,141 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
> >>      qdict_put_bool(options, "no-mbr", no_mbr);
> >>  }
> >>
> >> +static void vvfat_get_size_parameters(uint64_t size, BDRVVVFATState *s,
> >> +                                      bool floppy, Error **errp)
> >> +{
> >> +    if (floppy) {
> >> +        /*
> >> +         * Floppy emulation only supports 1.44 MB or 2.88 MB (default).
> >> +         * In order to avoid floating operations ambiguity, 1 MB is
> >> +         * recognized for 1.44 MB and 2 MB for 2.88 MB.
> >> +         */
> >> +        if (!size) {
> >> +            size = 2 * 1024 * 1024;
> >> +        } else {
> >> +            if (size == 1024 * 1024 && s->fat_type == 16) {
> >> +                error_setg(errp,
> >> +                           "floppy FAT16 unsupported size; only support 2M "
> >> +                           "(for an effective size of 2.88 MB)");
> >> +            } else if (size != 2 * 1024 * 1024 && size != 1024 * 1024) {
> >> +                error_setg(errp,
> >> +                           "floppy unsupported size; should be 1MB (for "
> >> +                           "an effective size of 1.44 MB) or 2.88M (for "
> >> +                           "2.88MB)");
> >> +            }
> >> +        }
> >
> > This is horrible. To be fair, it's pretty hard to do something not
> > horrible when the usual units to describe floppy sizes are already
> > horrible. :-)
>
> Yes :)

I did have a first version that ignored this new size option for
floppy. I did extend it because why not. But if you find it will bring
too much complexity I can bring it back.

> > But I'd still like us to do better here.
> >
> > To me it looks a bit like what we really want is an enum for floppy
> > sizes (though is there any real reason why we have only those two?), but
> > an arbitrary size for hard disks.
> >
> > Without the enum, obviously, users could specify 1440k and that would do
> > the right thing. Maybe special casing whatever 1.44M and 2.88M result
> > in and translating them into 1440k and 2880k could be more justifiable
> > than special casing 1M and 2M, but it would still be ugly.
> >
> > Markus, do you have any advice how this should be represented in QAPI?
>
> Maybe, but first I'd like to understand what @size does.
>

Re: [PATCH 5/5] vvfat: add support for "size" options

[Markus Armbruster]

Clément Chigot <chigot@adacore.com> writes:

> On Fri, Oct 24, 2025 at 10:35 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Kevin Wolf <kwolf@redhat.com> writes:
>>
>> > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
>> >> This allows more flexibility to vvfat backend. The value for "Number of
>> >> Heads" and "Sectors per track" are based on SD specifications Part 2.
>>
>> This is too terse to remind me of how vvfat picks cylinders, heads, and
>> sectors before this patch, so I need to go dig through the source code.
>> I figure it depends on configuration parameters @floppy and @fat-type
>> like this:
>>
>>     floppy  fat-type    cyls heads secs   cyls*heads*secs*512
>>     false      12         64    16   63         31.5 MiB
>>     false      16       1024    16   63        504   MiB
>>     false      32       1024    16   63        504   MiB
>>     true       12         80     2   18       1440   KiB
>>     true       16         80     2   36       2880   KiB
>>     true       32         80     2   36       2880   KiB
>>
>> How exactly does the new parameter @size change this?
>
> My prime goal was to create a 256 Mib VVFAT disk. As you can see,
> today for hard-disks there are only two possibilities: 31.5 Mib or 504
> Mib. Hence, I've introduced the option `size=xxx` to allow more
> granular choices.
> This option changes how cyls, heads and secs parameters are computed
> to be as closed as possible of its value.
>
> I did try to keep it simple. I could have introduced options to select
> cylinders, heads, etc. But I think "size=xxx" would be more intuitive.
> There are also approximations made, as not all sizes can be reached. I
> didn't add errors or warnings for them. I'm fine adding them.

I don't have an opinion on whether we should support more sizes and/or
provide full control over CHS geometry.

>> >> Some limitations remains, the size parameter is recognized only when
>> >> "format=vvfat" is passed. In particular, "format=raw,size=xxx" is
>> >> keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
>> >> FAT12. FAT32 has not been adjusted and thus still default to 504MB.
>>
>> 31.5MiB unless I'm mistaken.
>
> True, I will fix it.
>
>> I'm not sure what you're trying to convey in this paragraph.  As far as
>> I can tell, you're adding a @size parameter to vvfat, so of course it
>> doesn't affect raw.
>
> Yes, but AFAICT, `if=sd,format=raw` will result in vvfat backend being
> called. I didn't manage to make the new option work with
> `if=sd,format=raw,size=256Mb`. Thus, when the "size" option is not
> provided, I keep the previous value (those for your above comment).
> Hence this paragraph to mostly warn people about the current
> limitation.

Are you talking about -drive?

Complete examples, please.

I'm confused about the connection between SD (from if=sd here, and "SD
specification" above) and vvfat.  SD is a frontend.  vvfat is a backend.

[...]

Re: [PATCH 5/5] vvfat: add support for "size" options

[Clément Chigot]

On Mon, Oct 27, 2025 at 1:09 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Clément Chigot <chigot@adacore.com> writes:
>
> > On Fri, Oct 24, 2025 at 10:35 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Am 03.09.2025 um 09:57 hat Clément Chigot geschrieben:
> >> >> This allows more flexibility to vvfat backend. The value for "Number of
> >> >> Heads" and "Sectors per track" are based on SD specifications Part 2.
> >>
> >> This is too terse to remind me of how vvfat picks cylinders, heads, and
> >> sectors before this patch, so I need to go dig through the source code.
> >> I figure it depends on configuration parameters @floppy and @fat-type
> >> like this:
> >>
> >>     floppy  fat-type    cyls heads secs   cyls*heads*secs*512
> >>     false      12         64    16   63         31.5 MiB
> >>     false      16       1024    16   63        504   MiB
> >>     false      32       1024    16   63        504   MiB
> >>     true       12         80     2   18       1440   KiB
> >>     true       16         80     2   36       2880   KiB
> >>     true       32         80     2   36       2880   KiB
> >>
> >> How exactly does the new parameter @size change this?
> >
> > My prime goal was to create a 256 Mib VVFAT disk. As you can see,
> > today for hard-disks there are only two possibilities: 31.5 Mib or 504
> > Mib. Hence, I've introduced the option `size=xxx` to allow more
> > granular choices.
> > This option changes how cyls, heads and secs parameters are computed
> > to be as closed as possible of its value.
> >
> > I did try to keep it simple. I could have introduced options to select
> > cylinders, heads, etc. But I think "size=xxx" would be more intuitive.
> > There are also approximations made, as not all sizes can be reached. I
> > didn't add errors or warnings for them. I'm fine adding them.
>
> I don't have an opinion on whether we should support more sizes and/or
> provide full control over CHS geometry.
>
> >> >> Some limitations remains, the size parameter is recognized only when
> >> >> "format=vvfat" is passed. In particular, "format=raw,size=xxx" is
> >> >> keeping the previously hardcoded value: 504MB for FAT16 and 32 MB for
> >> >> FAT12. FAT32 has not been adjusted and thus still default to 504MB.
> >>
> >> 31.5MiB unless I'm mistaken.
> >
> > True, I will fix it.
> >
> >> I'm not sure what you're trying to convey in this paragraph.  As far as
> >> I can tell, you're adding a @size parameter to vvfat, so of course it
> >> doesn't affect raw.
> >
> > Yes, but AFAICT, `if=sd,format=raw` will result in vvfat backend being
> > called. I didn't manage to make the new option work with
> > `if=sd,format=raw,size=256Mb`. Thus, when the "size" option is not
> > provided, I keep the previous value (those for your above comment).
> > Hence this paragraph to mostly warn people about the current
> > limitation.
>
> Are you talking about -drive?
>
> Complete examples, please.
>
> I'm confused about the connection between SD (from if=sd here, and "SD
> specification" above) and vvfat.  SD is a frontend.  vvfat is a backend.

Alright, I'll try to explain how I came up with this patch. And sorry
if it's a bit blurry, I made it some months ago hence I don't remember
all the details...
So, first, my prime goal was to access a local folder in a QNX system
running on Raspi 4B emulation.
My usual way to pass such a local folder is through `-drive
file=fat:rw:<host_folder>,format=raw`. For virt, it's usually
connected to virtio-blk-device: `-drive id=disk0,if=none,... -device
virtio-blk-device,drive=disk0`. For the Raspi 4b, adding the
virtio-blk-device is not possible, hence I have to connect it as a SD
card: `-drive if=sd,...`.

However, without any `size=` argument, QEMU will complain that the SD
card has not a valid size:
  | (host) $ qemu-system-aarch64 -M raspi4b -kernel raspi4b-kernel
-nographic -no-reboot -append "earlycon=pl011,mmio32,0xfe201000
console=ttyAMA0 noreboot" -dtb bcm2711-rpi-4-b.dtb -initrd
rootfs.cpio.gz  -drive
id=sdcard,file=fat:rw:<host_folder>,format=raw,if=sd
  | qemu-system-aarch64: Invalid SD card size: 504 MiB
  | SD card size has to be a power of 2, e.g. 512 MiB.
  | You can resize disk images with 'qemu-img resize <imagefile> <new-size>'
  | (note that this will lose data if you make the image smaller than
it currently is).

("raspi4b-kernel", the dtb and the rootfs come from
functional/aarch64/test_raspi4.py)

Hence, I've added `size=256M` to reduce the size of that SD card and
make QEMU happy. This allows me to mount my host folder on Linux:
  | (host) $ qemu-system-aarch64 -M raspi4b -kernel raspi4b-kernel
-nographic -no-reboot -append "earlycon=pl011,mmio32,0xfe201000
console=ttyAMA0 noreboot" -dtb bcm2711-rpi-4-b.dtb -initrd
rootfs.cpio.gz  -drive
id=sdcard,file=fat:rw:<host_folder>,format=raw,if=sd,size=256M
  | (QEMU) # fdisk -l /dev/mmcblk1
  | Disk /dev/mmcblk1: 256 MB, 268435456 bytes, 524288 sectors
  | 520 cylinders, 16 heads, 63 sectors/track
  | Units: sectors of 1 * 512 = 512 bytes
  |
  | Device       Boot StartCHS    EndCHS        StartLBA     EndLBA
Sectors  Size Id Type
  | /dev/mmcblk1p1 *  0,1,1       1023,15,63          63    1032191
1032129  503M  6 FAT16

As you can see the "Disk" has the right size (256MB) but the partition
still has a 503M size. However, Linux doesn't seem to care too much
about that as I'm able to mount this partition, and perform IO
operations.
  | (QEMU) # mount /dev/mmcblk1p1 /mnt
  | (QEMU) # ls /mnt
  | file.txt
  | (QEMU) # cat /mnt/file.txt
  | Hello World
  | (QEMU) # echo "OK" > /mnt/test.txt
  | (host) $ cat <host_folder>/test.txt
  | OK

Now, QNX comes into play.
First, the SD card must be connected to another bus. That patch is not
part of this series as I'm considering it a QNX issue. Just FTR here
is the patch:
  | --- a/hw/arm/bcm2838_peripherals.c
  | +++ b/hw/arm/bcm2838_peripherals.c
  | @@ -190,7 +190,7 @@ static void
bcm2838_peripherals_realize(DeviceState *dev, Error **errp)
  |          &s_base->peri_mr, GPIO_OFFSET,
  |          sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gpio), 0));
  |
  | -    object_property_add_alias(OBJECT(s), "sd-bus",
OBJECT(&s->gpio), "sd-bus");
  | +    object_property_add_alias(OBJECT(s), "sd-bus",
OBJECT(&s->emmc2), "sd-bus");
  |
  |      /* BCM2838 RPiVid ASB must be mapped to prevent kernel crash */

Afterwards, QNX is able to see the SD card, but not able to mount it.
It complains about the filesystem being corrupted. Looking at `fdisk`
output, it shows a mismatch between the "total section" value and the
Cylinders/Heads/etc values.
  | (QEMU) # fdisk /dev/hd0 info
  | Physical disk characteristics: (/dev/hd0)
  |     Disk type        : Direct Access (0)
  |     Cylinders        : 520
  |     Heads            : 16
  |     Sectors/Track    : 63
  |     Total Sectors    : 524288
  |     Block Size       : 512
  |
  | Warning: total sectors field does not agree with
  |            cylinders*sectors/track*heads!! (524288 vs 524160)

The "no-mbr" option introduced in patch 1 is something we (Adacore's
QEMU team) have for a long time. I don't remember the details but we
are using it for other OSes as well (notably RTEMS).
Once added, and the drive command line updated for `-drive
id=sdcard,file=fat:rw:no-mbr:<host_folder>,format=raw,if=sd,size=256M`,
I don't have this warning anymore. Though, I'm still getting the
corrupted filesystem error.

Afterwards, it's a bit blurry but I think by trial and errors we ended
up removing the SD size error and realize that `-drive
id=sdcard,file=fat:rw:no-mbr:<host_folder>,format=raw,if=sd` was
working. However, `size=256M` still results in a corrupted filesystem.
As a comment in vvfat.c states that it either creates a "32MB or 504
MB disk". I decided to check if I can adapt, hence this patch.

I didn't find any VFAT documentation explaining the relation between
the size and the cylinders, heads, sector per track values. However,
the SD documentation was giving some recommandations, hence I used it
as a base.

I was unable to make `vvfat.c` recognize the "size" argument passed
along `format=raw`, even if hardcoding the value in `vvfat.c` did make
a difference. And that's why I'm adding the warning in the commit
message.

I've also realized that following my patch, the mismatch in between
the disk and the partition in Linux was going away when using `-drive
format=vvfat,size=xxx`. Making it not just QNX-oriented.
  | (host) $ qemu-system-aarch64 -M raspi4b -kernel raspi4b-kernel
-nographic -no-reboot -append "earlycon=pl011,mmio32,0xfe201000
console=ttyAMA0 noreboot" -dtb bcm2711-rpi-4-b.dtb -initrd
rootfs.cpio.gz  -drive
id=sdcard,file=fat:rw:<host_folder>,format=vvfat,if=sd,size=256M
  | (QEMU) # fdisk -l /dev/mmcblk1
  | Disk /dev/mmcblk1: 256 MB, 268435456 bytes, 524288 sectors
  | 1024 cylinders, 16 heads, 32 sectors/track
  | Units: sectors of 1 * 512 = 512 bytes
  |
  | Device       Boot StartCHS    EndCHS        StartLBA     EndLBA
Sectors  Size Id Type
  | /dev/mmcblk1p1 *  0,1,32      1023,15,32          63     524287
 524225  255M  6 FAT16

I probably missed a few things, but I hope this is clearer to you why
and how this series was made.

Re: [PATCH 0/5] block/vvfat: introduce "size" option

[Clément Chigot]

On Wed, Sep 3, 2025 at 9:57 AM Clément Chigot <chigot@adacore.com> wrote:
>
> The main goal of this series is to introduce a new option "size" within
> the vvfat backend (patch 5). It allows more control over SD cards' size.
> The value for "Number of Heads" and "Sectors per track" are based on SD
> specifications Part 2.
>
> This series also includes minor patches:
>  - patch 1 introduces another option to remove the Master Boot Record
>    (this is mandatory for QNX)
>  - patch 2-4 are minor improvements easing the introducing of "size"
>    option
>
> This was tested on with a aarch64-linux kernel taken from
> functional/aarch64/test-virt and on aarch64-qnx over raspi4b with a
> workaround, not included here (the SD bus must be associated to the EMMC2
> port instead of through GPIOs).

Gentle ping for this series.

TIA
Clément

> Clément Chigot (5):
>   vvfat: introduce no-mbr option
>   vvfat: move fat_type check prior to size setup
>   vvfat: add a define for SECTOR_SIZE
>   vvfat: move size parameters within driver structure
>   vvfat: add support for "size" options
>
>  block/vvfat.c | 279 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 213 insertions(+), 66 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH 0/5] block/vvfat: introduce "size" option

[Clément Chigot]

On Wed, Sep 3, 2025 at 9:57 AM Clément Chigot <chigot@adacore.com> wrote:
>
> The main goal of this series is to introduce a new option "size" within
> the vvfat backend (patch 5). It allows more control over SD cards' size.
> The value for "Number of Heads" and "Sectors per track" are based on SD
> specifications Part 2.
>
> This series also includes minor patches:
>  - patch 1 introduces another option to remove the Master Boot Record
>    (this is mandatory for QNX)
>  - patch 2-4 are minor improvements easing the introducing of "size"
>    option
>
> This was tested on with a aarch64-linux kernel taken from
> functional/aarch64/test-virt and on aarch64-qnx over raspi4b with a
> workaround, not included here (the SD bus must be associated to the EMMC2
> port instead of through GPIOs).

Gentle ping for this series.

TIA
Clément

> Clément Chigot (5):
>   vvfat: introduce no-mbr option
>   vvfat: move fat_type check prior to size setup
>   vvfat: add a define for SECTOR_SIZE
>   vvfat: move size parameters within driver structure
>   vvfat: add support for "size" options
>
>  block/vvfat.c | 279 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 213 insertions(+), 66 deletions(-)
>
> --
> 2.34.1
>