Re: [PATCH v1 14/17] xen/riscv: add guest page fault handling stub

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 8/18/26 10:29 AM, Jan Beulich wrote:
> On 17.08.2026 18:10, Oleksii Kurochko wrote:
> > On 8/12/26 5:48 PM, Jan Beulich wrote:
> > > On 20.07.2026 18:02, Oleksii Kurochko wrote:
> > > > --- a/xen/arch/riscv/traps.c
> > > > +++ b/xen/arch/riscv/traps.c
> > > > @@ -191,6 +191,67 @@ static void timer_interrupt(void)
> > > >        raise_softirq(TIMER_SOFTIRQ);
> > > >    }
> > > >    
> > > > +static always_inline unsigned long get_faulting_gpa(void)
> > > May I suggest to use always_inline only when inlining is _functionally_
> > > required?
> > Sure. But it ins't clear to me why it isn't a case here? Is it connected
> > to that function is static and too simple so a compiler will do by itself?
> Counter question: What is it that would functionally break if the function
> ended up not being inlined? (This is the question you generally need to
> answer to justify use of always_inline. Of course there's the additional
> case of performance being affected, but I don't view that as applicable
> here; I'm open to be proven wrong, though.)
Now it is clear how to identrify if function should be always_inline.

I put it only for the purpose to be sure that this function won't be 
called with prologue/epilogue but I agree that compiler will do that by 
itself.
> > > > +{
> > > > +    /*
> > > > +     * According to RISC-V spec:
> > > > +     *  18.2.8. Hypervisor Trap Value Register (htval)
> > > > +     *   ...
> > > > +     *   A guest physical address written to htval is shifted right by 2 bits
> > > > +     *   to accommodate addresses wider than the current XLEN.
> > > > +     *   ...
> > > > +     *   If the least-significant two bits of a faulting guest physical address
> > > > +     *   are needed, these bits are ordinarily the same as the
> > > > +     *   least-significant two bits of the faulting virtual address in stval.
> > > > +     *   For faults due to implicit memory accesses for VS-stage address
> > > > +     *   translation, the least-significant two bits are instead zeros. These
> > > > +     *   cases can be distinguished using the value provided in register 
> > > > htinst.
> > > > +     */
> > > > +    return (csr_read(CSR_HTVAL) << 2) | (csr_read(CSR_STVAL) & 0x3);
> > > Well, okay, but instead of not losing the bottom two bits you're now losing
> > > the top two ones.
> > Oh, right, I will add a cast ((uint64_t)csr_read(CSR_HTVAL) << 2) | ...
> >
> > It will cover all the cases RV32 which has 34-bit guest address and it
> > will be enough for RV64 where GPA is 59bit (the highest possible for Sv59).
> Only if the function return type then also changes.
>
> > > Also the spec reads as if htval only _may_ hold the original address of the
> > > faulting access. What if htval ends up 0?
> > good point. then we have to emulate fault instruction and get an address
> > from an instruction. I think that for now it will be enough just to
> > support platforms which always write GPA to HTVAL.
> >
> > If I understand correctly if htval is supported by platform then htval
> > will be always filled for guest page fault. To verify if HTVAL is
> > supported we could do:
> >
> > 'Unless it has reason to assume otherwise (such as a platform standard),
> > software that writes a value to htval should read back from htval to
> > confirm the stored value.'
> How does this matter here? It's one thing for htval to be capable of
> holding (all?) non-zero values, and another that it would always be
> written. If the platform doesn't indicate the behavior, I fear you have
> to assume that you may (perhaps even randomly) observe 0.
So to be very sure we could check for two extensions: Sstval and Shtval.
They will guarantee that under any circumstances it will be filled.

Also, as an option we could check that htinst value isn't zero as 
according to the spec:
For guest-page faults, the trap instruction register is written with a 
special pseudoinstruction value if:
(a) the fault is caused by an implicit memory access for VS-stage 
address translation, and (b) a nonzero
value (the faulting guest physical address) is written to mtval2 or htval.

So if htinst != 0 then htval is filled with GPA and a nonzero guest 
physical address written to mtval2/htval shall correspond to the exact 
virtual address written to mtval/stval.
But if htinst is 0 then we have to do VS-stage software pagewalk to get 
GPA and also we will need to decode instruction to get GVA.
I am thinking if it will be okay for now to cover the case htinst != 0 
and have BUG_ON(!htinst) to not miss that the possible future case when 
VS-stage s/w page walks and parsing of GVA from an instruction are 
needed. I think it is fine as all real boards on which I was able to 
test Xen has htval and stval properly filled and of course QEMU code 
guarantees that htval and stval will be properly filled in the case of QEMU.
Also, KVM is based also only htval and stval and I assume that they 
tested it on real hardware too so it seems like it is okay to go with 
solution that for now we are using htval + stval to get faulty address.
> > And is it true because:
> > ```
> > A value of zero in mtval signifies either that the feature is not
> > supported, or an illegal zero instruction was fetched.
> > ```
> > (yes, it is about mtval but I asssume that htval has the same behaviour').
> Right, but what you quote is specific to illegal instruction exceptions.
Oh, right.

~ Oleksii