On Wed, Aug 10, 2016 at 5:08 AM, Peter Xu <pet...@redhat.com> wrote: > 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". >
I changed that to "encountered during a page walk" - I'm sure no one has a problem with that :-) > > > > > > > > + * > > > > + * @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. > I mean Qemu PCI bus doesn't abort any transactions regardless of whether a device has set abort status. > -- peterx >