On Tue, Aug 9, 2016 at 8:44 AM, Peter Xu <pet...@redhat.com> 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 >