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

[Markus Armbruster <armbru@redhat.com>]

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

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

Fun!

> ("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

520 * 16 * 63 is 524160 sectors, 128 less than the 524288 reported.  I
figure that's harmless.  Only ancient software should look at CHS, and
losing a few sectors with ancient software is fine.

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

The partition table's EndLBA is 1032191 even though the disk has only
524288 cylinders.  Scary!

Its EndCHS is consistent with its EndLBA: 1024*16*63 = 1032191 + 1.

>                        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

Have you tried with a host folder containing more than 256MiB?  What
happens if you try to read all of it?

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

I'm not sure this mismatch causes the problem.  I suspect the bogus
EndLBA does.

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

Suppressing MBR initialization avoids the partially bogus partition
table.

But if we create a partition table, it better make sense, don't you
think?

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

This one:

    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.

> 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

EndLBA matches disk size.  EndCHS still matches EndLBA.  Good.

The difference to the command line you showed above is format=raw (bad)
vs. format=vvfat (good).

With format=raw,size=256M you get that size, but a bogus partition
table.

With format=vvfat,size=256M you get that size, and a sensible partition
table.

Correct?

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