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

[Alistair Francis <alistair23@gmail.com>]

On Sun, Oct 18, 2020 at 8:58 AM Richard Henderson
<richard.henderson@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.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.

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~
>