[Xen-devel] [PATCH v4] xen/arm32: Introduce alternative runtime patching

[Wei Chen]

This patch is based on the implementation of ARM64, it introduces
alternative runtime patching to ARM32. This allows to patch assembly
instruction at runtime to either fix hardware bugs or optimize for
certain hardware features on ARM32 platform.

Xen hypervisor is using ARM execution state only on ARM32 platform,
Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
TBB and TBH) are not considered in alternatives.

The left ARM32 branch instructions are BX, BLX, BL and B. The
instruction BX is taking a register in parameter, so we don't need to
rewrite it. The instructions BLX, BL and B are using the similar
encoding for the offset and will avoid specific case when extracting
and updating the offset.

In this patch, we include alternative.h header file to livepatch.c
directly for ARM32 compilation issues. When the alternative patching
config is enabled, the livepatch.c will use the alternative functions.
In this case, we should include the alternative header file to this
file. But for ARM64, it does not include this header file directly.
It includes this header file indirectly through:
sched.h->domain.h->page.h->alternative.h.
But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
and we don't have the reason to include it to ARM32 page.h now. So we
have to include the alternative.h directly in livepatch.c.

Signed-off-by: Wei Chen 
Reviewed-by: Konrad Rzeszutek Wilk 
---
v3->v4:
1. Use open-code to replace the macro __AARCH32_INSN_FUNCS
2. Add code comment to explain why we can use one check to catch
   all the branch instructions we should handle.
---
 xen/arch/arm/Kconfig |  2 +-
 xen/arch/arm/arm32/Makefile  |  1 +
 xen/arch/arm/arm32/insn.c| 91 
 xen/common/livepatch.c   |  1 +
 xen/include/asm-arm/arm32/insn.h | 71 +++
 xen/include/asm-arm/insn.h   |  2 +
 6 files changed, 167 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm32/insn.c
 create mode 100644 xen/include/asm-arm/arm32/insn.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2e023d1..43123e6 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,11 +12,11 @@ config ARM_32
 config ARM_64
def_bool y
depends on 64BIT
-   select HAS_ALTERNATIVE
select HAS_GICV3
 
 config ARM
def_bool y
+   select HAS_ALTERNATIVE
select HAS_ARM_HDLCD
select HAS_DEVICE_TREE
select HAS_MEM_ACCESS
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 4395693..0ac254f 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-y += insn.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += proc-v7.o proc-caxx.o
 obj-y += smpboot.o
diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
new file mode 100644
index 000..7a5dbc5
--- /dev/null
+++ b/xen/arch/arm/arm32/insn.c
@@ -0,0 +1,91 @@
+/*
+  * Copyright (C) 2017 ARM Ltd.
+  *
+  * This program is free software; you can redistribute it and/or modify
+  * it under the terms of the GNU General Public License version 2 as
+  * published by the Free Software Foundation.
+  *
+  * 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 .
+  */
+#include 
+#include 
+#include 
+#include 
+
+/* Mask of branch instructions' immediate. */
+#define BRANCH_INSN_IMM_MASKGENMASK(23, 0)
+/* Shift of branch instructions' immediate. */
+#define BRANCH_INSN_IMM_SHIFT   0
+
+static uint32_t branch_insn_encode_immediate(uint32_t insn, int32_t offset)
+{
+uint32_t imm;
+
+/*
+ * Encode the offset to imm. All ARM32 instructions must be word aligned.
+ * Therefore the offset value's bits [1:0] equal to zero.
+ * (see ARM DDI 0406C.c A8.8.18/A8.8.25 for more encode/decode details
+ * about ARM32 branch instructions)
+ */
+imm = ((offset >> 2) & BRANCH_INSN_IMM_MASK) << BRANCH_INSN_IMM_SHIFT;
+
+/* Update the immediate field. */
+insn &= ~(BRANCH_INSN_IMM_MASK << BRANCH_INSN_IMM_SHIFT);
+insn |= imm;
+
+return insn;
+}
+
+/*
+ * Decode the branch offset from a branch instruction's imm field.
+ * The branch offset is a signed value, so it can be used to compute
+ * a new branch target.
+ */
+int32_t aarch32_get_branch_offset(uint32_t insn)
+{
+uint32_t imm;
+
+/* Retrieve imm from branch instruction. */
+imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & BRANCH_INSN_IMM_MASK;
+
+/*
+ * Check the imm signed bit. If the imm is a negative value, we

Re: [Xen-devel] [RFC PATCH v3 1/2] xen: credit2: enable per cpu runqueue creation

[Dario Faggioli]

Hello Praveen,

On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> The patch introduces a new command line option 'cpu' that when used
> will
> create runqueue per logical pCPU.
> 
I'd add a quick note about why we think it's a good idea to have this.
Something like:

"This may be useful for small systems, and also for development,
performance evaluation and comparison."

> Signed-off-by: Praveen Kumar 
>
With that addition, at least this patch can have my:

Reviewed-by: Dario Faggioli 

Thanks and Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [ovmf baseline-only test] 71129: all pass

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 71129 ovmf real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/71129/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf 251779fca719bf9ea00505ee7629c08b452c150d
baseline version:
 ovmf 072060a6f81b3e43473b9e5dcba7049ad9de4b18

Last test of basis71128  2017-03-30 23:47:27 Z0 days
Testing same since71129  2017-03-31 05:17:20 Z0 days1 attempts


People who touched revisions under test:
  Jiewen Yao 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



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.


commit 251779fca719bf9ea00505ee7629c08b452c150d
Author: Jiewen Yao 
Date:   Mon Mar 27 23:01:02 2017 +0800

MdeModulePkg/SmmCore: Fix memory leak on Profile unregistered.

Issue reported at bugzillar 445.

Cc: Jeff Fan 
Cc: Feng Tian 
Cc: Star Zeng 
Cc: Bret Barkelew 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
Reviewed-by: Jeff Fan 

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

Re: [Xen-devel] [PATCH v3 1/9] x86/mce: handle LMCE locally

[Jan Beulich]

>>> On 31.03.17 at 04:34,  wrote:
> On 03/30/17 08:35 -0600, Jan Beulich wrote:
>> >>> On 30.03.17 at 08:19,  wrote:
>> > --- a/xen/arch/x86/cpu/mcheck/mce.c
>> > +++ b/xen/arch/x86/cpu/mcheck/mce.c
>> > @@ -42,6 +42,13 @@ DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, 
>> > poll_bankmask);
>> >  DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, no_cmci_banks);
>> >  DEFINE_PER_CPU_READ_MOSTLY(struct mca_banks *, mce_clear_banks);
>> >  
>> > +/*
>> > + * Flag to indicate that at least one non-local MCE on this CPU has
>> > + * not been completed handled. It's set by mcheck_cmn_handler() and
>> > + * cleared by mce_softirq().
>> > + */
>> > +static DEFINE_PER_CPU(bool, nonlocal_mce_in_progress);
>> 
>> Does a boolean really suffice here? I.e. can't there be a 2nd one
>> while the first one is still being dealt with?
> 
> It's to indicate the existence of a non-local MCE, and mce_softirq()
> will call mctelem_process_deferred() to handle all MCE's if it's true,
> so a boolean flag is suffice.

I don't buy this, I'm sorry. What if a 2nd #MC occurs at the
instruction boundary of setting the variable to false (in
mce_softirq())? A subsequent mce_softirq() would then see
the variable being false, wouldn't it?

Jan


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

Re: [Xen-devel] [PATCH v2] x86/emul: Poision the stubs with debug traps

[Jan Beulich]

>>> On 30.03.17 at 19:09,  wrote:
> If you do have an external debugger attached, you don't need embedded
> int3's to start with, and you'd still hit the debugger_trap_*() calls
> before dying.

Well, my experience (on other OSes with a better debugger) is
different - there are occasions where it is easier to embed an int3
right away than to break into the system (doing of which may be
a problem of its own if you need the breakpoint quite early during
boot), go hunt for the precise instruction you want the breakpoint
on, and then set the intended breakpoint using debugger facilities.

> If you don't have an external debugger attached, ignoring int3's is very
> poor behaviour.

Well, that's your opinion. Mine (again based on past experience) is
different, and it appears to match that of whoever originally wrote
things this way. Nevertheless I appreciate the (assumed) reasoning
behind your way of thinking.

Jan



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

Re: [Xen-devel] [PATCH RFC] x86/emulate: implement hvmemul_cmpxchg() with an actual CMPXCHG

[Jan Beulich]

>>> On 31.03.17 at 08:17,  wrote:
> On 03/30/2017 06:47 PM, Jan Beulich wrote:
>>> Speaking of emulated MMIO, I've got this when the guest was crashing
>>> immediately (pre RETRY loop):
>>>
>>>  MMIO emulation failed: d3v8 32bit @ 0008:82679f3c -> f0 0f ba 30 00 72
>>> 07 8b cb e8 da 4b ff ff 8b 45
>> 
>> That's a BTR, which we should be emulating fine. More information
>> would need to be collected to have a chance to understand what
>> might be going one (first of all the virtual and physical memory
>> address this was trying to act on).
> 
> Right, the BTR part should be fine, but I think the LOCK part is what's
> causing the issue. I've done a few more test runs to see what return
> RETRY (dumping the instruction with an "(r)" prefix to distinguish from
> the UNHANDLEABLE dump), and a couple of instructions return RETRY (BTR
> and XADD, both LOCK-prefixed, which means they now involve CMPXCHG
> handler, which presumably now fails - possibly simply because it's
> always LOCKed in my patch):

Well, all of that looks to be expected behavior. I'm afraid I don't see
how this information helps understanding the MMIO emulation failure
above.

Jan


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

Re: [Xen-devel] [PATCH for-4.9 2/4] tools/libxc: Avoid generating inappropriate zero-content records

[Jan Beulich]

>>> On 30.03.17 at 18:32,  wrote:
> --- a/tools/libxc/xc_sr_save_x86_hvm.c
> +++ b/tools/libxc/xc_sr_save_x86_hvm.c
> @@ -112,6 +112,10 @@ static int write_hvm_params(struct xc_sr_context *ctx)
>  }
>  }
>  
> +/* No params? Skip this record. */
> +if ( hdr.count == 0 )
> +return 0;

Purely out of curiosity - under what conditions would this happen?
Some of the params in the array look like they would always have
a non-zero value.

Jan


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

Re: [Xen-devel] [PATCH v2] x86: suppress duplicate symbol warnings for CONFIG_GCOV

[Ross Lagerwall]

On 03/30/2017 05:12 PM, Jan Beulich wrote:

There are quite a few of these, and as the option is a development one
only, duplicate symbol names should not be an issue there. In other
environments allow the user to control this, unless Live patching is
enabled.

Signed-off-by: Jan Beulich 


Reviewed-by: Ross Lagerwall 

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

Re: [Xen-devel] [PATCH for-4.9 4/4] docs: Clarify the expected behaviour of zero-content records

[Jan Beulich]

>>> On 30.03.17 at 18:32,  wrote:
> @@ -631,6 +631,11 @@ The set of valid records depends on the guest 
> architecture and type.  No
>  assumptions should be made about the ordering or interleaving of
>  independent records.  Record dependencies are noted below.
>  
> +Some records are used for signalling, and explicitly have zero length.  All
> +other records contain data relevent to the migration.  Data records with no

relevant?

> +content should be elided on the source side, as they their presence serves no

Stray "they"?

> +purpose, but result in extra work for the restore side.

results?

> @@ -719,3 +724,12 @@ restored.
>  The image header may only be extended by _appending_ additional
>  fields.  In particular, the `marker`, `id` and `version` fields must
>  never change size or location.
> +
> +
> +Errata
> +==
> +
> +1. For compatibility with older code, the receving side of a stream should
> +   tolerate and ignore variable sized records with zero content.  Xen 
> releases
> +   between 4.6 and 4.8 could end up generating valid HVM\_PARAMS or
> +   X86\_PV\_VCPU\_{EXTENDED,XSAVE,MSRS} records with 0 content.

Also elsewhere in the series you use expressions similar to this "0
content", but especially here (with no code next to it) it is rather
ambiguous: Do you mean zero-length content, or non-zero-length
content being all zero, or both?

Jan


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

Re: [Xen-devel] [PATCH] xen, kdump: handle pv domain in paddr_vmcoreinfo_note()

[Juergen Gross]

On 30/03/17 17:05, Jan Beulich wrote:
 On 30.03.17 at 16:18,  wrote:
>> @@ -2903,3 +2906,13 @@ int xen_unmap_domain_gfn_range(struct vm_area_struct 
>> *vma,
>>  return -EINVAL;
>>  }
>>  EXPORT_SYMBOL_GPL(xen_unmap_domain_gfn_range);
>> +
>> +#ifdef CONFIG_KEXEC_CORE
>> +phys_addr_t paddr_vmcoreinfo_note(void)
>> +{
>> +if (xen_pv_domain())
>> +return virt_to_machine(&vmcoreinfo_note).maddr;
>> +else
>> +return __pa((unsigned long)(char *)&vmcoreinfo_note);
> 
> I don't think you need the double cast here.
> 
> This being placed in x86 code is correct only as long as the
> assumption is correct that no other architecture will allow for
> PV guests. And this being placed in Xen code is correct only
> as long as the assumption is true that no other hypervisors
> will allow for PV guests.

With virt_to_machine() defined in arch/x86/include/asm/xen/page.h
I think changing any of those assumptions will require major work.


Juergen

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

Re: [Xen-devel] [PATCH] xen, kdump: handle pv domain in paddr_vmcoreinfo_note()

[Juergen Gross]

On 30/03/17 16:51, Boris Ostrovsky wrote:
> On 03/30/2017 10:18 AM, Juergen Gross wrote:
>> For kdump to work correctly it needs the physical address of
>> vmcoreinfo_note. When running as dom0 this means the virtual address
>> has to be translated to the related machine address.
>>
>> paddr_vmcoreinfo_note() is meant to do the translation via __pa() only,
>> but being attributed "weak" it can be replaced easily in Xen case.
>>
>> Signed-off-by: Juergen Gross 
>> ---
>> This patch needs to be rebased on top of Vitaly's series to split
>> pv- and hvm-code. I'll do this as soon as his series is in the Xen
>> tree in its final form.
>> ---
>>  arch/x86/xen/mmu.c | 13 +
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
>> index 37cb5aa..0e2b8d7 100644
>> --- a/arch/x86/xen/mmu.c
>> +++ b/arch/x86/xen/mmu.c
>> @@ -49,6 +49,9 @@
>>  #include 
>>  #include 
>>  #include 
>> +#ifdef CONFIG_KEXEC_CORE
>> +#include 
>> +#endif
>>  
>>  #include 
>>  
>> @@ -2903,3 +2906,13 @@ int xen_unmap_domain_gfn_range(struct vm_area_struct 
>> *vma,
>>  return -EINVAL;
>>  }
>>  EXPORT_SYMBOL_GPL(xen_unmap_domain_gfn_range);
>> +
>> +#ifdef CONFIG_KEXEC_CORE
>> +phys_addr_t paddr_vmcoreinfo_note(void)
>> +{
>> +if (xen_pv_domain())
>> +return virt_to_machine(&vmcoreinfo_note).maddr;
>> +else
>> +return __pa((unsigned long)(char *)&vmcoreinfo_note);
> 
> Why not __pa_symbol(), just like in the weak version?

Hmm, you are right. This patch originated from a 4.4 based one.


Juergen


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

Re: [Xen-devel] Legacy PCI interrupt {de}assertion count

[Roger Pau Monné]

On Fri, Mar 31, 2017 at 05:05:44AM +, Tian, Kevin wrote:
> > From: Jan Beulich [mailto:jbeul...@suse.com]
> > Sent: Monday, March 27, 2017 4:00 PM
> > 
> > >>> On 24.03.17 at 17:54,  wrote:
> > > As I understand it, for level triggered legacy PCI interrupts Xen sets
> > > up a timer in order to perform the EOI if the guest takes too long in
> > > deasserting the line. This is done in pt_irq_time_out. What I don't
> > > understand is why this function also does a deassertion of the guest view
> > of the PCI interrupt, ie:
> > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending
> > > assert in the guest, and thus the guest will end up loosing one interrupt.
> > 
> > Especially with the comment next to the respective set_timer() it looks to 
> > me
> > as if this was the intended effect: If the guest didn't care to at least 
> > start
> > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost in
> > order to not have it block other interrupts inside the guest (i.e. there's 
> > more
> > to it than just guarding the host here).
> > 
> > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared
> > interrupts") introducing this has no description at all. Let's see if Kevin
> > remembers any further details ...
> > 
> 
> Sorry I don't remember more detail other than existing comments.
> Roger, did you encounter a problem now?

No, I didn't encounter any problems with this so far, any well behaved guest
will deassert those lines anyway, it just seems to be against the spec.  AFAIK
on bare metal the line will be asserted until the OS deasserts it, so I was
wondering if this was some kind of workaround?

Note that the physical line is already automatically deasserted (on the PIRQ
layer), so this just deasserts the guest line which IMHO Xen shouldn't care at
all (at the end the guest is free to not deassert it, and it shouldn't affect
Xen at all).

Roger.

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

Re: [Xen-devel] [PATCH v11 2/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt

[Jan Beulich]

>>> On 31.03.17 at 01:01,  wrote:
> On Fri, Mar 31, 2017 at 01:46:33PM +0800, Tian, Kevin wrote:
>>> From: Gao, Chao
>>> Sent: Wednesday, March 29, 2017 1:12 PM
>>> --- a/xen/drivers/passthrough/io.c
>>> +++ b/xen/drivers/passthrough/io.c
>>> @@ -618,6 +618,8 @@ int pt_irq_destroy_bind(
>>>  else
>>>  what = "bogus";
>>>  }
>>> +else if ( iommu_intpost && pirq && pirq_dpci->gmsi.posted )
>>
>>No need to check iommu_intpost. Just keep later conditions.
> 
> ok. Is it for if posted is set, the iommu_intpost should be true?
> 
>>
>>btw isn't the condition be " (pirq_dpci && pirq_dpci->gmsi.posted)"?
> 
> I don't think so. pirq, not pirq_dpci, is consumed by pi_update_irte.
> Furthermore if pirq_dpci is null, pirq is also null. But it is not true
> in turn.

With

pirq_dpci = pirq_dpci(pirq);

it _is_ the other way around, which can then also be expressed
as "if pirq_dpci is non-NULL, pirq is non-NULL too", which is the
precondition you need to consume (dereference) pirq. There is
a reason other code around here also only ever checks pirq_dpci
(even when using pirq).

Jan


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

Re: [Xen-devel] arm64: dma_to_phys/phys_to_dma need to be properly implemented

[Oleksandr Andrushchenko]

Hi, Stefano

Unfortunately I had to switch to other tasks,

but I'll get back to this issue asap

Thank you


On 03/30/2017 01:36 AM, Stefano Stabellini wrote:

On Wed, 29 Mar 2017, Oleksandr Andrushchenko wrote:

Hi, Stefano!

Ok, probably I need to put more details into the use-case
so it is clear. What I am doing is a DRM driver which
utilizes PRIME buffer sharing [1] to implement zero-copy
of display buffers between DomU and Dom0. PRIME is based on
DMA Buffer Sharing API [2], so this is the reason I am
dealing with sg_table here.

On 03/28/2017 10:20 PM, Stefano Stabellini wrote:

On Tue, 28 Mar 2017, Oleksandr Andrushchenko wrote:

Hi, Stefano!

On 03/27/2017 11:23 PM, Stefano Stabellini wrote:

Hello Oleksandr,

Just to clarify, you are talking about dma_to_phys/phys_to_dma in Linux
(not in Xen), right?

I am talking about Linux, sorry I was not clear

Drivers shouldn't use those functions directly (see the comment in
arch/arm64/include/asm/dma-mapping.h), they should call the appropriate
dma_map_ops functions instead.

Yes, you are correct and I know about this and do not call
dma_to_phys/phys_to_dma directly

The dma_map_ops functions should do the
right thing on Xen, even for pages where pfn != mfn, thanks to the
swiotlb-xen (drivers/xen/swiotlb-xen.c). Specifically, you can see the
special case for pfn != mfn here (see the "local" variable):

arch/arm/include/asm/xen/page-coherent.h:xen_dma_map_page

Yes, the scenarios with pfn != mfn we had so
far are all working correct

So, why are you calling dma_to_phys and phys_to_dma instead of the
dma_map_ops functions?

Ok, let me give you an example of failing scenario which
was not used before this by any backend (this is from
my work on implementing zero copy for DRM drivers):

1. Create sg table from pages:
sg_alloc_table_from_pages(sgt, PFN_0, ...);
map it and get dev_bus_addr - at this stage sg table is
perfectly correct and properly mapped,
dev_bus_addr == (MFN_u << PAGE_SHIFT)

Let me get this straight: one of the pages passed to
sg_alloc_table_from_pages is actually a foreign page (pfn != mfn), is
that right?

And by "map", you mean dma_get_sgtable_attrs is called on it, right?

What happening here is:
- my driver 
1. I create an sg table from pages with pfn != mfn (PFN_0/MFN_u)
using drm_prime_pages_to_sg [3] which effectively is
sg_alloc_table_from_pages
- DRM framework 
2. I pass the sgt via PRIME to the real display driver
and it does drm_gem_map_dma_buf [4]
3. So, at this point everyting is just fine, because sgt is
correct (sgl->page_link points to my PFN_0 and p2m translation
succeeds)
- real HW DRM driver 
4. When real HW display driver accesses sgt it calls dma_get_sgtable
[5] and then dma_map_sg [6]. And all this is happening on the sgt
which my driver has provided, but PFN_0 is not honored anymore
because dma_get_sgtable is expected to be able to figure out
pfn from the corresponding DMA address.

So, strictly speaking real HW DRM driver has no issues,
the API it uses is perfectly valid.

2. Duplicate it:
dma_get_sgtable(..., sgt, ... dev_bus_addr,...);

Yeah, if one of the pages passed to sg_alloc_table_from_pages is
foreign, as Andrii pointed out, dma_get_sgtable
(xen_swiotlb_get_sgtable) doesn't actually work.

This is the case

Is it legitimate that one of those pages is foreign or is it a mistake?

This is the goal - I want pages from DomU to be directly
accessed by the HW in Dom0 (I have DomU 1:1 mapped,
so even contiguous buffers can come from DomU, if not
1:1 then IPMMU will be in place)

If it is a mistake, you could fix it.

 From the above - this is the intention

   Otherwise, if the use of
sg_alloc_table_from_pages or the following call to dma_get_sgtable are
part of your code, I suggest you work-around the problem by avoiding
the dma_get_sgtable call altogether.

As seen from the above the problematic part is not in my
driver, it is either DRM framework or HW display driver

   Don't use the sg_ dma api, use the
regular dma api instead.

I use what DRM provides and dma_xxx if something is missed


Unfortunately, if the dma_get_sgtable is part of existing code, then we
have a problem. In that case, could you point me to the code that call
dma_get_sgtable?

This is the case, see [5]

There is no easy way to make it work on Xen: virt_to_phys doesn't work
on ARM and dma_to_phys doesn't work on Xen. We could implement
xen_swiotlb_get_sgtable correctly if we had the physical address of the
page, because we could easily find out if the page is local or foreign
with a pfn != mfn check (similar to the one in
include/xen/arm/page-coherent.h:xen_dma_map_page).

Yes, I saw this code and it helped me to figure out
where the use-case fails

If the page is local, then we would do what we do today. If the page is
foreign, then we would need to do something like:

  int ret = sg_alloc_table(sgt, 1, GFP_KERNEL);

if (!ret) {
sg_set_pag

Re: [Xen-devel] [PATCH v11 2/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt

[Chao Gao]

On Fri, Mar 31, 2017 at 02:11:08AM -0600, Jan Beulich wrote:
 On 31.03.17 at 01:01,  wrote:
>> On Fri, Mar 31, 2017 at 01:46:33PM +0800, Tian, Kevin wrote:
 From: Gao, Chao
 Sent: Wednesday, March 29, 2017 1:12 PM
 --- a/xen/drivers/passthrough/io.c
 +++ b/xen/drivers/passthrough/io.c
 @@ -618,6 +618,8 @@ int pt_irq_destroy_bind(
  else
  what = "bogus";
  }
 +else if ( iommu_intpost && pirq && pirq_dpci->gmsi.posted )
>>>
>>>No need to check iommu_intpost. Just keep later conditions.
>> 
>> ok. Is it for if posted is set, the iommu_intpost should be true?
>> 
>>>
>>>btw isn't the condition be " (pirq_dpci && pirq_dpci->gmsi.posted)"?
>> 
>> I don't think so. pirq, not pirq_dpci, is consumed by pi_update_irte.
>> Furthermore if pirq_dpci is null, pirq is also null. But it is not true
>> in turn.
>
>With
>
>pirq_dpci = pirq_dpci(pirq);
>
>it _is_ the other way around, which can then also be expressed
>as "if pirq_dpci is non-NULL, pirq is non-NULL too", which is the
>precondition you need to consume (dereference) pirq. There is
>a reason other code around here also only ever checks pirq_dpci
>(even when using pirq).

Got it. Kevin and you are right.

Thanks
Chao

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

Re: [Xen-devel] [PATCH v3] xen/arm: alternative: Register re-mapped Xen area as a temporary virtual region

[Julien Grall]

On 03/31/2017 07:47 AM, Jan Beulich wrote:

On 29.03.17 at 16:23,  wrote:

On 27/03/17 09:40, Wei Chen wrote:

@@ -154,8 +155,12 @@ static int __apply_alternatives_multi_stop(void *unused)
 int ret;
 struct alt_region region;
 mfn_t xen_mfn = _mfn(virt_to_mfn(_start));
-unsigned int xen_order = get_order_from_bytes(_end - _start);
+unsigned int xen_size = _end - _start;


I didn't notice it on the previous reviews. xen_size should technically
be paddr_t.

It is more for consistency than a real bug as the result _end - _start
will unlikely ever be > 4GB. I think Stefano should be able to fix on
commit. So no need to resend the patch.


Hmm, this mostly addresses one of the two complaints I was going
to make (pushing a patch which didn't go via xen-devel).


Actually, I was expected the patch to be fixed up on commit and not seen 
a separate commit. I am not sure why it has been done like that... and 
the commit title does not make that much sense.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v7 2/4] xen: introduce a C99 headers check

[Jan Beulich]

>>> On 30.03.17 at 23:04,  wrote:
> --- a/.gitignore
> +++ b/.gitignore
> @@ -274,6 +274,7 @@ xen/arch/*/efi/compat.c
>  xen/arch/*/efi/efi.h
>  xen/arch/*/efi/runtime.c
>  xen/include/headers.chk
> +xen/include/headers99.chk
>  xen/include/headers++.chk

I'm sorry for not having noticed on the previous round, but please
make this xen/include/headers*.chk just like was done in the clean
target. Generally clean targets and .gitignore entries should match.

> @@ -104,16 +105,21 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Makefile
>   done >$@.new
>   mv $@.new $@
>  
> +headers99.chk: $(PUBLIC_C99_HEADERS) Makefile
> + rm -f $@.new
> + $(foreach i, $(filter %.h,$^), $(CC) -x c -std=c99 -Wall -Werror\
> + -include stdint.h $(foreach j, $($(i)-prereq), -include $(j).h) \
> + -S -o /dev/null $(i) || exit 1; echo $(i) >> $@.new;)
> + mv $@.new $@
> +
>  headers++.chk: $(PUBLIC_HEADERS) Makefile
> - if $(CXX) -v >/dev/null 2>&1; then \
> - for i in $(filter %.h,$^); do \
> - echo '#include "'$$i'"' \
> - | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
> -   -include stdint.h -include public/xen.h -S -o /dev/null - \
> - || exit 1; \
> - echo $$i; \
> - done ; \
> - fi >$@.new
> + rm -f $@.new
> + $(CXX) -v >/dev/null 2>&1 || exit 0

As said before, the lack of a semicolon and line continuation here
will result in this not dong what you want: If there's no C++
compiler available, you'll exit only the shell which was invoked to
execute above command, signaling success to make, which will
then continue with the next command in the rule, which will then
fail due to there not being a C++ compiler.

> + $(foreach i, $(filter %.h,$^), echo "#include "\"$(i)\"|   \

Preferably I'd like to see the | on the start of the next line, as it
was before (the same would then go for the later ||). Alternatively
at least have a blank between closing quote and pipeline character.

> + $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__   \
> + -include stdint.h -include public/xen.h\
> + $(foreach j, $($(i)-prereq), -include c$(j))   \
> + -S -o /dev/null - || exit 1; echo $(i) >> $@.new;)

Would you mind switching to "exit $$?" instead of "exit 1" here,
to propagate the exit code from the compiler? Also in the C99
case then. An alternative would be to use "set -e" up front.

I'm sorry, I again should have noticed these last two earlier.

Jan


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

Re: [Xen-devel] [PATCH v7 4/4] Introduce the pvcalls header

[Jan Beulich]

>>> On 30.03.17 at 23:04,  wrote:
> Define the ring and request and response structs according to the
> specification. Use the new DEFINE_XEN_FLEX_RING macro.
> 
> Add the header to the C99 check.
> 
> Signed-off-by: Stefano Stabellini 
> CC: jbeul...@suse.com 
> CC: konrad.w...@oracle.com 
> ---
>  xen/include/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

The header itself want amiss here.

Jan


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

Re: [Xen-devel] [RFC PATCH v3 2/2] xen: credit2: provide custom option to create

[Dario Faggioli]

On Fri, 2017-03-31 at 00:58 +0530, Praveen Kumar wrote:
> The patch introduces a new command line option 'custom' that when 
used will
> create runqueue based upon the pCPU subset provide during bootup.
> 
"introduces a new, very flexible, way of arranging runqueues in
Credit2. It allows to specify, explicitly and precisely, what pCPUs
should belong to which runqueue"

Or something like this.

It's great that you've got the code working. I have some comments,
though, on how it actually looks like.

> Signed-off-by: Praveen Kumar 
> ---

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -525,7 +525,7 @@ also slow in responding to load changes.
>  The default value of `1 sec` is rather long.
>  
>  ### credit2\_runqueue
> -> `= cpu | core | socket | node | all`
> +> `= cpu | core | socket | node | all | custom`
>  
Err... but this is not really correct, right?

I mean, it's not that the user should use the word 'custom' here.

You probably want to use something like , and then define what
that means below.
 
> @@ -543,6 +543,12 @@ Available alternatives, with their meaning, are:
>  * `node`: one runqueue per each NUMA node of the host;
>  * `all`: just one runqueue shared by all the logical pCPUs of
>   the host
> +* `custom`: one runqueue per subset. Example:
> +credit2_runqueue=[[0,1,4,5][2,3,6,7][8,9,12,13][10,11,14
> ,15]]
>
Maybe just use 2 (or at most 3) pCPUs per runqueue in the example. It'd
make the line shorter and easier to read and understand.

> +- pCPUs 0, 1, 4 and 5 belong to runqueue 0
> +- pCPUs 2, 3, 6 and 7 belong to runqueue 1
> +- pCPUs 8, 9, 12 and 13 belong to runqueue 2
> +- pCPUs 10, 11, 14 and 15 belong to runqueue 3

> --- a/xen/common/sched_credit2.c
> +++ b/xen/common/sched_credit2.c
> @@ -330,18 +339,60 @@ integer_param("credit2_balance_over",
> opt_overload_balance_tolerance);
>  #define OPT_RUNQUEUE_SOCKET 2
>  #define OPT_RUNQUEUE_NODE   3
>  #define OPT_RUNQUEUE_ALL4
> +#define OPT_RUNQUEUE_CUSTOM 5
>  static const char *const opt_runqueue_str[] = {
>  [OPT_RUNQUEUE_CPU] = "cpu",
>  [OPT_RUNQUEUE_CORE] = "core",
>  [OPT_RUNQUEUE_SOCKET] = "socket",
>  [OPT_RUNQUEUE_NODE] = "node",
> -[OPT_RUNQUEUE_ALL] = "all"
> +[OPT_RUNQUEUE_ALL] = "all",
> +[OPT_RUNQUEUE_CUSTOM] = "custom"
>  };
>  static int __read_mostly opt_runqueue = OPT_RUNQUEUE_SOCKET;
>  
> +static int __read_mostly custom_cpu_runqueue[NR_CPUS];
> +
I think this can be nr_cpu_ids (and be allocated dynamically during
parsing).

> +#define GETTOKEN( token, len, start, end )   \
> +{\
> +char _tmp[len+1];\
> +int _i;  \
> +safe_strcpy(_tmp, start);\
> +_tmp[len] = '\0';\
> +for ( _i = 0; _tmp[_i] != '\0'; _i++ )   \
> +token = ( ( token * 10 ) + ( _tmp[_i] - '0' ) ); \
> +}
> +
If you really need this, make it 'static inline', as you do for other
functions.

As a matter of fact, I don't think you need it, as, for instance, we do
have simple_strtoul() (and some others). :-)

> +static inline int trim(const char *c, char *t, char elem)
> +{
> +int l = strlen(c);
> +const char *x = c ;
> +int i = 0;
> +if ( !c || !t )
> +return -1;
> +while ( *x != '\0' && i < l )
> +{
> +if ( *x != elem )
> +t[i++] = *x;
> +x++;
> +}
> +t[i] = '\0';
> +return 0;
> +}
> +
Again, why we need this? Can't we just deal with invalid characters
while parsing the string (by, e.g., either skipping them or aborting,
depending on whether or not they're innocuous)?

> +static inline int getlen(char *start, char *end)
> +{
> +if ( ( start ) && ( end ) && ( end > start ) )
> +return end-start;
> +else
> +return -1;
> +}
> +
>
Same.

>  static void parse_credit2_runqueue(const char *s)
>  {
>  unsigned int i;
> +const char *s_end = NULL;
> +char m[strlen(s)];
> +char *_s = NULL;
>  
No '_' prefixed variable names, please. :-)

Also, what's m for?

> @@ -351,7 +402,115 @@ static void parse_credit2_runqueue(const char
> *s)
>  return;
>  }
>  }
> +/*
> + * At this stage we are either unknown value of credit2_runqueue
> or we can
> + * consider it to be custom cpu. Lets try parsing the same.
> + * Resetting the custom_cpu_runqueue for future use. Only the
> non-negative
> + * entries will be valid. The index 'i' in custom_cpu_runqueue
> will store
> + * the specific runqueue it belongs to.
> + * Example:
> + * If custom_cpu_runqueue[3] == 2
> + * Then, it means that cpu 3 belong to runqueue 2.
> + * If custom_cpu_runqueue[4] == -1
> + * Then, it means that cpu 4 

Re: [Xen-devel] [PATCH v2 02/19] xen/arm: Remove vwfi while setting HCR_EL2 in init_traps

[Julien Grall]

Hi Stefano,

On 03/30/2017 11:29 PM, Stefano Stabellini wrote:

I tried on different boards, and it gets stuck when initializing stage-2 page
table configuration on each CPU (see setup_virt_paging). The secondary CPUs
don't receive the SGI.

Reading a bit more the ARM ARM (D7.2.33 in ARM DDI 0487A.k_iss10775), HCR_EL2
is unknown at reset. Whilst I already knew that, I would have expected to have
no impact on EL2 (at least in the non-VHE case). However, the value of the
{A,I,F}MO bits will affect the routing of physical IRQs to Xen.

I have only gone quickly through the spec, so it might be possible to have
other necessary bits. It might be good to keep initialization here until
someone sit in front of the spec for few hours and check everything.

So in this case I would prefer to keep the helper avoiding to have declared
twice the same flags. Stefano any opinions?


I don't particularly care whether we keep the helper or not. From what
you say we need to set HCR_EL2 in init_traps, but given that's only
temporary, we don't necessarily need to write the same set of flags: for
example HCR_TWI|HCR_TWE are useless at this stage. A minimal set would
suffice. Either way, it's fine by me.


If we don't write the same set of flags, we need some documentation in 
the code to explain why those flags are set for initialization (I 
suspect a lot of them are not necessary) and that HCR flags whilst a 
domain is running are set somewhere else.


Otherwise this is a call to miss setting some HCR flags in the right place.

That's why I suggested a helper (or a global variable), a single place 
to get the default HCR flags rather than spreading.


Cheers,

--
Julien Grall

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

[Xen-devel] [libvirt test] 107016: regressions - FAIL

[osstest service owner]

flight 107016 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107016/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-armhf-libvirt   5 libvirt-buildfail REGR. vs. 106829

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-xsm  1 build-check(1)   blocked  n/a
 build-arm64-pvops 5 kernel-build fail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 build-arm64-xsm   5 xen-buildfail   never pass
 build-arm64   5 xen-buildfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass

version targeted for testing:
 libvirt  fa3b51071157885c40d76e3f18a7fdb2e6e74b39
baseline version:
 libvirt  9b14b2bc3ba95457589fe08f139542476314ff19

Last test of basis   106829  2017-03-22 04:22:56 Z9 days
Failing since106855  2017-03-23 04:22:59 Z8 days8 attempts
Testing same since   107016  2017-03-31 04:20:48 Z0 days1 attempts


People who touched revisions under test:
  Andrea Bolognani 
  Cédric Bosdonnat 
  Eric Blake 
  Erik Skultety 
  Jiri Denemark 
  John Ferlan 
  Ján Tomko 
  Laine Stump 
  Martin Kletzander 
  Michal Privoznik 
  Peter Krempa 
  Roman Bogorodskiy 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  fail
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  fail
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  blocked 
 build-armhf-libvirt  fail
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopsfail
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-libvirt-xsm pass
 test-arm64-arm64-libvirt-xsm blocked 
 test-armhf-armhf-libvirt-xsm blocked 
 test-amd64-i386-libvirt-xsm  pass
 test-amd64-amd64-libvirt pass
 test-arm64-arm64-libvirt blocked 
 test-armhf-armhf-libvirt blocked 
 test-amd64-i386-libvirt  pass
 test-amd64-amd64-libvirt-pairpass
 test-amd64-i386-libvirt-pair pass
 test-arm64-arm64-libvirt-qcow2   blocked 
 test-armhf-armhf-libvirt-raw blocked 
 test-amd64-amd64-libvirt-vhd 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
h

Re: [Xen-devel] [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Julien Grall]

Hi Wei,

On 03/31/2017 03:10 AM, Wei Chen wrote:

Hi Julien and Stefano,

On 2017/3/31 6:03, Stefano Stabellini wrote:

On Thu, 30 Mar 2017, Julien Grall wrote:

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..8af223e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
 return rc;

 /*
+ * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
+ * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
+ * context for restoring it in later.
+ */
+current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);


I don't understand why we care here. idle vCPU will never restore HCR_EL2 nor
return from the hypervisor.


I don't understand this either



Yes, idle vCPU will never return from hypervisor. But in construct_dom0,
it has one chance to restore HCR_EL2.
In in construct_dom0 we will call p2m_restore_state twice.
 saved_current = current;
 p2m_restore_state(v); --->> This is dom0's vCPU0
 [...]
 set_current(saved_current);
 p2m_restore_state(saved_current); --->> this is idle vCPU0

In p2m_restore_state, we will write vcpu->arch.hcr_el2 to HCR_EL2. in
this case, we will write an unknown value to HCR_EL2. Even though this
would not cause any problem from my current testing. But from code
scope, I think it would be a drawback.


The p2m_restore_state will do nothing for idle vCPU (see check at the 
beginning of the function). So there are no issue.


Cheers,

--
Julien Grall

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

[Xen-devel] [distros-debian-jessie test] 71130: tolerable trouble: blocked/broken/pass

[Platform Team regression test user]

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

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 build-armhf-pvops 3 host-install(3)  broken like 68668
 build-armhf   3 host-install(3)  broken like 68668
 build-armhf-pvops 4 capture-logs broken like 68668
 build-armhf   4 capture-logs broken like 68668

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-armhf-jessie-netboot-pygrub  1 build-check(1) blocked n/a
 test-arm64-arm64-armhf-jessie-netboot-pygrub  1 build-check(1) blocked n/a
 build-arm64-pvops 2 hosts-allocate   broken never pass
 build-arm64   2 hosts-allocate   broken never pass
 build-arm64-pvops 3 capture-logs broken never pass
 build-arm64   3 capture-logs broken never pass

baseline version:
 flight   68668

jobs:
 build-amd64  pass
 build-arm64  broken  
 build-armhf  broken  
 build-i386   pass
 build-amd64-pvopspass
 build-arm64-pvopsbroken  
 build-armhf-pvopsbroken  
 build-i386-pvops pass
 test-amd64-amd64-amd64-jessie-netboot-pvgrub pass
 test-amd64-i386-i386-jessie-netboot-pvgrub   pass
 test-amd64-i386-amd64-jessie-netboot-pygrub  pass
 test-arm64-arm64-armhf-jessie-netboot-pygrub blocked 
 test-armhf-armhf-armhf-jessie-netboot-pygrub blocked 
 test-amd64-amd64-i386-jessie-netboot-pygrub  pass



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] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Jan Beulich]

>>> On 30.03.17 at 14:10,  wrote:
> On 17-03-30 05:55:52, Jan Beulich wrote:
>> >>> On 30.03.17 at 03:37,  wrote:
>> > On 17-03-29 03:57:52, Jan Beulich wrote:
>> >> >>> On 29.03.17 at 03:36,  wrote:
>> >> > On 17-03-29 09:20:21, Yi Sun wrote:
>> >> >> On 17-03-28 06:20:48, Jan Beulich wrote:
>> >> >> > >>> On 28.03.17 at 13:59,  wrote:
>> >> >> > > I think we at least need a 'get_val()' hook.
>> >> >> > 
>> >> >> > Of course.
>> >> >> > 
>> >> >> > > I try to implement CAT/CDP hook.
>> >> >> > > Please help to check if this is what you thought.
>> >> >> > 
>> >> >> > One remark below, but other than that - yes.
>> >> >> > 
>> >> >> > > static void cat_get_val(const struct feat_node *feat, unsigned int 
>> >> >> > > cos,
>> >> >> > > enum cbm_type type, int flag, uint32_t 
>> >> >> > > *val)
>> >> >> > > {
>> >> >> > > *val = feat->cos_reg_val[cos];
>> >> >> > > }
>> >> >> > > 
>> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, unsigned 
>> >> >> > > int 
>> > cos,
>> >> >> > >enum cbm_type type, int flag, uint32_t 
>> >> >> > > *val)
>> >> >> > > {
>> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 )
>> >> >> > > *val = get_cdp_data(feat, cos);
>> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 )
>> >> >> > > *val = get_cdp_code(feat, cos);
>> >> >> > > }
>> >> >> > 
>> >> >> > Why the redundancy between type and flag?
>> >> >> > 
>> >> >> For psr_get_val, upper layer input the cbm_type to get either DATA or 
>> >> >> CODE
>> >> >> value. For other cases, we use flag as cos_num index to get either 
>> >> >> DATA or
>> >> >> CODE.
>> >> >> 
>> >> > Let me explain more to avoid confusion. For other cases, we use cos_num 
>> >> > as
>> >> > index to get values from a feature. In these cases, we do not know the
>> >> > cbm_type of the feature. So, I use the cos_num as flag to make 'get_val'
>> >> > know which value should be returned.
>> >> 
>> >> I'm pretty sure this redundancy can be avoided.
>> >> 
>> > Then, I think I have to reuse the 'type'. As only CDP needs type to decide
>> > which value to be returned so far, I think I can implement codes like below
>> > to make CDP can handle all scenarios.
>> > 
>> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int cos,
>> >enum cbm_type type, uint32_t *val)
>> > {
>> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 )
>> > *val = get_cdp_data(feat, cos);
>> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 )
>> > *val = get_cdp_code(feat, cos);
>> > }
>> > 
>> > static bool fits_cos_max(...)
>> > {
>> > ..
>> > for (i = 0; i < feat->props->cos_num; i++)
>> > {
>> > feat->props->get_val(feat, cos, i + 0xF000, &default_val);
>> > if ( val[i] == default_val )
>> > ..
>> > }
>> > ..
>> > }
>> > 
>> > Is that good for you?
>> 
>> Urgh - no, not really: This is hackery. Do you have a tree
>> somewhere so I could look at the file after at least the CDP
>> patches have all been applied, to see whether I have a
>> better idea? Alternatively, could you attach psr.c the way
>> you have it right now with the entire series applied?
>> 
> I think you can check v9 codes here:
> https://github.com/yisun-git/xen/tree/l2_cat_v9 

Looking at this made me notice that cat_get_old_val() passes a
bogus literal 0 to cat_get_val(), which needs taking care of too.
One option I can see is for each feature to make available an
array of type enum cbm_type, with cos_num elements. The order
would match that of the order of values in their arrays. This will
allow elimination of all of the get_old_val, compare_val, and
fits_cos_max hooks afaict. At that point the "new" in
set_new_val is also no longer needed to contrast with the "old"
in the no longer existing get_old_val. Arguably insert_val may
be even slightly more precise in describing what the function
does.

The checks done first in what is currently *_set_new_val() also
look like they could be moved into the (generic) caller(s), making
clear that at least for now even cbm_len (just like suggested for
cos_max) should be a generic rather than unionized field in
struct feat_node. In the worst case a new check_val() hook
might be needed.

Jan

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

Re: [Xen-devel] [PATCH v2 09/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Julien Grall]

Hi Stefano,

On 03/30/2017 10:28 PM, Stefano Stabellini wrote:

On Thu, 30 Mar 2017, Julien Grall wrote:

On 30/03/17 10:13, Wei Chen wrote:

In the later patches of this series, we want to use the alternative
patching framework to avoid checking serror_op in every entries.
So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
serror_op. When serror_op is not equal to SERROR_DIVERSE, this
feature will be set to cpu_hwcaps.

Currently, the default serror_op is SERROR_DIVERSE, if we want to
change the serror_op value we have to place the serror parameter
in command line. It seems no problem to update cpu_hwcaps directly
in the serror parameter parsing function.

But one day, if we change the default serror_op to SERROR_PANIC or
SERROR_FORWARD by some security policy. We may not place the serror
parameter in command line. In this case, if we rely on the serror
parameter parsing function to update cpu_hwcaps, this function would
not be invoked and the "SKIP_CHECK_PENDING_VSERROR" could not be
set in cpu_hwcaps.

So, we introduce this initcall to guarantee the cpu_hwcaps can be
updated no matter the serror parameter is placed in the command line
or not.

Signed-off-by: Wei Chen 
---
v1->v2:
1. Explain this initcall is to future-proof the code in commit
   message.
2. Fix a coding style of this initcall.
---
 xen/arch/arm/traps.c | 9 +
 xen/include/asm-arm/cpufeature.h | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 955d97c..dafb730 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -120,6 +120,15 @@ static void __init parse_serrors_behavior(const char
*str)
 }
 custom_param("serrors", parse_serrors_behavior);

+static int __init update_serrors_cpu_caps(void)
+{
+if ( serrors_op != SERRORS_DIVERSE )
+cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);


Thinking a bit more of this. I am wondering if we should add a warning (see
warning_add) if the user is selecting an option other than diverse. Two
reasons for that:
1) The user is fully aware that he is not classifying the SError at
his own risks
2) If someone send an e-mail saying: "My guest crashed the hypervisor
with an SError". We can directly know from the log.

Any opinions?


I would not do that: warnings are good to warn the user about something
unexpected or potentially unknown. In this case the user has to look up
the docs to find about the other options, where it's clearly written
what they are for. We have to expect taht they know what their are
doing.


Fair enough :). Wei, please ignore my request.



For this patch:

Reviewed-by: Stefano Stabellini 



Cheers,

--
Julien Grall

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

[Xen-devel] Outreachy Project

[Nakka Ricci]

Hello!

My name is Nazira and I am interested in the project called "Xen
Hypervisor". This mail is displayed as a contact information, so I wanted
to ask how to connect to a mentor of the project?
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [ovmf test] 107018: all pass - PUSHED

[osstest service owner]

flight 107018 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107018/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf de87f23291620d36d69ec55ea53a1c38b8780f0b
baseline version:
 ovmf 251779fca719bf9ea00505ee7629c08b452c150d

Last test of basis   107013  2017-03-31 00:45:04 Z0 days
Testing same since   107018  2017-03-31 05:47:36 Z0 days1 attempts


People who touched revisions under test:
  Bell Song 
  Song, BinX 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  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=ovmf
+ revision=de87f23291620d36d69ec55ea53a1c38b8780f0b
+ . ./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 ovmf 
de87f23291620d36d69ec55ea53a1c38b8780f0b
+ branch=ovmf
+ revision=de87f23291620d36d69ec55ea53a1c38b8780f0b
+ . ./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=ovmf
+ xenbranch=xen-unstable
+ '[' xovmf = xlinux ']'
+ linuxbranch=
+ '[' x = x ']'
+ qemuubranch=qemu-upstream-unstable
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable
+ prevxenbranch=xen-4.8-testing
+ '[' xde87f23291620d36d69ec55ea53a1c38b8780f0b = 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/git/firmware/linux-firmware.git
++ : osst...@xenbits.xen.org:/home/xen/git/

Re: [Xen-devel] [PATCH v2 04/19] xen/arm: Restore HCR_EL2 register

[Wei Chen]

On 2017/3/31 16:39, Julien Grall wrote:
> Hi Wei,
>
> On 03/31/2017 03:10 AM, Wei Chen wrote:
>> Hi Julien and Stefano,
>>
>> On 2017/3/31 6:03, Stefano Stabellini wrote:
>>> On Thu, 30 Mar 2017, Julien Grall wrote:
 Hi Wei,

 On 30/03/17 10:13, Wei Chen wrote:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index de59e5f..8af223e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2171,6 +2171,13 @@ int construct_dom0(struct domain *d)
>  return rc;
>
>  /*
> + * The HCR_EL2 will temporarily switch to dom0's HCR_EL2 value
> + * by p2m_restore_state. We have to save HCR_EL2 to idle vCPU's
> + * context for restoring it in later.
> + */
> +current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);

 I don't understand why we care here. idle vCPU will never restore HCR_EL2 
 nor
 return from the hypervisor.
>>>
>>> I don't understand this either
>>>
>>
>> Yes, idle vCPU will never return from hypervisor. But in construct_dom0,
>> it has one chance to restore HCR_EL2.
>> In in construct_dom0 we will call p2m_restore_state twice.
>>  saved_current = current;
>>  p2m_restore_state(v); --->> This is dom0's vCPU0
>>  [...]
>>  set_current(saved_current);
>>  p2m_restore_state(saved_current); --->> this is idle vCPU0
>>
>> In p2m_restore_state, we will write vcpu->arch.hcr_el2 to HCR_EL2. in
>> this case, we will write an unknown value to HCR_EL2. Even though this
>> would not cause any problem from my current testing. But from code
>> scope, I think it would be a drawback.
>
> The p2m_restore_state will do nothing for idle vCPU (see check at the
> beginning of the function). So there are no issue.
>

Oh, yes, you are right. I missed the idle vCPU check at the beginning.
I would remove this code from next version.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v2 1/2] ACPICA: ACPI 6.0: Add support for IORT table.

[Jan Beulich]

>>> On 31.03.17 at 01:00,  wrote:
> --- a/xen/include/acpi/actbl2.h
> +++ b/xen/include/acpi/actbl2.h
> @@ -69,6 +69,7 @@
>  #define ACPI_SIG_HPET   "HPET"   /* High Precision Event Timer 
> table */
>  #define ACPI_SIG_IBFT   "IBFT"   /* i_sCSI Boot Firmware Table */
>  #define ACPI_SIG_IVRS   "IVRS"   /* I/O Virtualization Reporting 
> Structure */
> +#define ACPI_SIG_IORT   "IORT"   /* IO Remapping Table */

The insertion point is ahead of IVRS in the original commit, retaining
alphabetical sorting. I'll fix this up while committing, but please be
more careful next time round.

Jan


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

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Yi Sun]

On 17-03-31 02:47:25, Jan Beulich wrote:
> >>> On 30.03.17 at 14:10,  wrote:
> > On 17-03-30 05:55:52, Jan Beulich wrote:
> >> >>> On 30.03.17 at 03:37,  wrote:
> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> >> >> >>> On 29.03.17 at 03:36,  wrote:
> >> >> > On 17-03-29 09:20:21, Yi Sun wrote:
> >> >> >> On 17-03-28 06:20:48, Jan Beulich wrote:
> >> >> >> > >>> On 28.03.17 at 13:59,  wrote:
> >> >> >> > > I think we at least need a 'get_val()' hook.
> >> >> >> > 
> >> >> >> > Of course.
> >> >> >> > 
> >> >> >> > > I try to implement CAT/CDP hook.
> >> >> >> > > Please help to check if this is what you thought.
> >> >> >> > 
> >> >> >> > One remark below, but other than that - yes.
> >> >> >> > 
> >> >> >> > > static void cat_get_val(const struct feat_node *feat, unsigned 
> >> >> >> > > int cos,
> >> >> >> > > enum cbm_type type, int flag, uint32_t 
> >> >> >> > > *val)
> >> >> >> > > {
> >> >> >> > > *val = feat->cos_reg_val[cos];
> >> >> >> > > }
> >> >> >> > > 
> >> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, 
> >> >> >> > > unsigned int 
> >> > cos,
> >> >> >> > >enum cbm_type type, int flag, 
> >> >> >> > > uint32_t *val)
> >> >> >> > > {
> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 )
> >> >> >> > > *val = get_cdp_data(feat, cos);
> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 )
> >> >> >> > > *val = get_cdp_code(feat, cos);
> >> >> >> > > }
> >> >> >> > 
> >> >> >> > Why the redundancy between type and flag?
> >> >> >> > 
> >> >> >> For psr_get_val, upper layer input the cbm_type to get either DATA 
> >> >> >> or CODE
> >> >> >> value. For other cases, we use flag as cos_num index to get either 
> >> >> >> DATA or
> >> >> >> CODE.
> >> >> >> 
> >> >> > Let me explain more to avoid confusion. For other cases, we use 
> >> >> > cos_num as
> >> >> > index to get values from a feature. In these cases, we do not know the
> >> >> > cbm_type of the feature. So, I use the cos_num as flag to make 
> >> >> > 'get_val'
> >> >> > know which value should be returned.
> >> >> 
> >> >> I'm pretty sure this redundancy can be avoided.
> >> >> 
> >> > Then, I think I have to reuse the 'type'. As only CDP needs type to 
> >> > decide
> >> > which value to be returned so far, I think I can implement codes like 
> >> > below
> >> > to make CDP can handle all scenarios.
> >> > 
> >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int 
> >> > cos,
> >> >enum cbm_type type, uint32_t *val)
> >> > {
> >> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 )
> >> > *val = get_cdp_data(feat, cos);
> >> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 )
> >> > *val = get_cdp_code(feat, cos);
> >> > }
> >> > 
> >> > static bool fits_cos_max(...)
> >> > {
> >> > ..
> >> > for (i = 0; i < feat->props->cos_num; i++)
> >> > {
> >> > feat->props->get_val(feat, cos, i + 0xF000, &default_val);
> >> > if ( val[i] == default_val )
> >> > ..
> >> > }
> >> > ..
> >> > }
> >> > 
> >> > Is that good for you?
> >> 
> >> Urgh - no, not really: This is hackery. Do you have a tree
> >> somewhere so I could look at the file after at least the CDP
> >> patches have all been applied, to see whether I have a
> >> better idea? Alternatively, could you attach psr.c the way
> >> you have it right now with the entire series applied?
> >> 
> > I think you can check v9 codes here:
> > https://github.com/yisun-git/xen/tree/l2_cat_v9 
> 
> Looking at this made me notice that cat_get_old_val() passes a
> bogus literal 0 to cat_get_val(), which needs taking care of too.
> One option I can see is for each feature to make available an
> array of type enum cbm_type, with cos_num elements. The order
> would match that of the order of values in their arrays. This will

Sorry, not very clear your meaning. How to do that? Could you please
provide pieces of codes? Thanks!

> allow elimination of all of the get_old_val, compare_val, and
> fits_cos_max hooks afaict. At that point the "new" in
> set_new_val is also no longer needed to contrast with the "old"
> in the no longer existing get_old_val. Arguably insert_val may
> be even slightly more precise in describing what the function
> does.

I have modified it to 'set_val_to_array'.

> 
> The checks done first in what is currently *_set_new_val() also
> look like they could be moved into the (generic) caller(s), making

MBA value is throttling value which is different with CBM. So, the
check is different. So, we have to have 'check_val()' at least.

> clear that at least for now even cbm_len (just like suggested for
> cos_max) should be a generic rather than unionized field in

MBA does not have cbm_len.

> struct feat_node. In the worst case a new check_val() hook
> might be needed.
> 
After analyzing codes again, for 'gather_val', 'compare_val' and
'fit

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Yi Sun]

