Re: [SeaBIOS] [PATCH v2] ramdisk: search for all available floppy images instead of one

[Kevin O'Connor]

On Thu, Dec 06, 2018 at 07:12:19PM +0300, Mike Banon wrote:
> Hi Kevin, thank you very much for the suggestions! This is a second
> version of my "more_than_one_floppy" patch : now it will search for
> the max size of floppy stored inside CBFS, then allocate this size of
> RAM and use it later for any floppy selected by user. sha256 =
> 2f8bb18c1606d437de49e585769f100c085234de29d8520e2dc74d25b7f7645a

Thanks, see my comments below.

[...]
> ---
> diff --git a/src/block.h b/src/block.h
> index f64e880..aaa236f 100644
> --- a/src/block.h
> +++ b/src/block.h
> @@ -2,7 +2,7 @@
>  #define __BLOCK_H
>
> #include "types.h" // u32
> -
> +#include "romfile.h" // struct romfile_s
>
> /****************************************************************
>   * Disk command request
> @@ -48,6 +48,7 @@ struct drive_s {
>  struct drive_s {
>      u8 type;            // Driver type (DTYPE_*)
>      u8 floppy_type;     // Type of floppy (only for floppy drives).
> +    struct romfile_s *floppy_file; // Floppy file (only for virtual floppies).
>      struct chs_s lchs;  // Logical CHS
>      u64 sectors;        // Total sectors count
>      u32 cntl_id;        // Unique id for a given driver type.

Driver specific fields should not be added to drive_s - the code
should use the container_of() trick to allocate driver specific
fields.  (Create and allocate a custom driver struct that contains a
'struct drive_s' and when called with a drive_s use container_of() to
get a pointer back to that main struct - for an example, take a look
at what src/hw/usb-msc.c does.)
> diff --git a/src/boot.c b/src/boot.c
> index 9f82f3c..79f1e7d 100644
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -584,7 +584,7 @@ bcv_prepboot(void)
>              break;
>          case IPL_TYPE_FLOPPY:
>              map_floppy_drive(pos->drive);
> -            add_bev(IPL_TYPE_FLOPPY, 0);
> +            add_bev(IPL_TYPE_FLOPPY, (u32)pos->drive);
>              break;
>          case IPL_TYPE_HARDDISK:
>              map_hd_drive(pos->drive);
> @@ -733,6 +733,12 @@ do_boot(int seq_nr)
>  static void
>  do_boot(int seq_nr)
>  {
> +
> +    int ret;
> +    void *pos;
> +    struct romfile_s *file;
> +    struct drive_s *drive;
> +
>      if (! CONFIG_BOOT)
>          panic("Boot support not compiled in.\n");
>
> @@ -744,6 +750,16 @@ do_boot(int seq_nr)
>      switch (ie->type) {
>      case IPL_TYPE_FLOPPY:
>          printf("Booting from Floppy...\n");
> +        drive = (struct drive_s *)ie->vector;
> +        file = drive->floppy_file;
> +        // File is NULL if a floppy is physical.
> +        if (file) {
> +            // Copy virtual floppy image into ram.
> +            pos = (void *)drive->cntl_id;
> +            ret = file->copy(file, pos, file->size);
> +            if (ret < 0)
> +                break;
> +        }

Similarly, we shouldn't add driver specific code into the boot phase.
A better place to do this would be during the BCV mapping phase (eg,
bcv_prepboot() ).  The idea is to do the work after the boot menu, but
before the boot phase.  Ideally the code would detect the driver wants
a callback and then call driver specific code that resides in
src/hw/ramdisk.c.

-Kevin