Re: [Qemu-devel] [V15 3/4] hw/i386: Introduce AMD IOMMU

[Peter Xu <peterx@redhat.com>]

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