On 17-03-31 17:12:36, Yi Sun wrote:
> On 17-03-31 02:47:25, Jan Beulich wrote:
> > >>> On 30.03.17 at 14:10,  wrote:
> > > On 17-03-30 05:55:52, Jan Beulich wrote:
> > >> >>> On 30.03.17 at 03:37,  wrote:
> > >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> > >> >> >>> On 29.03.17 at 03:36,  wrote:
> > >> >> > On 17-03-29 09:20:21, Yi Sun wrote:
> > >> >> >> On 17-03-28 06:20:48, Jan Beulich wrote:
> > >> >> >> > >>> On 28.03.17 at 13:59,  wrote:
> > >> >> >> > > I think we at least need a 'get_val()' hook.
> > >> >> >> > 
> > >> >> >> > Of course.
> > >> >> >> > 
> > >> >> >> > > I try to implement CAT/CDP hook.
> > >> >> >> > > Please help to check if this is what you thought.
> > >> >> >> > 
> > >> >> >> > One remark below, but other than that - yes.
> > >> >> >> > 
> > >> >> >> > > static void cat_get_val(const struct feat_node *feat, unsigned 
> > >> >> >> > > int cos,
> > >> >> >> > > enum cbm_type type, int flag, uint32_t 
> > >> >> >> > > *val)
> > >> >> >> > > {
> > >> >> >> > > *val = feat->cos_reg_val[cos];
> > >> >> >> > > }
> > >> >> >> > > 
> > >> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, 
> > >> >> >> > > unsigned int 
> > >> > cos,
> > >> >> >> > >enum cbm_type type, int flag, 
> > >> >> >> > > uint32_t *val)
> > >> >> >> > > {
> > >> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 )
> > >> >> >> > > *val = get_cdp_data(feat, cos);
> > >> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 )
> > >> >> >> > > *val = get_cdp_code(feat, cos);
> > >> >> >> > > }
> > >> >> >> > 
> > >> >> >> > Why the redundancy between type and flag?
> > >> >> >> > 
> > >> >> >> For psr_get_val, upper layer input the cbm_type to get either DATA 
> > >> >> >> or CODE
> > >> >> >> value. For other cases, we use flag as cos_num index to get either 
> > >> >> >> DATA or
> > >> >> >> CODE.
> > >> >> >> 
> > >> >> > Let me explain more to avoid confusion. For other cases, we use 
> > >> >> > cos_num as
> > >> >> > index to get values from a feature. In these cases, we do not know 
> > >> >> > the
> > >> >> > cbm_type of the feature. So, I use the cos_num as flag to make 
> > >> >> > 'get_val'
> > >> >> > know which value should be returned.
> > >> >> 
> > >> >> I'm pretty sure this redundancy can be avoided.
> > >> >> 
> > >> > Then, I think I have to reuse the 'type'. As only CDP needs type to 
> > >> > decide
> > >> > which value to be returned so far, I think I can implement codes like 
> > >> > below
> > >> > to make CDP can handle all scenarios.
> > >> > 
> > >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int 
> > >> > cos,
> > >> >enum cbm_type type, uint32_t *val)
> > >> > {
> > >> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 )
> > >> > *val = get_cdp_data(feat, cos);
> > >> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 )
> > >> > *val = get_cdp_code(feat, cos);
> > >> > }
> > >> > 
> > >> > static bool fits_cos_max(...)
> > >> > {
> > >> > ..
> > >> > for (i = 0; i < feat->props->cos_num; i++)
> > >> > {
> > >> > feat->props->get_val(feat, cos, i + 0xF000, &default_val);
> > >> > if ( val[i] == default_val )
> > >> > ..
> > >> > }
> > >> > ..
> > >> > }
> > >> > 
> > >> > Is that good for you?
> > >> 
> > >> Urgh - no, not really: This is hackery. Do you have a tree
> > >> somewhere so I could look at the file after at least the CDP
> > >> patches have all been applied, to see whether I have a
> > >> better idea? Alternatively, could you attach psr.c the way
> > >> you have it right now with the entire series applied?
> > >> 
> > > I think you can check v9 codes here:
> > > https://github.com/yisun-git/xen/tree/l2_cat_v9 
> > 
> > Looking at this made me notice that cat_get_old_val() passes a
> > bogus literal 0 to cat_get_val(), which needs taking care of too.
> > One option I can see is for each feature to make available an
> > array of type enum cbm_type, with cos_num elements. The order
> > would match that of the order of values in their arrays. This will
> 
> Sorry, not very clear your meaning. How to do that? Could you please
> provide pieces of codes? Thanks!
> 
> > allow elimination of all of the get_old_val, compare_val, and
> > fits_cos_max hooks afaict. At that point the "new" in
> > set_new_val is also no longer needed to contrast with the "old"
> > in the no longer existing get_old_val. Arguably insert_val may
> > be even slightly more precise in describing what the function
> > does.
> 
> I have modified it to 'set_val_to_array'.
> 
> > 
> > The checks done first in what is currently *_set_new_val() also
> > look like they could be moved into the (generic) caller(s), making
> 
> MBA value is throttling value which is different with CBM. So, the
> check is different. So, we have to have 'check_val()' at least.
> 
> > clear that at least 

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Jan Beulich]

>>> On 29.03.17 at 07:11,  wrote:
> @@ -442,17 +397,24 @@ int pt_irq_create_bind(
>  dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>  pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>  spin_unlock(&d->event_lock);
> +
> +pirq_dpci->gmsi.posted = false;
> +vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> +if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )

Why again would dest_Fixed not allow posted delivery? This needs
recording in a comment, if there really is such a restriction. Or did
you really mean to place ...

> +{
> +vcpu = vector_hashing_dest(d, dest, dest_mode,
> +   pirq_dpci->gmsi.gvec);
> +if ( vcpu )
> +pirq_dpci->gmsi.posted = true;

... this ...

> +}

... after this brace (which then wouldn't be needed anymore)? If
so, is there any point calling vector_hashing_dest() when vcpu is
already non-NULL prior to the if()?

This then also raises the question whether the call to
hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest
priority delivery mode.

Jan


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

Re: [Xen-devel] [PATCH] setup vwfi correctly on cpu0

[Julien Grall]

Hi Stefano,

On 03/30/2017 11:35 PM, Stefano Stabellini wrote:

parse_vwfi runs after init_traps on cpu0, potentially resulting in the
wrong HCR_EL2 for it. Secondary cpus boot after parse_vwfi, so in their
case init_traps will write the correct set of flags to HCR_EL2.

For cpu0, fix the issue by changing HCR_EL2 setting directly in
parse_vwfi.

Signed-off-by: Stefano Stabellini 

---
This patch should be apply to 4.8, 4.7, 4.6, not to unstable (it will be
fixed differently there).
---
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 614501f..94d2e8a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -112,6 +112,16 @@ static void __init parse_vwfi(const char *s)
vwfi = NATIVE;
else
vwfi = TRAP;
+/*
+ * HCR_EL2 has already been set on cpu0, change the setting here, if
+ * needed. Other cpus haven't booted yet, init_traps will setup
+ * HCR_EL2 correctly.
+ */
+if (vwfi == NATIVE) {


Coding style:

if ( ... )
{


+register_t hcr;
+hcr = READ_SYSREG(HCR_EL2);
+WRITE_SYSREG(hcr & ~(HCR_TWI|HCR_TWE), HCR_EL2);


You are assuming the default value of vwfi and it makes very complicate 
for someone to follow the code and modify it.


IHMO, a parsing function should only parse the command line and setup 
variable. Hardware configuration or initialization should be done 
separately (such as in an init call).


Cheers,

--
Julien Grall

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

[Xen-devel] [Outreachy] Interested in contribution: Code Standards Checking using clang-format

[Ishani]

Hello Sir,

I am Ishani Chugh, a fourth year undergraduate student of Electronics and 
Communications Engineering at International Institute of Information 
Technology, Hyderabad. I am passionate about Open Source Development. I am 
interested to contribute to your organization Xen Project as an Outreachy 
intern . I am interested to work in the project: Code Standards Checking using 
clang-format. I am proficient in C++ and have slight experience of working in 
clang-format.
Please guide me for initial contributions to prove my candidature.

Regards
Ishani

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

[Xen-devel] [linux-linus test] 107010: regressions - FAIL

[osstest service owner]

flight 107010 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107010/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-armhf-armhf-xl-arndale  11 guest-start   fail REGR. vs. 59254
 test-armhf-armhf-libvirt-xsm 11 guest-start   fail REGR. vs. 59254
 test-armhf-armhf-xl-cubietruck 11 guest-start fail REGR. vs. 59254
 test-armhf-armhf-libvirt 11 guest-start   fail REGR. vs. 59254
 test-armhf-armhf-xl-xsm  11 guest-start   fail REGR. vs. 59254
 test-armhf-armhf-xl-multivcpu 11 guest-start  fail REGR. vs. 59254
 test-armhf-armhf-xl  11 guest-start   fail REGR. vs. 59254
 test-armhf-armhf-xl-credit2  11 guest-start fail in 106998 REGR. vs. 59254

Tests which are failing intermittently (not blocking):
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 6 xen-boot fail in 106998 pass in 
107010
 test-armhf-armhf-xl-credit2   6 xen-boot   fail pass in 106998

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-xl-rtds 11 guest-start   fail REGR. vs. 59254
 test-amd64-amd64-xl-rtds  9 debian-installfail REGR. vs. 59254
 test-armhf-armhf-xl-vhd   9 debian-di-install   fail baseline untested
 test-armhf-armhf-libvirt-raw  9 debian-di-install   fail baseline untested
 test-amd64-amd64-xl-pvh-amd 19 guest-start/debian.repeat fail blocked in 59254
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop  fail like 59254
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 59254
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stop fail like 59254
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop  fail like 59254

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-rtds  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 build-arm64-xsm   5 xen-buildfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 build-arm64   5 xen-buildfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 linux89970a04d70c6c9e5e4492fd4096c0b5630a478c
baseline version:
 linux45820c294fe1b1a9df495d57f40585ef2d069a39

Last test of basis59254  2015-07-09 04:20:48 Z  631 days
Failing since 59348  2015-07-10 04:24:05 Z  630 days  366 attempts
Testing same since   106998  2017-03-30 11:52:36 Z0 days2 attempts


8116 people touched revisions under test,
not listing them all

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  fail
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  fail
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-arm64-libvirt  blocked 
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-arm64-pvopspass
 build-armhf-pvopspass
 build-i386-pv

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Chao Gao]

On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote:
 On 29.03.17 at 07:11,  wrote:
>> @@ -442,17 +397,24 @@ int pt_irq_create_bind(
>>  dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>>  pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>>  spin_unlock(&d->event_lock);
>> +
>> +pirq_dpci->gmsi.posted = false;
>> +vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
>> +if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )
>
>Why again would dest_Fixed not allow posted delivery? This needs

No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives
the same output with pi_find_dest_vcpu(). Thus we don't call it again, just
use the return value of hvm_girq_dest_2_vcpu_id().

>recording in a comment, if there really is such a restriction. Or did
>you really mean to place ...
>
>> +{
>> +vcpu = vector_hashing_dest(d, dest, dest_mode,
>> +   pirq_dpci->gmsi.gvec);
>> +if ( vcpu )
>> +pirq_dpci->gmsi.posted = true;
>
>... this ...
>
>> +}
>
>... after this brace (which then wouldn't be needed anymore)? If
>so, is there any point calling vector_hashing_dest() when vcpu is
>already non-NULL prior to the if()?
>
>This then also raises the question whether the call to
>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest
>priority delivery mode.

For lowest priority delivery mode, if VT-d PI is enabled, the result (the
destination vcpu) is overrided by vector_hashing_dest() to keep the 
existing behavior. I think the only point we should keep in mind is
for cases other than lowest priority delivery mode, pi_find_dest_vcpu()
and hvm_girq_dest_2_vcpu_id() give the same destination vcpu.

Thanks
Chao

>
>Jan
>

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

Re: [Xen-devel] [PATCH v11 2/6] VT-d: Introduce new fields in msi_desc to track binding with guest interrupt

[Jan Beulich]

>>> On 29.03.17 at 07:11,  wrote:
> v11:
> - Commit message changes.
> - Add code to clear the two new fields introduced in this patch.

The latter appears to contradict ...

> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -578,6 +578,7 @@ static struct msi_desc *alloc_msi_entry(unsigned int nr)
>  entry[nr].dev = NULL;
>  entry[nr].irq = -1;
>  entry[nr].remap_index = -1;
> +entry[nr].pi_desc = NULL;
>  }

... only pi_desc being cleared here. I think the code is fine here (as
you use gvec only when pi_desc is non-NULL), but please try to
avoid giving imprecise information in the future.

> --- a/xen/drivers/passthrough/io.c
> +++ b/xen/drivers/passthrough/io.c
> @@ -618,6 +618,8 @@ int pt_irq_destroy_bind(
>  else
>  what = "bogus";
>  }
> +else if ( iommu_intpost && pirq && pirq_dpci->gmsi.posted )
> +pi_update_irte(NULL, pirq, 0);

The more I look at this passing of a NULL vCPU pointer, the less
I like it: If the pointer was used for anything other than
retrieving pi_desc, this would be actively dangerous. I think the
function would better be passed a const struct pi_desc * instead.

> --- a/xen/include/asm-x86/msi.h
> +++ b/xen/include/asm-x86/msi.h
> @@ -118,6 +118,8 @@ struct msi_desc {
>   struct msi_msg msg; /* Last set MSI message */
>  
>   int remap_index;/* index in interrupt remapping table */
> + const struct pi_desc *pi_desc;  /* pointer to posted descriptor */
> + uint8_t gvec;   /* guest vector. valid when pi_desc 
> isn't NULL */
>  };

Please avoid adding further holes in this structure, by moving
gvec right after msi_attrib (I'll also queue a patch to move
remap_index up ahead of msg).

Jan


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

Re: [Xen-devel] [PATCH RFC] x86/emulate: implement hvmemul_cmpxchg() with an actual CMPXCHG

[Razvan Cojocaru]

On 03/31/2017 10:34 AM, Jan Beulich wrote:
 On 31.03.17 at 08:17,  wrote:
>> On 03/30/2017 06:47 PM, Jan Beulich wrote:
 Speaking of emulated MMIO, I've got this when the guest was crashing
 immediately (pre RETRY loop):

  MMIO emulation failed: d3v8 32bit @ 0008:82679f3c -> f0 0f ba 30 00 72
 07 8b cb e8 da 4b ff ff 8b 45
>>>
>>> That's a BTR, which we should be emulating fine. More information
>>> would need to be collected to have a chance to understand what
>>> might be going one (first of all the virtual and physical memory
>>> address this was trying to act on).
>>
>> Right, the BTR part should be fine, but I think the LOCK part is what's
>> causing the issue. I've done a few more test runs to see what return
>> RETRY (dumping the instruction with an "(r)" prefix to distinguish from
>> the UNHANDLEABLE dump), and a couple of instructions return RETRY (BTR
>> and XADD, both LOCK-prefixed, which means they now involve CMPXCHG
>> handler, which presumably now fails - possibly simply because it's
>> always LOCKed in my patch):
> 
> Well, all of that looks to be expected behavior. I'm afraid I don't see
> how this information helps understanding the MMIO emulation failure
> above.

I've managed to obtain this log of emulation errors:
https://pastebin.com/Esy1SkHx

The "virtual address" lines that are not followed by any "Mem event"
line correspond to CMXCHG_FAILED return codes.

The very last line is a MMIO emulation failed.

It's probably important that this happens with the model where
hvm_emulate_one_vm_event() does _not_ re-try the emulation until it
succeeds. The other model allows me to go further with the guest, but
eventually I get timeout-related BSODs or the guest becomes unresponsive.


Thanks,
Razvan

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

[Xen-devel] [qemu-mainline test] 107011: tolerable FAIL - PUSHED

[osstest service owner]

flight 107011 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107011/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 106999
 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail  like 106999
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 106999
 test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail  like 106999
 test-amd64-amd64-xl-rtds  9 debian-install   fail  like 106999
 test-armhf-armhf-libvirt 13 saverestore-support-checkfail  like 106999

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-rtds  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 build-arm64-xsm   5 xen-buildfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 build-arm64   5 xen-buildfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 build-arm64-pvops 5 kernel-build fail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail 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-xl-xsm  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  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-cubietruck 12 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-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-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   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-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass

version targeted for testing:
 qemuuddc2c3a57e0752c0650fdb735a8b8322542d4248
baseline version:
 qemuub529aec1ee1cbb642f236893bf46a712d0187e4d

Last test of basis   106999  2017-03-30 12:16:09 Z0 days
Testing same since   107011  2017-03-30 23:44:31 Z0 days1 attempts


People who touched revisions under test:
  Alexey Kardashevskiy 
  Andrew Baumann 
  David Gibson 
  Halil Pasic 
  Jason Wang 
  Laurent Vivier 
  Laurent Vivier 
  Marc-André Lureau 
  Maxime Coquelin 
  Michael S. Tsirkin 
  Peter Maydell 
  Samuel Thibault 
  Stefan Weil 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  fail
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  fail
 build-armhf  pass
 build-i386   

Re: [Xen-devel] [PATCH v11 5/6] VT-d: introduce update_irte to update irte safely

[Jan Beulich]

>>> On 29.03.17 at 07:11,  wrote:
> We used structure assignment to update irte which was non-atomic when the
> whole IRTE was to be updated. It is unsafe when a interrupt happened during
> update. Furthermore, no bug or warning would be reported when this happened.
> 
> This patch introduces two variants, atomic and non-atomic, to update
> irte. Both variants will update IRTE if possible. If the caller requests a
> atomic update but we can't meet it, we raise a bug.
> 
> Signed-off-by: Chao Gao 
> ---
> v11:
> - Add two variant function to update IRTE. Call the non-atomic one for init
> and clear operations. Call the atomic one for other cases.
> - Add a new field to indicate the remap_entry associated with msi_desc is
> initialized or not.
> 
> v10:
> - rename copy_irte_to_irt to update_irte
> - remove copy_from_to_irt
> - change commmit message and add some comments to illustrate on which
> condition update_irte() is safe.
> 
>  xen/arch/x86/msi.c |  1 +
>  xen/drivers/passthrough/vtd/intremap.c | 78 
> --
>  xen/include/asm-x86/msi.h  |  1 +
>  3 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index 3374cd4..7ed1243 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -578,6 +578,7 @@ static struct msi_desc *alloc_msi_entry(unsigned int nr)
>  entry[nr].dev = NULL;
>  entry[nr].irq = -1;
>  entry[nr].remap_index = -1;
> +entry[nr].remap_entry_initialized = false;
>  entry[nr].pi_desc = NULL;
>  }
>  
> diff --git a/xen/drivers/passthrough/vtd/intremap.c 
> b/xen/drivers/passthrough/vtd/intremap.c
> index b992f23..b7f3cf1 100644
> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -169,10 +169,64 @@ bool_t __init iommu_supports_eim(void)
>  return 1;
>  }
>  
> +static void update_irte(struct iremap_entry *entry,
> +const struct iremap_entry *new_ire,
> +bool atomic)
> +{
> +if ( cpu_has_cx16 )
> +{
> +__uint128_t ret;
> +struct iremap_entry old_ire;
> +
> +old_ire = *entry;
> +ret = cmpxchg16b(entry, &old_ire, new_ire);
> +
> +/*
> + * In the above, we use cmpxchg16 to atomically update the 128-bit
> + * IRTE, and the hardware cannot update the IRTE behind us, so
> + * the return value of cmpxchg16 should be the same as old_ire.
> + * This ASSERT validate it.
> + */
> +ASSERT(ret == old_ire.val);
> +}
> +else
> +{
> +/*
> + * The following code will update irte atomically if possible.

There's nothing atomic below - between the compares and stores
the value in the table could change. Please don't make false
promises in comments.

> + * If the caller requests a atomic update but we can't meet it, 
> + * a bug will be raised.
> + */
> +if ( entry->lo == new_ire->lo )
> +entry->hi = new_ire->hi;
> +else if ( entry->hi == new_ire->hi )
> +entry->lo = new_ire->lo;

Best effort would still call for use of write_atomic() instead of both
of the assignments above.

> @@ -353,7 +409,11 @@ static int ioapic_rte_to_remap_entry(struct iommu *iommu,
>  remap_rte->format = 1;/* indicate remap format */
>  }
>  
> -*iremap_entry = new_ire;
> +if ( init )
> +update_irte_non_atomic(iremap_entry, &new_ire);
> +else
> +update_irte_atomic(iremap_entry, &new_ire);

Seems like you'd better call update_irte() here directly, instead of
having this if/else. Which puts under question the usefulness of the
two wrappers.

> @@ -639,7 +702,14 @@ static int msi_msg_to_remap_entry(
>  remap_rte->address_hi = 0;
>  remap_rte->data = index - i;
>  
> -*iremap_entry = new_ire;
> +if ( msi_desc->remap_entry_initialized )
> +update_irte_atomic(iremap_entry, &new_ire);
> +else
> +{
> +update_irte_non_atomic(iremap_entry, &new_ire);
> +msi_desc->remap_entry_initialized = true;
> +}

Same here.

I also wonder whether you really need the new flag: The function
knows whether it's initializing the IRTE for the first time, as it
allocates a table index just like ioapic_rte_to_remap_entry() does
(where you get away without a new flag).

Jan

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

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Jan Beulich]

>>> On 31.03.17 at 04:42,  wrote:
> On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote:
> On 29.03.17 at 07:11,  wrote:
>>> @@ -442,17 +397,24 @@ int pt_irq_create_bind(
>>>  dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>>>  pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>>>  spin_unlock(&d->event_lock);
>>> +
>>> +pirq_dpci->gmsi.posted = false;
>>> +vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
>>> +if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )
>>
>>Why again would dest_Fixed not allow posted delivery? This needs
> 
> No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives
> the same output with pi_find_dest_vcpu(). Thus we don't call it again, just
> use the return value of hvm_girq_dest_2_vcpu_id().

But as pointed out you don't set the new posted field in that case.

>>recording in a comment, if there really is such a restriction. Or did
>>you really mean to place ...
>>
>>> +{
>>> +vcpu = vector_hashing_dest(d, dest, dest_mode,
>>> +   pirq_dpci->gmsi.gvec);
>>> +if ( vcpu )
>>> +pirq_dpci->gmsi.posted = true;
>>
>>... this ...
>>
>>> +}
>>
>>... after this brace (which then wouldn't be needed anymore)? If
>>so, is there any point calling vector_hashing_dest() when vcpu is
>>already non-NULL prior to the if()?
>>
>>This then also raises the question whether the call to
>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest
>>priority delivery mode.
> 
> For lowest priority delivery mode, if VT-d PI is enabled, the result (the
> destination vcpu) is overrided by vector_hashing_dest() to keep the 
> existing behavior. I think the only point we should keep in mind is
> for cases other than lowest priority delivery mode, pi_find_dest_vcpu()
> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu.

Well, the override is done for the iommu_intpost case. The remark
on hvm_girq_dest_2_vcpu_id(), however, was made in general.

Jan


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

Re: [Xen-devel] Xen ARM community call - meeting minutes and date for the next one

[Julien Grall]

On 03/30/2017 09:17 PM, Volodymyr Babchuk wrote:

Hi Julien,


Hi Volodymyr,


On 30 March 2017 at 22:57, Julien Grall  wrote:

Hi Volodymyr

On 30/03/2017 20:19, Volodymyr Babchuk wrote:


On 30 March 2017 at 21:52, Stefano Stabellini 
wrote:


On Thu, 30 Mar 2017, Volodymyr Babchuk wrote:


And yes, my profiler shows that there are ways to further decrease
latency. Most obvious way is to get rid of 2nd stage translation and
thus eliminate p2m code from the call chain. Currently hypervisor
spends 20% of time in spinlocks code and about ~10-15% in p2m. So
there definitely are areas to improve :)



Correct me if I am wrong. You are suggesting to remove stage-2 MMU
translation, right? If so, what are the benefits to have some Xen code
running at EL0?

Because that will be loadable code :) Also, this will provide some
degree of isolation as this code will communicate with Xen only via
SVCs.Hi

Look, we need two stages in the first place because conventional guest
wants to control own MMU. But Xen native app (lets call it in this
way) should not control MMU. In my hack I had to create stage-1 table
with 1:1 mapping to make things work. Actually it just came to me
that I can try to disable stage 1 MMU and leave only stage 2. Not sure
if it is possible, need to check TRM...


I cannot see why stage-1 page table cannot be disabled.


But, anyways, my initial idea was to disable second stage MMU (drop VM
bit in HCR_EL2) and program only TTBR0_EL1 with friends. With this
approach there will be no need to save/restore p2m context when I
switch from guest context to app context and back.


Even if you disable stage-2 translation, you still have to program 
VTTBR_EL2.VMID because, from my understanding the TLBs will still be 
tagged with the VMID (see the note in page D4-1823 in ARM DDI 
0487A.k_iss10775).


If you keep the VMID of the guest, you would need to flush the TLBs at 
every context switch to avoid TLB conflict.


To prevent that, you would need a specific VMID for the EL0 app, and 
therefore save/restore a part of the p21m context everytime.


The benefits of using stage-2 page table, is you can leverage the p2m 
code rather than creating another version to handle stage-1.


Cheers,

--
Julien Grall

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

[Xen-devel] [ovmf test] 107022: regressions - FAIL

[osstest service owner]

flight 107022 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107022/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64-xsm   5 xen-buildfail REGR. vs. 107018
 build-i3865 xen-buildfail REGR. vs. 107018
 build-amd64   5 xen-buildfail REGR. vs. 107018
 build-i386-xsm5 xen-buildfail REGR. vs. 107018

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1) blocked n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a

version targeted for testing:
 ovmf af8388059a2805f61dd5d06c7cad21bb90a72526
baseline version:
 ovmf de87f23291620d36d69ec55ea53a1c38b8780f0b

Last test of basis   107018  2017-03-31 05:47:36 Z0 days
Testing same since   107022  2017-03-31 09:16:43 Z0 days1 attempts


People who touched revisions under test:
  Ruiyu Ni 

jobs:
 build-amd64-xsm  fail
 build-i386-xsm   fail
 build-amd64  fail
 build-i386   fail
 build-amd64-libvirt  blocked 
 build-i386-libvirt   blocked 
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked 
 test-amd64-i386-xl-qemuu-ovmf-amd64  blocked 



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


Not pushing.


commit af8388059a2805f61dd5d06c7cad21bb90a72526
Author: Ruiyu Ni 
Date:   Fri Sep 2 19:48:29 2016 +0800

UefiCpuPkg/MtrrLib: All functions use definitions in Msr.h

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit 10c361ad026f141985b65e58597b472fdfc33944
Author: Ruiyu Ni 
Date:   Fri Sep 2 18:10:45 2016 +0800

UefiCpuPkg/MtrrLib: Refine MtrrGetMemoryAttributeByAddressWorker

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit 8051302a36b31f7619dc7753280ba449f163b62f
Author: Ruiyu Ni 
Date:   Fri Sep 2 12:09:54 2016 +0800

UefiCpuPkg/MtrrLib: Use a better algorithm to calculate MTRR

The new algorithm finds out the more optimal MTRR solution for
current memory type settings.
Compare against the original algorithm, the new one guarantees
to find the correct MTRR solution, but doesn't guarantee to
find the most optimal MTRR solution.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit 012f4054dbf0debbe913422d0fd3545b36cb5dc9
Author: Ruiyu Ni 
Date:   Fri Sep 2 20:23:35 2016 +0800

UefiCpuPkg/MtrrLib: MtrrLibInitializeMtrrMask() uses definitions in CpuId.h

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit b8f015999e01100065322fed662efa9584697e37
Author: Ruiyu Ni 
Date:   Fri Sep 2 10:45:53 2016 +0800

UefiCpuPkg/MtrrLib: Add MtrrLib prefix to several internal functions

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit 94240f1b05fb72f1fd104713229c95b4541a8199
Author: Ruiyu Ni 
Date:   Fri Sep 2 10:41:28 2016 +0800

UefiCpuPkg/MtrrLib: Add MtrrLib prefix to ProgramFixedMtrr

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Reviewed-by: Jeff Fan 

commit 386f5785ea31fc535a7d85e7f477da2dc0a8c9ba
Author: Ruiyu Ni 
Date:   Fri Sep 2 10:35:45 2016 +0800

UefiCpuPkg/MtrrLib: GetVariableMtrrCountWorker uses definitions in Msr.h

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
   

[Xen-devel] [PATCH v2] xen, kdump: handle pv domain in paddr_vmcoreinfo_note()

[Juergen Gross]

For kdump to work correctly it needs the physical address of
vmcoreinfo_note. When running as dom0 this means the virtual address
has to be translated to the related machine address.

paddr_vmcoreinfo_note() is meant to do the translation via
__pa_symbol() only, but being attributed "weak" it can be replaced
easily in Xen case.

Signed-off-by: Juergen Gross 
---
Changes in V2:
- use __pa_symbol() (Boris Ostrovsky)
- remove unneeded casts (Jan Beulich)

This patch needs to be rebased on top of Vitaly's series to split
pv- and hvm-code. I'll do this as soon as his series is in the Xen
tree in its final form.
---
 arch/x86/xen/mmu.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 37cb5aa..33ab96c 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -49,6 +49,9 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_KEXEC_CORE
+#include 
+#endif
 
 #include 
 
