From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44469) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJwdk-0006Xt-6M for qemu-devel@nongnu.org; Mon, 04 Jul 2016 01:41:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bJwde-00013K-3g for qemu-devel@nongnu.org; Mon, 04 Jul 2016 01:41:51 -0400 Received: from mout.web.de ([212.227.17.11]:51046) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJwdd-00012z-P2 for qemu-devel@nongnu.org; Mon, 04 Jul 2016 01:41:46 -0400 References: <1465993312-18119-1-git-send-email-davidkiarie4@gmail.com> <1465993312-18119-4-git-send-email-davidkiarie4@gmail.com> <576AF3E2.4060108@web.de> From: Jan Kiszka Message-ID: <5779F708.7060704@web.de> Date: Mon, 4 Jul 2016 07:41:28 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="DVqvG4aPnrh53noqdKOaXs7p0R9WglHCG" Subject: Re: [Qemu-devel] [V12 3/4] hw/i386: Introduce AMD IOMMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Kiarie Cc: QEMU Developers , Marcel Apfelbaum , "Michael S. Tsirkin" , Peter Xu , alex.williamson@redhat.com, Valentine Sinitsyn , Eduardo Habkost This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --DVqvG4aPnrh53noqdKOaXs7p0R9WglHCG From: Jan Kiszka To: David Kiarie Cc: QEMU Developers , Marcel Apfelbaum , "Michael S. Tsirkin" , Peter Xu , alex.williamson@redhat.com, Valentine Sinitsyn , Eduardo Habkost Message-ID: <5779F708.7060704@web.de> Subject: Re: [V12 3/4] hw/i386: Introduce AMD IOMMU References: <1465993312-18119-1-git-send-email-davidkiarie4@gmail.com> <1465993312-18119-4-git-send-email-davidkiarie4@gmail.com> <576AF3E2.4060108@web.de> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 2016-07-04 07:06, David Kiarie wrote: > On Wed, Jun 22, 2016 at 11:24 PM, Jan Kiszka wrote:= >> On 2016-06-15 14:21, David Kiarie wrote: >>> +static uint64_t amdvi_mmio_read(void *opaque, hwaddr addr, unsigned = size) >>> +{ >>> + AMDVIState *s =3D opaque; >>> + >>> + uint64_t val =3D -1; >>> + if (addr + size > AMDVI_MMIO_SIZE) { >>> + trace_amdvi_mmio_read("error: addr outside region: max ", >>> + (uint64_t)AMDVI_MMIO_SIZE, addr, size); >>> + return (uint64_t)-1; >>> + } >>> + >>> + if (size =3D=3D 2) { >>> + val =3D amdvi_readw(s, addr); >>> + } else if (size =3D=3D 4) { >>> + val =3D amdvi_readl(s, addr); >>> + } else if (size =3D=3D 8) { >>> + val =3D amdvi_readq(s, addr); >>> + } >>> + >>> + switch (addr & ~0x07) { >>> + case AMDVI_MMIO_DEVICE_TABLE: >>> + trace_amdvi_mmio_read("MMIO_DEVICE_TABLE", addr, size, addr = & ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_BASE: >>> + trace_amdvi_mmio_read("MMIO_COMMAND_BASE", addr, size, addr = & ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_BASE: >>> + trace_amdvi_mmio_read("MMIO_EVENT_BASE", addr, size, addr & = ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_CONTROL: >>> + trace_amdvi_mmio_read("MMIO_MMIO_CONTROL", addr, size, addr = & ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_EXCL_BASE: >>> + trace_amdvi_mmio_read("MMIO_EXCL_BASE", addr, size, addr & ~= 0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_EXCL_LIMIT: >>> + trace_amdvi_mmio_read("MMIO_EXCL_LIMIT", addr, size, addr & = ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_HEAD: >>> + trace_amdvi_mmio_read("MMIO_COMMAND_HEAD", addr, size, addr = & ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_TAIL: >>> + trace_amdvi_mmio_read("MMIO_COMMAND_TAIL", addr, size, addr = & ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_HEAD: >>> + trace_amdvi_mmio_read("MMIO_EVENT_HEAD", addr, size, addr & = ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_TAIL: >>> + trace_amdvi_mmio_read("MMIO_EVENT_TAIL", addr, size, addr & = ~0x07); >>> + break; >>> + >>> + case AMDVI_MMIO_STATUS: >>> + trace_amdvi_mmio_read("MMIO_STATUS", addr, size, addr & ~0x0= 7); >>> + break; >>> + >>> + case AMDVI_MMIO_EXT_FEATURES: >>> + trace_amdvi_mmio_read("MMIO_EXT_FEATURES", addr, size, addr = & ~0x07); >>> + break; >> >> What about a lookup table for that name? >=20 > I can't find an obvious way to index a table given the register address= =2E Well, you would need a low ((addr & 0x2000) =3D=3D 0) and a high table (a= ddr & 0x2000), and then do the indexing based on (addr & ~0x2000) / 8. >=20 >> >>> + >>> + default: >>> + trace_amdvi_mmio_read("UNHANDLED READ", addr, size, addr & ~= 0x07); >>> + } >>> + return val; >>> +} >>> + >>> +static void amdvi_handle_control_write(AMDVIState *s) >>> +{ >>> + /* >>> + * read whatever is already written in case >>> + * software is writing in chucks less than 8 bytes >>> + */ >>> + unsigned long control =3D amdvi_readq(s, AMDVI_MMIO_CONTROL); >>> + s->enabled =3D !!(control & AMDVI_MMIO_CONTROL_AMDVIEN); >>> + >>> + s->ats_enabled =3D !!(control & AMDVI_MMIO_CONTROL_HTTUNEN); >>> + s->evtlog_enabled =3D s->enabled && !!(control & >>> + AMDVI_MMIO_CONTROL_EVENTLOGEN); >>> + >>> + s->evtlog_intr =3D !!(control & AMDVI_MMIO_CONTROL_EVENTINTEN); >>> + s->completion_wait_intr =3D !!(control & AMDVI_MMIO_CONTROL_COMW= AITINTEN); >>> + s->cmdbuf_enabled =3D s->enabled && !!(control & >>> + AMDVI_MMIO_CONTROL_CMDBUFLEN); >>> + >>> + /* update the flags depending on the control register */ >>> + if (s->cmdbuf_enabled) { >>> + amdvi_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_CMDBUF_RUN= ); >>> + } else { >>> + amdvi_and_assignq(s, AMDVI_MMIO_STATUS, ~AMDVI_MMIO_STATUS_C= MDBUF_RUN); >>> + } >>> + if (s->evtlog_enabled) { >>> + amdvi_orq(s, AMDVI_MMIO_STATUS, AMDVI_MMIO_STATUS_EVT_RUN); >>> + } else { >>> + amdvi_and_assignq(s, AMDVI_MMIO_STATUS, ~AMDVI_MMIO_STATUS_E= VT_RUN); >>> + } >>> + >>> + trace_amdvi_control_status(control); >>> + >>> + amdvi_cmdbuf_run(s); >>> +} >>> + >>> +static inline void amdvi_handle_devtab_write(AMDVIState *s) >>> + >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_DEVICE_TABLE); >>> + s->devtab =3D (val & AMDVI_MMIO_DEVTAB_BASE_MASK); >>> + >>> + /* set device table length */ >>> + s->devtab_len =3D ((val & AMDVI_MMIO_DEVTAB_SIZE_MASK) + 1 * >>> + (AMDVI_MMIO_DEVTAB_SIZE_UNIT / >>> + AMDVI_MMIO_DEVTAB_ENTRY_SIZE)); >>> +} >>> + >>> +static inline void amdvi_handle_cmdhead_write(AMDVIState *s) >>> +{ >>> + s->cmdbuf_head =3D amdvi_readq(s, AMDVI_MMIO_COMMAND_HEAD) >>> + & AMDVI_MMIO_CMDBUF_HEAD_MASK; >>> + amdvi_cmdbuf_run(s); >>> +} >>> + >>> +static inline void amdvi_handle_cmdbase_write(AMDVIState *s) >>> +{ >>> + s->cmdbuf =3D amdvi_readq(s, AMDVI_MMIO_COMMAND_BASE) >>> + & AMDVI_MMIO_CMDBUF_BASE_MASK; >>> + s->cmdbuf_len =3D 1UL << (s->mmior[AMDVI_MMIO_CMDBUF_SIZE_BYTE] >>> + & AMDVI_MMIO_CMDBUF_SIZE_MASK); >>> + s->cmdbuf_head =3D s->cmdbuf_tail =3D 0; >>> +} >>> + >>> +static inline void amdvi_handle_cmdtail_write(AMDVIState *s) >>> +{ >>> + s->cmdbuf_tail =3D amdvi_readq(s, AMDVI_MMIO_COMMAND_TAIL) >>> + & AMDVI_MMIO_CMDBUF_TAIL_MASK; >>> + amdvi_cmdbuf_run(s); >>> +} >>> + >>> +static inline void amdvi_handle_excllim_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_EXCL_LIMIT); >>> + s->excl_limit =3D (val & AMDVI_MMIO_EXCL_LIMIT_MASK) | >>> + AMDVI_MMIO_EXCL_LIMIT_LOW; >>> +} >>> + >>> +static inline void amdvi_handle_evtbase_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_EVENT_BASE); >>> + s->evtlog =3D val & AMDVI_MMIO_EVTLOG_BASE_MASK; >>> + s->evtlog_len =3D 1UL << (*(uint64_t *)&s->mmior[AMDVI_MMIO_EVTL= OG_SIZE_BYTE] >>> + & AMDVI_MMIO_EVTLOG_SIZE_MASK); >>> +} >>> + >>> +static inline void amdvi_handle_evttail_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_EVENT_TAIL); >>> + s->evtlog_tail =3D val & AMDVI_MMIO_EVTLOG_TAIL_MASK; >>> +} >>> + >>> +static inline void amdvi_handle_evthead_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_EVENT_HEAD); >>> + s->evtlog_head =3D val & AMDVI_MMIO_EVTLOG_HEAD_MASK; >>> +} >>> + >>> +static inline void amdvi_handle_pprbase_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_PPR_BASE); >>> + s->ppr_log =3D val & AMDVI_MMIO_PPRLOG_BASE_MASK; >>> + s->pprlog_len =3D 1UL << (*(uint64_t *)&s->mmior[AMDVI_MMIO_PPRL= OG_SIZE_BYTE] >>> + & AMDVI_MMIO_PPRLOG_SIZE_MASK); >>> +} >>> + >>> +static inline void amdvi_handle_pprhead_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_PPR_HEAD); >>> + s->pprlog_head =3D val & AMDVI_MMIO_PPRLOG_HEAD_MASK; >>> +} >>> + >>> +static inline void amdvi_handle_pprtail_write(AMDVIState *s) >>> +{ >>> + uint64_t val =3D amdvi_readq(s, AMDVI_MMIO_PPR_TAIL); >>> + s->pprlog_tail =3D val & AMDVI_MMIO_PPRLOG_TAIL_MASK; >>> +} >>> + >>> +/* FIXME: something might go wrong if System Software writes in chun= ks >>> + * of one byte but linux writes in chunks of 4 bytes so currently it= >>> + * works correctly with linux but will definitely be busted if softw= are >>> + * reads/writes 8 bytes >> >> What does the spec tell us about non-dword accesses? If they aren't >> allowed, filter them out completely at the beginning. >=20 > Non-dword accesses are allowed. Spec 2.62 >=20 > ".... Accesses must be aligned to the size of the access and the size > in bytes must be a power > of two. Software may use accesses as small as one byte..... " >=20 > Linux uses dword accesses though but even in this case a change in the > order of access whereby the high part of the register is accessed > first then the lower accessed could cause a problem (??) I do not get yet what makes it tricky to support all allowed access sizes. You are just missing byte access, and that will easy to add once the size dispatching is done in a single function like suggested below. >=20 >> >>> + */ >>> +static void amdvi_mmio_write(void *opaque, hwaddr addr, uint64_t val= , >>> + unsigned size) >>> +{ >>> + AMDVIState *s =3D opaque; >>> + unsigned long offset =3D addr & 0x07; >>> + >>> + if (addr + size > AMDVI_MMIO_SIZE) { >>> + trace_amdvi_mmio_write("error: addr outside region: max ", >>> + (uint64_t)AMDVI_MMIO_SIZE, size, val, offset); >>> + return; >>> + } >>> + >>> + switch (addr & ~0x07) { >>> + case AMDVI_MMIO_CONTROL: >>> + trace_amdvi_mmio_write("MMIO_CONTROL", addr, size, val, offs= et); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >> >> This pattern repeats and screams for being factored out into a helper >> function. >> >>> + amdvi_handle_control_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_DEVICE_TABLE: >>> + trace_amdvi_mmio_write("MMIO_DEVICE_TABLE", addr, size, val,= offset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + >>> + /* set device table address >>> + * This also suffers from inability to tell whether softwar= e >>> + * is done writing >>> + */ >>> + >>> + if (offset || (size =3D=3D 8)) { >>> + amdvi_handle_devtab_write(s); >>> + } >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_HEAD: >>> + trace_amdvi_mmio_write("MMIO_COMMAND_HEAD", addr, size, val,= offset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + >>> + amdvi_handle_cmdhead_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_BASE: >>> + trace_amdvi_mmio_write("MMIO_COMMAND_BASE", addr, size, val,= offset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + >>> + /* FIXME - make sure System Software has finished writing in= case >>> + * it writes in chucks less than 8 bytes in a robust way.As = for >>> + * now, this hacks works for the linux driver >>> + */ >>> + if (offset || (size =3D=3D 8)) { >>> + amdvi_handle_cmdbase_write(s); >>> + } >>> + break; >>> + >>> + case AMDVI_MMIO_COMMAND_TAIL: >>> + trace_amdvi_mmio_write("MMIO_COMMAND_TAIL", addr, size, val,= offset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_cmdtail_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_BASE: >>> + trace_amdvi_mmio_write("MMIO_EVENT_BASE", addr, size, val, o= ffset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_evtbase_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_HEAD: >>> + trace_amdvi_mmio_write("MMIO_EVENT_HEAD", addr, size, val, o= ffset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_evthead_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_EVENT_TAIL: >>> + trace_amdvi_mmio_write("MMIO_EVENT_TAIL", addr, size, val, o= ffset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_evttail_write(s); >>> + break; >>> + >>> + case AMDVI_MMIO_EXCL_LIMIT: >>> + trace_amdvi_mmio_write("MMIO_EXCL_LIMIT", addr, size, val, o= ffset); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_excllim_write(s); >>> + break; >>> + >>> + /* PPR log base - unused for now */ >>> + case AMDVI_MMIO_PPR_BASE: >>> + trace_amdvi_mmio_write("MMIO_PPR_BASE", addr, size, val, off= set); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_pprbase_write(s); >>> + break; >>> + /* PPR log head - also unused for now */ >>> + case AMDVI_MMIO_PPR_HEAD: >>> + trace_amdvi_mmio_write("MMIO_PPR_HEAD", addr, size, val, off= set); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_pprhead_write(s); >>> + break; >>> + /* PPR log tail - unused for now */ >>> + case AMDVI_MMIO_PPR_TAIL: >>> + trace_amdvi_mmio_write("MMIO_PPR_TAIL", addr, size, val, off= set); >>> + if (size =3D=3D 2) { >>> + amdvi_writew(s, addr, val); >>> + } else if (size =3D=3D 4) { >>> + amdvi_writel(s, addr, val); >>> + } else if (size =3D=3D 8) { >>> + amdvi_writeq(s, addr, val); >>> + } >>> + amdvi_handle_pprtail_write(s); >>> + break; >>> + >>> + /* ignore write to ext_features */ >>> + default: >>> + trace_amdvi_mmio_write("UNHANDLED MMIO", addr, size, val, of= fset); >>> + } >>> + >>> +} >>> + >>> + >>> +/* PCI SIG constants */ >>> +#define PCI_BUS_MAX 256 >>> +#define PCI_SLOT_MAX 32 >>> +#define PCI_FUNC_MAX 8 >>> +#define PCI_DEVFN_MAX 256 >> >> Shouldn't those four go to the pci header? >=20 > The macros/defines in PCI header are picked from linux while some of > these are not picked from linux. I'v prefixed them with AMDVI_ though. They are not AMDVI-specific, rather PCI-generic. Jan --DVqvG4aPnrh53noqdKOaXs7p0R9WglHCG Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEYEARECAAYFAld59wgACgkQitSsb3rl5xRfjQCfclrjaUkWe43M2lqxF/nyzwCM E5wAoNYWEMNUfUUzQ5m2D0XshiIWwwPg =Wl3q -----END PGP SIGNATURE----- --DVqvG4aPnrh53noqdKOaXs7p0R9WglHCG--