ide/core.c ATA Major Version reporting incorrect

[Gregory Price]

@@ -165,7 +165,7 @@ static void ide_identify(IDEState *s)
        put_le16(p + 76, (1 << 8));
    }

    put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */
- put_le16(p + 80, 0xf0); /* ata3 -> ata6 supported */
+ put_le16(p + 80, ((1 << 6) | (1 << 5) (1 << 4) (1 << 3)); /* ata3 -> ata6 supported */
    put_le16(p + 81, 0x16); /* conforms to ata5 */
    /* 14=NOP supported, 5=WCACHE supported, 0=SMART supported */
    put_le16(p + 82, (1 << 14) | (1 << 5) | 1);

This field Major Version Number field is presently reporting support for ATA-4 through ATA-7.
Bitfield[80] is defined in the ATA-6 specification below.

0xF0 = (1<<7) | (1<<6) | (1 << 5) | (1 << 4) // 4-7 - current settings
0x78 = (1<<6) | (1<<5) | (1 << 4) | (1 << 3) // 3-6 - new settings

Either the comment is wrong, or the field is wrong. If the field is wrong it can cause errors in drivers that check support vs conformity. This will not break most guests, since the conformity field is set to ATA-5.

I'm not sure whether this component supports ATA-7, but since it's commented as if it supports up through 6, correcting the field assignment seems more correct.

ATA/ATAPI-6 Specification
https://web.archive.org/web/20200124094822/https://www.t13.org/Documents/UploadedDocuments/project/d1410r3b-ATA-ATAPI-6.pdf

Page 116
80 - M Major version number
0000h or FFFFh = device does not report version
F 15 Reserved
F 14 Reserved for ATA/ATAPI-14
F 13 Reserved for ATA/ATAPI-13
F 12 Reserved for ATA/ATAPI-12
F 11 Reserved for ATA/ATAPI-11
F 10 Reserved for ATA/ATAPI-10
F 9 Reserved for ATA/ATAPI-9
F 8 Reserved for ATA/ATAPI-8
F 7 Reserved for ATA/ATAPI-7
F 6 1 = supports ATA/ATAPI-6
F 5 1 = supports ATA/ATAPI-5
F 4 1 = supports ATA/ATAPI-4
F 3 1 = supports ATA-3
X 2 Obsolete
X 1 Obsolete
F 0 Reserved

[Thomas Huth (th-huth)]

John, could you please have a look?

[John Snow (jnsnow)]

I doubt we truly implement *any* standard precisely correctly, but if we are advertising support for ATA7 and it works, I'd rather just fix the comment to keep behavior the same.

It probably was a mistake in the original commit from ... sometime before 2006, but if nothing is observably broken, maybe it shouldn't be changed.

[Gregory Price (gourryinverse)]

for what it's worth, i have yet to see a driver actually check this field.

I have seen a ton of code (OVMF and others) detect other information and just straight up say "I'm in QEMU" and YOLO a bunch of things like assuming DMA is available and such, so I somewhat doubt anyone *actually* checks these fields.

[John Snow (jnsnow)]

That's probably the single most reasonable thing to do, truth be told!

I don't have the time to audit these fields properly; I don't know which versions we truly ought to advertise support for. I know I looked at ATA8-AC3 at some point fairly recently and concluded that we don't support all of the "must-support" features of that spec, because we don't implement any of the logging features whatsoever.

I often consult ATA8-ACS3 to take advantage of clarifications made in later revisions and cross-correlate with ATA7; but I don't know what the most modern specification we can be said to support the minimum feature set from truly is.

Patches (and reviewers) always welcome; but generally I am afraid of touching too many things because I don't want to break legacy operating systems that might not have an awareness of QEMU. Our testing for older operating systems is not particularly robust, here.

I think I am still leaning towards just fixing the comment, but if you are aware of some ATA7 thing we are required to support but don't, I'll remove the bit.

[Gregory Price (gourryinverse)]

I would just fix the comment for now, likewise I don't have the time to audit
whether the emulation provides full coverage of the standard.

Really there's two cases here
1) We report ATA4-7 (current status), someone discovers missing bits feature
Response: They file a ticket on the missing feature.

2) We report ATA3-6 (changed status), legacy stuff checking this bit suddenly break
Response: Rabid screeching somewhere in the depths of the internets.

Fixing the comment is more risk averse and more likely to produce the desirable
outcome (some reads the comment, and discovers a missing ATA feature for the
reported standard).

[Thomas Huth (th-huth)]

This is an automated cleanup. This bug report has been moved
to QEMU's new bug tracker on gitlab.com and thus gets marked
as 'expired' now. Please continue with the discussion here:

 https://gitlab.com/qemu-project/qemu/-/issues/59