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 <jbeulich@suse.com>

--- 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;
     }

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 <jbeulich@suse.com>
> 
> --- 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.

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 <jbeulich@suse.com>
>>
>> --- 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

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 <jbeulich@suse.com>
> >>
> >> --- 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.

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 <jbeulich@suse.com>
>>>
>>> --- 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