Re: [PATCH 1/2] hw/ide/core.c (cmd_read_native_max): Avoid limited device parameters

[John Snow <jsnow@redhat.com>]

On Mon, Oct 10, 2022 at 4:52 AM Lev Kujawski <lkujaw@mailbox.org> wrote:
>
> Always use the native CHS device parameters for the ATA commands READ
> NATIVE MAX ADDRESS and READ NATIVE MAX ADDRESS EXT, not those limited
> by the ATA command INITIALIZE_DEVICE_PARAMETERS (introduced in patch
> 176e4961, hw/ide/core.c: Implement ATA INITIALIZE_DEVICE_PARAMETERS
> command, 2022-07-07.)
>
> As stated by the ATA/ATAPI specification, "[t]he native maximum is the
> highest address accepted by the device in the factory default
> condition."  Therefore this patch substitutes the native values in
> drive_heads and drive_sectors before calling ide_set_sector().
>
> One consequence of the prior behavior was that setting zero sectors
> per track could lead to an FPE within ide_set_sector().  Thanks to
> Alexander Bulekov for reporting this issue.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1243
> Signed-off-by: Lev Kujawski <lkujaw@mailbox.org>

Does this need attention?

--js

> ---
>  hw/ide/core.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 39afdc0006..ee836401bc 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1608,11 +1608,24 @@ static bool cmd_read_native_max(IDEState *s, uint8_t cmd)
>      /* Refuse if no sectors are addressable (e.g. medium not inserted) */
>      if (s->nb_sectors == 0) {
>          ide_abort_command(s);
> -        return true;
> -    }
> +    } else {
> +        /*
> +         * Save the active drive parameters, which may have been
> +         * limited from their native counterparts by, e.g., INITIALIZE
> +         * DEVICE PARAMETERS or SET MAX ADDRESS.
> +         */
> +        const int aheads = s->heads;
> +        const int asectors = s->sectors;
>
> -    ide_cmd_lba48_transform(s, lba48);
> -    ide_set_sector(s, s->nb_sectors - 1);
> +        s->heads = s->drive_heads;
> +        s->sectors = s->drive_sectors;
> +
> +        ide_cmd_lba48_transform(s, lba48);
> +        ide_set_sector(s, s->nb_sectors - 1);
> +
> +        s->heads = aheads;
> +        s->sectors = asectors;
> +    }
>
>      return true;
>  }
> --
> 2.34.1
>