[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN PATCH] xen: cache clearing and invalidation helpers refactoring
Hi Stefano, On 2024-02-24 00:05, Stefano Stabellini wrote: On Fri, 23 Feb 2024, Nicola Vetrini wrote:On 2024-02-19 16:14, Nicola Vetrini wrote: > The cache clearing and invalidation helpers in x86 and Arm didn't > comply with MISRA C Rule 17.7: "The value returned by a function > having non-void return type shall be used". On Arm they > were always returning 0, while some in x86 returned -EOPNOTSUPP > and in common/grant_table the return value is saved. > > As a consequence, a common helper arch_grant_cache_flush that returns > an integer is introduced, so that each architecture can choose whether to > return an error value on certain conditions, and the helpers have either > been changed to return void (on Arm) or deleted entirely (on x86). > > Signed-off-by: Julien Grall <julien@xxxxxxx> > Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx> > --- > The original refactor idea came from Julien Grall in [1]; I edited that > proposal > to fix build errors. > > I did introduce a cast to void for the call to flush_area_local on x86, > because > even before this patch the return value of that function wasn't checked in > all > but one use in x86/smp.c, and in this context the helper (perhaps > incidentally) > ignored the return value of flush_area_local. > > [1] > https://lore.kernel.org/xen-devel/09589e8f-77b6-47f7-b5bd-cf485e4b60f1@xxxxxxx/ > --- > xen/arch/arm/include/asm/page.h | 33 ++++++++++++++++++----------- > xen/arch/x86/include/asm/flushtlb.h | 23 ++++++++++---------- > xen/common/grant_table.c | 9 +------- > 3 files changed, 34 insertions(+), 31 deletions(-) >I'll put this patch in the backlog at the moment: too many intricacies while trying to untangle xen/flushtlb from xen/mm.h, and there are easier cases that can be done faster. If someone is interested I can post the partial work I'vedone so far, even though it doesn't build on x86.I understand that the blocker is:diff --git a/xen/arch/arm/include/asm/page.h b/xen/arch/arm/include/asm/page.hindex 69f817d1e6..e90c9de361 100644 --- a/xen/arch/arm/include/asm/page.h +++ b/xen/arch/arm/include/asm/page.h @@ -123,6 +123,7 @@ #ifndef __ASSEMBLY__ +#include <public/grant_table.h> #include <xen/errno.h> #include <xen/types.h> #include <xen/lib.h> And the headers disentagling required to solve it, right? Let me ask a silly question. public/grant_table.h seems needed by arch_grant_cache_flush. Can we move arch_grant_cache_flush somewhere else? It is not like page.h is a perfect fit for it anyway. For instance, can we move it to xen/arch/arm/include/asm/grant_table.h ? Yes, this is what was suggested and what I was trying to accomplish. Basically my plan is:1. move the arch_grant_cache_flush helper to asm/grant_table.h for both architectures 2. pull out of xen/mm.h this hunk (note the inclusion of asm/flushtlb in the middle of the file) because there is a build error on tlbflush_current_time() induced in some .c file (don't remember which) by the earlier movement -#include <asm/flushtlb.h> - -static inline void accumulate_tlbflush(bool *need_tlbflush, - const struct page_info *page, - uint32_t *tlbflush_timestamp) -{ - if ( page->u.free.need_tlbflush && - page->tlbflush_timestamp <= tlbflush_current_time() && - (!*need_tlbflush || - page->tlbflush_timestamp > *tlbflush_timestamp) ) - { - *need_tlbflush = true; - *tlbflush_timestamp = page->tlbflush_timestamp; - } -} - -static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) -{ - cpumask_t mask; - - cpumask_copy(&mask, &cpu_online_map); - tlbflush_filter(&mask, tlbflush_timestamp); - if ( !cpumask_empty(&mask) ) - { - perfc_incr(need_flush_tlb_flush); - arch_flush_tlb_mask(&mask); - } -} - which is going to be in a new header xen/flushtlb.h3. replace various inclusions the previously relied on the fact that xen/mm.h included asm/flushtlb.h (some even stating this as evidenced from the hunk below) diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c index 75fc84e445ce..91ee7e6ec39e 100644 --- a/xen/arch/x86/cpu/microcode/amd.c +++ b/xen/arch/x86/cpu/microcode/amd.c @@ -15,8 +15,8 @@ */ #include <xen/err.h> +#include <xen/flushtlb.h> #include <xen/init.h> -#include <xen/mm.h> /* TODO: Fix asm/tlbflush.h breakage */ and then make everything build.However, the dependencies tied to xen/mm.h are quite numerous on x86, and I'm not seeing an obvious way to avoid touching xen/mm.h. See this tree [1] for the latest state of the patch. If anyone has an idea how to tackle this in a smarter way, I'm open to suggestions. Specifically in step (2) there might be a way to avoid doing that modification, perhaps. [1] https://gitlab.com/xen-project/people/bugseng/xen/-/commits/cache_helpers -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |