Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

[Richard Henderson]

On 10/18/20 5:03 AM, Georg Kotheimer wrote:
> According to the specification the "field SPVP of hstatus controls the
> privilege level of the access" for the hypervisor virtual-machine load
> and store instructions HLV, HLVX and HSV.
> 
> We introduce the new virtualization register field HS_HYP_LD_ST,
> similar to HS_TWO_STAGE, which tracks whether we are currently
> executing a hypervisor virtual-macine load or store instruction.
> 
> Signed-off-by: Georg Kotheimer <georg.kotheimer@kernkonzept.com>

Well, let me start by mentioning the existing implementation of hyp_load et al
is broken.  I guess I wasn't paying attention when Alistair implemented them.

Here's the problem: When you change how riscv_cpu_tlb_fill works, as you are by
modifying get_physical_address, you have to remember that those results are
*saved* in the qemu tlb.

So by setting some flags, performing one memory operation, and resetting the
flags, we have in fact corrupted the tlb for the next operation without those
flags.

You need to either:

(1) perform the memory operation *without* using the qemu tlb machinery (i.e.
use get_physical_address directly, then use address_space_ld/st), or

(2) add a new mmu index for the HLV/HSV operation, with all of the differences
implied.

The second is imo preferable, as it means that helper_hyp_load et al can go
away and become normal qemu_ld/st opcodes with the new mmu indexes.

Annoyingly, for the moment you wouldn't be able to remove helper_hyp_x_load,
because there's no qemu_ld variant that uses execute permission.  But it can be
done with helpers.  I'll note that the current implementation is broken,
because it uses cpu_lduw_data_ra, and not cpu_lduw_code_ra, which is exactly
the difference that uses execute permissions.  After the conversion to the new
mmuidx, you would use cpu_lduw_mmuidx_ra.  I would also split the function into
two, so that one performs HLVX.HU and the other HLVX.WU, so that you don't have
to pass the size as a parameter.

Finally, this patch, changing behaviour when hstatus.SPVP changes... depends on
how often this bit is expected to be toggled.

If the bit toggles frequently, e.g. around some small section of kernel code,
then one might copy the SPVP bit into tlb_flags and use two different mmu
indexes to imply the state of the SPVP bit.

If the bit does not toggle frequently, e.g. whatever bit of kernel code runs
infrequently, or it only happens around priv level changes, then simply
flushing the qemu tlb when the SPVP bit changes is sufficient.  You then get to
look at SPVP directly within tlb_fill.


r~