[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 05/17] RISC-V: add vector extension load and
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH v2 05/17] RISC-V: add vector extension load and store instructions |
Date: |
Wed, 8 Jan 2020 12:08:15 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 |
On 1/8/20 11:32 AM, LIU Zhiwei wrote:
>>> + switch (width) {
>>> + case 8:
>>> + if (vector_elem_mask(env, vm, width, lmul, i)) {
>>> + while (k >= 0) {
>>> + read = i * (nf + 1) + k;
>>> + env->vfp.vreg[dest + k * lmul].u8[j] =
>>> + cpu_ldub_data(env, env->gpr[rs1] + read);
>> You must not modify vreg[x] before you've recognized all possible exceptions,
>> e.g. validating that a subsequent access will not trigger a page fault.
>> Otherwise you will have a partially modified register value when the
>> exception
>> handler is entered.
> There are two questions here.
>
> 1) How to validate access before real access to registers?
>
> As pointed in another comment for patchset v1,
>
> "instructions that perform more than one host store must probe
> the entire range to be stored before performing any stores.
> "
Use probe_access (or one of the probe_write/probe_read helpers).
Ideally one would then use the result, which is a host address, and perform
direct loads/stores using that. The result may be null, indicating that the
operation needs the i/o path. But in any case, after the probe we are
guaranteed that the page is mapped and readable/writable.
Note that probe_* does not allow [addr, addr+size) to cross a page boundary.
So you do have to be prepared for the vector operation to consist of 2 pages,
and probe both of them.
> I didn't see the validation of page in SVE, for example, sve_st1_r,
> which directly use the helper_ret_*_mmu that may cause an page fault
> exception or ovelap a watchpoint,
> before probe the entire range to be stored .
Yes, this is a bug in SVE that will be fixed.
Note that you should not use helper_ret_* anymore. I've just introduced
cpu_{ld,st}*_mmuidx_ra() that should be used instead.
> 2) Why not use the cpu_ld* API?
It's possible to use cpu_ld*, but then you need to store the results into a
temporary, and copy the result to the register afterward.
But I think it's better to probe first and avoid a second copy.
> I see in SVE that ld*_p is used to directly access the host memory. And
> helper_ret_*_mmu
> is used to access guest memory. But from the definition of cpu_ld*, it's the
> combination of
> ld*_p and helper_ret_*_mmu.
This is all changed now, FWIW.
> I will take it. However I didn't have a big-endian host to test the feature.
You can apply for a gcc compile farm account, and then you will have access to
ppc64 big-endian hosts.
https://cfarm.tetaneutral.net/users/new/
r~