From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41237 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PKyVm-0004i8-Av for qemu-devel@nongnu.org; Tue, 23 Nov 2010 14:27:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PKyVJ-0003Mm-Mx for qemu-devel@nongnu.org; Tue, 23 Nov 2010 14:26:42 -0500 Received: from mail-qy0-f173.google.com ([209.85.216.173]:59260) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PKyVJ-0003Mc-Cc for qemu-devel@nongnu.org; Tue, 23 Nov 2010 14:26:13 -0500 Received: by qyk1 with SMTP id 1so2590890qyk.4 for ; Tue, 23 Nov 2010 11:26:12 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <57485942-0489-41C2-843D-D086D7208D49@suse.de> References: <1290135413-21462-1-git-send-email-agraf@suse.de> <1290135413-21462-9-git-send-email-agraf@suse.de> <57485942-0489-41C2-843D-D086D7208D49@suse.de> From: Blue Swirl Date: Tue, 23 Nov 2010 19:25:52 +0000 Message-ID: Subject: Re: [Qemu-devel] [PATCH 08/11] ahci: add ahci emulation Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Kevin Wolf , Joerg Roedel , QEMU-devel Developers , Gerd Hoffmann , Stefan Hajnoczi , tj@kernel.org, Sebastian Herbszt , Roland Elek On Tue, Nov 23, 2010 at 1:48 PM, Alexander Graf wrote: > > On 21.11.2010, at 13:54, Blue Swirl wrote: > >> On Fri, Nov 19, 2010 at 2:56 AM, Alexander Graf wrote: >>> >>> +typedef struct AHCIControlRegs { >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0cap; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0ghc; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0irqstatus; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0impl; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0version; >>> +} __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 struc= t. 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 e= ach field, so we don't need it here anymore. > >> >>> + >>> +typedef struct AHCIPortRegs { >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0lst_addr; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0lst_addr_hi; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0fis_addr; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0fis_addr_hi; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0irq_stat; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0irq_mask; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0cmd; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0unused0; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0tfdata; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0sig; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0scr_stat; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0scr_ctl; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0scr_err; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0scr_act; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0cmd_issue; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0reserved; >>> +} __attribute__ ((packed)) AHCIPortRegs; > > Same as above for this one. I also changed it. > >>> + >>> +typedef struct AHCICmdHdr { >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0opts; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0status; >>> + =C2=A0 =C2=A0uint64_t =C2=A0 =C2=A0tbl_addr; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0reserved[4]; >>> +} __attribute__ ((packed)) AHCICmdHdr; > > These have to be packed. We cast guest ram regions to this struct and the= n do leXX_to_cpu() on that variable to make sure we take host endianness in= to account. That's faster than going through the mapping logic for every si= ngle word. And yes, they're always LE in ram :). That's OK. >>> + >>> +typedef struct AHCI_SG { >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0addr; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0addr_hi; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0reserved; >>> + =C2=A0 =C2=A0uint32_t =C2=A0 =C2=A0flags_size; >>> +} __attribute__ ((packed)) AHCI_SG; >>> + >>> +typedef struct AHCIDevice AHCIDevice; >>> + >>> +typedef struct NCQTransferState { >>> + =C2=A0 =C2=A0AHCIDevice *drive; >>> + =C2=A0 =C2=A0QEMUSGList sglist; >>> + =C2=A0 =C2=A0int is_read; >>> + =C2=A0 =C2=A0uint16_t sector_count; >>> + =C2=A0 =C2=A0uint64_t lba; >>> + =C2=A0 =C2=A0uint8_t tag; >>> + =C2=A0 =C2=A0int slot; >>> + =C2=A0 =C2=A0int used; >>> +} NCQTransferState; >>> + >>> +struct AHCIDevice { >>> + =C2=A0 =C2=A0IDEBus port; >>> + =C2=A0 =C2=A0BMDMAState bmdma; >>> + =C2=A0 =C2=A0int port_no; >>> + =C2=A0 =C2=A0uint32_t port_state; >>> + =C2=A0 =C2=A0uint32_t finished; >>> + =C2=A0 =C2=A0AHCIPortRegs port_regs; >>> + =C2=A0 =C2=A0struct AHCIState *hba; >>> + =C2=A0 =C2=A0uint8_t *lst; >>> + =C2=A0 =C2=A0uint8_t *res_fis; >>> + =C2=A0 =C2=A0uint8_t *cmd_fis; >>> + =C2=A0 =C2=A0int cmd_fis_len; >>> + =C2=A0 =C2=A0AHCICmdHdr *cur_cmd; >>> + =C2=A0 =C2=A0NCQTransferState ncq_tfs[AHCI_MAX_CMDS]; >>> +}; >>> + >>> +typedef struct AHCIState { >>> + =C2=A0 =C2=A0AHCIDevice dev[SATA_PORTS]; >>> + =C2=A0 =C2=A0AHCIControlRegs control_regs; >>> + =C2=A0 =C2=A0int mem; >>> + =C2=A0 =C2=A0qemu_irq irq; >>> +} AHCIState; >>> + >>> +typedef struct AHCIPciState { >> >> AHCIPCIState. >> >>> + =C2=A0 =C2=A0PCIDevice card; >>> + =C2=A0 =C2=A0AHCIState 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 F= rame Information Structure". I changed it to H2dNcqFis for now. NCQFrame? Most of the words do not seem very interesting.