Re: [PATCH 05/12] uprobes: move offset and ref_ctr_offset into uprobe_consumer
On Mon, 24 Jun 2024 17:21:37 -0700 Andrii Nakryiko wrote: > Simplify uprobe registration/unregistration interfaces by making offset > and ref_ctr_offset part of uprobe_consumer "interface". In practice, all > existing users already store these fields somewhere in uprobe_consumer's > containing structure, so this doesn't pose any problem. We just move > some fields around. > > On the other hand, this simplifies uprobe_register() and > uprobe_unregister() API by having only struct uprobe_consumer as one > thing representing attachment/detachment entity. This makes batched > versions of uprobe_register() and uprobe_unregister() simpler. > > This also makes uprobe_register_refctr() unnecessary, so remove it and > simplify consumers. > > No functional changes intended. > Looks good to me. Acked-by: Masami Hiramatsu (Google) Thanks! > Signed-off-by: Andrii Nakryiko > --- > include/linux/uprobes.h | 18 +++ > kernel/events/uprobes.c | 19 ++- > kernel/trace/bpf_trace.c | 21 +++- > kernel/trace/trace_uprobe.c | 53 --- > .../selftests/bpf/bpf_testmod/bpf_testmod.c | 23 > 5 files changed, 55 insertions(+), 79 deletions(-) > > diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h > index b503fafb7fb3..a75ba37ce3c8 100644 > --- a/include/linux/uprobes.h > +++ b/include/linux/uprobes.h > @@ -42,6 +42,11 @@ struct uprobe_consumer { > enum uprobe_filter_ctx ctx, > struct mm_struct *mm); > > + /* associated file offset of this probe */ > + loff_t offset; > + /* associated refctr file offset of this probe, or zero */ > + loff_t ref_ctr_offset; > + /* for internal uprobe infra use, consumers shouldn't touch fields > below */ > struct uprobe_consumer *next; > }; > > @@ -110,10 +115,9 @@ extern bool is_trap_insn(uprobe_opcode_t *insn); > extern unsigned long uprobe_get_swbp_addr(struct pt_regs *regs); > extern unsigned long uprobe_get_trap_addr(struct pt_regs *regs); > extern int uprobe_write_opcode(struct arch_uprobe *auprobe, struct mm_struct > *mm, unsigned long vaddr, uprobe_opcode_t); > -extern int uprobe_register(struct inode *inode, loff_t offset, struct > uprobe_consumer *uc); > -extern int uprobe_register_refctr(struct inode *inode, loff_t offset, loff_t > ref_ctr_offset, struct uprobe_consumer *uc); > +extern int uprobe_register(struct inode *inode, struct uprobe_consumer *uc); > extern int uprobe_apply(struct inode *inode, loff_t offset, struct > uprobe_consumer *uc, bool); > -extern void uprobe_unregister(struct inode *inode, loff_t offset, struct > uprobe_consumer *uc); > +extern void uprobe_unregister(struct inode *inode, struct uprobe_consumer > *uc); > extern int uprobe_mmap(struct vm_area_struct *vma); > extern void uprobe_munmap(struct vm_area_struct *vma, unsigned long start, > unsigned long end); > extern void uprobe_start_dup_mmap(void); > @@ -152,11 +156,7 @@ static inline void uprobes_init(void) > #define uprobe_get_trap_addr(regs) instruction_pointer(regs) > > static inline int > -uprobe_register(struct inode *inode, loff_t offset, struct uprobe_consumer > *uc) > -{ > - return -ENOSYS; > -} > -static inline int uprobe_register_refctr(struct inode *inode, loff_t offset, > loff_t ref_ctr_offset, struct uprobe_consumer *uc) > +uprobe_register(struct inode *inode, struct uprobe_consumer *uc) > { > return -ENOSYS; > } > @@ -166,7 +166,7 @@ uprobe_apply(struct inode *inode, loff_t offset, struct > uprobe_consumer *uc, boo > return -ENOSYS; > } > static inline void > -uprobe_unregister(struct inode *inode, loff_t offset, struct uprobe_consumer > *uc) > +uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc) > { > } > static inline int uprobe_mmap(struct vm_area_struct *vma) > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c > index 8ce669bc6474..2544e8b79bad 100644 > --- a/kernel/events/uprobes.c > +++ b/kernel/events/uprobes.c > @@ -1197,14 +1197,13 @@ __uprobe_unregister(struct uprobe *uprobe, struct > uprobe_consumer *uc) > /* > * uprobe_unregister - unregister an already registered probe. > * @inode: the file in which the probe has to be removed. > - * @offset: offset from the start of the file. > - * @uc: identify which probe if multiple probes are colocated. > + * @uc: identify which probe consumer to unregister. > */ > -void uprobe_unregister(struct inode *inode, loff_t offset, struct > uprobe_consumer *uc) > +void uprobe_unregister(struct inode *inode, struct uprobe_consumer *uc) > { > struct uprobe *uprobe; > > - uprobe = find_uprobe(inode, offset); > + uprobe = find_uprobe(inode, uc->offset); > if (WARN_ON(!uprobe)) > return; > > @@ -1277,20 +1276,12 @@ static int __uprobe_register(struct inode *inode, > loff_t offset, > return ret; > }
[PATCH v1 3/4] platform/x86/intel/ifs: Add SBAF test support
From: Jithu Joseph In a core, the SBAF test engine is shared between sibling CPUs. An SBAF test image contains multiple bundles. Each bundle is further composed of subunits called programs. When a SBAF test (for a particular core) is triggered by the user, each SBAF bundle from the loaded test image is executed sequentially on all the threads on the core using the stop_core_cpuslocked mechanism. Each bundle execution is initiated by writing to MSR_ACTIVATE_SBAF. SBAF test bundle execution may be aborted when an interrupt occurs or if the CPU does not have enough power budget for the test. In these cases the kernel restarts the test from the aborted bundle. SBAF execution is not retried if the test fails or if the test makes no forward progress after 5 retries. Reviewed-by: Ashok Raj Reviewed-by: Tony Luck Signed-off-by: Jithu Joseph Signed-off-by: Kuppuswamy Sathyanarayanan --- drivers/platform/x86/intel/ifs/ifs.h | 30 +++ drivers/platform/x86/intel/ifs/runtest.c | 234 +++ 2 files changed, 264 insertions(+) diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 600bb8a1b285..b261be46bce8 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -157,6 +157,8 @@ #define MSR_SBAF_HASHES_STATUS 0x02b9 #define MSR_AUTHENTICATE_AND_COPY_SBAF_CHUNK 0x02ba #define MSR_SBAF_CHUNKS_AUTHENTICATION_STATUS 0x02bb +#define MSR_ACTIVATE_SBAF 0x02bc +#define MSR_SBAF_STATUS0x02bd #define MSR_COPY_SCAN_HASHES 0x02c2 #define MSR_SCAN_HASHES_STATUS 0x02c3 @@ -283,6 +285,34 @@ union ifs_array { }; }; +/* MSR_ACTIVATE_SBAF bit fields */ +union ifs_sbaf { + u64 data; + struct { + u32 bundle_idx :9; + u32 rsvd1 :5; + u32 pgm_idx :2; + u32 rsvd2 :16; + u32 delay :31; + u32 sigmce :1; + }; +}; + +/* MSR_SBAF_STATUS bit fields */ +union ifs_sbaf_status { + u64 data; + struct { + u32 bundle_idx :9; + u32 rsvd1 :5; + u32 pgm_idx :2; + u32 rsvd2 :16; + u32 error_code :8; + u32 rsvd3 :21; + u32 test_fail :1; + u32 sbaf_status :2; + }; +}; + /* * Driver populated error-codes * 0xFD: Test timed out before completing all the chunks. diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index 282e4bfe30da..bdb31b2f45b4 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -29,6 +29,13 @@ struct run_params { union ifs_status status; }; +struct sbaf_run_params { + struct ifs_data *ifsd; + int *retry_cnt; + union ifs_sbaf *activate; + union ifs_sbaf_status status; +}; + /* * Number of TSC cycles that a logical CPU will wait for the other * logical CPU on the core in the WRMSR(ACTIVATE_SCAN). @@ -146,6 +153,7 @@ static bool can_restart(union ifs_status status) #define SPINUNIT 100 /* 100 nsec */ static atomic_t array_cpus_in; static atomic_t scan_cpus_in; +static atomic_t sbaf_cpus_in; /* * Simplified cpu sibling rendezvous loop based on microcode loader __wait_for_cpus() @@ -387,6 +395,226 @@ static void ifs_array_test_gen1(int cpu, struct device *dev) ifsd->status = SCAN_TEST_PASS; } +#define SBAF_STATUS_PASS 0 +#define SBAF_STATUS_SIGN_FAIL 1 +#define SBAF_STATUS_INTR 2 +#define SBAF_STATUS_TEST_FAIL 3 + +enum sbaf_status_err_code { + IFS_SBAF_NO_ERROR = 0, + IFS_SBAF_OTHER_THREAD_COULD_NOT_JOIN= 1, + IFS_SBAF_INTERRUPTED_BEFORE_RENDEZVOUS = 2, + IFS_SBAF_UNASSIGNED_ERROR_CODE3 = 3, + IFS_SBAF_INVALID_BUNDLE_INDEX = 4, + IFS_SBAF_MISMATCH_ARGS_BETWEEN_THREADS = 5, + IFS_SBAF_CORE_NOT_CAPABLE_CURRENTLY = 6, + IFS_SBAF_UNASSIGNED_ERROR_CODE7 = 7, + IFS_SBAF_EXCEED_NUMBER_OF_THREADS_CONCURRENT= 8, + IFS_SBAF_INTERRUPTED_DURING_EXECUTION = 9, + IFS_SBAF_INVALID_PROGRAM_INDEX = 0xA, + IFS_SBAF_CORRUPTED_CHUNK= 0xB, + IFS_SBAF_DID_NOT_START = 0xC, +}; + +static const char * const sbaf_test_status[] = { + [IFS_SBAF_NO_ERROR] = "SBAF no error", + [IFS_SBAF_OTHER_THREAD_COULD_NOT_JOIN] = "Other thread could not join.", +
[PATCH v1 4/4] trace: platform/x86/intel/ifs: Add SBAF trace support
From: Jithu Joseph Add tracing support for the SBAF IFS tests, which may be useful for debugging systems that fail these tests. Log details like test content batch number, SBAF bundle ID, program index and the exact errors or warnings encountered by each HT thread during the test. Reviewed-by: Ashok Raj Reviewed-by: Tony Luck Signed-off-by: Jithu Joseph Signed-off-by: Kuppuswamy Sathyanarayanan --- include/trace/events/intel_ifs.h | 27 drivers/platform/x86/intel/ifs/runtest.c | 1 + 2 files changed, 28 insertions(+) diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h index 0d88ebf2c980..9c7413de432b 100644 --- a/include/trace/events/intel_ifs.h +++ b/include/trace/events/intel_ifs.h @@ -35,6 +35,33 @@ TRACE_EVENT(ifs_status, __entry->status) ); +TRACE_EVENT(ifs_sbaf, + + TP_PROTO(int batch, union ifs_sbaf activate, union ifs_sbaf_status status), + + TP_ARGS(batch, activate, status), + + TP_STRUCT__entry( + __field(int,batch ) + __field(u64,status ) + __field(u16,bundle ) + __field(u16,pgm ) + ), + + TP_fast_assign( + __entry->batch = batch; + __entry->bundle = activate.bundle_idx; + __entry->pgm= activate.pgm_idx; + __entry->status = status.data; + ), + + TP_printk("batch: 0x%.2x, bundle_idx: 0x%.4x, pgm_idx: 0x%.4x, status: 0x%.16llx", + __entry->batch, + __entry->bundle, + __entry->pgm, + __entry->status) +); + #endif /* _TRACE_IFS_H */ /* This part must be outside protection */ diff --git a/drivers/platform/x86/intel/ifs/runtest.c b/drivers/platform/x86/intel/ifs/runtest.c index bdb31b2f45b4..69ee0eb72025 100644 --- a/drivers/platform/x86/intel/ifs/runtest.c +++ b/drivers/platform/x86/intel/ifs/runtest.c @@ -530,6 +530,7 @@ static int dosbaf(void *data) */ wrmsrl(MSR_ACTIVATE_SBAF, run_params->activate->data); rdmsrl(MSR_SBAF_STATUS, status.data); + trace_ifs_sbaf(ifsd->cur_batch, *run_params->activate, status); /* Pass back the result of the test */ if (cpu == first) -- 2.25.1
[PATCH v1 1/4] platform/x86/intel/ifs: Refactor MSR usage in IFS test code
IFS tests such as Scan at Field (SAF) or Structural Based Functional Test at Field (SBAF), require the user to load a test image. The image loading process is similar across these tests, with the only difference being MSR addresses used. To reuse the code between these tests, remove the hard coding of MSR addresses and allow the driver to pass the MSR addresses per IFS test (via driver device data). Add a new structure named "struct ifs_test_msrs" to specify the test-specific MSR addresses. Each IFS test will provide this structure, enabling them to reuse the common code. This is a preliminary patch in preparation for the addition of SBAF support. Reviewed-by: Ashok Raj Reviewed-by: Tony Luck Signed-off-by: Kuppuswamy Sathyanarayanan --- drivers/platform/x86/intel/ifs/ifs.h | 25 + drivers/platform/x86/intel/ifs/core.c | 9 + drivers/platform/x86/intel/ifs/load.c | 24 ++-- 3 files changed, 48 insertions(+), 10 deletions(-) diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 56b9f3e3cf76..738cbc7a5d00 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -266,6 +266,22 @@ struct ifs_test_caps { int test_num; }; +/** + * struct ifs_test_msrs - MSRs used in IFS tests + * @copy_hashes: Copy test hash data + * @copy_hashes_status: Status of copied test hash data + * @copy_chunks: Copy chunks of the test data + * @copy_chunks_status: Status of the copied test data chunks + * @test_ctrl: Control the test attributes + */ +struct ifs_test_msrs { + u32 copy_hashes; + u32 copy_hashes_status; + u32 copy_chunks; + u32 copy_chunks_status; + u32 test_ctrl; +}; + /** * struct ifs_data - attributes related to intel IFS driver * @loaded_version: stores the currently loaded ifs image version. @@ -299,6 +315,7 @@ struct ifs_work { struct ifs_device { const struct ifs_test_caps *test_caps; + const struct ifs_test_msrs *test_msrs; struct ifs_data rw_data; struct miscdevice misc; }; @@ -319,6 +336,14 @@ static inline const struct ifs_test_caps *ifs_get_test_caps(struct device *dev) return d->test_caps; } +static inline const struct ifs_test_msrs *ifs_get_test_msrs(struct device *dev) +{ + struct miscdevice *m = dev_get_drvdata(dev); + struct ifs_device *d = container_of(m, struct ifs_device, misc); + + return d->test_msrs; +} + extern bool *ifs_pkg_auth; int ifs_load_firmware(struct device *dev); int do_core_test(int cpu, struct device *dev); diff --git a/drivers/platform/x86/intel/ifs/core.c b/drivers/platform/x86/intel/ifs/core.c index 7b11198d85a1..1a7ca74abb61 100644 --- a/drivers/platform/x86/intel/ifs/core.c +++ b/drivers/platform/x86/intel/ifs/core.c @@ -40,9 +40,18 @@ static const struct ifs_test_caps array_test = { .test_num = IFS_TYPE_ARRAY_BIST, }; +static const struct ifs_test_msrs scan_msrs = { + .copy_hashes = MSR_COPY_SCAN_HASHES, + .copy_hashes_status = MSR_SCAN_HASHES_STATUS, + .copy_chunks = MSR_AUTHENTICATE_AND_COPY_CHUNK, + .copy_chunks_status = MSR_CHUNKS_AUTHENTICATION_STATUS, + .test_ctrl = MSR_SAF_CTRL, +}; + static struct ifs_device ifs_devices[] = { [IFS_TYPE_SAF] = { .test_caps = _test, + .test_msrs = _msrs, .misc = { .name = "intel_ifs_0", .minor = MISC_DYNAMIC_MINOR, diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c index 39f19cb51749..ad0c107f0922 100644 --- a/drivers/platform/x86/intel/ifs/load.c +++ b/drivers/platform/x86/intel/ifs/load.c @@ -118,15 +118,17 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work) union ifs_scan_hashes_status hashes_status; union ifs_chunks_auth_status chunk_status; struct device *dev = local_work->dev; + const struct ifs_test_msrs *msrs; int i, num_chunks, chunk_size; struct ifs_data *ifsd; u64 linear_addr, base; u32 err_code; ifsd = ifs_get_data(dev); + msrs = ifs_get_test_msrs(dev); /* run scan hash copy */ - wrmsrl(MSR_COPY_SCAN_HASHES, ifs_hash_ptr); - rdmsrl(MSR_SCAN_HASHES_STATUS, hashes_status.data); + wrmsrl(msrs->copy_hashes, ifs_hash_ptr); + rdmsrl(msrs->copy_hashes_status, hashes_status.data); /* enumerate the scan image information */ num_chunks = hashes_status.num_chunks; @@ -147,8 +149,8 @@ static void copy_hashes_authenticate_chunks(struct work_struct *work) linear_addr = base + i * chunk_size; linear_addr |= i; - wrmsrl(MSR_AUTHENTICATE_AND_COPY_CHUNK, linear_addr); - rdmsrl(MSR_CHUNKS_AUTHENTICATION_STATUS, chunk_status.data); + wrmsrl(msrs->copy_chunks,
[PATCH v1 2/4] platform/x86/intel/ifs: Add SBAF test image loading support
From: Jithu Joseph Structural Based Functional Test at Field (SBAF) is a new type of testing that provides comprehensive core test coverage complementing existing IFS tests like Scan at Field (SAF) or ArrayBist. SBAF device will appear as a new device instance (intel_ifs_2) under /sys/devices/virtual/misc. The user interaction necessary to load the test image and test a particular core is the same as the existing scan test (intel_ifs_0). During the loading stage, the driver will look for a file named ff-mm-ss-.sbft in the /lib/firmware/intel/ifs_2 directory. The hardware interaction needed for loading the image is similar to SAF, with the only difference being the MSR addresses used. Reuse the SAF image loading code, passing the SBAF-specific MSR addresses via struct ifs_test_msrs in the driver device data. Unlike SAF, the SBAF test image chunks are further divided into smaller logical entities called bundles. Since the SBAF test is initiated per bundle, cache the maximum number of bundles in the current image, which is used for iterating through bundles during SBAF test execution. Reviewed-by: Ashok Raj Reviewed-by: Tony Luck Signed-off-by: Jithu Joseph Co-developed-by: Kuppuswamy Sathyanarayanan Signed-off-by: Kuppuswamy Sathyanarayanan --- arch/x86/include/asm/msr-index.h | 2 ++ drivers/platform/x86/intel/ifs/ifs.h | 37 ++- drivers/platform/x86/intel/ifs/core.c | 24 + drivers/platform/x86/intel/ifs/load.c | 15 --- 4 files changed, 73 insertions(+), 5 deletions(-) diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index e022e6eb766c..503d7acdda3f 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -247,6 +247,8 @@ #define MSR_INTEGRITY_CAPS_ARRAY_BIST BIT(MSR_INTEGRITY_CAPS_ARRAY_BIST_BIT) #define MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT 4 #define MSR_INTEGRITY_CAPS_PERIODIC_BIST BIT(MSR_INTEGRITY_CAPS_PERIODIC_BIST_BIT) +#define MSR_INTEGRITY_CAPS_SBAF_BIT8 +#define MSR_INTEGRITY_CAPS_SBAF BIT(MSR_INTEGRITY_CAPS_SBAF_BIT) #define MSR_INTEGRITY_CAPS_SAF_GEN_MASKGENMASK_ULL(10, 9) #define MSR_LBR_NHM_FROM 0x0680 diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 738cbc7a5d00..600bb8a1b285 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -126,11 +126,38 @@ * The driver does not make use of this, it only tests one core at a time. * * .. [#f1] https://github.com/intel/TBD + * + * + * Structural Based Functional Test at Field (SBAF): + * + * + * SBAF is a new type of testing that provides comprehensive core test + * coverage complementing Scan at Field (SAF) testing. SBAF mimics the + * manufacturing screening environment and leverages the same test suite. + * It makes use of Design For Test (DFT) observation sites and features + * to maximize coverage in minimum time. + * + * Similar to the SAF test, SBAF isolates the core under test from the + * rest of the system during execution. Upon completion, the core + * seamlessly resets to its pre-test state and resumes normal operation. + * Any machine checks or hangs encountered during the test are confined to + * the isolated core, preventing disruption to the overall system. + * + * Like the SAF test, the SBAF test is also divided into multiple batches, + * and each batch test can take hundreds of milliseconds (100-200 ms) to + * complete. If such a lengthy interruption is undesirable, it is + * recommended to relocate the time-sensitive applications to other cores. */ #include #include #define MSR_ARRAY_BIST 0x0105 + +#define MSR_COPY_SBAF_HASHES 0x02b8 +#define MSR_SBAF_HASHES_STATUS 0x02b9 +#define MSR_AUTHENTICATE_AND_COPY_SBAF_CHUNK 0x02ba +#define MSR_SBAF_CHUNKS_AUTHENTICATION_STATUS 0x02bb + #define MSR_COPY_SCAN_HASHES 0x02c2 #define MSR_SCAN_HASHES_STATUS 0x02c3 #define MSR_AUTHENTICATE_AND_COPY_CHUNK0x02c4 @@ -140,6 +167,7 @@ #define MSR_ARRAY_TRIGGER 0x02d6 #define MSR_ARRAY_STATUS 0x02d7 #define MSR_SAF_CTRL 0x04f0 +#define MSR_SBAF_CTRL 0x04f8 #define SCAN_NOT_TESTED0 #define SCAN_TEST_PASS 1 @@ -147,6 +175,7 @@ #define IFS_TYPE_SAF 0 #define IFS_TYPE_ARRAY_BIST1 +#define IFS_TYPE_SBAF 2 #define ARRAY_GEN0 0 #define ARRAY_GEN1 1 @@ -196,7 +225,8 @@ union ifs_chunks_auth_status_gen2 { u16 valid_chunks; u16 total_chunks;
[PATCH v1 0/4] Add SBAF test to IFS
This patch series adds support for Structural Based Functional Test at Field (SBAF) in the IFS driver. SBAF is a new type of testing that provides comprehensive core test coverage, complementing existing IFS tests like Scan at Field (SAF) and ArrayBist. Granite Rapids (GNR) is the first platform that supports SBAF. SBAF mimics the manufacturing screening environment and leverages the same test suite. It makes use of Design For Test (DFT) observation sites and features to maximize coverage in minimum time. Similar to the SAF test, SBAF isolates the core under test from the rest of the system during execution. Upon completion, the core seamlessly resets to its pre-test state and resumes normal operation. Any machine checks or hangs encountered during the test are confined to the isolated core, preventing disruption to the overall system. Like SAF test, the SBAF test is also divided into multiple batches, and each batch test can take hundreds of milliseconds (100-200 ms) to complete. If such a lengthy interruption is undesirable, it is recommended to relocate the time-sensitive applications to other cores for the duration of the test. Patch Details: Patch 1/4: Refactors MSR usage in IFS image loading code to share the code between SBAF and SAF tests. Patch 2/4: Leverages SAF image loading logic and adds SBAF image loading support. Patch 3/4: Adds support for user to trigger SBAF test. Patch 4/4: Adds trace support for SBAF tests. This series is originally authored by Jithu Joseph. I have made cleanups related to code reuse between the SBAF and SAF tests and resubmitting it for review. Jithu Joseph (3): platform/x86/intel/ifs: Add SBAF test image loading support platform/x86/intel/ifs: Add SBAF test support trace: platform/x86/intel/ifs: Add SBAF trace support Kuppuswamy Sathyanarayanan (1): platform/x86/intel/ifs: Refactor MSR usage in IFS test code arch/x86/include/asm/msr-index.h | 2 + drivers/platform/x86/intel/ifs/ifs.h | 92 - include/trace/events/intel_ifs.h | 27 +++ drivers/platform/x86/intel/ifs/core.c| 33 drivers/platform/x86/intel/ifs/load.c| 39 ++-- drivers/platform/x86/intel/ifs/runtest.c | 235 +++ 6 files changed, 413 insertions(+), 15 deletions(-) -- 2.25.1
Re: [PATCH 04/12] uprobes: revamp uprobe refcounting and lifetime management
On Mon, 24 Jun 2024 17:21:36 -0700 Andrii Nakryiko wrote: > Anyways, under exclusive writer lock, we double-check that refcount > didn't change and is still zero. If it is, we proceed with destruction, > because at that point we have a guarantee that find_active_uprobe() > can't successfully look up this uprobe instance, as it's going to be > removed in destructor under writer lock. If, on the other hand, > find_active_uprobe() managed to bump refcount from zero to one in > between put_uprobe()'s atomic_dec_and_test(>ref) and > write_lock(_treelock), we'll deterministically detect this with > extra atomic_read(>ref) check, and if it doesn't hold, we > pretend like atomic_dec_and_test() never returned true. There is no > resource freeing or any other irreversible action taken up till this > point, so we just exit early. > > One tricky part in the above is actually two CPUs racing and dropping > refcnt to zero, and then attempting to free resources. This can happen > as follows: > - CPU #0 drops refcnt from 1 to 0, and proceeds to grab uprobes_treelock; > - before CPU #0 grabs a lock, CPU #1 updates refcnt as 0 -> 1 -> 0, at > which point it decides that it needs to free uprobe as well. > > At this point both CPU #0 and CPU #1 will believe they need to destroy > uprobe, which is obviously wrong. To prevent this situations, we augment > refcount with epoch counter, which is always incremented by 1 on either > get or put operation. This allows those two CPUs above to disambiguate > who should actually free uprobe (it's the CPU #1, because it has > up-to-date epoch). See comments in the code and note the specific values > of UPROBE_REFCNT_GET and UPROBE_REFCNT_PUT constants. Keep in mind that > a single atomi64_t is actually a two sort-of-independent 32-bit counters > that are incremented/decremented with a single atomic_add_and_return() > operation. Note also a small and extremely rare (and thus having no > effect on performance) need to clear the highest bit every 2 billion > get/put operations to prevent high 32-bit counter from "bleeding over" > into lower 32-bit counter. I have a question here. Is there any chance to the CPU#1 to put the uprobe before CPU#0 gets the uprobes_treelock, and free uprobe before CPU#0 validate uprobe->ref again? e.g. CPU#0 CPU#1 put_uprobe() { atomic64_add_return() __get_uprobe(); put_uprobe() { kfree(uprobe) } write_lock(_treelock); atomic64_read(>ref); } I think it is very rare case, but I could not find any code to prevent this scenario. Thank you, -- Masami Hiramatsu (Google)
Re: [PATCH 06/12] uprobes: add batch uprobe register/unregister APIs
On Wed, Jun 26, 2024 at 4:27 AM Jiri Olsa wrote: > > On Mon, Jun 24, 2024 at 05:21:38PM -0700, Andrii Nakryiko wrote: > > SNIP > > > + for (i = 0; i < cnt; i++) { > > + uc = get_uprobe_consumer(i, ctx); > > + > > + /* Each consumer must have at least one set consumer */ > > + if (!uc || (!uc->handler && !uc->ret_handler)) > > + return -EINVAL; > > + /* Racy, just to catch the obvious mistakes */ > > + if (uc->offset > i_size_read(inode)) > > + return -EINVAL; > > + if (uc->uprobe) > > + return -EINVAL; > > + /* > > + * This ensures that copy_from_page(), copy_to_page() and > > + * __update_ref_ctr() can't cross page boundary. > > + */ > > + if (!IS_ALIGNED(uc->offset, UPROBE_SWBP_INSN_SIZE)) > > + return -EINVAL; > > + if (!IS_ALIGNED(uc->ref_ctr_offset, sizeof(short))) > > + return -EINVAL; > > + } > > > > - down_write(>register_rwsem); > > - consumer_add(uprobe, uc); > > - ret = register_for_each_vma(uprobe, uc); > > - if (ret) > > - __uprobe_unregister(uprobe, uc); > > - up_write(>register_rwsem); > > + for (i = 0; i < cnt; i++) { > > + uc = get_uprobe_consumer(i, ctx); > > > > - if (ret) > > - put_uprobe(uprobe); > > + uprobe = alloc_uprobe(inode, uc->offset, uc->ref_ctr_offset); > > + if (IS_ERR(uprobe)) { > > + ret = PTR_ERR(uprobe); > > + goto cleanup_uprobes; > > + } > > + > > + uc->uprobe = uprobe; > > + } > > + > > + for (i = 0; i < cnt; i++) { > > + uc = get_uprobe_consumer(i, ctx); > > + uprobe = uc->uprobe; > > + > > + down_write(>register_rwsem); > > + consumer_add(uprobe, uc); > > + ret = register_for_each_vma(uprobe, uc); > > + if (ret) > > + __uprobe_unregister(uprobe, uc); > > + up_write(>register_rwsem); > > + > > + if (ret) { > > + put_uprobe(uprobe); > > + goto cleanup_unreg; > > + } > > + } > > + > > + return 0; > > > > +cleanup_unreg: > > + /* unregister all uprobes we managed to register until failure */ > > + for (i--; i >= 0; i--) { > > + uc = get_uprobe_consumer(i, ctx); > > + > > + down_write(>register_rwsem); > > + __uprobe_unregister(uc->uprobe, uc); > > + up_write(>register_rwsem); > > + } > > +cleanup_uprobes: > > when we jump here from 'goto cleanup_uprobes' not all of the > consumers might have uc->uprobe set up > > perhaps we can set cnt = i - 1 before the goto, or just check uc->uprobe below yep, you are right, I missed this part during multiple rounds of refactorings. I think the `if (uc->uprobe)` check is the cleanest approach here. > > > > + /* put all the successfully allocated/reused uprobes */ > > + for (i = cnt - 1; i >= 0; i--) { > > curious, any reason why we go from the top here? No particular reason. This started as (i = i - 1; i >= 0; i--), but then as I kept splitting steps I needed to do this over all uprobes. Anyways, I can do a clean `i = 0; i < cnt; i++` with `if (uc->uprobe)` check. > > thanks, > jirka > > > + uc = get_uprobe_consumer(i, ctx); > > + > > + put_uprobe(uc->uprobe); > > + uc->uprobe = NULL; > > + } > > return ret; > > } > > > > int uprobe_register(struct inode *inode, struct uprobe_consumer *uc) > > { > > - return __uprobe_register(inode, uc->offset, uc->ref_ctr_offset, uc); > > + return uprobe_register_batch(inode, 1, uprobe_consumer_identity, uc); > > } > > EXPORT_SYMBOL_GPL(uprobe_register); > > > > -- > > 2.43.0 > >
Re: [PATCH 04/12] uprobes: revamp uprobe refcounting and lifetime management
On Tue, Jun 25, 2024 at 11:03 PM kernel test robot wrote: > > Hi Andrii, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on next-20240624] > [also build test WARNING on v6.10-rc5] > [cannot apply to perf-tools-next/perf-tools-next tip/perf/core > perf-tools/perf-tools linus/master acme/perf/core v6.10-rc5 v6.10-rc4 > v6.10-rc3] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: > https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/uprobes-update-outdated-comment/20240626-001728 > base: next-20240624 > patch link: > https://lore.kernel.org/r/20240625002144.3485799-5-andrii%40kernel.org > patch subject: [PATCH 04/12] uprobes: revamp uprobe refcounting and lifetime > management > config: x86_64-defconfig > (https://download.01.org/0day-ci/archive/20240626/202406261300.ebbfm0xj-...@intel.com/config) > compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 > reproduce (this is a W=1 build): > (https://download.01.org/0day-ci/archive/20240626/202406261300.ebbfm0xj-...@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version > of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot > | Closes: > https://lore.kernel.org/oe-kbuild-all/202406261300.ebbfm0xj-...@intel.com/ > > All warnings (new ones prefixed by >>): > > >> kernel/events/uprobes.c:638: warning: Function parameter or struct member > >> 'uprobe' not described in '__get_uprobe' > >> kernel/events/uprobes.c:638: warning: expecting prototype for Caller has > >> to make sure that(). Prototype was for __get_uprobe() instead > > > vim +638 kernel/events/uprobes.c > > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 625 > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 626 /** I shouldn't have used /** here, I'll fix this. > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 627 * Caller has to make sure > that: > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 628 * a) either uprobe's > refcnt is positive before this call; > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 629 * b) or uprobes_treelock > is held (doesn't matter if for read or write), > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 630 * preventing uprobe's > destructor from removing it from uprobes_tree. > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 631 * > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 632 * In the latter case, > uprobe's destructor will "resurrect" uprobe instance if > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 633 * it detects that its > refcount went back to being positive again inbetween it > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 634 * dropping to zero at some > point and (potentially delayed) destructor > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 635 * callback actually running. > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 636 */ > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 637 static struct uprobe > *__get_uprobe(struct uprobe *uprobe) > f231722a2b27ee Oleg Nesterov 2015-07-21 @638 { > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 639 s64 v; > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 640 > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 641 v = > atomic64_add_return(UPROBE_REFCNT_GET, >ref); > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 642 > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 643 /* > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 644 * If the highest bit > is set, we need to clear it. If cmpxchg() fails, > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 645 * we don't retry > because there is another CPU that just managed to > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 646 * update refcnt and > will attempt the same "fix up". Eventually one of > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 647 * them will succeed > to clear highset bit. > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 648 */ > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 649 if (unlikely(v < 0)) > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 650 > (void)atomic64_cmpxchg(>ref, v, v & ~(1ULL << 63)); > b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 651 > f231722a2b27ee Oleg Nesterov 2015-07-21 652 return uprobe; > f231722a2b27ee Oleg Nesterov 2015-07-21 653 } > f231722a2b27ee Oleg Nesterov 2015-07-21 654 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki
Re: [PATCH 02/12] uprobes: grab write mmap lock in unapply_uprobe()
On Tue, Jun 25, 2024 at 12:09 PM Oleg Nesterov wrote: > > On 06/25, Andrii Nakryiko wrote: > > > > On Tue, Jun 25, 2024 at 7:51 AM Oleg Nesterov wrote: > > > > > > Why? > > > > > > So far I don't understand this change. Quite possibly I missed something, > > > but in this case the changelog should explain the problem more clearly. > > > > > > > I just went off of "Called with mm->mmap_lock held for write." comment > > in uprobe_write_opcode(), tbh. > > Ah, indeed... and git blame makes me sad ;) > > I _think_ that 29dedee0e693a updated this comment without any thinking, > but today I can't recall. In any case, today this nothing to do with > mem_cgroup_charge(). Not sure __replace_page() is correct (in this respect) > when it returns -EAGAIN but this is another story. > > > If we don't actually need writer > > mmap_lock, we should probably update at least that comment. > > Agreed. Ok, I'll drop this change and will just update the comment. > > > There is a > > lot going on in uprobe_write_opcode(), and I don't understand all the > > requirements there. > > Heh. Neither me today ;) > > Oleg. >
Re: [PATCH 06/12] uprobes: add batch uprobe register/unregister APIs
On Mon, Jun 24, 2024 at 05:21:38PM -0700, Andrii Nakryiko wrote: SNIP > + for (i = 0; i < cnt; i++) { > + uc = get_uprobe_consumer(i, ctx); > + > + /* Each consumer must have at least one set consumer */ > + if (!uc || (!uc->handler && !uc->ret_handler)) > + return -EINVAL; > + /* Racy, just to catch the obvious mistakes */ > + if (uc->offset > i_size_read(inode)) > + return -EINVAL; > + if (uc->uprobe) > + return -EINVAL; > + /* > + * This ensures that copy_from_page(), copy_to_page() and > + * __update_ref_ctr() can't cross page boundary. > + */ > + if (!IS_ALIGNED(uc->offset, UPROBE_SWBP_INSN_SIZE)) > + return -EINVAL; > + if (!IS_ALIGNED(uc->ref_ctr_offset, sizeof(short))) > + return -EINVAL; > + } > > - down_write(>register_rwsem); > - consumer_add(uprobe, uc); > - ret = register_for_each_vma(uprobe, uc); > - if (ret) > - __uprobe_unregister(uprobe, uc); > - up_write(>register_rwsem); > + for (i = 0; i < cnt; i++) { > + uc = get_uprobe_consumer(i, ctx); > > - if (ret) > - put_uprobe(uprobe); > + uprobe = alloc_uprobe(inode, uc->offset, uc->ref_ctr_offset); > + if (IS_ERR(uprobe)) { > + ret = PTR_ERR(uprobe); > + goto cleanup_uprobes; > + } > + > + uc->uprobe = uprobe; > + } > + > + for (i = 0; i < cnt; i++) { > + uc = get_uprobe_consumer(i, ctx); > + uprobe = uc->uprobe; > + > + down_write(>register_rwsem); > + consumer_add(uprobe, uc); > + ret = register_for_each_vma(uprobe, uc); > + if (ret) > + __uprobe_unregister(uprobe, uc); > + up_write(>register_rwsem); > + > + if (ret) { > + put_uprobe(uprobe); > + goto cleanup_unreg; > + } > + } > + > + return 0; > > +cleanup_unreg: > + /* unregister all uprobes we managed to register until failure */ > + for (i--; i >= 0; i--) { > + uc = get_uprobe_consumer(i, ctx); > + > + down_write(>register_rwsem); > + __uprobe_unregister(uc->uprobe, uc); > + up_write(>register_rwsem); > + } > +cleanup_uprobes: when we jump here from 'goto cleanup_uprobes' not all of the consumers might have uc->uprobe set up perhaps we can set cnt = i - 1 before the goto, or just check uc->uprobe below > + /* put all the successfully allocated/reused uprobes */ > + for (i = cnt - 1; i >= 0; i--) { curious, any reason why we go from the top here? thanks, jirka > + uc = get_uprobe_consumer(i, ctx); > + > + put_uprobe(uc->uprobe); > + uc->uprobe = NULL; > + } > return ret; > } > > int uprobe_register(struct inode *inode, struct uprobe_consumer *uc) > { > - return __uprobe_register(inode, uc->offset, uc->ref_ctr_offset, uc); > + return uprobe_register_batch(inode, 1, uprobe_consumer_identity, uc); > } > EXPORT_SYMBOL_GPL(uprobe_register); > > -- > 2.43.0 >
Re: [PATCH 04/12] uprobes: revamp uprobe refcounting and lifetime management
Hi Andrii, kernel test robot noticed the following build warnings: [auto build test WARNING on next-20240624] [also build test WARNING on v6.10-rc5] [cannot apply to perf-tools-next/perf-tools-next tip/perf/core perf-tools/perf-tools linus/master acme/perf/core v6.10-rc5 v6.10-rc4 v6.10-rc3] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/uprobes-update-outdated-comment/20240626-001728 base: next-20240624 patch link: https://lore.kernel.org/r/20240625002144.3485799-5-andrii%40kernel.org patch subject: [PATCH 04/12] uprobes: revamp uprobe refcounting and lifetime management config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20240626/202406261300.ebbfm0xj-...@intel.com/config) compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240626/202406261300.ebbfm0xj-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202406261300.ebbfm0xj-...@intel.com/ All warnings (new ones prefixed by >>): >> kernel/events/uprobes.c:638: warning: Function parameter or struct member >> 'uprobe' not described in '__get_uprobe' >> kernel/events/uprobes.c:638: warning: expecting prototype for Caller has to >> make sure that(). Prototype was for __get_uprobe() instead vim +638 kernel/events/uprobes.c b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 625 b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 626 /** b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 627 * Caller has to make sure that: b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 628 * a) either uprobe's refcnt is positive before this call; b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 629 * b) or uprobes_treelock is held (doesn't matter if for read or write), b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 630 * preventing uprobe's destructor from removing it from uprobes_tree. b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 631 * b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 632 * In the latter case, uprobe's destructor will "resurrect" uprobe instance if b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 633 * it detects that its refcount went back to being positive again inbetween it b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 634 * dropping to zero at some point and (potentially delayed) destructor b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 635 * callback actually running. b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 636 */ b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 637 static struct uprobe *__get_uprobe(struct uprobe *uprobe) f231722a2b27ee Oleg Nesterov 2015-07-21 @638 { b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 639 s64 v; b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 640 b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 641 v = atomic64_add_return(UPROBE_REFCNT_GET, >ref); b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 642 b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 643 /* b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 644 * If the highest bit is set, we need to clear it. If cmpxchg() fails, b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 645 * we don't retry because there is another CPU that just managed to b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 646 * update refcnt and will attempt the same "fix up". Eventually one of b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 647 * them will succeed to clear highset bit. b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 648 */ b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 649 if (unlikely(v < 0)) b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 650 (void)atomic64_cmpxchg(>ref, v, v & ~(1ULL << 63)); b9adadbcb8dfc8 Andrii Nakryiko 2024-06-24 651 f231722a2b27ee Oleg Nesterov 2015-07-21 652 return uprobe; f231722a2b27ee Oleg Nesterov 2015-07-21 653 } f231722a2b27ee Oleg Nesterov 2015-07-21 654 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki