[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/3] x86/PVH: Support relocatable dom0 kernels
On 21.03.2024 14:45, Jason Andryuk wrote: > On 2024-03-20 10:39, Jan Beulich wrote: >> On 19.03.2024 21:58, Jason Andryuk wrote: >>> +/* Find an e820 RAM region that fits the kernel at a suitable alignment. */ >>> +static paddr_t __init find_kernel_memory( >>> + const struct domain *d, struct elf_binary *elf, >>> + const struct elf_dom_parms *parms) >>> +{ >>> + paddr_t kernel_size = elf->dest_size; >>> + unsigned int i; >>> + >>> + for ( i = 0; i < d->arch.nr_e820; i++ ) >>> + { >>> + paddr_t start = d->arch.e820[i].addr; >>> + paddr_t end = d->arch.e820[i].addr + d->arch.e820[i].size; >>> + paddr_t kstart, kend; >>> + >>> + if ( d->arch.e820[i].type != E820_RAM || >>> + d->arch.e820[i].size < kernel_size ) >>> + continue; >>> + >>> + kstart = ROUNDUP(start, parms->phys_align); >>> + kstart = max_t(paddr_t, kstart, parms->phys_min); >> >> I'd generally try to avoid max_t(), but I cannot think of any good way >> of expressing this without using it. >> >>> + kend = kstart + kernel_size; >>> + >>> + if ( kend > parms->phys_max ) >> >> So despite its default phys_max is exclusive aiui. Otherwise with >> kend being exclusive too, this would look to be off by one. > > Yes, I'll fix the off-by-one. Hmmm, phys_max being 32bit, we want it to > be inclusive to represent the maximum range. I specifically didn't ask for that, as I think it would end up a little awkward. But I also don't mind you adjusting things to that effect. >>> + return 0; >>> + >>> + if ( kend <= end ) >>> + return kstart; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/* Check the kernel load address, and adjust if necessary and possible. */ >>> +static bool __init check_and_adjust_load_address( >>> + const struct domain *d, struct elf_binary *elf, struct elf_dom_parms >>> *parms) >>> +{ >>> + paddr_t reloc_base; >>> + >>> + if ( check_load_address(d, elf) ) >>> + return true; >>> + >>> + if ( !parms->phys_reloc ) >>> + { >>> + printk("%pd kernel: Address conflict and not relocatable\n", d); >>> + return false; >> >> This better wouldn't result in -ENOMEM in the caller. Then again reasons >> are logged, so the specific error code perhaos doesn't matter that much. > > Failure here is turned into -ENOMEM by pvh_load_kernel(). -ENOMEM is > already returned for later failure with find_memory(), so I thought it > was acceptable. Without this code, elf_load_binary() would failed with > -1 and that would be returned. I'll change it to whatever you prefer. ENOSPC would likely make it easily distinguishable from anything else. >>> + } >>> + >>> + reloc_base = find_kernel_memory(d, elf, parms); >>> + if ( reloc_base == 0 ) >> >> With ! used above please be consistent and do so here, too. > > phys_reloc is a bool, and reloc_base is a paddr_t. Hmm, I see. But still - we generally prefer ! over "== 0" (or even "== NULL"). >>> + { >>> + printk("%pd kernel: Failed find a load address\n", d); >>> + return false; >>> + } >>> + >>> + if ( opt_dom0_verbose ) >>> + printk("%pd kernel: Moving [%p, %p] -> [%"PRIpaddr", >>> %"PRIpaddr"]\n", d, >>> + elf->dest_base, elf->dest_base + elf->dest_size - 1, >>> + reloc_base, reloc_base + elf->dest_size - 1); >>> + >>> + parms->phys_entry = reloc_base + >>> + (parms->phys_entry - (paddr_t)elf->dest_base); >>> + elf->dest_base = (char *)reloc_base; >> >> Just as a remark, no request to change anything: We're not dealing with >> strings here. Hence char * isn't quite right, just that "dest_base" is >> of that type (for whatever reason). > > I think the reason is just to be byte addressable. Sure, but that would call for void *, unsigned char *, or uint8_t *. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |