[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 1/6] Revert "xen/x86: bzImage parse kernel_alignment"


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Jason Andryuk <jason.andryuk@xxxxxxx>
  • Date: Wed, 27 Mar 2024 10:49:13 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=O/wHybwKnqkOrPy2lrQ1gouZB8NQXFES5OR2FLFWALw=; b=npXxmG1vGBOBFHBIQRcRWGcRiDE1Y8QI/y+3vPbu04luyc02gzoR0okekGi723mbThNyI+eFlYa//w4d3kAA5P4HTbmKLLHzSWSFJpoK32Fm+3Q9UsOg6Yih9MgqPyyqVQdZehJ3MEucHV/fN4IouenT9GpQ0WFpRDQPa+gfaYKTWxTESqjSl4dmfQfFW6ZnQbg6x+cwHHp0t8TeyRqGjfJDn6ZIGbp34douTCTJmOTFFqLVMd/gwwjrRMvhcoAYOsgstlZAGzfZs1SOUEnySU4MXj0DWlssjykCo4jqwatFO70IhUY/Hj8Zesji/Uq77ZNquWwjBatCwCGQ+y0eeQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Cxcm2U7yMKvyD5NQ2xkcpDKE0Wi1mn/SgQ6z/+WVMUUURXM3lS5ehTnPldkf9gs8qNN/aChHZMm9YPnpBLbCccNYFKsb6YNF2fASyQmmNor2I8Wsvzc3/U5izeUXwfNj9lGBZhJjUxkUvIPW795c6fYhoZAmEpmhmk6f0EWvG1Fcn3RK+eJa+NUqMcht3601gsushdWWKd/NaQ6lGms2AFfctBp3l6i+7b/hOI+5WvBqu7/Kz2xOdO7OslGQoyhttQzlT0KTCnpkrA8pkCuGPDpUm+T+psedLJg1hlWxSMROwsf5ZWgYSNsyujTVhEgWEMDJH5lAJWCSIFsQt/2rGg==
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 27 Mar 2024 14:49:26 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 2024-03-27 10:19, Jan Beulich wrote:
On 27.03.2024 15:08, Jason Andryuk wrote:
On 2024-03-27 04:59, Roger Pau Monné wrote:
On Wed, Mar 27, 2024 at 08:22:41AM +0100, Jan Beulich wrote:
On 26.03.2024 22:38, Jason Andryuk wrote:
A new ELF note will specify the alignment for a relocatable PVH kernel.
ELF notes are suitable for vmlinux and other ELF files, so this
Linux-specific bzImage parsing in unnecessary.

This reverts commit c44cac229067faeec8f49247d1cf281723ac2d40.

Signed-off-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Since you keep re-sending this: In private discussion Roger has indicated
that, like me, he too would prefer falling back to the ELF data, before
falling back to the arch default (Roger, please correct me if I got it
wrong). That would make it necessary that the change you're proposing to
revert here is actually kept.

Sorry, was meaning to reply yesterday but Jason is very fast at
sending new version so I'm always one version behind.

:)

I was hoping to finish this up and get it in...

IMO the order: ELF note, PHDR alignment, arch default should be the
preferred one.

Or wait - what you're reverting is taking the alignment out of the
bzImage header. I don't expect the BSDs to use that protocol; aiui that's
entirely Linux-specific.

Yeah, I don't have strong opinions in keeping this, we already do
bzImage parsing, so we might as well attempt to fetch the alignment
from there if correct:

ELF note, bzImage kernel_alignment, ELF PHDR alignment, arch default

I'm not sure how to handle ELF PHDR vs. arch default.  ELF PHDR will
always be set, AFAIU.  Should that always be respected, which means we
don't need an arch default?

A value of 0 (and 1) is specifically permitted, to indicate no alignment.
We may take 0 to mean default, but what you suggest below is another
plausible approach. Yet another might be to take anything below PAGE_SIZE
as "use default".

To include arch default, it would be something like this:

      if ( parms->phys_align != UNSET_ADDR )
          align = parms->phys_align;
      else if ( bz_align )
          align = bz_align;

Why do you include bz again here? Didn't you previously indicate the
header field can't be relied upon? Which is also why, finally, I committed
this revert earlier today.

Roger wanted to consult the bz value? He mentioned it above. Locally, I haven't synced with staging yet, and I dropped the revert to start re-working this...

If present, the bzImage header field is valid. But being bzImage-specific, it isn't useful for other ELF files. Xen will only move a kernel with the PHSY32_RELOC Note, so it can just specify an alignment if it needs to.

Personally, I think using the Note's value or a default is fine. It seems like the PHDR aligment will just be 0x200000 anyway (for x86-64 at lease), which matches the default. Specifying the PHYS32_RELOC Note, but relying on a search for the alignment, seems slightly questionable to me.

Still, it seemed like the path of least resistance is to just implement the fallback search like Roger requested. Dropping the bzImage, I guess I'd go with your PAGE_SIZE suggestions for:

    if ( parms->phys_align != UNSET_ADDR )
        align = parms->phys_align;
    else if ( elf->palign > PAGE_SIZE )
        align = elf->palign;
    else
        align = PHYS32_RELOC_ALIGN_DEFAULT;

Thanks for your reviews.

Regards,
Jason



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.