| Message ID | alpine.DEB.2.22.394.2502141811180.3858257@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/x86: resolve the last 3 MISRA R16.6 violations | expand |
On 15.02.2025 03:16, Stefano Stabellini wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) > { > ASSERT(v == current || !vcpu_runnable(v)); > > - switch ( reg ) > - { > - default: > - return alternative_call(hvm_funcs.get_reg, v, reg); > - } > + return alternative_call(hvm_funcs.get_reg, v, reg); > } > > void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) > { > ASSERT(v == current || !vcpu_runnable(v)); > > - switch ( reg ) > - { > - default: > - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > - } > + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > } Both of these were, iirc, deliberately written using switch(), to ease possible future changes. Jan
On Mon, 17 Feb 2025, Jan Beulich wrote: > On 15.02.2025 03:16, Stefano Stabellini wrote: > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) > > { > > ASSERT(v == current || !vcpu_runnable(v)); > > > > - switch ( reg ) > > - { > > - default: > > - return alternative_call(hvm_funcs.get_reg, v, reg); > > - } > > + return alternative_call(hvm_funcs.get_reg, v, reg); > > } > > > > void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) > > { > > ASSERT(v == current || !vcpu_runnable(v)); > > > > - switch ( reg ) > > - { > > - default: > > - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > > - } > > + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > > } > > Both of these were, iirc, deliberately written using switch(), to ease > possible future changes. To be honest, I do not see any value in the way they are currently written. However, if you prefer, I can add a deviation for this, with one SAF comment for each of these two. The reason for the deviation would be "deliberate to ease possible future change". Please let me know how you would like to proceed.
On 18.02.2025 00:12, Stefano Stabellini wrote: > On Mon, 17 Feb 2025, Jan Beulich wrote: >> On 15.02.2025 03:16, Stefano Stabellini wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) >>> { >>> ASSERT(v == current || !vcpu_runnable(v)); >>> >>> - switch ( reg ) >>> - { >>> - default: >>> - return alternative_call(hvm_funcs.get_reg, v, reg); >>> - } >>> + return alternative_call(hvm_funcs.get_reg, v, reg); >>> } >>> >>> void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) >>> { >>> ASSERT(v == current || !vcpu_runnable(v)); >>> >>> - switch ( reg ) >>> - { >>> - default: >>> - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >>> - } >>> + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >>> } >> >> Both of these were, iirc, deliberately written using switch(), to ease >> possible future changes. > > To be honest, I do not see any value in the way they are currently > written. However, if you prefer, I can add a deviation for this, with > one SAF comment for each of these two. The reason for the deviation > would be "deliberate to ease possible future change". Please let me know > how you would like to proceed. Well, best next thing you can do is seek input from the person who has written that code, i.e. Andrew. Jan
On Tue, 18 Feb 2025, Jan Beulich wrote: > On 18.02.2025 00:12, Stefano Stabellini wrote: > > On Mon, 17 Feb 2025, Jan Beulich wrote: > >> On 15.02.2025 03:16, Stefano Stabellini wrote: > >>> --- a/xen/arch/x86/hvm/hvm.c > >>> +++ b/xen/arch/x86/hvm/hvm.c > >>> @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) > >>> { > >>> ASSERT(v == current || !vcpu_runnable(v)); > >>> > >>> - switch ( reg ) > >>> - { > >>> - default: > >>> - return alternative_call(hvm_funcs.get_reg, v, reg); > >>> - } > >>> + return alternative_call(hvm_funcs.get_reg, v, reg); > >>> } > >>> > >>> void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) > >>> { > >>> ASSERT(v == current || !vcpu_runnable(v)); > >>> > >>> - switch ( reg ) > >>> - { > >>> - default: > >>> - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > >>> - } > >>> + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); > >>> } > >> > >> Both of these were, iirc, deliberately written using switch(), to ease > >> possible future changes. > > > > To be honest, I do not see any value in the way they are currently > > written. However, if you prefer, I can add a deviation for this, with > > one SAF comment for each of these two. The reason for the deviation > > would be "deliberate to ease possible future change". Please let me know > > how you would like to proceed. > > Well, best next thing you can do is seek input from the person who has > written that code, i.e. Andrew. Andrew wrote in chat that he is OK with a deviation and he can live with a SAF deviation. Here is the patch. --- xen/x86: resolve the last 3 MISRA R16.6 violations MISRA R16.6 states that "Every switch statement shall have at least two switch-clauses". There are only 3 violations left on x86 (zero on ARM). One of them is only a violation depending on the kconfig configuration. So deviate it instead with a SAF comment. Two of them are deliberate to enable future additions. Deviate them as such. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> diff --git a/docs/misra/safe.json b/docs/misra/safe.json index b8a4f878ea..3d68b59169 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -92,6 +92,22 @@ }, { "id": "SAF-11-safe", + "analyser": { + "eclair": "MC3A2.R16.6" + }, + "name": "Rule 16.6: single clause due to kconfig", + "text": "A switch statement with a single switch clause because other switch clauses are disabled in a given kconfig is safe." + }, + { + "id": "SAF-12-safe", + "analyser": { + "eclair": "MC3A2.R16.6" + }, + "name": "Rule 16.6: single clause due to future expansion", + "text": "A switch statement with a single switch clause to purposely enable future additions of new cases is safe." + }, + { + "id": "SAF-13-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 39e39ce4ce..0f0630769b 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3797,6 +3797,7 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) { ASSERT(v == current || !vcpu_runnable(v)); + /* SAF-12-safe */ switch ( reg ) { default: @@ -3808,6 +3809,7 @@ void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) { ASSERT(v == current || !vcpu_runnable(v)); + /* SAF-12-safe */ switch ( reg ) { default: diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 87b30ce4df..dca11a613d 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -436,6 +436,7 @@ unsigned long get_stack_trace_bottom(unsigned long sp) static unsigned long get_shstk_bottom(unsigned long sp) { + /* SAF-11-safe */ switch ( get_stack_page(sp) ) { #ifdef CONFIG_XEN_SHSTK
On 2025-02-18 22:42, Stefano Stabellini wrote: > On Tue, 18 Feb 2025, Jan Beulich wrote: >> On 18.02.2025 00:12, Stefano Stabellini wrote: >> > On Mon, 17 Feb 2025, Jan Beulich wrote: >> >> On 15.02.2025 03:16, Stefano Stabellini wrote: >> >>> --- a/xen/arch/x86/hvm/hvm.c >> >>> +++ b/xen/arch/x86/hvm/hvm.c >> >>> @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) >> >>> { >> >>> ASSERT(v == current || !vcpu_runnable(v)); >> >>> >> >>> - switch ( reg ) >> >>> - { >> >>> - default: >> >>> - return alternative_call(hvm_funcs.get_reg, v, reg); >> >>> - } >> >>> + return alternative_call(hvm_funcs.get_reg, v, reg); >> >>> } >> >>> >> >>> void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) >> >>> { >> >>> ASSERT(v == current || !vcpu_runnable(v)); >> >>> >> >>> - switch ( reg ) >> >>> - { >> >>> - default: >> >>> - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >> >>> - } >> >>> + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >> >>> } >> >> >> >> Both of these were, iirc, deliberately written using switch(), to ease >> >> possible future changes. >> > >> > To be honest, I do not see any value in the way they are currently >> > written. However, if you prefer, I can add a deviation for this, with >> > one SAF comment for each of these two. The reason for the deviation >> > would be "deliberate to ease possible future change". Please let me know >> > how you would like to proceed. >> >> Well, best next thing you can do is seek input from the person who has >> written that code, i.e. Andrew. > > Andrew wrote in chat that he is OK with a deviation and he can live > with > a SAF deviation. Here is the patch. > > > --- > xen/x86: resolve the last 3 MISRA R16.6 violations > > MISRA R16.6 states that "Every switch statement shall have at least two > switch-clauses". There are only 3 violations left on x86 (zero on ARM). > > One of them is only a violation depending on the kconfig configuration. > So deviate it instead with a SAF comment. > > Two of them are deliberate to enable future additions. Deviate them as > such. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > Looks good to me, from an ECLAIR point of view. Did you have a chance to run a pipeline on it to confirm that the SAF comments are recognized correctly? With that, Reviewed-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > diff --git a/docs/misra/safe.json b/docs/misra/safe.json > index b8a4f878ea..3d68b59169 100644 > --- a/docs/misra/safe.json > +++ b/docs/misra/safe.json > @@ -92,6 +92,22 @@ > }, > { > "id": "SAF-11-safe", > + "analyser": { > + "eclair": "MC3A2.R16.6" > + }, > + "name": "Rule 16.6: single clause due to kconfig", > + "text": "A switch statement with a single switch clause > because other switch clauses are disabled in a given kconfig is safe." > + }, > + { > + "id": "SAF-12-safe", > + "analyser": { > + "eclair": "MC3A2.R16.6" > + }, > + "name": "Rule 16.6: single clause due to future > expansion", > + "text": "A switch statement with a single switch clause to > purposely enable future additions of new cases is safe." > + }, > + { > + "id": "SAF-13-safe", > "analyser": {}, > "name": "Sentinel", > "text": "Next ID to be used" > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 39e39ce4ce..0f0630769b 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -3797,6 +3797,7 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int > reg) > { > ASSERT(v == current || !vcpu_runnable(v)); > > + /* SAF-12-safe */ > switch ( reg ) > { > default: > @@ -3808,6 +3809,7 @@ void hvm_set_reg(struct vcpu *v, unsigned int > reg, uint64_t val) > { > ASSERT(v == current || !vcpu_runnable(v)); > > + /* SAF-12-safe */ > switch ( reg ) > { > default: > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c > index 87b30ce4df..dca11a613d 100644 > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -436,6 +436,7 @@ unsigned long get_stack_trace_bottom(unsigned long > sp) > > static unsigned long get_shstk_bottom(unsigned long sp) > { > + /* SAF-11-safe */ > switch ( get_stack_page(sp) ) > { > #ifdef CONFIG_XEN_SHSTK
On 18.02.2025 22:42, Stefano Stabellini wrote: > On Tue, 18 Feb 2025, Jan Beulich wrote: >> On 18.02.2025 00:12, Stefano Stabellini wrote: >>> On Mon, 17 Feb 2025, Jan Beulich wrote: >>>> On 15.02.2025 03:16, Stefano Stabellini wrote: >>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>> @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) >>>>> { >>>>> ASSERT(v == current || !vcpu_runnable(v)); >>>>> >>>>> - switch ( reg ) >>>>> - { >>>>> - default: >>>>> - return alternative_call(hvm_funcs.get_reg, v, reg); >>>>> - } >>>>> + return alternative_call(hvm_funcs.get_reg, v, reg); >>>>> } >>>>> >>>>> void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) >>>>> { >>>>> ASSERT(v == current || !vcpu_runnable(v)); >>>>> >>>>> - switch ( reg ) >>>>> - { >>>>> - default: >>>>> - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >>>>> - } >>>>> + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); >>>>> } >>>> >>>> Both of these were, iirc, deliberately written using switch(), to ease >>>> possible future changes. >>> >>> To be honest, I do not see any value in the way they are currently >>> written. However, if you prefer, I can add a deviation for this, with >>> one SAF comment for each of these two. The reason for the deviation >>> would be "deliberate to ease possible future change". Please let me know >>> how you would like to proceed. >> >> Well, best next thing you can do is seek input from the person who has >> written that code, i.e. Andrew. > > Andrew wrote in chat that he is OK with a deviation and he can live with > a SAF deviation. Here is the patch. > > > --- > xen/x86: resolve the last 3 MISRA R16.6 violations > > MISRA R16.6 states that "Every switch statement shall have at least two > switch-clauses". There are only 3 violations left on x86 (zero on ARM). > > One of them is only a violation depending on the kconfig configuration. > So deviate it instead with a SAF comment. > > Two of them are deliberate to enable future additions. Deviate them as > such. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> Acked-by: Jan Beulich <jbeulich@suse.com>
diff --git a/docs/misra/safe.json b/docs/misra/safe.json index b8a4f878ea..e1f950f7b1 100644 --- a/docs/misra/safe.json +++ b/docs/misra/safe.json @@ -92,6 +92,14 @@ }, { "id": "SAF-11-safe", + "analyser": { + "eclair": "MC3A2.R16.6" + }, + "name": "Rule 16.6: single clause due to kconfig", + "text": "A switch statement with a single switch clause because other switch clauses are disabled in a given kconfig is allowed." + }, + { + "id": "SAF-12-safe", "analyser": {}, "name": "Sentinel", "text": "Next ID to be used" diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 39e39ce4ce..c10c6bd833 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3797,22 +3797,14 @@ uint64_t hvm_get_reg(struct vcpu *v, unsigned int reg) { ASSERT(v == current || !vcpu_runnable(v)); - switch ( reg ) - { - default: - return alternative_call(hvm_funcs.get_reg, v, reg); - } + return alternative_call(hvm_funcs.get_reg, v, reg); } void hvm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val) { ASSERT(v == current || !vcpu_runnable(v)); - switch ( reg ) - { - default: - return alternative_vcall(hvm_funcs.set_reg, v, reg, val); - } + return alternative_vcall(hvm_funcs.set_reg, v, reg, val); } static bool cf_check is_sysdesc_access( diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 87b30ce4df..dca11a613d 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -436,6 +436,7 @@ unsigned long get_stack_trace_bottom(unsigned long sp) static unsigned long get_shstk_bottom(unsigned long sp) { + /* SAF-11-safe */ switch ( get_stack_page(sp) ) { #ifdef CONFIG_XEN_SHSTK
MISRA R16.6 states that "Every switch statement shall have at least two switch-clauses". There are only 3 violations left on x86 (zero on ARM). Two of them can be simply fixed. One of them is only a violation depending on the kconfig configuration. So deviate it instead with a SAF comment. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>