| Message ID | 20240326213847.3944-2-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pvh: Support relocating dom0 kernel | expand |
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@amd.com> 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. 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. I further meanwhile realized that consulting the ELF phdrs may also be ambiguous, as there may be more than one. I guess it would need to be the maximum of all of them then. Jan
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@amd.com> > > 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. 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 further meanwhile realized that consulting the ELF phdrs may also be > ambiguous, as there may be more than one. I guess it would need to be the > maximum of all of them then. My suggestion (not sure if I mentioned this before) was to use the alignment of the first LOAD PHDR, which is the one that defines the value of the dest_base field used as the image load start address. Using the maximum of all load PHDRs might be safer. Thanks, Roger.
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@amd.com> >> >> 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? 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; else if ( elf->palign > PHYS32_RELOC_ALIGN_DEFAULT ) align = elf->palign; else align = PHYS32_RELOC_ALIGN_DEFAULT; >> I further meanwhile realized that consulting the ELF phdrs may also be >> ambiguous, as there may be more than one. I guess it would need to be the >> maximum of all of them then. > > My suggestion (not sure if I mentioned this before) was to use the > alignment of the first LOAD PHDR, which is the one that defines the > value of the dest_base field used as the image load start address. > > Using the maximum of all load PHDRs might be safer. I'll find the maximum. Thanks, Jason
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@amd.com> >>> >>> 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. Jan > else if ( elf->palign > PHYS32_RELOC_ALIGN_DEFAULT ) > align = elf->palign; > else > align = PHYS32_RELOC_ALIGN_DEFAULT; > > >>> I further meanwhile realized that consulting the ELF phdrs may also be >>> ambiguous, as there may be more than one. I guess it would need to be the >>> maximum of all of them then. >> >> My suggestion (not sure if I mentioned this before) was to use the >> alignment of the first LOAD PHDR, which is the one that defines the >> value of the dest_base field used as the image load start address. >> >> Using the maximum of all load PHDRs might be safer. > > I'll find the maximum. > > Thanks, > Jason
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@amd.com> >>>> >>>> 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
diff --git a/xen/arch/x86/bzimage.c b/xen/arch/x86/bzimage.c index 0f4cfc49f7..ac4fd428be 100644 --- a/xen/arch/x86/bzimage.c +++ b/xen/arch/x86/bzimage.c @@ -105,7 +105,7 @@ unsigned long __init bzimage_headroom(void *image_start, } int __init bzimage_parse(void *image_base, void **image_start, - unsigned long *image_len, unsigned int *align) + unsigned long *image_len) { struct setup_header *hdr = (struct setup_header *)(*image_start); int err = bzimage_check(hdr, *image_len); @@ -118,8 +118,6 @@ int __init bzimage_parse(void *image_base, void **image_start, { *image_start += (hdr->setup_sects + 1) * 512 + hdr->payload_offset; *image_len = hdr->payload_length; - if ( align ) - *align = hdr->kernel_alignment; } if ( elf_is_elfbinary(*image_start, *image_len) ) diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index bbae8a5645..0ceda4140b 100644 --- a/xen/arch/x86/hvm/dom0_build.c +++ b/xen/arch/x86/hvm/dom0_build.c @@ -548,14 +548,12 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image, struct elf_binary elf; struct elf_dom_parms parms; paddr_t last_addr; - unsigned int align = 0; struct hvm_start_info start_info = { 0 }; struct hvm_modlist_entry mod = { 0 }; struct vcpu *v = d->vcpu[0]; int rc; - rc = bzimage_parse(image_base, &image_start, &image_len, &align); - if ( rc != 0 ) + if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 ) { printk("Error trying to detect bz compressed kernel\n"); return rc; diff --git a/xen/arch/x86/include/asm/bzimage.h b/xen/arch/x86/include/asm/bzimage.h index 2e04f5cc7b..7ed69d3910 100644 --- a/xen/arch/x86/include/asm/bzimage.h +++ b/xen/arch/x86/include/asm/bzimage.h @@ -6,6 +6,6 @@ unsigned long bzimage_headroom(void *image_start, unsigned long image_length); int bzimage_parse(void *image_base, void **image_start, - unsigned long *image_len, unsigned int *align); + unsigned long *image_len); #endif /* __X86_BZIMAGE_H__ */ diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c index e9fa8a9a82..d8043fa58a 100644 --- a/xen/arch/x86/pv/dom0_build.c +++ b/xen/arch/x86/pv/dom0_build.c @@ -416,7 +416,7 @@ int __init dom0_construct_pv(struct domain *d, d->max_pages = ~0U; - if ( (rc = bzimage_parse(image_base, &image_start, &image_len, NULL)) != 0 ) + if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 ) return rc; if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
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@amd.com> --- xen/arch/x86/bzimage.c | 4 +--- xen/arch/x86/hvm/dom0_build.c | 4 +--- xen/arch/x86/include/asm/bzimage.h | 2 +- xen/arch/x86/pv/dom0_build.c | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-)