| Message ID | bb6191b21bd387f265e0e25322a30f4ade6e8b3b.1733937787.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unflattening and relocation of host device tree | expand |
On 11.12.2024 18:27, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/include/asm/page.h > +++ b/xen/arch/riscv/include/asm/page.h > @@ -7,6 +7,7 @@ > > #include <xen/bug.h> > #include <xen/const.h> > +#include <xen/errno.h> > #include <xen/types.h> > > #include <asm/atomic.h> > @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) > return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); > } > > +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) > +{ > +#ifdef CONFIG_QEMU > + return 0; > +#else > + return -EOPNOTSUPP; > +#endif > +} > + > +static inline int clean_dcache_va_range(const void *p, unsigned long size) > +{ > +#ifdef CONFIG_QEMU > + return 0; > +#else > + return -EOPNOTSUPP; > +#endif > +} So testing on real hardware will then effectively become impossible, until someone goes and implements these? > --- /dev/null > +++ b/xen/arch/riscv/platforms/Kconfig > @@ -0,0 +1,5 @@ > +config QEMU > + bool "QEMU aarch virt machine support" > + depends on RISCV_64 I understand Arm has it like this, but: Is QEMU really a sufficiently non- ambiguous name to use? Is the RISCV_64 dependency really needed? > + help > + Enable all the required drivers for QEMU riscv64 virt emulated machine. This line looks to be slightly too long now (after you apparently unwrapped what Arm has). Jan
On 12/16/24 3:23 PM, Jan Beulich wrote: > On 11.12.2024 18:27, Oleksii Kurochko wrote: >> --- a/xen/arch/riscv/include/asm/page.h >> +++ b/xen/arch/riscv/include/asm/page.h >> @@ -7,6 +7,7 @@ >> >> #include <xen/bug.h> >> #include <xen/const.h> >> +#include <xen/errno.h> >> #include <xen/types.h> >> >> #include <asm/atomic.h> >> @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) >> return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); >> } >> >> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) >> +{ >> +#ifdef CONFIG_QEMU >> + return 0; >> +#else >> + return -EOPNOTSUPP; >> +#endif >> +} >> + >> +static inline int clean_dcache_va_range(const void *p, unsigned long size) >> +{ >> +#ifdef CONFIG_QEMU >> + return 0; >> +#else >> + return -EOPNOTSUPP; >> +#endif >> +} > So testing on real hardware will then effectively become impossible, until > someone goes and implements these? Yes... I am not sure what better we can do. It seems like it will be the best one to check if CMO extensions is supported and use instructions for this extensions to implement these functions as they are in the specification and not expected to be changed. But I want to back a little bit later to this implemntation as this not issue for QEMU as it doesn't model cache and h/w on which I can ask to run Xen has IO cache coherency so for it will be needed just to add a new config and implementation will still be 'return 0'. ( I thought to introduce instead of CONFIG_QEMU something like CONFIG_HAS_CACHE_COHERENCY ) And also in the spec it is mentioned: ``` This suggests that RISC-V platforms prefer to support full cache-coherent I/O, but it isn't actually mandatory. As a result, the PMBT and CMO extensions aren't mandatory either, meaning that some platforms might not have instructions to properly flush, clean, or invalidate the cache. ``` Based on that I also think to implement that in the following way: ``` #ifdef CONFIG_QEMU static inline int plat_clean_and_invalidate_dcache_va_range() { return 0; } static inline int plat_clean_dcache_va_range() { return 0; } #else /* !CONFIG_QEMU */ static inline void plat_clean_and_invalidate_dcache_va_range() { printk_once("%s: should it be implemented for your platform?\n", __func__); return 0; } static inline void plat_clean_dcache_va_range() { printk_once("%s: should it be implemented for your platform?\n", __func__); return 0; } #endif static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) { return plat_clean_and_invalidate_dcache_va_range(); } .... ``` So we will have a notification for others none-QEMU platforms notification that probably some changes are required. > >> --- /dev/null >> +++ b/xen/arch/riscv/platforms/Kconfig >> @@ -0,0 +1,5 @@ >> +config QEMU >> + bool "QEMU aarch virt machine support" >> + depends on RISCV_64 > I understand Arm has it like this, but: Is QEMU really a sufficiently non- > ambiguous name to use? Yes, it sounds good to me to have such naming for the platform which are running on top of QEMU. The other option I thought about it is to use CONFIG_VIRT_PLATFORM. > Is the RISCV_64 dependency really needed? At the moment , we are testing only RISCV_64 so not to have none-verified config I added RISCV_64 but probably it is too much and `depends on` should be dropped at all. Thanks. ~ Oleksii > >> + help >> + Enable all the required drivers for QEMU riscv64 virt emulated machine. > This line looks to be slightly too long now (after you apparently unwrapped > what Arm has). > > Jan
On 16.12.2024 18:40, Oleksii Kurochko wrote: > On 12/16/24 3:23 PM, Jan Beulich wrote: >> On 11.12.2024 18:27, Oleksii Kurochko wrote: >>> --- a/xen/arch/riscv/include/asm/page.h >>> +++ b/xen/arch/riscv/include/asm/page.h >>> @@ -7,6 +7,7 @@ >>> >>> #include <xen/bug.h> >>> #include <xen/const.h> >>> +#include <xen/errno.h> >>> #include <xen/types.h> >>> >>> #include <asm/atomic.h> >>> @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) >>> return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); >>> } >>> >>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) >>> +{ >>> +#ifdef CONFIG_QEMU >>> + return 0; >>> +#else >>> + return -EOPNOTSUPP; >>> +#endif >>> +} >>> + >>> +static inline int clean_dcache_va_range(const void *p, unsigned long size) >>> +{ >>> +#ifdef CONFIG_QEMU >>> + return 0; >>> +#else >>> + return -EOPNOTSUPP; >>> +#endif >>> +} >> So testing on real hardware will then effectively become impossible, until >> someone goes and implements these? > > Yes... > > I am not sure what better we can do. It seems like it will be the best one to check if CMO > extensions is supported and use instructions for this extensions to implement these functions as they > are in the specification and not expected to be changed. Yes, using CMO when available is certainly the route to go. The main question there is what the behavior ought to be when CMO is unavailable. > But I want to back a little bit later to this implemntation as this not issue for QEMU as it doesn't model cache and > h/w on which I can ask to run Xen has IO cache coherency so for it will be needed just to add a new config > and implementation will still be 'return 0'. ( I thought to introduce instead of CONFIG_QEMU something like > CONFIG_HAS_CACHE_COHERENCY ) > > And also in the spec it is mentioned: > ``` > This suggests that RISC-V platforms prefer to support full > cache-coherent I/O, but it isn't actually mandatory. > As a result, the PMBT and CMO extensions aren't mandatory either, > meaning that some platforms might not > have instructions to properly flush, clean, or invalidate the cache. > ``` Based on that I also think to implement that in the following way: > ``` > #ifdef CONFIG_QEMU > static inline int plat_clean_and_invalidate_dcache_va_range() { return 0; } > static inline int plat_clean_dcache_va_range() { return 0; } > #else /* !CONFIG_QEMU */ > static inline void plat_clean_and_invalidate_dcache_va_range() > { > printk_once("%s: should it be implemented for your platform?\n", __func__); > return 0; > } > > static inline void plat_clean_dcache_va_range() > { > printk_once("%s: should it be implemented for your platform?\n", __func__); > return 0; > } > #endif > > static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) > { > return plat_clean_and_invalidate_dcache_va_range(); > } > .... > ``` > So we will have a notification for others none-QEMU platforms notification that probably some > changes are required. Yet failing to get cache management right can easily result in data corruption. I don't think a on-time printk() is appropriate to handle the lack of respective implementation. At least not anymore once RISC-V leaves "experimental" status. >>> --- /dev/null >>> +++ b/xen/arch/riscv/platforms/Kconfig >>> @@ -0,0 +1,5 @@ >>> +config QEMU >>> + bool "QEMU aarch virt machine support" >>> + depends on RISCV_64 >> I understand Arm has it like this, but: Is QEMU really a sufficiently non- >> ambiguous name to use? > > Yes, it sounds good to me to have such naming for the platform which are running on top of QEMU. > > The other option I thought about it is to use CONFIG_VIRT_PLATFORM. I don't think QEMU should be fully omitted from the name. Nor do I think that you can generally infer from "virtual platform" that caches aren't modeled. What I was rather getting at is to perhaps add some qualifier to QEMU, e.g. QEMU_PLATFORM. Jan
On 12/17/24 9:32 AM, Jan Beulich wrote: > On 16.12.2024 18:40, Oleksii Kurochko wrote: >> On 12/16/24 3:23 PM, Jan Beulich wrote: >>> On 11.12.2024 18:27, Oleksii Kurochko wrote: >>>> --- a/xen/arch/riscv/include/asm/page.h >>>> +++ b/xen/arch/riscv/include/asm/page.h >>>> @@ -7,6 +7,7 @@ >>>> >>>> #include <xen/bug.h> >>>> #include <xen/const.h> >>>> +#include <xen/errno.h> >>>> #include <xen/types.h> >>>> >>>> #include <asm/atomic.h> >>>> @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) >>>> return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); >>>> } >>>> >>>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) >>>> +{ >>>> +#ifdef CONFIG_QEMU >>>> + return 0; >>>> +#else >>>> + return -EOPNOTSUPP; >>>> +#endif >>>> +} >>>> + >>>> +static inline int clean_dcache_va_range(const void *p, unsigned long size) >>>> +{ >>>> +#ifdef CONFIG_QEMU >>>> + return 0; >>>> +#else >>>> + return -EOPNOTSUPP; >>>> +#endif >>>> +} >>> So testing on real hardware will then effectively become impossible, until >>> someone goes and implements these? >> Yes... >> >> I am not sure what better we can do. It seems like it will be the best one to check if CMO >> extensions is supported and use instructions for this extensions to implement these functions as they >> are in the specification and not expected to be changed. > Yes, using CMO when available is certainly the route to go. The main > question there is what the behavior ought to be when CMO is unavailable. If CMO ( or SoC specific extension for cache operation ) isn't available then IMO it means that memory is coherent and nothing specific should be done in the mentioned above functions what means returning 0 should be fine. Then implementation of these functions could look like: ``` static inline int <.....>(....) { #if !defined(CONFIG_QEMU) #warning should implementation of <....> be updated? #endif return 0; } ``` Or just to be sure that user see the message change #warning -> #error. ~ Oleksii > >> But I want to back a little bit later to this implemntation as this not issue for QEMU as it doesn't model cache and >> h/w on which I can ask to run Xen has IO cache coherency so for it will be needed just to add a new config >> and implementation will still be 'return 0'. ( I thought to introduce instead of CONFIG_QEMU something like >> CONFIG_HAS_CACHE_COHERENCY ) >> >> And also in the spec it is mentioned: >> ``` >> This suggests that RISC-V platforms prefer to support full >> cache-coherent I/O, but it isn't actually mandatory. >> As a result, the PMBT and CMO extensions aren't mandatory either, >> meaning that some platforms might not >> have instructions to properly flush, clean, or invalidate the cache. >> ``` Based on that I also think to implement that in the following way: >> ``` >> #ifdef CONFIG_QEMU >> static inline int plat_clean_and_invalidate_dcache_va_range() { return 0; } >> static inline int plat_clean_dcache_va_range() { return 0; } >> #else /* !CONFIG_QEMU */ >> static inline void plat_clean_and_invalidate_dcache_va_range() >> { >> printk_once("%s: should it be implemented for your platform?\n", __func__); >> return 0; >> } >> >> static inline void plat_clean_dcache_va_range() >> { >> printk_once("%s: should it be implemented for your platform?\n", __func__); >> return 0; >> } >> #endif >> >> static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) >> { >> return plat_clean_and_invalidate_dcache_va_range(); >> } >> .... >> ``` >> So we will have a notification for others none-QEMU platforms notification that probably some >> changes are required. > Yet failing to get cache management right can easily result in data corruption. > I don't think a on-time printk() is appropriate to handle the lack of respective > implementation. At least not anymore once RISC-V leaves "experimental" status. > >>>> --- /dev/null >>>> +++ b/xen/arch/riscv/platforms/Kconfig >>>> @@ -0,0 +1,5 @@ >>>> +config QEMU >>>> + bool "QEMU aarch virt machine support" >>>> + depends on RISCV_64 >>> I understand Arm has it like this, but: Is QEMU really a sufficiently non- >>> ambiguous name to use? >> Yes, it sounds good to me to have such naming for the platform which are running on top of QEMU. >> >> The other option I thought about it is to use CONFIG_VIRT_PLATFORM. > I don't think QEMU should be fully omitted from the name. Nor do I think that > you can generally infer from "virtual platform" that caches aren't modeled. > What I was rather getting at is to perhaps add some qualifier to QEMU, e.g. > QEMU_PLATFORM. > > Jan
On 17.12.2024 11:37, Oleksii Kurochko wrote: > > On 12/17/24 9:32 AM, Jan Beulich wrote: >> On 16.12.2024 18:40, Oleksii Kurochko wrote: >>> On 12/16/24 3:23 PM, Jan Beulich wrote: >>>> On 11.12.2024 18:27, Oleksii Kurochko wrote: >>>>> --- a/xen/arch/riscv/include/asm/page.h >>>>> +++ b/xen/arch/riscv/include/asm/page.h >>>>> @@ -7,6 +7,7 @@ >>>>> >>>>> #include <xen/bug.h> >>>>> #include <xen/const.h> >>>>> +#include <xen/errno.h> >>>>> #include <xen/types.h> >>>>> >>>>> #include <asm/atomic.h> >>>>> @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) >>>>> return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); >>>>> } >>>>> >>>>> +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) >>>>> +{ >>>>> +#ifdef CONFIG_QEMU >>>>> + return 0; >>>>> +#else >>>>> + return -EOPNOTSUPP; >>>>> +#endif >>>>> +} >>>>> + >>>>> +static inline int clean_dcache_va_range(const void *p, unsigned long size) >>>>> +{ >>>>> +#ifdef CONFIG_QEMU >>>>> + return 0; >>>>> +#else >>>>> + return -EOPNOTSUPP; >>>>> +#endif >>>>> +} >>>> So testing on real hardware will then effectively become impossible, until >>>> someone goes and implements these? >>> Yes... >>> >>> I am not sure what better we can do. It seems like it will be the best one to check if CMO >>> extensions is supported and use instructions for this extensions to implement these functions as they >>> are in the specification and not expected to be changed. >> Yes, using CMO when available is certainly the route to go. The main >> question there is what the behavior ought to be when CMO is unavailable. > > If CMO ( or SoC specific extension for cache operation ) isn't available then IMO it means that memory is > coherent and nothing specific should be done in the mentioned above functions what means returning 0 should > be fine. Hmm, imo that would be a pretty bold assumption. Jan
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig index 1858004676..00f329054c 100644 --- a/xen/arch/riscv/Kconfig +++ b/xen/arch/riscv/Kconfig @@ -52,6 +52,8 @@ config RISCV_ISA_C endmenu +source "arch/riscv/platforms/Kconfig" + source "common/Kconfig" source "drivers/Kconfig" diff --git a/xen/arch/riscv/configs/tiny64_defconfig b/xen/arch/riscv/configs/tiny64_defconfig index fc7a04872f..47076e357c 100644 --- a/xen/arch/riscv/configs/tiny64_defconfig +++ b/xen/arch/riscv/configs/tiny64_defconfig @@ -10,3 +10,4 @@ CONFIG_RISCV_64=y CONFIG_DEBUG=y CONFIG_DEBUG_INFO=y CONFIG_EXPERT=y +CONFIG_QEMU=y diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h index bf3f75e85d..54c6fe6515 100644 --- a/xen/arch/riscv/include/asm/page.h +++ b/xen/arch/riscv/include/asm/page.h @@ -7,6 +7,7 @@ #include <xen/bug.h> #include <xen/const.h> +#include <xen/errno.h> #include <xen/types.h> #include <asm/atomic.h> @@ -148,9 +149,27 @@ static inline bool pte_is_mapping(pte_t p) return (p.pte & PTE_VALID) && (p.pte & PTE_ACCESS_MASK); } +static inline int clean_and_invalidate_dcache_va_range(const void *p, unsigned long size) +{ +#ifdef CONFIG_QEMU + return 0; +#else + return -EOPNOTSUPP; +#endif +} + +static inline int clean_dcache_va_range(const void *p, unsigned long size) +{ +#ifdef CONFIG_QEMU + return 0; +#else + return -EOPNOTSUPP; +#endif +} + static inline void invalidate_icache(void) { - BUG_ON("unimplemented"); + asm volatile ( "fence.i" ::: "memory" ); } #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE) diff --git a/xen/arch/riscv/platforms/Kconfig b/xen/arch/riscv/platforms/Kconfig new file mode 100644 index 0000000000..30ed938d52 --- /dev/null +++ b/xen/arch/riscv/platforms/Kconfig @@ -0,0 +1,5 @@ +config QEMU + bool "QEMU aarch virt machine support" + depends on RISCV_64 + help + Enable all the required drivers for QEMU riscv64 virt emulated machine.
Implement following cache operations: - clean_and_invalidate_dcache_va_range() - clean_dcache_va_range() - invalidate_icache() The first two functions may require support for the CMO (Cache Management Operations) extension and/or hardware-specific instructions. Currently, only QEMU is supported, which does not model cache behavior. Therefore, clean_and_invalidate_dcache_va_range() and clean_dcache_va_range() are implemented to simply return 0. For other cases, -ENOTSUPP is returned. If hardware supports CMO or hardware-specific instructions, these functions should be updated accordingly. To support current implementation of these function CONFIG_QEMU is introduced. invalidate_icache() is implemented using fence.i instruction as mentioned in the unpriv spec: The FENCE.I instruction was designed to support a wide variety of implementations. A simple implementation can flush the local instruction cache and the instruction pipeline when the FENCE.I is executed. A more complex implementation might snoop the instruction (data) cache on every data (instruction) cache miss, or use an inclusive unified private L2 cache to invalidate lines from the primary instruction cache when they are being written by a local store instruction. If instruction and data caches are kept coherent in this way, or if the memory system consists of only uncached RAMs, then just the fetch pipeline needs to be flushed at a FENCE.I. The FENCE.I instruction requires the presence of the Zifencei extension, which might not always be available. However, Xen uses the RV64G ISA, which guarantees the presence of the Zifencei extension. According to the unprivileged ISA specification (version 20240411): One goal of the RISC-V project is that it be used as a stable software development target. For this purpose, we define a combination of a base ISA (RV32I or RV64I) plus selected standard extensions (IMAFD, Zicsr, Zifencei) as a "general-purpose" ISA, and we use the abbreviation G for the IMAFDZicsr_Zifencei combination of instruction-set extensions. Set CONFIG_QEMU=y in tiny64_defconfig to have proper implemtation of clean_and_invalidate_dcache_va_range() and clean_dcache_va_range() for CI. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V2: - Update the commit message and subject: - drop information about HAS_CMO; - add information about Zifencei extension; - Introdce platforms directory and CONFIG_QEMU; update implementation of data/instruction cache operations as returning 0 for CONFIG_QEMU and for others - return -ENOTSUPP. - Drop HAS_CMO config. --- xen/arch/riscv/Kconfig | 2 ++ xen/arch/riscv/configs/tiny64_defconfig | 1 + xen/arch/riscv/include/asm/page.h | 21 ++++++++++++++++++++- xen/arch/riscv/platforms/Kconfig | 5 +++++ 4 files changed, 28 insertions(+), 1 deletion(-) create mode 100644 xen/arch/riscv/platforms/Kconfig