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
