AHCI IDENTIFY command troubles

[mariuszp]

I have looked at the AHCI spec as well as the AHCI article on osdev wiki (http://wiki.osdev.org/AHCI) but I am very confused about one simple thing. I am sending the IDENTIFY commands follows:

Code: Select all

						// we must now identify the device to find out its size
						// just use the first header
						cmdhead->cfl = sizeof(FIS_REG_H2D)/4;
						cmdhead->w = 0;
						cmdhead->prdtl = 1;
						cmdhead->p = 1;
						
						AHCICommandTable *cmdtbl =
							(AHCICommandTable*) ((char*)dmaGetPtr(&dev->dmabuf) + 1024+256+8*1024);
						memset(cmdtbl, 0, sizeof(AHCICommandTable));
						
						cmdtbl->prdt_entry[0].dba = dmaGetPhys(&dev->iobuf);
						cmdtbl->prdt_entry[0].dbc = 2048;
						cmdtbl->prdt_entry[0].i = 1;	// interrupt when identify complete
						
						FIS_REG_H2D *cmdfis = (FIS_REG_H2D*) cmdtbl->cfis;
						cmdfis->fis_type = FIS_TYPE_REG_H2D;
						cmdfis->c = 1;
						cmdfis->command = ATA_CMD_IDENTIFY;

						__sync_synchronize();
						port->ci = 1;
						__sync_synchronize();
						
						kprintf("IDENTIFY command sent\n");
						while (1)
						{
							if ((port->ci & 1) == 0)
							{
								break;
							};
							
							__sync_synchronize();
						};
						
						kprintf("IDENTIFY command complete\n");
						uint32_t size = *((uint32_t*)((char*)dmaGetPtr(&dev->iobuf) + ATA_IDENT_MAX_LBA_EXT));
						
						kprintf("Size in MB: %d\n", (int) (size * 512 / 1024 / 1024));

It reports the size in MB to be 0 (the drive is actually 4GB). If I use ATA_IDENT_MAX_LBA instead of ATA_IDENT_MAX_LBA_EXT, it also reports 0. I therefore assume that the IDENTIFY data is in fact not loaded into the buffer specified by the PRDT??? In that case, how do I extract the IDENTIFY structure so I can find the disk size?

[SpyderTL]

Check your port interrupt status register for errors. If the error flag is set, that would explain why you are getting a zero back.

[mariuszp]

Check your port interrupt status register for errors. If the error flag is set, that would explain why you are getting a zero back.

The value of the interrupt status register is "0x1", meaning only the device-to-host FIS interrupt has been set.

EDIT: Here are my values for all the offsets

Code: Select all

#define ATA_IDENT_DEVICETYPE				0
#define ATA_IDENT_CYLINDERS				2
#define ATA_IDENT_HEADS					6
#define ATA_IDENT_SECTORS				12
#define ATA_IDENT_SERIAL				20
#define ATA_IDENT_MODEL					54
#define ATA_IDENT_CAPABILITIES				98
#define ATA_IDENT_FIELDVALID				106
#define ATA_IDENT_MAX_LBA				120
#define ATA_IDENT_COMMANDSETS				164
#define ATA_IDENT_MAX_LBA_EXT				200

Those offsets work for IDE, does AHCI append some kind of header or something to this? The first byte I'm getting is 0x40.

[shmx]

FIS_REG_H2D *cmdfis = (FIS_REG_H2D*) cmdtbl->cfis;
cmdfis->fis_type = FIS_TYPE_REG_H2D;
cmdfis->c = 1;
cmdfis->command = ATA_CMD_IDENTIFY;

Do You clean the unused fields?

[shmx]

Code: Select all

cmdtbl->prdt_entry[0].dbc = 2048;

Why 2048? For ATA_CMD_IDENTIFY need to use 511 (512 bytes - 1).

[mariuszp]

I now fixed it so that it does memset(cmdfis, 0, sizeof(FIS_REG_H2D)) beofre filling in the structure, and also set the size dbc to 511 as suggested, but it's still givving exactly the same result.

I have decided to do a dump of the iobuf, and I have attached the screenshot. There is obviously something in the data.

[shmx]

All right. 0x800000 * 0x200 = 4G

[shmx]

ATA/ATAPI specification define offsets in words (two bytes).

[mariuszp]

Hmm, my offsets were correct (I gave them in bytes). I changed the pointer arithmetic a bit, so that I cast to uint8_t* in one step and then dereference the various fields in later steps, and somehow it works. Not sure where the mistake was.

[Octocontrabass]

Type punning isn't allowed by the C standard, so your compiler was probably optimizing undefined behavior to zero.

[shmx]

so your compiler was probably optimizing undefined behavior to zero.

I really doubt. Most likely the data does not to have time to be transmitted through the DMA.
To mariuszp, "port" is defined as volatile?

[mariuszp]

Yes, the port was (and is) declared volatile. Also note i call __sync_synchronize() to prevent optimisations from messing with the i/o

[shmx]

i call __sync_synchronize() to prevent optimisations from messing with the i/o

I see. I don't know why it's happened. I don't see UB in this code. However, there may be a problem with bit fields (I did not encounter itself). Bit fields are not recommended use for MMIO.

[Octocontrabass]

I don't see UB in this code.

Code: Select all

uint32_t size = *((uint32_t*)((char*)dmaGetPtr(&dev->iobuf) + ATA_IDENT_MAX_LBA_EXT));

Casting the char* to uint32_t* and then dereferencing is undefined behavior.

[shmx]

Casting the char* to uint32_t* and then dereferencing is undefined behavior.

Where is it written in the Standard (C99)?