Re: [Minios-devel] [UNIKRAFT PATCH v4 2/9] plat/linuxu: Add scheduling support for x86_64

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Sharan,

Please see my comments inline.

On 08/24/2018 04:03 PM, Sharan Santhanam wrote:
> Hello,
> 
> Please find the comments inline:
> 
> On 08/20/2018 01:21 PM, Florian Schmidt wrote:
>> From: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
>>
>> We are using the software context for scheduling on plat/linuxu.
>>
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>>   plat/linuxu/Makefile.uk | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/plat/linuxu/Makefile.uk b/plat/linuxu/Makefile.uk
>> index 8d79fd7..38fcedc 100644
>> --- a/plat/linuxu/Makefile.uk
>> +++ b/plat/linuxu/Makefile.uk
>> @@ -11,12 +11,20 @@ $(eval $(call addplatlib,linuxu,liblinuxuplat))
>>   ##
>>   ## Platform library definitions
>>   ##
>> -LIBLINUXUPLAT_CINCLUDES-y += -I$(LIBLINUXUPLAT_BASE)/include
>> +LIBLINUXUPLAT_ASINCLUDES-y        += -I$(LIBLINUXUPLAT_BASE)/include
>> +LIBLINUXUPLAT_ASINCLUDES-y        += -I$(UK_PLAT_COMMON_BASE)/include
>> +LIBLINUXUPLAT_CINCLUDES-y         += -I$(LIBLINUXUPLAT_BASE)/include
>> +LIBLINUXUPLAT_CINCLUDES-y         += -I$(UK_PLAT_COMMON_BASE)/include
>>   
> 
> Not related to this patch but ARCH_X86_32 replaced with CONFIG_ARCH_X86_32
>>   LIBLINUXUPLAT_SRCS-$(ARCH_X86_32) +=
>> $(LIBLINUXUPLAT_BASE)/x86/entry32.S
>>   LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
>> $(LIBLINUXUPLAT_BASE)/x86/entry64.S
>>   LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_ARM_32) +=
>> $(LIBLINUXUPLAT_BASE)/arm/entry32.S
> Not related to this patch but ARCH_ARM_64 replaced with CONFIG_ARCH_ARM_64
> 
> It is wise to fix it as a separate patch
>>   LIBLINUXUPLAT_SRCS-$(ARCH_ARM_64) +=
>> $(LIBLINUXUPLAT_BASE)/arm/entry64.S
>> +ifeq ($(CONFIG_HAVE_SCHED),y)
>> +LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
>> $(UK_PLAT_COMMON_BASE)/x86/thread_start.S|common
> 
> Why are thread.c and sw_ctx.c specific to the X86_64?

You're right, they aren't.

> 
> If we only support x86_64 platform for now it might be wise to mention
> it as a comment or else it is better to add those files to the x86 folder?

For sure these files should stay in the common folder. That flag there
was set like that because at that time it was very fuzzy regarding the
ARM support on linuxu. Besides that, after adding ARM support that flag
would be changed to 'y' anyhow.

>> +LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
>> $(UK_PLAT_COMMON_BASE)/thread.c|common
>> +LIBLINUXUPLAT_SRCS-$(CONFIG_ARCH_X86_64) +=
>> $(UK_PLAT_COMMON_BASE)/sw_ctx.c|common
>> +endif
>>   LIBLINUXUPLAT_SRCS-y              += $(LIBLINUXUPLAT_BASE)/setup.c
>>   LIBLINUXUPLAT_SRCS-y              += $(LIBLINUXUPLAT_BASE)/console.c
>>   LIBLINUXUPLAT_SRCS-y              += $(LIBLINUXUPLAT_BASE)/shutdown.c
>>
> 
> 
> Thanks & Regards
> Sharan
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel