[Xen-devel] [PATCH] x86/Intel: Broadwell doesn't have PKG_C{8, 9, 10}_RESIDENCY MSRs

[Jan Beulich]

According to
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01797.html 
this partially reverts commit 350bc1a9d4 ("x86: support newer Intel CPU
models") to account for the appearant earlier mis-documentation.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -66,9 +66,9 @@
 #define GET_PC3_RES(val)  GET_HW_RES_IN_NS(0x3F8, val)
 #define GET_PC6_RES(val)  GET_HW_RES_IN_NS(0x3F9, val)
 #define GET_PC7_RES(val)  GET_HW_RES_IN_NS(0x3FA, val)
-#define GET_PC8_RES(val)  GET_HW_RES_IN_NS(0x630, val)
-#define GET_PC9_RES(val)  GET_HW_RES_IN_NS(0x631, val)
-#define GET_PC10_RES(val) GET_HW_RES_IN_NS(0x632, val)
+#define GET_PC8_RES(val)  GET_HW_RES_IN_NS(0x630, val) /* some Haswells only */
+#define GET_PC9_RES(val)  GET_HW_RES_IN_NS(0x631, val) /* some Haswells only */
+#define GET_PC10_RES(val) GET_HW_RES_IN_NS(0x632, val) /* some Haswells only */
 #define GET_CC1_RES(val)  GET_HW_RES_IN_NS(0x660, val) /* Silvermont only */
 #define GET_CC3_RES(val)  GET_HW_RES_IN_NS(0x3FC, val)
 #define GET_CC6_RES(val)  GET_HW_RES_IN_NS(0x3FD, val)
@@ -142,8 +142,6 @@ static void do_get_hw_residencies(void *
 {
 /* 4th generation Intel Core (Haswell) */
 case 0x45:
-/* Xeon E5/E7 v4 (Broadwell) */
-case 0x4F:
 GET_PC8_RES(hw_res->pc8);
 GET_PC9_RES(hw_res->pc9);
 GET_PC10_RES(hw_res->pc10);
@@ -161,6 +159,7 @@ static void do_get_hw_residencies(void *
 /* Broadwell */
 case 0x3D:
 case 0x47:
+case 0x4F:
 case 0x56:
 /* Skylake */
 case 0x4E:



x86/Intel: Broadwell doesn't have PKG_C{8,9,10}_RESIDENCY MSRs

According to
https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01797.html
this partially reverts commit 350bc1a9d4 ("x86: support newer Intel CPU
models") to account for the appearant earlier mis-documentation.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -66,9 +66,9 @@
 #define GET_PC3_RES(val)  GET_HW_RES_IN_NS(0x3F8, val)
 #define GET_PC6_RES(val)  GET_HW_RES_IN_NS(0x3F9, val)
 #define GET_PC7_RES(val)  GET_HW_RES_IN_NS(0x3FA, val)
-#define GET_PC8_RES(val)  GET_HW_RES_IN_NS(0x630, val)
-#define GET_PC9_RES(val)  GET_HW_RES_IN_NS(0x631, val)
-#define GET_PC10_RES(val) GET_HW_RES_IN_NS(0x632, val)
+#define GET_PC8_RES(val)  GET_HW_RES_IN_NS(0x630, val) /* some Haswells only */
+#define GET_PC9_RES(val)  GET_HW_RES_IN_NS(0x631, val) /* some Haswells only */
+#define GET_PC10_RES(val) GET_HW_RES_IN_NS(0x632, val) /* some Haswells only */
 #define GET_CC1_RES(val)  GET_HW_RES_IN_NS(0x660, val) /* Silvermont only */
 #define GET_CC3_RES(val)  GET_HW_RES_IN_NS(0x3FC, val)
 #define GET_CC6_RES(val)  GET_HW_RES_IN_NS(0x3FD, val)
@@ -142,8 +142,6 @@ static void do_get_hw_residencies(void *
 {
 /* 4th generation Intel Core (Haswell) */
 case 0x45:
-/* Xeon E5/E7 v4 (Broadwell) */
-case 0x4F:
 GET_PC8_RES(hw_res->pc8);
 GET_PC9_RES(hw_res->pc9);
 GET_PC10_RES(hw_res->pc10);
@@ -161,6 +159,7 @@ static void do_get_hw_residencies(void *
 /* Broadwell */
 case 0x3D:
 case 0x47:
+case 0x4F:
 case 0x56:
 /* Skylake */
 case 0x4E:
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Julien Grall]

Hello Peng,

On 19/09/2016 04:08, van.free...@gmail.com wrote:

From: Peng Fan 

This patchset is to support XEN run on big.little SoC.
The idea of the patch is from
"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";

There are some changes to cpupool and add x86 stub functions to avoid build
break. Sending The RFC patchset out is to request for comments to see whether
this implementation is acceptable or not. Patchset have been tested based on
xen-4.8 unstable on NXP i.MX8.

I use Big/Little CPU and cpupool to explain the idea.
A pool contains Big CPUs is called Big Pool.
A pool contains Little CPUs is called Little Pool.
If a pool does not contains any physical cpus, Little CPUs or Big CPUs
can be added to the cpupool. But the cpupool can not contain both Little
and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value for 
ARM).
CPUs can not be added to the cpupool which contains cpus that have different 
cpu type.
Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
type(same midr value)
into cpupool0.


As mentioned in the mail you pointed above, this series is not enough to 
make big.LITTLE working on then. Xen is always using the boot CPU to 
detect the list of features. With big.LITTLE features may not be the same.


And I would prefer to see Xen supporting big.LITTLE correctly before 
beginning to think to expose big.LITTLE to the userspace (via cpupool) 
automatically.


See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).



Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first one
that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
`xl cpupool-list -c` will show cpu[0-3] in Pool-0.

Then use the following script to create a new cpupool and add cpu[4-5] to
the cpupool.
 #xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
 #xl cpupool-cpu-add Pool-A72 4
 #xl cpupool-cpu-add Pool-A72 5
 #xl create -d /root/xen/domu-test pool=\"Pool-A72\"


I am a bit confused with these runes. It means that only the first kind 
of CPUs have pool assigned. Why don't you directly create all the pools 
at boot time?


Also, in which pool a domain will be created if none is specified?


Now `xl cpupool-list -c` shows:
NameCPU list
Pool-0  0,1,2,3
Pool-A724,5

`xl cpupool-list` shows:
Name   CPUs   Sched Active   Domain count
Pool-0   4credit   y  1
Pool-A72 2   credit2   y  1

`xl cpupool-cpu-remove Pool-A72 4`, then `xl cpupool-cpu-add Pool-0 4`
not success, because Pool-0 contains A53 CPUs, but CPU4 is an A72 CPU.

`xl cpupool-migrate DomU Pool-0` will also fail, because DomU is created
in Pool-A72 with A72 vcpu, while Pool-0 have A53 physical cpus.

Patch 1/5:
use "cpumask_weight(cpupool0->cpu_valid);" to replace "num_online_cpus()",
because num_online_cpus() counts all the online CPUs, but now we only
need Big or Little CPUs.


So if I understand correctly, if the boot CPU is a little CPU, DOM0 will 
always be able to only use little ones. Is that right?


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 2/2] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

[Jan Beulich]

>>> On 15.09.16 at 18:51,  wrote:
> @@ -1793,7 +1793,17 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt 
> *hvmemul_ctxt,
>  pfec |= PFEC_user_mode;
>  
>  hvmemul_ctxt->insn_buf_eip = regs->eip;
> -if ( !vio->mmio_insn_bytes )
> +
> +if ( unlikely(hvmemul_ctxt->set_context_insn) && curr->arch.vm_event )
> +{
> +BUILD_BUG_ON(sizeof(hvmemul_ctxt->insn_buf_bytes) ==
> + sizeof(curr->arch.vm_event->emul.insn));

This should quite clearly be !=, and I think it builds only because you
use the wrong operand in the first sizeof().

> +hvmemul_ctxt->insn_buf_bytes = 
> sizeof(curr->arch.vm_event->emul.insn);
> +memcpy(hvmemul_ctxt->insn_buf, &curr->arch.vm_event->emul.insn,
> +   hvmemul_ctxt->insn_buf_bytes);

This memcpy()s between dissimilar types. Please omit the & and
properly add .data on the second argument (and this .data
addition should then also be mirrored in the BUILD_BUG_ON()).

> +}
> +else if ( !vio->mmio_insn_bytes )

And then - I'm sorry for not having thought of this before - I think
this would better not live here, or have an effect more explicitly
only when coming here through hvm_emulate_one_vm_event().
Since the former seems impractical, I think giving _hvm_emulate_one()
one or two extra parameters would be the most straightforward
approach.

> @@ -1944,11 +1954,11 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, 
> unsigned int trapnr,
>  case EMUL_KIND_NOWRITE:
>  rc = hvm_emulate_one_no_write(&ctx);
>  break;
> -case EMUL_KIND_SET_CONTEXT:
> -ctx.set_context = 1;
> -/* Intentional fall-through. */
>  default:
> +ctx.set_context_data = (kind == EMUL_KIND_SET_CONTEXT_DATA);
> +ctx.set_context_insn = (kind == EMUL_KIND_SET_CONTEXT_INSN);
>  rc = hvm_emulate_one(&ctx);
> +break;
>  }

Thanks - this is a lot better readable now!

> @@ -1983,7 +1993,8 @@ void hvm_emulate_prepare(
>  hvmemul_ctxt->ctxt.force_writeback = 1;
>  hvmemul_ctxt->seg_reg_accessed = 0;
>  hvmemul_ctxt->seg_reg_dirty = 0;
> -hvmemul_ctxt->set_context = 0;
> +hvmemul_ctxt->set_context_data = 0;
> +hvmemul_ctxt->set_context_insn = 0;

I think you can almost guess the comment here and on the declaration
of these fields: Please use false here and plain bool there.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -489,13 +489,16 @@ void hvm_do_resume(struct vcpu *v)
>  
>  if ( v->arch.vm_event->emulate_flags &
>   VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -kind = EMUL_KIND_SET_CONTEXT;
> +kind = EMUL_KIND_SET_CONTEXT_DATA;
>  else if ( v->arch.vm_event->emulate_flags &
>VM_EVENT_FLAG_EMULATE_NOWRITE )
>  kind = EMUL_KIND_NOWRITE;
> +else if ( v->arch.vm_event->emulate_flags &
> + VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +kind = EMUL_KIND_SET_CONTEXT_INSN;

The header talking about incompatibilities between these flags -
wouldn't it be a good idea to actually -EINVAL such combinations?

> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -57,6 +57,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 

I have to admit that looking through the header changes you do I
can't figure why this adjustment is necessary.

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -209,11 +209,18 @@ void vm_event_emulate_check(struct vcpu *v, 
> vm_event_response_t *rsp)
>  if ( p2m_mem_access_emulate_check(v, rsp) )
>  {
>  if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
> -v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +v->arch.vm_event->emul.read = rsp->data.emul.read;
>  
>  v->arch.vm_event->emulate_flags = rsp->flags;
>  }
>  break;
> +case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
> +if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
> +{
> +v->arch.vm_event->emul.insn = rsp->data.emul.insn;
> +v->arch.vm_event->emulate_flags = rsp->flags;
> +}
> +break;
>  default:

Blank lines between non-fall-through case statements please (part
of me thinks I've said so before).

> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -398,7 +398,6 @@ void vm_event_resume(struct domain *d, struct 
> vm_event_domain *ved)
>   * In some cases the response type needs extra handling, so here
>   * we call the appropriate handlers.
>   */
> -
>  /* Check flags which apply only when the vCPU is paused */

Stray change.

Jan

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

Re: [Xen-devel] [PATCH v5 1/2] xen: replace tlbflush check and operation with inline functions

[Jan Beulich]

>>> On 19.09.16 at 04:50,  wrote:
> This patch cleaned up the code by replacing complicated tlbflush check and
> operation with inline functions. We should use those inline functions to
> avoid the complicated tlbflush check and tlbflush operations when
> implementing TODOs left in commit a902c12ee45fc9389eb8fe54eeddaf267a555c58
> (More efficient TLB-flush filtering in alloc_heap_pages()).
> 
> "#include " is removed from xen/arch/x86/acpi/suspend.c to
> avoid the compiling error after we include "" to
> xen/include/xen/mm.h.
> 
> Signed-off-by: Dongli Zhang 
> ---
> Changed since v4:
>   * Wrap the filtered tlbflush mask operation as inline function (suggested
> by Jan).

That was only one half of my request, the other half was to ...

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -827,10 +827,8 @@ static struct page_info *alloc_heap_pages(
>  BUG_ON(pg[i].count_info != PGC_state_free);
>  pg[i].count_info = PGC_state_inuse;
>  
> -if ( pg[i].u.free.need_tlbflush &&
> - (pg[i].tlbflush_timestamp <= tlbflush_current_time()) &&
> - (!need_tlbflush ||
> -  (pg[i].tlbflush_timestamp > tlbflush_timestamp)) )
> +if ( accumulate_tlbflush(need_tlbflush, &pg[i],
> + tlbflush_timestamp) )
>  {
>  need_tlbflush = 1;
>  tlbflush_timestamp = pg[i].tlbflush_timestamp;

... also move the if() including its body into the helper (whether you
make the new value of need_tlbflush the return value or handle it
via indirection is up to you).

Jan


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

Re: [Xen-devel] [PATCH v5 2/2] xen: move TLB-flush filtering out into populate_physmap during vm creation

[Jan Beulich]

>>> On 19.09.16 at 04:50,  wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1004,6 +1004,14 @@ int domain_unpause_by_systemcontroller(struct domain 
> *d)
>  {
>  int old, new, prev = d->controller_pause_count;
>  
> +/*
> + * We record this information here for populate_physmap to figure out
> + * that the domain has finished being created. In fact, we're only
> + * allowed to set the MEMF_no_tlbflush flag during VM creation.
> + */
> +if ( unlikely(!d->creation_finished) )
> +d->creation_finished = true;

Already on a much earlier version it was pointed out that the
conditional here is rather pointless and potentially confusing.
Please remove it unless you have a very good reason for it to
be there.

> @@ -150,6 +152,17 @@ static void populate_physmap(struct memop_args *a)
>  max_order(curr_d)) )
>  return;
>  
> +/*
> + * With MEMF_no_tlbflush set, alloc_heap_pages() will ignore
> + * TLB-flushes. After VM creation, this is a security issue (it can
> + * make pages accessible to guest B, when guest A may still have a
> + * cached mapping to them). So we only do this only during domain

Duplicate "only".

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -474,6 +474,12 @@ struct domain
>  unsigned int guest_request_enabled   : 1;
>  unsigned int guest_request_sync  : 1;
>  } monitor;
> +
> +/*
> + * Set to true at the very end of domain creation, when the domain is
> + * unpaused for the first time by the systemcontroller.
> + */
> +bool creation_finished;

Please place this next to the other group of booleans, the more that
there is a 1 byte padding slot available there (or even 2 bytes when
!CONFIG_HAS_PASSTHROUGH).

Jan


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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Peng Fan]

Hello Julien,

On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:
>Hello Peng,
>
>On 19/09/2016 04:08, van.free...@gmail.com wrote:
>>From: Peng Fan 
>>
>>This patchset is to support XEN run on big.little SoC.
>>The idea of the patch is from
>>"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";
>>
>>There are some changes to cpupool and add x86 stub functions to avoid build
>>break. Sending The RFC patchset out is to request for comments to see whether
>>this implementation is acceptable or not. Patchset have been tested based on
>>xen-4.8 unstable on NXP i.MX8.
>>
>>I use Big/Little CPU and cpupool to explain the idea.
>>A pool contains Big CPUs is called Big Pool.
>>A pool contains Little CPUs is called Little Pool.
>>If a pool does not contains any physical cpus, Little CPUs or Big CPUs
>>can be added to the cpupool. But the cpupool can not contain both Little
>>and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value 
>>for ARM).
>>CPUs can not be added to the cpupool which contains cpus that have different 
>>cpu type.
>>Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
>>and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
>>When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
>>type(same midr value)
>>into cpupool0.
>
>As mentioned in the mail you pointed above, this series is not enough to make
>big.LITTLE working on then. Xen is always using the boot CPU to detect the
>list of features. With big.LITTLE features may not be the same.
>
>And I would prefer to see Xen supporting big.LITTLE correctly before
>beginning to think to expose big.LITTLE to the userspace (via cpupool)
>automatically.

Do you mean vcpus be scheduled between big and little cpus freely?

This patchset is to use cpupool to block the vcpu be scheduled between big and
little cpus.

>
>See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).

Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
the A53 and A72 TRM about AArch64 implementationd defined registers.
This actlr can be added to the cpupool_arch_info as midr.

Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
Please advise if I missed anything else.

>
>>
>>Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first one
>>that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
>>cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
>>`xl cpupool-list -c` will show cpu[0-3] in Pool-0.
>>
>>Then use the following script to create a new cpupool and add cpu[4-5] to
>>the cpupool.
>> #xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
>> #xl cpupool-cpu-add Pool-A72 4
>> #xl cpupool-cpu-add Pool-A72 5
>> #xl create -d /root/xen/domu-test pool=\"Pool-A72\"
>
>I am a bit confused with these runes. It means that only the first kind of
>CPUs have pool assigned. Why don't you directly create all the pools at boot
>time?

If need to create all the pools, need to decided how many pools need to be 
created.
I thought about this, but I do not come out a good idea.

The cpupool0 is defined in xen/common/cpupool.c, if need to create many pools,
need to alloc cpupools dynamically when booting. I would not like to change a
lot to common code.

The implementation in this patchset I think is an easy way to let Big and Little
CPUs all run.

>
>Also, in which pool a domain will be created if none is specified?
>
>>Now `xl cpupool-list -c` shows:
>>NameCPU list
>>Pool-0  0,1,2,3
>>Pool-A724,5
>>
>>`xl cpupool-list` shows:
>>Name   CPUs   Sched Active   Domain count
>>Pool-0   4credit   y  1
>>Pool-A72 2   credit2   y  1
>>
>>`xl cpupool-cpu-remove Pool-A72 4`, then `xl cpupool-cpu-add Pool-0 4`
>>not success, because Pool-0 contains A53 CPUs, but CPU4 is an A72 CPU.
>>
>>`xl cpupool-migrate DomU Pool-0` will also fail, because DomU is created
>>in Pool-A72 with A72 vcpu, while Pool-0 have A53 physical cpus.
>>
>>Patch 1/5:
>>use "cpumask_weight(cpupool0->cpu_valid);" to replace "num_online_cpus()",
>>because num_online_cpus() counts all the online CPUs, but now we only
>>need Big or Little CPUs.
>
>So if I understand correctly, if the boot CPU is a little CPU, DOM0 will
>always be able to only use little ones. Is that right?

Yeah. Dom0 only use the little ones.

Thanks,
Peng.
>
>Regards,
>
>-- 
>Julien Grall

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

Re: [Xen-devel] [PATCH v6 1/6] livepatch: Disallow applying after an revert

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> On general this is unhealthy - as the payload's .bss (definitly)
> or .data (maybe) will be modified once the payload is running.
> 
> Doing an revert and then re-applying the payload with a non-pristine
> .bss or .data can lead to unforseen consequences (.bss are assumed
> to always contain zero value but now they may have a different value).
> 
> There is one exception - if the payload contains only one .data section
> - the .livepatch.funcs, then it is OK to re-apply an revert.
> 
> Suggested-by: Jan Beulich 
> Signed-off-by: Konrad Rzeszutek Wilk 
> 
> ---
> Cc: Andrew Cooper 
> Cc: Jan Beulich 

I think Ross is actually the primary person to be listed here.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -52,6 +52,8 @@ struct livepatch_build_id {
>  struct payload {
>  uint32_t state;  /* One of the LIVEPATCH_STATE_*. */
>  int32_t rc;  /* 0 or -XEN_EXX. */
> +bool_t reverted; /* Whether it was reverted. */
> +bool_t safe_to_reapply;  /* Can apply safely after revert. */

You nicely use "true" int the code below - please also use plain "bool"
here.

> @@ -381,7 +383,11 @@ static int move_payload(struct payload *payload, struct 
> livepatch_elf *elf)
>  if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
>  buf = text_buf;
>  else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
> +{
>  buf = rw_buf;
> +rw_buf_sec = i;
> +rw_buf_cnt++;

Wouldn't it make sense to exclude zero-size sections here? Perhaps
even worth skipping them together with !SHF_ALLOC ones (and then
of course also in the other earlier loop)? The more that ISTR you
mentioning that empty .data and .bss get created by the assembler
even if there's no respective source contribution?

Jan


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

Re: [Xen-devel] [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections.

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -86,6 +86,10 @@ static int elf_resolve_sections(struct livepatch_elf *elf, 
> const void *data)
>  delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past 
> end");
>  return -EINVAL;
>  }
> +else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
> +  sec[i].sec->sh_type == SHT_NOBITS &&
> +  sec[i].sec->sh_size > BSS_MAX_SIZE )
> +return -EINVAL;
>  
>  sec[i].data = data + delta;
>  /* Name is populated in elf_resolve_section_names. */
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -30,6 +30,8 @@ struct xen_sysctl_livepatch_op;
>  #define ELF_LIVEPATCH_FUNC".livepatch.funcs"
>  #define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
>  #define ELF_BUILD_ID_NOTE  ".note.gnu.build-id"
> +/* Arbitrary limit. */
> +#define BSS_MAX_SIZEMB(2)

Hmm, this wasn't quite what I was thinking about in the v5
comments: I really meant to unify this and the other 2Mb limit
into one (and then obviously with a name that's more generic).
I'm sorry for not having expressed this in an explicit enough
way.

Jan


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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Julien Grall]

Hello,

On 19/09/2016 10:36, Peng Fan wrote:

On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:

Hello Peng,

On 19/09/2016 04:08, van.free...@gmail.com wrote:

From: Peng Fan 

This patchset is to support XEN run on big.little SoC.
The idea of the patch is from
"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";

There are some changes to cpupool and add x86 stub functions to avoid build
break. Sending The RFC patchset out is to request for comments to see whether
this implementation is acceptable or not. Patchset have been tested based on
xen-4.8 unstable on NXP i.MX8.

I use Big/Little CPU and cpupool to explain the idea.
A pool contains Big CPUs is called Big Pool.
A pool contains Little CPUs is called Little Pool.
If a pool does not contains any physical cpus, Little CPUs or Big CPUs
can be added to the cpupool. But the cpupool can not contain both Little
and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value for 
ARM).
CPUs can not be added to the cpupool which contains cpus that have different 
cpu type.
Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
type(same midr value)
into cpupool0.


As mentioned in the mail you pointed above, this series is not enough to make
big.LITTLE working on then. Xen is always using the boot CPU to detect the
list of features. With big.LITTLE features may not be the same.

And I would prefer to see Xen supporting big.LITTLE correctly before
beginning to think to expose big.LITTLE to the userspace (via cpupool)
automatically.


Do you mean vcpus be scheduled between big and little cpus freely?


By supporting big.LITTLE correctly I meant Xen thinks that all the cores 
has the same set of features. So the feature detection is only done the 
boot CPU. See processor_setup for instance...


Moving vCPUs between big and little cores would be a hard task (cache 
line issue, and possibly feature) and I don't expect to ever cross this 
in Xen. However, I am expecting to see big.LITTLE exposed to the guest 
(i.e having big and little vCPUs).




This patchset is to use cpupool to block the vcpu be scheduled between big and
little cpus.



See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).


Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
the A53 and A72 TRM about AArch64 implementationd defined registers.
This actlr can be added to the cpupool_arch_info as midr.

Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
Please advise if I missed anything else.


Have you check the register emulation?







Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first one
that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
`xl cpupool-list -c` will show cpu[0-3] in Pool-0.

Then use the following script to create a new cpupool and add cpu[4-5] to
the cpupool.
#xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
#xl cpupool-cpu-add Pool-A72 4
#xl cpupool-cpu-add Pool-A72 5
#xl create -d /root/xen/domu-test pool=\"Pool-A72\"


I am a bit confused with these runes. It means that only the first kind of
CPUs have pool assigned. Why don't you directly create all the pools at boot
time?


If need to create all the pools, need to decided how many pools need to be 
created.
I thought about this, but I do not come out a good idea.

The cpupool0 is defined in xen/common/cpupool.c, if need to create many pools,
need to alloc cpupools dynamically when booting. I would not like to change a
lot to common code.


Why? We should avoid to choose a specific design just because the common 
code does not allow you to do it without heavy change.


We never came across the big.LITTLE problem on x86, so it is normal to 
modify the code.



The implementation in this patchset I think is an easy way to let Big and Little
CPUs all run.


I care about having a design allowing an easy use of big.LITTLE on Xen. 
Your solution requires the administrator to know the underlying platform 
and create the pool.


In the solution I suggested, the pools would be created by Xen (and the 
info exposed to the userspace for the admin).






Also, in which pool a domain will be created if none is specified?


Now `xl cpupool-list -c` shows:
NameCPU list
Pool-0  0,1,2,3
Pool-A724,5

`xl cpupool-list` shows:
Name   CPUs   Sched Active   Domain count
Pool-0   4credit   y  1
Pool-A72 2   credit2   y  1

`xl cpupool-cpu-remove Pool-A72 4`, then `xl cpupool-cpu-add Pool-0 4`
not success, because Pool-0 contains A53 CPUs, but CPU4 is an A72 CPU.

`xl cpupool-migrate DomU Pool-0` will also fail, because DomU is created
in Pool-A72 with A72 vcpu, wh

[Xen-devel] [xen-unstable test] 101008: tolerable FAIL

[osstest service owner]

flight 101008 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/101008/

Failures :-/ but no regressions.

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-xsm   6 xen-boot fail in 100998 pass in 101008
 test-armhf-armhf-libvirt  4 host-ping-check-native fail pass in 100998

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 15 guest-start/debian.repeat fail in 100998 like 
100992
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 100998
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 100998
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 100998
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 100998
 test-amd64-amd64-xl-rtds  9 debian-install   fail  like 100998

Tests which did not succeed, but are not blocking:
 test-amd64-i386-rumprun-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-rumprun-amd64  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt12 migrate-support-check fail in 100998 never pass
 test-armhf-armhf-libvirt 14 guest-saverestorefail in 100998 never pass
 build-amd64-rumprun   5 rumprun-buildfail   never pass
 build-i386-rumprun5 rumprun-buildfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 xen  6559a686ae77bca2539d826120c9f3bd0d75cdf8
baseline version:
 xen  6559a686ae77bca2539d826120c9f3bd0d75cdf8

Last test of basis   101008  2016-09-19 01:58:31 Z0 days
Testing same since0  1970-01-01 00:00:00 Z 17063 days0 attempts

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>  
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> -/* No NOP patching yet. */
> -if ( !func->new_size )
> +/* If NOPing only do up to maximum amount we can put in the ->opaque. */
> +if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>  return -EOPNOTSUPP;
>  
> -if ( func->old_size < PATCH_INSN_SIZE )
> +if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>  return -EINVAL;

Is that indeed a requirement when NOPing? You can easily NOP out
just a single byte on x86. Or shouldn't in that case old_size == new_size
anyway? In which case the comment further down stating that new_size
can be zero would also be wrong.

> @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct 
> livepatch_func *func)
>  
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> -int32_t val;
>  uint8_t *old_ptr;
> -
> -BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> -BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> +uint8_t insn[sizeof(func->opaque)];
> +unsigned int len;
>  
>  old_ptr = func->old_addr;
> -memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> +len = livepatch_insn_len(func);
> +if ( !len )
> +return;
> +
> +memcpy(func->opaque, old_ptr, len);
> +if ( func->new_addr )
> +{
> +int32_t val;
> +
> +BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> +insn[0] = 0xe9;
> +val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +
> +memcpy(&insn[1], &val, sizeof(val));
> +}
> +else
> +add_nops(&insn, len);
>  
> -*old_ptr++ = 0xe9; /* Relative jump */

Are you btw intentionally getting rid of this comment? And with the
NOP addition here, perhaps worth dropping the _jmp from the
function name (and its revert counterpart)?

> +static inline size_t livepatch_insn_len(const struct livepatch_func *func)

I think it would be nice to use consistent types: The current sole caller
stores the result of the function in an unsigned int, and I see no reason
why the function couldn't also return such.

Jan


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

Re: [Xen-devel] [PATCH v6 6/6] livepatch/tests: Move the .name value to .rodata

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
> Right now the contents of 'name' are all located in
> the .data section. We want them in the .rodata section
> so change the type to have const on them.
> 
> Signed-off-by: Konrad Rzeszutek Wilk 

I've only now noticed that the tests get covered in the LIVEPATCH
section only by patch 8 of the other series, so patch 5 and 6 here:
Acked-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v4 01/16] arm/x86/common: Add HAS_[ALTERNATIVE|EX_TABLE]

[Jan Beulich]

>>> On 16.09.16 at 18:38,  wrote:
> x86 implements all of them by default - and we just
> add two extra HAS_ variables to be declared in autoconf.h.
> 
> ARM 64 only has alternative while ARM 32 has none of them.
> 
> And while at it change the livepatch common code that
> would benefit from this.
> 
> Suggested-by: Julien Grall 
> Signed-off-by: Konrad Rzeszutek Wilk 

Relevant parts
Acked-by: Jan Beulich 


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

Re: [Xen-devel] [PATCH v4 05/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

[Jan Beulich]

>>> On 16.09.16 at 18:38,  wrote:
> If the distance is too great we are in trouble - as our relocation

s/great/big/ (or large), as mentioned before?

> @@ -68,7 +69,7 @@ int arch_livepatch_secure(const void *va, unsigned int 
> pages, enum va_type types
>  void arch_livepatch_init(void);
>  
>  #include  /* For struct livepatch_func. */
> -#include  /* For ARCH_PATCH_INSN_SIZE. */
> +#include  /* For ARCH_[PATCH_INSN_SIZE, LIVEPATCH_RANGE]. */

Perhaps better to drop the comment - it'll get unwieldy to extend it
the same way for the next addition, and it's kind of expected to
include the per-arch header here.

> @@ -78,6 +79,21 @@ static inline size_t livepatch_insn_len(const struct 
> livepatch_func *func)
>  
>  return ARCH_PATCH_INSN_SIZE;
>  }
> +
> +static inline int livepatch_verify_distance(const struct livepatch_func 
> *func)
> +{
> +long offset;
> +long range = (long)ARCH_LIVEPATCH_RANGE;

So you've dropped a few casts, but left this one around. What good
does it do?

Jan


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

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 16.09.16 at 17:29,  wrote:
>  docs/misc/livepatch.markdown  |  7 +--
>  xen/arch/x86/alternative.c|  2 +-
>  xen/arch/x86/livepatch.c  | 40 
> +--
>  xen/common/livepatch.c|  3 ++-
>  xen/include/asm-x86/alternative.h |  1 +
>  xen/include/asm-x86/livepatch.h   | 21 
>  xen/include/xen/livepatch.h   |  9 +
>  7 files changed, 65 insertions(+), 18 deletions(-)
>  create mode 100644 xen/include/asm-x86/livepatch.h

I've noticed only while reviewing the other series that this addition
should imo be accompanied by a change to ./MAINTAINERS.

Jan


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

Re: [Xen-devel] [RFC PATCH] x86/apicv: fix RTC periodic timer and apicv issue

[Tian, Kevin]

> From: Xuquan (Euler) [mailto:xuqu...@huawei.com]
> Sent: Monday, September 12, 2016 5:08 PM
> 
> On September 12, 2016 3:58 PM, Tian, Kevin  wrote:
> >> From: Xuquan (Euler) [mailto:xuqu...@huawei.com]
> >> Sent: Friday, September 09, 2016 11:02 AM
> >>
> >> On August 30, 2016 1:58 PM, Tian Kevin < kevin.t...@intel.com > wrote:
> >> >> From: Xuquan (Euler) [mailto:xuqu...@huawei.com]
> >> >> Sent: Friday, August 19, 2016 8:59 PM
> >> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> >> index 1d5d287..cc247c3 100644
> >> --- a/xen/arch/x86/hvm/vlapic.c
> >> +++ b/xen/arch/x86/hvm/vlapic.c
> >> @@ -433,6 +433,11 @@ void vlapic_EOI_set(struct vlapic *vlapic)  void
> >> vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)  {
> >>  struct domain *d = vlapic_domain(vlapic);
> >> +struct hvm_intack pt_intack;
> >> +
> >> +pt_intack.vector = vector;
> >> +pt_intack.source = hvm_intsrc_lapic;
> >> +pt_intr_post(vlapic_vcpu(vlapic), pt_intack);
> >>
> >>  if ( vlapic_test_and_clear_vector(vector,
> >&vlapic->regs->data[APIC_TMR]) )
> >>  vioapic_update_EOI(d, vector); diff --git
> >> a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c index
> >> 8fca08c..29d9bbf 100644
> >> --- a/xen/arch/x86/hvm/vmx/intr.c
> >> +++ b/xen/arch/x86/hvm/vmx/intr.c
> >> @@ -333,8 +333,6 @@ void vmx_intr_assist(void)
> >>  clear_bit(i, &v->arch.hvm_vmx.eoi_exitmap_changed);
> >>  __vmwrite(EOI_EXIT_BITMAP(i),
> >v->arch.hvm_vmx.eoi_exit_bitmap[i]);
> >>  }
> >> -
> >> -pt_intr_post(v, intack);
> >>  }
> >>  else
> >>  {
> >>
> >
> >Because we update pt irq in every vmentry, there is a chance that
> >already-injected instance (before EOI-induced exit happens) will incur 
> >another
> >pending IRR setting if there is a VM-exit happens between HW virtual 
> >interrupt
> >injection (vIRR->0, vISR->1) and EOI-induced exit (vISR->0), since 
> >pt_intr_post
> >hasn't been invoked yet. I guess this is the reason why you still see faster
> >wallclock.
> >
> 
> Agreed. A good description. My bad description is from another aspect.
> 
> >I think you need mark this pending_intr_post situation explicitly.
> >Then pt_update_irq should skip such pt timer when pending_intr_post of that
> >timer is true (otherwise the update is meaningless since previous one hasn't
> >been posted yet). Then with your change to post in EOI-induced exit handler, 
> >it
> >should work correctly to meet the goal
> >- one virtual interrupt delivery for one pending pt intr...
> >
> I think we are at least on the right track.
> But I can't follow ' pending_intr_post ', a new parameter? Thanks.
> 
> 

yes, a new parameter to record whether a intr_post operation is pending

Thanks
Kevin

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

Re: [Xen-devel] [PATCH v4 01/16] arm/x86/common: Add HAS_[ALTERNATIVE|EX_TABLE]

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

x86 implements all of them by default - and we just
add two extra HAS_ variables to be declared in autoconf.h.

ARM 64 only has alternative while ARM 32 has none of them.

And while at it change the livepatch common code that
would benefit from this.

Suggested-by: Julien Grall 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Doug Goldstein 

v2: First submission
v3: Move the config options to common code
Don't include  in the file.
Don't even include  in the file as xen/Rules.mk automatically
includes the config.h for every GCC invocation.
v4: Depend on "arm64: s/ALTERNATIVE/HAS_ALTERNATIVE/"


I can't find this patch on the ML. Did you forget to send it?

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.

[Jan Beulich]

>>> On 16.09.16 at 18:38,  wrote:
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf 
> *elf,
>  return true;
>  }
>  
> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
> +const struct livepatch_elf_sym *sym)
> +{
> +#ifdef CONFIG_ARM_32
> +/*
> + * Xen does not use Thumb instructions - and we should not see any of
> + * them. If we do, abort.
> + */
> +if ( sym->name && *sym->name == '$' && sym->name[1] == 't' )

I'm not sure here: Are all symbols starting with $t to be rejected, or
only $t but not e.g. $txyz? According to some other code I have
lying around it ought to be "$t" and any symbols starting with "$t.",
and would be in line with patch 6. But I guess the ARM maintainers
will know best.

Jan


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

Re: [Xen-devel] [PATCH] x86/Intel: hide CPUID faulting capability from guests

[Tian, Kevin]

> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Friday, September 16, 2016 2:32 PM
> 
> We don't currently emulate it, so guests should not be misguided to
> believe they can (try to) use it.
> 
> For now, simply return zero to guests for platform MSR reads, and only
> accept (by discarding) writes of zero. If ever there will be bits we
> can safely expose to guests, let's handle them by white listing.
> 
> (As a side note - according to SDM version 059 bit 31 is reserved on
> all known families.)
> 
> Reported-by: Kyle Huey 
> Signed-off-by: Jan Beulich 
> 

Acked-by: Kevin Tian 

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

Re: [Xen-devel] [PATCH] x86/Intel: Broadwell doesn't have PKG_C{8, 9, 10}_RESIDENCY MSRs

[Tian, Kevin]

> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: Monday, September 19, 2016 3:52 PM
> 
> According to
> https://lists.xenproject.org/archives/html/xen-devel/2016-09/msg01797.html
> this partially reverts commit 350bc1a9d4 ("x86: support newer Intel CPU
> models") to account for the appearant earlier mis-documentation.
> 
> Signed-off-by: Jan Beulich 
> 

Acked-by: Kevin Tian 

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

[Xen-devel] [PATCH] docs: correct values for old VMDP unplug

[Olaf Hering]

Fix commit f6d4cf5 ("docs: document old SUSE/Novell unplug for HVM").
The values which VMDP used to control either NIC or disk are flipped.
What the code does is:

 case 8:
if (val == 1 ) {
ide_unplug_harddisks();
} else if (val == 2) {
pci_unplug_netifs();
net_tap_shutdown_all();
}
break;

Signed-off-by: Olaf Hering 
---
 docs/misc/hvm-emulated-unplug.markdown | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/misc/hvm-emulated-unplug.markdown 
b/docs/misc/hvm-emulated-unplug.markdown
index 70fb024..256cea2 100644
--- a/docs/misc/hvm-emulated-unplug.markdown
+++ b/docs/misc/hvm-emulated-unplug.markdown
@@ -89,8 +89,8 @@ Novells VMDP. Depending on how VMDP was configured it would 
control all
 devices, or either NIC or storage. To control all devices the value 0x1
 was written to offset 0x4 in the memory region of the Xen Platform PCI
 Device. This was supposed to unplug NIC, IDE and SCSI devices. If VMDP
-was configured to control just NIC devices it would write the value 0x1
+was configured to control just NIC devices it would write the value 0x2
 to offset 0x8. If VMDP was configured to control just storage devices it
-would write the value 0x2 to offset 0x8. Starting with VMDP version 1.7
+would write the value 0x1 to offset 0x8. Starting with VMDP version 1.7
 (released 2011) the official protocol was used.
 

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

Re: [Xen-devel] [PATCH v4 16/16] livepatch: In xen_nop test-case remove the .bss and .data sections

[Jan Beulich]

>>> On 16.09.16 at 18:38,  wrote:
> As they are not needed for the livepatch to work.

I think this patch can be dropped when skipping zero-size sections,
as suggested earlier.

Jan


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

Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.

[Jan Beulich]

>>> On 16.09.16 at 18:38,  wrote:
> Most of the WARN_ON or BUG_ON sections are properly aligned on
> x86. However on ARM and on x86 assembler the macros don't include
> any aligment information - hence they end up being the default
> byte granularity.
> 
> On ARM32 it is paramount that the aligment is word-size (4)
> otherwise if one tries to use (uint32_t*) access (such
> as livepatch ELF relocations) we get a Data Abort.
> 
> Enforcing bug_frames to have the proper aligment across all
> architectures and in both C and x86 makes them all the same.
> 
> Furthermore on x86 the bloat-o-meter detects that with this
> change:
> 
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> function old new   delta
> 
> On ARM32:
> add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96)
> function old new   delta
> gnttab_unpopulate_status_frames- 384+384
> do_grant_table_op  10808   10520-288
> 
> And ARM64:
> add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72)
> function old new   delta
> gnttab_map_grant_ref   -4164   +4164
> do_grant_table_op   98929836 -56
> grant_map_exists 300   --300
> __gnttab_map_grant_ref  3880   -   -3880
> 
> Signed-off-by: Konrad Rzeszutek Wilk 

x86 parts:
Acked-by: Jan Beulich 


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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Ian Jackson]

osstest service owner writes ("[xtf test] 100874: all pass - PUSHED"):
> flight 100874 xtf real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/100874/
> 
> Perfect :-)

