Xen project Mailing List

On 29. Apr 2019, at 17:47, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:

On 4/16/19 1:07 PM, Pawel Wieczorkiewicz wrote:

Handle .livepatch.hooks* and .altinstr_replacement sections as the
special sections with assigned group_size resolution function.
By default each .livepatch.hooks* sections' entry is 8 bytes long (a
pointer). The .altinstr_replacement section follows the .altinstructions
section settings.
Allow to specify different .livepatch.hooks* section entry size using
shell environment variable HOOK_STRUCT_SIZE.
Cleanup an incorrect indentation around group_size resulution functions.
Signed-off-by: Pawel Wieczorkiewicz <wipawel@xxxxxxxxx>
Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
Reviewed-by: Bjoern Doebel <doebel@xxxxxxxxx>
Reviewed-by: Norbert Manthey <nmanthey@xxxxxxxxx>
---
create-diff-object.c | 87 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/create-diff-object.c b/create-diff-object.c
index b0b4dcb..f6060cd 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -960,51 +960,64 @@ static void kpatch_mark_constant_labels_same(struct kpatch_elf *kelf)

Fix the indentation in the patch which introduces the problem rather than fixing it in a subsequent patch.

Oh, certainly. I forgot to update the commits involved properly.

ACK. Will fix.

   static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
{
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
}
   static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset)
{
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("BUG_STRUCT_SIZE");
-        size = str ? atoi(str) : 16;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("BUG_STRUCT_SIZE");
+ size = str ? atoi(str) : 16;
+ }
+
+ return size;
}
   static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
{
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("EX_STRUCT_SIZE");
-        size = str ? atoi(str) : 8;
-    }
-
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("EX_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ return size;
}
   static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
{
-    static int size = 0;
-    char *str;
-    if (!size) {
-        str = getenv("ALT_STRUCT_SIZE");
-        size = str ? atoi(str) : 12;
-    }
-
-    printf("altinstr_size=%d\n", size);
-    return size;
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("ALT_STRUCT_SIZE");
+ size = str ? atoi(str) : 12;
+ }
+
+ printf("altinstr_size=%d\n", size);
+ return size;
+}
+
+static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
+{
+ static int size = 0;
+ char *str;
+ if (!size) {
+ str = getenv("HOOK_STRUCT_SIZE");
+ size = str ? atoi(str) : 8;
+ }
+
+ printf("livepatch_hooks_size=%d\n", size);

If you want to keep this debugging output, rather use log_debug().

ACK. Will fix.

+ return size;
}
/*
@@ -1084,6 +1097,14 @@ static struct special_section special_sections[] = {
.name = ".altinstructions",
.group_size = altinstructions_group_size,
},
+ {
+ .name = ".altinstr_replacement",
+ .group_size = altinstructions_group_size,
+ },
+ {
+ .name = ".livepatch.hooks",
+ .group_size = livepatch_hooks_group_size,
+ },
{},
};

I don't understand the point of this change.

IIUC unlike .altinstructions, the .altinstr_replacement section does not have a fixed group size, so this change would not work other than by luck. If

That’s true. The .altinstr_replacement group_size should be 0 (aka undefined).

ACK. Will fix.

the goal was to prune bits of the .altinstr_replacement table that do not need to be included, you need to update the code in kpatch_process_special_sections() which has special handling for .altinstr_replacement. It needs to parse the updated, pruned .altinstructions section and regenerate .altinstr_replacement, including only what is needed.

No this was not the goal. I am explaining it below.

I don't know why .livepatch.hooks is included in this list either since all hooks are always included and there is nothing to regenerate/prune.

As discussed when reviewing [1], I need a function telling which section is special.

By special I mean:

a) should not be a subject of extra validation (See [2])

b) should not be a subject of STN_UNDEF purging (See [3])

Thus, I want to explicitly call out all "special" sections by their name and specify

their group_size whenever applicable (I did it incorrectly for .altinstr_replacement,

as you have spotted - thanks!) in the common array and use it with the helper

function or for obtaining a group_size.

[1] [livepatch-build-tools part2 3/6] create-diff-object: Add is_special_section() helper function

Re: [Xen-devel] [livepatch-build-tools part2 5/6] create-diff-object: Add new entries to special sections array