| Message ID | 6476706490cc15406bcf3377a57b7c4a303c4901.1696833629.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix or deviate various instances of missing declarations | expand |
On 09.10.2023 08:54, Nicola Vetrini wrote: > As stated in rules.rst, functions used only in asm code > are allowed to have no prior declaration visible when being > defined, hence these functions are deviated. > This also fixes violations of MISRA C:2012 Rule 8.4. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/x86/hvm/svm/intr.c | 1 + > xen/arch/x86/hvm/svm/nestedsvm.c | 1 + > xen/arch/x86/hvm/svm/svm.c | 2 ++ Once again - why are you not also adjusting the respective VMX code? Iirc it was agreed long ago that scans should be extended to cover as much of the code base as possible. Jan
On Mon, 16 Oct 2023, Jan Beulich wrote: > On 09.10.2023 08:54, Nicola Vetrini wrote: > > As stated in rules.rst, functions used only in asm code > > are allowed to have no prior declaration visible when being > > defined, hence these functions are deviated. > > This also fixes violations of MISRA C:2012 Rule 8.4. > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > --- > > xen/arch/x86/hvm/svm/intr.c | 1 + > > xen/arch/x86/hvm/svm/nestedsvm.c | 1 + > > xen/arch/x86/hvm/svm/svm.c | 2 ++ > > Once again - why are you not also adjusting the respective VMX code? > Iirc it was agreed long ago that scans should be extended to cover as > much of the code base as possible. Let me summarize here our past discussions on the subject to make sure we are all aligned. With my AMD hat on, of course we want to work with the upstream community as much as possible and improve the overall codebase. But it is not a goal for AMD to improve Intel-specific drivers (VMX and others). Our safety configuration for Xen, including the public ECLAIR instance currently sponsored by AMD, only includes SVM files, not VMX files. MISRA compliance costs time and effort; this was done both because of lack of interest in VMX and also as a cost saving measure. Upon maintainer's request we can expand the scope of individual patches. For example, AMD is not interested in ARM32 either, but in the past we did address certain MISRA C issues on ARM32 too, if nothing else for consistency of the code base. It comes down to a compromise what we should do for consistency of the codebase versus addressing things that makes sense for AMD business. For sure we could work on a few violations affecting Intel drivers, but overall I don't think AMD could be asked to make Intel drivers MISRA compliant. In addition to the above, we also discussed during one of the past MISRA C working group meetings to have larger-than-usual ECLAIR scans. ECLAIR takes a couple of hours to run, so it is a good idea to restrict its configuration in the usual runs. However, at least once a week maybe on a Sunday, it would be good to run a comprehensive scan including components that are not currently targeted for safety. This would help us detect regressions and in general spot potential bugs. As part of this larger-than-usual ECLAIR scan we could certainly include Intel drivers as well as other things currently unsupported. Now, concrete action items. Nicola, I think we should look into having a larger-than-usual ECLAIR scan every week which includes all of Intel files and in general as much as possible of the codebase. Jan, for this specific patch, I don't think we have the scan including Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola wouldn't be able to easily expand this patch to also cover Intel vmx violations of this rule because we don't have the list of violations affecting those files.
On 17.10.2023 00:34, Stefano Stabellini wrote: > On Mon, 16 Oct 2023, Jan Beulich wrote: >> On 09.10.2023 08:54, Nicola Vetrini wrote: >>> As stated in rules.rst, functions used only in asm code >>> are allowed to have no prior declaration visible when being >>> defined, hence these functions are deviated. >>> This also fixes violations of MISRA C:2012 Rule 8.4. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> >>> --- >>> xen/arch/x86/hvm/svm/intr.c | 1 + >>> xen/arch/x86/hvm/svm/nestedsvm.c | 1 + >>> xen/arch/x86/hvm/svm/svm.c | 2 ++ >> >> Once again - why are you not also adjusting the respective VMX code? >> Iirc it was agreed long ago that scans should be extended to cover as >> much of the code base as possible. > > > Let me summarize here our past discussions on the subject to make sure > we are all aligned. > > With my AMD hat on, of course we want to work with the upstream > community as much as possible and improve the overall codebase. But it > is not a goal for AMD to improve Intel-specific drivers (VMX and > others). Our safety configuration for Xen, including the public ECLAIR > instance currently sponsored by AMD, only includes SVM files, not VMX > files. MISRA compliance costs time and effort; this was done both > because of lack of interest in VMX and also as a cost saving measure. > > Upon maintainer's request we can expand the scope of individual patches. > For example, AMD is not interested in ARM32 either, but in the past we > did address certain MISRA C issues on ARM32 too, if nothing else for > consistency of the code base. It comes down to a compromise what we > should do for consistency of the codebase versus addressing things that > makes sense for AMD business. For sure we could work on a few violations > affecting Intel drivers, but overall I don't think AMD could be asked to > make Intel drivers MISRA compliant. > > > In addition to the above, we also discussed during one of the past MISRA > C working group meetings to have larger-than-usual ECLAIR scans. ECLAIR > takes a couple of hours to run, so it is a good idea to restrict its > configuration in the usual runs. However, at least once a week maybe on > a Sunday, it would be good to run a comprehensive scan including > components that are not currently targeted for safety. This would help > us detect regressions and in general spot potential bugs. > > As part of this larger-than-usual ECLAIR scan we could certainly > include Intel drivers as well as other things currently unsupported. > > > Now, concrete action items. Nicola, I think we should look into having a > larger-than-usual ECLAIR scan every week which includes all of Intel > files and in general as much as possible of the codebase. > > Jan, for this specific patch, I don't think we have the scan including > Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola > wouldn't be able to easily expand this patch to also cover Intel vmx > violations of this rule because we don't have the list of violations > affecting those files. I'm afraid I disagree: There are likely direct VMX counterparts to the SVM items adjusted. No scan is needed to spot those. Anything VMX-only can be left separate, sure. Jan
> Now, concrete action items. Nicola, I think we should look into having > a > larger-than-usual ECLAIR scan every week which includes all of Intel > files and in general as much as possible of the codebase. > This can be arranged. I'll keep you posted about this.
On Tue, 17 Oct 2023, Jan Beulich wrote: > > Jan, for this specific patch, I don't think we have the scan including > > Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola > > wouldn't be able to easily expand this patch to also cover Intel vmx > > violations of this rule because we don't have the list of violations > > affecting those files. > > I'm afraid I disagree: There are likely direct VMX counterparts to the SVM > items adjusted. No scan is needed to spot those. Anything VMX-only can be > left separate, sure. Nicola is new to the codebase, so let us help. This patch adds deviations for these SVM functions: - svm_intr_assist - nsvm_vcpu_switch - svm_vmenter_helper - svm_vmexit_handler I take these are the VMX equivalents: - vmx_intr_assist - nvmx_switch_guest - vmx_vmenter_helper - vmx_asm_vmexit_handler Jan, did I miss anything?
On 19.10.2023 02:07, Stefano Stabellini wrote: > On Tue, 17 Oct 2023, Jan Beulich wrote: >>> Jan, for this specific patch, I don't think we have the scan including >>> Intel vmx files yet. Nicola please correct me if I am wrong. So Nicola >>> wouldn't be able to easily expand this patch to also cover Intel vmx >>> violations of this rule because we don't have the list of violations >>> affecting those files. >> >> I'm afraid I disagree: There are likely direct VMX counterparts to the SVM >> items adjusted. No scan is needed to spot those. Anything VMX-only can be >> left separate, sure. > > Nicola is new to the codebase, so let us help. > > This patch adds deviations for these SVM functions: > - svm_intr_assist > - nsvm_vcpu_switch > - svm_vmenter_helper > - svm_vmexit_handler > > I take these are the VMX equivalents: > - vmx_intr_assist > - nvmx_switch_guest > - vmx_vmenter_helper > - vmx_asm_vmexit_handler > > Jan, did I miss anything? At the first glance - no, I don't think you do. Jan
>> - vmx_asm_vmexit_handler >> Isn't this just a declaration: void nocall vmx_asm_vmexit_handler(void); while the function to be deviated is this: void vmx_vmexit_handler(struct cpu_user_regs *regs) Am I correct?
On 19.10.2023 10:04, Nicola Vetrini wrote: > >>> - vmx_asm_vmexit_handler >>> > > Isn't this just a declaration: > > void nocall vmx_asm_vmexit_handler(void); > > while the function to be deviated is this: > > void vmx_vmexit_handler(struct cpu_user_regs *regs) > > Am I correct? Looks like you are, yes. Jan
diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c index 192e17ebbfbb..bd9dc560bbc6 100644 --- a/xen/arch/x86/hvm/svm/intr.c +++ b/xen/arch/x86/hvm/svm/intr.c @@ -123,6 +123,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack) vmcb, general1_intercepts | GENERAL1_INTERCEPT_VINTR); } +/* SAF-1-safe */ void svm_intr_assist(void) { struct vcpu *v = current; diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c index a09b6abaaeaf..c80d59e0728e 100644 --- a/xen/arch/x86/hvm/svm/nestedsvm.c +++ b/xen/arch/x86/hvm/svm/nestedsvm.c @@ -1441,6 +1441,7 @@ nestedsvm_vcpu_vmexit(struct vcpu *v, struct cpu_user_regs *regs, } /* VCPU switch */ +/* SAF-1-safe */ void nsvm_vcpu_switch(void) { struct cpu_user_regs *regs = guest_cpu_user_regs(); diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index beb076ea8d62..b9fabd45a119 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -1044,6 +1044,7 @@ static void noreturn cf_check svm_do_resume(void) reset_stack_and_jump(svm_asm_do_resume); } +/* SAF-1-safe */ void svm_vmenter_helper(void) { const struct cpu_user_regs *regs = guest_cpu_user_regs(); @@ -2574,6 +2575,7 @@ const struct hvm_function_table * __init start_svm(void) return &svm_function_table; } +/* SAF-1-safe */ void svm_vmexit_handler(void) { struct cpu_user_regs *regs = guest_cpu_user_regs(); diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 0a005f088bca..f27ddb728b2c 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -2260,6 +2260,7 @@ void asm_domain_crash_synchronous(unsigned long addr) } #ifdef CONFIG_DEBUG +/* SAF-1-safe */ void check_ist_exit(const struct cpu_user_regs *regs, bool ist_exit) { const unsigned int ist_mask = diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c index e03e80813e36..5963d26d7848 100644 --- a/xen/arch/x86/x86_64/traps.c +++ b/xen/arch/x86/x86_64/traps.c @@ -266,6 +266,7 @@ void show_page_walk(unsigned long addr) l1_table_offset(addr), l1e_get_intpte(l1e), pfn); } +/* SAF-1-safe */ void do_double_fault(struct cpu_user_regs *regs) { unsigned int cpu;