Re: [PATCH v5 15/16] x86: make Xen early boot code relocatable

From: "Jan Beulich" <JBeulich@suse.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Juergen Gross <JGross@suse.com>,
	sstabellini@kernel.org, andrew.cooper3@citrix.com,
	cardoe@cardoe.com, pgnet.dev@gmail.com, ning.sun@intel.com,
	david.vrabel@citrix.com, xen-devel@lists.xenproject.org,
	qiaowei.ren@intel.com, gang.wei@intel.com, fu.wei@linaro.org
Subject: Re: [PATCH v5 15/16] x86: make Xen early boot code relocatable
Date: Thu, 01 Sep 2016 01:46:24 -0600	[thread overview]
Message-ID: <57C7F8F0020000780010AB88@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20160831205044.GJ5364@olila.local.net-space.pl>

>>> On 31.08.16 at 22:50, <daniel.kiper@oracle.com> wrote:
> On Wed, Aug 31, 2016 at 09:46:31AM -0600, Jan Beulich wrote:
>> >>> On 31.08.16 at 16:59, <daniel.kiper@oracle.com> wrote:
>> > On Thu, Aug 25, 2016 at 08:41:39AM -0600, Jan Beulich wrote:
>> >> >>> On 20.08.16 at 00:43, <daniel.kiper@oracle.com> wrote:
>> >> > --- a/xen/arch/x86/boot/head.S
>> >> > +++ b/xen/arch/x86/boot/head.S
>> >> > @@ -12,13 +12,16 @@
>> >> >          .text
>> >> >          .code32
>> >> >
>> >> > -#define sym_phys(sym)     ((sym) - __XEN_VIRT_START)
>> >> > +#define sym_offset(sym)   ((sym) - __XEN_VIRT_START)
>> >>
>> >> In a patch already large and hence hard to review, did you really
>> >> need to do this rename?
>> >
>> > I think that sym_offset() is better term here. However, if you wish
>> > I can leave sym_phys() as is. Though, IMO, sym_phys() name suggests
>> > that we are playing with physical addresses and it is not correct in
>> > all contexts, e.g. loot at fs_offset() (or sym_fs() as you wish).
>>
>> After some more thinking about this I agree sym_phys() isn't
>> ideal anymore after this patch. Still, to avoid unrelated changes in
>> this quite hard to review patch, I'd like to ask that you keep the
>> name here and do just the rename in a subsequent patch.
> 
> Granted!

Btw., to save on typing and since you will need to touch this anyway
- can I talk you into using sym_offs() instead of sym_offset()?

>> >> >          .pushsection .trampoline_rel, "a"
>> >> >          .long   trampoline_gdt + BOOT_PSEUDORM_CS + 2 - .
>> >> >          .long   trampoline_gdt + BOOT_PSEUDORM_DS + 2 - .
>> >> >          .popsection
>> >> >
>> >> > +GLOBAL(xen_img_load_base_addr)
>> >> > +        .long   0
>> >>
>> >> I've yet to understand the purpose of this symbol, but in any case:
>> >> If the trampoline code doesn't reference it, why does it get placed
>> >> here?
>> >
>> > This symbol was used by code which unconditionally relocated Xen image
>> > even if boot loader did work for us. I removed most of this code because
>> > we agreed that if boot loader relocated Xen image then we do not have to
>> > relocate it higher once again. If you are still OK with this idea I can go
>> > further, as I planned, and remove all such remnants from next release.
>> > However, it looks that then I have to store load address in xen_phys_start
>> > from 32-bit assembly code. So, it will be nice if I can define it as
>> > "unsigned int" instead of "unsigned long". Is it safe?
>>
>> I don't see why you shouldn't be able to store into the low 32 bits of
>> the variable without altering the high ones (which are all zero).
> 
> I was thinking about that too but IMO it is not nice. However, if you
> are OK with that I can do it.

Well, imo that's something which is perfectly fine in assembly code.

>> >> >          s = (boot_e820.map[i].addr + mask) & ~mask;
>> >> >          e = (boot_e820.map[i].addr + boot_e820.map[i].size) & ~mask;
>> >> > -        s = max_t(uint64_t, s, BOOTSTRAP_MAP_BASE);
>> >> >          if ( (boot_e820.map[i].type != E820_RAM) || (s >= e) )
>> >> >              continue;
>> >>
>> >> This means you now map memory between 2Mb and 16Mb here. Is
>> >> this necessary?
>> >
>> > Without this change Xen relocated by boot loader crashes with:
>> >
>> > (XEN) Panic on CPU 0:
>> > (XEN) Assertion 'l2e_get_flags(l2e) & _PAGE_PRESENT' failed at x86_64/mm.c:902
>> >
>> > So, it must be mapped.
>>
>> That's too little context to be useful for understanding the full
>> background.
> 
> Here it is:
>[...]
> Is it sufficient? If not please drop me a line what else do you need.

I'm sorry, but no. I didn't mean the whole register and stack dump. I
meant an explanation of _why_ this assertion triggers (after all it
doesn't trigger without your patches, and hence I can't just go and
check the relevant source files).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel