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

[Markus Armbruster <armbru@redhat.com>]

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.