Hooray.  I see this is running in xen-unstable now, as expected.

Just to check, are we expecting `xtf/test-pv64-xsa-167' to SKIP ?

The log says:
   PV superpage support not detected

I assume this is not a missing hardware feature, but rather a missing
build option or something ?

https://wiki.xenproject.org/wiki/Xen_Project_Release_Features suggests
that superpages are "supported" so it would be good to have this test
working.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

The current byte sequence is '0xcc' which makes sense on x86,
but on ARM it is:

stclgt  12, cr12, [ip], {204}   ; 0xcc

Picking something more ARM applicable such as:

efefefefsvc 0x00efefef

Creates a nice crash if one executes that code:
(XEN) CPU1: Unexpected Trap: Supervisor Call

But unfortunatly that may not be a good choice either as in the future


s/unfortunatly/unfortunately/


we may want to implement support for it.

Julien suggested that we use a 4-byte insn instruction instead
of trying to work with one byte.

As such on ARM 32 we use the udf instruction (see A8.8.247
in ARM DDI 0406C.c) and on ARM 64 use the AARCH64_BREAK_FAULT
instruction (aka brk instruction).

We don't have to worry about Thumb code so this instruction
is a safe to execute.

Signed-off-by: Konrad Rzeszutek Wilk 
---
Cc: Julien Grall 
Cc: Stefano Stabellini 

v3: New submission
v4: Instead of using 0xef, use specific insn for architectures.
---
 xen/arch/arm/mm.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 07e2037..438bed7 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -994,8 +994,23 @@ void free_init_memory(void)
 {
 paddr_t pa = virt_to_maddr(__init_begin);
 unsigned long len = __init_end - __init_begin;
+uint32_t insn;
+unsigned int i, nr = len / sizeof(insn);
+
 set_pte_flags_on_range(__init_begin, len, mg_rw);
-memset(__init_begin, 0xcc, len);
+#ifdef CONFIG_ARM_32
+/* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
+insn = 0xe7f000f0;
+#else
+insn = AARCH64_BREAK_FAULT;
+#endif
+for ( i = 0; i < nr; i++ )
+*(__init_begin + i) = insn;


__init_begin is char[], so you will only copy the first byte of the 
instruction.


And because of nr = len / sizeof(insn) only 1/4 of the initmem will be 
poisoned.


So this should be something like:

uint32_t *p = (uint32_t *)__init_begin;
for ( i = 0; i < nr; i++)
   *(p + i) = insn;


+
+nr = len % sizeof(insn);
+if ( nr )
+memset(__init_begin + len - nr, 0xcc, nr);


I am wondering if we should instead align __init_end to 4-byte. With a 
BUILD_BUG_ON in the code to check this assumption.


Any opinions?


+
 set_pte_flags_on_range(__init_begin, len, mg_clear);
 init_domheap_pages(pa, pa + len);
 printk("Freed %ldkB init memory.\n", (long)(__init_end-__init_begin)>>10);



Regards,

--
Julien Grall

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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Peng Fan]

On Mon, Sep 19, 2016 at 10:53:56AM +0200, Julien Grall wrote:
>Hello,
>
>On 19/09/2016 10:36, Peng Fan wrote:
>>On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:
>>>Hello Peng,
>>>
>>>On 19/09/2016 04:08, van.free...@gmail.com wrote:
From: Peng Fan 

This patchset is to support XEN run on big.little SoC.
The idea of the patch is from
"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";

There are some changes to cpupool and add x86 stub functions to avoid build
break. Sending The RFC patchset out is to request for comments to see 
whether
this implementation is acceptable or not. Patchset have been tested based on
xen-4.8 unstable on NXP i.MX8.

I use Big/Little CPU and cpupool to explain the idea.
A pool contains Big CPUs is called Big Pool.
A pool contains Little CPUs is called Little Pool.
If a pool does not contains any physical cpus, Little CPUs or Big CPUs
can be added to the cpupool. But the cpupool can not contain both Little
and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value 
for ARM).
CPUs can not be added to the cpupool which contains cpus that have 
different cpu type.
Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
type(same midr value)
into cpupool0.
>>>
>>>As mentioned in the mail you pointed above, this series is not enough to make
>>>big.LITTLE working on then. Xen is always using the boot CPU to detect the
>>>list of features. With big.LITTLE features may not be the same.
>>>
>>>And I would prefer to see Xen supporting big.LITTLE correctly before
>>>beginning to think to expose big.LITTLE to the userspace (via cpupool)
>>>automatically.
>>
>>Do you mean vcpus be scheduled between big and little cpus freely?
>
>By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
>the same set of features. So the feature detection is only done the boot CPU.
>See processor_setup for instance...
>
>Moving vCPUs between big and little cores would be a hard task (cache line
>issue, and possibly feature) and I don't expect to ever cross this in Xen.
>However, I am expecting to see big.LITTLE exposed to the guest (i.e having
>big and little vCPUs).

big vCPUs scheduled on big Physical CPUs and little vCPUs scheduled on little
physical cpus, right?
If it is, is there is a need to let Xen think all the cores has the same set
of features?

Developing big.little guest support, I am not sure how much efforts needed.
Is this really needed? 

>
>>
>>This patchset is to use cpupool to block the vcpu be scheduled between big and
>>little cpus.
>>
>>>
>>>See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).
>>
>>Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
>>the A53 and A72 TRM about AArch64 implementationd defined registers.
>>This actlr can be added to the cpupool_arch_info as midr.
>>
>>Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
>>Please advise if I missed anything else.
>
>Have you check the register emulation?

Checked midr. Have not checked others.
I think I missed some registers in ctxt_switch_to.

>
>>
>>>

Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first 
one
that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
`xl cpupool-list -c` will show cpu[0-3] in Pool-0.

Then use the following script to create a new cpupool and add cpu[4-5] to
the cpupool.
#xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
#xl cpupool-cpu-add Pool-A72 4
#xl cpupool-cpu-add Pool-A72 5
#xl create -d /root/xen/domu-test pool=\"Pool-A72\"
>>>
>>>I am a bit confused with these runes. It means that only the first kind of
>>>CPUs have pool assigned. Why don't you directly create all the pools at boot
>>>time?
>>
>>If need to create all the pools, need to decided how many pools need to be 
>>created.
>>I thought about this, but I do not come out a good idea.
>>
>>The cpupool0 is defined in xen/common/cpupool.c, if need to create many pools,
>>need to alloc cpupools dynamically when booting. I would not like to change a
>>lot to common code.
>
>Why? We should avoid to choose a specific design just because the common code
>does not allow you to do it without heavy change.
>
>We never came across the big.LITTLE problem on x86, so it is normal to modify
>the code.
>
>>The implementation in this patchset I think is an easy way to let Big and 
>>Little
>>CPUs all run.
>
>I care about having a design allowing an easy use of big.LITTLE on Xen. Your
>solution requires the administrator to know the underlying platform and
>create the pool.

I suppose big.little is

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[George Dunlap]

On Mon, Sep 19, 2016 at 9:53 AM, Julien Grall  wrote:
>>> As mentioned in the mail you pointed above, this series is not enough to
>>> make
>>> big.LITTLE working on then. Xen is always using the boot CPU to detect
>>> the
>>> list of features. With big.LITTLE features may not be the same.
>>>
>>> And I would prefer to see Xen supporting big.LITTLE correctly before
>>> beginning to think to expose big.LITTLE to the userspace (via cpupool)
>>> automatically.
>>
>>
>> Do you mean vcpus be scheduled between big and little cpus freely?
>
>
> By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
> the same set of features. So the feature detection is only done the boot
> CPU. See processor_setup for instance...
>
> Moving vCPUs between big and little cores would be a hard task (cache line
> issue, and possibly feature) and I don't expect to ever cross this in Xen.
> However, I am expecting to see big.LITTLE exposed to the guest (i.e having
> big and little vCPUs).

So it sounds like the big and LITTLE cores are architecturally
different enough that software must be aware of which one it's running
on?

Exposing varying numbers of big and LITTLE vcpus to guests seems like
a sensible approach.  But at the moment cpupools only allow a domain
to be in exactly one pool -- meaning if we use cpupools to control the
big.LITTLE placement, you won't be *able* to have guests with both big
and LITTLE vcpus.

>> If need to create all the pools, need to decided how many pools need to be
>> created.
>> I thought about this, but I do not come out a good idea.
>>
>> The cpupool0 is defined in xen/common/cpupool.c, if need to create many
>> pools,
>> need to alloc cpupools dynamically when booting. I would not like to
>> change a
>> lot to common code.
>
>
> Why? We should avoid to choose a specific design just because the common
> code does not allow you to do it without heavy change.
>
> We never came across the big.LITTLE problem on x86, so it is normal to
> modify the code.

Julien is correct; there's no reason we couldn't have a default
multiple pools on boot.

>> The implementation in this patchset I think is an easy way to let Big and
>> Little
>> CPUs all run.
>
>
> I care about having a design allowing an easy use of big.LITTLE on Xen. Your
> solution requires the administrator to know the underlying platform and
> create the pool.
>
> In the solution I suggested, the pools would be created by Xen (and the info
> exposed to the userspace for the admin).

FWIW another approach could be the one taken by "xl
cpupool-numa-split": you could have "xl cpupool-bigLITTLE-split" or
something that would automatically set up the pools.

But expanding the schedulers to know about different classes of cpus,
and having vcpus specified as running only on specific types of pcpus,
seems like a more flexible approach.

 -George

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

[Xen-devel] [distros-debian-sid test] 67729: trouble: blocked/broken

[Platform Team regression test user]

flight 67729 distros-debian-sid real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/67729/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf   3 host-install(3) broken REGR. vs. 67696
 build-i3863 host-install(3) broken REGR. vs. 67696
 build-amd64   3 host-install(3) broken REGR. vs. 67696
 build-armhf-pvops 3 host-install(3) broken REGR. vs. 67696
 build-amd64-pvops 3 host-install(3) broken REGR. vs. 67696
 build-i386-pvops  3 host-install(3) broken REGR. vs. 67696

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-i386-sid-netboot-pygrub  1 build-check(1) blocked n/a
 test-amd64-amd64-amd64-sid-netboot-pvgrub  1 build-check(1)blocked n/a
 test-amd64-i386-i386-sid-netboot-pvgrub  1 build-check(1)  blocked n/a
 test-armhf-armhf-armhf-sid-netboot-pygrub  1 build-check(1)blocked n/a
 test-amd64-i386-amd64-sid-netboot-pygrub  1 build-check(1) blocked n/a

baseline version:
 flight   67696

jobs:
 build-amd64  broken  
 build-armhf  broken  
 build-i386   broken  
 build-amd64-pvopsbroken  
 build-armhf-pvopsbroken  
 build-i386-pvops broken  
 test-amd64-amd64-amd64-sid-netboot-pvgrubblocked 
 test-amd64-i386-i386-sid-netboot-pvgrub  blocked 
 test-amd64-i386-amd64-sid-netboot-pygrub blocked 
 test-armhf-armhf-armhf-sid-netboot-pygrubblocked 
 test-amd64-amd64-i386-sid-netboot-pygrub blocked 



sg-report-flight on osstest.xs.citrite.net
logs: /home/osstest/logs
images: /home/osstest/images

Logs, config files, etc. are available at
http://osstest.xs.citrite.net/~osstest/testlogs/logs

Test harness code can be found at
http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary


Push not applicable.


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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Julien Grall]

On 19/09/2016 11:38, Peng Fan wrote:

On Mon, Sep 19, 2016 at 10:53:56AM +0200, Julien Grall wrote:

Hello,

On 19/09/2016 10:36, Peng Fan wrote:

On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:

Hello Peng,

On 19/09/2016 04:08, van.free...@gmail.com wrote:

From: Peng Fan 

This patchset is to support XEN run on big.little SoC.
The idea of the patch is from
"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";

There are some changes to cpupool and add x86 stub functions to avoid build
break. Sending The RFC patchset out is to request for comments to see whether
this implementation is acceptable or not. Patchset have been tested based on
xen-4.8 unstable on NXP i.MX8.

I use Big/Little CPU and cpupool to explain the idea.
A pool contains Big CPUs is called Big Pool.
A pool contains Little CPUs is called Little Pool.
If a pool does not contains any physical cpus, Little CPUs or Big CPUs
can be added to the cpupool. But the cpupool can not contain both Little
and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value for 
ARM).
CPUs can not be added to the cpupool which contains cpus that have different 
cpu type.
Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
type(same midr value)
into cpupool0.


As mentioned in the mail you pointed above, this series is not enough to make
big.LITTLE working on then. Xen is always using the boot CPU to detect the
list of features. With big.LITTLE features may not be the same.

And I would prefer to see Xen supporting big.LITTLE correctly before
beginning to think to expose big.LITTLE to the userspace (via cpupool)
automatically.


Do you mean vcpus be scheduled between big and little cpus freely?


By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
the same set of features. So the feature detection is only done the boot CPU.
See processor_setup for instance...

Moving vCPUs between big and little cores would be a hard task (cache line
issue, and possibly feature) and I don't expect to ever cross this in Xen.
However, I am expecting to see big.LITTLE exposed to the guest (i.e having
big and little vCPUs).


big vCPUs scheduled on big Physical CPUs and little vCPUs scheduled on little
physical cpus, right?
If it is, is there is a need to let Xen think all the cores has the same set
of features?


I think you missed my point. The feature registers on big and little 
cores may be different. Currently, Xen is reading the feature registers 
of the CPU boot and wrongly assumes that those features will exists on 
all CPUs. This is not the case and should be fixed before we are getting 
in trouble.




Developing big.little guest support, I am not sure how much efforts needed.
Is this really needed?


This is not necessary at the moment, although I have seen some interest 
about it. Running a guest only on a little core is a nice beginning, but 
a guest may want to take advantage of big.LITTLE (running hungry app on 
big one and little on small one).








This patchset is to use cpupool to block the vcpu be scheduled between big and
little cpus.



See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).


Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
the A53 and A72 TRM about AArch64 implementationd defined registers.
This actlr can be added to the cpupool_arch_info as midr.

Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
Please advise if I missed anything else.


Have you check the register emulation?


Checked midr. Have not checked others.
I think I missed some registers in ctxt_switch_to.









Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first one
that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
`xl cpupool-list -c` will show cpu[0-3] in Pool-0.

Then use the following script to create a new cpupool and add cpu[4-5] to
the cpupool.
#xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
#xl cpupool-cpu-add Pool-A72 4
#xl cpupool-cpu-add Pool-A72 5
#xl create -d /root/xen/domu-test pool=\"Pool-A72\"


I am a bit confused with these runes. It means that only the first kind of
CPUs have pool assigned. Why don't you directly create all the pools at boot
time?


If need to create all the pools, need to decided how many pools need to be 
created.
I thought about this, but I do not come out a good idea.

The cpupool0 is defined in xen/common/cpupool.c, if need to create many pools,
need to alloc cpupools dynamically when booting. I would not like to change a
lot to common code.


Why? We should avoid to choose a specific design just because the common code
does not allow you to do it without heavy change.

We never came across the big.LITTLE probl

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Julien Grall]

Hi George,

On 19/09/2016 11:45, George Dunlap wrote:

On Mon, Sep 19, 2016 at 9:53 AM, Julien Grall  wrote:

As mentioned in the mail you pointed above, this series is not enough to
make
big.LITTLE working on then. Xen is always using the boot CPU to detect
the
list of features. With big.LITTLE features may not be the same.

And I would prefer to see Xen supporting big.LITTLE correctly before
beginning to think to expose big.LITTLE to the userspace (via cpupool)
automatically.



Do you mean vcpus be scheduled between big and little cpus freely?



By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
the same set of features. So the feature detection is only done the boot
CPU. See processor_setup for instance...

Moving vCPUs between big and little cores would be a hard task (cache line
issue, and possibly feature) and I don't expect to ever cross this in Xen.
However, I am expecting to see big.LITTLE exposed to the guest (i.e having
big and little vCPUs).


So it sounds like the big and LITTLE cores are architecturally
different enough that software must be aware of which one it's running
on?


That's correct. Each big and LITTLE cores may have different errata, 
different features...


It has also the advantage to let the guest dealing itself with its own 
power efficiency without introducing a specific Xen interface.




Exposing varying numbers of big and LITTLE vcpus to guests seems like
a sensible approach.  But at the moment cpupools only allow a domain
to be in exactly one pool -- meaning if we use cpupools to control the
big.LITTLE placement, you won't be *able* to have guests with both big
and LITTLE vcpus.


If need to create all the pools, need to decided how many pools need to be
created.
I thought about this, but I do not come out a good idea.

The cpupool0 is defined in xen/common/cpupool.c, if need to create many
pools,
need to alloc cpupools dynamically when booting. I would not like to
change a
lot to common code.



Why? We should avoid to choose a specific design just because the common
code does not allow you to do it without heavy change.

We never came across the big.LITTLE problem on x86, so it is normal to
modify the code.


Julien is correct; there's no reason we couldn't have a default
multiple pools on boot.


The implementation in this patchset I think is an easy way to let Big and
Little
CPUs all run.



I care about having a design allowing an easy use of big.LITTLE on Xen. Your
solution requires the administrator to know the underlying platform and
create the pool.

In the solution I suggested, the pools would be created by Xen (and the info
exposed to the userspace for the admin).


FWIW another approach could be the one taken by "xl
cpupool-numa-split": you could have "xl cpupool-bigLITTLE-split" or
something that would automatically set up the pools.

But expanding the schedulers to know about different classes of cpus,
and having vcpus specified as running only on specific types of pcpus,
seems like a more flexible approach.


So, if I understand correctly, you would not recommend to extend the 
number of CPU pool per domain, correct?


Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 2/5] x86/time: implement tsc as clocksource

[Jan Beulich]

>>> On 14.09.16 at 19:37,  wrote:
> This patch introduces support for using TSC as platform time source
> which is the highest resolution time and most performant to get.
> Though there are also several problems associated with its usage, and
> there isn't a complete (and architecturally defined) guarantee that
> all machines will provide reliable and monotonic TSC in all cases (I
> believe Intel to be the only that can guarantee that?) For this reason
> it's set with less priority when compared to HPET unless adminstrator
> changes "clocksource" boot option to "tsc".

In the following sentence you removed the exclusive mentioning
of HPET, but above you don't. Furthermore I don't think this
sentence is in line with what the patch does: There's no priority
given to it, and it won't be used at all when not requested on the
command line.

> Initializing TSC
> clocksource requires all CPUs up to have the tsc reliability checks
> performed. init_xen_time is called before all CPUs are up, so for
> example we would start with HPET (or ACPI, PIT) at boot time, and
> switch later to TSC. The switch then happens on verify_tsc_reliability
> initcall that is invoked when all CPUs are up. When attempting to
> initialize TSC we also check for time warps and if it has invariant
> TSC. Note that while we deem reliable a CONSTANT_TSC with no deep
> C-states, it might not always be the case, so we're conservative and
> allow TSC to be used as platform timer only with invariant TSC.
> Additionally we check if CPU Hotplug isn't meant to be performed on
> the host which will either be when max vcpus and num_present_cpu are
> the same. This is because a newly hotplugged CPU may not satisfy the
> condition of having all TSCs synchronized - so when having tsc
> clocksource being used we allow offlining CPUs but not onlining any
> ones back. Finally we prevent TSC from being used as clocksource on
> multiple sockets because it isn't guaranteed to be invariant. Further
> relaxing of this last requirement is added in a separate patch, such
> that we allow vendors with such guarantee to use TSC as clocksource.
> In case any of these conditions is not met, we keep the clocksource
> that was previously initialized on init_xen_time.
> 
> Since b64438c7c ("x86/time: use correct (local) time stamp in
> constant-TSC calibration fast path") updates to cpu time use local
> stamps, which means platform timer is only used to seed the initial
> cpu time.  With clocksource=tsc there is no need to be in sync with
> another clocksource, so we reseed the local/master stamps to be values
> of TSC and update the platform time stamps accordingly. Time
> calibration is set to 1sec after we switch to TSC, thus these stamps
> are reseeded to also ensure monotonic returning values right after the
> point we switch to TSC. This is also to avoid the possibility of
> having inconsistent readings in this short period (i.e. until
> calibration fires).

And within this one second, which may cover some of Dom0's
booting up, it is okay to have inconsistencies?

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -475,6 +475,50 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>  }
>  
>  /
> + * PLATFORM TIMER 4: TSC
> + */
> +
> +/*
> + * Called in verify_tsc_reliability() under reliable TSC conditions
> + * thus reusing all the checks already performed there.
> + */
> +static s64 __init init_tsc(struct platform_timesource *pts)
> +{
> +u64 ret = pts->frequency;
> +
> +if ( nr_cpu_ids != num_present_cpus() )
> +{
> +printk(XENLOG_INFO "TSC: CPU Hotplug intended\n");
> +ret = 0;
> +}
> +
> +if ( nr_sockets > 1 )
> +{
> +printk(XENLOG_INFO "TSC: Not invariant across sockets\n");
> +ret = 0;
> +}
> +
> +if ( !ret )
> +printk(XENLOG_INFO "TSC: Not setting it as clocksource\n");

I think this last message is redundant with the former two. But since
I also think that info level is too low for those earlier ones, perhaps
keeping the latter (at info or even debug level) would be okay, once
the other got bumped to warning level.

> +static struct platform_timesource __initdata plt_tsc =
> +{
> +.id = "tsc",
> +.name = "TSC",
> +.read_counter = read_tsc,
> +.counter_bits = 63,

Please add a brief comment explaining why this is not 64.

> @@ -604,7 +667,9 @@ static u64 __init init_platform_timer(void)
>  unsigned int i;
>  s64 rc = -1;
>  
> -if ( opt_clocksource[0] != '\0' )
> +/* clocksource=tsc is initialized via __initcalls (when CPUs are up). */
> +if ( (opt_clocksource[0] != '\0') &&
> + strcmp(opt_clocksource, "tsc") )

No real need to split this if() across two lines.

> +static void __init try_platform_timer_tail(void)
> +{
> +init_timer(&plt_overflow_timer, plt_overflow, NULL, 0);
> +plt_overflow(NULL);
> +
> +platform_timer_stamp = plt_stamp64;
> +stime_platform_stamp = 

Re: [Xen-devel] [PATCH v4 3/5] x86/time: refactor read_platform_stime()

[Jan Beulich]

>>> On 14.09.16 at 19:37,  wrote:
> To allow the caller to fetch the last read from the clocksource which
> was used to calculate system_time. This is a prerequisite for a
> subsequent patch that will use this last read.
> 
> Signed-off-by: Joao Martins 

Acked-by: Jan Beulich 
with one further minor request:

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -581,18 +581,22 @@ static void plt_overflow(void *unused)
>  set_timer(&plt_overflow_timer, NOW() + plt_overflow_period);
>  }
>  
> -static s_time_t read_platform_stime(void)
> +static s_time_t read_platform_stime(u64 *stamp)
>  {
> -u64 count;
> +u64 plt_counter, count;
>  s_time_t stime;
>  
>  ASSERT(!local_irq_is_enabled());
>  
>  spin_lock(&platform_timer_lock);
> -count = plt_stamp64 + ((plt_src.read_counter() - plt_stamp) & plt_mask);
> +plt_counter = plt_src.read_counter();
> +count = plt_stamp64 + ((plt_counter - plt_stamp) & plt_mask);
>  stime = __read_platform_stime(count);
>  spin_unlock(&platform_timer_lock);
>  
> +if ( stamp )
> +*stamp = plt_counter;

Considering that all current callers pass in NULL and you mean to
add only one (iirc) which doesn't, please add unlikely() here.

Jan


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

Re: [Xen-devel] [PATCH net-next RESEND] xen-netfront: avoid packet loss when ethernet header crosses page boundary

[Vitaly Kuznetsov]

David Miller  writes:

> From: Vitaly Kuznetsov 
> Date: Fri, 16 Sep 2016 12:59:14 +0200
>
>> @@ -595,6 +596,19 @@ static int xennet_start_xmit(struct sk_buff *skb, 
>> struct net_device *dev)
>>  offset = offset_in_page(skb->data);
>>  len = skb_headlen(skb);
>>  
>> +/* The first req should be at least ETH_HLEN size or the packet will be
>> + * dropped by netback.
>> + */
>> +if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) {
>> +nskb = skb_copy(skb, GFP_ATOMIC);
>> +if (!nskb)
>> +goto drop;
>> +dev_kfree_skb_any(skb);
>> +skb = nskb;
>> +page = virt_to_page(skb->data);
>> +offset = offset_in_page(skb->data);
>> +}
>> +
>>  spin_lock_irqsave(&queue->tx_lock, flags);
>
> I think you also have to recalculate 'len' in this case too, as
> skb_headlen() will definitely be different for nskb.
>
> In fact, I can't see how this code can work properly without that fix.

Thank you for your feedback David,

in my testing (even when I tried doing skb_copy() for all skbs
unconditionally) skb_headlen(nskb) always equals 'len' so I was under an
impression that both 'skb->len' and 'skb->data_len' remain the same when
we do skb_copy(). However, in case you think there are cases when
headlen changes, I see no problem with re-calculating 'len' as it won't
bring any significant performace penalty compared to the already added
skb_copy().

I'll send 'v2'.

-- 
  Vitaly

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

Re: [Xen-devel] [PATCH v4 4/5] x86/time: implement PVCLOCK_TSC_STABLE_BIT

[Jan Beulich]

>>> On 14.09.16 at 19:37,  wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -951,6 +951,14 @@ static void __update_vcpu_system_time(struct vcpu *v, 
> int force)
>  _u.tsc_timestamp = tsc_stamp;
>  _u.system_time   = t->stamp.local_stime;
>  
> +/*
> + * It's expected that domains cope with this bit changing on every
> + * pvclock read to check whether they can resort solely on this tuple
> + * or if it further requires monotonicity checks with other vcpus.
> + */
> +if ( clocksource_is_tsc() )
> +_u.flags|= XEN_PVCLOCK_TSC_STABLE_BIT;

Stray blanks ahead of the |=.

With that taken care of
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Juergen Gross]

On 19/09/16 12:06, Julien Grall wrote:
> Hi George,
> 
> On 19/09/2016 11:45, George Dunlap wrote:
>> On Mon, Sep 19, 2016 at 9:53 AM, Julien Grall 
>> wrote:
> As mentioned in the mail you pointed above, this series is not
> enough to
> make
> big.LITTLE working on then. Xen is always using the boot CPU to detect
> the
> list of features. With big.LITTLE features may not be the same.
>
> And I would prefer to see Xen supporting big.LITTLE correctly before
> beginning to think to expose big.LITTLE to the userspace (via cpupool)
> automatically.


 Do you mean vcpus be scheduled between big and little cpus freely?
>>>
>>>
>>> By supporting big.LITTLE correctly I meant Xen thinks that all the
>>> cores has
>>> the same set of features. So the feature detection is only done the boot
>>> CPU. See processor_setup for instance...
>>>
>>> Moving vCPUs between big and little cores would be a hard task (cache
>>> line
>>> issue, and possibly feature) and I don't expect to ever cross this in
>>> Xen.
>>> However, I am expecting to see big.LITTLE exposed to the guest (i.e
>>> having
>>> big and little vCPUs).
>>
>> So it sounds like the big and LITTLE cores are architecturally
>> different enough that software must be aware of which one it's running
>> on?
> 
> That's correct. Each big and LITTLE cores may have different errata,
> different features...
> 
> It has also the advantage to let the guest dealing itself with its own
> power efficiency without introducing a specific Xen interface.
> 
>>
>> Exposing varying numbers of big and LITTLE vcpus to guests seems like
>> a sensible approach.  But at the moment cpupools only allow a domain
>> to be in exactly one pool -- meaning if we use cpupools to control the
>> big.LITTLE placement, you won't be *able* to have guests with both big
>> and LITTLE vcpus.
>>
 If need to create all the pools, need to decided how many pools need
 to be
 created.
 I thought about this, but I do not come out a good idea.

 The cpupool0 is defined in xen/common/cpupool.c, if need to create many
 pools,
 need to alloc cpupools dynamically when booting. I would not like to
 change a
 lot to common code.
>>>
>>>
>>> Why? We should avoid to choose a specific design just because the common
>>> code does not allow you to do it without heavy change.
>>>
>>> We never came across the big.LITTLE problem on x86, so it is normal to
>>> modify the code.
>>
>> Julien is correct; there's no reason we couldn't have a default
>> multiple pools on boot.
>>
 The implementation in this patchset I think is an easy way to let
 Big and
 Little
 CPUs all run.
>>>
>>>
>>> I care about having a design allowing an easy use of big.LITTLE on
>>> Xen. Your
>>> solution requires the administrator to know the underlying platform and
>>> create the pool.
>>>
>>> In the solution I suggested, the pools would be created by Xen (and
>>> the info
>>> exposed to the userspace for the admin).
>>
>> FWIW another approach could be the one taken by "xl
>> cpupool-numa-split": you could have "xl cpupool-bigLITTLE-split" or
>> something that would automatically set up the pools.
>>
>> But expanding the schedulers to know about different classes of cpus,
>> and having vcpus specified as running only on specific types of pcpus,
>> seems like a more flexible approach.
> 
> So, if I understand correctly, you would not recommend to extend the
> number of CPU pool per domain, correct?

Before deciding in which direction to go (multiple cpupools, sub-pools,
kind of implicit cpu pinning) I think we should think about the
implications regarding today's interfaces:

- Do we want to be able to use different schedulers for big/little
  (this would mean some cpupool related solution)? I'd prefer to
  have only one scheduler type for each domain. :-)

- What about scheduling parameters like weight and cap? How would
  those apply (answer probably influencing pinning solution).
  Remember that especially the downsides of pinning led to the
  introduction of cpupools.

- Is big.LITTLE to be expected to be combined with NUMA?

- Do we need to support live migration for domains containing both
  types of cpus?


Juergen

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

[Xen-devel] Interested in Participating in Outreachy Round 13

[Veronia Bahaa]

Hello,

My name is Veronia Bahaa. I'm a student at Ain Shams university, Egypt. I
am interested in participating in Outreachy Round 13 in the Xen Hypervisor
Userspace Tools Project. Could you point me to some small tasks I could do
to get familiar with the project?

Thanks!
Veronia
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 04/16] livepatch: Initial ARM64 support.

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 000..49eb69b
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c


[...]


+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+const struct livepatch_elf_sec *base,
+const struct livepatch_elf_sec *rela)
+{


[...]


+
+/* MOVW instruction relocations. */
+case R_AARCH64_MOVW_UABS_G0_NC:
+overflow_check = false;


Can you add a comment here to mention the fall-through?


+
+case R_AARCH64_MOVW_UABS_G0:
+ovf = reloc_insn_movw(RELOC_OP_ABS, dest, val, 0,
+  AARCH64_INSN_IMM_MOVKZ);
+break;
+
+case R_AARCH64_MOVW_UABS_G1_NC:
+overflow_check = false;


Ditto.


+
+case R_AARCH64_MOVW_UABS_G1:
+ovf = reloc_insn_movw(RELOC_OP_ABS, dest, val, 16,
+  AARCH64_INSN_IMM_MOVKZ);
+break;
+
+case R_AARCH64_MOVW_UABS_G2_NC:
+overflow_check = false;


Ditto.


+
+case R_AARCH64_MOVW_UABS_G2:
+ovf = reloc_insn_movw(RELOC_OP_ABS, dest, val, 32,
+  AARCH64_INSN_IMM_MOVKZ);
+break;


[...]


+/* Instructions. */
+case R_AARCH64_ADR_PREL_LO21:
+ovf = reloc_insn_imm(RELOC_OP_PREL, dest, val, 0, 21,
+ AARCH64_INSN_IMM_ADR);
+break;
+
+case R_AARCH64_ADR_PREL_PG_HI21_NC:
+overflow_check = false;


Ditto.


+case R_AARCH64_ADR_PREL_PG_HI21:
+ovf = reloc_insn_imm(RELOC_OP_PAGE, dest, val, 12, 21,
+ AARCH64_INSN_IMM_ADR);
+break;


[...]


diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..b4b4b6c 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c


[...]


 void arch_livepatch_revive(void)
 {
+/*
+ * Nuke the instruction cache. Data cache has been cleaned before in
+ * arch_livepatch_apply_jmp.


I think you forgot to clean text region from the payload. Without that, 
you may receive a crash if you have a separate cache for data and 
instruction.



+ */
+invalidate_icache();
+
+if ( vmap_of_xen_text )
+vunmap(vmap_of_xen_text);
+
+vmap_of_xen_text = NULL;
 }



Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 5/5] x86/time: extend "tsc" param with "stable:socket"

[Jan Beulich]

>>> On 14.09.16 at 19:37,  wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -270,7 +270,9 @@ If set, override Xen's default choice for the platform 
> timer.
>  Having TSC as platform timer requires being explicitly set. This is because
>  TSC can only be safely used if CPU hotplug isn't performed on the system. In
>  some platforms, "maxcpus" parameter may require further adjustment to the
> -number of online cpus.
> +number of online cpus. When running under platforms that can guarantee a

... running on platforms ...

> +monotonic TSC across sockets you require adjusting "tsc" command line 
> parameter

... you may want to adjust the ...

> +parameter to "stable:sockets".

Redundant "parameter" (I guess the one on the earlier line was meant
to get moved due to line length). Also you say "sockets" here but ...

> @@ -1508,7 +1510,7 @@ pages) must also be specified via the tbuf\_size 
> parameter.
>  > `= `
>  
>  ### tsc
> -> `= unstable | skewed`
> +> `= unstable | skewed | stable:socket`

"socket" here - the two really should match.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -477,6 +477,10 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>  /
>   * PLATFORM TIMER 4: TSC
>   */
> +static unsigned int __read_mostly tsc_flags;

__initdata instead of __read_mostly

Jan


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

Re: [Xen-devel] [PATCH net-next RESEND] xen-netfront: avoid packet loss when ethernet header crosses page boundary

[David Vrabel]

On 19/09/16 11:22, Vitaly Kuznetsov wrote:
> David Miller  writes:
> 
>> From: Vitaly Kuznetsov 
>> Date: Fri, 16 Sep 2016 12:59:14 +0200
>>
>>> @@ -595,6 +596,19 @@ static int xennet_start_xmit(struct sk_buff *skb, 
>>> struct net_device *dev)
>>> offset = offset_in_page(skb->data);
>>> len = skb_headlen(skb);
>>>  
>>> +   /* The first req should be at least ETH_HLEN size or the packet will be
>>> +* dropped by netback.
>>> +*/
>>> +   if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) {
>>> +   nskb = skb_copy(skb, GFP_ATOMIC);
>>> +   if (!nskb)
>>> +   goto drop;
>>> +   dev_kfree_skb_any(skb);
>>> +   skb = nskb;
>>> +   page = virt_to_page(skb->data);
>>> +   offset = offset_in_page(skb->data);
>>> +   }
>>> +
>>> spin_lock_irqsave(&queue->tx_lock, flags);
>>
>> I think you also have to recalculate 'len' in this case too, as
>> skb_headlen() will definitely be different for nskb.
>>
>> In fact, I can't see how this code can work properly without that fix.
> 
> Thank you for your feedback David,
> 
> in my testing (even when I tried doing skb_copy() for all skbs
> unconditionally) skb_headlen(nskb) always equals 'len' so I was under an
> impression that both 'skb->len' and 'skb->data_len' remain the same when
> we do skb_copy(). However, in case you think there are cases when
> headlen changes, I see no problem with re-calculating 'len' as it won't
> bring any significant performace penalty compared to the already added
> skb_copy().

I think you can move the len = skb_headlen(skb) after the if, no need to
recalculate it.

David

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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[George Dunlap]

On 19/09/16 11:06, Julien Grall wrote:
> Hi George,
> 
> On 19/09/2016 11:45, George Dunlap wrote:
>> On Mon, Sep 19, 2016 at 9:53 AM, Julien Grall 
>> wrote:
> As mentioned in the mail you pointed above, this series is not
> enough to
> make
> big.LITTLE working on then. Xen is always using the boot CPU to detect
> the
> list of features. With big.LITTLE features may not be the same.
>
> And I would prefer to see Xen supporting big.LITTLE correctly before
> beginning to think to expose big.LITTLE to the userspace (via cpupool)
> automatically.


 Do you mean vcpus be scheduled between big and little cpus freely?
>>>
>>>
>>> By supporting big.LITTLE correctly I meant Xen thinks that all the
>>> cores has
>>> the same set of features. So the feature detection is only done the boot
>>> CPU. See processor_setup for instance...
>>>
>>> Moving vCPUs between big and little cores would be a hard task (cache
>>> line
>>> issue, and possibly feature) and I don't expect to ever cross this in
>>> Xen.
>>> However, I am expecting to see big.LITTLE exposed to the guest (i.e
>>> having
>>> big and little vCPUs).
>>
>> So it sounds like the big and LITTLE cores are architecturally
>> different enough that software must be aware of which one it's running
>> on?
> 
> That's correct. Each big and LITTLE cores may have different errata,
> different features...
> 
> It has also the advantage to let the guest dealing itself with its own
> power efficiency without introducing a specific Xen interface.

Well in theory there would be advantages either way -- either to
allowing Xen to automatically add power-saving "smarts" to guests which
weren't programmed for them, or to exposing the power-saving abilities
to guests which were.  But it sounds like automatically migrating
between them isn't really an option (or would be a lot more trouble than
it's worth).

>>> I care about having a design allowing an easy use of big.LITTLE on
>>> Xen. Your
>>> solution requires the administrator to know the underlying platform and
>>> create the pool.
>>>
>>> In the solution I suggested, the pools would be created by Xen (and
>>> the info
>>> exposed to the userspace for the admin).
>>
>> FWIW another approach could be the one taken by "xl
>> cpupool-numa-split": you could have "xl cpupool-bigLITTLE-split" or
>> something that would automatically set up the pools.
>>
>> But expanding the schedulers to know about different classes of cpus,
>> and having vcpus specified as running only on specific types of pcpus,
>> seems like a more flexible approach.
> 
> So, if I understand correctly, you would not recommend to extend the
> number of CPU pool per domain, correct?

Well imagine trying to set the scheduling parameters, such as weight,
which in the past have been per-domain.  Now you have to specify
parameters for a domain in each of the cpupools that its' in.

No, I think it would be a lot simpler to just teach the scheduler about
different classes of cpus.  credit1 would probably need to be modified
so that its credit algorithm would be per-class rather than pool-wide;
but credit2 shouldn't need much modification at all, other than to make
sure that a given runqueue doesn't include more than one class; and to
do load-balancing only with runqueues of the same class.

 -George

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

[Xen-devel] [PATCH net-next v2] xen-netfront: avoid packet loss when ethernet header crosses page boundary

[Vitaly Kuznetsov]

Small packet loss is reported on complex multi host network configurations
including tunnels, NAT, ... My investigation led me to the following check
in netback which drops packets:

if (unlikely(txreq.size < ETH_HLEN)) {
netdev_err(queue->vif->dev,
   "Bad packet size: %d\n", txreq.size);
xenvif_tx_err(queue, &txreq, extra_count, idx);
break;
}

But this check itself is legitimate. SKBs consist of a linear part (which
has to have the ethernet header) and (optionally) a number of frags.
Netfront transmits the head of the linear part up to the page boundary
as the first request and all the rest becomes frags so when we're
reconstructing the SKB in netback we can't distinguish between original
frags and the 'tail' of the linear part. The first SKB needs to be at
least ETH_HLEN size. So in case we have an SKB with its linear part
starting too close to the page boundary the packet is lost.

I see two ways to fix the issue:
- Change the 'wire' protocol between netfront and netback to start keeping
  the original SKB structure. We'll have to add a flag indicating the fact
  that the particular request is a part of the original linear part and not
  a frag. We'll need to know the length of the linear part to pre-allocate
  memory.
- Avoid transmitting SKBs with linear parts starting too close to the page
  boundary. That seems preferable short-term and shouldn't bring
  significant performance degradation as such packets are rare. That's what
  this patch is trying to achieve with skb_copy().

Signed-off-by: Vitaly Kuznetsov 
Acked-by: David Vrabel 
---
- Changes since 'v1':
  Recalculate 'len' as in some cases it changes after skb_copy()
  [David Miller]
  I keep David's ACK on my patch as I think the change doesn't
  significantly change the patch, sorry in advance if I'm wrong.
---
 drivers/net/xen-netfront.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 96ccd4e..043dd04 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -565,6 +565,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
struct netfront_queue *queue = NULL;
unsigned int num_queues = dev->real_num_tx_queues;
u16 queue_index;
+   struct sk_buff *nskb;
 
/* Drop the packet if no queues are set up */
if (num_queues < 1)
@@ -595,6 +596,20 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
offset = offset_in_page(skb->data);
len = skb_headlen(skb);
 
+   /* The first req should be at least ETH_HLEN size or the packet will be
+* dropped by netback.
+*/
+   if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) {
+   nskb = skb_copy(skb, GFP_ATOMIC);
+   if (!nskb)
+   goto drop;
+   dev_kfree_skb_any(skb);
+   skb = nskb;
+   page = virt_to_page(skb->data);
+   offset = offset_in_page(skb->data);
+   len = skb_headlen(skb);
+   }
+
spin_lock_irqsave(&queue->tx_lock, flags);
 
if (unlikely(!netif_carrier_ok(dev) ||
-- 
2.7.4


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

Re: [Xen-devel] [PATCH net-next RESEND] xen-netfront: avoid packet loss when ethernet header crosses page boundary

[Vitaly Kuznetsov]

David Vrabel  writes:

> On 19/09/16 11:22, Vitaly Kuznetsov wrote:
>> David Miller  writes:
>> 
>>> From: Vitaly Kuznetsov 
>>> Date: Fri, 16 Sep 2016 12:59:14 +0200
>>>
 @@ -595,6 +596,19 @@ static int xennet_start_xmit(struct sk_buff *skb, 
 struct net_device *dev)
offset = offset_in_page(skb->data);
len = skb_headlen(skb);
  
 +  /* The first req should be at least ETH_HLEN size or the packet will be
 +   * dropped by netback.
 +   */
 +  if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) {
 +  nskb = skb_copy(skb, GFP_ATOMIC);
 +  if (!nskb)
 +  goto drop;
 +  dev_kfree_skb_any(skb);
 +  skb = nskb;
 +  page = virt_to_page(skb->data);
 +  offset = offset_in_page(skb->data);
 +  }
 +
spin_lock_irqsave(&queue->tx_lock, flags);
>>>
>>> I think you also have to recalculate 'len' in this case too, as
>>> skb_headlen() will definitely be different for nskb.
>>>
>>> In fact, I can't see how this code can work properly without that fix.
>> 
>> Thank you for your feedback David,
>> 
>> in my testing (even when I tried doing skb_copy() for all skbs
>> unconditionally) skb_headlen(nskb) always equals 'len' so I was under an
>> impression that both 'skb->len' and 'skb->data_len' remain the same when
>> we do skb_copy(). However, in case you think there are cases when
>> headlen changes, I see no problem with re-calculating 'len' as it won't
>> bring any significant performace penalty compared to the already added
>> skb_copy().
>
> I think you can move the len = skb_headlen(skb) after the if, no need to
> recalculate it.

Sorry, I was too fast with sending 'v2' and did the other way
around. I'll do v3.

-- 
  Vitaly

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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Wei Liu]

On Mon, Sep 19, 2016 at 10:37:30AM +0100, Ian Jackson wrote:
> osstest service owner writes ("[xtf test] 100874: all pass - PUSHED"):
> > flight 100874 xtf real [real]
> > http://logs.test-lab.xenproject.org/osstest/logs/100874/
> > 
> > Perfect :-)
> 
> Hooray.  I see this is running in xen-unstable now, as expected.
> 
> Just to check, are we expecting `xtf/test-pv64-xsa-167' to SKIP ?
> 

Yes.

> The log says:
>PV superpage support not detected
> 
> I assume this is not a missing hardware feature, but rather a missing
> build option or something ?
> 
> https://wiki.xenproject.org/wiki/Xen_Project_Release_Features suggests
> that superpages are "supported" so it would be good to have this test
> working.
> 

PV superpage is different from hvm superpage. Also that wiki page could
be referring to hv support, I'm not sure.

As far as I can tell, PV superpage is a feature that nobody uses except
for Oracle (?).

Wei.

> Thanks,
> Ian.

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

Re: [Xen-devel] edk2 compile error

[Wei Liu]

Hi all

In order to make things clearer: this seems to be a problem in upstream
EDK2. Xen Project only tracks upstream, no additional patch is added on
top.

So, I deleted the irrelevant bits below.

On Sun, Sep 18, 2016 at 03:38:19AM +, Chen, Farrah wrote:

> I also tried:
> 
> git clone https://github.com/tianocore/edk2.git
> 
> cd edk2
> 
> OvmfPkg/build.sh -a X64 -b DEBUG -n 4
> The same error occurred.
> .
> 
> log:
> ..
> Running edk2 build for OvmfPkgX64
> ..
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:173:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:175:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:177:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:179:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:313:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:315:
>  error: invalid combination of opcode and operands
> make[7]: Leaving directory 
> `/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib'
> make[7]: *** 
> [/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.obj]
>  Error 1
> 
> 
> build.py...
> : error 7000: Failed to execute command
> make tbuild 
> [/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib]
> 
> 
> build.py...
> : error F002: Failed to build module
> 
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>  [X64, GCC44, DEBUG]
> 
> - Failed -
> 
> 
> Thanks,
> Fan Chen
> 
> 

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

[Xen-devel] [PATCH net-next v3] xen-netfront: avoid packet loss when ethernet header crosses page boundary

[Vitaly Kuznetsov]

Small packet loss is reported on complex multi host network configurations
including tunnels, NAT, ... My investigation led me to the following check
in netback which drops packets:

if (unlikely(txreq.size < ETH_HLEN)) {
netdev_err(queue->vif->dev,
   "Bad packet size: %d\n", txreq.size);
xenvif_tx_err(queue, &txreq, extra_count, idx);
break;
}

But this check itself is legitimate. SKBs consist of a linear part (which
has to have the ethernet header) and (optionally) a number of frags.
Netfront transmits the head of the linear part up to the page boundary
as the first request and all the rest becomes frags so when we're
reconstructing the SKB in netback we can't distinguish between original
frags and the 'tail' of the linear part. The first SKB needs to be at
least ETH_HLEN size. So in case we have an SKB with its linear part
starting too close to the page boundary the packet is lost.

I see two ways to fix the issue:
- Change the 'wire' protocol between netfront and netback to start keeping
  the original SKB structure. We'll have to add a flag indicating the fact
  that the particular request is a part of the original linear part and not
  a frag. We'll need to know the length of the linear part to pre-allocate
  memory.
- Avoid transmitting SKBs with linear parts starting too close to the page
  boundary. That seems preferable short-term and shouldn't bring
  significant performance degradation as such packets are rare. That's what
  this patch is trying to achieve with skb_copy().

Signed-off-by: Vitaly Kuznetsov 
Acked-by: David Vrabel 
---
- Changes since 'v2':
  Move 'len' calculation after the 'if' instead of re-calculating it
  inside. [David Vrabel]

- Changes since 'v1':
  Recalculate 'len' as in some cases it changes after skb_copy()
  [David Miller]
  I keep David's ACK on my patch as I think the change doesn't
  significantly change the patch, sorry in advance if I'm wrong.
---
 drivers/net/xen-netfront.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 96ccd4e..e17879d 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -565,6 +565,7 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
struct netfront_queue *queue = NULL;
unsigned int num_queues = dev->real_num_tx_queues;
u16 queue_index;
+   struct sk_buff *nskb;
 
/* Drop the packet if no queues are set up */
if (num_queues < 1)
@@ -593,6 +594,20 @@ static int xennet_start_xmit(struct sk_buff *skb, struct 
net_device *dev)
 
page = virt_to_page(skb->data);
offset = offset_in_page(skb->data);
+
+   /* The first req should be at least ETH_HLEN size or the packet will be
+* dropped by netback.
+*/
+   if (unlikely(PAGE_SIZE - offset < ETH_HLEN)) {
+   nskb = skb_copy(skb, GFP_ATOMIC);
+   if (!nskb)
+   goto drop;
+   dev_kfree_skb_any(skb);
+   skb = nskb;
+   page = virt_to_page(skb->data);
+   offset = offset_in_page(skb->data);
+   }
+
len = skb_headlen(skb);
 
spin_lock_irqsave(&queue->tx_lock, flags);
-- 
2.7.4


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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Jan Beulich]

>>> On 19.09.16 at 12:42,  wrote:
> On Mon, Sep 19, 2016 at 10:37:30AM +0100, Ian Jackson wrote:
>> The log says:
>>PV superpage support not detected
>> 
>> I assume this is not a missing hardware feature, but rather a missing
>> build option or something ?
>> 
>> https://wiki.xenproject.org/wiki/Xen_Project_Release_Features suggests
>> that superpages are "supported" so it would be good to have this test
>> working.
>> 
> 
> PV superpage is different from hvm superpage. Also that wiki page could
> be referring to hv support, I'm not sure.

I guess the respective command line option ("allowsuperpage") was
not given?

> As far as I can tell, PV superpage is a feature that nobody uses except
> for Oracle (?).

And even they half proposed removing the support, but that
proposal was never really brought into full life. Konrad?

Jan


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

Re: [Xen-devel] [edk2] edk2 compile error

[Laszlo Ersek]

On 09/18/16 05:38, Chen, Farrah wrote:
> Hi,
> 
> When I compile xen with the latest commit in RHEL 6.7, it failed when make 
> tools. Errors showed when running edk2 build for OvmfPkgX64.
> Bisected and this error occurred from commit 
> 8c8b6fb02342f7aa78e611a5f0f63dcf8fbf48f2.
> 
> commit 8c8b6fb02342f7aa78e611a5f0f63dcf8fbf48f2
> Author: Wei Liu mailto:wei.l...@citrix.com>>
> Date:   Tue Sep 6 12:54:47 2016 +0100
> 
> Config.mk: update OVMF commit
> 
> Signed-off-by: Wei Liu wei.l...@citrix.com
> 
> 
> We have updated OVMF to the latest master and cleaned everything before 
> rebuilding.
> 
> 
> 
> Steps:
> 
> make clean
> 
> make xen -j8
> 
> ./configure --enable-ovmf
> 
> make tools -j8
> 
> Then the error occurred.
> 
> 
> 
> 
> 
> I also tried:
> 
> git clone https://github.com/tianocore/edk2.git
> 
> cd edk2
> 
> OvmfPkg/build.sh -a X64 -b DEBUG -n 4
> The same error occurred.
> .
> 
> log:
> ..
> Running edk2 build for OvmfPkgX64
> ..
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:173:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:175:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:177:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:179:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:313:
>  error: invalid combination of opcode and operands
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.iii:315:
>  error: invalid combination of opcode and operands
> make[7]: Leaving directory 
> `/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib'
> make[7]: *** 
> [/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib/OUTPUT/X64/ExceptionHandlerAsm.obj]
>  Error 1
> 
> 
> build.py...
> : error 7000: Failed to execute command
> make tbuild 
> [/home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/Build/OvmfX64/DEBUG_GCC44/X64/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib]
> 
> 
> build.py...
> : error F002: Failed to build module
> 
> /home/www/builds_xen_unstable/xen-src-8c8b6fb0-20160912/tools/firmware/ovmf-dir-remote/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>  [X64, GCC44, DEBUG]
> 
> - Failed -

