Re: [Qemu-devel] [PATCH] ide: refuse WIN_READ_NATIVE_MAX on empty device

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Apr 5, 2013 at 2:57 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> On Fri, Apr 05, 2013 at 12:56:07PM +0200, Markus Armbruster wrote:
>>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>>
>>> > What is the highest addressable sector on an empty CD-ROM?  Nothing is
>>> > addressable so produce an error.
>>> >
>>> > This patch prevents a divide-by-zero in ide_set_sector() since
>>> > s->sectors and s->heads would be 0.  Not to mention that a sector=-1
>>> > argument would be nonsense.
>>> >
>>> > Note that WIN_READ_NATIVE_MAX can be triggered using hdparm -N 1024
>>> > /dev/cdrom.  The LBA bit will be set to 1 though, so the only easy way
>>> > to go down the ide_set_sector() CHS code path which divides by zero is
>>> > to comment out the s->select & 0x40 case for testing.
>>>
>>> Suggests you did that.
>>>
>>> Have you tried the reproducer with a physical drive?  Does it fail the
>>> command when empty, too?
>>
>> Believe it or not, I don't have access to an ATAPI CD-ROM drive.  Would
>> you be able to try out hdparm -N 1024 /dev/cdrom?
>>
>> Note that READ NATIVE MAX is optional, real drives may not implement it
>> since it seems geared towards the Host Protected Area feature which
>> makes no sense on CD-ROMs.  (The idea is a reserved area on the disk
>> where system data can be stored and the OS will not touch it.)
>>
>> Stefan
>
> # hdparm -N /dev/cdrom
>
> /dev/cdrom:
>  READ_NATIVE_MAX_ADDRESS failed: Input/output error
> # hdparm -N 1024 /dev/cdrom
>
> /dev/cdrom:
>  setting max visible sectors to 1024 (temporary)
>  READ_NATIVE_MAX_ADDRESS failed: Input/output error
>  READ_NATIVE_MAX_ADDRESS failed: Input/output error
>
> Same with and without media.
>
> If the command makes no sense for CD-ROMs, and generally isn't
> implemented by them, we should consider not implementing either, by
> clearing its IDE_CD bit in ide_cmd_table.

Thank you!

You're getting the same result that we get in the guest.  This looks good.

There's no harm in supporting READ NATIVE MAX when the CD-ROM is
inserted.  It's basically another way of finding out the block device
size.

I also like this fix better than blacklisting the command since it now
protects us in case HD or CFA devices ever have nb_sectors = 0 too.
In other words, it's more defensive than just fixing CD-ROMs.

Stefan