block: handle "rechs" translation option

The rechs translation option is so obscure that we support it but do
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 <pbonzini@redhat.com>
---
 blockdev.c |  2 ++
 vl.c       | 18 ++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

Paolo Bonzini <pbonzini@redhat.com> 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 <pbonzini@redhat.com>
> ---
>  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 :)

On Thu, Jan 30, 2014 at 02:08:02PM +0100, Paolo Bonzini wrote:
> The rechs translation option is so obscure that we support it but do
> 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 <pbonzini@redhat.com>
> ---
>  blockdev.c |  2 ++
>  vl.c       | 18 ++++++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)

Is my understanding correct that QEMU actually does nothing with RECHS except
poke a magic number into CMOS memory when x86 guests start?

(I ask because we don't even use the constant:
$ git grep _RECHS
include/hw/block/block.h:#define BIOS_ATA_TRANSLATION_RECHS  4)

Please also add changes for:

blockdev.c:
QemuOptsList qemu_legacy_drive_opts = {
        ...
        },{
            .name = "trans",
            .type = QEMU_OPT_STRING,
            .help = "chs translation (auto, lba, none)",
        },{

qemu-options.hx:
Force hard disk 0 physical geometry (1 <= @var{c} <= 16383, 1 <=
@var{h} <= 16, 1 <= @var{s} <= 63) and optionally force the BIOS
translation mode (@var{t}=none, lba or auto).

Il 30/01/2014 15:37, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> 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

As Stefan said, all we do is pass the info to the BIOS.

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

This one is handled by the other series I sent today.  That's how I 
found it.

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

Yeah, you are right that this patch should cover "large" as well.  Will 
send v2.

Paolo