RHEL-6 does not have a nasm version that is recent enough to build edk2.
RHEL-6 ships nasm-2.07-*, but edk2 requires nasm-2.10 or later with the
GCC toolchain family.

Please see this mailing list thread:
https://www.mail-archive.com/edk2-devel@lists.01.org/msg14420.html

And the resultant docs commit:
https://github.com/tianocore/edk2/commit/9c4dbdff1d56

... Before anyone suggests otherwise, this was not a gratuitous version
requirement bump. The edk2 assembly code had already been there, the
nasm version bump only documented the status, after the fact.

For RHEL-6 specifically, I suggest to grab a recent enough nasm SRPM
from Fedora Koji, and rebuild / install that locally. Better yet, I
recommend upgrading to RHEL-7, whose nasm is good enough.

Thanks
Laszlo


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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Wei Liu]

On Mon, Sep 19, 2016 at 04:56:01AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 12:42,  wrote:
> > On Mon, Sep 19, 2016 at 10:37:30AM +0100, Ian Jackson wrote:
> >> The log says:
> >>PV superpage support not detected
> >> 
> >> I assume this is not a missing hardware feature, but rather a missing
> >> build option or something ?
> >> 
> >> https://wiki.xenproject.org/wiki/Xen_Project_Release_Features suggests
> >> that superpages are "supported" so it would be good to have this test
> >> working.
> >> 
> > 
> > PV superpage is different from hvm superpage. Also that wiki page could
> > be referring to hv support, I'm not sure.
> 
> I guess the respective command line option ("allowsuperpage") was
> not given?
> 

Correct, that option is not given in osstest.

Wei.

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

Re: [Xen-devel] [PATCH v6 01/15] x86: properly calculate ELF end of image address

[Jan Beulich]

>>> On 16.09.16 at 22:43,  wrote:
> On Mon, Sep 12, 2016 at 10:18:16PM +0200, Daniel Kiper wrote:
>> Currently ELF end of image address is calculated using first line from
>> "nm -nr xen/xen-syms" output. However, today usually it contains random
>> symbol address not related to end of image in any way. So, it looks
> 
> Weird. The -n says:
> 
> "  -n
>-v
>--numeric-sort
>Sort symbols numerically by their addresses, rather than 
> alphabetically by their names.
> "
> 
> And you are right. It is ignoring it:
> 
> [konrad@char xen]$ nm -nr xen/xen-syms| sort | head -1
> 82d08000 T __image_base__
> [konrad@char xen]$ nm -nr xen/xen-syms | head -1
> 82d08033d000 B efi

So what is it ignoring, you think? The -n? Surely not. Are you perhaps
overlooking that -r means "reverse" (and hence that piping through
"sort" inverts all the sorting done by nm)?

> [konrad@char xen]$ nm -nr xen/xen-syms| sort | tail -5
> 82d08033cb00 B _end
> 82d08033cb00 B __per_cpu_data_end
> 82d08033d000 B __2M_rwdata_end
> 82d08033d000 B efi
>  U _GLOBAL_OFFSET_TABLE_
> 
>> that for years that stuff have been working just by lucky coincidence.
>> Hence, it have to be changed to something more reliable. So, let's take
>> ELF end of image address by reading _end symbol address from nm output.

So before taking this patch I'd really like to see proof that what gets
done currently does actually go wrong in at least one case. So far I
can't see things working as "just by lucky coincidence". In particular
I can't see why __2M_rwdata_end (aliased to efi) does not relate to
the intended image end. Once we switch back to using large pages
even when not using xen.efi I'd even question whether preferring
_end over __2M_rwdata_end is actually correct.

The only real (but reasonably slim) risk we have right now is that an
absolute symbol might appear with a value larger than
__2M_rwdata_end. nm would certainly benefit from an option
allowing to filter out absolute symbols (just like one can filter out
undefined ones).

Jan


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

Re: [Xen-devel] [PATCH] docs: correct values for old VMDP unplug

[Wei Liu]

On Mon, Sep 19, 2016 at 09:29:46AM +, Olaf Hering wrote:
> Fix commit f6d4cf5 ("docs: document old SUSE/Novell unplug for HVM").
> The values which VMDP used to control either NIC or disk are flipped.
> What the code does is:
> 
>  case 8:
> if (val == 1 ) {
> ide_unplug_harddisks();
> } else if (val == 2) {
> pci_unplug_netifs();
> net_tap_shutdown_all();
> }
> break;
> 
> Signed-off-by: Olaf Hering 

Acked-by: Wei Liu 

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

Re: [Xen-devel] [PATCH v6 01/15] x86: properly calculate ELF end of image address

[Daniel Kiper]

On Fri, Sep 16, 2016 at 04:43:21PM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Sep 12, 2016 at 10:18:16PM +0200, Daniel Kiper wrote:
> > Currently ELF end of image address is calculated using first line from
> > "nm -nr xen/xen-syms" output. However, today usually it contains random
> > symbol address not related to end of image in any way. So, it looks
>
> Weird. The -n says:
>
> "  -n
>-v
>--numeric-sort
>Sort symbols numerically by their addresses, rather than 
> alphabetically by their names.
> "
>
> And you are right. It is ignoring it:
>
> [konrad@char xen]$ nm -nr xen/xen-syms| sort | head -1
> 82d08000 T __image_base__
> [konrad@char xen]$ nm -nr xen/xen-syms | head -1
> 82d08033d000 B efi
>
> [konrad@char xen]$ nm -nr xen/xen-syms| sort | tail -5
> 82d08033cb00 B _end
> 82d08033cb00 B __per_cpu_data_end
> 82d08033d000 B __2M_rwdata_end
> 82d08033d000 B efi
>  U _GLOBAL_OFFSET_TABLE_

Well, TBH, I have never checked what "-nr" means. However, it looks
that it works at least with nm from binutils 2.22. Please look below:

82d080345000 A efi
82d080345000 A __2M_rwdata_end
82d080344b80 A _end
82d080344b80 B __per_cpu_data_end
82d080344b80 B __bss_end
82d080344b28 b per_cpu__vmxon_region
82d080344b20 b per_cpu__root_vmcb
82d080344b18 b per_cpu__hsa

[...]

Anyway, I think that we should apply my fix. Though I can agree that we
do not need "-nr" for nm here any more.

> > that for years that stuff have been working just by lucky coincidence.
> > Hence, it have to be changed to something more reliable. So, let's take
> > ELF end of image address by reading _end symbol address from nm output.
> >
> > Signed-off-by: Daniel Kiper 
> > ---
> >  xen/arch/x86/Makefile |2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index d3875c5..a4fe740 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -91,7 +91,7 @@ endif
> >
> >  $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32
> > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x10 \
> > -   `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'`
> > +   `$(NM) -nr $(TARGET)-syms | awk '$$3 == "_end" {print "0x"$$1}'`
> >
>
> Something is off with your tabs/spaces.

I think that it is OK. I added second tab to mark that it is a continuation.

> I would also modify the arch/x86/xen.lds.S and put a comment
> around _end = .; to mention this dependency - in case somebody adds some
> extra things after _end.

I am not sure it is needed. However, if Andrew and Jan do not object I can add 
that.

Daniel

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

Re: [Xen-devel] [PATCH v6 02/15] x86/boot/reloc: create generic alloc and copy functions

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> -multiboot_info_t __stdcall *reloc(multiboot_info_t *mbi_old, u32 trampoline)
> +multiboot_info_t __stdcall *reloc(u32 mbi_old, u32 trampoline)
>  {
>  multiboot_info_t *mbi;
>  int i;
>  
>  alloc = trampoline;
>  
> -mbi = reloc_mbi_struct(mbi_old, sizeof(*mbi));
> +mbi = _p(copy_mem(mbi_old, sizeof(*mbi)));
>  
>  if ( mbi->flags & MBI_CMDLINE )
> -mbi->cmdline = (u32)reloc_mbi_string((char *)mbi->cmdline);
> +mbi->cmdline = copy_string(mbi->cmdline);
>  
>  if ( mbi->flags & MBI_MODULES )
>  {
> -module_t *mods = reloc_mbi_struct(
> -(module_t *)mbi->mods_addr, mbi->mods_count * sizeof(module_t));
> +module_t *mods;
>  
> -mbi->mods_addr = (u32)mods;
> +mbi->mods_addr = copy_mem(mbi->mods_addr, mbi->mods_count * 
> sizeof(module_t));

With this long line suitably wrapped,
Reviewed-by: Jan Beulich 


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

Re: [Xen-devel] [PATCH] docs: correct values for old VMDP unplug

[Wei Liu]

On Mon, Sep 19, 2016 at 12:16:06PM +0100, Wei Liu wrote:
> On Mon, Sep 19, 2016 at 09:29:46AM +, Olaf Hering wrote:
> > Fix commit f6d4cf5 ("docs: document old SUSE/Novell unplug for HVM").
> > The values which VMDP used to control either NIC or disk are flipped.
> > What the code does is:
> > 
> >  case 8:
> > if (val == 1 ) {
> > ide_unplug_harddisks();
> > } else if (val == 2) {
> > pci_unplug_netifs();
> > net_tap_shutdown_all();
> > }
> > break;
> > 
> > Signed-off-by: Olaf Hering 
> 
> Acked-by: Wei Liu 

Pushed.

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

[Xen-devel] [xen-unstable-smoke test] 101009: tolerable all pass - PUSHED

[osstest service owner]

flight 101009 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/101009/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  6bd621d7010bb8561196aedf5198d5fe7c146822
baseline version:
 xen  6559a686ae77bca2539d826120c9f3bd0d75cdf8

Last test of basis   100991  2016-09-16 16:02:30 Z2 days
Testing same since   101009  2016-09-19 10:01:51 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  George Dunlap 
  Jan Beulich 
  Kevin Tian 
  Razvan Cojocaru 
  Tamas K Lengyel 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=6bd621d7010bb8561196aedf5198d5fe7c146822
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
6bd621d7010bb8561196aedf5198d5fe7c146822
+ branch=xen-unstable-smoke
+ revision=6bd621d7010bb8561196aedf5198d5fe7c146822
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' x6bd621d7010bb8561196aedf5198d5fe7c146822 = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/xtf.git
++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git
++ : git://xenbits.xen.org/xtf.git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git
++ : git://git.seabios.org/seabios.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git
++ : git://xenbits.xen.org/osstest/seabios.git
++ : https://github.com/tianocore/edk2.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/linux-firmware.git
++ : osst...@xenbits.xen.org:/home/osste

Re: [Xen-devel] [PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON

[Ian Jackson]

Wei Liu writes ("[PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON"):
> The implementation is taken from libxc.
...
> I could have put it in a header file accessible to all libraries under
> libs but this construct is only relevant to xengnttab library at the
> moment so it's put under gnttab/private.h. It can be easily moved to
> a common place when other libraries under libs require it.

Other bits of tools already have this:

mariner:xen.git> git-grep -i 'define.*build_bug' tools | cat
tools/firmware/hvmloader/util.h:#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 
2 * !!(p)]))
tools/libxc/xc_private.h:#define XC_BUILD_BUG_ON(p) ({ _Static_assert(!(p), 
"!(" #p ")"); })
tools/libxc/xc_private.h:#define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { 
int:-!!(p); }))
tools/libxl/libxl_internal.h:#define BUILD_BUG_ON(p) ({ _Static_assert(!(p), 
"!(" #p ")"); })
tools/libxl/libxl_internal.h:#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 * 
!!(p)]))
mariner:xen.git> 

Do we really need to introduce yet another copy of this ?  Is gnttab
not allowed to include xc_private.h ?  Is there not some one place
where we could put this ?

Ian.

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

Re: [Xen-devel] [PATCH v6 04/15] x86: add multiboot2 protocol support

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> @@ -65,13 +82,11 @@ static u32 copy_string(u32 src)
>  return copy_mem(src, p - src + 1);
>  }
>  
> -multiboot_info_t __stdcall *reloc(u32 mbi_in, u32 trampoline)
> +static multiboot_info_t *mbi_mbi(u32 mbi_in)

This is rather unhelpful a name - how about mbi_reloc() (and then
below mbi2_reloc() accordingly)?

> +static multiboot_info_t *mbi2_mbi(u32 mbi_in)
> +{
> +const multiboot2_fixed_t *mbi_fix = _p(mbi_in);
> +const multiboot2_memory_map_t *mmap_src;
> +const multiboot2_tag_t *tag;
> +module_t *mbi_out_mods = NULL;
> +memory_map_t *mmap_dst;
> +multiboot_info_t *mbi_out;
> +u32 ptr;
> +unsigned int i, mod_idx = 0;
> +
> +ptr = alloc_mem(sizeof(*mbi_out));
> +mbi_out = _p(ptr);
> +zero_mem(ptr, sizeof(*mbi_out));
> +
> +/* Skip Multiboot2 information fixed part. */
> +ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +/* Get the number of modules. */
> +for ( tag = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
> +  tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
> +{
> +if ( tag->type == MULTIBOOT2_TAG_TYPE_MODULE )
> +++mbi_out->mods_count;
> +else if ( tag->type == MULTIBOOT2_TAG_TYPE_END )
> +break;
> +}
> +
> +if ( mbi_out->mods_count )
> +{
> +mbi_out->flags = MBI_MODULES;

Even if this is the first adjustment to the field right now, code would
be more robust wrt future additions if you used |= here just like you
do further down.

> +mbi_out->mods_addr = alloc_mem(mbi_out->mods_count * 
> sizeof(module_t));
> +mbi_out_mods = _p(mbi_out->mods_addr);
> +}
> +
> +/* Skip Multiboot2 information fixed part. */
> +ptr = ALIGN_UP(mbi_in + sizeof(multiboot2_fixed_t), 
> MULTIBOOT2_TAG_ALIGN);
> +
> +/* Put all needed data into mbi_out. */
> +for ( tag = _p(ptr); (u32)tag - mbi_in < mbi_fix->total_size;
> +  tag = _p(ALIGN_UP((u32)tag + tag->size, MULTIBOOT2_TAG_ALIGN)) )
> +switch ( tag->type )
> +{
> +case MULTIBOOT2_TAG_TYPE_BOOT_LOADER_NAME:
> +mbi_out->flags |= MBI_LOADERNAME;
> +ptr = get_mb2_string(tag, string, string);
> +mbi_out->boot_loader_name = copy_string(ptr);
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_CMDLINE:
> +mbi_out->flags |= MBI_CMDLINE;
> +ptr = get_mb2_string(tag, string, string);
> +mbi_out->cmdline = copy_string(ptr);
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_BASIC_MEMINFO:
> +mbi_out->flags |= MBI_MEMLIMITS;
> +mbi_out->mem_lower = get_mb2_data(tag, basic_meminfo, mem_lower);
> +mbi_out->mem_upper = get_mb2_data(tag, basic_meminfo, mem_upper);
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_MMAP:
> +if ( get_mb2_data(tag, mmap, entry_size) < sizeof(*mmap_src) )
> +break;
> +
> +mbi_out->flags |= MBI_MEMMAP;
> +mbi_out->mmap_length = get_mb2_data(tag, mmap, size);
> +mbi_out->mmap_length -= sizeof(multiboot2_tag_mmap_t);
> +mbi_out->mmap_length /= get_mb2_data(tag, mmap, entry_size);
> +mbi_out->mmap_length *= sizeof(memory_map_t);
> +
> +mbi_out->mmap_addr = alloc_mem(mbi_out->mmap_length);
> +
> +mmap_src = get_mb2_data(tag, mmap, entries);
> +mmap_dst = _p(mbi_out->mmap_addr);
> +
> +for ( i = 0; i < mbi_out->mmap_length / sizeof(memory_map_t); 
> i++ )
> +{
> +/* Init size member properly. */
> +mmap_dst[i].size = sizeof(memory_map_t);

Just like you already do in other sizeof() instances, this as well as the
one in the loop control would better be sizeof(*mmap_dst).

> +mmap_dst[i].size -= sizeof(((memory_map_t){0}).size);
> +/* Now copy a given region data. */
> +mmap_dst[i].base_addr_low = (u32)mmap_src->addr;
> +mmap_dst[i].base_addr_high = (u32)(mmap_src->addr >> 32);
> +mmap_dst[i].length_low = (u32)mmap_src->len;
> +mmap_dst[i].length_high = (u32)(mmap_src->len >> 32);
> +mmap_dst[i].type = mmap_src->type;
> +mmap_src = _p(mmap_src) + get_mb2_data(tag, mmap, 
> entry_size);
> +}
> +break;
> +
> +case MULTIBOOT2_TAG_TYPE_MODULE:
> +if ( mod_idx >= mbi_out->mods_count )
> +break;
> +
> +mbi_out_mods[mod_idx].mod_start = get_mb2_data(tag, module, 
> mod_start);
> +mbi_out_mods[mod_idx].mod_end = get_mb2_data(tag, module, 
> mod_end);
> +ptr = get_mb2_string(tag, module, cmdline);
> +mbi_out_mods[mod_idx].string = copy_string(ptr);

How is no (or an empty) command line being represented? Surely
you could avoid alloca

Re: [Xen-devel] [PATCH v6 05/15] efi: create efi_enabled()

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
>   * domain's page tables but current may point at another domain's VCPU.
>   * Return NULL as though current is not properly set up yet.
>   */
> -if ( efi_enabled && efi_rs_using_pgtables() )
> +if ( efi_enabled(EFI_RS) && efi_rs_using_pgtables() )

I think the efi_enabled() here is pointless now.

With this dropped (unless you know of a reason that it needs to stay)
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v6 06/15] x86: allow EFI reboot method neither on EFI platforms...

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> ..nor EFI platforms with runtime services enabled.
> 
> Suggested-by: Jan Beulich 
> Signed-off-by: Daniel Kiper 

Acked-by: Jan Beulich 

Albeit I think the title/description is now not really fitting the
single efi_enabled() check anymore.

Jan




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

Re: [Xen-devel] [PATCH v6 08/15] x86/efi: create new early memory allocator

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -520,6 +520,8 @@ static void noinline init_done(void)
>  
>  system_state = SYS_STATE_active;
>  
> +free_ebmalloc_unused_mem();

Now that the allocator properly lives in common code, this appears
to lack an ARM side counterpart.

> @@ -1158,7 +1160,38 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  for( ; ; ); /* not reached */
>  }
>  
> -#ifndef CONFIG_ARM /* TODO - runtime service support */
> +#ifndef CONFIG_ARM
> +
> +#define EBMALLOC_SIZEMB(1)
> +
> +static char __section(".bss.page_aligned") __aligned(PAGE_SIZE) 
> ebmalloc_mem[EBMALLOC_SIZE];

Long line.

> +static unsigned long __initdata ebmalloc_allocated;
> +
> +/* EFI boot allocator. */
> +static void __init *ebmalloc(size_t size)
> +{
> +void *ptr = ebmalloc_mem + ebmalloc_allocated;
> +
> +ebmalloc_allocated += (size + sizeof(void *) - 1) & 
> ~((typeof(size))sizeof(void *) - 1);

What's the point of this ugly cast?

Jan


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

Re: [Xen-devel] [PATCH v6 09/15] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -3,6 +3,43 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * Here we are in EFI stub. EFI calls are not supported due to lack
> + * of relevant functionality in compiler and/or linker.
> + *
> + * efi_multiboot2() is an exception. Please look below for more details.
> + */
> +
> +paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTable)

Long line.

> +{
> +CHAR16 *err = L"Xen does not have EFI code build in!!!\r\nSystem 
> halted!!!\r\n";
> +SIMPLE_TEXT_OUTPUT_INTERFACE *StdErr;
> +
> +StdErr = SystemTable->StdErr ? SystemTable->StdErr : SystemTable->ConOut;
> +
> +/*
> + * Print error message and halt the system.
> + *
> + * We have to open code MS x64 calling convention
> + * in assembly because here this convention is not
> + * directly supported by C compiler and/or linker.

So how can lack of calling convention support be due to missing
functionality in the linker? Please avoid such misleading comments:
Linker capabilities get probed solely for PE32+ output format
support. With the linker part dropped here,
Acked-by: Jan Beulich 

Jan




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

[Xen-devel] [PATCH v2 2/4] libxl: add basic support for devices without backend

[Juergen Gross]

With the planned support of HVM USB passthrough via the USB emulation
capabilities of qemu libxl has to support guest devices which have no
back- and frontend. Information about those devices will live in the
libxl part of Xenstore only.

Add some basic support to libxl to be able to cope with this scenario.

Signed-off-by: Juergen Gross 
Acked-by: Wei Liu 
---
 tools/libxl/libxl_device.c   | 59 
 tools/libxl/libxl_types_internal.idl |  1 +
 tools/libxl/libxl_xshelp.c   |  6 +++-
 3 files changed, 46 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index dbf157d..f53f772 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -114,15 +114,21 @@ int libxl__device_generic_add(libxl__gc *gc, 
xs_transaction_t t,
 libxl__device *device, char **bents, char **fents, char **ro_fents)
 {
 libxl_ctx *ctx = libxl__gc_owner(gc);
-char *frontend_path, *backend_path, *libxl_path;
+char *frontend_path = NULL, *backend_path = NULL, *libxl_path;
 struct xs_permissions frontend_perms[2];
 struct xs_permissions ro_frontend_perms[2];
 struct xs_permissions backend_perms[2];
 int create_transaction = t == XBT_NULL;
+int libxl_only = device->backend_kind == LIBXL__DEVICE_KIND_NONE;
 int rc;
 
-frontend_path = libxl__device_frontend_path(gc, device);
-backend_path = libxl__device_backend_path(gc, device);
+if (libxl_only) {
+/* bents should be set as this is used to setup libxl_path content. */
+assert(!fents && !ro_fents);
+} else {
+frontend_path = libxl__device_frontend_path(gc, device);
+backend_path = libxl__device_backend_path(gc, device);
+}
 libxl_path = libxl__device_libxl_path(gc, device);
 
 frontend_perms[0].id = device->domid;
@@ -144,13 +150,15 @@ retry_transaction:
 rc = libxl__xs_rm_checked(gc, t, libxl_path);
 if (rc) goto out;
 
-rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/frontend",libxl_path),
- frontend_path);
-if (rc) goto out;
+if (!libxl_only) {
+rc = libxl__xs_write_checked(gc, t, 
GCSPRINTF("%s/frontend",libxl_path),
+ frontend_path);
+if (rc) goto out;
 
-rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
- backend_path);
-if (rc) goto out;
+rc = libxl__xs_write_checked(gc, t, GCSPRINTF("%s/backend",libxl_path),
+ backend_path);
+if (rc) goto out;
+}
 
 /* xxx much of this function lacks error checks! */
 
@@ -179,12 +187,15 @@ retry_transaction:
 }
 
 if (bents) {
-xs_rm(ctx->xsh, t, backend_path);
-xs_mkdir(ctx->xsh, t, backend_path);
-xs_set_permissions(ctx->xsh, t, backend_path, backend_perms, 
ARRAY_SIZE(backend_perms));
-xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
- frontend_path, strlen(frontend_path));
-libxl__xs_writev(gc, t, backend_path, bents);
+if (!libxl_only) {
+xs_rm(ctx->xsh, t, backend_path);
+xs_mkdir(ctx->xsh, t, backend_path);
+xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
+   ARRAY_SIZE(backend_perms));
+xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
+ frontend_path, strlen(frontend_path));
+libxl__xs_writev(gc, t, backend_path, bents);
+}
 
 /*
  * We make a copy of everything for the backend in the libxl
@@ -194,6 +205,9 @@ retry_transaction:
  * instead.  But there are still places in libxl that try to
  * reconstruct a config from xenstore.
  *
+ * For devices without PV backend (e.g. USB devices emulated via qemu)
+ * only the libxl path is written.
+ *
  * This duplication will typically produces duplicate keys
  * which will go out of date, but that's OK because nothing
  * reads those.  For example, there is usually
@@ -679,14 +693,20 @@ void libxl__multidev_prepared(libxl__egc *egc,
 
 int libxl__device_destroy(libxl__gc *gc, libxl__device *dev)
 {
-const char *be_path = libxl__device_backend_path(gc, dev);
-const char *fe_path = libxl__device_frontend_path(gc, dev);
+const char *be_path = NULL;
+const char *fe_path = NULL;
 const char *libxl_path = libxl__device_libxl_path(gc, dev);
 const char *tapdisk_path = GCSPRINTF("%s/%s", be_path, "tapdisk-params");
 const char *tapdisk_params;
 xs_transaction_t t = 0;
 int rc;
 uint32_t domid;
+int libxl_only = dev->backend_kind == LIBXL__DEVICE_KIND_NONE;
+
+if (!libxl_only) {
+be_path = libxl__device_backend_path(gc, dev);
+fe_path = libxl__device_frontend_path(gc, dev);
+}
 
 rc = libxl_

[Xen-devel] [PATCH v2 1/4] libxl: add function to remove usb controller xenstore entries

[Juergen Gross]

In case of failure when trying to add a new USB controller to a domain
libxl might leak xenstore entries. Add a function to remove them and
call this function in case of failure.

Signed-off-by: Juergen Gross 
---
This patch might be a backport candidate to 4.7 (will have to modify
tools/libxl/libxl_pvusb.c instead).
---
 tools/libxl/libxl_usb.c | 58 +
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 6b30e0f..2493464 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -194,6 +194,47 @@ out:
 return rc;
 }
 
+static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
+{
+const char *be_path;
+int r;
+
+r = libxl__xs_read_checked(gc, XBT_NULL,
+   GCSPRINTF("%s/backend", libxl_path),
+   &be_path);
+if (r || !be_path) return NULL;
+
+return be_path;
+}
+
+static void libxl__device_usbctrl_del_xenstore(libxl__gc *gc, uint32_t domid,
+   libxl_device_usbctrl *usbctrl)
+{
+const char *libxl_path, *be_path;
+xs_transaction_t t = XBT_NULL;
+int rc;
+
+libxl_path = GCSPRINTF("%s/device/vusb/%d",
+   libxl__xs_libxl_path(gc, domid), usbctrl->devid);
+be_path = vusb_be_from_xs_libxl(gc, libxl_path);
+
+for (;;) {
+rc = libxl__xs_transaction_start(gc, &t);
+if (rc) goto out;
+
+libxl__xs_path_cleanup(gc, t, be_path);
+
+rc = libxl__xs_transaction_commit(gc, &t);
+if (!rc) break;
+if (rc < 0) goto out;
+}
+
+return;
+
+out:
+libxl__xs_transaction_abort(gc, &t);
+}
+
 static char *pvusb_get_device_type(libxl_usbctrl_type type)
 {
 switch (type) {
@@ -250,13 +291,15 @@ static void libxl__device_usbctrl_add(libxl__egc *egc, 
uint32_t domid,
 
 GCNEW(device);
 rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
-if (rc) goto out;
+if (rc) goto outrm;
 
 aodev->dev = device;
 aodev->action = LIBXL__DEVICE_ACTION_ADD;
 libxl__wait_device_connection(egc, aodev);
 return;
 
+outrm:
+libxl__device_usbctrl_del_xenstore(gc, domid, usbctrl);
 out:
 aodev->rc = rc;
 aodev->callback(egc, aodev);
@@ -340,19 +383,6 @@ out:
 return;
 }
 
-static const char *vusb_be_from_xs_libxl(libxl__gc *gc, const char *libxl_path)
-{
-const char *be_path;
-int r;
-
-r = libxl__xs_read_checked(gc, XBT_NULL,
-   GCSPRINTF("%s/backend", libxl_path),
-   &be_path);
-if (r || !be_path) return NULL;
-
-return be_path;
-}
-
 libxl_device_usbctrl *
 libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
 {
-- 
2.6.6


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

[Xen-devel] [PATCH v2 0/4] libxl: add HVM USB passthrough capability

[Juergen Gross]

Add the capability to pass USB devices to HVM domains by using the
emulation of USB controllers of qemu.

The user interface via xl is the same as for pvusb passthrough, only
the type of the usbctrl is different: instead of "qusb" (qemu-based
pvusb backend) or "vusb" (kernel-based pvusb backend) the type
"devicemodel" is used.

Especially the communication with qemu via qmp commands is based on
the patches of George Dunlap sent in 2014:

https://lists.xen.org/archives/html/xen-devel/2014-06/msg00085.html

Changes in V2:
- patches 1-3 removed as already pushed
- split out (new) patch 1 from patch 3 (was 5) as requested by Wei Liu
- addressed code style issues in patch 3 (was 5) as requested by Wei Liu
- added some assert()s n patch 3 (was 5) as requested by Wei Liu

Juergen Gross (4):
  libxl: add function to remove usb controller xenstore entries
  libxl: add basic support for devices without backend
  libxl: add HVM usb passthrough support
  docs: add HVM USB passthrough documentation

 docs/man/xl.cfg.pod.5.in |  12 +-
 tools/libxl/libxl_device.c   |  62 +++--
 tools/libxl/libxl_types_internal.idl |   1 +
 tools/libxl/libxl_usb.c  | 435 +++
 tools/libxl/libxl_xshelp.c   |   6 +-
 tools/libxl/xl_cmdimpl.c |   4 +-
 6 files changed, 400 insertions(+), 120 deletions(-)

-- 
2.6.6


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

[Xen-devel] [PATCH v2 3/4] libxl: add HVM usb passthrough support

[Juergen Gross]

Add HVM usb passthrough support to libxl by using qemu's capability
to emulate standard USB controllers.

A USB controller is added via qmp command to the emulated hardware
when a usbctrl device of type DEVICEMODEL is requested. Depending on
the requested speed the appropriate hardware type is selected. A host
USB device can then be added to the emulated USB controller via qmp
command.

Removing of the devices is done via qmp commands, too.

Signed-off-by: Juergen Gross 
---
V2: code style issues (Wei Liu)
adding some assert()s (Wei Liu)
split out libxl__device_usbctrl_del_xenstore() (Wei Liu)
---
 tools/libxl/libxl_device.c |   3 +-
 tools/libxl/libxl_usb.c| 397 +++--
 tools/libxl/xl_cmdimpl.c   |   4 +-
 3 files changed, 311 insertions(+), 93 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index f53f772..2acfb48 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -808,8 +808,7 @@ void libxl__devices_destroy(libxl__egc *egc, 
libxl__devices_remove_state *drs)
 aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
 aodev->dev = dev;
 aodev->force = drs->force;
-if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB ||
-dev->backend_kind == LIBXL__DEVICE_KIND_QUSB)
+if (dev->kind == LIBXL__DEVICE_KIND_VUSB)
 libxl__initiate_device_usbctrl_remove(egc, aodev);
 else
 libxl__initiate_device_generic_remove(egc, aodev);
diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
index 2493464..09bfa24 100644
--- a/tools/libxl/libxl_usb.c
+++ b/tools/libxl/libxl_usb.c
@@ -17,6 +17,7 @@
 
 #include "libxl_internal.h"
 #include 
+#include 
 
 #define USBBACK_INFO_PATH "/libxl/usbback"
 
@@ -43,12 +44,6 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, 
uint32_t domid,
 int rc;
 libxl_domain_type domtype = libxl__domain_type(gc, domid);
 
-if (!usbctrl->version)
-usbctrl->version = 2;
-
-if (!usbctrl->ports)
-usbctrl->ports = 8;
-
 if (usbctrl->type == LIBXL_USBCTRL_TYPE_AUTO) {
 if (domtype == LIBXL_DOMAIN_TYPE_PV) {
 rc = usbback_is_loaded(gc);
@@ -62,6 +57,71 @@ static int libxl__device_usbctrl_setdefault(libxl__gc *gc, 
uint32_t domid,
 }
 }
 
+switch (usbctrl->type) {
+case LIBXL_USBCTRL_TYPE_PV:
+case LIBXL_USBCTRL_TYPE_QUSB:
+if (!usbctrl->version)
+usbctrl->version = 2;
+if (usbctrl->version < 1 || usbctrl->version > 2) {
+LOG(ERROR,
+"USB version for paravirtualized devices must be 1 or 2");
+rc = ERROR_INVAL;
+goto out;
+}
+if (!usbctrl->ports)
+usbctrl->ports = 8;
+if (usbctrl->ports < 1 || usbctrl->ports > USBIF_MAX_PORTNR) {
+LOG(ERROR, "Number of ports for USB controller is limited to %u",
+USBIF_MAX_PORTNR);
+rc = ERROR_INVAL;
+goto out;
+}
+break;
+case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
+if (!usbctrl->version)
+usbctrl->version = 2;
+switch (usbctrl->version) {
+case 1:
+/* uhci controller in qemu has fixed number of ports. */
+if (usbctrl->ports && usbctrl->ports != 2) {
+LOG(ERROR,
+"Number of ports for USB controller of version 1 is always 
2");
+rc = ERROR_INVAL;
+goto out;
+}
+usbctrl->ports = 2;
+break;
+case 2:
+/* ehci controller in qemu has fixed number of ports. */
+if (usbctrl->ports && usbctrl->ports != 6) {
+LOG(ERROR,
+"Number of ports for USB controller of version 2 is always 
6");
+rc = ERROR_INVAL;
+goto out;
+}
+usbctrl->ports = 6;
+break;
+case 3:
+if (!usbctrl->ports)
+usbctrl->ports = 8;
+/* xhci controller in qemu supports up to 15 ports. */
+if (usbctrl->ports > 15) {
+LOG(ERROR,
+"Number of ports for USB controller of version 3 is 
limited to 15");
+rc = ERROR_INVAL;
+goto out;
+}
+break;
+default:
+LOG(ERROR, "Illegal USB version");
+rc = ERROR_INVAL;
+goto out;
+}
+break;
+default:
+break;
+}
+
 rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
   &usbctrl->backend_domid);
 
