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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2635 bytes --]

On 20.05.21 11:28, Jan Beulich wrote:
> 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.

Would it be possible to add some comment to xen/include/public/elfnote.h
Indicating which elfnotes are evaluated for which guest types, including
a hint which elfnotes _have_ been evaluated before this series? This
will help cleaning up guests regarding advertisement of elfnotes
(something I've been planning to do for the Linux kernel).


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]