Re: [PATCH v2] libelf: improve PVH elfnote parsing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2021 11:27, Roger Pau Monné wrote:
> On Wed, May 19, 2021 at 12:34:19PM +0200, Jan Beulich wrote:
>> On 18.05.2021 16:47, Roger Pau Monne wrote:
>>> @@ -425,8 +425,11 @@ static elf_errorstatus elf_xen_addr_calc_check(struct 
>>> elf_binary *elf,
>>>          return -1;
>>>      }
>>>  
>>> -    /* Initial guess for virt_base is 0 if it is not explicitly defined. */
>>> -    if ( parms->virt_base == UNSET_ADDR )
>>> +    /*
>>> +     * Initial guess for virt_base is 0 if it is not explicitly defined in 
>>> the
>>> +     * PV case. For PVH virt_base is forced to 0 because paging is 
>>> disabled.
>>> +     */
>>> +    if ( parms->virt_base == UNSET_ADDR || hvm )
>>>      {
>>>          parms->virt_base = 0;
>>>          elf_msg(elf, "ELF: VIRT_BASE unset, using %#" PRIx64 "\n",
>>
>> This message is wrong now if hvm is true but parms->virt_base != UNSET_ADDR.
>> Best perhaps is to avoid emitting the message altogether when hvm is true.
>> (Since you'll be touching it anyway, perhaps a good opportunity to do away
>> with passing parms->virt_base to elf_msg(), and instead simply use a literal
>> zero.)
>>
>>> @@ -441,8 +444,10 @@ static elf_errorstatus elf_xen_addr_calc_check(struct 
>>> elf_binary *elf,
>>>       *
>>>       * If we are using the modern ELF notes interface then the default
>>>       * is 0.
>>> +     *
>>> +     * For PVH this is forced to 0, as it's already a legacy option for PV.
>>>       */
>>> -    if ( parms->elf_paddr_offset == UNSET_ADDR )
>>> +    if ( parms->elf_paddr_offset == UNSET_ADDR || hvm )
>>>      {
>>>          if ( parms->elf_note_start )
>>
>> Don't you want "|| hvm" here as well, or alternatively suppress the
>> fallback to the __xen_guest section in the PVH case (near the end of
>> elf_xen_parse())?
> 
> The legacy __xen_guest section doesn't support PHYS32_ENTRY, so yes,
> that part could be completely skipped when called from an HVM
> container.
> 
> I think I will fix that in another patch though if you are OK, as
> it's not strictly related to the calculation fixes done here.

That's fine; it wants to be a prereq to the one here then, though,
I think.

Jan