x86/PVH: Dom0 "broken ELF" reporting adjustments

Commit Message

Jan Beulich Jan. 17, 2024, 8:53 a.m. UTC

elf_load_binary() isn't the primary source of brokenness being
indicated. Therefore make the respective log message there conditional
(much like PV has it), and add another instance when elf_xen_parse()
failed (again matching behavior in the PV case).

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Roger Pau Monne Jan. 17, 2024, 10:25 a.m. UTC | #1

On Wed, Jan 17, 2024 at 09:53:26AM +0100, Jan Beulich wrote:
> elf_load_binary() isn't the primary source of brokenness being
> indicated. Therefore make the respective log message there conditional
> (much like PV has it), and add another instance when elf_xen_parse()
> failed (again matching behavior in the PV case).
> 
> Signed-off-by: Jan Beulich <[e-mail]>
> 
> --- a/xen/arch/x86/hvm/dom0_build.c
> +++ b/xen/arch/x86/hvm/dom0_build.c
> @@ -570,6 +570,8 @@ static int __init pvh_load_kernel(struct
>      if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
>      {
>          printk("Unable to parse kernel for ELFNOTES\n");
> +        if ( elf_check_broken(&elf) )
> +            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));

I would rather use "%pd: kernel broken ELF: %s\n", in case this gets
used for loading more than dom0 in the dom0less case.  The 'Xen'
prefix is IMO useless here (I know it was here before).

Thanks, Roger.

Jan Beulich Jan. 17, 2024, 10:42 a.m. UTC | #2

On 17.01.2024 11:25, Roger Pau Monné wrote:
> On Wed, Jan 17, 2024 at 09:53:26AM +0100, Jan Beulich wrote:
>> elf_load_binary() isn't the primary source of brokenness being
>> indicated. Therefore make the respective log message there conditional
>> (much like PV has it), and add another instance when elf_xen_parse()
>> failed (again matching behavior in the PV case).
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>>
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -570,6 +570,8 @@ static int __init pvh_load_kernel(struct
>>      if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
>>      {
>>          printk("Unable to parse kernel for ELFNOTES\n");
>> +        if ( elf_check_broken(&elf) )
>> +            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> 
> I would rather use "%pd: kernel broken ELF: %s\n", in case this gets
> used for loading more than dom0 in the dom0less case.  The 'Xen'
> prefix is IMO useless here (I know it was here before).

Can do. But if I do, I'd like to bring PV in sync with this as well,
right in the same patch. I hope you don't mind that.

Jan

Andrew Cooper Jan. 17, 2024, 10:44 a.m. UTC | #3

On 17/01/2024 10:42 am, Jan Beulich wrote:
> On 17.01.2024 11:25, Roger Pau Monné wrote:
>> On Wed, Jan 17, 2024 at 09:53:26AM +0100, Jan Beulich wrote:
>>> elf_load_binary() isn't the primary source of brokenness being
>>> indicated. Therefore make the respective log message there conditional
>>> (much like PV has it), and add another instance when elf_xen_parse()
>>> failed (again matching behavior in the PV case).
>>>
>>> Signed-off-by: Jan Beulich <[e-mail]>
>>>
>>> --- a/xen/arch/x86/hvm/dom0_build.c
>>> +++ b/xen/arch/x86/hvm/dom0_build.c
>>> @@ -570,6 +570,8 @@ static int __init pvh_load_kernel(struct
>>>      if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
>>>      {
>>>          printk("Unable to parse kernel for ELFNOTES\n");
>>> +        if ( elf_check_broken(&elf) )
>>> +            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
>> I would rather use "%pd: kernel broken ELF: %s\n", in case this gets
>> used for loading more than dom0 in the dom0less case.  The 'Xen'
>> prefix is IMO useless here (I know it was here before).
> Can do. But if I do, I'd like to bring PV in sync with this as well,
> right in the same patch. I hope you don't mind that.

Sounds good to me.

~Andrew

Roger Pau Monne Jan. 17, 2024, 11:13 a.m. UTC | #4

On Wed, Jan 17, 2024 at 11:42:53AM +0100, Jan Beulich wrote:
> On 17.01.2024 11:25, Roger Pau Monné wrote:
> > On Wed, Jan 17, 2024 at 09:53:26AM +0100, Jan Beulich wrote:
> >> elf_load_binary() isn't the primary source of brokenness being
> >> indicated. Therefore make the respective log message there conditional
> >> (much like PV has it), and add another instance when elf_xen_parse()
> >> failed (again matching behavior in the PV case).
> >>
> >> Signed-off-by: Jan Beulich <[e-mail]>
> >>
> >> --- a/xen/arch/x86/hvm/dom0_build.c
> >> +++ b/xen/arch/x86/hvm/dom0_build.c
> >> @@ -570,6 +570,8 @@ static int __init pvh_load_kernel(struct
> >>      if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
> >>      {
> >>          printk("Unable to parse kernel for ELFNOTES\n");
> >> +        if ( elf_check_broken(&elf) )
> >> +            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> > 
> > I would rather use "%pd: kernel broken ELF: %s\n", in case this gets
> > used for loading more than dom0 in the dom0less case.  The 'Xen'
> > prefix is IMO useless here (I know it was here before).
> 
> Can do. But if I do, I'd like to bring PV in sync with this as well,
> right in the same patch. I hope you don't mind that.

Sure, please go ahead.

Thanks, Roger.

Patch

--- a/xen/arch/x86/hvm/dom0_build.c
+++ b/xen/arch/x86/hvm/dom0_build.c
@@ -570,6 +570,8 @@  static int __init pvh_load_kernel(struct
     if ( (rc = elf_xen_parse(&elf, &parms, true)) != 0 )
     {
         printk("Unable to parse kernel for ELFNOTES\n");
+        if ( elf_check_broken(&elf) )
+            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
         return rc;
     }
 
@@ -588,7 +590,8 @@  static int __init pvh_load_kernel(struct
     if ( rc < 0 )
     {
         printk("Failed to load kernel: %d\n", rc);
-        printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
+        if ( elf_check_broken(&elf) )
+            printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
         return rc;
     }