Re: [PATCH v2 5/7] x86: relocate pvh_info

From: "Jan Beulich" <JBeulich@suse.com>
To: Wei Liu <wei.liu2@citrix.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2 5/7] x86: relocate pvh_info
Date: Mon, 22 Jan 2018 03:31:22 -0700	[thread overview]
Message-ID: <5A65CB8A02000078001A0FC8@prv-mh.provo.novell.com> (raw)
In-Reply-To: <20180119163900.auf6tkm7o2vjvy5g@citrix.com>

>>> On 19.01.18 at 17:39, <wei.liu2@citrix.com> wrote:
> On Fri, Jan 19, 2018 at 04:29:31PM +0000, Roger Pau Monné wrote:
>> On Fri, Jan 19, 2018 at 03:34:56PM +0000, Wei Liu wrote:
>> > diff --git a/xen/arch/x86/boot/build32.mk b/xen/arch/x86/boot/build32.mk
>> > index 48c7407c00..028ac19b96 100644
>> > --- a/xen/arch/x86/boot/build32.mk
>> > +++ b/xen/arch/x86/boot/build32.mk
>> > @@ -36,5 +36,8 @@ CFLAGS := $(filter-out -flto,$(CFLAGS))
>> >  cmdline.o: cmdline.c $(CMDLINE_DEPS)
>> >  
>> >  reloc.o: reloc.c $(RELOC_DEPS)
>> > +ifeq ($(CONFIG_PVH_GUEST),y)
>> > +reloc.o: CFLAGS += -DCONFIG_PVH_GUEST
>> > +endif
>> 
>> I would maybe do this above, where the rest of the CFLAGS are set.
>> Certainly setting -DCONFIG_PVH_GUEST shouldn't cause issues elsewhere.
>> 
>> CFLAGS-$(CONFIG_PVH_GUEST) += -DCONFIG_PVH_GUEST
>> CFLAGS += $(CFLAGS-y)
>> 
>> >  .PRECIOUS: %.bin %.lnk
>> > diff --git a/xen/arch/x86/boot/defs.h b/xen/arch/x86/boot/defs.h
>> > index 6abdc15446..05921a64a3 100644
>> > --- a/xen/arch/x86/boot/defs.h
>> > +++ b/xen/arch/x86/boot/defs.h
>> > @@ -51,6 +51,9 @@ typedef unsigned short u16;
>> >  typedef unsigned int u32;
>> >  typedef unsigned long long u64;
>> >  typedef unsigned int size_t;
>> > +typedef u8 uint8_t;
>> > +typedef u32 uint32_t;
>> > +typedef u64 uint64_t;
>> 
>> This this seems to be always expanding, maybe better to simply replace
>> the stdbool.h include above with types.h?
>> 
> 
> I'm two minded here. My impression is that this wants to be minimal and
> standalone. The content in types.h is a lot more than we need here.

Please keep it the (minimal) way you have it.

>> >  #define U16_MAX		((u16)(~0U))
>> >  #define UINT_MAX	(~0U)
>> > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
>> > index 0f652cea11..614e53081e 100644
>> > --- a/xen/arch/x86/boot/head.S
>> > +++ b/xen/arch/x86/boot/head.S
>> > @@ -414,6 +414,7 @@ __pvh_start:
>> >  
>> >          /* Set trampoline_phys to use mfn 1 to avoid having a mapping at VA 0 */
>> >          movw    $0x1000, sym_esi(trampoline_phys)
>> > +        movl    $0x336ec578, %eax /* mov $XEN_HVM_START_MAGIC_VALUE, %eax */
>> 
>> Hm, if XEN_HVM_START_MAGIC_VALUE cannot be used I would rather prefer
>> to use (%ebx).
> 
> The same reason I didn't include types.h + hvm_start_info.h here.
> 
> We can include both to make $XEN_HVM_START_MAGIC_VALUE work. But I think
> using (%ebx) is better in here.

I agree (%ebx) is preferable.

Jan

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