@@ -75,9 +135,20 @@ static int libxl__device_from_usbctrl(libxl__gc *gc, 
uint32_t domid,
 {
 device->backend_devid   = usbctrl->devid;
 device->backend_domid   = usbctrl->backend_domid;
-device->backend_kind= 

[Xen-devel] [PATCH v2 4/4] docs: add HVM USB passthrough documentation

[Juergen Gross]

Update the man page regarding passthrough of USB devices to HVM
domains via qemu USB emulation.

Signed-off-by: Juergen Gross 
Acked-by: Wei Liu 
---
 docs/man/xl.cfg.pod.5.in | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 77a1be3..076b2a6 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -745,19 +745,25 @@ Specifies the usb controller type.
 
 "qusb" specifies a qemu base backend for pvusb.
 
+"devicemodel" specifies a USB controller emulated by qemu. It will show up as
+a PCI-device in the guest.
+
 "auto" (the default) determines whether a kernel based backend is installed.
 If this is the case, "pv" is selected, "qusb" will be selected if no kernel
-backend is currently available.
+backend is currently available. For HVM domains "devicemodel" is being 
selected.
 
 =item B
 
 Specifies the usb controller version.  Possible values include
-1 (USB1.1) and 2 (USB2.0). Default is 2 (USB2.0).
+1 (USB1.1), 2 (USB2.0) and 3 (USB3.0). Default is 2 (USB2.0). 3 (USB3.0) is
+available for the type "devicemodel" only.
 
 =item B
 
 Specifies the total ports of the usb controller. The maximum
-number is 31. Default is 8.
+number is 31. Default is 8. With the type "devicemodel" the number of ports
+is more limited: a USB1.1 controller always has 2 ports, a USB2.0 controller
+always has 6 ports and a USB3.0 controller can have up to 15 ports.
 
 USB controller ids start from 0.  In line with the USB spec, however,
 ports on a controller start from 1.
-- 
2.6.6


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

Re: [Xen-devel] [PATCH v6 10/15] x86/boot: implement early command line parser in C

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/boot/Makefile
> +++ b/xen/arch/x86/boot/Makefile
> @@ -1,9 +1,16 @@
>  obj-bin-y += head.o
>  
> -RELOC_DEPS = $(BASEDIR)/include/asm-x86/config.h 
> $(BASEDIR)/include/xen/multiboot.h \
> +DEFS_H_DEPS = $(BASEDIR)/include/xen/stdbool.h
> +
> +CMDLINE_DEPS = defs.h $(DEFS_H_DEPS) video.h
> +
> +RELOC_DEPS = defs.h $(DEFS_H_DEPS) $(BASEDIR)/include/xen/multiboot.h \

I think it would be more natural for DEFS_H_DEPS to include defs.h,
as I can't see a valid use of one without the other; perhaps it would
then better be renamed.

> --- /dev/null
> +++ b/xen/arch/x86/boot/defs.h
> @@ -0,0 +1,52 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see .
> + *
> + * max() was copied from xen/xen/include/xen/kernel.h.
> + */
> +
> +#ifndef __BOOT_DEFS_H__
> +#define __BOOT_DEFS_H__
> +
> +#include "../../../include/xen/stdbool.h"
> +
> +#define __packed __attribute__((__packed__))
> +#define __stdcall__attribute__((__stdcall__))
> +
> +#define NULL ((void *)0)
> +
> +#define ALIGN_UP(arg, align) \
> +(((arg) + (align) - 1) & ~((typeof(arg))(align) - 1))
> +
> +#define max(x,y) ({ \
> +const typeof(x) _x = (x);   \
> +const typeof(y) _y = (y);   \
> +(void) (&_x == &_y);\
> +_x > _y ? _x : _y; })

Please don't add max() without also adding min().

> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -220,8 +220,22 @@ trampoline_boot_cpu_entry:
>  /* Jump to the common bootstrap entry point. */
>  jmp trampoline_protmode_entry
>  
> +#include "video.h"
> +
> +.align  2
> +.byte   0

While this odd placement of the requested padding byte will fulfill
the immediate purpose, please do it the originally requested more
conventional way (putting it inside the structure and adding a
respective field to the C representation). For someone wanting to
add another field it'll then be far more obvious what to do without
re-introducing mis-alignment.

Jan


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

Re: [Xen-devel] [PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON

[Wei Liu]

On Mon, Sep 19, 2016 at 12:41:47PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON"):
> > The implementation is taken from libxc.
> ...
> > I could have put it in a header file accessible to all libraries under
> > libs but this construct is only relevant to xengnttab library at the
> > moment so it's put under gnttab/private.h. It can be easily moved to
> > a common place when other libraries under libs require it.
> 
> Other bits of tools already have this:
> 
> mariner:xen.git> git-grep -i 'define.*build_bug' tools | cat
> tools/firmware/hvmloader/util.h:#define BUILD_BUG_ON(p) ((void)sizeof(char[1 
> - 2 * !!(p)]))
> tools/libxc/xc_private.h:#define XC_BUILD_BUG_ON(p) ({ _Static_assert(!(p), 
> "!(" #p ")"); })
> tools/libxc/xc_private.h:#define XC_BUILD_BUG_ON(p) ((void)sizeof(struct { 
> int:-!!(p); }))
> tools/libxl/libxl_internal.h:#define BUILD_BUG_ON(p) ({ _Static_assert(!(p), 
> "!(" #p ")"); })
> tools/libxl/libxl_internal.h:#define BUILD_BUG_ON(p) ((void)sizeof(char[1 - 2 
> * !!(p)]))
> mariner:xen.git> 
> 
> Do we really need to introduce yet another copy of this ?  Is gnttab
> not allowed to include xc_private.h ?  Is there not some one place
> where we could put this ?
> 

AIUI BUILD_BUG_ON is a private thing to each library, hence the
duplication. But I'm fine with having a central header for that, too.

If we want to share that macro across all libraries, we can create a
tools/include/private.h.  But that would require a fair bit of work --
adjust xen build system, fix stubdom build, fix mini-os etc -- which I
think it would be best to leave to me.

In the meantime, we should unblock Paulina on this one so that her
project can move forward.

Wei.

> Ian.

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

Re: [Xen-devel] [PATCH v6 11/15] x86: change default load address from 1 MiB to 2 MiB

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -1,6 +1,10 @@
>  
>  # x86-specific definitions
>  
> +XEN_IMG_OFFSET = 0x20

Please prefer := for simple assignments like this one.

> +CFLAGS += -DXEN_IMG_OFFSET=$(XEN_IMG_OFFSET)

Please move this past ...

>  CFLAGS += -I$(BASEDIR)/include
>  CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-generic
>  CFLAGS += -I$(BASEDIR)/include/asm-x86/mach-default

... these, to keep together with the other -D setting.

With those adjustments
Acked-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v6 12/15] x86/setup: use XEN_IMG_OFFSET instead of...

[Jan Beulich]

>>> On 12.09.16 at 22:18,  wrote:
> ..calculating its value during runtime.
> 
> Signed-off-by: Daniel Kiper 

Acked-by: Jan Beulich 


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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Peng Fan]

Hello Julien,
On Mon, Sep 19, 2016 at 10:53:56AM +0200, Julien Grall wrote:
>Hello,
>
>On 19/09/2016 10:36, Peng Fan wrote:
>>On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:
>>>Hello Peng,
>>>
>>>On 19/09/2016 04:08, van.free...@gmail.com wrote:
From: Peng Fan 

This patchset is to support XEN run on big.little SoC.
The idea of the patch is from
"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";

There are some changes to cpupool and add x86 stub functions to avoid build
break. Sending The RFC patchset out is to request for comments to see 
whether
this implementation is acceptable or not. Patchset have been tested based on
xen-4.8 unstable on NXP i.MX8.

I use Big/Little CPU and cpupool to explain the idea.
A pool contains Big CPUs is called Big Pool.
A pool contains Little CPUs is called Little Pool.
If a pool does not contains any physical cpus, Little CPUs or Big CPUs
can be added to the cpupool. But the cpupool can not contain both Little
and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr value 
for ARM).
CPUs can not be added to the cpupool which contains cpus that have 
different cpu type.
Little CPUs can not be moved to Big Pool if there are Big CPUs in Big Pool,
and versa. Domain in Big Pool can not be migrated to Little Pool, and versa.
When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
type(same midr value)
into cpupool0.
>>>
>>>As mentioned in the mail you pointed above, this series is not enough to make
>>>big.LITTLE working on then. Xen is always using the boot CPU to detect the
>>>list of features. With big.LITTLE features may not be the same.
>>>
>>>And I would prefer to see Xen supporting big.LITTLE correctly before
>>>beginning to think to expose big.LITTLE to the userspace (via cpupool)
>>>automatically.
>>
>>Do you mean vcpus be scheduled between big and little cpus freely?
>
>By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
>the same set of features. So the feature detection is only done the boot CPU.
>See processor_setup for instance...
>
>Moving vCPUs between big and little cores would be a hard task (cache line
>issue, and possibly feature) and I don't expect to ever cross this in Xen.
>However, I am expecting to see big.LITTLE exposed to the guest (i.e having
>big and little vCPUs).
>
>>
>>This patchset is to use cpupool to block the vcpu be scheduled between big and
>>little cpus.
>>
>>>
>>>See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).

Back to this.
In xen/arch/arm/traps.c, I found that
"
WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
 HCR_TWE|HCR_TWI|HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,
 HCR_EL2);
"

HCR_TACR, HCR_TIDx is not set. HCR_TIDCP is set, but this is used to trap
implementation defined registers.

So accessing the actlr and cpu feature registers(exclude the implementation 
defined ones)
in guest os will not trap to xen, right?
If this is true, the actlr and cpu feature registers for DomU in Pool-A72 in my 
case
should be correct.

Thanks,
Peng.

>>
>>Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
>>the A53 and A72 TRM about AArch64 implementationd defined registers.
>>This actlr can be added to the cpupool_arch_info as midr.
>>
>>Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
>>Please advise if I missed anything else.
>
>Have you check the register emulation?


>
>>
>>>

Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first 
one
that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
`xl cpupool-list -c` will show cpu[0-3] in Pool-0.

Then use the following script to create a new cpupool and add cpu[4-5] to
the cpupool.
#xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
#xl cpupool-cpu-add Pool-A72 4
#xl cpupool-cpu-add Pool-A72 5
#xl create -d /root/xen/domu-test pool=\"Pool-A72\"
>>>
>>>I am a bit confused with these runes. It means that only the first kind of
>>>CPUs have pool assigned. Why don't you directly create all the pools at boot
>>>time?
>>
>>If need to create all the pools, need to decided how many pools need to be 
>>created.
>>I thought about this, but I do not come out a good idea.
>>
>>The cpupool0 is defined in xen/common/cpupool.c, if need to create many pools,
>>need to alloc cpupools dynamically when booting. I would not like to change a
>>lot to common code.
>
>Why? We should avoid to choose a specific design just because the common code
>does not allow you to do it without heavy change.
>
>We never came across the big.LITTLE problem on x86, so it is normal to modify
>the code.
>
>>The implementation in this patchset I think is an easy way to let Big and 
>>Littl

Re: [Xen-devel] [RFC PATCH 0/9] Introduce AMD SVM AVIC

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 12:52:39AM -0500, Suravee Suthikulpanit wrote:
> GITHUB
> ==
> Latest git tree can be found at:
> http://github.com/ssuthiku/xen.gitxen_avic_part1_v1
> 
> OVERVIEW
> 
> This patch set is the first of the two-part patch series to introduce 
> the new AMD Advance Virtual Interrupt Controller (AVIC) support.
> 
> Basically, SVM AVIC hardware virtualizes local APIC registers of each
> vCPU via the virtual APIC (vAPIC) backing page. This allows guest access
> to certain APIC registers without the need to emulate the hardware behavior
> in the hypervisor. More information about AVIC can be found in the
> AMD64 Architecture Programmer’s Manual Volume 2 - System Programming.
> 
>   http://support.amd.com/TechDocs/24593.pdf
> 
> For SVM AVIC, we extend the existing kvm_amd driver to:

kvm_amd ?, heheh

.. snip..
> Note: In "w/o evtchn" case, the Linux guest is built w/o
>   Xen guest support.

You can also just boot Linux with 'xen_nopv' parameter which would
do the same thing.


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

Re: [Xen-devel] [PATCH v4 05/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

[Julien Grall]

Hello Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

If the distance is too great we are in trouble - as our relocation
distance can surely be clipped, or still have a valid width - but
cause an overflow of distance.

On various architectures the maximum displacement for a unconditional
branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86
for 32-bit relocations is +/- 2G.

Note: On x86 we could use the 64-bit jmpq instruction which
would provide much bigger displacement to do a jump, but we would
still have issues with the new function not being able to reach
any of the old functions (as all the relocations would assume 32-bit
displacement). And "furthermore would require an register or
memory location to load/store the address to." (From Jan).

On ARM the conditional branch supports even a smaller displacement
but fortunatly we are not using that.


s/fortunatly/fortunately/



Signed-off-by: Konrad Rzeszutek Wilk 

---


[...]


diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 9e72897..5baaa0a 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1100,7 +1100,7 @@ and no .data or .bss sections.
 The hypervisor should verify that the in-place patching would fit within
 the code or data.

-### Trampoline (e9 opcode)
+### Trampoline (e9 opcode), x86

 The e9 opcode used for jmpq uses a 32-bit signed displacement. That means
 we are limited to up to 2GB of virtual address to place the new code
@@ -1134,3 +1134,15 @@ that in the hypervisor is advised.
 The tool for generating payloads currently does perform a compile-time
 check to ensure that the function to be replaced is large enough.

+The hypervisor also checks the displacement during loading of the payload.
+
+ Trampoline (ea opcode), ARM
+
+The 0xea00 instruction (with proper offset) is used for an unconditional
+branch to the new code.


The opcode/encoding mentioned is wrong for AArch64. Anyway, I am not 
sure why you want to mention the opcode in the documentation. I think it 
would be enough to specify: "unconditional branch instruction (for the 
encoding see the ARM ARM).".



This means we are limited on ARM32 to +/- 32MB
+displacement and on ARM64 to +/- 128MB displacement.
+
+The new code is placed in the 8M - 10M virtual address space while the
+Xen code is in 2M - 4M. That gives us enough space.
+
+The hypervisor also checks the displacement during loading of the payload.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Peng Fan]

On Mon, Sep 19, 2016 at 11:59:05AM +0200, Julien Grall wrote:
>
>
>On 19/09/2016 11:38, Peng Fan wrote:
>>On Mon, Sep 19, 2016 at 10:53:56AM +0200, Julien Grall wrote:
>>>Hello,
>>>
>>>On 19/09/2016 10:36, Peng Fan wrote:
On Mon, Sep 19, 2016 at 10:09:06AM +0200, Julien Grall wrote:
>Hello Peng,
>
>On 19/09/2016 04:08, van.free...@gmail.com wrote:
>>From: Peng Fan 
>>
>>This patchset is to support XEN run on big.little SoC.
>>The idea of the patch is from
>>"https://lists.xenproject.org/archives/html/xen-devel/2016-05/msg00465.html";
>>
>>There are some changes to cpupool and add x86 stub functions to avoid 
>>build
>>break. Sending The RFC patchset out is to request for comments to see 
>>whether
>>this implementation is acceptable or not. Patchset have been tested based 
>>on
>>xen-4.8 unstable on NXP i.MX8.
>>
>>I use Big/Little CPU and cpupool to explain the idea.
>>A pool contains Big CPUs is called Big Pool.
>>A pool contains Little CPUs is called Little Pool.
>>If a pool does not contains any physical cpus, Little CPUs or Big CPUs
>>can be added to the cpupool. But the cpupool can not contain both Little
>>and Big CPUs. The CPUs in a cpupool must have the same cpu type(midr 
>>value for ARM).
>>CPUs can not be added to the cpupool which contains cpus that have 
>>different cpu type.
>>Little CPUs can not be moved to Big Pool if there are Big CPUs in Big 
>>Pool,
>>and versa. Domain in Big Pool can not be migrated to Little Pool, and 
>>versa.
>>When XEN tries to bringup all the CPUs, only add CPUs with the same cpu 
>>type(same midr value)
>>into cpupool0.
>
>As mentioned in the mail you pointed above, this series is not enough to 
>make
>big.LITTLE working on then. Xen is always using the boot CPU to detect the
>list of features. With big.LITTLE features may not be the same.
>
>And I would prefer to see Xen supporting big.LITTLE correctly before
>beginning to think to expose big.LITTLE to the userspace (via cpupool)
>automatically.

Do you mean vcpus be scheduled between big and little cpus freely?
>>>
>>>By supporting big.LITTLE correctly I meant Xen thinks that all the cores has
>>>the same set of features. So the feature detection is only done the boot CPU.
>>>See processor_setup for instance...
>>>
>>>Moving vCPUs between big and little cores would be a hard task (cache line
>>>issue, and possibly feature) and I don't expect to ever cross this in Xen.
>>>However, I am expecting to see big.LITTLE exposed to the guest (i.e having
>>>big and little vCPUs).
>>
>>big vCPUs scheduled on big Physical CPUs and little vCPUs scheduled on little
>>physical cpus, right?
>>If it is, is there is a need to let Xen think all the cores has the same set
>>of features?
>
>I think you missed my point. The feature registers on big and little cores
>may be different. Currently, Xen is reading the feature registers of the CPU
>boot and wrongly assumes that those features will exists on all CPUs. This is
>not the case and should be fixed before we are getting in trouble.
>
>>
>>Developing big.little guest support, I am not sure how much efforts needed.
>>Is this really needed?
>
>This is not necessary at the moment, although I have seen some interest about
>it. Running a guest only on a little core is a nice beginning, but a guest
>may want to take advantage of big.LITTLE (running hungry app on big one and
>little on small one).
>
>>
>>>

This patchset is to use cpupool to block the vcpu be scheduled between big 
and
little cpus.

>
>See for instance v->arch.actlr = READ_SYSREG32(ACTLR_EL1).

Thanks for this. I only expose cpuid to guest, missed actlr. I'll check
the A53 and A72 TRM about AArch64 implementationd defined registers.
This actlr can be added to the cpupool_arch_info as midr.

Reading "vcpu_initialise", seems only MIDR and ACTLR needs to be handled.
Please advise if I missed anything else.
>>>
>>>Have you check the register emulation?
>>
>>Checked midr. Have not checked others.
>>I think I missed some registers in ctxt_switch_to.
>>
>>>