@@ -2903,3 +2906,13 @@ int xen_unmap_domain_gfn_range(struct vm_area_struct 
*vma,
return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(xen_unmap_domain_gfn_range);
+
+#ifdef CONFIG_KEXEC_CORE
+phys_addr_t paddr_vmcoreinfo_note(void)
+{
+   if (xen_pv_domain())
+   return virt_to_machine(&vmcoreinfo_note).maddr;
+   else
+   return __pa_symbol(&vmcoreinfo_note);
+}
+#endif /* CONFIG_KEXEC_CORE */
-- 
2.10.2


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

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Jan Beulich]

>>> On 31.03.17 at 11:12,  wrote:
> On 17-03-31 02:47:25, Jan Beulich wrote:
>> >>> On 30.03.17 at 14:10,  wrote:
>> > On 17-03-30 05:55:52, Jan Beulich wrote:
>> >> >>> On 30.03.17 at 03:37,  wrote:
>> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
>> >> >> >>> On 29.03.17 at 03:36,  wrote:
>> >> >> > On 17-03-29 09:20:21, Yi Sun wrote:
>> >> >> >> On 17-03-28 06:20:48, Jan Beulich wrote:
>> >> >> >> > >>> On 28.03.17 at 13:59,  wrote:
>> >> >> >> > > I think we at least need a 'get_val()' hook.
>> >> >> >> > 
>> >> >> >> > Of course.
>> >> >> >> > 
>> >> >> >> > > I try to implement CAT/CDP hook.
>> >> >> >> > > Please help to check if this is what you thought.
>> >> >> >> > 
>> >> >> >> > One remark below, but other than that - yes.
>> >> >> >> > 
>> >> >> >> > > static void cat_get_val(const struct feat_node *feat, unsigned 
>> >> >> >> > > int cos,
>> >> >> >> > > enum cbm_type type, int flag, uint32_t 
>> >> >> >> > > *val)
>> >> >> >> > > {
>> >> >> >> > > *val = feat->cos_reg_val[cos];
>> >> >> >> > > }
>> >> >> >> > > 
>> >> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, 
>> >> >> >> > > unsigned int 
>> >> > cos,
>> >> >> >> > >enum cbm_type type, int flag, 
>> >> >> >> > > uint32_t *val)
>> >> >> >> > > {
>> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 )
>> >> >> >> > > *val = get_cdp_data(feat, cos);
>> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 )
>> >> >> >> > > *val = get_cdp_code(feat, cos);
>> >> >> >> > > }
>> >> >> >> > 
>> >> >> >> > Why the redundancy between type and flag?
>> >> >> >> > 
>> >> >> >> For psr_get_val, upper layer input the cbm_type to get either DATA 
>> >> >> >> or 
> CODE
>> >> >> >> value. For other cases, we use flag as cos_num index to get either 
>> >> >> >> DATA 
> or
>> >> >> >> CODE.
>> >> >> >> 
>> >> >> > Let me explain more to avoid confusion. For other cases, we use 
>> >> >> > cos_num 
> as
>> >> >> > index to get values from a feature. In these cases, we do not know 
>> >> >> > the
>> >> >> > cbm_type of the feature. So, I use the cos_num as flag to make 
>> >> >> > 'get_val'
>> >> >> > know which value should be returned.
>> >> >> 
>> >> >> I'm pretty sure this redundancy can be avoided.
>> >> >> 
>> >> > Then, I think I have to reuse the 'type'. As only CDP needs type to 
>> >> > decide
>> >> > which value to be returned so far, I think I can implement codes like 
> below
>> >> > to make CDP can handle all scenarios.
>> >> > 
>> >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int 
>> >> > cos,
>> >> >enum cbm_type type, uint32_t *val)
>> >> > {
>> >> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 )
>> >> > *val = get_cdp_data(feat, cos);
>> >> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 )
>> >> > *val = get_cdp_code(feat, cos);
>> >> > }
>> >> > 
>> >> > static bool fits_cos_max(...)
>> >> > {
>> >> > ..
>> >> > for (i = 0; i < feat->props->cos_num; i++)
>> >> > {
>> >> > feat->props->get_val(feat, cos, i + 0xF000, &default_val);
>> >> > if ( val[i] == default_val )
>> >> > ..
>> >> > }
>> >> > ..
>> >> > }
>> >> > 
>> >> > Is that good for you?
>> >> 
>> >> Urgh - no, not really: This is hackery. Do you have a tree
>> >> somewhere so I could look at the file after at least the CDP
>> >> patches have all been applied, to see whether I have a
>> >> better idea? Alternatively, could you attach psr.c the way
>> >> you have it right now with the entire series applied?
>> >> 
>> > I think you can check v9 codes here:
>> > https://github.com/yisun-git/xen/tree/l2_cat_v9 
>> 
>> Looking at this made me notice that cat_get_old_val() passes a
>> bogus literal 0 to cat_get_val(), which needs taking care of too.
>> One option I can see is for each feature to make available an
>> array of type enum cbm_type, with cos_num elements. The order
>> would match that of the order of values in their arrays. This will
> 
> Sorry, not very clear your meaning. How to do that? Could you please
> provide pieces of codes? Thanks!

I'm sorry, but I'm afraid I don't see how I would reasonably supply
code here without taking over your series altogether (which I don't
intend to do). What is unclear with, at the example of CDP, you
needing to add an array at initialization time, slot 0 of which holds
PSR_CBM_TYPE_L3_DATA and slot 1 PSR_CBM_TYPE_L3_CODE (or
the other way around). Granted I was wrong with the type of the
array (as the above aren't enum psr_feat_type enumerators, but
enum cbm_type ones), but I think the basic idea should have been
clear anyway: You need to provide a way for generic code to pass
suitable type information into ->get_val().

>> allow elimination of all of the get_old_val, compare_val, and
>> fits_cos_max hooks afaict. At that point the "new" in
>> set_new_val is also no longer needed to 

Re: [Xen-devel] [PATCH] xl: Add 'pvh' config option

[Roger Pau Monné]

On Thu, Mar 30, 2017 at 07:06:15PM -0400, Boris Ostrovsky wrote:
> In addition to 'device_model_version="none"' config option users can
> also use 'pvh=1' in xl configuration file when creating a PVH guest.

I'm not sure, but I think the plan was to remove device_model_version="none"
and instead just use pvh=1, instead of keeping both. I don't have a strong
opinion here, so I will leave that to the xl maintainers.

I'm also not sure, but if you use device_model_version="none" can you use QDISK
for PVH disk backend? (and pygrub).

> We can skip parsing options related to device model once we establish
> that we are building PVH guest.
> 
> Also process 'device_model_version="none"' for HVM guests only since
> it is not a valid model for PV guests.
> 
> Signed-off-by: Boris Ostrovsky 
> ---
>  docs/man/xl.cfg.pod.5.in |  7 ++-
>  tools/xl/xl_parse.c  | 12 +++-
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
> index 206d33e..5833987 100644
> --- a/docs/man/xl.cfg.pod.5.in
> +++ b/docs/man/xl.cfg.pod.5.in
> @@ -1201,6 +1201,11 @@ expose unexpected bugs in the guest, or find bugs in 
> Xen, so it is
>  possible to disable this feature.  Use of out of sync page tables,
>  when Xen thinks it appropriate, is the default.
>  
> +=item B
> +
> +Don't use any device model. This requires a kernel capable of booting
> +without emulated devices. Default is 0.
> +
>  =item B
>  
>  Number of megabytes to set aside for shadowing guest pagetable pages
> @@ -1966,7 +1971,7 @@ This device-model is still the default for NetBSD dom0.
>  =item B
>  
>  Don't use any device model. This requires a kernel capable of booting
> -without emulated devices.
> +without emulated devices. This is a synonym for L option above.
>  
>  =back
>  
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 66327dc..aa591cd 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1817,6 +1817,12 @@ skip_usbdev:
>  break;
>  }
>  
> +if (c_info->type == LIBXL_DOMAIN_TYPE_HVM &&

Hm, this will mean that the user needs to specify:

builder="hvm"
pvh=1

Or else the option is not going to be parsed.

> +!xlu_cfg_get_long(config, "pvh", &l, 0) && l) {
> +b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_NONE;
> +goto skip_device_model;
> +}
> +

Roger.

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

[Xen-devel] [For Xen-4.10 RFC PATCH 0/3] Reduce unnecessary icache maintenance operations

[Punit Agrawal]

Hi,

Although icache maintenance operations are required when changing page
table mappings, in certain situations they are being called far more
frequently than necessary.

As icache maintenance has a performance penalty, one that increases
with the number of cores in the system, this RFC attempts to improve
the situation by hoisting knowledge of required icache maintenance
operations to higher layers.

Patch 1 adds a parameter to flush_page_to_ram() to prevent performing
icache maintenance per page. Current calls to flush_page_to_ram() loop
over pages and performing a full icache flush for each page is
excessive.

Patch 2 hoists icache maintenance from flush_page_to_ram() to
p2m_cache_flush().

Patch 3 introduces a new MEMF_ flag to indicate to alloc_heap_pages()
that icache maintenance will be performed by the caller. The icache
maintenance operations are performed in populate_physmap(). As I
couldn't icache maintenance operations for x86, an empty helper is
introduced.

Is it reasonable to move cache maintenance operation to higher layers?
The alternative would be perform cache maintenance operations on
ranges larger than page size but the benefit (reduced icache
operations) might not be equivalent.

This is my first contribution to Xen, so please go gentle if I've
missed something obvious.

Thanks,
Punit

Punit Agrawal (3):
  Allow control of icache invalidations when calling flush_page_to_ram()
  arm: p2m: Prevent redundant icache flushes
  Prevent redundant icache flushes in populate_physmap()

 xen/arch/arm/mm.c  | 5 +++--
 xen/arch/arm/p2m.c | 4 +++-
 xen/common/memory.c| 6 ++
 xen/common/page_alloc.c| 2 +-
 xen/include/asm-arm/page.h | 2 +-
 xen/include/asm-x86/flushtlb.h | 2 +-
 xen/include/asm-x86/page.h | 4 
 xen/include/xen/mm.h   | 2 ++
 8 files changed, 21 insertions(+), 6 deletions(-)

-- 
2.11.0


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

[Xen-devel] [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Punit Agrawal]

populate_physmap() calls alloc_heap_pages() per requested extent. As
alloc_heap_pages() performs icache maintenance operations affecting the
entire instruction cache, this leads to redundant cache flushes when
allocating multiple extents in populate_physmap().

To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
which can be used prevent alloc_heap_pages() to perform unnecessary
icache maintenance operations. Use the flag in populate_physmap() and
perform the required icache maintenance function at the end of the
operation.

Signed-off-by: Punit Agrawal 
---
 xen/common/memory.c| 6 ++
 xen/common/page_alloc.c| 2 +-
 xen/include/asm-x86/page.h | 4 
 xen/include/xen/mm.h   | 2 ++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index ad0b33ceb6..507f363924 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
 if ( unlikely(!d->creation_finished) )
 a->memflags |= MEMF_no_tlbflush;
 
+a->memflags |= MEMF_no_icache_flush;
+
 for ( i = a->nr_done; i < a->nr_extents; i++ )
 {
 if ( i != a->nr_done && hypercall_preempt_check() )
@@ -253,6 +255,10 @@ static void populate_physmap(struct memop_args *a)
 out:
 if ( need_tlbflush )
 filtered_flush_tlb_mask(tlbflush_timestamp);
+
+if ( a->memflags & MEMF_no_icache_flush )
+invalidate_icache();
+
 a->nr_done = i;
 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 15450a3b6d..1a51bc6b15 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -838,7 +838,7 @@ static struct page_info *alloc_heap_pages(
 /* Ensure cache and RAM are consistent for platforms where the
  * guest can control its own visibility of/through the cache.
  */
-flush_page_to_ram(page_to_mfn(&pg[i]), true);
+flush_page_to_ram(page_to_mfn(&pg[i]), !(memflags & 
MEMF_no_icache_flush));
 }
 
 spin_unlock(&heap_lock);
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index bc5946b9d2..13dc9e2299 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -375,6 +375,10 @@ perms_strictly_increased(uint32_t old_flags, uint32_t 
new_flags)
 
 #define PAGE_ALIGN(x) (((x) + PAGE_SIZE - 1) & PAGE_MASK)
 
+static inline void invalidate_icache(void)
+{
+}
+
 #endif /* __X86_PAGE_H__ */
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 88de3c1fa6..ee50d4cd7b 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -224,6 +224,8 @@ struct npfec {
 #define  MEMF_no_owner(1U<<_MEMF_no_owner)
 #define _MEMF_no_tlbflush 6
 #define  MEMF_no_tlbflush (1U<<_MEMF_no_tlbflush)
+#define _MEMF_no_icache_flush 7
+#define  MEMF_no_icache_flush (1U<<_MEMF_no_icache_flush)
 #define _MEMF_node8
 #define  MEMF_node_mask   ((1U << (8 * sizeof(nodeid_t))) - 1)
 #define  MEMF_node(n) n) + 1) & MEMF_node_mask) << _MEMF_node)
-- 
2.11.0


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

[Xen-devel] [For Xen-4.10 RFC PATCH 2/3] arm: p2m: Prevent redundant icache flushes

[Punit Agrawal]

When toolstack requests flushing the caches, flush_page_to_ram() is
called for each page of the requested domain. This needs to unnecessary
icache invalidation operations.

Let's take the responsibility of performing icache operations and use
the recently introduced flag to prevent redundant icache operations by
flush_page_to_ram().

Signed-off-by: Punit Agrawal 
---
 xen/arch/arm/p2m.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 76cd1c34f3..8136522ed8 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1400,13 +1400,15 @@ int p2m_cache_flush(struct domain *d, gfn_t start, 
unsigned long nr)
 /* XXX: Implement preemption */
 while ( gfn_x(start) < gfn_x(next_gfn) )
 {
-flush_page_to_ram(mfn_x(mfn), true);
+flush_page_to_ram(mfn_x(mfn), false);
 
 start = gfn_add(start, 1);
 mfn = mfn_add(mfn, 1);
 }
 }
 
+invalidate_icache();
+
 p2m_read_unlock(p2m);
 
 return 0;
-- 
2.11.0


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

[Xen-devel] [For Xen-4.10 RFC PATCH 1/3] Allow control of icache invalidations when calling flush_page_to_ram()

[Punit Agrawal]

flush_page_to_ram() unconditionally drops the icache. In certain
situations this leads to execessive icache flushes when
flush_page_to_ram() ends up being repeatedly called in a loop.

Introduce a parameter to allow callers of flush_page_to_ram() to take
responsibility of synchronising the icache. This is in preparations for
adding logic to make the callers perform the necessary icache
maintenance operations.

Signed-off-by: Punit Agrawal 
---
 xen/arch/arm/mm.c  | 5 +++--
 xen/arch/arm/p2m.c | 2 +-
 xen/common/page_alloc.c| 2 +-
 xen/include/asm-arm/page.h | 2 +-
 xen/include/asm-x86/flushtlb.h | 2 +-
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index f0a2eddaaf..e1ab5f8694 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -383,7 +383,7 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
 }
 #endif
 
-void flush_page_to_ram(unsigned long mfn)
+void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
 void *v = map_domain_page(_mfn(mfn));
 
@@ -398,7 +398,8 @@ void flush_page_to_ram(unsigned long mfn)
  * I-Cache (See D4.9.2 in ARM DDI 0487A.k_iss10775). Instead of using flush
  * by VA on select platforms, we just flush the entire cache here.
  */
-invalidate_icache();
+if (sync_icache)
+invalidate_icache();
 }
 
 void __init arch_init_memory(void)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 626376090d..76cd1c34f3 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1400,7 +1400,7 @@ int p2m_cache_flush(struct domain *d, gfn_t start, 
unsigned long nr)
 /* XXX: Implement preemption */
 while ( gfn_x(start) < gfn_x(next_gfn) )
 {
-flush_page_to_ram(mfn_x(mfn));
+flush_page_to_ram(mfn_x(mfn), true);
 
 start = gfn_add(start, 1);
 mfn = mfn_add(mfn, 1);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 42c20cb201..15450a3b6d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -838,7 +838,7 @@ static struct page_info *alloc_heap_pages(
 /* Ensure cache and RAM are consistent for platforms where the
  * guest can control its own visibility of/through the cache.
  */
-flush_page_to_ram(page_to_mfn(&pg[i]));
+flush_page_to_ram(page_to_mfn(&pg[i]), true);
 }
 
 spin_unlock(&heap_lock);
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4b46e8831c..497b4c86ad 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -407,7 +407,7 @@ static inline void flush_xen_data_tlb_range_va(unsigned 
long va,
 }
 
 /* Flush the dcache for an entire page. */
-void flush_page_to_ram(unsigned long mfn);
+void flush_page_to_ram(unsigned long mfn, bool sync_icache);
 
 /*
  * Print a walk of a page table or p2m
diff --git a/xen/include/asm-x86/flushtlb.h b/xen/include/asm-x86/flushtlb.h
index 8b7adef7c5..bd2be7e482 100644
--- a/xen/include/asm-x86/flushtlb.h
+++ b/xen/include/asm-x86/flushtlb.h
@@ -118,7 +118,7 @@ void flush_area_mask(const cpumask_t *, const void *va, 
unsigned int flags);
 #define flush_tlb_one_all(v)\
 flush_tlb_one_mask(&cpu_online_map, v)
 
-static inline void flush_page_to_ram(unsigned long mfn) {}
+static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache) {}
 static inline int invalidate_dcache_va_range(const void *p,
  unsigned long size)
 { return -EOPNOTSUPP; }
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Jan Beulich]

>>> On 31.03.17 at 01:10,  wrote:
> On Fri, Mar 31, 2017 at 01:28:02PM +0800, Tian, Kevin wrote:
>>> From: Chao Gao
>>> Sent: Wednesday, March 29, 2017 1:12 PM
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -438,6 +438,9 @@ static int hvm_migrate_pirq(struct domain *d, struct
>>> hvm_pirq_dpci *pirq_dpci,
>>>  struct vcpu *v = arg;
>>> 
>>>  if ( (pirq_dpci->flags & HVM_IRQ_DPCI_MACH_MSI) &&
>>> + (pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI) &&
>>> + /* Needn't migrate pirq if this pirq is delivered to guest 
>>> directly.*/
>>> + (!pirq_dpci->gmsi.posted) &&
>>>   (pirq_dpci->gmsi.dest_vcpu_id == v->vcpu_id) )
>>
>>simply looking at above change it's more than what you intend to change.
>>Previously even w/o GUEST_MSI flag will fall into that path, but now
>>you limit it to only GUEST_MSI and irq remapping (i.e. changed the
>>behavior for both posted case and w/o GUEST_MSI case). I haven't looked
>>whether MACH_MASI always set with GUEST_MSI, but my gut-feeling 
>>looks not correct here.
> 
> Yes. It's a problem. I think the original code may be wrong for it acquires
> gmsi.dest_vcpu_id without checking 'flags' of pirq_dpci. Do we need to migrate
> pirq when its type is GUEST_PCI?

It probably would be nice, but we don't know where to migrate it to:
The names in "pirq_dpci->gmsi.dest_vcpu_id" should make this pretty
clear, I would think.

Jan


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

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Chao Gao]

On Fri, Mar 31, 2017 at 04:06:27AM -0600, Jan Beulich wrote:
 On 31.03.17 at 04:42,  wrote:
>> On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote:
>> On 29.03.17 at 07:11,  wrote:
 @@ -442,17 +397,24 @@ int pt_irq_create_bind(
  dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
  pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
  spin_unlock(&d->event_lock);
 +
 +pirq_dpci->gmsi.posted = false;
 +vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
 +if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )
>>>
>>>Why again would dest_Fixed not allow posted delivery? This needs
>> 
>> No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives
>> the same output with pi_find_dest_vcpu(). Thus we don't call it again, just
>> use the return value of hvm_girq_dest_2_vcpu_id().
>
>But as pointed out you don't set the new posted field in that case.
>

Indeed.

How about this:
pirq_dpci->gmsi.posted = false;
if ( iommu_intpost )
{
vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
if ( delivery_mode == dest_LowestPrio )
vcpu = vector_hashing_dest(d, dest, dest_mode,
   pirq_dpci->gmsi.gvec);
if ( vcpu )
pirq_dpci->gmsi.posted = true;
}

>>>recording in a comment, if there really is such a restriction. Or did
>>>you really mean to place ...
>>>
 +{
 +vcpu = vector_hashing_dest(d, dest, dest_mode,
 +   pirq_dpci->gmsi.gvec);
 +if ( vcpu )
 +pirq_dpci->gmsi.posted = true;
>>>
>>>... this ...
>>>
 +}
>>>
>>>... after this brace (which then wouldn't be needed anymore)? If
>>>so, is there any point calling vector_hashing_dest() when vcpu is
>>>already non-NULL prior to the if()?
>>>
>>>This then also raises the question whether the call to
>>>hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest
>>>priority delivery mode.
>> 
>> For lowest priority delivery mode, if VT-d PI is enabled, the result (the
>> destination vcpu) is overrided by vector_hashing_dest() to keep the 
>> existing behavior. I think the only point we should keep in mind is
>> for cases other than lowest priority delivery mode, pi_find_dest_vcpu()
>> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu.
>
>Well, the override is done for the iommu_intpost case. The remark
>on hvm_girq_dest_2_vcpu_id(), however, was made in general.

Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match the 
method
used by real hardware. I will check it.

Thanks
Chao

>
>Jan
>

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

Re: [Xen-devel] [PATCH v11 1/6] passthrough: don't migrate pirq when it is delivered through VT-d PI

[Jan Beulich]

>>> On 31.03.17 at 05:27,  wrote:
> On Fri, Mar 31, 2017 at 04:06:27AM -0600, Jan Beulich wrote:
> On 31.03.17 at 04:42,  wrote:
>>> On Fri, Mar 31, 2017 at 03:31:47AM -0600, Jan Beulich wrote:
>>> On 29.03.17 at 07:11,  wrote:
> @@ -442,17 +397,24 @@ int pt_irq_create_bind(
>  dest_vcpu_id = hvm_girq_dest_2_vcpu_id(d, dest, dest_mode);
>  pirq_dpci->gmsi.dest_vcpu_id = dest_vcpu_id;
>  spin_unlock(&d->event_lock);
> +
> +pirq_dpci->gmsi.posted = false;
> +vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> +if ( iommu_intpost && (delivery_mode == dest_LowestPrio) )

Why again would dest_Fixed not allow posted delivery? This needs
>>> 
>>> No this restriction. For dest_Fixed case, hvm_girq_dest_2_vcpu_id() gives
>>> the same output with pi_find_dest_vcpu(). Thus we don't call it again, just
>>> use the return value of hvm_girq_dest_2_vcpu_id().
>>
>>But as pointed out you don't set the new posted field in that case.
>>
> 
> Indeed.
> 
> How about this:
>   pirq_dpci->gmsi.posted = false;
> if ( iommu_intpost )
> {
> vcpu = (dest_vcpu_id >= 0) ? d->vcpu[dest_vcpu_id] : NULL;
> if ( delivery_mode == dest_LowestPrio )
> vcpu = vector_hashing_dest(d, dest, dest_mode,
>pirq_dpci->gmsi.gvec);
> if ( vcpu )
> pirq_dpci->gmsi.posted = true;
> }

I think the setting of vcpu needs to stay outside the if(), but other
than that the above looks reasonable at a first glance.

This then also raises the question whether the call to
hvm_girq_dest_2_vcpu_id() is actually legitimate for lowest
priority delivery mode.
>>> 
>>> For lowest priority delivery mode, if VT-d PI is enabled, the result (the
>>> destination vcpu) is overrided by vector_hashing_dest() to keep the 
>>> existing behavior. I think the only point we should keep in mind is
>>> for cases other than lowest priority delivery mode, pi_find_dest_vcpu()
>>> and hvm_girq_dest_2_vcpu_id() give the same destination vcpu.
>>
>>Well, the override is done for the iommu_intpost case. The remark
>>on hvm_girq_dest_2_vcpu_id(), however, was made in general.
> 
> Ok. You meant the method using in hvm_girq_dest_2_vcpu_id() may not match 
> the method
> used by real hardware. I will check it.

I mean that the method used there is pretty clearly "fixed" mode
handling. Thanks for checking.

Jan


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

Re: [Xen-devel] Legacy PCI interrupt {de}assertion count

[Jan Beulich]

>>> On 31.03.17 at 10:07,  wrote:
> On Fri, Mar 31, 2017 at 05:05:44AM +, Tian, Kevin wrote:
>> > From: Jan Beulich [mailto:jbeul...@suse.com]
>> > Sent: Monday, March 27, 2017 4:00 PM
>> > 
>> > >>> On 24.03.17 at 17:54,  wrote:
>> > > As I understand it, for level triggered legacy PCI interrupts Xen sets
>> > > up a timer in order to perform the EOI if the guest takes too long in
>> > > deasserting the line. This is done in pt_irq_time_out. What I don't
>> > > understand is why this function also does a deassertion of the guest view
>> > of the PCI interrupt, ie:
>> > > why it calls hvm_pci_intx_deassert. This AFAICT will clear the pending
>> > > assert in the guest, and thus the guest will end up loosing one 
>> > > interrupt.
>> > 
>> > Especially with the comment next to the respective set_timer() it looks to 
>> > me
>> > as if this was the intended effect: If the guest didn't care to at least 
>> > start
>> > handling the interrupt within PT_IRQ_TIME_OUT, we want it look to be lost 
>> > in
>> > order to not have it block other interrupts inside the guest (i.e. there's 
>> > more
>> > to it than just guarding the host here).
>> > 
>> > "Luckily" commit 0f843ba00c ("vt-d: Allow pass-through of shared
>> > interrupts") introducing this has no description at all. Let's see if Kevin
>> > remembers any further details ...
>> > 
>> 
>> Sorry I don't remember more detail other than existing comments.
>> Roger, did you encounter a problem now?
> 
> No, I didn't encounter any problems with this so far, any well behaved guest
> will deassert those lines anyway, it just seems to be against the spec.  AFAIK
> on bare metal the line will be asserted until the OS deasserts it, so I was
> wondering if this was some kind of workaround?

"OS deasserts" is a term I don't understand. Aiui it's the origin device
which would need to de-assert its interrupt, and I think it is not
uncommon for devices to de-assert interrupts after a certain amount
of time. If that wasn't the case, spurious interrupts could never occur.

Jan


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

Re: [Xen-devel] Outreachy Project

[Lars Kurth]

Nazira,
please check https://wiki.xenproject.org/wiki/2017-Summer-Internships 
 and then 
https://wiki.xenproject.org/wiki/Outreach_Program_Projects 
, which contains a 
list of possible project including their mentors. Please also check the 
Outreachy Eligibility requirements on https://wiki.gnome.org/Outreachy 
. If in doubt, please ask 
outreachy-l...@gnome.org   and CC me.
Best Regards
Lars

> On 31 Mar 2017, at 09:45, Nakka Ricci  wrote:
> 
> Hello! 
> 
> My name is Nazira and I am interested in the project called "Xen Hypervisor". 
> This mail is displayed as a contact information, so I wanted to ask how to 
> connect to a mentor of the project? 
> ___
> 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] [PATCH] Enable compiling with gcc tracing

[Felix Schmoll]

2017-03-30 17:46 GMT+02:00 Wei Liu :

> Cool. I think Xen should boot with this.
>
> You've successfully finished what I asked for. Now you can divert your
> energy to polish your application. :-)
>
> On Thu, Mar 30, 2017 at 05:39:56PM +0200, Felix Schmoll wrote:
> > Make minimal adjustments in order to enable the compilation of the
> > xen source-code with gcc-6's -fsanitize-coverage=trace-pc option.
> >
> > Due to a bug in Xen's build-system the flag for the compiler has
> > to be handed in via the command line, i.e. for compiling one would
> > use:
> >
> >   make CC=
> >
>
> OOI, how can I reproduce the bug you met?
>
> If you mean it doesn't work with:
>
>CC=XXX make
>
> it is expected.
>
Yeah, this is what I meant.

I recalled having read on some archived mailing list that it was a bug, but
whatever.

Felix

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

Hi Julien,

On 2017/3/31 2:38, Julien Grall wrote:
>
>
> On 30/03/17 19:32, Julien Grall wrote:
>> On 30/03/17 19:28, Julien Grall wrote:
>>> Hi Wei,
>>>
>>> On 30/03/17 10:13, Wei Chen wrote:
 +void synchronize_serror(void)
>>>
>>> Sorry for been late in the party. Looking at the way you use the
>>> function, you execute depending on the behavior chosen by the user when
>>> an SErrors happen. This behavior will not change at runtime, so always
>>> checking the option chosen in the hot path does not sound very efficient.
>>>
>>> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
>>> I.e
>>>
>>> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
>>> the place.
>>
>> To complete what I was suggestion, you could define:
>>
>> /* Synchronize SError unless the feature is selected */
>> #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>
> Or even:
>
> /*
>   * Synchronize SError unless the feature is selected.
>   * This is relying on the SErrors are currently unmasked.
>   */
> #define SYNCHRONIZE_SERROR(feat)\
>   do {  \
> ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
> ALTERNATIVE("dsb sy; isb", "nop; nop"); \
>   while (0)
>
> The ASSERT is here to check that we have abort enabled. Otherwise, doing
> the synchronization would be pointless.
>

Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.

I prefer to define a similar macro as the following:
#define SYNCHRONIZE_SERROR(feat)\
 do {\
 ALTERNATIVE("dsb sy; isb" : : : "memory", "nop; nop", feat);\
 } while (0)

> Note that the function local_abort_is_enabled is not implemented. But it
> is easy to write it. Looking how local_irq_is_enabled was implemented.
>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v4 3/4] xenstore: rework of transaction handling

[Wei Liu]

On Thu, Mar 30, 2017 at 06:41:11PM +0200, Juergen Gross wrote:
[...]
> > 
> >>node->generation = generation++;
> >> -  return;
> >> +  if (conn && !conn->transaction)
> >> +  wrl_apply_debit_direct(conn);
> >> +  }
> >> +
> >> +  if (!conn || !conn->transaction) {
> >> +  if (key)
> >> +  set_tdb_key(node->name, key);
> >> +  return 0;
> >>}
> >>  
> >>trans = conn->transaction;
> >>  
> >> -  node->generation = generation + trans->trans_gen++;
> >> +  trans_name = transaction_get_node_name(node, trans, node->name);
> >> +  if (!trans_name)
> >> +  goto nomem;
> >>  
> >> -  list_for_each_entry(i, &trans->changes, list) {
> >> -  if (streq(i->node, node->name)) {
> >> -  if (recurse)
> >> -  i->recurse = recurse;
> >> -  return;
> >> -  }
> >> -  }
> >> -
> >> -  i = talloc(trans, struct changed_node);
> >> +  i = find_accessed_node(trans, node->name);
> >>if (!i) {
> >> -  /* All we can do is let the transaction fail. */
> >> -  generation++;
> >> -  return;
> >> +  i = talloc_zero(trans, struct accessed_node);
> >> +  if (!i)
> >> +  goto nomem;
> >> +  i->node = talloc_strdup(i, node->name);
> >> +  if (!i->node)
> >> +  goto nomem;
> >> +
> >> +  introduce = true;
> >> +  i->ta_node = false;
> >> +
> >> +  /* We have to verify read nodes only if we didn't write them. */
> >> +  if (type == NODE_ACCESS_READ) {
> >> +  i->generation = node->generation;
> >> +  i->check_gen = true;
> >> +  if (node->generation != NO_GENERATION) {
> >> +  set_tdb_key(trans_name, &local_key);
> >> +  ret = write_node_raw(conn, &local_key, node);
> >> +  if (ret)
> >> +  goto err;
> >> +  i->ta_node = true;
> >> +  }
> >> +  }
> > 
> > I think the current code structure is a bit confusing.
> > 
> > For write types, the call to write_node_raw is outside of this function
> > but for read type it is inside this function.
> 
> This is due to the different purpose of write_node_raw() in both
> cases:
> 
> In the read case we write an _additional_ node into the transaction,
> while in the write case it is just the user's operation.
> 
> > 
> >> +  list_add_tail(&i->list, &trans->accessed);
> >>}
> >> -  i->node = talloc_strdup(i, node->name);
> >> -  if (!i->node) {
> >> -  /* All we can do is let the transaction fail. */
> >> -  generation++;
> >> +
> >> +  if (type != NODE_ACCESS_READ)
> >> +  i->modified = true;
> >> +
> >> +  if (introduce && type == NODE_ACCESS_DELETE)
> >> +  /* Nothing to delete. */
> >> +  return -1;
> >> +
> >> +  if (key) {
> >> +  set_tdb_key(trans_name, key);
> >> +  if (type == NODE_ACCESS_WRITE)
> >> +  i->ta_node = true;
> >> +  if (type == NODE_ACCESS_DELETE)
> >> +  i->ta_node = false;
> > 
> > The same caveat made me wonder if ta_node was really what it meant to
> > be because I couldn't find where write_node_raw was.
> > 
> > Can I suggest you make them consistent? Either lift the write_node_raw
> > for read to read_node or push down the write_node_raw in delete_node and
> > write_node here?
> > 
> > If I misunderstand how this works, please tell me...
> 
> As I already said: in the read case writing the node is depending
> on the context and lifting it up would seem rather strange: why
> should reading a node contain a write_node_raw() call?
> 
> In the write case I believe we should keep write_node_raw() where
> it belongs: in the appropriate function modifying the node.
> 
> All actions in access_node() are transaction specific and _additional_
> in the transaction case.


OK, my line of reasoning is that whatever DB operation is necessary to
make a functionality work should be consistent. That means even having a
DB write in read is OK.  But I think your explanation is fine, too.

May I then suggest adding the following to the write for read type in
access_node?

  /* Additional transaction-specific node for read type. We only have to 
   * verify read nodes only if we didn't write them.
   *
   * The node is created and written to DB here to distinguish from the
   * write types.
   */

Wei.

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

Re: [Xen-devel] Outreachy project - Xen Code Review Dashboard

[Lars Kurth]

Hi,

> On 31 Mar 2017, at 02:48, Vaishnavi Ramesh Jayaraman 
>  wrote:
> 
> Hi,
> I am Vaishnavi, interested in contributing to the Xen Project as part of the 
> Outreachy Program. I am particularly interested in working on the Xen Code 
> Review Dashboard.
> 
> I have worked on the ElasticSearch - Logstash- Kibana (ELK) stack previously 
> and am comfortable with Javascript.
> 
> It would be great if you could give me pointers on how to get started!

You may want to look at http://markmail.org/message/7adkmords3imkswd 


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

Re: [Xen-devel] [PATCH v4 3/4] xenstore: rework of transaction handling

[Juergen Gross]

On 31/03/17 13:00, Wei Liu wrote:
> On Thu, Mar 30, 2017 at 06:41:11PM +0200, Juergen Gross wrote:
> [...]
>>>
node->generation = generation++;
 -  return;
 +  if (conn && !conn->transaction)
 +  wrl_apply_debit_direct(conn);
 +  }
 +
 +  if (!conn || !conn->transaction) {
 +  if (key)
 +  set_tdb_key(node->name, key);
 +  return 0;
}
  
trans = conn->transaction;
  
 -  node->generation = generation + trans->trans_gen++;
 +  trans_name = transaction_get_node_name(node, trans, node->name);
 +  if (!trans_name)
 +  goto nomem;
  
 -  list_for_each_entry(i, &trans->changes, list) {
 -  if (streq(i->node, node->name)) {
 -  if (recurse)
 -  i->recurse = recurse;
 -  return;
 -  }
 -  }
 -
 -  i = talloc(trans, struct changed_node);
 +  i = find_accessed_node(trans, node->name);
if (!i) {
 -  /* All we can do is let the transaction fail. */
 -  generation++;
 -  return;
 +  i = talloc_zero(trans, struct accessed_node);
 +  if (!i)
 +  goto nomem;
 +  i->node = talloc_strdup(i, node->name);
 +  if (!i->node)
 +  goto nomem;
 +
 +  introduce = true;
 +  i->ta_node = false;
 +
 +  /* We have to verify read nodes only if we didn't write them. */
 +  if (type == NODE_ACCESS_READ) {
 +  i->generation = node->generation;
 +  i->check_gen = true;
 +  if (node->generation != NO_GENERATION) {
 +  set_tdb_key(trans_name, &local_key);
 +  ret = write_node_raw(conn, &local_key, node);
 +  if (ret)
 +  goto err;
 +  i->ta_node = true;
 +  }
 +  }
>>>
>>> I think the current code structure is a bit confusing.
>>>
>>> For write types, the call to write_node_raw is outside of this function
>>> but for read type it is inside this function.
>>
>> This is due to the different purpose of write_node_raw() in both
>> cases:
>>
>> In the read case we write an _additional_ node into the transaction,
>> while in the write case it is just the user's operation.
>>
>>>
 +  list_add_tail(&i->list, &trans->accessed);
}
 -  i->node = talloc_strdup(i, node->name);
 -  if (!i->node) {
 -  /* All we can do is let the transaction fail. */
 -  generation++;
 +
 +  if (type != NODE_ACCESS_READ)
 +  i->modified = true;
 +
 +  if (introduce && type == NODE_ACCESS_DELETE)
 +  /* Nothing to delete. */
 +  return -1;
 +
 +  if (key) {
 +  set_tdb_key(trans_name, key);
 +  if (type == NODE_ACCESS_WRITE)
 +  i->ta_node = true;
 +  if (type == NODE_ACCESS_DELETE)
 +  i->ta_node = false;
>>>
>>> The same caveat made me wonder if ta_node was really what it meant to
>>> be because I couldn't find where write_node_raw was.
>>>
>>> Can I suggest you make them consistent? Either lift the write_node_raw
>>> for read to read_node or push down the write_node_raw in delete_node and
>>> write_node here?
>>>
>>> If I misunderstand how this works, please tell me...
>>
>> As I already said: in the read case writing the node is depending
>> on the context and lifting it up would seem rather strange: why
>> should reading a node contain a write_node_raw() call?
>>
>> In the write case I believe we should keep write_node_raw() where
>> it belongs: in the appropriate function modifying the node.
>>
>> All actions in access_node() are transaction specific and _additional_
>> in the transaction case.
> 
> 
> OK, my line of reasoning is that whatever DB operation is necessary to
> make a functionality work should be consistent. That means even having a
> DB write in read is OK.  But I think your explanation is fine, too.
> 
> May I then suggest adding the following to the write for read type in
> access_node?
> 
>   /* Additional transaction-specific node for read type. We only have to 
>* verify read nodes only if we didn't write them.
>*
>* The node is created and written to DB here to distinguish from the
>* write types.
>*/

Will do.

Thanks,


Juergen


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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Julien Grall]

On 31/03/17 11:55, Wei Chen wrote:

Hi Julien,


Hi Wei,


On 2017/3/31 2:38, Julien Grall wrote:



On 30/03/17 19:32, Julien Grall wrote:

On 30/03/17 19:28, Julien Grall wrote:

Hi Wei,

On 30/03/17 10:13, Wei Chen wrote:

+void synchronize_serror(void)


Sorry for been late in the party. Looking at the way you use the
function, you execute depending on the behavior chosen by the user when
an SErrors happen. This behavior will not change at runtime, so always
checking the option chosen in the hot path does not sound very efficient.

I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
I.e

ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
the place.


To complete what I was suggestion, you could define:

/* Synchronize SError unless the feature is selected */
#define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");


Or even:

/*
  * Synchronize SError unless the feature is selected.
  * This is relying on the SErrors are currently unmasked.
  */
#define SYNCHRONIZE_SERROR(feat)\
   do {  \
 ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
 ALTERNATIVE("dsb sy; isb", "nop; nop");  \
   while (0)

The ASSERT is here to check that we have abort enabled. Otherwise, doing
the synchronization would be pointless.



Think a bit more about it. This macro will import more check than read
serror_op. This macro seems as a generic macro, it can be used in any
place. So you enable the feature check and abort check. But in this
patch, we know where the macro will be used, we know the abort is
enabled. And want to reduce the overhead as much as possible.


ASSERTs are used to verify your assumption is correct, for non-debug 
built they will be turned into a NOP.


I care about overhead in non-debug build, it is not much a concern for 
debug build. In the latter it is more important to verify the correctness.


The abort bit is enabled at the entry in the hypervisor, and neither you 
nor I can tell if we need to disable abort in the future in some path. 
This ASSERT will catch those changes.


So please give me a reason to remove it, because I see only benefits so far.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 16/19] xen/arm: Introduce a helper to synchronize SError

[Wei Chen]

On 2017/3/31 19:06, Julien Grall wrote:
>
>
> On 31/03/17 11:55, Wei Chen wrote:
>> Hi Julien,
>
> Hi Wei,
>
>> On 2017/3/31 2:38, Julien Grall wrote:
>>>
>>>
>>> On 30/03/17 19:32, Julien Grall wrote:
 On 30/03/17 19:28, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:13, Wei Chen wrote:
>> +void synchronize_serror(void)
>
> Sorry for been late in the party. Looking at the way you use the
> function, you execute depending on the behavior chosen by the user when
> an SErrors happen. This behavior will not change at runtime, so always
> checking the option chosen in the hot path does not sound very efficient.
>
> I would recommend to look at ALTERNATIVE and streamline (dsb sy, isb).
> I.e
>
> ALTERNATIVE("dsb sy; isb", "nop; nop", ...) or the invert depending of
> the place.

 To complete what I was suggestion, you could define:

 /* Synchronize SError unless the feature is selected */
 #define SYNCHRONIZE_SERROR(feat) ALTERNATIVE("dsb sy; isb", "nop; nop");
>>>
>>> Or even:
>>>
>>> /*
>>>   * Synchronize SError unless the feature is selected.
>>>   * This is relying on the SErrors are currently unmasked.
>>>   */
>>> #define SYNCHRONIZE_SERROR(feat)\
>>>do {  \
>>>  ASSERT(cpus_have_cap(feat) && local_abort_is_enabled());\
>>>  ALTERNATIVE("dsb sy; isb", "nop; nop");  \
>>>while (0)
>>>
>>> The ASSERT is here to check that we have abort enabled. Otherwise, doing
>>> the synchronization would be pointless.
>>>
>>
>> Think a bit more about it. This macro will import more check than read
>> serror_op. This macro seems as a generic macro, it can be used in any
>> place. So you enable the feature check and abort check. But in this
>> patch, we know where the macro will be used, we know the abort is
>> enabled. And want to reduce the overhead as much as possible.
>
> ASSERTs are used to verify your assumption is correct, for non-debug
> built they will be turned into a NOP.
>
> I care about overhead in non-debug build, it is not much a concern for
> debug build. In the latter it is more important to verify the correctness.
>
> The abort bit is enabled at the entry in the hypervisor, and neither you
> nor I can tell if we need to disable abort in the future in some path.
> This ASSERT will catch those changes.
>
> So please give me a reason to remove it, because I see only benefits so far.

Ok, thanks, I understand know. As it just affects the debug built. I
will keep them in the macro.

>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [Xen-devel] [PATCH v3 8/9] tools/libxc: add support of injecting MC# to specified CPUs

[Wei Liu]

On Thu, Mar 30, 2017 at 02:20:02PM +0800, Haozhong Zhang wrote:
> Though XEN_MC_inject_v2 allows injecting MC# to specified CPUs, the
> current xc_mca_op() does not use this feature and not provide an
> interface to callers. This commit add a new xc_mca_op_inject_v2() that
> receives a cpumap providing the set of target CPUs.
> 
> Signed-off-by: Haozhong Zhang 

Acked-by: Wei Liu 

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

Re: [Xen-devel] [PATCH v4] dm_op: Add xendevicemodel_modified_memory_bulk.

[Wei Liu]

On Wed, Mar 29, 2017 at 05:03:04PM +, Jennifer Herbert wrote:
> From: Jennifer Herbert 
> 
> This new lib devicemodel call allows multiple extents of pages to be
> marked as modified in a single call.  This is something needed for a
> usecase I'm working on.
> 
> The xen side of the modified_memory call has been modified to accept
> an array of extents.  The devicemodle library either provides an array
> of length 1, to support the original library function, or a second
> function allows an array to be provided.
> 
> Signed-off-by: Jennifer Herbert 
> ---
> Changes as discussed on V3.
> 
> Cc: Jan Beulich 
> Cc: Ian Jackson 
> Cc: Wei Liu 
> Cc: Andrew Cooper 
> Cc: Paul Durrant 
> ---
>  tools/libs/devicemodel/core.c   |   30 --
>  tools/libs/devicemodel/include/xendevicemodel.h |   19 +++-

Toolstack side looks reasonable.

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

[Xen-devel] [PATCH 1/2] xen: correct error handling in xen-acpi-processor

[Juergen Gross]

Error handling in upload_pm_data() is rather questionable: possible
errno values from called functions are or'ed together resulting in
strange return values: -EINVAL and -ENOMEM would e.g. result in
returning -EXDEV.

Fix this by bailing out early after the first error.

Cc: sta...@vger.kernel.org
Signed-off-by: Juergen Gross 
---
 drivers/xen/xen-acpi-processor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 23e391d..3659ed3 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -283,8 +283,8 @@ static int upload_pm_data(struct acpi_processor *_pr)
if (_pr->flags.power)
err = push_cxx_to_hypervisor(_pr);
 
-   if (_pr->performance && _pr->performance->states)
-   err |= push_pxx_to_hypervisor(_pr);
+   if (!err && _pr->performance && _pr->performance->states)
+   err = push_pxx_to_hypervisor(_pr);
 
mutex_unlock(&acpi_ids_mutex);
return err;
-- 
2.10.2


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

[Xen-devel] [PATCH 0/2] xen: fix and cleanup of xen-acpi-processor

[Juergen Gross]

Juergen Gross (2):
  xen: correct error handling in xen-acpi-processor
  xen: make functions in xen-acpi-processor return void

 drivers/xen/xen-acpi-processor.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

-- 
2.10.2


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

[Xen-devel] [PATCH 2/2] xen: make functions in xen-acpi-processor return void

[Juergen Gross]

The return value of xen_copy_pct_data() is always 0 and never checked.
xen_copy_psd_data() returns always 0, too, so checking the value is
kind of pointless.

Make the functions return void.

Signed-off-by: Juergen Gross 
---
 drivers/xen/xen-acpi-processor.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index 3659ed3..8f01585 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -161,8 +161,8 @@ xen_copy_pss_data(struct acpi_processor *_pr,
}
return dst_states;
 }
-static int xen_copy_psd_data(struct acpi_processor *_pr,
-struct xen_processor_performance *dst)
+static void xen_copy_psd_data(struct acpi_processor *_pr,
+ struct xen_processor_performance *dst)
 {
struct acpi_psd_package *pdomain;
 
@@ -189,10 +189,9 @@ static int xen_copy_psd_data(struct acpi_processor *_pr,
 
}
memcpy(&(dst->domain_info), pdomain, sizeof(struct acpi_psd_package));
-   return 0;
 }
-static int xen_copy_pct_data(struct acpi_pct_register *pct,
-struct xen_pct_register *dst_pct)
+static void xen_copy_pct_data(struct acpi_pct_register *pct,
+ struct xen_pct_register *dst_pct)
 {
/* It would be nice if you could just do 'memcpy(pct, dst_pct') but
 * sadly the Xen structure did not have the proper padding so the
@@ -205,7 +204,6 @@ static int xen_copy_pct_data(struct acpi_pct_register *pct,
dst_pct->bit_offset = pct->bit_offset;
dst_pct->reserved = pct->reserved;
dst_pct->address = pct->address;
-   return 0;
 }
 static int push_pxx_to_hypervisor(struct acpi_processor *_pr)
 {
@@ -233,8 +231,8 @@ static int push_pxx_to_hypervisor(struct acpi_processor 
*_pr)
set_xen_guest_handle(dst_perf->states, dst_states);
dst_perf->flags |= XEN_PX_PSS;
}
-   if (!xen_copy_psd_data(_pr, dst_perf))
-   dst_perf->flags |= XEN_PX_PSD;
+   xen_copy_psd_data(_pr, dst_perf);
+   dst_perf->flags |= XEN_PX_PSD;
 
if (dst_perf->flags != (XEN_PX_PSD | XEN_PX_PSS | XEN_PX_PCT | 
XEN_PX_PPC)) {
pr_warn("ACPI CPU%u missing some P-state data (%x), skipping\n",
-- 
2.10.2


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

Re: [Xen-devel] [PATCH] xl: Add 'pvh' config option

[Wei Liu]

On Fri, Mar 31, 2017 at 11:23:04AM +0100, Roger Pau Monné wrote:
> On Thu, Mar 30, 2017 at 07:06:15PM -0400, Boris Ostrovsky wrote:
> > In addition to 'device_model_version="none"' config option users can
> > also use 'pvh=1' in xl configuration file when creating a PVH guest.
> 
> I'm not sure, but I think the plan was to remove device_model_version="none"
> and instead just use pvh=1, instead of keeping both. I don't have a strong
> opinion here, so I will leave that to the xl maintainers.
> 

Yes, that's the plan.

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

[Xen-devel] [PATCH v5 3/4] xenstore: rework of transaction handling

[Juergen Gross]

The handling of transactions in xenstored is rather clumsy today:

- Each transaction in progress is keeping a local copy of the complete
  xenstore data base
- A transaction will fail as soon as any node is being modified outside
  the transaction

This is leading to a very bad behavior in case of a large xenstore.
Memory consumption of xenstored is much higher than necessary and with
many domains up transactions failures will be more and more common.

Instead of keeping a complete copy of the data base for each
transaction store the transaction data in the same data base as the
normal xenstore entries prepended with the transaction in the single
nodes either read or modified. At the end of the transaction walk
through all nodes accessed and check for conflicting modifications.
In case no conflicts are found write all modified nodes to the data
base without transaction identifier.

Following tests have been performed:
- create/destroy of various domains, including HVM with ioemu-stubdom
  (xenstored and xenstore-stubdom)
- multiple concurrent runs of xs-test over several minutes
  (xenstored and xenstore-stubdom)
- test for memory leaks of xenstored by dumping talloc reports before
  and after the tests

Signed-off-by: Juergen Gross 
---
 tools/xenstore/xenstored_core.c| 147 +--
 tools/xenstore/xenstored_core.h|  20 +-
 tools/xenstore/xenstored_domain.c  |  24 +-
 tools/xenstore/xenstored_domain.h  |   2 +-
 tools/xenstore/xenstored_transaction.c | 443 +++--
 tools/xenstore/xenstored_transaction.h |  18 +-
 6 files changed, 480 insertions(+), 174 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index ee4c9e1..c8e4237 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -54,8 +54,6 @@
 #include "xenstored_control.h"
 #include "tdb.h"
 
-#include "hashtable.h"
-
 #ifndef NO_SOCKETS
 #if defined(HAVE_SYSTEMD)
 #define XEN_SYSTEMD_ENABLED 1
@@ -81,9 +79,8 @@ static bool recovery = true;
 static int reopen_log_pipe[2];
 static int reopen_log_pipe0_pollfd_idx = -1;
 char *tracefile = NULL;
-static TDB_CONTEXT *tdb_ctx = NULL;
+TDB_CONTEXT *tdb_ctx = NULL;
 
-static void corrupt(struct connection *conn, const char *fmt, ...);
 static const char *sockmsg_string(enum xsd_sockmsg_type type);
 
 #define log(...)   \
@@ -105,24 +102,6 @@ int quota_nb_watch_per_domain = 128;
 int quota_max_entry_size = 2048; /* 2K */
 int quota_max_transaction = 10;
 
-TDB_CONTEXT *tdb_context(struct connection *conn)
-{
-   /* conn = NULL used in manual_node at setup. */
-   if (!conn || !conn->transaction)
-   return tdb_ctx;
-   return tdb_transaction_context(conn->transaction);
-}
-
-bool replace_tdb(const char *newname, TDB_CONTEXT *newtdb)
-{
-   if (!(tdb_ctx->flags & TDB_INTERNAL))
-   if (rename(newname, xs_daemon_tdb()) != 0)
-   return false;
-   tdb_close(tdb_ctx);
-   tdb_ctx = talloc_steal(talloc_autofree_context(), newtdb);
-   return true;
-}
-
 void trace(const char *fmt, ...)
 {
va_list arglist;
@@ -385,35 +364,38 @@ static struct node *read_node(struct connection *conn, 
const void *ctx,
TDB_DATA key, data;
struct xs_tdb_record_hdr *hdr;
struct node *node;
-   TDB_CONTEXT * context = tdb_context(conn);
 
-   key.dptr = (void *)name;
-   key.dsize = strlen(name);
-   data = tdb_fetch(context, key);
+   node = talloc(ctx, struct node);
+   if (!node) {
+   errno = ENOMEM;
+   return NULL;
+   }
+   node->name = talloc_strdup(node, name);
+   if (!node->name) {
+   talloc_free(node);
+   errno = ENOMEM;
+   return NULL;
+   }
+
+   if (transaction_prepend(conn, name, &key))
+   return NULL;
+
+   data = tdb_fetch(tdb_ctx, key);
 
if (data.dptr == NULL) {
-   if (tdb_error(context) == TDB_ERR_NOEXIST)
+   if (tdb_error(tdb_ctx) == TDB_ERR_NOEXIST) {
+   node->generation = NO_GENERATION;
+   access_node(conn, node, NODE_ACCESS_READ, NULL);
errno = ENOENT;
-   else {
-   log("TDB error on read: %s", tdb_errorstr(context));
+   } else {
+   log("TDB error on read: %s", tdb_errorstr(tdb_ctx));
errno = EIO;
}
-   return NULL;
-   }
-
-   node = talloc(ctx, struct node);
-   if (!node) {
-   errno = ENOMEM;
-   return NULL;
-   }
-   node->name = talloc_strdup(node, name);
-   if (!node->name) {
talloc_free(node);
-   errno = ENOMEM;
return NULL;
}
+
node->parent = NULL;
-   node->tdb = tdb_context(conn

[Xen-devel] [ovmf baseline-only test] 71131: all pass

[Platform Team regression test user]

This run is configured for baseline tests only.

flight 71131 ovmf real [real]
http://osstest.xs.citrite.net/~osstest/testlogs/logs/71131/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf de87f23291620d36d69ec55ea53a1c38b8780f0b
baseline version:
 ovmf 251779fca719bf9ea00505ee7629c08b452c150d

Last test of basis71129  2017-03-31 05:17:20 Z0 days
Testing same since71131  2017-03-31 09:18:57 Z0 days1 attempts


People who touched revisions under test:
  Bell Song 
  Song, BinX 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  pass



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.


commit de87f23291620d36d69ec55ea53a1c38b8780f0b
Author: Song, BinX 
Date:   Thu Mar 30 16:23:10 2017 +0800

BaseTools: Update Brotli and BrotliCompress mode and format

V2:
- Update correct patch info

V1:
- Add x mode for Brotli and BrotliCompress
- Change Brotli and BrotliCompress format from DOS to UNIX

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Bell Song 
Reviewed-by: Liming Gao 

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

Re: [Xen-devel] [For Xen-4.10 RFC PATCH 3/3] Prevent redundant icache flushes in populate_physmap()

[Wei Liu]

On Fri, Mar 31, 2017 at 11:24:24AM +0100, Punit Agrawal wrote:
> populate_physmap() calls alloc_heap_pages() per requested extent. As
> alloc_heap_pages() performs icache maintenance operations affecting the
> entire instruction cache, this leads to redundant cache flushes when
> allocating multiple extents in populate_physmap().
> 
> To alleviate this problem, introduce a new flag "MEMF_no_icache_flush"
> which can be used prevent alloc_heap_pages() to perform unnecessary
> icache maintenance operations. Use the flag in populate_physmap() and
> perform the required icache maintenance function at the end of the
> operation.
> 
> Signed-off-by: Punit Agrawal 
> ---
>  xen/common/memory.c| 6 ++
>  xen/common/page_alloc.c| 2 +-
>  xen/include/asm-x86/page.h | 4 
>  xen/include/xen/mm.h   | 2 ++
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index ad0b33ceb6..507f363924 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -162,6 +162,8 @@ static void populate_physmap(struct memop_args *a)
>  if ( unlikely(!d->creation_finished) )
>  a->memflags |= MEMF_no_tlbflush;
>  
> +a->memflags |= MEMF_no_icache_flush;
> +

Not a good idea.

I think what you need is probably to group this under
!d->creation_finished.

>  for ( i = a->nr_done; i < a->nr_extents; i++ )
>  {
>  if ( i != a->nr_done && hypercall_preempt_check() )
> @@ -253,6 +255,10 @@ static void populate_physmap(struct memop_args *a)
>  out:
>  if ( need_tlbflush )
>  filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> +if ( a->memflags & MEMF_no_icache_flush )

This should be inverted, right?

The modification to this function is definitely wrong as-is. You end up
always setting the no flush flag and later always flushing the cache.


Wei.

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

[Xen-devel] [xen-unstable test] 107015: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-armhf-armhf-libvirt-xsm 13 saverestore-support-checkfail  like 106959
 test-armhf-armhf-libvirt 13 saverestore-support-checkfail  like 106959
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 106959
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 106959
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 106959
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 106959
 test-armhf-armhf-libvirt-raw 12 saverestore-support-checkfail  like 106959
 test-amd64-amd64-xl-rtds  9 debian-install   fail  like 106959

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-rtds  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 build-arm64   5 xen-buildfail   never pass
 build-arm64-xsm   5 xen-buildfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 build-arm64-pvops 5 kernel-build fail   never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-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-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass
 test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail  never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail  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-cubietruck 12 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 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-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  3bdb14004b9fe8c35e4961f8a7c73c19f0fb4365
baseline version:
 xen  ac9ff74f39a734756af90ccbb7184551f7b1e22a

Last test of basis   106959  2017-03-28 09:14:17 Z3 days
Failing since106979  2017-03-29 16:19:31 Z1 days4 attempts
Testing same since   107015  2017-03-31 02:21:45 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Boris Ostrovsky 
  George Dunlap 
  Ian Jackson 
  Jan Beulich 
  Jonathan Davies 
  Joshua Otto 
  Juergen Gross 
  Owen Smith 
  Paul Durrant 
  Stefano Stabellini 
  Stefano Stabellini 
  Thomas Sanders 
  Wei Chen 
  Wei Liu 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  fail
 build-armhf-xsm  pass
 build-i386-xsm 

Re: [Xen-devel] [PATCH v5 3/4] xenstore: rework of transaction handling

[Wei Liu]

Another stupid question below.

On Fri, Mar 31, 2017 at 01:29:19PM +0200, Juergen Gross wrote:
>  
> -struct changed_node
> +/*
> + * Some notes regarding detection and handling of transaction conflicts:
> + *
> + * Basic source of reference is the 'generation' count. Each writing access
> + * (either normal write or in a transaction) to the tdb data base will set
> + * the node specific generation count to the global generation count.
> + * For being able to identify a transaction the transaction specific 
> generation
> + * count is initialized with the global generation count when starting the
> + * transaction.
> + * Each time the global generation count is copied to either a node or a
> + * transaction it is incremented. This ensures all nodes and/or transactions
> + * are having a unique generation count.
> + *
> + * Transaction conflicts are detected by checking the generation count of all
> + * nodes read in the transaction to match with the generation count in the
> + * global data base at the end of the transaction. Nodes which have been
> + * modified in the transaction don't have to be checked to match even if they
> + * have been read, as the modified node will be globally visible after the
> + * succeeded transaction possibly overwriting another modification which may
> + * have occurred concurrent to the transaction.
> + *
> + * Examples:
> + * -
> + * The following notation is used:
> + * I:  initial state
> + * G   global generation count
> + * g(X)generation count of node X
> + * G(1)generation count of transaction 1
> + * g(1:Y)  saved generation count of node Y in transaction 1

Assuming this is recorded in accessed_node, can you point me to where
about in the code this is implemented? I looked at access_node but that
only records generation for read.

> + * TA1:operation in transaction 1
> + * X=1:X   replace global node X with transaction 1 specific value of X
> + *
> + * 1. Simple transaction doing: read node A, write node B
> + *I: g(A) = 1, g(B) = 2, G = 3
> + *Start transaction 1: G(1) = 3, G = 4
> + *TA1: read node A:g(1:A) = 1
> + *TA1: write node B:   g(1:B) = 4, G = 5

Specifically, here you mention write type also records generation count.

Wei.

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

Re: [Xen-devel] [PATCH v5 3/4] xenstore: rework of transaction handling

[Juergen Gross]

On 31/03/17 14:03, Wei Liu wrote:
> Another stupid question below.
> 
> On Fri, Mar 31, 2017 at 01:29:19PM +0200, Juergen Gross wrote:
>>  
>> -struct changed_node
>> +/*
>> + * Some notes regarding detection and handling of transaction conflicts:
>> + *
>> + * Basic source of reference is the 'generation' count. Each writing access
>> + * (either normal write or in a transaction) to the tdb data base will set
>> + * the node specific generation count to the global generation count.
>> + * For being able to identify a transaction the transaction specific 
>> generation
>> + * count is initialized with the global generation count when starting the
>> + * transaction.
>> + * Each time the global generation count is copied to either a node or a
>> + * transaction it is incremented. This ensures all nodes and/or transactions
>> + * are having a unique generation count.
>> + *
>> + * Transaction conflicts are detected by checking the generation count of 
>> all
>> + * nodes read in the transaction to match with the generation count in the
>> + * global data base at the end of the transaction. Nodes which have been
>> + * modified in the transaction don't have to be checked to match even if 
>> they
>> + * have been read, as the modified node will be globally visible after the
>> + * succeeded transaction possibly overwriting another modification which may
>> + * have occurred concurrent to the transaction.
>> + *
>> + * Examples:
>> + * -
>> + * The following notation is used:
>> + * I:  initial state
>> + * G   global generation count
>> + * g(X)generation count of node X
>> + * G(1)generation count of transaction 1
>> + * g(1:Y)  saved generation count of node Y in transaction 1
> 
> Assuming this is recorded in accessed_node, can you point me to where
> about in the code this is implemented? I looked at access_node but that
> only records generation for read.

It is in the node itself stored in the data base.

>> + * TA1:operation in transaction 1
>> + * X=1:X   replace global node X with transaction 1 specific value of X
>> + *
>> + * 1. Simple transaction doing: read node A, write node B
>> + *I: g(A) = 1, g(B) = 2, G = 3
>> + *Start transaction 1: G(1) = 3, G = 4
>> + *TA1: read node A:g(1:A) = 1
>> + *TA1: write node B:   g(1:B) = 4, G = 5
> 
> Specifically, here you mention write type also records generation count.

In the end the generation count of a node modified by a transaction
isn't important for the transaction.


Juergen


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

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Yi Sun]

On 17-03-31 04:19:49, Jan Beulich wrote:
> >>> On 31.03.17 at 11:12,  wrote:
> > On 17-03-31 02:47:25, Jan Beulich wrote:
> >> >>> On 30.03.17 at 14:10,  wrote:
> >> > On 17-03-30 05:55:52, Jan Beulich wrote:
> >> >> >>> On 30.03.17 at 03:37,  wrote:
> >> >> > On 17-03-29 03:57:52, Jan Beulich wrote:
> >> >> >> >>> On 29.03.17 at 03:36,  wrote:
> >> >> >> > On 17-03-29 09:20:21, Yi Sun wrote:
> >> >> >> >> On 17-03-28 06:20:48, Jan Beulich wrote:
> >> >> >> >> > >>> On 28.03.17 at 13:59,  wrote:
> >> >> >> >> > > I think we at least need a 'get_val()' hook.
> >> >> >> >> > 
> >> >> >> >> > Of course.
> >> >> >> >> > 
> >> >> >> >> > > I try to implement CAT/CDP hook.
> >> >> >> >> > > Please help to check if this is what you thought.
> >> >> >> >> > 
> >> >> >> >> > One remark below, but other than that - yes.
> >> >> >> >> > 
> >> >> >> >> > > static void cat_get_val(const struct feat_node *feat, 
> >> >> >> >> > > unsigned int cos,
> >> >> >> >> > > enum cbm_type type, int flag, 
> >> >> >> >> > > uint32_t *val)
> >> >> >> >> > > {
> >> >> >> >> > > *val = feat->cos_reg_val[cos];
> >> >> >> >> > > }
> >> >> >> >> > > 
> >> >> >> >> > > static void l3_cdp_get_val(const struct feat_node *feat, 
> >> >> >> >> > > unsigned int 
> >> >> > cos,
> >> >> >> >> > >enum cbm_type type, int flag, 
> >> >> >> >> > > uint32_t *val)
> >> >> >> >> > > {
> >> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0 )
> >> >> >> >> > > *val = get_cdp_data(feat, cos);
> >> >> >> >> > > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 1 )
> >> >> >> >> > > *val = get_cdp_code(feat, cos);
> >> >> >> >> > > }
> >> >> >> >> > 
> >> >> >> >> > Why the redundancy between type and flag?
> >> >> >> >> > 
> >> >> >> >> For psr_get_val, upper layer input the cbm_type to get either 
> >> >> >> >> DATA or 
> > CODE
> >> >> >> >> value. For other cases, we use flag as cos_num index to get 
> >> >> >> >> either DATA 
> > or
> >> >> >> >> CODE.
> >> >> >> >> 
> >> >> >> > Let me explain more to avoid confusion. For other cases, we use 
> >> >> >> > cos_num 
> > as
> >> >> >> > index to get values from a feature. In these cases, we do not know 
> >> >> >> > the
> >> >> >> > cbm_type of the feature. So, I use the cos_num as flag to make 
> >> >> >> > 'get_val'
> >> >> >> > know which value should be returned.
> >> >> >> 
> >> >> >> I'm pretty sure this redundancy can be avoided.
> >> >> >> 
> >> >> > Then, I think I have to reuse the 'type'. As only CDP needs type to 
> >> >> > decide
> >> >> > which value to be returned so far, I think I can implement codes like 
> > below
> >> >> > to make CDP can handle all scenarios.
> >> >> > 
> >> >> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int 
> >> >> > cos,
> >> >> >enum cbm_type type, uint32_t *val)
> >> >> > {
> >> >> > if ( type == PSR_CBM_TYPE_L3_DATA || flag == 0xF000 )
> >> >> > *val = get_cdp_data(feat, cos);
> >> >> > if ( type == PSR_CBM_TYPE_L3_CODE || flag == 0xF001 )
> >> >> > *val = get_cdp_code(feat, cos);
> >> >> > }
> >> >> > 
> >> >> > static bool fits_cos_max(...)
> >> >> > {
> >> >> > ..
> >> >> > for (i = 0; i < feat->props->cos_num; i++)
> >> >> > {
> >> >> > feat->props->get_val(feat, cos, i + 0xF000, &default_val);
> >> >> > if ( val[i] == default_val )
> >> >> > ..
> >> >> > }
> >> >> > ..
> >> >> > }
> >> >> > 
> >> >> > Is that good for you?
> >> >> 
> >> >> Urgh - no, not really: This is hackery. Do you have a tree
> >> >> somewhere so I could look at the file after at least the CDP
> >> >> patches have all been applied, to see whether I have a
> >> >> better idea? Alternatively, could you attach psr.c the way
> >> >> you have it right now with the entire series applied?
> >> >> 
> >> > I think you can check v9 codes here:
> >> > https://github.com/yisun-git/xen/tree/l2_cat_v9 
> >> 
> >> Looking at this made me notice that cat_get_old_val() passes a
> >> bogus literal 0 to cat_get_val(), which needs taking care of too.
> >> One option I can see is for each feature to make available an
> >> array of type enum cbm_type, with cos_num elements. The order
> >> would match that of the order of values in their arrays. This will
> > 
> > Sorry, not very clear your meaning. How to do that? Could you please
> > provide pieces of codes? Thanks!
> 
> I'm sorry, but I'm afraid I don't see how I would reasonably supply
> code here without taking over your series altogether (which I don't
> intend to do). What is unclear with, at the example of CDP, you
> needing to add an array at initialization time, slot 0 of which holds
> PSR_CBM_TYPE_L3_DATA and slot 1 PSR_CBM_TYPE_L3_CODE (or
> the other way around). Granted I was wrong with the type of the
> array (as the above aren't enum psr_feat_type enumerators, but
> enum cbm_type ones), but I think the basic idea should have been
> 

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Jan Beulich]

>>> On 31.03.17 at 14:40,  wrote:
> On 17-03-31 04:19:49, Jan Beulich wrote:
>> >>> On 31.03.17 at 11:12,  wrote:
>> > On 17-03-31 02:47:25, Jan Beulich wrote:
>> >> >>> On 30.03.17 at 14:10,  wrote:
>> >> > I think you can check v9 codes here:
>> >> > https://github.com/yisun-git/xen/tree/l2_cat_v9 
>> >> 
>> >> Looking at this made me notice that cat_get_old_val() passes a
>> >> bogus literal 0 to cat_get_val(), which needs taking care of too.
>> >> One option I can see is for each feature to make available an
>> >> array of type enum cbm_type, with cos_num elements. The order
>> >> would match that of the order of values in their arrays. This will
>> > 
>> > Sorry, not very clear your meaning. How to do that? Could you please
>> > provide pieces of codes? Thanks!
>> 
>> I'm sorry, but I'm afraid I don't see how I would reasonably supply
>> code here without taking over your series altogether (which I don't
>> intend to do). What is unclear with, at the example of CDP, you
>> needing to add an array at initialization time, slot 0 of which holds
>> PSR_CBM_TYPE_L3_DATA and slot 1 PSR_CBM_TYPE_L3_CODE (or
>> the other way around). Granted I was wrong with the type of the
>> array (as the above aren't enum psr_feat_type enumerators, but
>> enum cbm_type ones), but I think the basic idea should have been
>> clear anyway: You need to provide a way for generic code to pass
>> suitable type information into ->get_val().
>> 
> May I change the 'get_val()' parameter 'enum cbm_type' to a generic type
> 'unsigned int' to make it be a flexible type,  and then combine feature
> type with cos_num together as a flag to indicate which feature it is,
> which value to get and distinguish it with cbm_type? For example:
> 
> #define CDP_GATHER_BOTH_DATA ( PSR_SOCKET_L3_CDP << 16 )
> #define CDP_GATHER_BOTH_CODE ( PSR_SOCKET_L3_CDP << 16 + 1 )
> static void l3_cdp_get_val(const struct feat_node *feat, unsigned int cos,
>unsigned int type, uint32_t *val)
> {
> switch ( type )
> {
> case PSR_CBM_TYPE_L3_DATA:
> case CDP_GATHER_BOTH_DATA:
> *val = get_cdp_data(feat, cos);
> break;
> case PSR_CBM_TYPE_L3_CODE:
> case CDP_GATHER_BOTH_CODE:
> *val = get_cdp_code(feat, cos);
> break;
> }
> }

The two case labels are still indicative of unnecessary redundancy
(and, even right now only highly theoretical, risk of collisions). What's
wrong with the model I've proposed?

Jan


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

Re: [Xen-devel] [PATCH v5 3/4] xenstore: rework of transaction handling

[Wei Liu]

On Fri, Mar 31, 2017 at 02:11:46PM +0200, Juergen Gross wrote:
> On 31/03/17 14:03, Wei Liu wrote:
> > Another stupid question below.
> > 
> > On Fri, Mar 31, 2017 at 01:29:19PM +0200, Juergen Gross wrote:
> >>  
> >> -struct changed_node
> >> +/*
> >> + * Some notes regarding detection and handling of transaction conflicts:
> >> + *
> >> + * Basic source of reference is the 'generation' count. Each writing 
> >> access
> >> + * (either normal write or in a transaction) to the tdb data base will set
> >> + * the node specific generation count to the global generation count.
> >> + * For being able to identify a transaction the transaction specific 
> >> generation
> >> + * count is initialized with the global generation count when starting the
> >> + * transaction.
> >> + * Each time the global generation count is copied to either a node or a
> >> + * transaction it is incremented. This ensures all nodes and/or 
> >> transactions
> >> + * are having a unique generation count.
> >> + *
> >> + * Transaction conflicts are detected by checking the generation count of 
> >> all
> >> + * nodes read in the transaction to match with the generation count in the
> >> + * global data base at the end of the transaction. Nodes which have been
> >> + * modified in the transaction don't have to be checked to match even if 
> >> they
> >> + * have been read, as the modified node will be globally visible after the
> >> + * succeeded transaction possibly overwriting another modification which 
> >> may
> >> + * have occurred concurrent to the transaction.
> >> + *
> >> + * Examples:
> >> + * -
> >> + * The following notation is used:
> >> + * I:  initial state
> >> + * G   global generation count
> >> + * g(X)generation count of node X
> >> + * G(1)generation count of transaction 1
> >> + * g(1:Y)  saved generation count of node Y in transaction 1
> > 
> > Assuming this is recorded in accessed_node, can you point me to where
> > about in the code this is implemented? I looked at access_node but that
> > only records generation for read.
> 
> It is in the node itself stored in the data base.
> 

Right, then ...

> >> + * TA1:operation in transaction 1
> >> + * X=1:X   replace global node X with transaction 1 specific value of X
> >> + *
> >> + * 1. Simple transaction doing: read node A, write node B
> >> + *I: g(A) = 1, g(B) = 2, G = 3
> >> + *Start transaction 1: G(1) = 3, G = 4
> >> + *TA1: read node A:g(1:A) = 1
> >> + *TA1: write node B:   g(1:B) = 4, G = 5
> > 

I suggest updating places like g(1:B) = 4 to g(B) = 4 to match the code.

I will continue my review.

Wei.

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

[Xen-devel] [PATCH v3 00/19] Provide a command line option to choose how to handle SErrors

[Wei Chen]

From XSA-201, we know that, a guest could trigger SErrors when accessing
memory mapped HW in a non-conventional way. In the patches for XSA-201,
we crash the guest when we captured such asynchronous aborts to avoid data
corruption.

In order to distinguish guest-generated SErrors from hypervisor-generated
SErrors. We have to place SError checking code in every EL1 -> EL2 paths.
That will be an overhead on entries caused by dsb/isb.

But not all platforms want to categorize the SErrors. For example, a host
that is running with trusted guests. The administrator can confirm that
all guests that are running on the host will not trigger such SErrors. In
this user scene, we should provide some options to administrator to avoid
categorizing the SErrors. And then reduce the overhead of dsb/isb.

We provided following 3 options to administrator to determine how to handle
the SErrors:

* `diverse`:
  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
  The guest generated SErrors will be forwarded to guests, the hypervisor
  generated SErrors will cause the whole system crash.
  It requires:
  1. Place dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
 correctly.
  2. Place dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
 SErrors to guests.
  3. Place dsb/isb in context switch to isolate the SErrors between 2 vCPUs.

* `forward`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will be forwarded to guests, except the SErrors generated when
  idle vCPU is running. The idle domain doesn't have the ability to hanle the
  SErrors, so we have to crash the whole system when we get SErros with idle
  vCPU. This option will avoid most overhead of the dsb/isb, except the dsb/isb
  in context switch which is used to isolate the SErrors between 2 vCPUs.

* `panic`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will crash the whole system. This option will avoid all overhead
  of the dsb/isb.

---
v2->v3 changes has been placed in separated patchs.

Wei Chen (19):
  xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome
check
  xen/arm: Introduce a helper to get default HCR_EL2 flags
  xen/arm: Set and restore HCR_EL2 register for each vCPU separately
  xen/arm: Avoid setting/clearing HCR_RW at every context switch
  xen/arm: Save HCR_EL2 when a guest took the SError
  xen/arm: Introduce a virtual abort injection helper
  xen/arm: Introduce a command line parameter for SErrors/Aborts
  xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op
  xen/arm64: Use alternative to skip the check of pending serrors
  xen/arm32: Use alternative to skip the check of pending serrors
  xen/arm: Move macro VABORT_GEN_BY_GUEST to common header
  xen/arm: Introduce new helpers to handle guest/hyp SErrors
  xen/arm: Replace do_trap_guest_serror with new helpers
  xen/arm: Unmask the Abort/SError bit in the exception entries
  xen/arm: Introduce a helper to check local abort is enabled
  xen/arm: Introduce a macro to synchronize SError
  xen/arm: Isolate the SError between the context switch of 2 vCPUs
  xen/arm: Prevent slipping hypervisor SError to guest
  xen/arm: Handle guest external abort as guest SError

 docs/misc/xen-command-line.markdown   |  44 
 xen/arch/arm/arm32/asm-offsets.c  |   1 +
 xen/arch/arm/arm32/entry.S|  28 -
 xen/arch/arm/arm32/traps.c|   5 +-
 xen/arch/arm/arm64/asm-offsets.c  |   1 +
 xen/arch/arm/arm64/domctl.c   |   6 ++
 xen/arch/arm/arm64/entry.S| 105 +--
 xen/arch/arm/arm64/traps.c|   2 +-
 xen/arch/arm/domain.c |  19 
 xen/arch/arm/domain_build.c   |   7 ++
 xen/arch/arm/p2m.c|  10 +-
 xen/arch/arm/traps.c  | 187 +-
 xen/include/asm-arm/arm32/processor.h |  12 +--
 xen/include/asm-arm/arm32/system.h|   7 ++
 xen/include/asm-arm/arm64/processor.h |   3 +-
 xen/include/asm-arm/arm64/system.h|   7 ++
 xen/include/asm-arm/cpufeature.h  |   4 +-
 xen/include/asm-arm/domain.h  |   4 +
 xen/include/asm-arm/processor.h   |  28 -
 19 files changed, 369 insertions(+), 111 deletions(-)

-- 
2.7.4


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

[Xen-devel] [PATCH v3 05/19] xen/arm: Save HCR_EL2 when a guest took the SError

[Wei Chen]

The HCR_EL2.VSE (HCR.VA for aarch32) bit can be used to generate a
virtual abort to guest. The HCR_EL2.VSE bit has a peculiar feature
of getting cleared when the guest has taken the abort (this is the
only bit that behaves as such in HCR_EL2 register).

This means that if we set the HCR_EL2.VSE bit to signal such an abort,
we must preserve it in the guest context until it disappears from
HCR_EL2, and at which point it must be cleared from the context. This
is achieved by reading back from HCR_EL2 until the guest takes the
fault.

If we preserved a pending VSE in guest context, we have to restore
it to HCR_EL2 when context switch to this guest. This is achieved
by writing saved HCR_EL2 value in guest context back to HCR_EL2
register before return to guest. This had been done by the patch
of "Restore HCR_EL2 register".

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/traps.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e6d88f2..5f758e1 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2641,7 +2641,18 @@ static void do_trap_smc(struct cpu_user_regs *regs, 
const union hsr hsr)
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
 if ( guest_mode(regs) )
+{
+/*
+ * If we pended a virtual abort, preserve it until it gets cleared.
+ * See ARM ARM DDI 0487A.j D1.14.3 (Virtual Interrupts) for details,
+ * but the crucial bit is "On taking a vSError interrupt, HCR_EL2.VSE
+ * (alias of HCR.VA) is cleared to 0."
+ */
+if ( current->arch.hcr_el2 & HCR_VA )
+current->arch.hcr_el2 = READ_SYSREG(HCR_EL2);
+
 gic_clear_lrs(current);
+}
 }
 
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
-- 
2.7.4


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

[Xen-devel] [PATCH v3 04/19] xen/arm: Avoid setting/clearing HCR_RW at every context switch

[Wei Chen]

The HCR_EL2 flags for 64-bit and 32-bit domains are different. But
when we initialized the HCR_EL2 for vcpu0 of Dom0 and all vcpus of
DomU in vcpu_initialise, we didn't know the domain's address size
information. We had to use compatible flags to initialize HCR_EL2,
and set HCR_RW for 64-bit domain or clear HCR_RW for 32-bit domain
at every context switch.

But, after we added the HCR_EL2 to vcpu's context, this behaviour
seems a little fussy. We can update the HCR_RW bit in vcpu's context
as soon as we get the domain's address size to avoid setting/clearing
HCR_RW at every context switch.

Signed-off-by: Wei Chen 
Acked-by: Julien Grall 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/arm64/domctl.c  | 6 ++
 xen/arch/arm/domain.c| 5 +
 xen/arch/arm/domain_build.c  | 7 +++
 xen/arch/arm/p2m.c   | 5 -
 xen/include/asm-arm/domain.h | 1 +
 5 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/domctl.c b/xen/arch/arm/arm64/domctl.c
index 44e1e7b..ab8781f 100644
--- a/xen/arch/arm/arm64/domctl.c
+++ b/xen/arch/arm/arm64/domctl.c
@@ -14,6 +14,8 @@
 
 static long switch_mode(struct domain *d, enum domain_type type)
 {
+struct vcpu *v;
+
 if ( d == NULL )
 return -EINVAL;
 if ( d->tot_pages != 0 )
@@ -23,6 +25,10 @@ static long switch_mode(struct domain *d, enum domain_type 
type)
 
 d->arch.type = type;
 
+if ( is_64bit_domain(d) )
+for_each_vcpu(d, v)
+vcpu_switch_to_aarch64_mode(v);
+
 return 0;
 }
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 5d18bb0..69c2854 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -537,6 +537,11 @@ void vcpu_destroy(struct vcpu *v)
 free_xenheap_pages(v->arch.stack, STACK_ORDER);
 }
 
+void vcpu_switch_to_aarch64_mode(struct vcpu *v)
+{
+v->arch.hcr_el2 |= HCR_RW;
+}
+
 int arch_domain_create(struct domain *d, unsigned int domcr_flags,
struct xen_arch_domainconfig *config)
 {
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index de59e5f..3abacc0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2148,6 +2148,10 @@ int construct_dom0(struct domain *d)
 return -EINVAL;
 }
 d->arch.type = kinfo.type;
+
+if ( is_64bit_domain(d) )
+vcpu_switch_to_aarch64_mode(v);
+
 #endif
 
 allocate_memory(d, &kinfo);
@@ -2240,6 +2244,9 @@ int construct_dom0(struct domain *d)
 printk("Failed to allocate dom0 vcpu %d on pcpu %d\n", i, cpu);
 break;
 }
+
+if ( is_64bit_domain(d) )
+vcpu_switch_to_aarch64_mode(d->vcpu[i]);
 }
 
 return 0;
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 83c4b7d..34d5776 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -137,11 +137,6 @@ void p2m_restore_state(struct vcpu *n)
 WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
 isb();
 
-if ( is_32bit_domain(n->domain) )
-n->arch.hcr_el2 &= ~HCR_RW;
-else
-n->arch.hcr_el2 |= HCR_RW;
-
 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 7b1dacc..68185e2 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -268,6 +268,7 @@ struct arch_vcpu
 
 void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
+void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
 unsigned int domain_max_vcpus(const struct domain *);
 
-- 
2.7.4


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

[Xen-devel] [PATCH v3 01/19] xen/arm: Save ESR_EL2 to avoid using mismatched value in syndrome check

[Wei Chen]

Xen will do exception syndrome check while some types of exception
take place in EL2. The syndrome check code read the ESR_EL2 register
directly, but in some situation this register maybe overridden by
nested exception.

For example, if we re-enable IRQ before reading ESR_EL2 which means
Xen may enter in IRQ exception mode and return the processor with
clobbered ESR_EL2 (See ARM ARM DDI 0487A.j D7.2.25)

In this case the guest exception syndrome has been overridden, we will
check the syndrome for guest sync exception with an incorrect ESR_EL2
value. So we want to save ESR_EL2 to cpu_user_regs as soon as the
exception takes place in EL2 to avoid using an incorrect syndrome value.

In order to save ESR_EL2, we added a 32-bit member hsr to cpu_user_regs.
But while saving registers in trap entry, we use stp to save ELR and
CPSR at the same time through 64-bit general registers. If we keep this
code, the hsr will be overridden by upper 32-bit of CPSR. So adjust the
code to use str to save ELR in a separate instruction and use stp to
save CPSR and HSR at the same time through 32-bit general registers.
This change affects the registers restore in trap exit, we can't use the
ldp to restore ELR and CPSR from stack at the same time. We have to use
ldr to restore them separately.

Signed-off-by: Wei Chen 

---
Note:
This patch is a bug fix, this bug affects the 4.8 and 4.7 source trees
too.

v2->v3:
1. Add note to the commit message.
2. Read ESR_EL2 value from vCPU context instead of reading from register
   directly in the places where use the ESR_EL2 value.
---
 xen/arch/arm/arm32/asm-offsets.c  |  1 +
 xen/arch/arm/arm32/entry.S|  3 +++
 xen/arch/arm/arm64/asm-offsets.c  |  1 +
 xen/arch/arm/arm64/entry.S| 13 +
 xen/arch/arm/arm64/traps.c|  2 +-
 xen/arch/arm/traps.c  |  4 ++--
 xen/include/asm-arm/arm32/processor.h |  2 +-
 xen/include/asm-arm/arm64/processor.h |  3 +--
 8 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm32/asm-offsets.c b/xen/arch/arm/arm32/asm-offsets.c
index f8e6b53..5b543ab 100644
--- a/xen/arch/arm/arm32/asm-offsets.c
+++ b/xen/arch/arm/arm32/asm-offsets.c
@@ -26,6 +26,7 @@ void __dummy__(void)
OFFSET(UREGS_lr, struct cpu_user_regs, lr);
OFFSET(UREGS_pc, struct cpu_user_regs, pc);
OFFSET(UREGS_cpsr, struct cpu_user_regs, cpsr);
+   OFFSET(UREGS_hsr, struct cpu_user_regs, hsr);
 
OFFSET(UREGS_LR_usr, struct cpu_user_regs, lr_usr);
OFFSET(UREGS_SP_usr, struct cpu_user_regs, sp_usr);
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 2a6f4f0..2187226 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -23,6 +23,9 @@
 add r11, sp, #UREGS_kernel_sizeof+4;\
 str r11, [sp, #UREGS_sp];   \
 \
+mrc CP32(r11, HSR); /* Save exception syndrome */   \
+str r11, [sp, #UREGS_hsr];  \
+\
 mrs r11, SPSR_hyp;  \
 str r11, [sp, #UREGS_cpsr]; \
 and r11, #PSR_MODE_MASK;\
diff --git a/xen/arch/arm/arm64/asm-offsets.c b/xen/arch/arm/arm64/asm-offsets.c
index 69ea92a..ce24e44 100644
--- a/xen/arch/arm/arm64/asm-offsets.c
+++ b/xen/arch/arm/arm64/asm-offsets.c
@@ -27,6 +27,7 @@ void __dummy__(void)
OFFSET(UREGS_SP, struct cpu_user_regs, sp);
OFFSET(UREGS_PC, struct cpu_user_regs, pc);
OFFSET(UREGS_CPSR, struct cpu_user_regs, cpsr);
+   OFFSET(UREGS_ESR_el2, struct cpu_user_regs, hsr);
 
OFFSET(UREGS_SPSR_el1, struct cpu_user_regs, spsr_el1);
 
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index c181b5e..02802c0 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -121,9 +121,13 @@ lr  .reqx30 // link register
 
 stp lr, x21, [sp, #UREGS_LR]
 
-mrs x22, elr_el2
-mrs x23, spsr_el2
-stp x22, x23, [sp, #UREGS_PC]
+mrs x21, elr_el2
+str x21, [sp, #UREGS_PC]
+
+add x21, sp, #UREGS_CPSR
+mrs x22, spsr_el2
+mrs x23, esr_el2
+stp w22, w23, [x21]
 
 .endm
 
@@ -307,7 +311,8 @@ ENTRY(return_to_new_vcpu64)
 return_from_trap:
 msr daifset, #2 /* Mask interrupts */
 
-ldp x21, x22, [sp, #UREGS_PC]   // load ELR, SPSR
+ldr x21, [sp, #UREGS_PC]// load ELR
+ldr w22, [sp, #UREGS_CPSR]  // load SPSR
 
 pop x0, x1
 pop x2, x3
diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
index 8e89376..36b3a30 100644
--- a/xen/arch/arm/ar

[Xen-devel] [PATCH v3 02/19] xen/arm: Introduce a helper to get default HCR_EL2 flags

[Wei Chen]

We want to add HCR_EL2 register to Xen context switch. And each copy
of HCR_EL2 in vcpu structure will be initialized with the same set
of trap flags as the HCR_EL2 register. We introduce a helper here to
represent these flags to be reused easily.

Signed-off-by: Wei Chen 
---
 xen/arch/arm/traps.c| 11 ---
 xen/include/asm-arm/processor.h |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6137272..e6d88f2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -115,6 +115,13 @@ static void __init parse_vwfi(const char *s)
 }
 custom_param("vwfi", parse_vwfi);
 
+register_t get_default_hcr_flags(void)
+{
+return  (HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
+ (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
+ HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
+}
+
 void init_traps(void)
 {
 /* Setup Hyp vector base */
@@ -139,9 +146,7 @@ void init_traps(void)
  CPTR_EL2);
 
 /* Setup hypervisor traps */
-WRITE_SYSREG(HCR_PTW|HCR_BSU_INNER|HCR_AMO|HCR_IMO|HCR_FMO|HCR_VM|
- (vwfi != NATIVE ? (HCR_TWI|HCR_TWE) : 0) |
- HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB,HCR_EL2);
+WRITE_SYSREG(get_default_hcr_flags(), HCR_EL2);
 isb();
 }
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index afc0e9a..4b6338b 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -708,6 +708,8 @@ int call_smc(register_t function_id, register_t arg0, 
register_t arg1,
 
 void do_trap_guest_error(struct cpu_user_regs *regs);
 
+register_t get_default_hcr_flags(void);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.7.4


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

[Xen-devel] [PATCH v3 03/19] xen/arm: Set and restore HCR_EL2 register for each vCPU separately

[Wei Chen]

Different domains may have different HCR_EL2 flags. For example, the
64-bit domain needs HCR_RW flag but the 32-bit does not need it. So
we give each domain a default HCR_EL2 value and save it in the vCPU's
context.

HCR_EL2 register has only one bit can be updated automatically without
explicit write (HCR_VSE). But we haven't used this bit currently, so
we can consider that the HCR_EL2 register will not be modified while
the guest is running. So save the HCR_EL2 while guest exiting to
hypervisor is not neccessary. We just have to restore this register for
each vCPU while context switching.

The p2m_restore_state which will be invoked in context switch progress
has included the writing of HCR_EL2 already. It updates the HCR_EL2.RW
bit to tell the hardware how to interpret the stage-1 page table as the
encodings are different between AArch64 and AArch32. We can reuse this
write to restore the HCR_EL2 for each vCPU. Of course, the value of each
vCPU's HCR_EL2 should be adjusted to have proper HCR_EL2.RW bit in this
function. In the later patch of this series, we will set the HCR_EL2.RW
for each vCPU while the domain is creating.

Signed-off-by: wei chen 
---
v2->v3:
1. Squash #2 #3 patchs of v2 into this patch.
2. Remove HCR_EL2 initialization of idle vCPU, it would be used.
3. Revert the change of vwfi.
4. Use the helper that introduced in previous patch to initialize
   HCR_EL2 for each vCPU.
5. Restore the initialization of HCR_EL2 in init_traps.
---
 xen/arch/arm/domain.c| 2 ++
 xen/arch/arm/p2m.c   | 9 +++--
 xen/include/asm-arm/domain.h | 3 +++
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index bb327da..5d18bb0 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -513,6 +513,8 @@ int vcpu_initialise(struct vcpu *v)
 
 v->arch.actlr = READ_SYSREG32(ACTLR_EL1);
 
+v->arch.hcr_el2 = get_default_hcr_flags();
+
 processor_vcpu_initialise(v);
 
 if ( (rc = vcpu_vgic_init(v)) != 0 )
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 6263760..83c4b7d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -128,27 +128,24 @@ void p2m_save_state(struct vcpu *p)
 
 void p2m_restore_state(struct vcpu *n)
 {
-register_t hcr;
 struct p2m_domain *p2m = &n->domain->arch.p2m;
 uint8_t *last_vcpu_ran;
 
 if ( is_idle_vcpu(n) )
 return;
 
-hcr = READ_SYSREG(HCR_EL2);
-
 WRITE_SYSREG64(p2m->vttbr, VTTBR_EL2);
 isb();
 
 if ( is_32bit_domain(n->domain) )
-hcr &= ~HCR_RW;
+n->arch.hcr_el2 &= ~HCR_RW;
 else
-hcr |= HCR_RW;
+n->arch.hcr_el2 |= HCR_RW;
 
 WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1);
 isb();
 
-WRITE_SYSREG(hcr, HCR_EL2);
+WRITE_SYSREG(n->arch.hcr_el2, HCR_EL2);
 isb();
 
 last_vcpu_ran = &p2m->last_vcpu_ran[smp_processor_id()];
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 09fe502..7b1dacc 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -204,6 +204,9 @@ struct arch_vcpu
 register_t tpidr_el1;
 register_t tpidrro_el0;
 
+/* HYP configuration */
+register_t hcr_el2;
+
 uint32_t teecr, teehbr; /* ThumbEE, 32-bit guests only */
 #ifdef CONFIG_ARM_32
 /*
-- 
2.7.4


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

[Xen-devel] [PATCH v3 08/19] xen/arm: Introduce a initcall to update cpu_hwcaps by serror_op

[Wei Chen]

In the later patches of this series, we want to use the alternative
patching framework to avoid checking serror_op in every entries.
So we define a new cpu feature "SKIP_CHECK_PENDING_VSERROR" for
serror_op. When serror_op is not equal to SERROR_DIVERSE, this
feature will be set to cpu_hwcaps.

Currently, the default serror_op is SERROR_DIVERSE, if we want to
change the serror_op value we have to place the serror parameter
in command line. It seems no problem to update cpu_hwcaps directly
in the serror parameter parsing function.

While the default option will be diverse today, this may change in the
future. So we introduce this initcall to guarantee the cpu_hwcaps can be
updated no matter the serror parameter is placed in the command line
or not.

Signed-off-by: Wei Chen 
Acked-by: Julien Grall 
Reviewed-by: Stefano Stabellini 
---
v2->v3
1. Rewrite the commit message to make it easer to understand.
2. Add Julien's Acked-by tag and Stefano's Reviewed-by tag.
---
 xen/arch/arm/traps.c | 9 +
 xen/include/asm-arm/cpufeature.h | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 76cda59..9d4ee39 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -141,6 +141,15 @@ static void __init parse_serrors_behavior(const char *str)
 }
 custom_param("serrors", parse_serrors_behavior);
 
+static int __init update_serrors_cpu_caps(void)
+{
+if ( serrors_op != SERRORS_DIVERSE )
+cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
+
+return 0;
+}
+__initcall(update_serrors_cpu_caps);
+
 void init_traps(void)
 {
 /* Setup Hyp vector base */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index c0a25ae..ec3f9e5 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -40,8 +40,9 @@
 #define ARM32_WORKAROUND_766422 2
 #define ARM64_WORKAROUND_834220 3
 #define LIVEPATCH_FEATURE   4
+#define SKIP_CHECK_PENDING_VSERROR 5
 
-#define ARM_NCAPS   5
+#define ARM_NCAPS   6
 
 #ifndef __ASSEMBLY__
 
-- 
2.7.4


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

[Xen-devel] [PATCH v3 09/19] xen/arm64: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to skip the check of pending SError,
in conventional way, we have to read the option every time before
we try to check the pending SError. This will add overhead to check
the option at every trap.

The ARM64 supports the alternative patching feature. We can use an
ALTERNATIVE to avoid checking option at every trap. We added a new
cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
enabled when the option is not diverse.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/arm64/entry.S | 41 +
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 02802c0..4baa3cb 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -1,5 +1,6 @@
 #include 
 #include 
+#include 
 #include 
 
 /*
@@ -229,12 +230,14 @@ hyp_irq:
 
 guest_sync:
 entry   hyp=0, compat=0
-bl  check_pending_vserror
 /*
- * If x0 is Non-zero, a vSError took place, the initial exception
- * doesn't have any significance to be handled. Exit ASAP
+ * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+ * not set. If a vSError took place, the initial exception will be
+ * skipped. Exit ASAP
  */
-cbnzx0, 1f
+ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+"nop; nop",
+SKIP_CHECK_PENDING_VSERROR)
 msr daifclr, #2
 mov x0, sp
 bl  do_trap_hypervisor
@@ -243,12 +246,14 @@ guest_sync:
 
 guest_irq:
 entry   hyp=0, compat=0
-bl  check_pending_vserror
 /*
- * If x0 is Non-zero, a vSError took place, the initial exception
- * doesn't have any significance to be handled. Exit ASAP
+ * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+ * not set. If a vSError took place, the initial exception will be
+ * skipped. Exit ASAP
  */
-cbnzx0, 1f
+ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+"nop; nop",
+SKIP_CHECK_PENDING_VSERROR)
 mov x0, sp
 bl  do_trap_irq
 1:
@@ -267,12 +272,14 @@ guest_error:
 
 guest_sync_compat:
 entry   hyp=0, compat=1
-bl  check_pending_vserror
 /*
- * If x0 is Non-zero, a vSError took place, the initial exception
- * doesn't have any significance to be handled. Exit ASAP
+ * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+ * not set. If a vSError took place, the initial exception will be
+ * skipped. Exit ASAP
  */
-cbnzx0, 1f
+ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+"nop; nop",
+SKIP_CHECK_PENDING_VSERROR)
 msr daifclr, #2
 mov x0, sp
 bl  do_trap_hypervisor
@@ -281,12 +288,14 @@ guest_sync_compat:
 
 guest_irq_compat:
 entry   hyp=0, compat=1
-bl  check_pending_vserror
 /*
- * If x0 is Non-zero, a vSError took place, the initial exception
- * doesn't have any significance to be handled. Exit ASAP
+ * The vSError will be checked while SKIP_CHECK_PENDING_VSERROR is
+ * not set. If a vSError took place, the initial exception will be
+ * skipped. Exit ASAP
  */
-cbnzx0, 1f
+ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
+"nop; nop",
+SKIP_CHECK_PENDING_VSERROR)
 mov x0, sp
 bl  do_trap_irq
 1:
-- 
2.7.4


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

[Xen-devel] [PATCH v3 13/19] xen/arm: Replace do_trap_guest_serror with new helpers

[Wei Chen]

We have introduced two helpers to handle the guest/hyp SErrors:
do_trap_guest_serror and do_trap_guest_hyp_serror. These handlers
can take the role of do_trap_guest_serror and reduce the assembly
code in the same time. So we use these two helpers to replace it
and drop it now.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/arm32/traps.c  |  5 +
 xen/arch/arm/arm64/entry.S  | 36 +++-
 xen/arch/arm/traps.c| 15 ---
 xen/include/asm-arm/processor.h |  2 --
 4 files changed, 4 insertions(+), 54 deletions(-)

diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index 4176f0e..5bc5f64 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -62,10 +62,7 @@ asmlinkage void do_trap_prefetch_abort(struct cpu_user_regs 
*regs)
 
 asmlinkage void do_trap_data_abort(struct cpu_user_regs *regs)
 {
-if ( VABORT_GEN_BY_GUEST(regs) )
-do_trap_guest_error(regs);
-else
-do_unexpected_trap("Data Abort", regs);
+do_trap_hyp_serror(regs);
 }
 
 /*
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 113e1c3..8d5a890 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -178,40 +178,10 @@ hyp_error_invalid:
 invalid BAD_ERROR
 
 hyp_error:
-/*
- * Only two possibilities:
- * 1) Either we come from the exit path, having just unmasked
- *PSTATE.A: change the return code to an EL2 fault, and
- *carry on, as we're already in a sane state to handle it.
- * 2) Or we come from anywhere else, and that's a bug: we panic.
- */
 entry   hyp=1
 msr daifclr, #2
-
-/*
- * The ELR_EL2 may be modified by an interrupt, so we have to use the
- * saved value in cpu_user_regs to check whether we come from 1) or
- * not.
- */
-ldr x0, [sp, #UREGS_PC]
-adr x1, abort_guest_exit_start
-cmp x0, x1
-adr x1, abort_guest_exit_end
-ccmpx0, x1, #4, ne
 mov x0, sp
-mov x1, #BAD_ERROR
-
-/*
- * Not equal, the exception come from 2). It's a bug, we have to
- * panic the hypervisor.
- */
-b.nedo_bad_mode
-
-/*
- * Otherwise, the exception come from 1). It happened because of
- * the guest. Crash this guest.
- */
-bl  do_trap_guest_error
+bl  do_trap_hyp_serror
 exithyp=1
 
 /* Traps taken in Current EL with SP_ELx */
@@ -267,7 +237,7 @@ guest_error:
 entry   hyp=0, compat=0
 msr daifclr, #2
 mov x0, sp
-bl  do_trap_guest_error
+bl  do_trap_guest_serror
 exithyp=0, compat=0
 
 guest_sync_compat:
@@ -309,7 +279,7 @@ guest_error_compat:
 entry   hyp=0, compat=1
 msr daifclr, #2
 mov x0, sp
-bl  do_trap_guest_error
+bl  do_trap_guest_serror
 exithyp=0, compat=1
 
 ENTRY(return_to_new_vcpu32)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4b53b7e..1c5cfb8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2900,21 +2900,6 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs 
*regs)
 }
 }
 
-asmlinkage void do_trap_guest_error(struct cpu_user_regs *regs)
-{
-enter_hypervisor_head(regs);
-
-/*
- * Currently, to ensure hypervisor safety, when we received a
- * guest-generated vSerror/vAbort, we just crash the guest to protect
- * the hypervisor. In future we can better handle this by injecting
- * a vSerror/vAbort to the guest.
- */
-gdprintk(XENLOG_WARNING, "Guest(Dom-%u) will be crashed by vSError\n",
- current->domain->domain_id);
-domain_crash_synchronous();
-}
-
 asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
 {
 enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 81227aa..bb24bee 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -707,8 +707,6 @@ void vcpu_regs_user_to_hyp(struct vcpu *vcpu,
 int call_smc(register_t function_id, register_t arg0, register_t arg1,
  register_t arg2);
 
-void do_trap_guest_error(struct cpu_user_regs *regs);
-
 void do_trap_hyp_serror(struct cpu_user_regs *regs);
 
 void do_trap_guest_serror(struct cpu_user_regs *regs);
-- 
2.7.4


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

[Xen-devel] [PATCH v3 11/19] xen/arm: Move macro VABORT_GEN_BY_GUEST to common header

[Wei Chen]

We want to move part of SErrors checking code from hyp_error assembly code
to a function. This new function will use this macro to distinguish the
guest SErrors from hypervisor SErrors. So we have to move this macro to
common header.

The VABORT_GEN_BY_GUEST macro uses the symbols abort_guest_exit_start
and abort_guest_exit_end. After we move this macro to a common header,
we need to make sure that the two symbols are visible to other source
files. Currently, they are declared .global in arm32/entry.S, but not
arm64/entry.S. Fix that.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 

---
v2->v3:
1. Modify the commit message as Stefano's suggestion.
2. Add Stefano's Reveiewed-by tag
---
 xen/arch/arm/arm64/entry.S|  2 ++
 xen/include/asm-arm/arm32/processor.h | 10 --
 xen/include/asm-arm/processor.h   | 10 ++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 4baa3cb..113e1c3 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -380,10 +380,12 @@ check_pending_vserror:
  * exception handler, and the elr_el2 will be set to
  * abort_guest_exit_start or abort_guest_exit_end.
  */
+.global abort_guest_exit_start
 abort_guest_exit_start:
 
 isb
 
+.global abort_guest_exit_end
 abort_guest_exit_end:
 /* Mask PSTATE asynchronous abort bit, close the checking window. */
 msr daifset, #4
diff --git a/xen/include/asm-arm/arm32/processor.h 
b/xen/include/asm-arm/arm32/processor.h
index f6d5df3..68cc821 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -56,16 +56,6 @@ struct cpu_user_regs
 uint32_t pad1; /* Doubleword-align the user half of the frame */
 };
 
-/* Functions for pending virtual abort checking window. */
-void abort_guest_exit_start(void);
-void abort_guest_exit_end(void);
-
-#define VABORT_GEN_BY_GUEST(r)  \
-( \
-( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
-( (unsigned long)abort_guest_exit_end == (r)->pc ) \
-)
-
 #endif
 
 /* Layout as used in assembly, with src/dest registers mixed in */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index d7b0711..163c39c 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -711,6 +711,16 @@ void do_trap_guest_error(struct cpu_user_regs *regs);
 
 register_t get_default_hcr_flags(void);
 
+/* Functions for pending virtual abort checking window. */
+void abort_guest_exit_start(void);
+void abort_guest_exit_end(void);
+
+#define VABORT_GEN_BY_GUEST(r)  \
+( \
+( (unsigned long)abort_guest_exit_start == (r)->pc ) || \
+( (unsigned long)abort_guest_exit_end == (r)->pc ) \
+)
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_ARM_PROCESSOR_H */
 /*
-- 
2.7.4


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

[Xen-devel] [PATCH v3 06/19] xen/arm: Introduce a virtual abort injection helper

[Wei Chen]

When guest triggers async aborts, in most platform, such aborts
will be routed to hypervisor. But we don't want the hypervisor
to handle such aborts, so we have to route such aborts back to
the guest.

This helper is using the HCR_EL2.VSE (HCR.VA for aarch32) bit to
route such aborts back to the guest. After updating HCR_EL2.VSE bit
in vCPU context, we write the value to HCR_EL2 immediately. In this
case we don't need to move the restoration of HCR_EL2 to other place,
and it worked regardless of whether we get preempted.

If the guest PC had been advanced by SVC/HVC/SMC instructions before
we caught the SError in hypervisor, we have to adjust the guest PC to
exact address while the SError generated.

About HSR_EC_SVC32/64, even thought we don't trap SVC32/64 today,
we would like them to be handled here. This would be useful when
VM introspection will gain support of SVC32/64 trapping.

After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
to HCR_EL2 immediately. In this case we don't need to move the
restoration of HCR_EL2 to leave_hypervisor_tail, and it worked
regardless of whether we get preempted.

This helper will be used by the later patches in this series, we
use #if 0 to disable it in this patch temporarily to remove the
warning message of unused function from compiler.

Signed-off-by: Wei Chen 
Acked-by: Stefano Stabellini 
Reviewed-by: Stefano Stabellini 

---
v2->v3:
1. After updating HCR_EL2.VSE bit of vCPU HCR_EL2, write the value
   to HCR_EL2 immediately.
2. Explain in commit message why we write the value to HCR_EL2 after
   updating the HCR_EL2.VSE bit in vCPU context.
3. Add Stefano's Reviewed-by tag.
---
 xen/arch/arm/traps.c| 33 +
 xen/include/asm-arm/processor.h |  1 +
 2 files changed, 34 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 5f758e1..b15923a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -618,6 +618,39 @@ static void inject_dabt_exception(struct cpu_user_regs 
*regs,
 #endif
 }
 
+#if 0
+/* Inject a virtual Abort/SError into the guest. */
+static void inject_vabt_exception(struct cpu_user_regs *regs)
+{
+const union hsr hsr = { .bits = regs->hsr };
+
+/*
+ * SVC/HVC/SMC already have an adjusted PC (See ARM ARM DDI 0487A.j
+ * D1.10.1 for more details), which we need to correct in order to
+ * return to after having injected the SError.
+ */
+switch ( hsr.ec )
+{
+case HSR_EC_SVC32:
+case HSR_EC_HVC32:
+case HSR_EC_SMC32:
+#ifdef CONFIG_ARM_64
+case HSR_EC_SVC64:
+case HSR_EC_HVC64:
+case HSR_EC_SMC64:
+#endif
+regs->pc -= hsr.len ? 4 : 2;
+break;
+
+default:
+break;
+}
+
+current->arch.hcr_el2 |= HCR_VA;
+WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
+}
+#endif
+
 struct reg_ctxt {
 /* Guest-side state */
 uint32_t sctlr_el1;
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 4b6338b..d7b0711 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -252,6 +252,7 @@
 #define HSR_EC_HVC320x12
 #define HSR_EC_SMC320x13
 #ifdef CONFIG_ARM_64
+#define HSR_EC_SVC640x15
 #define HSR_EC_HVC640x16
 #define HSR_EC_SMC640x17
 #define HSR_EC_SYSREG   0x18
-- 
2.7.4


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

[Xen-devel] [PATCH v3 12/19] xen/arm: Introduce new helpers to handle guest/hyp SErrors

[Wei Chen]

Currently, ARM32 and ARM64 has different SError exception handlers.
These handlers include lots of code to check SError handle options
and code to distinguish guest-generated SErrors from hypervisor
SErrors.

The new helpers: do_trap_guest_serror and do_trap_hyp_serror are
wrappers of __do_trap_serror with constant guest/hyp parameters.
__do_trap_serror moves the option checking code and SError checking
code from assembly to C source. This will make the code become more
readable and avoid placing check code in too many places.

These two helpers only handle the following 3 types of SErrors:
1) Guest-generated SError and had been delivered in EL1 and then
   been forwarded to EL2.
2) Guest-generated SError but hadn't been delivered in EL1 before
   trapping to EL2. This SError would be caught in EL2 as soon as
   we just unmasked the PSTATE.A bit.
3) Hypervisor generated native SError, that would be a bug.

In the new helpers, we have used the function "inject_vabt_exception"
which was disabled by "#if 0" before. Now, we can remove the "#if 0"
to make this function to be available.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/traps.c| 69 +++--
 xen/include/asm-arm/processor.h |  4 +++
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9d4ee39..4b53b7e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -646,7 +646,6 @@ static void inject_dabt_exception(struct cpu_user_regs 
*regs,
 #endif
 }
 
-#if 0
 /* Inject a virtual Abort/SError into the guest. */
 static void inject_vabt_exception(struct cpu_user_regs *regs)
 {
@@ -677,7 +676,59 @@ static void inject_vabt_exception(struct cpu_user_regs 
*regs)
 current->arch.hcr_el2 |= HCR_VA;
 WRITE_SYSREG(current->arch.hcr_el2, HCR_EL2);
 }
-#endif
+
+/*
+ * SError exception handler. We only handle the following 3 types of SErrors:
+ * 1) Guest-generated SError and had been delivered in EL1 and then
+ *been forwarded to EL2.
+ * 2) Guest-generated SError but hadn't been delivered in EL1 before
+ *trapping to EL2. This SError would be caught in EL2 as soon as
+ *we just unmasked the PSTATE.A bit.
+ * 3) Hypervisor generated native SError, that would be a bug.
+ *
+ * A true parameter "guest" means that the SError is type#1 or type#2.
+ */
+static void __do_trap_serror(struct cpu_user_regs *regs, bool guest)
+{
+/*
+ * Only "DIVERSE" option needs to distinguish the guest-generated SErrors
+ * from hypervisor SErrors.
+ */
+if ( serrors_op == SERRORS_DIVERSE )
+{
+/* Forward the type#1 and type#2 SErrors to guests. */
+if ( guest )
+return inject_vabt_exception(regs);
+
+/* Type#3 SErrors will panic the whole system */
+goto crash_system;
+}
+
+/*
+ * The "FORWARD" option will forward all SErrors to the guests, except
+ * idle domain generated SErrors.
+ */
+if ( serrors_op == SERRORS_FORWARD )
+{
+/*
+ * Because the idle domain doesn't have the ability to handle the
+ * SErrors, we have to crash the whole system while we get a SError
+ * generated by idle domain.
+ */
+if ( is_idle_vcpu(current) )
+goto crash_system;
+
+return inject_vabt_exception(regs);
+}
+
+crash_system:
+/* Three possibilities to crash the whole system:
+ * 1) "DIVERSE" option with Hypervisor generated SErrors.
+ * 2) "FORWARD" option with Idle Domain generated SErrors.
+ * 3) "PANIC" option with all SErrors.
+ */
+do_unexpected_trap("SError", regs);
+}
 
 struct reg_ctxt {
 /* Guest-side state */
@@ -2864,6 +2915,20 @@ asmlinkage void do_trap_guest_error(struct cpu_user_regs 
*regs)
 domain_crash_synchronous();
 }
 
+asmlinkage void do_trap_hyp_serror(struct cpu_user_regs *regs)
+{
+enter_hypervisor_head(regs);
+
+__do_trap_serror(regs, VABORT_GEN_BY_GUEST(regs));
+}
+
+asmlinkage void do_trap_guest_serror(struct cpu_user_regs *regs)
+{
+enter_hypervisor_head(regs);
+
+__do_trap_serror(regs, true);
+}
+
 asmlinkage void do_trap_irq(struct cpu_user_regs *regs)
 {
 enter_hypervisor_head(regs);
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 163c39c..81227aa 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -709,6 +709,10 @@ int call_smc(register_t function_id, register_t arg0, 
register_t arg1,
 
 void do_trap_guest_error(struct cpu_user_regs *regs);
 
+void do_trap_hyp_serror(struct cpu_user_regs *regs);
+
+void do_trap_guest_serror(struct cpu_user_regs *regs);
+
 register_t get_default_hcr_flags(void);
 
 /* Functions for pending virtual abort checking window. */
-- 
2.7.4


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

[Xen-devel] [PATCH v3 07/19] xen/arm: Introduce a command line parameter for SErrors/Aborts

[Wei Chen]

In order to distinguish guest-generated SErrors from hypervisor-generated
SErrors we have to place SError checking code in every EL1 <-> EL2 paths.
That will cause overhead on entries and exits due to dsb/isb.

However, not all platforms want to categorize SErrors. For example, a host
that is running with trusted guests. The administrator can confirm that
all guests that are running on the host will not trigger such SErrors. In
this use-case, we should provide some options to administrators to avoid
categorizing SErrors and then reduce the overhead of dsb/isb.

We provided the following 3 options to administrators to determine how
the hypervisors handle SErrors:

* `diverse`:
  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
  The guest generated SErrors will be forwarded to guests, the hypervisor
  generated SErrors will cause the whole system to crash.
  It requires:
  1. dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors
 correctly.
  2. dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
 SErrors to guests.
  3. dsb/isb in context switch to isolate SErrors between 2 vCPUs.

* `forward`:
  The hypervisor will not distinguish guest SErrors from hypervisor
  SErrors. All SErrors will be forwarded to guests, except the SErrors
  generated when  the idle vCPU is running. The idle domain doesn't have
  the ability to handle SErrors, so we have to crash the whole system when
  we get SErros with the idle vCPU. This option will avoid most overhead
  of the dsb/isb, except the dsb/isb in context switch which is used to
  isolate the SErrors between 2 vCPUs.

* `panic`:
  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
  All SErrors will crash the whole system. This option will avoid all
  overhead of the dsb/isb pairs.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 

---
v2->v3:
1. Replace "entries" to "entries and exits" in commit message and doc.
   because all options will take effect on entries and exits.
2. Fix a typo in commit message.
---
 docs/misc/xen-command-line.markdown | 44 +
 xen/arch/arm/traps.c| 19 
 2 files changed, 63 insertions(+)

diff --git a/docs/misc/xen-command-line.markdown 
b/docs/misc/xen-command-line.markdown
index 9eb85d6..9d42b6a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1470,6 +1470,50 @@ enabling more sockets and cores to go into deeper sleep 
states.
 
 Set the serial transmit buffer size.
 
+### serrors (ARM)
+> `= diverse | forward | panic`
+
+> Default: `diverse`
+
+This parameter is provided to administrators to determine how the
+hypervisors handle SErrors.
+
+In order to distinguish guest-generated SErrors from hypervisor-generated
+SErrors we have to place SError checking code in every EL1 <-> EL2 paths.
+That will cause overhead on entries and exits due to dsb/isb. However, not all
+platforms need to categorize SErrors. For example, a host that is running with
+trusted guests. The administrator can confirm that all guests that are running
+on the host will not trigger such SErrors. In this case, the administrator can
+use this parameter to skip categorizing SErrors and reduce the overhead of
+dsb/isb.
+
+We provided the following 3 options to administrators to determine how the
+hypervisors handle SErrors:
+
+* `diverse`:
+  The hypervisor will distinguish guest SErrors from hypervisor SErrors.
+  The guest generated SErrors will be forwarded to guests, the hypervisor
+  generated SErrors will cause the whole system to crash.
+  It requires:
+  1. dsb/isb on all EL1 -> EL2 trap entries to categorize SErrors correctly.
+  2. dsb/isb on EL2 -> EL1 return paths to prevent slipping hypervisor
+ SErrors to guests.
+  3. dsb/isb in context switch to isolate SErrors between 2 vCPUs.
+
+* `forward`:
+  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
+  All SErrors will be forwarded to guests, except the SErrors generated when
+  the idle vCPU is running. The idle domain doesn't have the ability to handle
+  SErrors, so we have to crash the whole system when we get SErros with the
+  idle vCPU. This option will avoid most overhead of the dsb/isb, except the
+  dsb/isb in context switch which is used to isolate the SErrors between 2
+  vCPUs.
+
+* `panic`:
+  The hypervisor will not distinguish guest SErrors from hypervisor SErrors.
+  All SErrors will crash the whole system. This option will avoid all overhead
+  of the dsb/isb pairs.
+
 ### smap
 > `=  | hvm`
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index b15923a..76cda59 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -122,6 +122,25 @@ register_t get_default_hcr_flags(void)
  HCR_TSC|HCR_TAC|HCR_SWIO|HCR_TIDCP|HCR_FB);
 }
 
+static enum {
+SERRORS_DIVERSE,
+SERRORS_FORWARD,
+SERRORS_PANIC,
+} serrors_op;
+
+static void __init pars

[Xen-devel] [PATCH v3 14/19] xen/arm: Unmask the Abort/SError bit in the exception entries

[Wei Chen]

Currently, we masked the Abort/SError bit in Xen exception entries.
So Xen could not capture any Abort/SError while it's running.
Now, Xen has the ability to handle the Abort/SError, we should unmask
the Abort/SError bit by default to let Xen capture Abort/SError while
it's running.

But in order to avoid receiving nested asynchronous abort, we don't
unmask Abort/SError bit in hyp_error and trap_data_abort.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/arm32/entry.S | 15 ++-
 xen/arch/arm/arm64/entry.S | 13 -
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 105cae8..592e4a8 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -116,6 +116,7 @@ skip_check:
 trap_##trap:\
 SAVE_ALL;   \
 cpsie i;/* local_irq_enable */  \
+cpsie a;/* asynchronous abort enable */ \
 adr lr, return_from_trap;   \
 mov r0, sp; \
 mov r11, sp;\
@@ -126,6 +127,18 @@ trap_##trap:   
 \
 ALIGN;  \
 trap_##trap:\
 SAVE_ALL;   \
+cpsie a;/* asynchronous abort enable */ \
+adr lr, return_from_trap;   \
+mov r0, sp; \
+mov r11, sp;\
+bic sp, #7; /* Align the stack pointer (noop on guest trap) */  \
+b do_trap_##trap
+
+#define DEFINE_TRAP_ENTRY_NOABORT(trap) \
+ALIGN;  \
+trap_##trap:\
+SAVE_ALL;   \
+cpsie i;/* local_irq_enable */  \
 adr lr, return_from_trap;   \
 mov r0, sp; \
 mov r11, sp;\
@@ -146,10 +159,10 @@ GLOBAL(hyp_traps_vector)
 DEFINE_TRAP_ENTRY(undefined_instruction)
 DEFINE_TRAP_ENTRY(supervisor_call)
 DEFINE_TRAP_ENTRY(prefetch_abort)
-DEFINE_TRAP_ENTRY(data_abort)
 DEFINE_TRAP_ENTRY(hypervisor)
 DEFINE_TRAP_ENTRY_NOIRQ(irq)
 DEFINE_TRAP_ENTRY_NOIRQ(fiq)
+DEFINE_TRAP_ENTRY_NOABORT(data_abort)
 
 return_from_trap:
 mov sp, r11
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 8d5a890..0401a41 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -187,13 +187,14 @@ hyp_error:
 /* Traps taken in Current EL with SP_ELx */
 hyp_sync:
 entry   hyp=1
-msr daifclr, #2
+msr daifclr, #6
 mov x0, sp
 bl  do_trap_hypervisor
 exithyp=1
 
 hyp_irq:
 entry   hyp=1
+msr daifclr, #4
 mov x0, sp
 bl  do_trap_irq
 exithyp=1
@@ -208,7 +209,7 @@ guest_sync:
 ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
 "nop; nop",
 SKIP_CHECK_PENDING_VSERROR)
-msr daifclr, #2
+msr daifclr, #6
 mov x0, sp
 bl  do_trap_hypervisor
 1:
@@ -224,6 +225,7 @@ guest_irq:
 ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
 "nop; nop",
 SKIP_CHECK_PENDING_VSERROR)
+msr daifclr, #4
 mov x0, sp
 bl  do_trap_irq
 1:
@@ -235,7 +237,7 @@ guest_fiq_invalid:
 
 guest_error:
 entry   hyp=0, compat=0
-msr daifclr, #2
+msr daifclr, #6
 mov x0, sp
 bl  do_trap_guest_serror
 exithyp=0, compat=0
@@ -250,7 +252,7 @@ guest_sync_compat:
 ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
 "nop; nop",
 SKIP_CHECK_PENDING_VSERROR)
-msr daifclr, #2
+msr daifclr, #6
 mov x0, sp
 bl  do_trap_hypervisor
 1:
@@ -266,6 +268,7 @@ guest_irq_compat:
 ALTERNATIVE("bl check_pending_vserror; cbnz x0, 1f",
 "nop; nop",
 SKIP_CHECK_PENDING_VSERROR)
+msr daifclr, #4
 mov x0, sp
 bl  do_trap_irq
 1:
@@ -277,7 +280,7 @@ guest_fiq_invalid

[Xen-devel] [PATCH v3 18/19] xen/arm: Prevent slipping hypervisor SError to guest

[Wei Chen]

If there is a pending SError while we're returning from trap. If the
SError handle option is "DIVERSE", we have to prevent slipping this
hypervisor SError to guest. So we have to use the dsb/isb to guarantee
that the pending hypervisor SError would be caught in hypervisor before
return to guest.

In previous patch, we will set SKIP_CHECK_PENDING_VSERROR to cpu_hwcaps
when option is NOT "DIVERSE". This means we can use the alternative to
skip synchronizing SErrors for other SErrors handle options.

Because we have umasked the Abort/SError bit in previous patch. We have
to disable the Abort/SError before returning to guest as we have done
for IRQ.

Signed-off-by: Wei Chen 

---
v2->v3:
1. Use alternative instead of check serror_op to skip sychronizing SErrors
   while option is NOT "DIVERSE".
2. Disable Abort/SError before returning to guest.
---
 xen/arch/arm/traps.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2c610c4..8f1a0cd 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2936,6 +2936,19 @@ asmlinkage void leave_hypervisor_tail(void)
 local_irq_disable();
 if (!softirq_pending(smp_processor_id())) {
 gic_inject();
+
+/*
+ * If the SErrors handle option is "DIVERSE", we have to prevent
+ * slipping the hypervisor SError to guest. In this option, before
+ * returning from trap, we have to synchronize SErrors to guarantee
+ * that the pending SError would be caught in hypervisor.
+ *
+ * If option is NOT "DIVERSE", SKIP_CHECK_PENDING_VSERROR will be
+ * set to cpu_hwcaps. This means we can use the alternative to skip
+ * synchronizing SErrors for other SErrors handle options.
+ */
+SYNCHRONIZE_SERROR(SKIP_CHECK_PENDING_VSERROR);
+
 return;
 }
 local_irq_enable();
-- 
2.7.4


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

[Xen-devel] [PATCH v3 10/19] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to skip the check of pending SError,
in conventional way, we have to read the option every time before
we try to check the pending SError. This will add overhead to check
the option at every trap.

The ARM32 supports the alternative patching feature. We can use an
ALTERNATIVE to avoid checking option at every trap. We added a new
cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
enabled when the option is not diverse.

Signed-off-by: Wei Chen 
Reviewed-by: Julien Grall 

---
Note:
This patch depends on the "xen/arm32: Introduce alternative runtime
patching" [1] which is still in review.

[1]https://lists.xenproject.org/archives/html/xen-devel/2017-03/msg04333.html
---
 xen/arch/arm/arm32/entry.S | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 2187226..105cae8 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -1,5 +1,6 @@
 #include 
 #include 
+#include 
 #include 
 
 #define SAVE_ONE_BANKED(reg)mrs r11, reg; str r11, [sp, #UREGS_##reg]
@@ -44,6 +45,14 @@ save_guest_regs:
 SAVE_BANKED(fiq)
 SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq); 
SAVE_ONE_BANKED(R10_fiq)
 SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
+
+/*
+ * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu feature,
+ * the checking of pending SErrors will be skipped.
+ */
+ALTERNATIVE("nop",
+"b skip_check",
+SKIP_CHECK_PENDING_VSERROR)
 /*
  * Start to check pending virtual abort in the gap of Guest -> HYP
  * world switch.
@@ -99,6 +108,7 @@ abort_guest_exit_end:
  */
 bne return_from_trap
 
+skip_check:
 mov pc, lr
 
 #define DEFINE_TRAP_ENTRY(trap) \
-- 
2.7.4


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

[Xen-devel] [PATCH v3 19/19] xen/arm: Handle guest external abort as guest SError

[Wei Chen]

The guest generated external data/instruction aborts can be treated
as guest SErrors. We already have a handler to handle the SErrors,
so we can reuse this handler to handle guest external aborts.

Signed-off-by: Wei Chen 
Reviewed-by: Stefano Stabellini 
---
 xen/arch/arm/traps.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8f1a0cd..9e8719c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2562,12 +2562,12 @@ static void do_trap_instr_abort_guest(struct 
cpu_user_regs *regs,
 
 /*
  * If this bit has been set, it means that this instruction abort is caused
- * by a guest external abort. Currently we crash the guest to protect the
- * hypervisor. In future one can better handle this by injecting a virtual
- * abort to the guest.
+ * by a guest external abort. We can handle this instruction abort as guest
+ * SError.
  */
 if ( hsr.iabt.eat )
-domain_crash_synchronous();
+return __do_trap_serror(regs, true);
+
 
 if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
 gpa = get_faulting_ipa(gva);
@@ -2665,12 +2665,10 @@ static void do_trap_data_abort_guest(struct 
cpu_user_regs *regs,
 
 /*
  * If this bit has been set, it means that this data abort is caused
- * by a guest external abort. Currently we crash the guest to protect the
- * hypervisor. In future one can better handle this by injecting a virtual
- * abort to the guest.
+ * by a guest external abort. We treat this data abort as guest SError.
  */
 if ( dabt.eat )
-domain_crash_synchronous();
+return __do_trap_serror(regs, true);
 
 info.dabt = dabt;
 #ifdef CONFIG_ARM_32
-- 
2.7.4


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

[Xen-devel] [PATCH v3 15/19] xen/arm: Introduce a helper to check local abort is enabled

[Wei Chen]

In previous patch, we have umasked the Abort/SError bit for Xen
in most of its running time. So in some use-cases, we have to
check whether the abort is enabled in current context. For example,
while we want to synchronize SErrors, we have to confirm the abort
is enabled. Otherwise synchronize SErrors is pointless.

Signed-off-by: Wei Chen 
---
 xen/include/asm-arm/arm32/system.h | 7 +++
 xen/include/asm-arm/arm64/system.h | 7 +++
 2 files changed, 14 insertions(+)

diff --git a/xen/include/asm-arm/arm32/system.h 
b/xen/include/asm-arm/arm32/system.h
index c617b40..6c5b9f5 100644
--- a/xen/include/asm-arm/arm32/system.h
+++ b/xen/include/asm-arm/arm32/system.h
@@ -35,6 +35,13 @@ static inline int local_irq_is_enabled(void)
 return !(flags & PSR_IRQ_MASK);
 }
 
+static inline int local_abort_is_enabled(void)
+{
+unsigned long flags;
+local_save_flags(flags);
+return !(flags & PSR_ABT_MASK);
+}
+
 #define local_fiq_enable()  __asm__("cpsie f   @ __stf\n" : : : "memory", "cc")
 #define local_fiq_disable() __asm__("cpsid f   @ __clf\n" : : : "memory", "cc")
 
diff --git a/xen/include/asm-arm/arm64/system.h 
b/xen/include/asm-arm/arm64/system.h
index 2e2ee21..309485f 100644
--- a/xen/include/asm-arm/arm64/system.h
+++ b/xen/include/asm-arm/arm64/system.h
@@ -58,6 +58,13 @@ static inline int local_fiq_is_enabled(void)
 return !(flags & PSR_FIQ_MASK);
 }
 
+static inline int local_abort_is_enabled(void)
+{
+unsigned long flags;
+local_save_flags(flags);
+return !(flags & PSR_ABT_MASK);
+}
+
 #endif
 /*
  * Local variables:
-- 
2.7.4


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

[Xen-devel] [PATCH v3 17/19] xen/arm: Isolate the SError between the context switch of 2 vCPUs

[Wei Chen]

If there is a pending SError while we are doing context switch, if the
SError handle option is "FORWARD", We have to guranatee this serror to
be caught by current vCPU, otherwise it will be caught by next vCPU and
be forwarded to this wrong vCPU.

So we have to synchronize SError before switch to next vCPU. But this is
only required by "FORWARD" option. In this case we added a new flag
SKIP_CTXT_SWITCH_SERROR_SYNC in cpu_hwcaps to skip synchronizing SError
in context switch for other options. In the meantime, we don't need to
export serror_op accessing to other source files.

Because we have umasked the Abort/SError bit in previous patch, we have
to disable Abort/SError before doing context switch as we have done for
IRQ.

Signed-off-by: Wei Chen 

---
v2->v3:
1. Use the macro instead of the function to synchronize SErrors.
2. Disable Abort/SError before doing context switch.
---
 xen/arch/arm/domain.c| 12 
 xen/arch/arm/traps.c |  3 +++
 xen/include/asm-arm/cpufeature.h |  3 ++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 69c2854..e1a620a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -312,6 +313,17 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
 
 local_irq_disable();
 
+/*
+ * If the SErrors option is "FORWARD", we have to prevent forwarding
+ * serror to wrong vCPU. So before context switch, we have to use the
+ * synchronize_serror to guarantee that the pending serror would be
+ * caught by current vCPU.
+ *
+ * The SKIP_CTXT_SWITCH_SERROR_SYNC will be set to cpu_hwcaps when the
+ * SErrors option is NOT "FORWARD".
+ */
+SYNCHRONIZE_SERROR(SKIP_CTXT_SWITCH_SERROR_SYNC);
+
 set_current(next);
 
 prev = __context_switch(prev, next);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1c5cfb8..2c610c4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -146,6 +146,9 @@ static int __init update_serrors_cpu_caps(void)
 if ( serrors_op != SERRORS_DIVERSE )
 cpus_set_cap(SKIP_CHECK_PENDING_VSERROR);
 
+if ( serrors_op != SERRORS_FORWARD )
+cpus_set_cap(SKIP_CTXT_SWITCH_SERROR_SYNC);
+
 return 0;
 }
 __initcall(update_serrors_cpu_caps);
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index ec3f9e5..99ecb2b 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -41,8 +41,9 @@
 #define ARM64_WORKAROUND_834220 3
 #define LIVEPATCH_FEATURE   4
 #define SKIP_CHECK_PENDING_VSERROR 5
+#define SKIP_CTXT_SWITCH_SERROR_SYNC 6
 
-#define ARM_NCAPS   6
+#define ARM_NCAPS   7
 
 #ifndef __ASSEMBLY__
 
-- 
2.7.4


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

[Xen-devel] [PATCH v3 16/19] xen/arm: Introduce a macro to synchronize SError

[Wei Chen]

In previous patches, we have provided the ability to synchronize
SErrors in exception entries. But we haven't synchronized SErrors
while returning to guest and doing context switch.

So we still have two risks:
1. Slipping hypervisor SErrors to guest. For example, hypervisor
   triggers a SError while returning to guest, but this SError may be
   delivered after entering guest. In "DIVERSE" option, this SError
   would be routed back to guest and panic the guest. But actually,
   we should crash the whole system due to this hypervisor SError.
2. Slipping previous guest SErrors to the next guest. In "FORWARD"
   option, if hypervisor triggers a SError while context switching.
   This SError may be delivered after switching to next vCPU. In this
   case, this SError will be forwarded to next vCPU and may panic
   an incorrect guest.

So we have have to introduce this macro to synchronize SErrors while
returning to guest and doing context switch.

We also added a barrier to this macro to prevent compiler reorder our
asm volatile code.

Signed-off-by: Wei Chen 
---
v2->v3:
1. Use a macro to replace function to synchronize SErrors.
2. Add barrier to avoid compiler reorder our code.

Note:
I had followed Julien's suggestion to add the Assert in to macro,
But I found I always hit the Assert. Because when option == DIVERSE,
the SKIP_CHECK_PENDING_VSERROR feature would not be set into cpu_hwcaps.
And in the later patch, I will use this feature to skip synchronize
SErrors before returning to guest.
The cpus_has_cap(SKIP_CHECK_PENDING_VSERROR) will always false.
And hit the ASSERT.

And about the local_abort enable check, should we disable the abort
before synchronizing SErrors while returning to guest or doing context
switch? Just like in these two places we have disable the IRQ.

For this testing, I have apply this series to latest staging tree.

...
(XEN) Command line: console=dtuart dtuart=serial0 conswitch=x loglvl=all
dom0_mem=8G dom0_max_vcpus=8 serrors=diverse
(XEN) Placing Xen at 0x0083fee0-0x0083ff00
...
(XEN) SYNCHRONIZE_SERROR ASSERT 0 1
(XEN) Assertion 'cpus_have_cap(5) && local_abort_is_enabled()' failed at
traps.c:2954
(XEN) [ Xen-4.9-unstable  arm64  debug=y   Not tainted ]
(XEN) CPU:0
(XEN) PC: 0025c09c leave_hypervisor_tail+0xa8/0x100
(XEN) LR: 0025c078
(XEN) SP: 8003fac07e80
(XEN) CPSR:   82c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)  X0: 005a  X1:   X2:

(XEN)  X3:   X4: 0010  X5:

(XEN)  X6: 8003ffe5  X7: 0001  X8:
fffd
(XEN)  X9: 000a X10: 0031 X11:
8003fac07bf8
(XEN) X12: 0001 X13: 0026c370 X14:
0020
(XEN) X15:  X16: 0083fff42fc0 X17:
fffe
(XEN) X18:  X19: 00292c58 X20:
00290028
(XEN) X21: 002ea000 X22:  X23:

(XEN) X24:  X25:  X26:

(XEN) X27:  X28:   FP:
8003fac07e80
(XEN)
(XEN)   VTCR_EL2: 80043594
(XEN)  VTTBR_EL2: 00010083fd036000
(XEN)
(XEN)  SCTLR_EL2: 30cd183d
(XEN)HCR_EL2: 8038663f
(XEN)  TTBR0_EL2: 0083fef0e000
(XEN)
(XEN)ESR_EL2: f201
(XEN)  HPFAR_EL2: 
(XEN)FAR_EL2: 
(XEN)
(XEN) Xen stack trace from sp=8003fac07e80:
(XEN)8003fac07ea0 00262934 

(XEN) 00249cac 00804800

(XEN)  

(XEN)  

(XEN)  

(XEN)  

(XEN)  

(XEN)  

(XEN)  

(XEN)  00804008
01c5
(XEN)  

(XEN)  

(XEN) Xen call trace:
(XEN)[<0025c09c>] leave_hypervisor_tail+0xa8/0x100 (PC)
(XEN)[<0025c078>] leave_hypervisor_tail+0x84/0x100 (LR)
(XEN)[<00262934>] return_to_new_vcpu64+0x4/0x30
(XEN)[<00249cac>] domain.c#continue_new_vcpu+0/0xa4
(XEN)
(XEN)
(XEN) 
(XEN) Panic on CPU 0:
(XEN) Assertion 'cpus_have_cap(5) && local_abort_is_enabled()' failed at
traps.c:2954
(XEN) 
(XEN)
(XEN) Reboot in five seconds...
---

[Xen-devel] [xen-4.8-testing test] 107019: tolerable FAIL - PUSHED

[osstest service owner]

flight 107019 xen-4.8-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/107019/

Failures :-/ but no regressions.

Regressions which are regarded as allowable (not blocking):
 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 106992
 test-amd64-amd64-xl-qemut-win7-amd64 16 guest-stopfail like 106992
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stopfail like 106992
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-stop fail like 106992
 test-amd64-amd64-xl-rtds  9 debian-install   fail  like 106992

Tests which did not succeed, but are not blocking:
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 build-arm64-libvirt   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-qcow2  1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-rtds  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 build-arm64   5 xen-buildfail   never pass
 test-amd64-amd64-xl-pvh-intel 11 guest-start  fail  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-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvh-amd  11 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 build-arm64-xsm   5 xen-buildfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 saverestore-support-checkfail   never pass
 build-arm64-pvops 5 kernel-build fail   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-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 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-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  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 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-vhd  11 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2  fail never pass

version targeted for testing:
 xen  f3623bdbe5f7ff63e728865a8b986b2312231685
baseline version:
 xen  ca41491f0507150139fc35ff6c9f076fdbe9487b

Last test of basis   106992  2017-03-30 05:53:36 Z1 days
Testing same since   107019  2017-03-31 06:44:43 Z0 days1 attempts


People who touched revisions under test:
  Daniel De Graaf 
  Dario Faggioli 
  Jan Beulich 
  Roger Pau Monné 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  fail
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64-xtf  pass

Re: [Xen-devel] [PATCH v9 12/25] x86: refactor psr: L3 CAT: set value: implement cos id picking flow.

[Yi Sun]

On 17-03-31 06:51:07, Jan Beulich wrote:
> >>> On 31.03.17 at 14:40,  wrote:
> > On 17-03-31 04:19:49, Jan Beulich wrote:
> >> >>> On 31.03.17 at 11:12,  wrote:
> >> > On 17-03-31 02:47:25, Jan Beulich wrote:
> >> >> >>> On 30.03.17 at 14:10,  wrote:
> >> >> > I think you can check v9 codes here:
> >> >> > https://github.com/yisun-git/xen/tree/l2_cat_v9 
> >> >> 
> >> >> Looking at this made me notice that cat_get_old_val() passes a
> >> >> bogus literal 0 to cat_get_val(), which needs taking care of too.
> >> >> One option I can see is for each feature to make available an
> >> >> array of type enum cbm_type, with cos_num elements. The order
> >> >> would match that of the order of values in their arrays. This will
> >> > 
> >> > Sorry, not very clear your meaning. How to do that? Could you please
> >> > provide pieces of codes? Thanks!
> >> 
> >> I'm sorry, but I'm afraid I don't see how I would reasonably supply
> >> code here without taking over your series altogether (which I don't
> >> intend to do). What is unclear with, at the example of CDP, you
> >> needing to add an array at initialization time, slot 0 of which holds
> >> PSR_CBM_TYPE_L3_DATA and slot 1 PSR_CBM_TYPE_L3_CODE (or
> >> the other way around). Granted I was wrong with the type of the
> >> array (as the above aren't enum psr_feat_type enumerators, but
> >> enum cbm_type ones), but I think the basic idea should have been
> >> clear anyway: You need to provide a way for generic code to pass
> >> suitable type information into ->get_val().
> >> 
> > May I change the 'get_val()' parameter 'enum cbm_type' to a generic type
> > 'unsigned int' to make it be a flexible type,  and then combine feature
> > type with cos_num together as a flag to indicate which feature it is,
> > which value to get and distinguish it with cbm_type? For example:
> > 
> > #define CDP_GATHER_BOTH_DATA ( PSR_SOCKET_L3_CDP << 16 )
> > #define CDP_GATHER_BOTH_CODE ( PSR_SOCKET_L3_CDP << 16 + 1 )
> > static void l3_cdp_get_val(const struct feat_node *feat, unsigned int cos,
> >unsigned int type, uint32_t *val)
> > {
> > switch ( type )
> > {
> > case PSR_CBM_TYPE_L3_DATA:
> > case CDP_GATHER_BOTH_DATA:
> > *val = get_cdp_data(feat, cos);
> > break;
> > case PSR_CBM_TYPE_L3_CODE:
> > case CDP_GATHER_BOTH_CODE:
> > *val = get_cdp_code(feat, cos);
> > break;
> > }
> > }
> 
> The two case labels are still indicative of unnecessary redundancy
> (and, even right now only highly theoretical, risk of collisions). What's
> wrong with the model I've proposed?
> 
Oh, sorry. I did not understand your proposal just now so I provided another
solution.

After reading your suggestion again, I think your meaing is below in codes:

struct feat_props {
...
unsigned int cos_num;
enum cbm_type cos_to_type[2];
...
}

static void cat_init_feature(...)
{
...
case PSR_SOCKET_L3_CDP:
feat->props->cos_to_type[0] = PSR_CBM_TYPE_L3_DATA;
feat->props->cos_to_type[1] = PSR_CBM_TYPE_L3_CODE;
...
}

Then, in functions to iterate 'cos_num', we can input 'cos_to_type[i]'. Right?

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

Re: [Xen-devel] [PATCH] xl: Add 'pvh' config option

[Boris Ostrovsky]

On 03/31/2017 06:23 AM, Roger Pau Monné wrote:
> On Thu, Mar 30, 2017 at 07:06:15PM -0400, Boris Ostrovsky wrote:
>> In addition to 'device_model_version="none"' config option users can
>> also use 'pvh=1' in xl configuration file when creating a PVH guest.
> I'm not sure, but I think the plan was to remove device_model_version="none"
> and instead just use pvh=1, instead of keeping both. I don't have a strong
> opinion here, so I will leave that to the xl maintainers.

I thought we had PVHv2 in 4.8 but apparently not. I will remove it then.

>
> I'm also not sure, but if you use device_model_version="none" can you use 
> QDISK
> for PVH disk backend? (and pygrub).

We are not starting qemu so I don't think I see how we can use qdisk
(unless you are suggesting that we should do that for qdisk specifically).

Not sure what you are asking for pygrub.

>
>> We can skip parsing options related to device model once we establish
>> that we are building PVH guest.
>>
>> Also process 'device_model_version="none"' for HVM guests only since
>> it is not a valid model for PV guests.
>>
>> Signed-off-by: Boris Ostrovsky 
>> ---
>>  docs/man/xl.cfg.pod.5.in |  7 ++-
>>  tools/xl/xl_parse.c  | 12 +++-
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
>> index 206d33e..5833987 100644
>> --- a/docs/man/xl.cfg.pod.5.in
>> +++ b/docs/man/xl.cfg.pod.5.in
>> @@ -1201,6 +1201,11 @@ expose unexpected bugs in the guest, or find bugs in 
>> Xen, so it is
>>  possible to disable this feature.  Use of out of sync page tables,
>>  when Xen thinks it appropriate, is the default.
>>  
>> +=item B
>> +
>> +Don't use any device model. This requires a kernel capable of booting
>> +without emulated devices. Default is 0.
>> +
>>  =item B
>>  
>>  Number of megabytes to set aside for shadowing guest pagetable pages
>> @@ -1966,7 +1971,7 @@ This device-model is still the default for NetBSD dom0.
>>  =item B
>>  
>>  Don't use any device model. This requires a kernel capable of booting
>> -without emulated devices.
>> +without emulated devices. This is a synonym for L option above.
>>  
>>  =back
>>  
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 66327dc..aa591cd 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1817,6 +1817,12 @@ skip_usbdev:
>>  break;
>>  }
>>  
>> +if (c_info->type == LIBXL_DOMAIN_TYPE_HVM &&
> Hm, this will mean that the user needs to specify:
>
> builder="hvm"
> pvh=1
>
> Or else the option is not going to be parsed.

Right. I've always had builder directive in my config file so I didn't
think of that. I'll remove the check.

-boris

>
>> +!xlu_cfg_get_long(config, "pvh", &l, 0) && l) {
>> +b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_NONE;
>> +goto skip_device_model;
>> +}
>> +
> Roger.


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