[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instruct
Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions
Wed, 21 Oct 2020 12:21:04 -0700
On Sun, Oct 18, 2020 at 8:58 AM 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 <firstname.lastname@example.org>
> 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
> 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
> 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
> 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
> 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
> two, so that one performs HLVX.HU and the other HLVX.WU, so that you don't
> to pass the size as a parameter.
I'm not clear what you mean here.
> Finally, this patch, changing behaviour when hstatus.SPVP changes... depends
> 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
> look at SPVP directly within tlb_fill.