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

[Blue Swirl]

On Tue, Nov 23, 2010 at 1:48 PM, Alexander Graf <address@hidden> wrote:
>
> 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 :).

That's OK.

>>> +
>>> +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.

NCQFrame? Most of the words do not seem very interesting.