From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXIwY-0003l2-Ez for qemu-devel@nongnu.org; Tue, 09 Aug 2016 22:08:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXIwT-0002IV-9k for qemu-devel@nongnu.org; Tue, 09 Aug 2016 22:08:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36584) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXIwT-0002IQ-1h for qemu-devel@nongnu.org; Tue, 09 Aug 2016 22:08:25 -0400 Date: Wed, 10 Aug 2016 10:08:20 +0800 From: Peter Xu Message-ID: <20160810020820.GD4201@pxdev.xzpeter.org> References: <1470127147-12442-1-git-send-email-davidkiarie4@gmail.com> <1470127147-12442-4-git-send-email-davidkiarie4@gmail.com> <20160809054434.GA3979@pxdev.xzpeter.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [V15 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 , rkrcmar@redhat.com, Jan Kiszka , Valentine Sinitsyn , Eduardo Habkost , "Michael S. Tsirkin" On Tue, Aug 09, 2016 at 03:52:07PM +0300, David Kiarie wrote: [...] > > > + if (dma_memory_write(&address_space_memory, s->evtlog_len + > > s->evtlog_tail, > > > + &evt, AMDVI_EVENT_LEN)) { > > > > Check with MEMTX_OK? > > > > I'm not sure what exactly you mean here. I mean we have return code macros for these memory operations, like MEMTX_OK/MEMTX_ERROR/... However please feel free to ignore this comment since I see merely no place in current QEMU code that is doing the checking at all. Your call. > > > > > > [...] > > > > > +/* > > > + * AMDVi event structure > > > + * 0:15 -> DeviceID > > > + * 55:63 -> event type + miscellaneous info > > > + * 64:127 -> related address > > > + */ > > > +static void amdvi_encode_event(uint64_t *evt, uint16_t devid, uint64_t > > addr, > > > + uint16_t info) > > > +{ > > > + amdvi_setevent_bits(evt, devid, 0, 16); > > > + amdvi_setevent_bits(evt, info, 55, 8); > > > + amdvi_setevent_bits(evt, addr, 63, 64); > > ^^ > > should here be 64? > > > > Also, I am not sure whether we need this amdvi_setevent_bits() if it's > > only used in this function. Though not a big problem for me. > > > > It's only used in this function but I actually wrote his mainly for future > use. The idea is that various events encode totally different information > while the above is an over-simplified version to encode information common > to most events. In case an event wants to encode more information it would > turn out much more easier. Yes my above comment is "Nit" for sure. :) Please have it if you like. > > > > > > > +} > > > +/* log an error encountered page-walking > > > > "during page-walking" > > > > "encountered page-walking" sounds right to me. "page-walking" is a verb, > in continuous tense, right ? how about I say "during hacking" ;-) I am not that good at English. I pointed that out since I "suspect" that is wrong (in case that would help). But if you are confident enough, please just ignore. I'm mostly ok with all comments as long as they are "understandable". > > > > > + * > > > + * @addr: virtual address in translation request > > > + */ > > > +static void amdvi_page_fault(AMDVIState *s, uint16_t devid, > > > + hwaddr addr, uint16_t info) > > > +{ > > > + uint64_t evt[4]; > > > + > > > + info |= AMDVI_EVENT_IOPF_I | AMDVI_EVENT_IOPF; > > > + amdvi_encode_event(evt, devid, addr, info); > > > + amdvi_log_event(s, evt); > > > + pci_word_test_and_set_mask(s->pci.dev.config + PCI_STATUS, > > > + PCI_STATUS_SIG_TARGET_ABORT); > > > > Nit: maybe we can provide a function for setting this bit. > > > > I've actually being ignoring these since Qemu doesn't seem to care about > them. > Sorry I failed to understand your sentence. -- peterx