linux-kernel - Re: [PATCH v2 1/4] trace, cxl: Introduce a TRACE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20221012174620.7cd7d062@rorschach.local.home>
Date:   Wed, 12 Oct 2022 17:46:20 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     alison.schofield@...el.com
Cc:     Dan Williams <dan.j.williams@...el.com>,
        Ira Weiny <ira.weiny@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Ben Widawsky <bwidawsk@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, linux-cxl@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/4] trace, cxl: Introduce a TRACE_EVENT for CXL
 poison records

On Wed, 12 Oct 2022 14:28:17 -0700
alison.schofield@...el.com wrote:

> +TRACE_EVENT(cxl_poison,
> +
> +	    TP_PROTO(pid_t pid, const char *region, const char *memdev,
> +		     const char *pcidev, u64 hpa, u64 dpa, u32 length,
> +		     u8 source, u8 flags, u64 overflow_t),
> +
> +	    TP_ARGS(pid, region, memdev, pcidev, hpa, dpa,
> +		    length, source, flags, overflow_t),
> +
> +	    TP_STRUCT__entry(
> +		__field(pid_t, pid)
> +		__string(region, region ? region : "")
> +		__string(memdev, memdev)
> +		__string(pcidev, pcidev)
> +		__field(u64, hpa)
> +		__field(u64, dpa)
> +		__field(u32, length)
> +		__field(u8, source)
> +		__field(u8, flags)
> +		__field(u64, overflow_t)

The above looks nice and compact.

> +	    ),
> +
> +	    TP_fast_assign(
> +		__entry->pid = pid;
> +		__assign_str(region, region ? region : "");
> +		__assign_str(memdev, memdev);
> +		__assign_str(pcidev, pcidev);
> +		__entry->hpa = hpa;
> +		__entry->dpa = dpa;
> +		__entry->length = length;
> +		__entry->source = source;
> +		__entry->flags = flags;
> +		__entry->overflow_t = overflow_t;

But I wonder if it would be better to move the computation of patch 2
here?

That is, this:

> +		for (i = 0; i < le16_to_cpu(po->count); i++) {
> +			u32 len = le32_to_cpu(po->record[i].length) *
> +					      CXL_POISON_LEN_MULT;
> +			u64 addr = le64_to_cpu(po->record[i].address);
> +			u8 source = addr & CXL_POISON_SOURCE_MASK;
> +			u64 dpa = addr & CXL_POISON_START_MASK;
> +			u64 hpa = 0;
> +
> +			trace_cxl_poison(current->pid, region_name,
> +					 dev_name(&cxlmd->dev),
> +					 dev_name(cxlds->dev), hpa, dpa, len,
> +					 source, po->flags, overflow_t);
> +		}

As:

// The trace_*_enabled() is a static branch which is true when the
// tracepoint is enabled, and a nop when not (skipping the for block)

		for (i = 0; trace_cxl_poison_enabled() && i < le16_to_cpu(po->count); i++)
			trace_cxl_poison(region_name, cxlmd, cxlds, &po->record[i]);

And then have:

	    TP_fast_assign(
		u32 len = le32_to_cpu(record->length) *
				      CXL_POISON_LEN_MULT;
		u64 addr = le64_to_cpu(record->address);
		u8 source = addr & CXL_POISON_SOURCE_MASK;
		u64 dpa = addr & CXL_POISON_START_MASK;
		u64 hpa = 0;

		__entry->pid = current->pid;

{ the above isn't needed as the trace event will have common_pid = current->pid }

		__assign_str(region, region ? region : "");
		__assign_str(memdev, dev_name(&cxlmd->dev));
		__assign_str(pcidev, dev_name(cxlds->dev));
		__entry->hpa = hpa;
		__entry->dpa = dpa;
		__entry->length = length;
		__entry->source = source;
		__entry->flags = flags;
		__entry->overflow_t = overflow_t;


Or something similar. This will keep the work out of the code path.

-- Steve


> +	    ),
> +
> +	    TP_printk("pid:%d region:%s memdev:%s pcidev:%s hpa:0x%llx dpa:0x%llx length:0x%x source:%s flags:%s overflow_time:%llu",
> +		__entry->pid,
> +		__get_str(region),
> +		__get_str(memdev),
> +		__get_str(pcidev),
> +		__entry->hpa,
> +		__entry->dpa,
> +		__entry->length,
> +		show_poison_source(__entry->source),
> +		show_poison_flags(__entry->flags),
> +		__entry->overflow_t)
> +);
> +#endif /* _CXL_TRACE_H */