>
>>
>>Thinking an SoC with 4 A53(cpu[0-3]) + 2 A72(cpu[4-5]), cpu0 is the first 
>>one
>>that boots up. When XEN tries to bringup secondary CPUs, add cpu[0-3] to
>>cpupool0 and leave cpu[4-5] not in any cpupool. Then when Dom0 boots up,
>>`xl cpupool-list -c` will show cpu[0-3] in Pool-0.
>>
>>Then use the following script to create a new cpupool and add cpu[4-5] to
>>the cpupool.
>>#xl cpupool-create name=\"Pool-A72\" sched=\"credit2\"
>>#xl cpupool-cpu-add Pool-A72 4
>>#xl cpupool-cpu-add Pool-A72 5
>>#xl create -d /root/xen/domu-test pool=\"Pool-A72\"
>
>I am a bit confused with these runes. It means that only the first kind of
>CPUs have pool assigned. Why don't you

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 11:42:05AM +0100, Wei Liu wrote:
> On Mon, Sep 19, 2016 at 10:37:30AM +0100, Ian Jackson wrote:
> > osstest service owner writes ("[xtf test] 100874: all pass - PUSHED"):
> > > flight 100874 xtf real [real]
> > > http://logs.test-lab.xenproject.org/osstest/logs/100874/
> > > 
> > > Perfect :-)
> > 
> > Hooray.  I see this is running in xen-unstable now, as expected.
> > 
> > Just to check, are we expecting `xtf/test-pv64-xsa-167' to SKIP ?
> > 
> 
> Yes.
> 
> > The log says:
> >PV superpage support not detected
> > 
> > I assume this is not a missing hardware feature, but rather a missing
> > build option or something ?
> > 
> > https://wiki.xenproject.org/wiki/Xen_Project_Release_Features suggests
> > that superpages are "supported" so it would be good to have this test
> > working.
> > 
> 
> PV superpage is different from hvm superpage. Also that wiki page could
> be referring to hv support, I'm not sure.
> 
> As far as I can tell, PV superpage is a feature that nobody uses except
> for Oracle (?).

We don't use it anymore either. I had a patch to rip it out but .. I need
to dust it off.

> 
> Wei.
> 
> > Thanks,
> > Ian.
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [Xen-devel] [RFC 0/5] xen/arm: support big.little SoC

[Peng Fan]

On Mon, Sep 19, 2016 at 11:33:58AM +0100, George Dunlap wrote:
>On 19/09/16 11:06, Julien Grall wrote:
>> Hi George,
>> 
>> On 19/09/2016 11:45, George Dunlap wrote:
>>> On Mon, Sep 19, 2016 at 9:53 AM, Julien Grall 
>>> wrote:
>> As mentioned in the mail you pointed above, this series is not
>> enough to
>> make
>> big.LITTLE working on then. Xen is always using the boot CPU to detect
>> the
>> list of features. With big.LITTLE features may not be the same.
>>
>> And I would prefer to see Xen supporting big.LITTLE correctly before
>> beginning to think to expose big.LITTLE to the userspace (via cpupool)
>> automatically.
>
>
> Do you mean vcpus be scheduled between big and little cpus freely?


 By supporting big.LITTLE correctly I meant Xen thinks that all the
 cores has
 the same set of features. So the feature detection is only done the boot
 CPU. See processor_setup for instance...

 Moving vCPUs between big and little cores would be a hard task (cache
 line
 issue, and possibly feature) and I don't expect to ever cross this in
 Xen.
 However, I am expecting to see big.LITTLE exposed to the guest (i.e
 having
 big and little vCPUs).
>>>
>>> So it sounds like the big and LITTLE cores are architecturally
>>> different enough that software must be aware of which one it's running
>>> on?
>> 
>> That's correct. Each big and LITTLE cores may have different errata,
>> different features...
>> 
>> It has also the advantage to let the guest dealing itself with its own
>> power efficiency without introducing a specific Xen interface.
>
>Well in theory there would be advantages either way -- either to
>allowing Xen to automatically add power-saving "smarts" to guests which
>weren't programmed for them, or to exposing the power-saving abilities
>to guests which were.  But it sounds like automatically migrating
>between them isn't really an option (or would be a lot more trouble than
>it's worth).
>
 I care about having a design allowing an easy use of big.LITTLE on
 Xen. Your
 solution requires the administrator to know the underlying platform and
 create the pool.

 In the solution I suggested, the pools would be created by Xen (and
 the info
 exposed to the userspace for the admin).
>>>
>>> FWIW another approach could be the one taken by "xl
>>> cpupool-numa-split": you could have "xl cpupool-bigLITTLE-split" or
>>> something that would automatically set up the pools.
>>>
>>> But expanding the schedulers to know about different classes of cpus,
>>> and having vcpus specified as running only on specific types of pcpus,
>>> seems like a more flexible approach.
>> 
>> So, if I understand correctly, you would not recommend to extend the
>> number of CPU pool per domain, correct?
>
>Well imagine trying to set the scheduling parameters, such as weight,
>which in the past have been per-domain.  Now you have to specify
>parameters for a domain in each of the cpupools that its' in.
>
>No, I think it would be a lot simpler to just teach the scheduler about
>different classes of cpus.  credit1 would probably need to be modified
>so that its credit algorithm would be per-class rather than pool-wide;
>but credit2 shouldn't need much modification at all, other than to make
>sure that a given runqueue doesn't include more than one class; and to
>do load-balancing only with runqueues of the same class.

I try to follow.
 - scheduler needs to be aware of different classes of cpus. ARM big.Little 
cpus.
 - scheduler schedules vcpus on different physical cpus in one cpupool.
 - different cpu classes needs to be in different runqueue.

Then for implementation.
 - When create a guest, specific physical cpus that the guest will be run on.
 - If the physical cpus are different cpus, indicate the guest would like to be 
a big.little guest.
   And have big vcpus and little vcpus.
 - If no physical cpus specificed, then the guest may runs on big cpus or on 
little cpus. But not both.
   How to decide runs on big or little physical cpus?
 - For Dom0, I am still not sure,default big.little or else?

If use scheduler to handle the different classes cpu, we do not need to use 
cpupool
to block vcpus be scheduled onto different physical cpus. And using scheudler 
to handle this
gives an opportunity to support big.little guest.

Thanks,
Peng.

>
> -George

-- 

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

Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.

[Julien Grall]

Hi,

On 19/09/2016 11:27, Jan Beulich wrote:

On 16.09.16 at 18:38,  wrote:

--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf 
*elf,
 return true;
 }

+bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
+const struct livepatch_elf_sym *sym)
+{
+#ifdef CONFIG_ARM_32
+/*
+ * Xen does not use Thumb instructions - and we should not see any of
+ * them. If we do, abort.
+ */
+if ( sym->name && *sym->name == '$' && sym->name[1] == 't' )


Please use sym->name[0] for readability. Also, you may want to check the 
length of the symbol before checking the second character.




I'm not sure here: Are all symbols starting with $t to be rejected, or
only $t but not e.g. $txyz? According to some other code I have
lying around it ought to be "$t" and any symbols starting with "$t.",
and would be in line with patch 6. But I guess the ARM maintainers
will know best.


Only $t and $t.* should be rejected. All the others may be valid in the 
future.


Looking at the spec, I am wondering if we should also check the type and 
binding of the symbols. I have the impression that the naming is 
specific to STT_NOTYPE and STB_LOCAL. Any opinions?


Similar question for the previous patch (#6).

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 09/16] livepatch: tests: Make them compile under ARM64

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

 /* Our replacement function for xen_extra_version. */
 const char *xen_hello_world(void)
 {
+#ifdef CONFIG_X86
 unsigned long tmp;
 int rc;

@@ -24,7 +27,10 @@ const char *xen_hello_world(void)
  */
 rc = __get_user(tmp, non_canonical_addr);
 BUG_ON(rc != -EFAULT);
-
+#endif
+#ifdef CONFIG_ARM_64
+asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));


I would prefer if you invert the order between #9 and #10 to avoid 
introducing this dummy ALTERNATIVE.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED"):
> On Mon, Sep 19, 2016 at 04:56:01AM -0600, Jan Beulich wrote:
> > I guess the respective command line option ("allowsuperpage") was
> > not given?
> 
> Correct, that option is not given in osstest.

It should be, in the XTF tests at least, perhaps ?

Unless we're proposing to rip the feature out in stable branches.

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [xtf test] 100874: all pass - 
PUSHED"):
> We don't use it anymore either. I had a patch to rip it out but .. I need
> to dust it off.

That would be great, thanks.

Failing that, we could clarify the security support status.

Ian.

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

Re: [Xen-devel] [PATCH v4 10/16] livepatch: x86, ARM, alternative: Expose FEATURE_LIVEPATCH

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

To use as a common way of testing alternative patching for
livepatches. Both architectures have this FEATURE and the
test-cases can piggyback on that.

Suggested-by: Julien Grall 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: New submission
v4: Move the LIVEPATCH_FEATURE to asm-x86/livepatch.h
---
 xen/arch/arm/livepatch.c  | 3 +++
 xen/include/asm-arm/alternative.h | 2 ++
 xen/include/asm-arm/cpufeature.h  | 5 +
 xen/include/asm-x86/livepatch.h   | 1 +
 xen/test/livepatch/xen_hello_world_func.c | 8 +---
 5 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 13d6c10..b771cb7 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -8,6 +8,7 @@
 #include 
 #include 

+#include 
 #include 
 #include 

@@ -175,6 +176,8 @@ void __init arch_livepatch_init(void)
 end = (void *)LIVEPATCH_VMAP_END;

 vm_init_type(VMAP_XEN, start, end);
+
+cpus_set_cap(LIVEPATCH_FEATURE);
 }

 /*
diff --git a/xen/include/asm-arm/alternative.h 
b/xen/include/asm-arm/alternative.h
index 6851217..cca5f17 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -165,6 +165,8 @@ static inline int apply_alternatives(void *start, size_t 
lenght)
 return 0;
 }

+#define ALTERNATIVE(oldinstr, newinstr, ...) ""
+


Why this change? Nobody should ever use ALTERNATIVE when 
CONFIG_ALTERNATIVE is not enabled. This is a call to have a Xen not 
patched for a buggy hardware.


Not to mention that a void ALTERNATIVE is really buggy. We use 
ALTERNATIVE to switch between two set of instructions. So we always need 
at least one integrated in Xen.



 #endif

 #endif /* __ASM_ALTERNATIVE_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 66e930f..19e768b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,7 +40,12 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3

+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_FEATURE   4
+#define ARM_NCAPS   5
+#else
 #define ARM_NCAPS   4
+#endif


I would rather define the feature livepatch unconditionally to avoid the 
definition of ARM_NCAPS twice.



diff --git a/xen/test/livepatch/xen_hello_world_func.c 
b/xen/test/livepatch/xen_hello_world_func.c
index 6f53ab4..765a871 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -20,7 +20,6 @@ const char *xen_hello_world(void)
 unsigned long tmp;
 int rc;

-alternative(ASM_NOP8, ASM_NOP1, X86_FEATURE_LM);


Any reason to move the code later on?


 /*
  * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
  * exceptions will be caught and processed properly.
@@ -28,8 +27,11 @@ const char *xen_hello_world(void)
 rc = __get_user(tmp, non_canonical_addr);
 BUG_ON(rc != -EFAULT);
 #endif
-#ifdef CONFIG_ARM_64
-asm(ALTERNATIVE("nop", "nop", ARM64_WORKAROUND_CLEAN_CACHE));
+#ifdef CONFIG_ARM
+asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));


Ah, this is why you need a nop ALTERNATIVE. My comment above still 
stands, this definition of ALTERNATIVE maybe be valid for your use case, 
but it is invalid for most of the others.



+#endif
+#ifdef CONFIG_X86
+asm(ALTERNATIVE(ASM_NOP8, ASM_NOP1, LIVEPATCH_FEATURE));
 #endif
 return "Hello World";
 }



Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON"):
> AIUI BUILD_BUG_ON is a private thing to each library, hence the
> duplication. But I'm fine with having a central header for that, too.
> 
> If we want to share that macro across all libraries, we can create a
> tools/include/private.h.  But that would require a fair bit of work --
> adjust xen build system, fix stubdom build, fix mini-os etc -- which I
> think it would be best to leave to me.
> 
> In the meantime, we should unblock Paulina on this one so that her
> project can move forward.

Well, you asked for my opinion.  I don't feel strongly enough to block
this patch.  I don't have any other objection to it.

The problem with the "we should unblock" approach is that as each
successive clone of this code is introduced, the task of fixing it up
becomes harder, and the precedent for further clones becomes stronger.

(As far as the rules for committing are concerned, I think this patch
can be committed with your ack but without mine.)

Ian.

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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Wei Liu]

On Mon, Sep 19, 2016 at 02:46:11PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED"):
> > On Mon, Sep 19, 2016 at 04:56:01AM -0600, Jan Beulich wrote:
> > > I guess the respective command line option ("allowsuperpage") was
> > > not given?
> > 
> > Correct, that option is not given in osstest.
> 
> It should be, in the XTF tests at least, perhaps ?
> 

Adding that in would be trivial. But the value of testing that ...

> Unless we're proposing to rip the feature out in stable branches.
> 
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [xtf test] 100874: all pass - 
> PUSHED"):
> > We don't use it anymore either. I had a patch to rip it out but .. I need
> > to dust it off.
> 
> That would be great, thanks.
> 
> Failing that, we could clarify the security support status.
> 

... depends on the security support status of that particular
feature.

Wei.

> Ian.

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

Re: [Xen-devel] [PATCH v2] libs/gnttab: introduce XENGNTTAB_BUILD_BUG_ON

[Wei Liu]

On Mon, Sep 19, 2016 at 02:49:05PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2] libs/gnttab: introduce 
> XENGNTTAB_BUILD_BUG_ON"):
> > AIUI BUILD_BUG_ON is a private thing to each library, hence the
> > duplication. But I'm fine with having a central header for that, too.
> > 
> > If we want to share that macro across all libraries, we can create a
> > tools/include/private.h.  But that would require a fair bit of work --
> > adjust xen build system, fix stubdom build, fix mini-os etc -- which I
> > think it would be best to leave to me.
> > 
> > In the meantime, we should unblock Paulina on this one so that her
> > project can move forward.
> 
> Well, you asked for my opinion.  I don't feel strongly enough to block
> this patch.  I don't have any other objection to it.
> 
> The problem with the "we should unblock" approach is that as each
> successive clone of this code is introduced, the task of fixing it up
> becomes harder, and the precedent for further clones becomes stronger.
> 

I did try to fix all sort of things (this particular issue included) in
tools, stubdom and mini-os build system, but I didn't get to the point
to completely fix everything, unfortunately.

Previously attempt to fix leaking of BUILD_BUG_ON from mini-os resulted
in other breakage, then I ran out of my free cycles.

> (As far as the rules for committing are concerned, I think this patch
> can be committed with your ack but without mine.)
> 

I can't ack it, because I wrote this patch. :-)

Wei.

> Ian.

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

Re: [Xen-devel] [PATCH] Xen/timer: Disable watchdog during dumping timer queues

[Lan, Tianyu]

On 9/15/2016 10:32 PM, Jan Beulich wrote:

On 15.09.16 at 16:16,  wrote:

On 9/13/2016 11:25 PM, Jan Beulich wrote:

Wait - what is do_invalid_op() doing on the stack? I don't think it
belongs there, and hence I wonder whether the keypress
happened after some already fatal event (in which case all bets
are off anyway).


Not clear why do_invalid_op() on the stack. There is no other fatal
event. The issue disappears when set watchdog_timeout to 10s.


Another solution is to schedule a tasklet to run keyhandler in timer
handler and invoke process_pending_softirqs() in the dump_timerq().
This also works but it requires to rework keyhandler mechanism.

Disable watchdog seems to be simpler and I found dump_registers() also
used the same way to deal with the issue.

That's true. Just that on large machines it defaults to the
alternative model, for which I'm not sure it actually needs the
watchdog disabled (as data for a single CPU shouldn't exceed
the threshold).



It seems not to be necessary to disable watchdog in alternative model
since dumping a single cpu's status will not last a long time.


For the issue in the dump timer info handler, disabling watchdog is ok
for you or you have other suggestions to resolve the issue?


Well, without a clear understanding of why the issue occurs (for
which I need to refer you back to the questionable stack dump)
I'm hesitant to agree to this step, yet ...


After some researches, I found do_invalid_op() on the stack dump is
caused by run_in_exception_handler(__ns16550_poll) in the ns16550_poll()
rather than fatal event. The timeout issue still exists when run
__ns16550_poll() directly in the ns16550_poll().





I also found other places where dump a lot of logs disable watchdog.
(E,G run_all_keyhandlers(), debugtrace_dump() debugtrace_toggle() and so
on). This seems a common solution.


... I'm also not entirely against it considering the various other
examples. I.e. as almost always: As long as the need for the
change can be properly explained, I won't stand in the way of
getting it in.

Jan



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

Re: [Xen-devel] [PATCH v2 3/3] Significant changes to decision making; some new roles and minor changes

[Ian Jackson]

Lars Kurth writes ("Re: [PATCH v2 3/3] Significant changes to decision making; 
some new roles and minor changes"):
> I could, but it isn't really that long. And it makes life hard for me, as
> there is only one file. Thus breaking it into bits and maintaining the
> changes is hard in git.

Are you aware of `git add -i' ?

Breaking multiple changes to a single file, into separate patches, is
routine for us git users.  We'd be happy to help...

If you find it too difficult I would be happy to try to rework your
final patch into a series of smaller changes.

Ian.

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

Re: [Xen-devel] [PATCH v6 01/15] x86: properly calculate ELF end of image address

[Daniel Kiper]

On Mon, Sep 19, 2016 at 05:14:07AM -0600, Jan Beulich wrote:
> >>> On 16.09.16 at 22:43,  wrote:
> > On Mon, Sep 12, 2016 at 10:18:16PM +0200, Daniel Kiper wrote:
> >> Currently ELF end of image address is calculated using first line from
> >> "nm -nr xen/xen-syms" output. However, today usually it contains random
> >> symbol address not related to end of image in any way. So, it looks
> >
> > Weird. The -n says:
> >
> > "  -n
> >-v
> >--numeric-sort
> >Sort symbols numerically by their addresses, rather than
> > alphabetically by their names.
> > "
> >
> > And you are right. It is ignoring it:
> >
> > [konrad@char xen]$ nm -nr xen/xen-syms| sort | head -1
> > 82d08000 T __image_base__
> > [konrad@char xen]$ nm -nr xen/xen-syms | head -1
> > 82d08033d000 B efi
>
> So what is it ignoring, you think? The -n? Surely not. Are you perhaps
> overlooking that -r means "reverse" (and hence that piping through
> "sort" inverts all the sorting done by nm)?
>
> > [konrad@char xen]$ nm -nr xen/xen-syms| sort | tail -5
> > 82d08033cb00 B _end
> > 82d08033cb00 B __per_cpu_data_end
> > 82d08033d000 B __2M_rwdata_end
> > 82d08033d000 B efi
> >  U _GLOBAL_OFFSET_TABLE_
> >
> >> that for years that stuff have been working just by lucky coincidence.
> >> Hence, it have to be changed to something more reliable. So, let's take
> >> ELF end of image address by reading _end symbol address from nm output.
>
> So before taking this patch I'd really like to see proof that what gets
> done currently does actually go wrong in at least one case. So far I

During initial work on this patch series I discovered that p_memsz in xen
ELF file is set to unreasonably huge value, IIRC, ~1 GiB. That is why at
first I enforced 16 MiB here (just temporary workaround) and later proposed
this patch. Sadly, right now I am not able to find commit which introduced
this issue. However, it seems that it was "fixed" later by another commit.

Anyway, I am not sure why are you against, IMO, more reliable solution.
This way we would avoid in the future similar issues which I described above.

> can't see things working as "just by lucky coincidence". In particular
> I can't see why __2M_rwdata_end (aliased to efi) does not relate to
> the intended image end. Once we switch back to using large pages
> even when not using xen.efi I'd even question whether preferring
> _end over __2M_rwdata_end is actually correct.

This is good question. I was thinking about that after posting v6. It seems
that from image POV _end is correct. However, taking into account that Xen
image mapping covers 16 MiB then I think we should use __2M_rwdata_end (or
define __end_of_image__ symbol equal to __2M_rwdata_end). This way boot
loader will allocate memory region for Xen image later covered properly by
mapping, nothing will be put in memory immediately after the Xen image and
later incorrectly mapped as Xen image.

Daniel

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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 02:51:03PM +0100, Wei Liu wrote:
> On Mon, Sep 19, 2016 at 02:46:11PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED"):
> > > On Mon, Sep 19, 2016 at 04:56:01AM -0600, Jan Beulich wrote:
> > > > I guess the respective command line option ("allowsuperpage") was
> > > > not given?
> > > 
> > > Correct, that option is not given in osstest.
> > 
> > It should be, in the XTF tests at least, perhaps ?
> > 
> 
> Adding that in would be trivial. But the value of testing that ...
> 
> > Unless we're proposing to rip the feature out in stable branches.
> > 
> > Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [xtf test] 100874: all pass 
> > - PUSHED"):
> > > We don't use it anymore either. I had a patch to rip it out but .. I need
> > > to dust it off.
> > 
> > That would be great, thanks.
> > 
> > Failing that, we could clarify the security support status.
> > 
> 
> ... depends on the security support status of that particular
> feature.

To enable it you had to provide the 'allowsuperpage' parameter so by default
nobody would use it. Which means no security support (like the #UD traping
for cross-vendor migration).

I somehow assumed that any non-default enabled parameters by default would not
receive any security support.

Like running an 'debug=y' kernel or such?



> 
> Wei.
> 
> > Ian.

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

Re: [Xen-devel] [PATCH v4 01/16] arm/x86/common: Add HAS_[ALTERNATIVE|EX_TABLE]

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 11:26:19AM +0200, Julien Grall wrote:
> Hi Konrad,
> 
> On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:
> > x86 implements all of them by default - and we just
> > add two extra HAS_ variables to be declared in autoconf.h.
> > 
> > ARM 64 only has alternative while ARM 32 has none of them.
> > 
> > And while at it change the livepatch common code that
> > would benefit from this.
> > 
> > Suggested-by: Julien Grall 
> > Signed-off-by: Konrad Rzeszutek Wilk 
> > 
> > ---
> > Cc: Stefano Stabellini 
> > Cc: Julien Grall 
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> > Cc: Doug Goldstein 
> > 
> > v2: First submission
> > v3: Move the config options to common code
> > Don't include  in the file.
> > Don't even include  in the file as xen/Rules.mk 
> > automatically
> > includes the config.h for every GCC invocation.
> > v4: Depend on "arm64: s/ALTERNATIVE/HAS_ALTERNATIVE/"
> 
> I can't find this patch on the ML. Did you forget to send it?

Oh darn. Here it is inline:

From 13bc2e2f1082ac72e89c598443781b5141412e9a Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Tue, 13 Sep 2016 12:45:14 -0400
Subject: [PATCH] arm64: s/ALTERNATIVE/HAS_ALTERNATIVE/

No functional change. Also move the entries in the Kconfig file
to be more in alphabetical order.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 
---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Doug Goldstein 

v4: New submission.
---
 xen/arch/arm/Kconfig  | 8 
 xen/arch/arm/Makefile | 2 +-
 xen/arch/arm/xen.lds.S| 2 +-
 xen/include/asm-arm/alternative.h | 4 ++--
 xen/include/asm-arm/cpuerrata.h   | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 797c91f..b188293 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,8 +12,8 @@ config ARM_32
 config ARM_64
def_bool y
depends on 64BIT
+   select HAS_ALTERNATIVE
select HAS_GICV3
-   select ALTERNATIVE
 
 config ARM
def_bool y
@@ -42,16 +42,16 @@ config ACPI
  Advanced Configuration and Power Interface (ACPI) support for Xen is
  an alternative to device tree on ARM64.
 
-config HAS_GICV3
+config HAS_ALTERNATIVE
bool
 
-config ALTERNATIVE
+config HAS_GICV3
bool
 
 endmenu
 
 menu "ARM errata workaround via the alternative framework"
-   depends on ALTERNATIVE
+   depends on HAS_ALTERNATIVE
 
 config ARM64_ERRATUM_827319
