On 21.11.2010, at 13:54, Blue Swirl wrote:
> On Fri, Nov 19, 2010 at 2:56 AM, Alexander Graf <ag...@suse.de> wrote:
>>
>> +typedef struct AHCIControlRegs {
>> + uint32_t cap;
>> + uint32_t ghc;
>> + uint32_t irqstatus;
>> + uint32_t impl;
>> + uint32_t version;
>> +} __attribute__ ((packed)) AHCIControlRegs;
>
> Why packed? These are used in native endian, so I'd let the compiler
> pick the best layout. Also in other structs.
Packed doesn't have too much to do with endianness, but gaps in the struct. The
reason I made these packed is that I casted the struct to an uint32_t array and
didn't want to have gaps there later on.
I changed that for the next version though to have explicit setters for each
field, so we don't need it here anymore.
>
>> +
>> +typedef struct AHCIPortRegs {
>> + uint32_t lst_addr;
>> + uint32_t lst_addr_hi;
>> + uint32_t fis_addr;
>> + uint32_t fis_addr_hi;
>> + uint32_t irq_stat;
>> + uint32_t irq_mask;
>> + uint32_t cmd;
>> + uint32_t unused0;
>> + uint32_t tfdata;
>> + uint32_t sig;
>> + uint32_t scr_stat;
>> + uint32_t scr_ctl;
>> + uint32_t scr_err;
>> + uint32_t scr_act;
>> + uint32_t cmd_issue;
>> + uint32_t reserved;
>> +} __attribute__ ((packed)) AHCIPortRegs;
Same as above for this one. I also changed it.
>> +
>> +typedef struct AHCICmdHdr {
>> + uint32_t opts;
>> + uint32_t status;
>> + uint64_t tbl_addr;
>> + uint32_t reserved[4];
>> +} __attribute__ ((packed)) AHCICmdHdr;
These have to be packed. We cast guest ram regions to this struct and then do
leXX_to_cpu() on that variable to make sure we take host endianness into
account. That's faster than going through the mapping logic for every single
word. And yes, they're always LE in ram :).
>> +
>> +typedef struct AHCI_SG {
>> + uint32_t addr;
>> + uint32_t addr_hi;
>> + uint32_t reserved;
>> + uint32_t flags_size;
>> +} __attribute__ ((packed)) AHCI_SG;
>> +
>> +typedef struct AHCIDevice AHCIDevice;
>> +
>> +typedef struct NCQTransferState {
>> + AHCIDevice *drive;
>> + QEMUSGList sglist;
>> + int is_read;
>> + uint16_t sector_count;
>> + uint64_t lba;
>> + uint8_t tag;
>> + int slot;
>> + int used;
>> +} NCQTransferState;
>> +
>> +struct AHCIDevice {
>> + IDEBus port;
>> + BMDMAState bmdma;
>> + int port_no;
>> + uint32_t port_state;
>> + uint32_t finished;
>> + AHCIPortRegs port_regs;
>> + struct AHCIState *hba;
>> + uint8_t *lst;
>> + uint8_t *res_fis;
>> + uint8_t *cmd_fis;
>> + int cmd_fis_len;
>> + AHCICmdHdr *cur_cmd;
>> + NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
>> +};
>> +
>> +typedef struct AHCIState {
>> + AHCIDevice dev[SATA_PORTS];
>> + AHCIControlRegs control_regs;
>> + int mem;
>> + qemu_irq irq;
>> +} AHCIState;
>> +
>> +typedef struct AHCIPciState {
>
> AHCIPCIState.
>
>> + PCIDevice card;
>> + AHCIState ahci;
>> +} AHCIPciState;
>> +
>> +typedef struct H2D_NCQ_FIS {
>
> This is not named according to CODING_STYLE. How about a more
> descriptive name which is not full of acronyms?
I'm open for suggestions. It's the "Host to Device Native Command Queue Frame
Information Structure". I changed it to H2dNcqFis for now.
Alex
