qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Kiarie
Subject: Re: [Qemu-devel] [V15 3/4] hw/i386: Introduce AMD IOMMU
Date: Wed, 10 Aug 2016 09:30:09 +0300

On Wed, Aug 10, 2016 at 5:08 AM, Peter Xu <address@hidden> 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
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]