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

[Juergen Gross <jgross@xxxxxxxx>]

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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature