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

[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 <mark.cave-ayland@ilande.co.uk>
> > ---
> >   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_CAPABILITIES                0x2a
> > +#define MODE_PAGE_APPLE                       0x30
> >   #define MODE_PAGE_ALLS                        0x3f
> >   /* Not in Mt. Fuji, but in ATAPI 2.6 -- deprecated now in favor
> >    * of MODE_PAGE_SENSE_POWER */
> > --
> > 2.20.1
>
> Fam


ATB,

Mark.