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

["Clément Chigot" <chigot@adacore.com>]

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.
>