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