| Message ID | 652289358975cf869e4bc0a6a70e3aba7bd2fbf6.1674818705.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
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@wdc.com> > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> > + * 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. Jan
Hi Oleksii, On 27/01/2023 13:59, Oleksii Kurochko wrote: > +static inline void wfi(void) > +{ > + __asm__ __volatile__ ("wfi"); I have starred at this line for a while and I am not quite too sure to understand why we don't need to clobber the memory like we do on Arm. FWIW, Linux is doing the same, so I guess this is correct. For Arm we also follow the Linux implementation. But I am wondering whether we are just too strict on Arm, RISCv compiler offer a different guarantee, or you expect the user to be responsible to prevent the compiler to do harmful optimization. > +/* > + * panic() isn't available at the moment so an infinite loop will be > + * used temporarily. > + * TODO: change it to panic() > + */ > +static inline void die(void) > +{ > + for( ;; ) wfi(); Please move wfi() to a newline. > +}
Hi Julien, On Fri, 2023-01-27 at 14:54 +0000, Julien Grall wrote: > Hi Oleksii, > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > +static inline void wfi(void) > > +{ > > + __asm__ __volatile__ ("wfi"); > > I have starred at this line for a while and I am not quite too sure > to > understand why we don't need to clobber the memory like we do on Arm. > I don't have an answer. The code was based on Linux so... > FWIW, Linux is doing the same, so I guess this is correct. For Arm we > also follow the Linux implementation. > > But I am wondering whether we are just too strict on Arm, RISCv > compiler > offer a different guarantee, or you expect the user to be responsible > to > prevent the compiler to do harmful optimization. > > > +/* > > + * panic() isn't available at the moment so an infinite loop will > > be > > + * used temporarily. > > + * TODO: change it to panic() > > + */ > > +static inline void die(void) > > +{ > > + for( ;; ) wfi(); > > Please move wfi() to a newline. Thanks. I thought that it is fine to put into one line in this case but I'll move it to a newline. It's fine. > > > +} > ~Oleksii
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@wdc.com> > > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> > > + * 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(). Also some registers are modified during construction of a domain. Thereby I prefer here to see the arch-specific register names instead of common. > Jan ~ Oleksii
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@wdc.com> >>> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> >>> + * 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. > 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. Jan
Hi, On 30/01/2023 11:40, Oleksii wrote: > On Fri, 2023-01-27 at 14:54 +0000, Julien Grall wrote: >> Hi Oleksii, >> >> On 27/01/2023 13:59, Oleksii Kurochko wrote: >>> +static inline void wfi(void) >>> +{ >>> + __asm__ __volatile__ ("wfi"); >> >> I have starred at this line for a while and I am not quite too sure >> to >> understand why we don't need to clobber the memory like we do on Arm. >> > I don't have an answer. The code was based on Linux so... Hmmm ok. It would probably wise to understand how code imported from Linux work so we don't end up introducing bug when calling such function. From your current use in this patch, I don't expect any issue. That may chance for the others use. >> FWIW, Linux is doing the same, so I guess this is correct. For Arm we >> also follow the Linux implementation. >> >> But I am wondering whether we are just too strict on Arm, RISCv >> compiler >> offer a different guarantee, or you expect the user to be responsible >> to >> prevent the compiler to do harmful optimization. >> >>> +/* >>> + * panic() isn't available at the moment so an infinite loop will >>> be >>> + * used temporarily. >>> + * TODO: change it to panic() >>> + */ >>> +static inline void die(void) >>> +{ >>> + for( ;; ) wfi(); >> >> Please move wfi() to a newline. > Thanks. > > I thought that it is fine to put into one line in this case but I'll > move it to a newline. It's fine. I am not aware of any place in Xen where we would combine the lines. Also, you want a space after 'for'. Cheers,
Hi Jan, On 30/01/2023 13:50, 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@wdc.com> >>>> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> >>>> + * 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. There are some force padding, a different ordering and some extra fields in the internal version to simplify the assembly code. The rationale is explained in 1c38a1e937d3 ("xen: arm: separate guest user regs from internal guest state"). We also have a split between 32-bit and 64-bit to avoid doubling up the size in the former case. Cheers,
Hi Julien, On Mon, 2023-01-30 at 22:11 +0000, Julien Grall wrote: > Hi, > > On 30/01/2023 11:40, Oleksii wrote: > > On Fri, 2023-01-27 at 14:54 +0000, Julien Grall wrote: > > > Hi Oleksii, > > > > > > On 27/01/2023 13:59, Oleksii Kurochko wrote: > > > > +static inline void wfi(void) > > > > +{ > > > > + __asm__ __volatile__ ("wfi"); > > > > > > I have starred at this line for a while and I am not quite too > > > sure > > > to > > > understand why we don't need to clobber the memory like we do on > > > Arm. > > > > > I don't have an answer. The code was based on Linux so... > > Hmmm ok. It would probably wise to understand how code imported from > Linux work so we don't end up introducing bug when calling such > function. > > From your current use in this patch, I don't expect any issue. That > may > chance for the others use. > Could you please share with me a link or explain what kind of problems may occur in case when we don't clobber the memory in the others use case during usage of "wfi" ? As I understand the reason for clobber the memory is to cause GCC to not keep memory values cached in registers across the assembler instruction and not optimize stores/load to the memory. But current one instruction isn't expected to work with the memory so it should be safe enough to stall current hart ( CPU ) until an interrupt might need servicing. Anyway we can change the code to: __asm__ __volatile__ ("wfi" ::: "memory") In order to be sure that no problems will arise in the future. > > > FWIW, Linux is doing the same, so I guess this is correct. For > > > Arm we > > > also follow the Linux implementation. > > > > > > But I am wondering whether we are just too strict on Arm, RISCv > > > compiler > > > offer a different guarantee, or you expect the user to be > > > responsible > > > to > > > prevent the compiler to do harmful optimization. > > > > > > > +/* > > > > + * panic() isn't available at the moment so an infinite loop > > > > will > > > > be > > > > + * used temporarily. > > > > + * TODO: change it to panic() > > > > + */ > > > > +static inline void die(void) > > > > +{ > > > > + for( ;; ) wfi(); > > > > > > Please move wfi() to a newline. > > Thanks. > > > > I thought that it is fine to put into one line in this case but > > I'll > > move it to a newline. It's fine. > > I am not aware of any place in Xen where we would combine the lines. > Also, you want a space after 'for'. > > Cheers, > ~ Oleksii
On 31/01/2023 12:24, Oleksii wrote: > Hi Julien, Hi Oleksii, > On Mon, 2023-01-30 at 22:11 +0000, Julien Grall wrote: >> Hi, >> >> On 30/01/2023 11:40, Oleksii wrote: >>> On Fri, 2023-01-27 at 14:54 +0000, Julien Grall wrote: >>>> Hi Oleksii, >>>> >>>> On 27/01/2023 13:59, Oleksii Kurochko wrote: >>>>> +static inline void wfi(void) >>>>> +{ >>>>> + __asm__ __volatile__ ("wfi"); >>>> >>>> I have starred at this line for a while and I am not quite too >>>> sure >>>> to >>>> understand why we don't need to clobber the memory like we do on >>>> Arm. >>>> >>> I don't have an answer. The code was based on Linux so... >> >> Hmmm ok. It would probably wise to understand how code imported from >> Linux work so we don't end up introducing bug when calling such >> function. >> >> From your current use in this patch, I don't expect any issue. That >> may >> chance for the others use. >> > Could you please share with me a link or explain what kind of problems > may occur in case when we don't clobber the memory in the others use > case during usage of "wfi" ? I don't have a link and that's why I was asking the question here. The concern I have is the following: 1) wfi(); val = *addr; 2) *addr = val; wfi(); Is the compiler allowed to re-order the sequence so '*addr' will be read before 'wfi' or (for the second case) write after 'wfi'? At the moment, I believe this is why we have the 'memory' clobber on Arm. But then I couldn't find any documentation implying that the compiler cannot do the re-ordering. > > As I understand the reason for clobber the memory is to cause GCC to > not keep memory values cached in registers across the > assembler instruction and not optimize stores/load to the memory. > But current one instruction isn't expected to work with the memory so > it should be safe enough to stall current hart ( CPU ) until an > interrupt might need servicing. > > Anyway we can change the code to: > __asm__ __volatile__ ("wfi" ::: "memory") > In order to be sure that no problems will arise in the future. As I wrote earlier, so far, I didn't suggest to change any code. I am simply trying to understand how this is meant to work. One action may be that we can remove the memory clobber on Arm. Cheers,
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@wdc.com> > >>> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> > >>> + * 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. > > 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.
On 30/01/2023 10:44 pm, Julien Grall wrote: > Hi Jan, > > On 30/01/2023 13:50, 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@wdc.com> >>>>> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> >>>>> + * 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. > > There are some force padding, a different ordering and some extra > fields in the internal version to simplify the assembly code. > > The rationale is explained in 1c38a1e937d3 ("xen: arm: separate guest > user regs from internal guest state"). > > We also have a split between 32-bit and 64-bit to avoid doubling up > the size in the former case. And on top of these reasons, I feel I need to remind you that we still need to break these apart on x86 to fix a stack OoB access in the #DF handler, and to fix stack alignment for UEFI, and to remove the relics of vm86 mode, and not to mention adding support for FRED. It was a fundamental design error that Xen's internal representation ever made it into the public API, and it absolutely does not want repeating again. ~Andrew
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@wdc.com> > > > > > + * Copyright 2021 (C) Bobby Eshleman > > > > > <bobby.eshleman@gmail.com> > > > > > + * 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
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h new file mode 100644 index 0000000000..4292de2efc --- /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@wdc.com> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> + * 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; +}; + +static inline void wfi(void) +{ + __asm__ __volatile__ ("wfi"); +} + +/* + * panic() isn't available at the moment so an infinite loop will be + * used temporarily. + * TODO: change it to panic() + */ +static inline void die(void) +{ + for( ;; ) wfi(); +} + +#endif /* __ASSEMBLY__ */ + +#endif /* _ASM_RISCV_PROCESSOR_H */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/riscv/riscv64/asm-offsets.c b/xen/arch/riscv/riscv64/asm-offsets.c index e69de29bb2..d632b75c2a 100644 --- a/xen/arch/riscv/riscv64/asm-offsets.c +++ b/xen/arch/riscv/riscv64/asm-offsets.c @@ -0,0 +1,53 @@ +#define COMPILE_OFFSETS + +#include <asm/processor.h> +#include <xen/types.h> + +#define DEFINE(_sym, _val) \ + asm volatile ("\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\"" \ + : : "i" (_val) ) +#define BLANK() \ + asm volatile ( "\n.ascii\"==><==\"" : : ) +#define OFFSET(_sym, _str, _mem) \ + DEFINE(_sym, offsetof(_str, _mem)); + +void asm_offsets(void) +{ + BLANK(); + DEFINE(CPU_USER_REGS_SIZE, sizeof(struct cpu_user_regs)); + OFFSET(CPU_USER_REGS_ZERO, struct cpu_user_regs, zero); + OFFSET(CPU_USER_REGS_RA, struct cpu_user_regs, ra); + OFFSET(CPU_USER_REGS_SP, struct cpu_user_regs, sp); + OFFSET(CPU_USER_REGS_GP, struct cpu_user_regs, gp); + OFFSET(CPU_USER_REGS_TP, struct cpu_user_regs, tp); + OFFSET(CPU_USER_REGS_T0, struct cpu_user_regs, t0); + OFFSET(CPU_USER_REGS_T1, struct cpu_user_regs, t1); + OFFSET(CPU_USER_REGS_T2, struct cpu_user_regs, t2); + OFFSET(CPU_USER_REGS_S0, struct cpu_user_regs, s0); + OFFSET(CPU_USER_REGS_S1, struct cpu_user_regs, s1); + OFFSET(CPU_USER_REGS_A0, struct cpu_user_regs, a0); + OFFSET(CPU_USER_REGS_A1, struct cpu_user_regs, a1); + OFFSET(CPU_USER_REGS_A2, struct cpu_user_regs, a2); + OFFSET(CPU_USER_REGS_A3, struct cpu_user_regs, a3); + OFFSET(CPU_USER_REGS_A4, struct cpu_user_regs, a4); + OFFSET(CPU_USER_REGS_A5, struct cpu_user_regs, a5); + OFFSET(CPU_USER_REGS_A6, struct cpu_user_regs, a6); + OFFSET(CPU_USER_REGS_A7, struct cpu_user_regs, a7); + OFFSET(CPU_USER_REGS_S2, struct cpu_user_regs, s2); + OFFSET(CPU_USER_REGS_S3, struct cpu_user_regs, s3); + OFFSET(CPU_USER_REGS_S4, struct cpu_user_regs, s4); + OFFSET(CPU_USER_REGS_S5, struct cpu_user_regs, s5); + OFFSET(CPU_USER_REGS_S6, struct cpu_user_regs, s6); + OFFSET(CPU_USER_REGS_S7, struct cpu_user_regs, s7); + OFFSET(CPU_USER_REGS_S8, struct cpu_user_regs, s8); + OFFSET(CPU_USER_REGS_S9, struct cpu_user_regs, s9); + OFFSET(CPU_USER_REGS_S10, struct cpu_user_regs, s10); + OFFSET(CPU_USER_REGS_S11, struct cpu_user_regs, s11); + OFFSET(CPU_USER_REGS_T3, struct cpu_user_regs, t3); + OFFSET(CPU_USER_REGS_T4, struct cpu_user_regs, t4); + OFFSET(CPU_USER_REGS_T5, struct cpu_user_regs, t5); + OFFSET(CPU_USER_REGS_T6, struct cpu_user_regs, t6); + OFFSET(CPU_USER_REGS_SEPC, struct cpu_user_regs, sepc); + OFFSET(CPU_USER_REGS_SSTATUS, struct cpu_user_regs, sstatus); + OFFSET(CPU_USER_REGS_PREGS, struct cpu_user_regs, pregs); +}
The patch introduces a set of registers which should be saved to and restored from a stack after an exception occurs and a set of defines which will be used during exception context saving/restoring. Originally <asm/processor.h> header was introduced in the patch series from Bobby so partially it was re-used and the following changes were done: - Move all RISCV_CPU_USER_REGS_* to asm/asm-offsets.c - Remove RISCV_CPU_USER_REGS_OFFSET & RISCV_CPU_USER_REGS_SIZE as there is no sense in them after RISCV_CPU_USER_REGS_* were moved to asm/asm-offsets.c - Remove RISCV_PCPUINFO_* as they aren't needed for current status of the RISC-V port - register_t renamed to unsigned long - rename wait_for_interrupt to wfi Signed-off-by: Bobby Eshleman <bobby.eshleman@gmail.com> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - All the changes were added to the commit message. - temporarily was added function die() to stop exectution it will be removed after panic() will be available. --- xen/arch/riscv/include/asm/processor.h | 82 ++++++++++++++++++++++++++ xen/arch/riscv/riscv64/asm-offsets.c | 53 +++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 xen/arch/riscv/include/asm/processor.h