Re: [Qemu-devel] [PATCH 08/11] ahci: add ahci emulation

[Alexander Graf]

On 21.11.2010, at 13:54, Blue Swirl wrote:

> On Fri, Nov 19, 2010 at 2:56 AM, Alexander Graf <address@hidden> 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