On Tue, Aug 09, 2016 at 08:46:09PM +0300, David Kiarie wrote: > 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.
Right. :) -- peterx