Re: [Qemu-devel] [PATCH v2 26/28] s390x/tcg: MVST: Fault-safe handling
On 11.09.19 23:52, Richard Henderson wrote: > On 9/6/19 3:57 AM, David Hildenbrand wrote: >> +/* >> + * Our access should not exceed single pages, as we must not report >> access >> + * exceptions exceeding the actually copied range (which we don't know >> at >> + * this point). We might over-indicate watchpoints within the pages >> + * (if we ever care, we have to limit processing to a single byte). >> + */ >> +srca = access_prepare(env, s, len, MMU_DATA_LOAD, ra); >> +desta = access_prepare(env, d, len, MMU_DATA_STORE, ra); >> +for (i = 0; i < len; i++) { >> +const uint8_t v = access_get_byte(env, , i, ra); >> + >> +access_set_byte(env, , i, v, ra); >> if (v == c) { >> -set_address_zero(env, r1, d + len); >> +set_address_zero(env, r1, d + i); >> return 1; >> } > > Worth using memchr + memmove w/ nondestructive overlap? In theory yes, however, the issue is that we would have multiple accesses, which is not documented for this instruction. In case the memory is modified between memchr + memmove by another CPU, we could have an inconsistent instruction result. Unlikely but possible :) > > That said, > Reviewed-by: Richard Henderson > > r~ > -- Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 26/28] s390x/tcg: MVST: Fault-safe handling
On 9/6/19 3:57 AM, David Hildenbrand wrote: > +/* > + * Our access should not exceed single pages, as we must not report > access > + * exceptions exceeding the actually copied range (which we don't know at > + * this point). We might over-indicate watchpoints within the pages > + * (if we ever care, we have to limit processing to a single byte). > + */ > +srca = access_prepare(env, s, len, MMU_DATA_LOAD, ra); > +desta = access_prepare(env, d, len, MMU_DATA_STORE, ra); > +for (i = 0; i < len; i++) { > +const uint8_t v = access_get_byte(env, , i, ra); > + > +access_set_byte(env, , i, v, ra); > if (v == c) { > -set_address_zero(env, r1, d + len); > +set_address_zero(env, r1, d + i); > return 1; > } Worth using memchr + memmove w/ nondestructive overlap? That said, Reviewed-by: Richard Henderson r~
[Qemu-devel] [PATCH v2 26/28] s390x/tcg: MVST: Fault-safe handling
Access at most single pages and document why. Using the access helpers might over-indicate watchpoints within the same page, I guess we can live with that. Signed-off-by: David Hildenbrand --- target/s390x/mem_helper.c | 23 --- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 4c67c6f37e..73b00b582b 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -845,21 +845,30 @@ uint32_t HELPER(mvst)(CPUS390XState *env, uint64_t c, uint32_t r1, uint32_t r2) { const uint64_t d = get_address(env, r1); const uint64_t s = get_address(env, r2); +const int len = MIN(-(d | TARGET_PAGE_MASK), -(s | TARGET_PAGE_MASK)); +S390Access srca, desta; uintptr_t ra = GETPC(); -uint32_t len; +int i; if (c & 0xff00ull) { s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra); } c = c & 0xff; -/* Lest we fail to service interrupts in a timely manner, limit the - amount of work we're willing to do. For now, let's cap at 8k. */ -for (len = 0; len < 0x2000; ++len) { -uint8_t v = cpu_ldub_data_ra(env, s + len, ra); -cpu_stb_data_ra(env, d + len, v, ra); +/* + * Our access should not exceed single pages, as we must not report access + * exceptions exceeding the actually copied range (which we don't know at + * this point). We might over-indicate watchpoints within the pages + * (if we ever care, we have to limit processing to a single byte). + */ +srca = access_prepare(env, s, len, MMU_DATA_LOAD, ra); +desta = access_prepare(env, d, len, MMU_DATA_STORE, ra); +for (i = 0; i < len; i++) { +const uint8_t v = access_get_byte(env, , i, ra); + +access_set_byte(env, , i, v, ra); if (v == c) { -set_address_zero(env, r1, d + len); +set_address_zero(env, r1, d + i); return 1; } } -- 2.21.0