On Sun, Oct 18, 2020 at 8:58 AM Richard Henderson <richard.hender...@linaro.org> wrote: > > 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.kothei...@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.
Ok, I have some patches to fix this. I'll send them soon. > > 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 I have tried to fix this as well, but it seems to break a virtual Linux guest booting. > 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. I'm not clear what you mean here. Alistair > > 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~ >