On 02.02.2024 16:16, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
>
> Effects caused by debug/logging macros and functions (like ASSERT,
> __bad_atomic_size,
> LOG, etc ...) that crash execution or produce logs are not dangerous in
> initializer
> lists. The evaluation order in abnormal conditions is not relevant.
> Evaluation order
> of logging effects is always safe.
I thought I said so before: When talking of just logging, evaluation order
may very well have a impact on correctness. Therefore we shouldn't mix
debugging and logging.
> Function hvm_get_guest_tsc_fixed (indirectly) performs different side effects.
> For example it calls hvm_get_guest_time_fixed that contains an ASSERT and
> calls
> to spin_lock and spin_unlock.
>
> These side effects are not dangerous: they can be executed regardless of the
> initializer list evaluation order
>
> This patch deviates violations using SAF commits caused by debug/logging
> macros and
> functions.
DYM "comments"?
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -331,6 +331,7 @@ int handle_device(struct domain *d, struct dt_device_node
> *dev, p2m_type_t p2mt,
> .p2mt = p2mt,
> .skip_mapping = !own_device ||
> (is_pci_passthrough_enabled() &&
> + /* SAF-3-safe effects for debugging/logging reasons
> are safe */
> (device_get_class(dev) == DEVICE_PCI_HOSTBRIDGE)),
Taking this just as example: I think the comment is too long. Just
saying (leaving aside my comment higher up) "debugging/logging"
would imo be sufficient.
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -110,26 +110,34 @@ static unsigned long copy_guest(void *buf, uint64_t
> addr, unsigned int len,
> unsigned long raw_copy_to_guest(void *to, const void *from, unsigned int len)
> {
> return copy_guest((void *)from, (vaddr_t)to, len,
> - GVA_INFO(current), COPY_to_guest | COPY_linear);
> + /* SAF-4-safe No persistent side effects */
> + GVA_INFO(current),
I _still_ think this leaves ambiguity. The more that you need to look
up GVA_INFO() to recognize what this is about.
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -800,6 +800,7 @@ static int cf_check hvm_save_cpu_ctxt(struct vcpu *v,
> hvm_domain_context_t *h)
> {
> struct segment_register seg;
> struct hvm_hw_cpu ctxt = {
> + /* SAF-3-safe effects for debugging/logging reasons are safe */
> .tsc = hvm_get_guest_tsc_fixed(v, v->domain->arch.hvm.sync_tsc),
A prereq for this imo is that the function take const struct vcpu *.
But I'm not sure that'll suffice. The function can change at any time,
rendering the comment here stale perhaps without anyone noticing.
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -1521,6 +1521,7 @@ long vcpu_yield(void)
>
> SCHED_STAT_CRANK(vcpu_yield);
>
> + /* SAF-4-safe No persistent side effects */
> TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id);
> raise_softirq(SCHEDULE_SOFTIRQ);
> return 0;
> @@ -1899,6 +1900,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> if ( copy_from_guest(&sched_shutdown, arg, 1) )
> break;
>
> + /* SAF-4-safe No persistent side effects */
> TRACE_3D(TRC_SCHED_SHUTDOWN,
> current->domain->domain_id, current->vcpu_id,
> sched_shutdown.reason);
> @@ -1916,6 +1918,7 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void)
> arg)
> if ( copy_from_guest(&sched_shutdown, arg, 1) )
> break;
>
> + /* SAF-4-safe No persistent side effects */
> TRACE_3D(TRC_SCHED_SHUTDOWN_CODE,
> d->domain_id, current->vcpu_id, sched_shutdown.reason);
>
For all of these iirc the suggestion was to latch current into a local
variable (named "curr" by convention).
Jan