bool "Cortex-A53: 827319: Data cache clean instructions might cause 
overlapping transactions to the interconnect"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 64fdf41..61e655b 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -4,7 +4,7 @@ subdir-y += platforms
 subdir-$(CONFIG_ARM_64) += efi
 subdir-$(CONFIG_ACPI) += acpi
 
-obj-$(CONFIG_ALTERNATIVE) += alternative.o
+obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
 obj-y += bootfdt.o
 obj-y += cpu.o
 obj-y += cpuerrata.o
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 3c5e7ba..47b910d 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -151,7 +151,7 @@ SECTIONS
*(.initcall1.init)
__initcall_end = .;
 
-#ifdef CONFIG_ALTERNATIVE
+#ifdef CONFIG_HAS_ALTERNATIVE
. = ALIGN(4);
__alt_instructions = .;
*(.altinstructions)
diff --git a/xen/include/asm-arm/alternative.h 
b/xen/include/asm-arm/alternative.h
index 9f88fd9..6851217 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -5,7 +5,7 @@
 #include 
 #include 
 
-#ifdef CONFIG_ALTERNATIVE
+#ifdef CONFIG_HAS_ALTERNATIVE
 
 #ifndef __ASSEMBLY__
 
@@ -154,7 +154,7 @@ int apply_alternatives(const struct alt_instr *start, const 
struct alt_instr *en
 #define ALTERNATIVE(oldinstr, newinstr, ...)   \
_ALTERNATIVE_CFG(oldinstr, newinstr, __VA_ARGS__, 1)
 
-#else /* !CONFIG_ALTERNATIVE */
+#else /* !CONFIG_HAS_ALTERNATIVE */
 
 static inline void apply_alternatives_all(void)
 {
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index 5e35b4f..8c57c6a 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -7,7 +7,7 @@
 
 void check_local_cpu_errata(void);
 
-#ifdef CONFIG_ALTERNATIVE
+#ifdef CONFIG_HAS_ALTERNATIVE
 
 #define CHECK_WORKAROUND_HELPER(erratum, feature, arch) \
 static inline bool_t check_workaround_##erratum(void)   \
@@ -27,7 +27,7 @@ static inline bool_t check_workaround_##erratum(void) 
  \
 }   \
 }
 
-#else /* CONFIG_ALTERNATIVE */
+#else /* CONFIG_HAS_ALTERNATIVE */
 
 #define CHECK_WORKAROUND_HELPER(erratum, feature, arch) \
 static inline bool_t check_workaround_##erratum(void)   \
-- 
2.4.11


___
Xen-devel mailing list
X

Re: [Xen-devel] trying to get started w/ osstest

[Ian Jackson]

Lai, Paul C writes ("trying to get started w/ osstest"):
> I?m looking to get started with osstest and running to some roadblocks.

Thanks for your interest.  Sorry for the late response - I've been
away.

> The page https://blog.xenproject.org/2013/02/02/
> xen-automatic-test-system-osstest/ looks like the place to begin, but
> 
> 1.   The README link returns 404 (file not found)
> 
> http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=refs/heads/
> standalone

The correct link is:

  http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=HEAD

That blog post is quite old now, and the `standalone' branch was
merged into master and deleted some time ago.

> 2.   When I try to clone the project via the https: URL, another file not
> found error returns.  The URL tried was https://xenbits.xen.org/git-http/
> osstest.git.

This was a configuration error which I have rectified.

It seems that several repos are bust in the same way.  I'm going to
ask our sysadmins to fix this.

> And it was taken from http://xenbits.xen.org/gitweb/?p=osstest.git;a=summary,

Right.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH v4 01/16] arm/x86/common: Add HAS_[ALTERNATIVE|EX_TABLE]

[Julien Grall]

Hi Konrad,

On 19/09/2016 16:04, Konrad Rzeszutek Wilk wrote:

On Mon, Sep 19, 2016 at 11:26:19AM +0200, Julien Grall wrote:

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

x86 implements all of them by default - and we just
add two extra HAS_ variables to be declared in autoconf.h.

ARM 64 only has alternative while ARM 32 has none of them.

And while at it change the livepatch common code that
would benefit from this.

Suggested-by: Julien Grall 
Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Doug Goldstein 

v2: First submission
v3: Move the config options to common code
Don't include  in the file.
Don't even include  in the file as xen/Rules.mk automatically
includes the config.h for every GCC invocation.
v4: Depend on "arm64: s/ALTERNATIVE/HAS_ALTERNATIVE/"


I can't find this patch on the ML. Did you forget to send it?


Oh darn. Here it is inline:

From 13bc2e2f1082ac72e89c598443781b5141412e9a Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Tue, 13 Sep 2016 12:45:14 -0400
Subject: [PATCH] arm64: s/ALTERNATIVE/HAS_ALTERNATIVE/

No functional change. Also move the entries in the Kconfig file
to be more in alphabetical order.

Suggested-by: Jan Beulich 
Signed-off-by: Konrad Rzeszutek Wilk 
---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: Doug Goldstein 

v4: New submission.
---
 xen/arch/arm/Kconfig  | 8 
 xen/arch/arm/Makefile | 2 +-
 xen/arch/arm/xen.lds.S| 2 +-
 xen/include/asm-arm/alternative.h | 4 ++--
 xen/include/asm-arm/cpuerrata.h   | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 797c91f..b188293 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,8 +12,8 @@ config ARM_32
 config ARM_64
def_bool y
depends on 64BIT
+   select HAS_ALTERNATIVE
select HAS_GICV3
-   select ALTERNATIVE

 config ARM
def_bool y
@@ -42,16 +42,16 @@ config ACPI
  Advanced Configuration and Power Interface (ACPI) support for Xen is
  an alternative to device tree on ARM64.

-config HAS_GICV3
+config HAS_ALTERNATIVE
bool

-config ALTERNATIVE
+config HAS_GICV3
bool


Why did you invert HAS_GICV3 and HAS_ALTERNATIVE? This makes the patch 
more difficult to read.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.

[Jan Beulich]

>>> On 19.09.16 at 15:33,  wrote:
> Hi,
> 
> On 19/09/2016 11:27, Jan Beulich wrote:
> On 16.09.16 at 18:38,  wrote:
>>> --- a/xen/arch/arm/livepatch.c
>>> +++ b/xen/arch/arm/livepatch.c
>>> @@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct 
>>> livepatch_elf 
> *elf,
>>>  return true;
>>>  }
>>>
>>> +bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
>>> +const struct livepatch_elf_sym *sym)
>>> +{
>>> +#ifdef CONFIG_ARM_32
>>> +/*
>>> + * Xen does not use Thumb instructions - and we should not see any of
>>> + * them. If we do, abort.
>>> + */
>>> +if ( sym->name && *sym->name == '$' && sym->name[1] == 't' )
> 
> Please use sym->name[0] for readability. Also, you may want to check the 
> length of the symbol before checking the second character.

Why would the length check be needed? If the first character is $,
then looking at the second one is always valid (and it being nul will
be properly dealt with by the expression above).

Jan


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

Re: [Xen-devel] [PATCH v4 07/16] livepatch/arm/x86: Check payload for for unwelcomed symbols.

[Julien Grall]

On 19/09/2016 16:11, Jan Beulich wrote:

On 19.09.16 at 15:33,  wrote:

On 19/09/2016 11:27, Jan Beulich wrote:

On 16.09.16 at 18:38,  wrote:

--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -117,6 +117,20 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf

*elf,

 return true;
 }

+bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf,
+const struct livepatch_elf_sym *sym)
+{
+#ifdef CONFIG_ARM_32
+/*
+ * Xen does not use Thumb instructions - and we should not see any of
+ * them. If we do, abort.
+ */
+if ( sym->name && *sym->name == '$' && sym->name[1] == 't' )


Please use sym->name[0] for readability. Also, you may want to check the
length of the symbol before checking the second character.


Why would the length check be needed? If the first character is $,
then looking at the second one is always valid (and it being nul will
be properly dealt with by the expression above).


Because you may have a payload which is not valid? Or maybe you consider 
that all the payload are trusted.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [xtf test] 100874: all pass - PUSHED

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [xtf test] 100874: all pass - 
PUSHED"):
> On Mon, Sep 19, 2016 at 02:51:03PM +0100, Wei Liu wrote:
> > ... depends on the security support status of that particular
> > feature.
> 
> To enable it you had to provide the 'allowsuperpage' parameter so by default
> nobody would use it. Which means no security support (like the #UD traping
> for cross-vendor migration).
> 
> I somehow assumed that any non-default enabled parameters by default
> would not receive any security support.
> 
> Like running an 'debug=y' kernel or such?

I can't find any documentation saying that security support is limited
to the default configuration.

The allowsuperpage command line option is documented in
docs/misc/xen-command-line.markdown and has no health warning.

Ian.

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

Re: [Xen-devel] [PATCH v4 03/16] arm: poison initmem when it is freed.

[Konrad Rzeszutek Wilk]

On Mon, Sep 19, 2016 at 11:35:57AM +0200, Julien Grall wrote:
> Hi Konrad,
> 
> On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:
> > The current byte sequence is '0xcc' which makes sense on x86,
> > but on ARM it is:
> > 
> > stclgt  12, cr12, [ip], {204}   ; 0xcc
> > 
> > Picking something more ARM applicable such as:
> > 
> > efefefefsvc 0x00efefef
> > 
> > Creates a nice crash if one executes that code:
> > (XEN) CPU1: Unexpected Trap: Supervisor Call
> > 
> > But unfortunatly that may not be a good choice either as in the future
> 
> s/unfortunatly/unfortunately/
> 
> > we may want to implement support for it.
> > 
> > Julien suggested that we use a 4-byte insn instruction instead
> > of trying to work with one byte.
> > 
> > As such on ARM 32 we use the udf instruction (see A8.8.247
> > in ARM DDI 0406C.c) and on ARM 64 use the AARCH64_BREAK_FAULT
> > instruction (aka brk instruction).
> > 
> > We don't have to worry about Thumb code so this instruction
> > is a safe to execute.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk 
> > ---
> > Cc: Julien Grall 
> > Cc: Stefano Stabellini 
> > 
> > v3: New submission
> > v4: Instead of using 0xef, use specific insn for architectures.
> > ---
> >  xen/arch/arm/mm.c | 17 -
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > index 07e2037..438bed7 100644
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -994,8 +994,23 @@ void free_init_memory(void)
> >  {
> >  paddr_t pa = virt_to_maddr(__init_begin);
> >  unsigned long len = __init_end - __init_begin;
> > +uint32_t insn;
> > +unsigned int i, nr = len / sizeof(insn);
> > +
> >  set_pte_flags_on_range(__init_begin, len, mg_rw);
> > -memset(__init_begin, 0xcc, len);
> > +#ifdef CONFIG_ARM_32
> > +/* udf instruction i.e (see A8.8.247 in ARM DDI 0406C.c) */
> > +insn = 0xe7f000f0;
> > +#else
> > +insn = AARCH64_BREAK_FAULT;
> > +#endif
> > +for ( i = 0; i < nr; i++ )
> > +*(__init_begin + i) = insn;
> 
> __init_begin is char[], so you will only copy the first byte of the
> instruction.
> 
> And because of nr = len / sizeof(insn) only 1/4 of the initmem will be
> poisoned.
> 
> So this should be something like:
> 
> uint32_t *p = (uint32_t *)__init_begin;
> for ( i = 0; i < nr; i++)
>*(p + i) = insn;
> 

Yes of course!
> > +
> > +nr = len % sizeof(insn);
> > +if ( nr )
> > +memset(__init_begin + len - nr, 0xcc, nr);
> 
> I am wondering if we should instead align __init_end to 4-byte. With a
> BUILD_BUG_ON in the code to check this assumption.

The __init_end is already aligned:

175   . = ALIGN(STACK_SIZE);

176   __init_end = .;

So we are good there, but I do like the BUILD_BUG_ON. Let me do that.
> 
> Any opinions?
> 
> > +
> >  set_pte_flags_on_range(__init_begin, len, mg_clear);
> >  init_domheap_pages(pa, pa + len);
> >  printk("Freed %ldkB init memory.\n", 
> > (long)(__init_end-__init_begin)>>10);
> > 
> 
> Regards,
> 
> -- 
> Julien Grall

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

Re: [Xen-devel] [PATCH v6 05/15] efi: create efi_enabled()

[Daniel Kiper]

On Mon, Sep 19, 2016 at 05:58:46AM -0600, Jan Beulich wrote:
> >>> On 12.09.16 at 22:18,  wrote:
> > --- a/xen/arch/x86/domain_page.c
> > +++ b/xen/arch/x86/domain_page.c
> > @@ -36,7 +36,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
> >   * domain's page tables but current may point at another domain's VCPU.
> >   * Return NULL as though current is not properly set up yet.
> >   */
> > -if ( efi_enabled && efi_rs_using_pgtables() )
> > +if ( efi_enabled(EFI_RS) && efi_rs_using_pgtables() )
>
> I think the efi_enabled() here is pointless now.

Nope, it seems that Xen will blow up on BUG() in
xen/arch/x86/efi/stub.c:efi_rs_using_pgtables() if
compiler/linker cannot be used to build proper PE binary.
Of course we can change efi_rs_using_pgtables() to
return false in such case.

> With this dropped (unless you know of a reason that it needs to stay)
> Reviewed-by: Jan Beulich 

Thanks a lot!

By the way, do you see this patch series (as whole or
at least partially) in 4.8? Should I repost them before
hard freeze? However, I am not sure that it (v7) can be
taken into 4.8 because we are after last posting date.

Wei, Jan, what is your standing in that case?

Daniel

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

Re: [Xen-devel] [PATCH 3/5] xen: replace TEST_COVERAGE with CONFIG_GCOV

[Ian Jackson]

Wei Liu writes ("[PATCH 3/5] xen: replace TEST_COVERAGE with CONFIG_GCOV"):
> The sole purpose of TEST_COVERAGE macro is to guard the availability of
> gcov sysctl. Now we have a proper CONFIG_GCOV, use it.

FAOD my reading of xen/Kconfig.debug is that CONFIG_GCOV depends on
CONFIG_DEBUG which says

 This option is intended for development purposes
 only, and not for production use.

and is therefore out of security support.

Accordingly I think the only security review needed for the hypervisor
part is to check that everything is indeed disabled without
CONFIG_GCOV.

Thanks,
Ian.

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

[Xen-devel] [xen-unstable-smoke test] 101011: tolerable all pass - PUSHED

[osstest service owner]

flight 101011 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/101011/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  de4e4fd12bdca17cb3511e9bfc8f2c1e94c09a7d
baseline version:
 xen  6bd621d7010bb8561196aedf5198d5fe7c146822

Last test of basis   101009  2016-09-19 10:01:51 Z0 days
Testing same since   101011  2016-09-19 12:00:53 Z0 days1 attempts


People who touched revisions under test:
  Olaf Hering 
  Wei Liu 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=de4e4fd12bdca17cb3511e9bfc8f2c1e94c09a7d
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
de4e4fd12bdca17cb3511e9bfc8f2c1e94c09a7d
+ branch=xen-unstable-smoke
+ revision=de4e4fd12bdca17cb3511e9bfc8f2c1e94c09a7d
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' xde4e4fd12bdca17cb3511e9bfc8f2c1e94c09a7d = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/xtf.git
++ : osst...@xenbits.xen.org:/home/xen/git/xtf.git
++ : git://xenbits.xen.org/xtf.git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : git
++ : git://xenbits.xen.org/osstest/rumprun.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/rumprun.git
++ : git://git.seabios.org/seabios.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git
++ : git://xenbits.xen.org/osstest/seabios.git
++ : https://github.com/tianocore/edk2.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/linux-firmware.git
++ : osst...@xenbits.xen.org:/home/osstest/ext/linux-firmware.git
++ : git://git.kernel.org/pub/scm/linux/kernel/gi

Re: [Xen-devel] [PATCH v4 04/16] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk]

> 
> >  void arch_livepatch_revive(void)
> >  {
> > +/*
> > + * Nuke the instruction cache. Data cache has been cleaned before in
> > + * arch_livepatch_apply_jmp.
> 
> I think you forgot to clean text region from the payload. Without that, you
> may receive a crash if you have a separate cache for data and instruction.

Help me out here please.

Why would we need to call clean_and_invalidate_dcache_va_range on the
payload .text area (the func->new_addr and func->new_size)?

We don't modify that .text area and after this function is done
(arch_livepatch_revive) it would be very first time that code would be called.

Hence there would not be any cache remains at all? 

Or did you mean the old_addr (the one we just patched?)

If we are reverting it then we just clear at func->old_addr for one
instruction? We could drop the dcache for the func->new_addr (so new
.text code), to explicitly tell the CPU to not waste cache space for
those instructions? Is that what you meant?

Anyhow did this:

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 49eb69b..07f0ce7 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -49,7 +49,10 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
 for ( i = 0; i < len; i++ )
 *(new_ptr + i) = insn;
 
+/* There should not be _any_ aliasing using vmap's, but just in case. */
 clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) * len);
+/* And definitly clear the old code. */
+clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr) * 
len);
 }
 
 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
@@ -68,6 +71,9 @@ void arch_livepatch_revert_jmp(const struct livepatch_func 
*func)
 *(new_ptr + i) = insn;
 }
 
+/* There should not be _any_ aliasing using vmap's, but just in case. */
+clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) * len);
+/* And definitly clear the old code. */
 clean_and_invalidate_dcache_va_range(func->old_addr, sizeof(*new_ptr) * 
len);
 }

And added the invalidation of dcache at old_addr (so the function we
patched).

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

Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.

[Julien Grall]

On 19/09/2016 16:34, Julien Grall wrote:

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

Most of the WARN_ON or BUG_ON sections are properly aligned on
x86. However on ARM and on x86 assembler the macros don't include
any aligment information - hence they end up being the default


s/aligment/alignment/


byte granularity.

On ARM32 it is paramount that the aligment is word-size (4)


Ditto


otherwise if one tries to use (uint32_t*) access (such
as livepatch ELF relocations) we get a Data Abort.

Enforcing bug_frames to have the proper aligment across all


Ditto


architectures and in both C and x86 makes them all the same.

Furthermore on x86 the bloat-o-meter detects that with this
change:

add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
function old new   delta

On ARM32:
add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96)
function old new   delta
gnttab_unpopulate_status_frames- 384+384
do_grant_table_op  10808   10520-288

And ARM64:
add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72)
function old new   delta
gnttab_map_grant_ref   -4164   +4164
do_grant_table_op   98929836 -56
grant_map_exists 300   --300
__gnttab_map_grant_ref  3880   -   -3880

Signed-off-by: Konrad Rzeszutek Wilk 


I forgot to mention that with those NITs fixed:

Reviewed-by: Julien Grall 


---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: First submission. Replaces the "livepatch/elf: Adjust section
aligment to word"
patch.
v4: Remove the .ALIGN(4) in xen.lds.S for x86 (the only place needing
that change).
Update the commit description with correct x86 results
Remove the . = ALIGN(4); in linker filer on x86 [the only file
needing the change]
---
 xen/arch/x86/xen.lds.S| 1 -
 xen/include/asm-arm/bug.h | 1 +
 xen/include/asm-x86/bug.h | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d903c31..7676de9 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -79,7 +79,6 @@ SECTIONS
   .rodata : {
_srodata = .;
/* Bug frames table */
-   . = ALIGN(4);
__start_bug_frames = .;
*(.bug_frames.0)
__stop_bug_frames_0 = .;
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 68353e1..773d63e 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -52,6 +52,7 @@ struct bug_frame {

".popsection\n"\
  ".pushsection .bug_frames." __stringify(type) ", \"a\",
%progbits\n"\

"4:\n" \
+ ".align
4\n"   \
  ".long (1b -
4b)\n"\
  ".long (2b -
4b)\n"\
  ".long (3b -
4b)\n"\
diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index c5d2d4c..9bb4a19 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -98,6 +98,7 @@ extern const struct bug_frame __start_bug_frames[],
 .popsection

 .pushsection .bug_frames.\type, "a", @progbits
+.p2align 2
 .L\@bf:
 .long (.L\@ud - .L\@bf) + \
((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)



Regards,



--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 12/16] bug/x86/arm: Align bug_frames sections.

[Julien Grall]

Hi Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

Most of the WARN_ON or BUG_ON sections are properly aligned on
x86. However on ARM and on x86 assembler the macros don't include
any aligment information - hence they end up being the default


s/aligment/alignment/


byte granularity.

On ARM32 it is paramount that the aligment is word-size (4)


Ditto


otherwise if one tries to use (uint32_t*) access (such
as livepatch ELF relocations) we get a Data Abort.

Enforcing bug_frames to have the proper aligment across all


Ditto


architectures and in both C and x86 makes them all the same.

Furthermore on x86 the bloat-o-meter detects that with this
change:

add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
function old new   delta

On ARM32:
add/remove: 1/0 grow/shrink: 0/1 up/down: 384/-288 (96)
function old new   delta
gnttab_unpopulate_status_frames- 384+384
do_grant_table_op  10808   10520-288

And ARM64:
add/remove: 1/2 grow/shrink: 0/1 up/down: 4164/-4236 (-72)
function old new   delta
gnttab_map_grant_ref   -4164   +4164
do_grant_table_op   98929836 -56
grant_map_exists 300   --300
__gnttab_map_grant_ref  3880   -   -3880

Signed-off-by: Konrad Rzeszutek Wilk 
---
Cc: Julien Grall 
Cc: Stefano Stabellini 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v3: First submission. Replaces the "livepatch/elf: Adjust section aligment to 
word"
patch.
v4: Remove the .ALIGN(4) in xen.lds.S for x86 (the only place needing
that change).
Update the commit description with correct x86 results
Remove the . = ALIGN(4); in linker filer on x86 [the only file needing the 
change]
---
 xen/arch/x86/xen.lds.S| 1 -
 xen/include/asm-arm/bug.h | 1 +
 xen/include/asm-x86/bug.h | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d903c31..7676de9 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -79,7 +79,6 @@ SECTIONS
   .rodata : {
_srodata = .;
/* Bug frames table */
-   . = ALIGN(4);
__start_bug_frames = .;
*(.bug_frames.0)
__stop_bug_frames_0 = .;
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 68353e1..773d63e 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -52,6 +52,7 @@ struct bug_frame {
  ".popsection\n"\
  ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\
  "4:\n" \
+ ".align 4\n"   \
  ".long (1b - 4b)\n"\
  ".long (2b - 4b)\n"\
  ".long (3b - 4b)\n"\
diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index c5d2d4c..9bb4a19 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -98,6 +98,7 @@ extern const struct bug_frame __start_bug_frames[],
 .popsection

 .pushsection .bug_frames.\type, "a", @progbits
+.p2align 2
 .L\@bf:
 .long (.L\@ud - .L\@bf) + \
((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)



Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 13/16] livepatch: Initial ARM32 support.

[Julien Grall]

Hello Konrad,

On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote:

The patch piggybacks on: livepatch: Initial ARM64 support, which
brings up all of the neccessary livepatch infrastructure pieces in.


s/neccessary/necessary/



This patch adds three major pieces:

 1) ELF relocations. ARM32 uses SHT_REL instead of SHT_RELA which
means the adddendum had to be extracted from within the
instruction. Which required parsing BL/BLX, B/BL,
MOVT, and MOVW instructions.

The code was written from scratch using the ARM ELF manual
(and the ARM Architecture Reference Manual)

 2) Inserting an trampoline. We use the B (branch to address)
which uses an offset that is based on the PC value: PC + imm32.
Because we insert the branch at the start of the old function
we have to account for the instruction already being fetched
and subtract -8 from the delta (new_addr - old_addr). See
ARM DDI 0406C.c, see A2.3 (pg 45) and A8.8.18 pg (pg 334,335)

 3) Allows the test-cases to be built under ARM 32.
The "livepatch: tests: Make them compile under ARM64"
put in the right infrastructure for it and we piggyback on it.

Acked-by: Jan Beulich  [for non-ARM parts]
Signed-off-by: Konrad Rzeszutek Wilk 


Acked-by: Julien Grall 

Regards,

--
Julien Grall

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