Re: [PATCH v2 07/14] xen/riscv: introduce exception context

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2023-01-31 at 17:30 -0800, Stefano Stabellini wrote:
> On Mon, 30 Jan 2023, Jan Beulich wrote:
> > On 30.01.2023 12:54, Oleksii wrote:
> > > Hi Jan,
> > > 
> > > On Fri, 2023-01-27 at 15:24 +0100, Jan Beulich wrote:
> > > > On 27.01.2023 14:59, Oleksii Kurochko wrote:
> > > > > --- /dev/null
> > > > > +++ b/xen/arch/riscv/include/asm/processor.h
> > > > > @@ -0,0 +1,82 @@
> > > > > +/* SPDX-License-Identifier: MIT */
> > > > > +/***********************************************************
> > > > > ******
> > > > > *************
> > > > > + *
> > > > > + * Copyright 2019 (C) Alistair Francis
> > > > > <alistair.francis@xxxxxxx>
> > > > > + * Copyright 2021 (C) Bobby Eshleman
> > > > > <bobby.eshleman@xxxxxxxxx>
> > > > > + * Copyright 2023 (C) Vates
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#ifndef _ASM_RISCV_PROCESSOR_H
> > > > > +#define _ASM_RISCV_PROCESSOR_H
> > > > > +
> > > > > +#ifndef __ASSEMBLY__
> > > > > +
> > > > > +/* On stack VCPU state */
> > > > > +struct cpu_user_regs
> > > > > +{
> > > > > +    unsigned long zero;
> > > > > +    unsigned long ra;
> > > > > +    unsigned long sp;
> > > > > +    unsigned long gp;
> > > > > +    unsigned long tp;
> > > > > +    unsigned long t0;
> > > > > +    unsigned long t1;
> > > > > +    unsigned long t2;
> > > > > +    unsigned long s0;
> > > > > +    unsigned long s1;
> > > > > +    unsigned long a0;
> > > > > +    unsigned long a1;
> > > > > +    unsigned long a2;
> > > > > +    unsigned long a3;
> > > > > +    unsigned long a4;
> > > > > +    unsigned long a5;
> > > > > +    unsigned long a6;
> > > > > +    unsigned long a7;
> > > > > +    unsigned long s2;
> > > > > +    unsigned long s3;
> > > > > +    unsigned long s4;
> > > > > +    unsigned long s5;
> > > > > +    unsigned long s6;
> > > > > +    unsigned long s7;
> > > > > +    unsigned long s8;
> > > > > +    unsigned long s9;
> > > > > +    unsigned long s10;
> > > > > +    unsigned long s11;
> > > > > +    unsigned long t3;
> > > > > +    unsigned long t4;
> > > > > +    unsigned long t5;
> > > > > +    unsigned long t6;
> > > > > +    unsigned long sepc;
> > > > > +    unsigned long sstatus;
> > > > > +    /* pointer to previous stack_cpu_regs */
> > > > > +    unsigned long pregs;
> > > > > +};
> > > > 
> > > > Just to restate what I said on the earlier version: We have a
> > > > struct
> > > > of
> > > > this name in the public interface for x86. Besides to confusion
> > > > about
> > > > re-using the name for something private, I'd still like to
> > > > understand
> > > > what the public interface plans are. This is specifically
> > > > because I
> > > > think it would be better to re-use suitable public interface
> > > > structs
> > > > internally where possible. But that of course requires spelling
> > > > out
> > > > such parts of the public interface first.
> > > > 
> > > I am not sure that I get you here.
> > > I greped a little bit and found out that each architecture
> > > declares
> > > this structure inside arch-specific folder.
> > > 
> > > Mostly the usage of the cpu_user_regs is to save/restore current
> > > state
> > > of CPU during traps ( exceptions/interrupts ) and
> > > context_switch().
> > 
> > Arm effectively duplicates the public interface struct
> > vcpu_guest_core_regs
> > and the internal struct cpu_user_regs (and this goes as far as also
> > duplicating the __DECL_REG() helper). Personally I find such
> > duplication
> > odd at the first glance at least; maybe there is a specific reason
> > for this
> > in Arm. But whether the public interface struct can be re-used can
> > likely
> > only be known once it was spelled out.
> 
> struct vcpu_guest_core_regs is used as part of struct
> vcpu_guest_context, which is used for VCPUOP_initialise, which is not
> used on ARM and RISC-V (we use a standard firmware interface
> instead),
> and for save/restore, which also is not going to work any time soon
> on
> ARM and RISC-V.
> 
> This is to say that we are probably not going to need the public
> interface for the next year or two, so it is difficult to tell at
> this
> stage if it aligns well with struct cpu_user_regs. I think we'll have
> to
> cross that bridge when we come to it.
> 
Agree that it will be better to return to the public interface later.
So I'll this part of the patch series as is now.
> 
> > > Also some registers are modified during construction of a domain.
> > > Thereby I prefer here to see the arch-specific register names
> > > instead
> > > of common.
> > 
> > Not sure what meaning of "common" you imply here. Surely register
> > names
> > want to be arch-specific, and hence can't be "common" with other
> > arch-es.
> 
> I think Oleksii misunderstood your request and believed you were
> asking
> him to make struct cpu_user_regs common across arches.
Yeah, that's what I thought at first...

~ Oleksii