Re: [PATCH 15/19] powerpc: htab routines for Celleb

[Ishizaki Kou <kou.ishizaki@toshiba.co.jp>]

Ben-san,

Thank you for you comment.

> > +
> > +	dummy0 = beat_lpar_hpte_getword0(slot);
> > +	if ((dummy0 & ~0x7FUL) != (want_v & ~0x7FUL)) {
> > +	   DBG_LOW("not found !\n");
> > +			return -1;
> > +			}
> > +
> > +	lpar_rc = beat_write_htab_entry(0, slot, 0, newpp, 0, 7, &dummy0,
> > +						 &dummy1);
> > +						 if (lpar_rc != 0 || dummy0 == 0) {
> > +						    DBG_LOW("not found !\n");
> > +								 return -1;
> > +								 }
> > +
> > +	DBG_LOW("ok %lx %lx\n", dummy0, dummy1);
> > +

> I just noticed... isn't there a race condition if the HPTE gets evicted
> between those two calls and/or replaced by another one ?

> That would be generally harmless unless the one replacing it is bolted,
> in which case you might override a bolted entry. Fortunately we don't
> currently insert much bolted entries after boot, but I still see a
> potential for trouble there.

> The best solution here is to actually add a function to the hypervisor,
> along the line of what I described in my document a while ago, which
> atomically compares AVPN and replaces the PP and N bits if it didn't
> change.
> 
> Another possible issue is that two processors or threads might
> continuously evict each other that way I think.
> 
> Since the bug only affect your platform and might actually not trigger
> at all in the current system since we don't play much with bolted
> entries, I'm acking it, but you should look into fixing it.

We understand your opinion and we should fix by locking all
table handlers by spinlock. We are now trying to add new functions
for our hypervisor to solve this problem, but we must support current
hypervisor, so we have to have a fix on current implementation.

Best regards,
Kou Ishizaki