Re: [Qemu-devel] [PATCH] block: handle "rechs" translation option

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> The rechs translation option is so obscure that we support it but do

"Support" is a rather strong word:

    $ git-grep -i rechs
    include/hw/block/block.h:#define BIOS_ATA_TRANSLATION_RECHS  4

> not even attempt to parse it.  Archeologists will surely be puzzled
> by this (assuming they care about QEMU and CHS translation), so fix it.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  blockdev.c |  2 ++
>  vl.c       | 18 ++++++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 36ceece..5946bbe 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -776,6 +776,8 @@ DriveInfo *drive_init(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>              translation = BIOS_ATA_TRANSLATION_NONE;
>          } else if (!strcmp(value, "lba")) {
>              translation = BIOS_ATA_TRANSLATION_LBA;
> +        } else if (!strcmp(value, "rechs")) {
> +            translation = BIOS_ATA_TRANSLATION_RECHS;
>          } else if (!strcmp(value, "auto")) {
>              translation = BIOS_ATA_TRANSLATION_AUTO;
>          } else {

This is -drive parameter "trans", kept for backward compatibility.

> diff --git a/vl.c b/vl.c
> index 7f4fe0d..f161a727f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3053,14 +3053,17 @@ int main(int argc, char **argv, char **envp)
>                          goto chs_fail;
>                      if (*p == ',') {
>                          p++;
> -                        if (!strcmp(p, "none"))
> +                        if (!strcmp(p, "none")) {
>                              translation = BIOS_ATA_TRANSLATION_NONE;
> -                        else if (!strcmp(p, "lba"))
> +                        } else if (!strcmp(p, "lba")) {
>                              translation = BIOS_ATA_TRANSLATION_LBA;
> -                        else if (!strcmp(p, "auto"))
> +                        } else if (!strcmp(p, "rechs")) {
> +                            translation = BIOS_ATA_TRANSLATION_RECHS;
> +                        } else if (!strcmp(p, "auto")) {
>                              translation = BIOS_ATA_TRANSLATION_AUTO;
> -                        else
> +                        } else {
>                              goto chs_fail;
> +                        }
>                      } else if (*p != '\0') {
>                      chs_fail:
>                          fprintf(stderr, "qemu: invalid physical CHS 
> format\n");
> @@ -3074,10 +3077,13 @@ int main(int argc, char **argv, char **envp)
>                          qemu_opt_set(hda_opts, "heads", num);
>                          snprintf(num, sizeof(num), "%d", secs);
>                          qemu_opt_set(hda_opts, "secs", num);
> -                        if (translation == BIOS_ATA_TRANSLATION_LBA)
> +                        if (translation == BIOS_ATA_TRANSLATION_RECHS) {
> +                            qemu_opt_set(hda_opts, "trans", "rechs");
> +                        } else if (translation == BIOS_ATA_TRANSLATION_LBA) {
>                              qemu_opt_set(hda_opts, "trans", "lba");
> -                        if (translation == BIOS_ATA_TRANSLATION_NONE)
> +                        } else if (translation == BIOS_ATA_TRANSLATION_NONE) 
> {
>                              qemu_opt_set(hda_opts, "trans", "none");
> +                        }
>                      }
>                  }
>                  break;

This is -hdachs.  So crusty you should wear protective clothing when you
touch it.

The right way to control IDE geometry translation is ide-hd property
bios-chs-trans.  Unfortunately, you missed that one entirely.  Have a
gander at bios_chs_trans_table[] in hw/core/qdev-properties.c.

We also don't let the user ask for BIOS_ATA_TRANSLATION_LARGE.  Its only
source is hd_geometry_guess().  I have no idea whether any user would
want to set it.  But that applies to "rechs", too :)