Re: [PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-24 Thread Mark Cave-Ayland 

On 21/04/2022 23:00, BALATON Zoltan wrote:


On Thu, 21 Apr 2022, Richard Henderson wrote:

On 4/21/22 08:29, Mark Cave-Ayland wrote:

You need (1 << SCSI_DISK_QUIRK_MODE_PAGE_APPLE) instead.


Doh, you're absolutely right. I believe the current recommendation is to use the 
BIT() macro in these cases.


I think it's not a recommendation (as in code style) but it often makes things 
simpler by reducing the number of parenthesis so using it is probably a good idea for 
readability. But if you never need the bit number only the value then you could 
define the quirks constants as that in the first place. (Otherwise if you want bit 
numbers maybe make it an enum.)



We probably need to fix BIT() to use 1ULL.

At present it's using 1UL, to match the other (unfortunate) uses of unsigned long 
within bitops.h.  The use of BIT() for things unrelated to bitops.h just bit a 
recent risc-v pull request, in that it failed to build on all 32-bit hosts.


There's already a BIT_ULL(nr) when ULL is needed but in this case quirks was declared 
uint32_t so probably OK with UL as well. (Was this bitops.h taken from Linux? Keeping 
it compatible then may be a good idea to avoid confusion.)


It seems there is still a bit of discussion around using BIT() here, so for v2 I'll 
add the shift directly with (1 << x). Then if the BIT() macro becomes suitable for 
more general use it can easily be updated as a separate patch later.



ATB,

Mark.

Re: [PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-21 Thread BALATON Zoltan 

On Thu, 21 Apr 2022, Richard Henderson wrote:

On 4/21/22 08:29, Mark Cave-Ayland wrote:

You need (1 << SCSI_DISK_QUIRK_MODE_PAGE_APPLE) instead.


Doh, you're absolutely right. I believe the current recommendation is to 
use the BIT() macro in these cases.


I think it's not a recommendation (as in code style) but it often makes 
things simpler by reducing the number of parenthesis so using it is 
probably a good idea for readability. But if you never need the bit number 
only the value then you could define the quirks constants as that in the 
first place. (Otherwise if you want bit numbers maybe make it an enum.)



We probably need to fix BIT() to use 1ULL.

At present it's using 1UL, to match the other (unfortunate) uses of unsigned 
long within bitops.h.  The use of BIT() for things unrelated to bitops.h just 
bit a recent risc-v pull request, in that it failed to build on all 32-bit 
hosts.


There's already a BIT_ULL(nr) when ULL is needed but in this case quirks 
was declared uint32_t so probably OK with UL as well. (Was this bitops.h 
taken from Linux? Keeping it compatible then may be a good idea to avoid 
confusion.)


Regards,
BALATON Zoltan

Re: [PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-21 Thread Richard Henderson 

On 4/21/22 08:29, Mark Cave-Ayland wrote:

You need (1 << SCSI_DISK_QUIRK_MODE_PAGE_APPLE) instead.


Doh, you're absolutely right. I believe the current recommendation is to use the BIT() 
macro in these cases.


We probably need to fix BIT() to use 1ULL.

At present it's using 1UL, to match the other (unfortunate) uses of unsigned long within 
bitops.h.  The use of BIT() for things unrelated to bitops.h just bit a recent risc-v pull 
request, in that it failed to build on all 32-bit hosts.



r~

Re: [PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-21 Thread Mark Cave-Ayland 

On 21/04/2022 14:27, Fam Zheng wrote:


On 2022-04-21 07:51, Mark Cave-Ayland wrote:

One of the mechanisms MacOS uses to identify drives compatible with MacOS is to
send a custom MODE SELECT command for page 0x30 to the drive. The response to
this is a hard-coded manufacturer string which must match in order for the
drive to be usable within MacOS.

Add an implementation of the MODE SELECT page 0x30 response guarded by a newly
defined SCSI_DISK_QUIRK_MODE_PAGE_APPLE quirk bit so that drives attached
to non-Apple machines function exactly as before.

Signed-off-by: Mark Cave-Ayland 
---
  hw/scsi/scsi-disk.c  | 19 +++
  include/hw/scsi/scsi.h   |  3 +++
  include/scsi/constants.h |  1 +
  3 files changed, 23 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index d89cdd4e4a..37013756d5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1085,6 +1085,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
uint8_t **p_outbuf,
  [MODE_PAGE_R_W_ERROR]  = (1 << TYPE_DISK) | (1 << 
TYPE_ROM),
  [MODE_PAGE_AUDIO_CTL]  = (1 << TYPE_ROM),
  [MODE_PAGE_CAPABILITIES]   = (1 << TYPE_ROM),
+[MODE_PAGE_APPLE]  = (1 << TYPE_ROM),
  };
  
  uint8_t *p = *p_outbuf + 2;

@@ -1229,6 +1230,22 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
uint8_t **p_outbuf,
  p[19] = (16 * 176) & 0xff;
  break;
  
+ case MODE_PAGE_APPLE:

+if (s->qdev.type == TYPE_DISK &&
+(s->quirks & SCSI_DISK_QUIRK_MODE_PAGE_APPLE)) {


This is always false. SCSI_DISK_QUIRK_MODE_PAGE_APPLE is defined 0.

You need (1 << SCSI_DISK_QUIRK_MODE_PAGE_APPLE) instead.


Doh, you're absolutely right. I believe the current recommendation is to use the 
BIT() macro in these cases.


I'll wait to see if there is any further feedback on the whole quirks concept before 
posting an updated v2.



+
+length = 0x24;
+if (page_control == 1) { /* Changeable Values */
+break;
+}
+
+memset(p, 0, length);
+strcpy((char *)p + 8, "APPLE COMPUTER, INC   ");
+break;
+} else {
+return -1;
+}
+
  default:
  return -1;
  }
@@ -3042,6 +3059,8 @@ static Property scsi_hd_properties[] = {
  DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
  DEFINE_PROP_INT32("scsi_version", SCSIDiskState, 
qdev.default_scsi_version,
5),
+DEFINE_PROP_BIT("quirk_mode_page_apple", SCSIDiskState, quirks,
+SCSI_DISK_QUIRK_MODE_PAGE_APPLE, 0),
  DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
  DEFINE_PROP_END_OF_LIST(),
  };
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1ffb367f94..f629706250 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -226,4 +226,7 @@ SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int 
target, int lun);
  /* scsi-generic.c. */
  extern const SCSIReqOps scsi_generic_req_ops;
  
+/* scsi-disk.c */

+#define SCSI_DISK_QUIRK_MODE_PAGE_APPLE 0
+
  #endif
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 2a32c08b5e..21ca7b50cd 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -234,6 +234,7 @@
  #define MODE_PAGE_FAULT_FAIL  0x1c
  #define MODE_PAGE_TO_PROTECT  0x1d
  #define MODE_PAGE_CAPABILITIES0x2a
+#define MODE_PAGE_APPLE   0x30
  #define MODE_PAGE_ALLS0x3f
  /* Not in Mt. Fuji, but in ATAPI 2.6 -- deprecated now in favor
   * of MODE_PAGE_SENSE_POWER */
--
2.20.1




Fam



ATB,

Mark.

Re: [PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-21 Thread Fam Zheng 
On 2022-04-21 07:51, Mark Cave-Ayland wrote:
> One of the mechanisms MacOS uses to identify drives compatible with MacOS is 
> to
> send a custom MODE SELECT command for page 0x30 to the drive. The response to
> this is a hard-coded manufacturer string which must match in order for the
> drive to be usable within MacOS.
> 
> Add an implementation of the MODE SELECT page 0x30 response guarded by a newly
> defined SCSI_DISK_QUIRK_MODE_PAGE_APPLE quirk bit so that drives attached
> to non-Apple machines function exactly as before.
> 
> Signed-off-by: Mark Cave-Ayland 
> ---
>  hw/scsi/scsi-disk.c  | 19 +++
>  include/hw/scsi/scsi.h   |  3 +++
>  include/scsi/constants.h |  1 +
>  3 files changed, 23 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index d89cdd4e4a..37013756d5 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1085,6 +1085,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
> uint8_t **p_outbuf,
>  [MODE_PAGE_R_W_ERROR]  = (1 << TYPE_DISK) | (1 << 
> TYPE_ROM),
>  [MODE_PAGE_AUDIO_CTL]  = (1 << TYPE_ROM),
>  [MODE_PAGE_CAPABILITIES]   = (1 << TYPE_ROM),
> +[MODE_PAGE_APPLE]  = (1 << TYPE_ROM),
>  };
>  
>  uint8_t *p = *p_outbuf + 2;
> @@ -1229,6 +1230,22 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
> uint8_t **p_outbuf,
>  p[19] = (16 * 176) & 0xff;
>  break;
>  
> + case MODE_PAGE_APPLE:
> +if (s->qdev.type == TYPE_DISK &&
> +(s->quirks & SCSI_DISK_QUIRK_MODE_PAGE_APPLE)) {

This is always false. SCSI_DISK_QUIRK_MODE_PAGE_APPLE is defined 0.

You need (1 << SCSI_DISK_QUIRK_MODE_PAGE_APPLE) instead.

> +
> +length = 0x24;
> +if (page_control == 1) { /* Changeable Values */
> +break;
> +}
> +
> +memset(p, 0, length);
> +strcpy((char *)p + 8, "APPLE COMPUTER, INC   ");
> +break;
> +} else {
> +return -1;
> +}
> +
>  default:
>  return -1;
>  }
> @@ -3042,6 +3059,8 @@ static Property scsi_hd_properties[] = {
>  DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
>  DEFINE_PROP_INT32("scsi_version", SCSIDiskState, 
> qdev.default_scsi_version,
>5),
> +DEFINE_PROP_BIT("quirk_mode_page_apple", SCSIDiskState, quirks,
> +SCSI_DISK_QUIRK_MODE_PAGE_APPLE, 0),
>  DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
>  DEFINE_PROP_END_OF_LIST(),
>  };
> diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
> index 1ffb367f94..f629706250 100644
> --- a/include/hw/scsi/scsi.h
> +++ b/include/hw/scsi/scsi.h
> @@ -226,4 +226,7 @@ SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, 
> int target, int lun);
>  /* scsi-generic.c. */
>  extern const SCSIReqOps scsi_generic_req_ops;
>  
> +/* scsi-disk.c */
> +#define SCSI_DISK_QUIRK_MODE_PAGE_APPLE 0
> +
>  #endif
> diff --git a/include/scsi/constants.h b/include/scsi/constants.h
> index 2a32c08b5e..21ca7b50cd 100644
> --- a/include/scsi/constants.h
> +++ b/include/scsi/constants.h
> @@ -234,6 +234,7 @@
>  #define MODE_PAGE_FAULT_FAIL  0x1c
>  #define MODE_PAGE_TO_PROTECT  0x1d
>  #define MODE_PAGE_CAPABILITIES0x2a
> +#define MODE_PAGE_APPLE   0x30
>  #define MODE_PAGE_ALLS0x3f
>  /* Not in Mt. Fuji, but in ATAPI 2.6 -- deprecated now in favor
>   * of MODE_PAGE_SENSE_POWER */
> -- 
> 2.20.1
> 
> 

Fam

[PATCH 3/6] scsi-disk: add MODE_PAGE_APPLE quirk for Macintosh

2022-04-21 Thread Mark Cave-Ayland 
One of the mechanisms MacOS uses to identify drives compatible with MacOS is to
send a custom MODE SELECT command for page 0x30 to the drive. The response to
this is a hard-coded manufacturer string which must match in order for the
drive to be usable within MacOS.

Add an implementation of the MODE SELECT page 0x30 response guarded by a newly
defined SCSI_DISK_QUIRK_MODE_PAGE_APPLE quirk bit so that drives attached
to non-Apple machines function exactly as before.

Signed-off-by: Mark Cave-Ayland 
---
 hw/scsi/scsi-disk.c  | 19 +++
 include/hw/scsi/scsi.h   |  3 +++
 include/scsi/constants.h |  1 +
 3 files changed, 23 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index d89cdd4e4a..37013756d5 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1085,6 +1085,7 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
uint8_t **p_outbuf,
 [MODE_PAGE_R_W_ERROR]  = (1 << TYPE_DISK) | (1 << 
TYPE_ROM),
 [MODE_PAGE_AUDIO_CTL]  = (1 << TYPE_ROM),
 [MODE_PAGE_CAPABILITIES]   = (1 << TYPE_ROM),
+[MODE_PAGE_APPLE]  = (1 << TYPE_ROM),
 };
 
 uint8_t *p = *p_outbuf + 2;
@@ -1229,6 +1230,22 @@ static int mode_sense_page(SCSIDiskState *s, int page, 
uint8_t **p_outbuf,
 p[19] = (16 * 176) & 0xff;
 break;
 
+ case MODE_PAGE_APPLE:
+if (s->qdev.type == TYPE_DISK &&
+(s->quirks & SCSI_DISK_QUIRK_MODE_PAGE_APPLE)) {
+
+length = 0x24;
+if (page_control == 1) { /* Changeable Values */
+break;
+}
+
+memset(p, 0, length);
+strcpy((char *)p + 8, "APPLE COMPUTER, INC   ");
+break;
+} else {
+return -1;
+}
+
 default:
 return -1;
 }
@@ -3042,6 +3059,8 @@ static Property scsi_hd_properties[] = {
 DEFINE_PROP_UINT16("rotation_rate", SCSIDiskState, rotation_rate, 0),
 DEFINE_PROP_INT32("scsi_version", SCSIDiskState, qdev.default_scsi_version,
   5),
+DEFINE_PROP_BIT("quirk_mode_page_apple", SCSIDiskState, quirks,
+SCSI_DISK_QUIRK_MODE_PAGE_APPLE, 0),
 DEFINE_BLOCK_CHS_PROPERTIES(SCSIDiskState, qdev.conf),
 DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/scsi/scsi.h b/include/hw/scsi/scsi.h
index 1ffb367f94..f629706250 100644
--- a/include/hw/scsi/scsi.h
+++ b/include/hw/scsi/scsi.h
@@ -226,4 +226,7 @@ SCSIDevice *scsi_device_get(SCSIBus *bus, int channel, int 
target, int lun);
 /* scsi-generic.c. */
 extern const SCSIReqOps scsi_generic_req_ops;
 
+/* scsi-disk.c */
+#define SCSI_DISK_QUIRK_MODE_PAGE_APPLE 0
+
 #endif
diff --git a/include/scsi/constants.h b/include/scsi/constants.h
index 2a32c08b5e..21ca7b50cd 100644
--- a/include/scsi/constants.h
+++ b/include/scsi/constants.h
@@ -234,6 +234,7 @@
 #define MODE_PAGE_FAULT_FAIL  0x1c
 #define MODE_PAGE_TO_PROTECT  0x1d
 #define MODE_PAGE_CAPABILITIES0x2a
+#define MODE_PAGE_APPLE   0x30
 #define MODE_PAGE_ALLS0x3f
 /* Not in Mt. Fuji, but in ATAPI 2.6 -- deprecated now in favor
  * of MODE_PAGE_SENSE_POWER */
-- 
2.20.1