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

[David Kiarie]

On Tue, Aug 9, 2016 at 8:44 AM, Peter Xu <address@hidden> wrote:

> On Tue, Aug 02, 2016 at 11:39:06AM +0300, David Kiarie wrote:
>
> [...]
>
> > +/* external write */
> > +static void amdvi_writew(AMDVIState *s, hwaddr addr, uint16_t val)
> > +{
> > +    uint16_t romask = lduw_le_p(&s->romask[addr]);
> > +    uint16_t w1cmask = lduw_le_p(&s->w1cmask[addr]);
> > +    uint16_t oldval = lduw_le_p(&s->mmior[addr]);
> > +    stw_le_p(&s->mmior[addr], (val & ~(val & w1cmask)) | (romask &
> oldval));
>
> I think the above is problematic, e.g., what if we write 1 to one of
> the romask while it's 0 originally? In that case, the RO bit will be
> written to 1.
>
> Maybe we need:
>
>   stw_le_p(&s->mmior[addr], ((oldval & romask) | (val & ~romask)) & \
>                             (val & w1cmask));
>
> Same question to the below two functions.
>

It seems to me you're not taking care of w1/c bits correctly ?

I think:

stw_le_p(&s->mmior[addr], ((oldval & romask) | (val & ~romask)) & \
                           ~ (val & w1cmask));
should suffice.


> > +}
> > +/*
> > + * 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?
>

The code is correct but the comment above is misleading.


>
> 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.
>
> > +}
> > +/* log an error encountered page-walking
>
> Thanks,
>
> -- peterx
>