Re: [hatari-devel] Patch: IDE support for sector sizes > 512 bytes

[Uwe Seimet <Uwe.Seimet@xxxxxxxxx>]

Hi Thomas,

> some comments on this patch:
> 
> @@ -983,11 +983,11 @@
>  	padstr((char *)(p + 23), FW_VERSION, 8); /* firmware version */
>  	if(s == opaque_ide_if) /* model */
>  	{
> -		padstr((char *)(p + 27), "Hatari IDE0 disk", 40);
> +		padstr((char *)(p + 27), "Hatari  IDE0 disk", 40);
>  	}
>  	else
>  	{
> -		padstr((char *)(p + 27), "Hatari IDE1 disk", 40);
> +		padstr((char *)(p + 27), "Hatari  IDE1 disk", 40);
>  	}
> 
> Why the white space change? Also, I think we agreed in another mail
> that the name should rather not depend on the slot, so maybe we should
> rather change this to "Hatari IDE disk" unconditionally?

For SCSI and MMC (optical SATA) drives the vendor ID is specified to
have 8 characters. A lot of IDE/SATA hard disks also follow this convention.
So this is about consistency, but for IDE it is not mandatory to have
this padding. The attached screenshots shows that at least some SATA drives
(the SSDs in the snapshot) use this padding. All in all, for hard disk
drives it may be a matter of taste/consistency, but non hard disk drives
(MMC) the padding rules are the same as for SCSI. (Same specification.)

Regarding the names we already discussed that the naming scheme should
probably be unified, so that ACSI drives might be called "Hatari  ACSI"
etc.  instead of "Hatari  EmulatedHarddisk". With ACSI Hatari is actually
using 8 characters for the vendor name.

> Anyway, this should be a separate patch instead.
> 
> @@ -1802,7 +1802,6 @@
>  		uint64_t total_sectors;
>  
>  		bdrv_get_geometry(s->bs, &total_sectors);
> -		total_sectors >>= 2;
>  		if (total_sectors == 0)
>  		{
>  			ide_atapi_cmd_error(s, SENSE_NOT_READY,
> 
> Why this change? Even current QEMU still contains the shift, so I
> somehow doubt that this is really right?

The sector size for emulated (data) CDs is 2048 bytes, and QEMU also
uses this size a few lines below:

		cpu_to_ube32(buf + 4, 2048);

But if you shift to the right the calculated capacity is just 1/4 of
what it actually is. You can also see in HDDRUTIL that the result of READ
CAPACITY is just 1/4 of the actual file size when the shift is applied.
This looks like a bug in the QEMU code to me. You may want to double-check.

> 
> @@ -1867,8 +1866,13 @@
>  		ide_atapi_cmd_ok(s);
>  		break;
>  	case GPCMD_INQUIRY:
> +	{
> +		uint64_t total_sectors;
> +
> +		bdrv_get_geometry(s->bs, &total_sectors);
> +
>  		max_len = packet[4];
> -		buf[0] = 0x05; /* CD-ROM */
> +		buf[0] = 0x05; /* MMC drive */
>  		buf[1] = 0x80; /* removable */
>  		buf[2] = 0x00; /* ISO */
>  		buf[3] = 0x21; /* ATAPI-2 (XXX: put ATAPI-4 ?) */
> @@ -1876,11 +1880,12 @@
>  		buf[5] = 0; /* reserved */
>  		buf[6] = 0; /* reserved */
>  		buf[7] = 0; /* reserved */
> -		padstr8(buf + 8, 8, "QEMU");
> -		padstr8(buf + 16, 16, "QEMU CD-ROM");
> +		padstr8(buf + 8, 8, "Hatari");
> +		padstr8(buf + 16, 16, "CD/DVD-ROM");
>  		padstr8(buf + 32, 4, FW_VERSION);
>  		ide_atapi_cmd_reply(s, 36, max_len);
>  		break;
> +	}
> 
> Fine for me if we change the strings here, but why do you introduce
> total_sectors here without using it?

Thank you for spotting this. total_sectors can be removed here.
Initially I wanted to use it, but then I forgot to remove it.

Best regards

Uwe

Attachment: Screenshot_20181109_081258.png
Description: PNG image