Re: [Xen-devel] broken build: 448c03b3cbe14873ee637755a29ea26ee7ca9ef9

[Juergen Gross]

On 19/03/18 20:04, Doug Goldstein wrote:
> commit 448c03b3cbe14873ee637755a29ea26ee7ca9ef9
> Author: Juergen Gross 
> Date:   Mon Feb 26 09:46:12 2018 +0100
> 
> This commit breaks the build of qemu-xen-traditional for:
> 
> Ubuntu 14.04: https://gitlab.com/cardoe/xen/-/jobs/58266170
> Ubuntu 16.04: https://gitlab.com/cardoe/xen/-/jobs/58266174
> 
> A short snippet of the failure is:
> 
>   ARi386-dm/libqemu.a
>   LINK  i386-dm/qemu-dm
> /builds/cardoe/xen/tools/../tools/xenstore/libxenstore.so: undefined
> reference to `dlsym'
> collect2: error: ld returned 1 exit status
> make[5]: *** [qemu-dm] Error 1
> make[5]: Leaving directory
> `/builds/cardoe/xen/tools/qemu-xen-traditional-dir-remote/i386-dm'
> make[4]: *** [subdir-i386-dm] Error 2
> make[4]: Leaving directory
> `/builds/cardoe/xen/tools/qemu-xen-traditional-dir-remote'
> make[3]: *** [subdir-all-qemu-xen-traditional-dir] Error 2
> make[3]: Leaving directory `/builds/cardoe/xen/tools'
> make[2]: *** [subdirs-install] Error 2
> make[2]: Leaving directory `/builds/cardoe/xen/tools'
> make[1]: *** [install] Error 2
> make[1]: Leaving directory `/builds/cardoe/xen/tools'
> make: *** [install-tools] Error 2
> 

Did you have commit c9bd8a73656d7435b1055ee8825823aee995993e ?


Juergen

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

[Xen-devel] [linux-4.1 test] 120916: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-arm64-pvops 6 kernel-build fail REGR. vs. 118294
 build-i386-pvops  6 kernel-build fail REGR. vs. 118294

Tests which are failing intermittently (not blocking):
 test-armhf-armhf-xl-arndale 5 host-ping-check-native fail in 120846 pass in 
120916
 test-armhf-armhf-xl-credit2  16 guest-start/debian.repeat  fail pass in 120846

Tests which did not succeed, but are not blocking:
 test-amd64-i386-examine   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-win10-i386  1 build-check(1)  blocked n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64-xsm  1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked  n/a
 test-arm64-arm64-libvirt-xsm  1 build-check(1)   blocked  n/a
 test-arm64-arm64-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-libvirt-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemuu-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-rumprun-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)  blocked n/a
 test-arm64-arm64-examine  1 build-check(1)   blocked  n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win10-i386  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-xsm1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a
 test-arm64-arm64-xl-xsm   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-ws16-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-pvshim 1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked 
n/a
 test-arm64-arm64-xl   1 build-check(1)   blocked  n/a
 test-amd64-i386-qemut-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-qemut-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 118294
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 118294
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 118294
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 118294
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-xsm  

Re: [Xen-devel] [PATCH v1 01/15] arm64: cputype: Add MIDR values for Cavium ThunderX1 CPU family

[Julien Grall]

Hi Manish,

On 03/16/2018 11:58 AM, Manish Jaggi wrote:

Add MIDR values for Cavium ThunderX1 SoC family.


Did you intend to use a : instead of .?


ThunderX1, 81XX, 83XX.

Signed-off-by: Manish Jaggi 

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 65eb1071e1..62ad244785 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -43,15 +43,24 @@
  })
  
  #define ARM_CPU_IMP_ARM 0x41

+#define ARM_CPU_IMP_CAVIUM  0x43
  
  #define ARM_CPU_PART_CORTEX_A15 0xC0F

  #define ARM_CPU_PART_CORTEX_A53 0xD03
  #define ARM_CPU_PART_CORTEX_A57 0xD07
  
+#define CAVIUM_CPU_PART_THUNDERX  0x0A1

+#define CAVIUM_CPU_PART_THUNDERX_81XX 0x0A2
+#define CAVIUM_CPU_PART_THUNDERX_83XX 0x0A3
+
  #define MIDR_CORTEX_A15 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, 
ARM_CPU_PART_CORTEX_A15)
  #define MIDR_CORTEX_A53 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, 
ARM_CPU_PART_CORTEX_A53)
  #define MIDR_CORTEX_A57 MIDR_CPU_MODEL(ARM_CPU_IMP_ARM, 
ARM_CPU_PART_CORTEX_A57)
  
+#define MIDR_THUNDERX  MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, CAVIUM_CPU_PART_THUNDERX)

+#define MIDR_THUNDERX_81XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, 
CAVIUM_CPU_PART_THUNDERX_81XX)
+#define MIDR_THUNDERX_83XX MIDR_CPU_MODEL(ARM_CPU_IMP_CAVIUM, 
CAVIUM_CPU_PART_THUNDERX_83XX)
+
  /* MPIDR Multiprocessor Affinity Register */
  #define _MPIDR_UP   (30)
  #define MPIDR_UP(_AC(1,U) << _MPIDR_UP)



--
Julien Grall

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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Jan Beulich]

>>> On 19.03.18 at 17:57,  wrote:
>>  -Original Message-
> [snip]
>> >> How are you making sure this is a mapping that was established via
>> >> the map op? Without that this can be (ab)used to ...
>> >>
>> >> > +put_page(page);
>> >>
>> >> ... underflow the refcount of a page.
>> >>
>> >
>> > Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820
>> reserved
>> > areas) are mapped through the IOMMU or this could indeed be abused.
>> 
>> Now I'm confused - then you don't need to deal with struct page_info
>> and page references at all. Nor would you need to call
>> get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
>> would the interface be if you couldn't map any RAM?
>> 
> 
> Sorry to confuse...
> 
> What I meant was that safety (against underflow) is predicated on 
> iommu_lookup_page() failing if the mapping was not established through an 
> iommu op hypercall. So, the only things that should be valid in the iommu 
> (and hence that iommu_lookup_page() would succeed for) at the point where the 
> guest starts to boot must all fall within reserved regions, so thay they are 
> ruled out by the earlier check.

Ah, I see. What I don't see is how you want to arrange for that.
The tool stack wouldn't know ahead of time whether the guest
wants to use the PV IOMMU interfaces, would it? IOW rather than
guaranteeing said state at start of guest, shouldn't you blow away
all non-special mappings the first time a PV IOMMU request is made?

Jan


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

Re: [Xen-devel] [PATCH v1 00/15] arm64: Mediate access to GICv3 sysregs at EL2

[Julien Grall]

On 03/16/2018 11:58 AM, Manish Jaggi wrote:

This patchset is a Xen port of Marc's patchset.
arm64: KVM: Mediate access to GICv3 sysregs at EL2 [1]

The current RFC patchset is a subset of [1], as it handleing only Group1 traps
as a PoC. Most of the trap code is added in vsysreg.c. Trap handler function is 
kept
independent of the usual guest trap handling code.
Looking for feedback on this approach.


This cover letter does not seem to match the series. Please update it on 
every time you send a series.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v1 03/15] arm: Placeholder for handling Group0/1 traps for Cavium Erratum 30115

[Julien Grall]

Hi Manish,

On 03/16/2018 11:58 AM, Manish Jaggi wrote:

Since this is a SoC errata and trapping of certain group1 registers
should not affect the normal flow. A new file vgic-v3-sr.c is added.


You trap both group1 and group0. But your first sentence is a bit 
difficult to understand. How about:


"The errata will require to emulate the GIC virtual CPU interface in 
Xen. Because the hypervisor will update its internal state of the vGIC, 
we want to avoid messing up with it. So the errata is handled separately 
from the rest of the hypervisor."




Function vgic_v3_handle_cpuif_access is called from do_trap_guest_sync
if ARM64_WORKAROUND_CAVIUM_30115 capability is found.

A flag skip_hyp_tail is introduced in struct cpu_info. This flag
specifies


The wrapping of the commit message looks wrong.


that leave_hypervisor_tail not to be called when handling group1 traps
under this errata.


No the flag is much more generic than what you claim here. The flag is 
used to skip the hypervisor_tail when enter_hypervisor_head. Your trap 
handling is one of the user.


Technically this is 2 distinct features, one is using the other and 
should be in separate patches. I am ok to keep in a single patch, but 
you should get the commit message right.



 enter_hypervisor_head is not invoked when workaround
30115 is in place. enter_hypervisor_head and leave_hypervisor_tail are
invoked in sync, if one is not called other one should be skipped,
otherwise guest vGIC state be out-of-date.

Signed-off-by: Manish Jaggi 

diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
index 718fe44455..02cc115239 100644
--- a/xen/arch/arm/arm64/Makefile
+++ b/xen/arch/arm/arm64/Makefile
@@ -11,3 +11,4 @@ obj-y += smpboot.o
  obj-y += traps.o
  obj-y += vfp.o
  obj-y += vsysreg.o
+obj-$(CONFIG_CAVIUM_ERRATUM_30115) += vgic-v3-sr.o
diff --git a/xen/arch/arm/arm64/vgic-v3-sr.c b/xen/arch/arm/arm64/vgic-v3-sr.c
new file mode 100644
index 00..56b02fd45b
--- /dev/null
+++ b/xen/arch/arm/arm64/vgic-v3-sr.c
@@ -0,0 +1,56 @@
+/*
+ * xen/arch/arm/arm64/vgic-v3-sr.c
+ *
+ * Code to handle Cavium Erratum 30115
+ *
+ * Manish Jaggi 
+ * Copyright (c) 2018 Cavium.
+ *
+ * Ths program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr 
hsr)
+{
+bool ret = true;
+
+/* Disabling interrupts to prevent change in guest state */
+local_irq_disable();
+if ( hsr.ec != HSR_EC_SYSREG )
+{
+ret = false;
+goto end;
+}
+
+switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
+{
+default:
+ret = false;
+break;
+}
+end:
+local_irq_enable();
+
+return ret;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..25778018fb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,27 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
  {
  const union hsr hsr = { .bits = regs->hsr };
  
+if ( check_workaround_30115() )

+{
+bool ret;


newline here. But ...


+ret  = vgic_v3_handle_cpuif_access(regs, hsr);


I don't think the separate variable is required.

You could just do if (vgic_v3_handle_cpuif_access(regs, hsr))...


+if ( ret )
+{
+   /* if true, g0/g1 vgic register trap is emulated for errata
+* so rest of handling of do_trap_guest_sync is not required.
+*/


Please follow Xen coding style comments:

/*
 * If ...

The if should also have the first letter uppercase and it would be 
better to say "group" rather than "g". It is much clearer.



+advance_pc(regs, hsr);
+/*
+ * enter_hypervisor_head is not invoked when workaround 30115
+ * is in place. enter_hypervisor_head and leave_hypervisor_tail
+ * are invoked in sync, if one is not called other one should be
+ * skipped, otherwise guest vGIC state be out-of-date.
+ */
+get_cpu_info()->skip_hyp_tail = true;
+return;
+}
+}
+
  enter_hypervisor_head(regs);
  
  switch (hsr.ec) {

@@ -2295,6 +2316,16 @@ void do_trap_fiq(struct cpu_user_regs *regs)
  
  void leave_hypervisor_tail(void)

  {
+/*
+ * if skip_hyp_tail is set simply retrun;
+ */


No can use the single line comment 

Re: [Xen-devel] [PATCH] x86/xen: Delay get_cpu_cap until stack canary is established

[Juergen Gross]

On 19/03/18 23:22, Boris Ostrovsky wrote:
> On 03/19/2018 12:58 PM, Jason Andryuk wrote:
>> Commit 2cc42bac1c79 ("x86-64/Xen: eliminate W+X mappings") introduced a
>> call to get_cpu_cap, which is fstack-protected.  This is works on x86-64
> 
> s/This is works/This works/
> 
> Reviewed-by: Boris Ostrovsky 
> 
> Do we still need 4f277295e54?

I'd rather keep it in order to avoid nasty problems in case something
changes. After all we are trying to do an initialization in C code
which should be done in assembly before entering the C part. Doing this
properly for 32-bit pv-kernels would be rather difficult, but this is no
reason to drop the correct solution for the 64-bit case.


Juergen

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

Re: [Xen-devel] [PATCH v1 02/15] arm64: Add config for Cavium Thunder erratum 30115

[Julien Grall]

Hi,

On 03/16/2018 11:58 AM, Manish Jaggi wrote:

Some Cavium Thunder CPUs suffer a problem where a Xen guest may
inadvertently cause the host kernel to quit receiving interrupts.
This patch adds CONFIG_CAVIUM_ERRATUM_30115. Subsequent patches will
provide workaround.

Signed-off-by: Manish Jaggi 

diff --git a/docs/misc/arm/silicon-errata.txt b/docs/misc/arm/silicon-errata.txt
index c9854c39f4..a2546d4bb5 100644
--- a/docs/misc/arm/silicon-errata.txt
+++ b/docs/misc/arm/silicon-errata.txt
@@ -48,3 +48,4 @@ stable hypervisors.
  | ARM| Cortex-A57  | #852523 | N/A
 |
  | ARM| Cortex-A57  | #832075 | ARM64_ERRATUM_832075   
 |
  | ARM| Cortex-A57  | #834220 | ARM64_ERRATUM_834220   
 |
+| CAVIUM | ThunderX1   | #30115  | CAVIUM_ERRATUM_30115
|
diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index f58019d6ed..762b761f7d 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -169,6 +169,17 @@ config ARM64_ERRATUM_834220
  
  	  If unsure, say Y.
  
+config CAVIUM_ERRATUM_30115

+   bool "Cavium Erratum 30115"
+   depends on HAS_GICV3
+   help
+ On ThunderX T88 pass 1.x through 2.2, T81 pass 1.0 through
+ 1.2, and T83 Pass 1.0, guest execution may disable
+ interrupts in host. Trapping both GICv3 group-0 and group-1
+ accesses sidesteps the issue.
+
+ If unsure, say Y.
+
  endmenu
  
  source "common/Kconfig"

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index fe9e9facbe..d49698f785 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -56,6 +56,27 @@ static const struct arm_cpu_capabilities arm_errata[] = {
  MIDR_RANGE(MIDR_CORTEX_A57, 0x00,
 (1 << MIDR_VARIANT_SHIFT) | 2),
  },
+#endif
+#ifdef CONFIG_CAVIUM_ERRATUM_30115
+{
+/* Cavium ThunderX, T88 pass 1.x - 2.2 */


This is quite odd. You specify a number in the commit message here, but 
in the previous one you just say "thunderx1". Can you please try to 
agree on the name? Like the right naming is MIDR_THUNDERX_88.



+.desc = "Cavium erratum 30115",
+.capability = ARM64_WORKAROUND_CAVIUM_30115,
+MIDR_RANGE(MIDR_THUNDERX, 0x00,
+   (1 << MIDR_VARIANT_SHIFT) | 2),
+},
+{
+/* Cavium ThunderX, T81 pass 1.0 - 1.2 */
+.desc = "Cavium erratum 30115",
+.capability = ARM64_WORKAROUND_CAVIUM_30115,
+MIDR_RANGE(MIDR_THUNDERX_81XX, 0x00, 0x02),
+},
+{
+/* Cavium ThunderX, T83 pass 1.0 */
+.desc = "Cavium erratum 30115",
+.capability = ARM64_WORKAROUND_CAVIUM_30115,
+MIDR_RANGE(MIDR_THUNDERX_83XX, 0x00, 0x00),
+},
  #endif
  {},
  };
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
index 8b158429c7..521f03521b 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -41,6 +41,7 @@ static inline bool check_workaround_##erratum(void)   
  \
  
  CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)

  CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
+CHECK_WORKAROUND_HELPER(30115, ARM64_WORKAROUND_CAVIUM_30115, CONFIG_ARM_64)


Please add cavium_ in the erratum name. So it is easy to know where the 
erratum is from.


  
  #undef CHECK_WORKAROUND_HELPER
  
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h

index f00b6dbd39..d409636bf0 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -42,8 +42,9 @@
  #define LIVEPATCH_FEATURE   4
  #define SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT 5
  #define SKIP_CTXT_SWITCH_SERROR_SYNC 6
+#define ARM64_WORKAROUND_CAVIUM_30115 7
  
-#define ARM_NCAPS   7

+#define ARM_NCAPS   8
  
  #ifndef __ASSEMBLY__
  



Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH RESEND v2 0/2] drm/xen-front: Add support for Xen PV display frontend

[Oleksandr Andrushchenko]

On 03/20/2018 01:23 AM, Boris Ostrovsky wrote:

On 03/13/2018 12:21 PM, Oleksandr Andrushchenko wrote:

From: Oleksandr Andrushchenko 

Hello!

Resending with all the patches squashed on Daniel's request.

Which of the two series are we supposed to review? The 8-patch one or
this? (I hope it's the former)

It was requested by Daniel Vetter that I squash all the series
into a single patch, so this squashed single patch is on review now
([PATCH RESEND v2 0/2] drm/xen-front: Add support for Xen PV display 
frontend)

I also discussed that with Juergen on IRC and he is ok with 3k LOC patch

-boris




Thank you,
Oleksandr

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

Re: [Xen-devel] [RFC PATCH 07/12] hvmloader: allocate MMCONFIG area in the MMIO hole + minor code refactoring

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 05:49:22AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 15:58:02 +
> Roger Pau Monné  wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:52AM +1000, Alexey Gerasimenko wrote:
> >> Much like normal PCI BARs or other chipset-specific memory-mapped
> >> resources, MMCONFIG area needs space in MMIO hole, so we must
> >> allocate it manually.
> >> 
> >> The actual MMCONFIG size depends on a number of PCI buses available
> >> which should be covered by ECAM. Possible options are 64MB, 128MB
> >> and 256MB. As we are limited to the bus 0 currently, thus using
> >> lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in
> >> hvmloader/config.h. When multiple PCI buses support for Xen will be
> >> implemented, PCI_MAX_MCFG_BUSES may be changed to calculation of the
> >> number of buses according to results of the PCI devices enumeration.
> >> 
> >> The way to allocate MMCONFIG range in MMIO hole is similar to how
> >> other PCI BARs are allocated. The patch extends 'bars' structure to
> >> make it universal for any arbitrary BAR type -- either IO, MMIO, ROM
> >> or a chipset-specific resource.  
> >
> >I'm not sure this is fully correct. The IOREQ interface can
> >differentiate PCI devices and forward config space accesses to
> >different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change you
> >will forward all MCFG accesses to QEMU, which will likely be wrong if
> >there are multiple PCI-device emulators for the same domain.
> >
> >Ie: AFAICT Xen needs to know about the MCFG emulation and detect
> >accesses to it in order to forward them to the right emulators.
> >
> >Adding Paul who knows more about all this.
> 
> In which use cases multiple PCI-device emulators are used for a single
> HVM domain? Is it a proprietary setup?

Likely. I think XenGT might be using it. It's a feature of the IOREQ
implementation in Xen.

Traditional PCI config space accesses are not IO port space accesses.
The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ
servers can register devices they would like to receive configuration
space accesses for. QEMU is already making use of this, see for
example xen_map_pcidev in the QEMU code.

By treating MCFG accesses as MMIO you are bypassing the IOREQ PCI
layer, and thus a IOREQ server could register a PCI device and only
receive PCI configuration accesses from the IO port space, while MCFG
accesses would be forwarded somewhere else.

I think you need to make the IOREQ code aware of the MCFG area and
XEN_DMOP_IO_RANGE_PCI needs to forward both IO space and MCFG accesses
to the right IOREQ server.

> I assume it is somehow related to this code in xen-hvm.c:
> /* Fake a write to port 0xCF8 so that
>  * the config space access will target the
>  * correct device model.
>  */
> val = (1u << 31) | ((req->addr & 0x0f00) <...>
> do_outp(0xcf8, 4, val);
> if yes, similar thing can be made for IOREQ_TYPE_COPY accesses to
> the emulated MMCONFIG if needed.

I have to admit I don't know that much about QEMU, and I have no idea
what the chunk above is supposed to accomplish.

> 
> In HVM+QEMU case we are not limited to merely passed through devices,
> most of the observable PCI config space devices belong to one particular
> QEMU instance. This dictates the overall emulated MMCONFIG layout
> for a domain which should be in sync to what QEMU emulates via CF8h/CFCh
> accesses... and between multiple device model instances (if there are
> any, still not sure what multiple PCI-device emulators you mentioned
> really are).

In newer versions of Xen (>4.5 IIRC, Paul knows more), QEMU doesn't
directly trap accesses to the 0xcf8/0xcfc IO ports, it's Xen instead
the one that detects and decodes such accesses, and then forwards them
to the IOREQ server that has been registered to handle them.

You cannot simply forward all MCFG accesses to QEMU as MMIO accesses,
Xen needs to decode them and they need to be handled as
IOREQ_TYPE_PCI_CONFIG requests, not IOREQ_TYPE_COPY IMO.

> 
> Basically, we have an emulated MMCONFIG area of 64/128/256MB size in
> the MMIO hole of the guest HVM domain. (BTW, this area itself can be
> considered a feature of the chipset the device model emulates.)
> It can be relocated to some other place in MMIO hole, this means that
> QEMU will trap accesses to the specific to the emulated chipset
> PCIEXBAR register and will issue same MMIO unmap/map calls as for
> any normal emulated MMIO range.
> 
> On the other hand, it won't be easy to provide emulated MMCONFIG
> translation into IOREQ_TYPE_PCI_CONFIG from Xen side. Xen should know
> current emulated MMCONFIG area position and size in order to translate
> (or not) accesses to it into corresponding BDF/reg pair (+whether that
> area is enabled for decoding or not). This will likely require to
> introduce new hypercall(s).

Yes, you will have to introduce new hypercalls to 

[Xen-devel] TLB flushing

[Juergen Gross]

While hunting a strange bug in my PCID patch series hinting at some
TLB invalidation problem I discovered a piece of code looking rather
fishy to me.

Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
of FLUSH_TLB_GLOBAL?

While not being a problem in current code as both will flush all TLB
entries my series will change that by using invpcid to flush only the
non-global entries if FLUSH_TLB_GLOBAL wasn't set.

I can send a patch if anyone can confirm that using FLUSH_TLB only is
wrong.


Juergen

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

Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 07:20:53AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 17:49:09 +
> Roger Pau Monné  wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote:
> >> This patch extends hvmloader_acpi_build_tables() with code which
> >> detects if MMCONFIG is available -- i.e. initialized and enabled
> >> (+we're running on Q35), obtains its base address and size and asks
> >> libacpi to build MCFG table for it via setting the flag
> >> ACPI_HAS_MCFG in a manner similar to other optional ACPI tables
> >> building.
> >> 
> >> Signed-off-by: Alexey Gerasimenko 
> >> ---
> >>  tools/firmware/hvmloader/util.c | 70
> >> + 1 file changed, 70
> >> insertions(+)
> >> 
> >> diff --git a/tools/firmware/hvmloader/util.c
> >> b/tools/firmware/hvmloader/util.c index d8db9e3c8e..c6fc81d52a 100644
> >> --- a/tools/firmware/hvmloader/util.c
> >> +++ b/tools/firmware/hvmloader/util.c
> >> @@ -782,6 +782,69 @@ int get_pc_machine_type(void)
> >>  return machine_type;
> >>  }
> >>  
> >> +#define PCIEXBAR_ADDR_MASK_64MB (~((1ULL << 26) - 1))
> >> +#define PCIEXBAR_ADDR_MASK_128MB(~((1ULL << 27) - 1))
> >> +#define PCIEXBAR_ADDR_MASK_256MB(~((1ULL << 28) - 1))
> >> +#define PCIEXBAR_LENGTH_BITS(reg)   (((reg) >> 1) & 3)
> >> +#define PCIEXBAREN  1  
> >
> >PCIEXBAR_ENABLE maybe?
> 
> PCIEXBAREN is just an official name of this bit from the
> Intel datasheet. :) OK, will rename it to PCIEXBAR_ENABLE.

Oh, if that's the name on the spec then leave it as-is. It's always
best to be able to search directly on the spec.

> >> +
> >> +static uint64_t mmconfig_get_base(void)
> >> +{
> >> +uint64_t base;
> >> +uint32_t reg = pci_readl(PCI_MCH_DEVFN, PCI_MCH_PCIEXBAR);
> >> +
> >> +base = reg | (uint64_t) pci_readl(PCI_MCH_DEVFN,
> >> PCI_MCH_PCIEXBAR+4) << 32;  
> >
> >Please add parentheses in the above expression.
> 
> Agree, parentheses will make the op priority clearer.
> 
> >> +
> >> +switch (PCIEXBAR_LENGTH_BITS(reg))
> >> +{
> >> +case 0:
> >> +base &= PCIEXBAR_ADDR_MASK_256MB;
> >> +break;
> >> +case 1:
> >> +base &= PCIEXBAR_ADDR_MASK_128MB;
> >> +break;
> >> +case 2:
> >> +base &= PCIEXBAR_ADDR_MASK_64MB;
> >> +break;  
> >
> >Missing newlines, plus this looks like it wants to use the defines
> >introduced in patch 7 (PCIEXBAR_{64,128,256}_BUSES). Also any reason
> >this patch and patch 7 cannot be put sequentially?
> 
> I think all these #defines should find a way to pci_regs.h, it seems
> like an appropriate place for them.

Hm, pci_regs.h seems to contain the generic PCI registers. Those
should maybe live in a q35.h header, since it's very device specific
AFAICT.

> Regarding the order of hvmloader patches -- will verify this for
> the next version.
> 
> >They are very related, and in fact I'm not sure why we need to write
> >this info to the device in patch 7 and then fetch it from the device
> >here. Isn't there an easier way to pass this information? At the end
> >this is all in hvmloader.
> 
> Well, the hvmloader_acpi_build_tables() function mostly does device
> probing (using I/O instruction) and xenstore reads to collect system
> information in order to discover which ACPI_HAS_* flags it should pass
> to acpi_build_tables(), but using global variables to pass this kind of
> information for MMCONFIG will be OK too I think.

It was just a suggestion, it seems kind of cumbersome to write
something to a register and then fetch it afterwards, when it's all
done in the same binary.

> >> +case 3:  
> >
> >default:
> 
> There is '& 3' for the switch argument, but ok I guess, it's clearer
> with 'default'.
> 
> >> +BUG();  /* a reserved value encountered */
> >> +}
> >> +
> >> +return base;
> >> +}
> >> +
> >> +static uint32_t mmconfig_get_size(void)  
> >
> >unsigned int or size_t?
> 
> Using types which are common to the existing code.
> 
> size_t have almost zero use in hvmloader.

If it's available I would rather use it.

> >> +{
> >> +if (get_pc_machine_type() == MACHINE_TYPE_Q35)
> >> +{
> >> +if (mmconfig_is_enabled() && mmconfig_get_base())  
> >
> >Coding style.
> >
> >Also you can join the conditions:
> >
> >if ( get_pc_machine_type() == MACHINE_TYPE_Q35 &&
> >mmconfig_is_enabled() &&
> > mmconfig_get_base() )
> > return true;
> >
> >Looking at this, is it actually a valid state to have
> >mmconfig_is_enabled() == true and mmconfig_get_base() == 0?
> 
> Yes, in theory we can have either PCIEXBAREN=0 and a valid PCIEXBAR
> base, or vice versa.
> Of course normally we should not encounter a situation where base=0 and
> PCIEXBAREN=1, just covering here possible cases which the register
> format allows.

But those registers are set by hvmloader, and I don't think hvmloader
will ever set PCIEXBAREN == 1 and PCIEXBAR base == 0?

> Regarding check merging 

Re: [Xen-devel] [RFC PATCH 10/12] libacpi: build ACPI MCFG table if requested

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 07:46:04AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 17:33:34 +
> Roger Pau Monné  wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:55AM +1000, Alexey Gerasimenko wrote:
> >> This adds construct_mcfg() function to libacpi which allows to build
> >> MCFG table for a given mmconfig_addr/mmconfig_len pair if the
> >> ACPI_HAS_MCFG flag was specified in acpi_config struct.
> >> 
> >> The maximum bus number is calculated from mmconfig_len using
> >> MCFG_SIZE_TO_NUM_BUSES macro (1MByte of MMIO space per bus).
> >> 
> >> Signed-off-by: Alexey Gerasimenko 
> >> ---
> >>  tools/libacpi/acpi2_0.h | 21 +
> >>  tools/libacpi/build.c   | 42
> >> ++ tools/libacpi/libacpi.h
> >> |  4  3 files changed, 67 insertions(+)
> >> 
> >> diff --git a/tools/libacpi/acpi2_0.h b/tools/libacpi/acpi2_0.h
> >> index 2619ba32db..209ad1acd3 100644
> >> --- a/tools/libacpi/acpi2_0.h
> >> +++ b/tools/libacpi/acpi2_0.h
> >> @@ -422,6 +422,25 @@ struct acpi_20_slit {
> >>  };
> >>  
> >>  /*
> >> + * PCI Express Memory Mapped Configuration Description Table
> >> + */
> >> +struct mcfg_range_entry {
> >> +uint64_t base_address;
> >> +uint16_t pci_segment;
> >> +uint8_t  start_pci_bus_num;
> >> +uint8_t  end_pci_bus_num;
> >> +uint32_t reserved;
> >> +};
> >> +
> >> +struct acpi_mcfg {
> >> +struct acpi_header header;
> >> +uint8_t reserved[8];
> >> +struct mcfg_range_entry entries[1];
> >> +};  
> >
> >I would define this as:
> >
> >struct acpi_10_mcfg {
> >struct acpi_header header;
> >uint8_t reserved[8];
> >struct acpi_10_mcfg_entry {
> >uint64_t base_address;
> >uint16_t pci_segment;
> >uint8_t  start_pci_bus;
> >uint8_t  end_pci_bus;
> >uint32_t reserved;
> >} entries[1];
> >};
> 
> Hmm, a choice of preference, but OK, will move it inside.

Note the name change also (acpi_10_mcfg). Also I think you can drop
the acpi_10_mcfg_entry name and just use an anonymous struct.

> >> +
> >> +mcfg->entries[0].base_address = config->mmconfig_addr;
> >> +mcfg->entries[0].pci_segment = 0;
> >> +mcfg->entries[0].start_pci_bus_num = 0;
> >> +mcfg->entries[0].end_pci_bus_num =
> >> +MCFG_SIZE_TO_NUM_BUSES(config->mmconfig_len) - 1;  
> >
> >Why not pass the start_bus and end_bus values in acpi_config at least?
> 
> start_pci_bus_num will be always 0.
> 
> It will be kinda ugly to pass config->mmconfig_addr along with
> config->end_pci_bus_num, baseaddr+size combo looks nicer I think.

I'm not going to insist, but ACPI doesn't really care about the size,
it just needs to know the start and end. Seems pointless to write a
value here that later libacpi needs to convert to the value it
actually needs. Also start/end buses are uint8_t, size is uint32_t.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 20 March 2018 08:12
> To: Paul Durrant 
> Cc: Andrew Cooper ; George Dunlap
> ; Ian Jackson ; Wei Liu
> ; Stefano Stabellini ; xen-
> de...@lists.xenproject.org; Tim (Xen.org) 
> Subject: RE: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and
> unmap pages, and also to flush the IOTLB
> 
> >>> On 19.03.18 at 17:57,  wrote:
> >>  -Original Message-
> > [snip]
> >> >> How are you making sure this is a mapping that was established via
> >> >> the map op? Without that this can be (ab)used to ...
> >> >>
> >> >> > +put_page(page);
> >> >>
> >> >> ... underflow the refcount of a page.
> >> >>
> >> >
> >> > Yes, I guess I need to ensure that only non-RAM (i.e. RMRR and E820
> >> reserved
> >> > areas) are mapped through the IOMMU or this could indeed be abused.
> >>
> >> Now I'm confused - then you don't need to deal with struct page_info
> >> and page references at all. Nor would you need to call
> >> get_page_from_gfn() and check p2m_is_any_ram(). Also - what use
> >> would the interface be if you couldn't map any RAM?
> >>
> >
> > Sorry to confuse...
> >
> > What I meant was that safety (against underflow) is predicated on
> > iommu_lookup_page() failing if the mapping was not established through
> an
> > iommu op hypercall. So, the only things that should be valid in the iommu
> > (and hence that iommu_lookup_page() would succeed for) at the point
> where the
> > guest starts to boot must all fall within reserved regions, so thay they are
> > ruled out by the earlier check.
> 
> Ah, I see. What I don't see is how you want to arrange for that.
> The tool stack wouldn't know ahead of time whether the guest
> wants to use the PV IOMMU interfaces, would it? IOW rather than
> guaranteeing said state at start of guest, shouldn't you blow away
> all non-special mappings the first time a PV IOMMU request is made?
> 

I suspect we want both. Kevin suggested a 'big switch' when the domain boots, 
in which I could blow away all non-reserved mappings. But, for performance 
sake, I think it would also be worth a Xen command line option to avoid 
populating the IOMMU mappings for dom0 in the first place (so when it pulls the 
'big switch' it's a no-op). Non-aware dom0s will, of course, probably fail to 
boot but whoever is setting the command line for Xen should know what their 
dom0 is capable of. As for other domains, it may be worth adding a similar 
domain create option to the toolstack but that could be done at a later date.

  Paul

> Jan


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

Re: [Xen-devel] [PATCH v3] xen/acpi: upload _PSD info for non Dom0 CPUs too

[Rafael J. Wysocki]

On Fri, Mar 16, 2018 at 2:57 PM, Joao Martins  wrote:
> On 03/15/2018 03:45 PM, Boris Ostrovsky wrote:
>> On 03/15/2018 10:22 AM, Joao Martins wrote:
>>> All uploaded PM data from non-dom0 CPUs takes the info from vCPU 0 and
>>> changing only the acpi_id. For processors which P-state coordination type
>>> is HW_ALL (0xFD) it is OK to upload bogus P-state dependency information
>>> (_PSD), because Xen will ignore any cpufreq domains created for past CPUs.
>>>
>>> Albeit for platforms which expose coordination types as SW_ANY or SW_ALL,
>>> this will have some unintended side effects. Effectively, it will look at
>>> the P-state domain existence and *if it already exists* it will skip the
>>> acpi-cpufreq initialization and thus inherit the policy from the first CPU
>>> in the cpufreq domain. This will finally lead to the original cpu not
>>> changing target freq to P0 other than the first in the domain. Which will
>>> make turbo boost not getting enabled (e.g. for 'performance' governor) for
>>> all cpus.
>>>
>>> This patch fixes that, by also evaluating _PSD when we enumerate all ACPI
>>> processors and thus always uploading the correct info to Xen. We export
>>> acpi_processor_get_psd() for that this purpose, but change signature
>>> to not assume an existent of acpi_processor given that ACPI isn't creating
>>> an acpi_processor for non-dom0 CPUs.
>>>
>>> Signed-off-by: Joao Martins 
>>
>> Reviewed-by: Boris Ostrovsky 
>>
> Thanks!
>
> I suppose what's remaining is review (or ack) from ACPI folks on the interface
> changes made to acpi_processor_get_psd().

There you go:

Acked-by: Rafael J. Wysocki 

Do you want to route this via Xen?

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

Re: [Xen-devel] [PATCH v2 02/45] ARM: Implement vcpu_kick()

[Jan Beulich]

>>> On 15.03.18 at 21:30,  wrote:
> If we change something in a vCPU that affects its runnability or
> otherwise needs the vCPU's attention, we might need to tell the scheduler
> about it.
> We are using this in one place (vIRQ injection) at the moment, but will
> need this at more places soon.
> So let's factor out this functionality, using the already existing
> vcpu_kick() prototype (used in x86 only so far), to make this available
> to the rest of the Xen code.

Having just seen this among the commits having gone in recently -
was it considered to make this a common function? The
implementations currently differ, but I'm not sure I see why that
needs to be. With x86's vcpu_kick_softirq() handler doing nothing
I could see the ARM implementation be suitable for x86, just like
I could see the x86 implementation be suitable for ARM.

Jan


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

Re: [Xen-devel] [PATCH v3] xen/acpi: upload _PSD info for non Dom0 CPUs too

[Joao Martins]

On 03/20/2018 09:41 AM, Rafael J. Wysocki wrote:
> On Fri, Mar 16, 2018 at 2:57 PM, Joao Martins  
> wrote:
>> On 03/15/2018 03:45 PM, Boris Ostrovsky wrote:
>>> On 03/15/2018 10:22 AM, Joao Martins wrote:
 All uploaded PM data from non-dom0 CPUs takes the info from vCPU 0 and
 changing only the acpi_id. For processors which P-state coordination type
 is HW_ALL (0xFD) it is OK to upload bogus P-state dependency information
 (_PSD), because Xen will ignore any cpufreq domains created for past CPUs.

 Albeit for platforms which expose coordination types as SW_ANY or SW_ALL,
 this will have some unintended side effects. Effectively, it will look at
 the P-state domain existence and *if it already exists* it will skip the
 acpi-cpufreq initialization and thus inherit the policy from the first CPU
 in the cpufreq domain. This will finally lead to the original cpu not
 changing target freq to P0 other than the first in the domain. Which will
 make turbo boost not getting enabled (e.g. for 'performance' governor) for
 all cpus.

 This patch fixes that, by also evaluating _PSD when we enumerate all ACPI
 processors and thus always uploading the correct info to Xen. We export
 acpi_processor_get_psd() for that this purpose, but change signature
 to not assume an existent of acpi_processor given that ACPI isn't creating
 an acpi_processor for non-dom0 CPUs.

 Signed-off-by: Joao Martins 
>>>
>>> Reviewed-by: Boris Ostrovsky 
>>>
>> Thanks!
>>
>> I suppose what's remaining is review (or ack) from ACPI folks on the 
>> interface
>> changes made to acpi_processor_get_psd().
> 
> There you go:
> 
> Acked-by: Rafael J. Wysocki 
>
Thank you!

> Do you want to route this via Xen?
> 
I suppose that makes sense given that the majority of the change is on xen acpi
processor. But it's ultimately Boris/Juergen call.

Joao

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

Re: [Xen-devel] [PATCH v10 07/11] vpci/bars: add handlers to map the BARs

[Roger Pau Monné]

On Mon, Mar 19, 2018 at 10:18:34AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 14:30,  wrote:
> > +static int modify_bars(const struct pci_dev *pdev, bool map, bool rom_only)
> > +{
> > +struct vpci_header *header = >vpci->header;
> > +struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> > +struct pci_dev *tmp, *dev = NULL;
> > +unsigned int i;
> > +int rc;
> > +
> > +if ( !mem )
> > +return -ENOMEM;
> > +
> > +/*
> > + * Create a rangeset that represents the current device BARs memory 
> > region
> > + * and compare it against all the currently active BAR memory regions. 
> > If
> > + * an overlap is found, subtract it from the region to be 
> > mapped/unmapped.
> > + *
> > + * First fill the rangeset with all the BARs of this device or with 
> > the ROM
> > + * BAR only, depending on whether the guest is toggling the memory 
> > decode
> > + * bit of the command register, or the enable bit of the ROM BAR 
> > register.
> > + */
> > +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
> > +{
> > +const struct vpci_bar *bar = >bars[i];
> > +
> > +if ( !MAPPABLE_BAR(bar) ||
> > + (rom_only ? bar->type != VPCI_BAR_ROM
> > +   : (bar->type == VPCI_BAR_ROM && 
> > !header->rom_enabled)) )
> > +continue;
> > +
> > +rc = rangeset_add_range(mem, PFN_DOWN(bar->addr),
> > +PFN_DOWN(bar->addr + bar->size - 1));
> > +if ( rc )
> > +{
> > +printk(XENLOG_G_WARNING
> > +   "Failed to add [%" PRI_gfn ", %" PRI_gfn "]: %d\n",
> > +   PFN_DOWN(bar->addr), PFN_DOWN(bar->addr + bar->size - 
> > 1),
> > +   rc);
> > +rangeset_destroy(mem);
> > +return rc;
> > +}
> > +}
> > +
> > +/*
> > + * Check for overlaps with other BARs. Note that only BARs that are
> > + * currently mapped (enabled) are checked for overlaps.
> > + */
> > +list_for_each_entry(tmp, >domain->arch.pdev_list, domain_list)
> > +{
> > +if ( tmp == pdev )
> > +{
> > +/*
> > + * Need to store the device so it's not constified and
> > + * maybe_defer_map can modify it in case of error.
> > + */
> > +dev = tmp;
> 
> There's no maybe_defer_map anymore.
> 
> And then I'm having a problem with this comment on const-ness:
> apply_map() only wants to hand the device to modify_decoding(),
> whose respective argument is const. defer_map() stores the
> pointer, but afaics again only for vpci_process_pending() to hand
> it on to modify_decoding(); the lock the function takes is in a
> structure pointed to from the device, so unaffected by the const.

vpci_process_pending calls vpci_remove_device which needs a
non-constified pdev, since it sets pdev->vpci = NULL.

> > + * memory has been added or removed from the p2m (because the
> > + * actual p2m changes are deferred in maybe_defer_map) and the 
> > ROM
> > + * enable bit has not been changed, so leave everything as-is,
> > + * hoping the guest will realize and try again.
> > + */
> > +return;
> > +}
> > +else
> > +{
> > +header->rom_enabled = new_enabled;
> > +pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
> > +}
> > +
> > +if ( !new_enabled )
> > +rom->addr = val & PCI_ROM_ADDRESS_MASK;
> 
> I'm struggling to understand this update, not the least seeing the
> other update further up.

I've added some comments now, but the point is that when mapping the
ROM BAR we should update the addr field first, so that the correct p2m
mappings are established

In the unmap case however (!new_enabled) the addr needs to be updated
after calling modify_bars, or else the wrong address might be
unmapped.

Does this address your concern?

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 05:03:09AM -0600, Jan Beulich wrote:
> >>> On 16.03.18 at 14:30,  wrote:
> > +int vpci_msix_arch_print(const struct vpci_msix *msix)
> > +{
> > +unsigned int i;
> > +
> > +for ( i = 0; i < msix->max_entries; i++ )
> > +{
> > +const struct vpci_msix_entry *entry = >entries[i];
> > +
> > +printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u 
> > pirq: %d\n",
> > +   i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
> > +   entry->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> > +   entry->data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> > +   entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> > +   entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
> > +   entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : 
> > "fixed",
> > +   MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
> > +   entry->masked, entry->arch.pirq);
> > +if ( i && !(i % 64) )
> > +{
> > +struct pci_dev *pdev = msix->pdev;
> > +
> > +spin_unlock(>pdev->vpci->lock);
> > +process_pending_softirqs();
> > +/* NB: we assume that pdev cannot go away for an alive domain. 
> > */
> > +if ( !pdev->vpci || !spin_trylock(>vpci->lock) )
> > +return -EBUSY;
> > +msix = pdev->vpci->msix;
> 
> I disagree with resuming with a potentially changed msix here: This
> can only lead to confusion of the consumer of the produced output.

OK, I will check that the previous msix pointer matches the current
one.

> > @@ -231,6 +232,23 @@ static int modify_bars(const struct pci_dev *pdev, 
> > bool map, bool rom_only)
> >  }
> >  }
> >  
> > +/* Remove any MSIX regions if present. */
> > +for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> > +{
> > +paddr_t start = vmsix_table_addr(pdev->vpci, i);
> > +paddr_t end = start + vmsix_table_size(pdev->vpci, i) - 1;
> > +
> > +rc = rangeset_remove_range(mem, PFN_DOWN(start), PFN_DOWN(end));
> > +if ( rc )
> > +{
> > +printk(XENLOG_G_WARNING
> > +   "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn 
> > "]: %d\n",
> > +   PFN_DOWN(start), PFN_DOWN(end), rc);
> 
> In cases like this (where you don't use plain start/end anywhere,
> but you do use the same calculation on them twice each), it's
> certainly more efficient for the local variables to be frame numbers
> right away.

Right, I've fixed it and also changed the printf formatters to lu, I
guess at some point in the series I used to print gfn values.

> 
> Considering that I didn't notice this earlier, I won't insist on the
> latter change to be made, i.e. with at least the former issue
> addressed
> Reviewed-by: Jan Beulich 

Thanks. FWIW I've fixed both your comments.

Since you only had comments on patch 7 and 11 and there's the extra
fix for the test harness, should I just send those and provide you
with a git branch that contains the rest?

I can also wait if you want to commit the start of the series
(provided I get all the relevant Acks) and rebase on top of that.

Roger.

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

Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 11:43:00AM +, Roger Pau Monné wrote:
> On Tue, Mar 20, 2018 at 05:03:09AM -0600, Jan Beulich wrote:
> > >>> On 16.03.18 at 14:30,  wrote:
> > > +printk(XENLOG_G_WARNING
> > > +   "Failed to remove MSIX table [%" PRI_gfn ", %" 
> > > PRI_gfn "]: %d\n",
> > > +   PFN_DOWN(start), PFN_DOWN(end), rc);
> > 
> > In cases like this (where you don't use plain start/end anywhere,
> > but you do use the same calculation on them twice each), it's
> > certainly more efficient for the local variables to be frame numbers
> > right away.
> 
> Right, I've fixed it and also changed the printf formatters to lu, I
> guess at some point in the series I used to print gfn values.

Meant lx not lu...

Sorry.

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

Re: [Xen-devel] [PATCH RESEND v2 1/2] drm/xen-front: Add support for Xen PV display frontend

[Oleksandr Andrushchenko]

On 03/19/2018 05:28 PM, Daniel Vetter wrote:

On Mon, Mar 19, 2018 at 3:19 PM, Oleksandr Andrushchenko
 wrote:

On 03/19/2018 03:51 PM, Daniel Vetter wrote:

On Fri, Mar 16, 2018 at 12:52:09PM +0200, Oleksandr Andrushchenko wrote:

Hi, Daniel!
Sorry, if I strip the patch too much below.

On 03/16/2018 10:23 AM, Daniel Vetter wrote:

S-o-b line went missing here :-)

will restore it back ;)

I've read through it, 2 actual review comments (around hot-unplug and
around the error recovery for failed flips), a few bikesheds, but looks
all reasonable to me. And much easier to read as one big patch (it's
just
3k).

One more thing I'd do as a follow-up (don't rewrite everything, this is
close to merge, better to get it in first): You have a lot of
indirections
and function calls across sources files. That's kinda ok if you have a
huge driver with 100+k lines of code where you have to split things up.
But for a small driver like yours here it's a bit overkill.

will review and try to rework after the driver is in

I'll probably merge xen_drm_front_drv.c and xen_drm_front.c now as
anyway I have to re-work driver unloading, e.g. "fishy" code below.

Personally I'd merge at least the xen backend stuff into the
corresponding
kms code, but that's up to you.

I prefer to have it in smaller chunks and all related code at
one place, so it is easier to maintain. That is why I didn't
plumb frontend <-> backend code right into the KMS code.

And as mentioned, if you decide to do
that, a follow-up patch (once this has merged) is perfectly fine.

Ok, after the merge

If you prefer your current layout, then pls keep it. Bikeshed = personal
style nit, feel free to ignore if you like stuff differently. In the end
it's your driver, not mine, and I can easily navigate the current code
(with a few extra jumps).

Some of the indirections will be removed by merging
xen_drm_front_drv.c and xen_drm_front.c. Are these what you
mean or is there anything else?


-Daniel


-Daniel


+int xen_drm_front_dbuf_create_from_sgt(struct xen_drm_front_info
*front_info,
+   uint64_t dbuf_cookie, uint32_t width, uint32_t height,
+   uint32_t bpp, uint64_t size, struct sg_table *sgt)
+{
+   return be_dbuf_create_int(front_info, dbuf_cookie, width,
height,
+   bpp, size, NULL, sgt);
+}
+
+int xen_drm_front_dbuf_create_from_pages(struct xen_drm_front_info
*front_info,
+   uint64_t dbuf_cookie, uint32_t width, uint32_t height,
+   uint32_t bpp, uint64_t size, struct page **pages)
+{
+   return be_dbuf_create_int(front_info, dbuf_cookie, width,
height,
+   bpp, size, pages, NULL);
+}

The above two wrappers seem a bit much, just to set sgt = NULL or pages
=
NULL in one of them. I'd drop them, but that's a bikeshed so feel free
to
ignore.

I had that the way you say in some of the previous implementations,
but finally decided to have these dummy wrappers: seems
to be cleaner this way

+static void displback_disconnect(struct xen_drm_front_info
*front_info)
+{
+   bool removed = true;
+
+   if (front_info->drm_pdev) {
+   if (xen_drm_front_drv_is_used(front_info->drm_pdev)) {
+   DRM_WARN("DRM driver still in use, deferring
removal\n");
+   removed = false;
+   } else
+   xen_drv_remove_internal(front_info);

Ok this logic here is fishy, since you're open-coding the drm unplug
infrastructure, but slightly differently and slightyl racy. If you have
a
driver where your underlying "hw" (well it's virtual here, but same
idea)
can disappear any time while userspace is still using the drm driver,
you
need to use the drm_dev_unplug() function and related code.
drm_dev_unplug() works like drm_dev_unregister, except for the hotplug
case.

Then you also have to guard all the driver entry points where you do
access the backchannel using drm_dev_is_unplugged() (I've seen a few of
those already). Then you can rip out all the logic here and the
xen_drm_front_drv_is_used() helper.

Will rework it with drm_dev_unplug, thank you

I thought there's some patches from Noralf in-flight that improved the
docs on this, I need to check

Yes, I will definitely use those as soon as they are available.
But at the moment let me clarify a bit on the use-cases for driver
unplugging and backend disconnection.

The backend, by disconnecting, expects full DRM driver teardown, because,
for example, it might need to replace current frontend’s configuration
completely or stop supporting para-virtualized display for some reason.

This means that once I have displback_disconnected callback (on XenBus state
change) I am trying to unregister and remove the DRM driver which seems to
be
not possible if I have relevant code in DRM callbacks (e.g. I cannot try
removing
driver from driver's callback).

So, even if I add drm_dev_unplug (which anyway seems to be the right thing)
I’ll have to have 

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

Are you sure? I just tested and that is not the case with
either gcc or clang.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).

This is exactly what we do.

> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.

Since we do not use -iquote, "" just adds the current directory.


> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

Using <> we avoid the problem completely and create slightly
less work for the preprocessor.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:
> 
> commit d8e39b70625d4ba1e998439d1a077b4b978930e7
> Author: Markus Armbruster 
> Date:   Thu Feb 1 12:18:28 2018 +0100
> 
> Use #include "..." for our own headers, <...> for others
> 
> System headers should be included with <...>, our own headers with
> "...".  Offenders tracked down with an ugly, brittle and probably
> buggy Perl script.  Previous iteration was commit a9c94277f0.
> 
> Delete inclusions of "string.h" and "strings.h" instead of fixing them
> to  and , because we always include these via
> osdep.h.
> 
> Put the cleaned up system header includes first.
> 
> While there, separate #include from file comment with exactly one
> blank line.
> 
> commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
> Author: Markus Armbruster 
> Date:   Wed Jun 22 19:11:19 2016 +0200
> 
> Use #include "..." for our own headers, <...> for others
> 
> Tracked down with an ugly, brittle and probably buggy Perl script.
> 
> Also move includes converted to <...> up so they get included before
> ours where that's obviously okay.
> 
> Thanks,
> Laurent

I suspect we previously actually did have headers in the
same directory as source so it was somewhat helpful.
They all have been moved out to include now.

-- 
MST

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

Re: [Xen-devel] [PATCH] xsm:schedop: introduce vcpuinfo permissions verification

[Andrii Anisov]

Dear All,

Any comments for the patch?

It addressed the issue mentioned here: 
https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00452.html



--

*Andrii Anisov*



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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > QEMU coding style at the moment asks for all non-system
> > > > include files to be used with #include "foo.h".
> > > > However this rule actually does not make sense and
> > > > creates issues for when the included file is generated.
> > > 
> > > If you change that, we can have issue when a system include has the same
> > > name as our local include. With "", system header are taken first.
> > 
> > > > In C, include "file" means look in current directory,
> > > > then on include search path. Current directory here
> > > > means the source file directory.
> > > > By comparison include  means look on include search path.
> > > 
> > > Not exactly, there is the notion of "system header" too.
> > > 
> > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > 
> > > #include 
> > > This variant is used for system header files. It searches for a file
> > > named file in a standard list of system directories. You can prepend
> > > directories to this list with the -I option (see Invocation).
> > > 
> > > #include "file"
> > > This variant is used for header files of your own program. It searches
> > > for a file named file first in the directory containing the current
> > > file, then in the quote directories and then the same directories used
> > > for . You can prepend directories to the list of quote directories
> > > with the -iquote option.
> > > 
> > > > As generated files are not in the search directory (unless the build
> > > > directory happens to match the source directory), it does not make sense
> > > > to include them with "" - doing so is merely more work for preprocessor
> > > > and a source or errors if a stale file happens to exist in the source
> > > > directory.
> > > 
> > > I agree there is a problem with stale files. But linux, for instance,
> > > asks for a "make mrproper" to avoid this.
> > 
> > We can follow what autoconf does, and add a check to configure to see if
> > there are generated files left in the source dir, when configuring with
> > builddir != srcdir, and exit with error, telling user to clean their
> > src dir first.
> > 
> > > > This changes include directives for all generated files, across the
> > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > merging, the changes will be split with one commit per file, e.g. for
> > > > ease of bisect in case of build failures, and to ease merging.
> > > > 
> > > > Note that should some generated files be missed by this tree-wide
> > > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > > and this can be addressed by a separate patch on top.
> > > > 
> > > > Signed-off-by: Michael S. Tsirkin 
> > > 
> > > I think your idea conflicts with what Markus has started to do:
> > 
> > Yes, I don't think we should revert what Markus started.   Both ways of
> > referencing QEMU headers have downsides, but I think "..." has fewer
> > downsides that "<">.
> 
> Could you please explain what the advantage of "" is?
> It seems to be gone since we moved headers away from
> source.

We moved *some* headers into the include/ directory tree.

I still count 650+ headers which are alongside the .c files.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.
> 
> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.
> 
> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 


For the record, the stated advantage is that one can
have a header file that happens to match the system
header.

To put it bluntly that does not work as designed.

For example, if a system header foo.h somewhere has #include 
then the compiler will happily pull in our own version (since that is in
the -I path) and completely ignore the system one, breaking things in
the process.

When does it make sense to use include ""? When the header is
a directory-specific one, located with the source.
This approach would both be enforced by the compiler
and help people know where to find the header.

-- 
MST

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 10:27:19AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote:
> > On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > 
> > We already do this in our makefile...it just doesn't check every
> > single generated file.
> 
> Ah yes, indeed:
> 
> $ make
> Makefile:59: *** This is an out of tree build but your source tree
> (/home/berrange/src/virt/qemu) seems to have been used for an in-tree
> build. You can fix this by running "make distclean && rm -rf *-linux-user
>  *-softmmu" in your source tree.  Stop.
> 
> 
> It is checking for existance of config-host.mak.
> 
> We have a convenient list of generated files in $(GENERATED_FILES), so
> I wonder if there's a practical way to check all of those too.
> 
> Regards,
> Daniel

With my patch presence of these files mostly stops being an issue.

> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH 1/3] x86/pv: Avoid locked bit manipulation in register_guest_callback()

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 11:58:40AM +, Andrew Cooper wrote:
> Changes to arch.vgc_flags are made to current in syncrhonous context only, and
> don't need to be locked.  (The only other changes are via
> arch_set_info_guest(), which operates on descheduled vcpus only).
> 
> Replace the {set,clear}_bit() calls with compiler-visible bitwise operations.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

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

Re: [Xen-devel] [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Stefan Hajnoczi]

On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.
> 
> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.
> 
> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.
> 
> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.

Stale header files are a pain.  I often do make distclean before
checking out a radically different QEMU version to avoid the problem.

This patch trades off the stale header file issue for a new approach to
using <> vs "", which will be hard to use consistently in the future
since it is unconventional.

Is the build time improvement worth it (please post numbers)?

Stefan


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] broken build: 448c03b3cbe14873ee637755a29ea26ee7ca9ef9

[Doug Goldstein]

On 3/20/18 1:07 AM, Juergen Gross wrote:
> On 19/03/18 20:04, Doug Goldstein wrote:
>> commit 448c03b3cbe14873ee637755a29ea26ee7ca9ef9
>> Author: Juergen Gross 
>> Date:   Mon Feb 26 09:46:12 2018 +0100
>>
>> This commit breaks the build of qemu-xen-traditional for:
>>
>> Ubuntu 14.04: https://gitlab.com/cardoe/xen/-/jobs/58266170
>> Ubuntu 16.04: https://gitlab.com/cardoe/xen/-/jobs/58266174
>>
>> A short snippet of the failure is:
>>
>>   ARi386-dm/libqemu.a
>>   LINK  i386-dm/qemu-dm
>> /builds/cardoe/xen/tools/../tools/xenstore/libxenstore.so: undefined
>> reference to `dlsym'
>> collect2: error: ld returned 1 exit status
>> make[5]: *** [qemu-dm] Error 1
>> make[5]: Leaving directory
>> `/builds/cardoe/xen/tools/qemu-xen-traditional-dir-remote/i386-dm'
>> make[4]: *** [subdir-i386-dm] Error 2
>> make[4]: Leaving directory
>> `/builds/cardoe/xen/tools/qemu-xen-traditional-dir-remote'
>> make[3]: *** [subdir-all-qemu-xen-traditional-dir] Error 2
>> make[3]: Leaving directory `/builds/cardoe/xen/tools'
>> make[2]: *** [subdirs-install] Error 2
>> make[2]: Leaving directory `/builds/cardoe/xen/tools'
>> make[1]: *** [install] Error 2
>> make[1]: Leaving directory `/builds/cardoe/xen/tools'
>> make: *** [install-tools] Error 2
>>
> 
> Did you have commit c9bd8a73656d7435b1055ee8825823aee995993e ?
> 
> 
> Juergen
> 

Indeed I do. You can see the branch that I used to build here:
https://gitlab.com/cardoe/xen/commits/gitlab which is just staging +
GitLab patches as of the compile date.

-- 
Doug Goldstein

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > So for these, we should use "".  None of these are generated files though.
> > 
> > That leads to crazy inconsistent message for developers where 50% of QEMU
> > header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

We have 1200 header files in QEMU - a developer might just reasonably remember
which header file is a QEMU header, vs which is a 3rd party system header.
Expecting devs to remember which of those 3 buckets each QEMU header falls
under is unreasonable IMHO. 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Xen-devel] [seabios test] 120946: regressions - FAIL

[osstest service owner]

flight 120946 seabios real [real]
http://logs.test-lab.xenproject.org/osstest/logs/120946/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop   fail REGR. vs. 115539

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 115539
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 115539
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 115539
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass

version targeted for testing:
 seabios  5adc8bdea6a77bdb457d9cbca9a49a7d01cc25cd
baseline version:
 seabios  0ca6d6277dfafc671a5b3718cbeb5c78e2a888ea

Last test of basis   115539  2017-11-03 20:48:58 Z  136 days
Failing since115733  2017-11-10 17:19:59 Z  129 days  150 attempts
Testing same since   120197  2018-03-03 11:37:53 Z   16 days   10 attempts


People who touched revisions under test:
  Kevin O'Connor 
  Marc-André Lureau 
  Marcel Apfelbaum 
  Michael S. Tsirkin 
  Nikolay Nikolov 
  Paul Menzel 
  Stefan Berger 
  Stephen Douthit 

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-libvirt-qemuu-debianhvm-amd64-xsm   pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmpass
 test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm pass
 test-amd64-amd64-qemuu-nested-amdfail
 test-amd64-i386-qemuu-rhel6hvm-amd   pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-i386-xl-qemuu-debianhvm-amd64 pass
 test-amd64-amd64-xl-qemuu-win7-amd64 fail
 test-amd64-i386-xl-qemuu-win7-amd64  fail
 test-amd64-amd64-xl-qemuu-ws16-amd64 fail
 test-amd64-i386-xl-qemuu-ws16-amd64  fail
 test-amd64-amd64-xl-qemuu-win10-i386 fail
 test-amd64-i386-xl-qemuu-win10-i386  fail
 test-amd64-amd64-qemuu-nested-intel  pass
 test-amd64-i386-qemuu-rhel6hvm-intel 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


Not pushing.

(No revision log; it would be 336 lines long.)

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

Re: [Xen-devel] [PATCH] xsm:schedop: introduce vcpuinfo permissions verification

[Dario Faggioli]

On Thu, 2018-02-15 at 12:51 +0200, Andrii Anisov wrote:
> From: Andrii Anisov 
> 
> Introduce per-vcpu scheduler operations permission verification.
> As long as Xvcpuinfo are in fact scheduler configuration
> manipulations
> there is no need to introduce specific access vectors.
> 
> Signed-off-by: Andrii Anisov 
>
Reviewed-by: Dario Faggioli 

Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

Re: [Xen-devel] [PATCH 3/3] x86/pv: Minor tweaks to {, compat_}register_guest_callback()

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 11:58:42AM +, Andrew Cooper wrote:
>  * Being internal functions, use int rather than long for the return value
>  * Factor out pv_vcpu into a local variable.  Reduces code volume, and removes
>some split lines.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

register_guest_callback and compat_register_guest_callback are fairly
similar, I wonder if we could translate compat_callback_register into
callback_register and pass an extra compat parameter to
register_guest_callback in order to get rid of
compat_register_guest_callback.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] xsm:schedop: introduce vcpuinfo permissions verification

[Dario Faggioli]

On Tue, 2018-03-20 at 14:19 +0200, Andrii Anisov wrote:
> Dear All,
> 
> Any comments for the patch?
> 
> It addressed the issue mentioned here: 
> https://lists.xenproject.org/archives/html/xen-devel/2017-07/msg00452
> .html
> 
Oops, sorry, I had missed it.

Now I've replied. I think it still needs a xsm / flask Ack.

Thanks and Regards,
Dario
-- 
<> (Raistlin Majere)
-
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Software Engineer @ SUSE https://www.suse.com/

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 12:39:00PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> > > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > > > QEMU coding style at the moment asks for all non-system
> > > > > > > include files to be used with #include "foo.h".
> > > > > > > However this rule actually does not make sense and
> > > > > > > creates issues for when the included file is generated.
> > > > > > 
> > > > > > If you change that, we can have issue when a system include has the 
> > > > > > same
> > > > > > name as our local include. With "", system header are taken 
> > > > > > first.
> > > > > 
> > > > > > > In C, include "file" means look in current directory,
> > > > > > > then on include search path. Current directory here
> > > > > > > means the source file directory.
> > > > > > > By comparison include  means look on include search path.
> > > > > > 
> > > > > > Not exactly, there is the notion of "system header" too.
> > > > > > 
> > > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > > > 
> > > > > > #include 
> > > > > > This variant is used for system header files. It searches for a file
> > > > > > named file in a standard list of system directories. You can prepend
> > > > > > directories to this list with the -I option (see Invocation).
> > > > > > 
> > > > > > #include "file"
> > > > > > This variant is used for header files of your own program. It 
> > > > > > searches
> > > > > > for a file named file first in the directory containing the current
> > > > > > file, then in the quote directories and then the same directories 
> > > > > > used
> > > > > > for . You can prepend directories to the list of quote 
> > > > > > directories
> > > > > > with the -iquote option.
> > > > > > 
> > > > > > > As generated files are not in the search directory (unless the 
> > > > > > > build
> > > > > > > directory happens to match the source directory), it does not 
> > > > > > > make sense
> > > > > > > to include them with "" - doing so is merely more work for 
> > > > > > > preprocessor
> > > > > > > and a source or errors if a stale file happens to exist in the 
> > > > > > > source
> > > > > > > directory.
> > > > > > 
> > > > > > I agree there is a problem with stale files. But linux, for 
> > > > > > instance,
> > > > > > asks for a "make mrproper" to avoid this.
> > > > > 
> > > > > We can follow what autoconf does, and add a check to configure to see 
> > > > > if
> > > > > there are generated files left in the source dir, when configuring 
> > > > > with
> > > > > builddir != srcdir, and exit with error, telling user to clean their
> > > > > src dir first.
> > > > > 
> > > > > > > This changes include directives for all generated files, across 
> > > > > > > the
> > > > > > > tree. The idea is to avoid sending a huge amount of email.  But 
> > > > > > > when
> > > > > > > merging, the changes will be split with one commit per file, e.g. 
> > > > > > > for
> > > > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > > > 
> > > > > > > Note that should some generated files be missed by this tree-wide
> > > > > > > refactoring, it isn't a big deal - this merely maintains the 
> > > > > > > status quo,
> > > > > > > and this can be addressed by a separate patch on top.
> > > > > > > 
> > > > > > > Signed-off-by: Michael S. Tsirkin 
> > > > > > 
> > > > > > I think your idea conflicts with what Markus has started to do:
> > > > > 
> > > > > Yes, I don't think we should revert what Markus started.   Both ways 
> > > > > of
> > > > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > > > downsides that "<">.
> > > > 
> > > > Could you please explain what the advantage of "" is?
> > > > It seems to be gone since we moved headers away from
> > > > source.
> > > 
> > > We moved *some* headers into the include/ directory tree.
> > > 
> > > I still count 650+ headers which are alongside the .c files.
> > 
> > So for these, we should use "".  None of these are generated files though.
> 
> That leads to crazy inconsistent message for developers where 50% of QEMU
> header files must use <> and the other 50% of header files must use "".
> Having a consistent message for developers is one of the key reasons why
> Markus submitted the patches to standardize on the use of "" for QEMU
> header files, leaving <> for system headers & external dependancies.
> 
> Regards,
> Daniel

I guess it's in the eye of the beholder. The simple rule since days of
K is that "" looks in the current directory of the source.
Whoever learned C knows this.

So use "" if your 

[Xen-devel] [xen-unstable-smoke test] 120987: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-arm64-arm64-xl-xsm   7 xen-boot fail REGR. vs. 120949

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

version targeted for testing:
 xen  5cb00d181c799d414438476f3af6e4ecf6afad18
baseline version:
 xen  7a1358bbe73e5f749c3d2f53478dc1f30720f949

Last test of basis   120949  2018-03-19 02:41:07 Z1 days
Testing same since   120987  2018-03-20 10:02:16 Z0 days1 attempts


People who touched revisions under test:
  Andre Przywara 
  Andre Przywara 
  Andrew Cooper 
  Julien Grall 
  Stefano Stabellini 

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



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

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

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

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


Not pushing.


commit 5cb00d181c799d414438476f3af6e4ecf6afad18
Author: Julien Grall 
Date:   Thu Mar 15 20:30:13 2018 +

ARM: GIC: extend LR read/write functions to cover EOI and source

So far our LR read/write functions do not handle the EOI bit and the
source CPUID bits in an LR, because the current VGIC implementation does
not use them.
Extend the gic_lr data structure to hold these bits of information by
using a union to differentiate field used depending on whether the vIRQ
has a corresponding pIRQ.

This allows the new VGIC to use this information.

This is based on the original patch sent by Andre Przywara [1].

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2018-03/msg00435.html

Signed-off-by: Julien Grall 
Reviewed-by: Andre Przywara 
Signed-off-by: Andre Przywara 
Acked-by: Stefano Stabellini 

commit ee375749052c649fc2710c73c7ce5371393f7742
Author: Julien Grall 
Date:   Thu Mar 15 20:30:12 2018 +

xen/arm: GIC: Only set pirq in the LR when hw_status is set

The field pirq should only be valid when the virtual interrupt
is associated to a physical interrupt.

This change will help to extend gic_lr for supporting specific virtual
interrupt field (e.g eoi, source) that clashes with the PIRQ field.

Signed-off-by: Julien Grall 
Reviewed-by: Andre Przywara 
Signed-off-by: Andre Przywara 
Acked-by: Stefano Stabellini 

commit ba8e3e422896d420510297011558cfaeb8aa75ce
Author: Julien Grall 
Date:   Thu Mar 15 20:30:11 2018 +

xen/arm: gic: Split the field state in gic_lr in 2 fields active and pending

Mostly making the code nicer to read.

Signed-off-by: Julien Grall 
Reviewed-by: Andre Przywara 
Signed-off-by: Andre Przywara 
Acked-by: Stefano Stabellini 

commit 9100b6f0e8fc3ea6e03ab56c9c7538326f654606
Author: Julien Grall 
Date:   Thu Mar 15 20:30:10 2018 +

xen/arm: gic: Use bool instead of uint8_t for the hw_status in gic_lr

hw_status can only be 1 or 0. So convert to a bool.

Signed-off-by: Julien Grall 
Reviewed-by: Andre Przywara 

Re: [Xen-devel] [Qemu-block] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:10:41PM +, Stefan Hajnoczi wrote:
> On Tue, Mar 20, 2018 at 03:54:36AM +0200, Michael S. Tsirkin wrote:
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> > 
> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> > 
> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> > 
> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> 
> Stale header files are a pain.  I often do make distclean before
> checking out a radically different QEMU version to avoid the problem.
> 
> This patch trades off the stale header file issue for a new approach to
> using <> vs "", which will be hard to use consistently

The proposed rule is to use <> everywhere except if the file is in the
source directory.  It will be very easy to use consistently for the
simple reason that compiler will enforce it.  If you use <> for a header
in the current directory build fails. So you are forced to use "".

> in the future
> since it is unconventional.

All compilers I know without exception implement
include <> meaning look in -I search path, and
include "" meaning look in current directory then
in -I search path.

Looks conventional to me.

> Is the build time improvement worth it (please post numbers)?
> 
> Stefan

Haven't tested it frankly. Will it convince anyone if it's marginally
faster?

-- 
MST

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > QEMU coding style at the moment asks for all non-system
> > > include files to be used with #include "foo.h".
> > > However this rule actually does not make sense and
> > > creates issues for when the included file is generated.
> > 
> > If you change that, we can have issue when a system include has the same
> > name as our local include. With "", system header are taken first.
> 
> > > In C, include "file" means look in current directory,
> > > then on include search path. Current directory here
> > > means the source file directory.
> > > By comparison include  means look on include search path.
> > 
> > Not exactly, there is the notion of "system header" too.
> > 
> > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > 
> > #include 
> > This variant is used for system header files. It searches for a file
> > named file in a standard list of system directories. You can prepend
> > directories to this list with the -I option (see Invocation).
> > 
> > #include "file"
> > This variant is used for header files of your own program. It searches
> > for a file named file first in the directory containing the current
> > file, then in the quote directories and then the same directories used
> > for . You can prepend directories to the list of quote directories
> > with the -iquote option.
> > 
> > > As generated files are not in the search directory (unless the build
> > > directory happens to match the source directory), it does not make sense
> > > to include them with "" - doing so is merely more work for preprocessor
> > > and a source or errors if a stale file happens to exist in the source
> > > directory.
> > 
> > I agree there is a problem with stale files. But linux, for instance,
> > asks for a "make mrproper" to avoid this.
> 
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.
> 
> > > This changes include directives for all generated files, across the
> > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > merging, the changes will be split with one commit per file, e.g. for
> > > ease of bisect in case of build failures, and to ease merging.
> > > 
> > > Note that should some generated files be missed by this tree-wide
> > > refactoring, it isn't a big deal - this merely maintains the status quo,
> > > and this can be addressed by a separate patch on top.
> > > 
> > > Signed-off-by: Michael S. Tsirkin 
> > 
> > I think your idea conflicts with what Markus has started to do:
> 
> Yes, I don't think we should revert what Markus started.   Both ways of
> referencing QEMU headers have downsides, but I think "..." has fewer
> downsides that "<">.

Could you please explain what the advantage of "" is?
It seems to be gone since we moved headers away from
source.

> The problem Michael is tackling should be pretty rare, because moist
> developers aren't frequently switching between srcdir==builddir and
> srcdir!=biulddir setups - they have their preference for which to use
> and stick with it. As long as we get ./configure to warn about the
> dirty srcdir it should be good enough
> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > QEMU coding style at the moment asks for all non-system
> > > > > include files to be used with #include "foo.h".
> > > > > However this rule actually does not make sense and
> > > > > creates issues for when the included file is generated.
> > > > 
> > > > If you change that, we can have issue when a system include has the same
> > > > name as our local include. With "", system header are taken first.
> > > 
> > > > > In C, include "file" means look in current directory,
> > > > > then on include search path. Current directory here
> > > > > means the source file directory.
> > > > > By comparison include  means look on include search path.
> > > > 
> > > > Not exactly, there is the notion of "system header" too.
> > > > 
> > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > 
> > > > #include 
> > > > This variant is used for system header files. It searches for a file
> > > > named file in a standard list of system directories. You can prepend
> > > > directories to this list with the -I option (see Invocation).
> > > > 
> > > > #include "file"
> > > > This variant is used for header files of your own program. It searches
> > > > for a file named file first in the directory containing the current
> > > > file, then in the quote directories and then the same directories used
> > > > for . You can prepend directories to the list of quote directories
> > > > with the -iquote option.
> > > > 
> > > > > As generated files are not in the search directory (unless the build
> > > > > directory happens to match the source directory), it does not make 
> > > > > sense
> > > > > to include them with "" - doing so is merely more work for 
> > > > > preprocessor
> > > > > and a source or errors if a stale file happens to exist in the source
> > > > > directory.
> > > > 
> > > > I agree there is a problem with stale files. But linux, for instance,
> > > > asks for a "make mrproper" to avoid this.
> > > 
> > > We can follow what autoconf does, and add a check to configure to see if
> > > there are generated files left in the source dir, when configuring with
> > > builddir != srcdir, and exit with error, telling user to clean their
> > > src dir first.
> > > 
> > > > > This changes include directives for all generated files, across the
> > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > merging, the changes will be split with one commit per file, e.g. for
> > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > 
> > > > > Note that should some generated files be missed by this tree-wide
> > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > quo,
> > > > > and this can be addressed by a separate patch on top.
> > > > > 
> > > > > Signed-off-by: Michael S. Tsirkin 
> > > > 
> > > > I think your idea conflicts with what Markus has started to do:
> > > 
> > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > downsides that "<">.
> > 
> > Could you please explain what the advantage of "" is?
> > It seems to be gone since we moved headers away from
> > source.
> 
> We moved *some* headers into the include/ directory tree.
> 
> I still count 650+ headers which are alongside the .c files.
> 
> Regards,
> Daniel

So for these, we should use "".  None of these are generated files though.

> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH 2/3] x86/pv: Fold {compat_}unregister_guest_callback() into its non-compat counterpart

[Roger Pau Monné]

On Thu, Mar 15, 2018 at 11:58:41AM +, Andrew Cooper wrote:
> These functions are almost identical.  They differ only in the error emitted
> for the use of CALLBACKTYPE_syscall (which is inconsequential to guests), and
> the type of their argument.
> 
> Have the callers pass the unreg.type parameter directly, avoiding the need for
> differently typed parameters.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 02:28:42PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 12:18:41PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:12:24PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Mar 20, 2018 at 09:44:06AM +, Daniel P. Berrangé wrote:
> > > > On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> > > > > Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > > > > > QEMU coding style at the moment asks for all non-system
> > > > > > include files to be used with #include "foo.h".
> > > > > > However this rule actually does not make sense and
> > > > > > creates issues for when the included file is generated.
> > > > > 
> > > > > If you change that, we can have issue when a system include has the 
> > > > > same
> > > > > name as our local include. With "", system header are taken 
> > > > > first.
> > > > 
> > > > > > In C, include "file" means look in current directory,
> > > > > > then on include search path. Current directory here
> > > > > > means the source file directory.
> > > > > > By comparison include  means look on include search path.
> > > > > 
> > > > > Not exactly, there is the notion of "system header" too.
> > > > > 
> > > > > https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> > > > > 
> > > > > #include 
> > > > > This variant is used for system header files. It searches for a file
> > > > > named file in a standard list of system directories. You can prepend
> > > > > directories to this list with the -I option (see Invocation).
> > > > > 
> > > > > #include "file"
> > > > > This variant is used for header files of your own program. It searches
> > > > > for a file named file first in the directory containing the current
> > > > > file, then in the quote directories and then the same directories used
> > > > > for . You can prepend directories to the list of quote 
> > > > > directories
> > > > > with the -iquote option.
> > > > > 
> > > > > > As generated files are not in the search directory (unless the build
> > > > > > directory happens to match the source directory), it does not make 
> > > > > > sense
> > > > > > to include them with "" - doing so is merely more work for 
> > > > > > preprocessor
> > > > > > and a source or errors if a stale file happens to exist in the 
> > > > > > source
> > > > > > directory.
> > > > > 
> > > > > I agree there is a problem with stale files. But linux, for instance,
> > > > > asks for a "make mrproper" to avoid this.
> > > > 
> > > > We can follow what autoconf does, and add a check to configure to see if
> > > > there are generated files left in the source dir, when configuring with
> > > > builddir != srcdir, and exit with error, telling user to clean their
> > > > src dir first.
> > > > 
> > > > > > This changes include directives for all generated files, across the
> > > > > > tree. The idea is to avoid sending a huge amount of email.  But when
> > > > > > merging, the changes will be split with one commit per file, e.g. 
> > > > > > for
> > > > > > ease of bisect in case of build failures, and to ease merging.
> > > > > > 
> > > > > > Note that should some generated files be missed by this tree-wide
> > > > > > refactoring, it isn't a big deal - this merely maintains the status 
> > > > > > quo,
> > > > > > and this can be addressed by a separate patch on top.
> > > > > > 
> > > > > > Signed-off-by: Michael S. Tsirkin 
> > > > > 
> > > > > I think your idea conflicts with what Markus has started to do:
> > > > 
> > > > Yes, I don't think we should revert what Markus started.   Both ways of
> > > > referencing QEMU headers have downsides, but I think "..." has fewer
> > > > downsides that "<">.
> > > 
> > > Could you please explain what the advantage of "" is?
> > > It seems to be gone since we moved headers away from
> > > source.
> > 
> > We moved *some* headers into the include/ directory tree.
> > 
> > I still count 650+ headers which are alongside the .c files.
> 
> So for these, we should use "".  None of these are generated files though.

That leads to crazy inconsistent message for developers where 50% of QEMU
header files must use <> and the other 50% of header files must use "".
Having a consistent message for developers is one of the key reasons why
Markus submitted the patches to standardize on the use of "" for QEMU
header files, leaving <> for system headers & external dependancies.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

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

[osstest service owner]

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

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 ovmf ae38c9765a27264ecea03c6430b956b74a427213
baseline version:
 ovmf 34d808add3dc23aaa37e1c9edb2fcc2b50118367

Last test of basis   120879  2018-03-17 15:34:47 Z2 days
Testing same since   120951  2018-03-19 02:43:26 Z1 days1 attempts


People who touched revisions under test:
  Carsey, Jaben 
  Dandan Bi 
  Jaben Carsey 
  Yonghong Zhu 

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 :

To xenbits.xen.org:/home/xen/git/osstest/ovmf.git
   34d808add3..ae38c9765a  ae38c9765a27264ecea03c6430b956b74a427213 -> 
xen-tested-master

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

Re: [Xen-devel] [PATCH v3] xen/acpi: upload _PSD info for non Dom0 CPUs too

[Boris Ostrovsky]

On 03/20/2018 05:41 AM, Rafael J. Wysocki wrote:
> On Fri, Mar 16, 2018 at 2:57 PM, Joao Martins  
> wrote:
>> On 03/15/2018 03:45 PM, Boris Ostrovsky wrote:
>>> On 03/15/2018 10:22 AM, Joao Martins wrote:
 All uploaded PM data from non-dom0 CPUs takes the info from vCPU 0 and
 changing only the acpi_id. For processors which P-state coordination type
 is HW_ALL (0xFD) it is OK to upload bogus P-state dependency information
 (_PSD), because Xen will ignore any cpufreq domains created for past CPUs.

 Albeit for platforms which expose coordination types as SW_ANY or SW_ALL,
 this will have some unintended side effects. Effectively, it will look at
 the P-state domain existence and *if it already exists* it will skip the
 acpi-cpufreq initialization and thus inherit the policy from the first CPU
 in the cpufreq domain. This will finally lead to the original cpu not
 changing target freq to P0 other than the first in the domain. Which will
 make turbo boost not getting enabled (e.g. for 'performance' governor) for
 all cpus.

 This patch fixes that, by also evaluating _PSD when we enumerate all ACPI
 processors and thus always uploading the correct info to Xen. We export
 acpi_processor_get_psd() for that this purpose, but change signature
 to not assume an existent of acpi_processor given that ACPI isn't creating
 an acpi_processor for non-dom0 CPUs.

 Signed-off-by: Joao Martins 
>>> Reviewed-by: Boris Ostrovsky 
>>>
>> Thanks!
>>
>> I suppose what's remaining is review (or ack) from ACPI folks on the 
>> interface
>> changes made to acpi_processor_get_psd().
> There you go:
>
> Acked-by: Rafael J. Wysocki 
>
> Do you want to route this via Xen?


Sure, I'll queue it for 4.17

Thanks.
-boris

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

[Xen-devel] Spectre Mitigations in Xen 4.6

[Jason Andryuk]

Hi,

I've been experimenting with Linux 4.14 on Xen 4.6.  Now that Intel
microcode is generally
available, I'm starting to exercise the new mitigation code paths.

For Xen 4.6-4.8, microcode loading happens after
init_speculation_mitigations, so Xen only
detects the boot firmware features.  The early microcode loading
f97838bbd980 ("x86: Move
microcode loading earlier") can be cherry-picked, though small fix ups
are needed for
bool/true/false -> bool_t/1/0 and smpboot.c:smp_store_cpu_info() to
retain "struct
cpuinfo_x86 *c = cpu_data + id;".

With that in place, I'm seeing Dom0 receive a general protection fault on boot

[   25.460035] general protection fault:  [#1] SMP
[   25.460292] EIP: switch_mm_irqs_off+0xbe/0x600

switch_mm_irqs_off+0xbe is the inlined
indirect_branch_prediction_barrier(void)
{
alternative_msr_write(MSR_IA32_PRED_CMD, PRED_CMD_IBPB,
  X86_FEATURE_USE_IBPB);
}

The system boots when dom0 disables IBPB manipulation with
nospectre_v2 on the kernel
command line.

I think Xen ends up here in xen/arch/x86/traps.c:emulate_privileged_op(),
case MSR_PRED_CMD:
domain_cpuid(currd, 7, 0, , , , );
domain_cpuid(currd, 0x8008, 0, , , , );
if ( !(edx & cpufeat_mask(X86_FEATURE_IBRSB)) &&
 !(ebx & cpufeat_mask(X86_FEATURE_IBPB)) )
goto fail; /* MSR available? */

/*
 * The only defined behaviour is when writing PRED_CMD_IBPB.  In
 * practice, real hardware accepts any value without faulting.
 */
if ( eax & PRED_CMD_IBPB )
wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
break;

...but Dom0 doesn't have a cpuid policy configured, so the IBRSB/IBPB
check fails and we GP.
Did I read that correctly?  If that is the case, how should Dom0 be handled?

Other 4.14 PV & HVM DomUs boot fine and detect (and use?) IBPB once
Dom0 boots (with
spectre mitigations disabled).

Regards,
Jason

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

Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

[Jan Beulich]

>>> On 20.03.18 at 12:43,  wrote:
> Since you only had comments on patch 7 and 11 and there's the extra
> fix for the test harness, should I just send those and provide you
> with a git branch that contains the rest?

Well, if it's not too much hassle for you I'd prefer if you resent the
full series (or, see below, whatever is left at the time you get
around to send it).

> I can also wait if you want to commit the start of the series
> (provided I get all the relevant Acks) and rebase on top of that.

I'd leave that up to you; it looks like both tool stack maintainers
are not around right now, so their ack for patch 1 may take a few
more days to arrive.

Jan


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

Re: [Xen-devel] [PATCH v5 03/16] xen/arm: mm: Use gaddr_to_gfn rather than _gfn(paddr_to_pfn(...))

[Julien Grall]

Hi George,

On 03/15/2018 04:15 PM, George Dunlap wrote:

On Wed, Mar 14, 2018 at 6:19 PM,   wrote:

From: Julien Grall 

The construction _gfn(paddr_to_pfn(...)) can be simplified by using
gaddr_to_gfn.

Signed-off-by: Julien Grall 


Not sure if "simplified" is the right word here (and in the previous
patch); simplified implies fewer steps in the calculation, but it
looks like the steps so far are identical.  Using a macro rather than
hand-coding stuff is cleaner and more maintainable; don't think it
really needs to be justified.


I can drop the justification in both patches.



In any case:

Reviewed-by: George Dunlap 


Thank you!

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Laurent Vivier]

Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> QEMU coding style at the moment asks for all non-system
> include files to be used with #include "foo.h".
> However this rule actually does not make sense and
> creates issues for when the included file is generated.

If you change that, we can have issue when a system include has the same
name as our local include. With "", system header are taken first.

> In C, include "file" means look in current directory,
> then on include search path. Current directory here
> means the source file directory.
> By comparison include  means look on include search path.

Not exactly, there is the notion of "system header" too.

https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html

#include 
This variant is used for system header files. It searches for a file
named file in a standard list of system directories. You can prepend
directories to this list with the -I option (see Invocation).

#include "file"
This variant is used for header files of your own program. It searches
for a file named file first in the directory containing the current
file, then in the quote directories and then the same directories used
for . You can prepend directories to the list of quote directories
with the -iquote option.

> As generated files are not in the search directory (unless the build
> directory happens to match the source directory), it does not make sense
> to include them with "" - doing so is merely more work for preprocessor
> and a source or errors if a stale file happens to exist in the source
> directory.

I agree there is a problem with stale files. But linux, for instance,
asks for a "make mrproper" to avoid this.

> This changes include directives for all generated files, across the
> tree. The idea is to avoid sending a huge amount of email.  But when
> merging, the changes will be split with one commit per file, e.g. for
> ease of bisect in case of build failures, and to ease merging.
> 
> Note that should some generated files be missed by this tree-wide
> refactoring, it isn't a big deal - this merely maintains the status quo,
> and this can be addressed by a separate patch on top.
> 
> Signed-off-by: Michael S. Tsirkin 

I think your idea conflicts with what Markus has started to do:

commit d8e39b70625d4ba1e998439d1a077b4b978930e7
Author: Markus Armbruster 
Date:   Thu Feb 1 12:18:28 2018 +0100

Use #include "..." for our own headers, <...> for others

System headers should be included with <...>, our own headers with
"...".  Offenders tracked down with an ugly, brittle and probably
buggy Perl script.  Previous iteration was commit a9c94277f0.

Delete inclusions of "string.h" and "strings.h" instead of fixing them
to  and , because we always include these via
osdep.h.

Put the cleaned up system header includes first.

While there, separate #include from file comment with exactly one
blank line.

commit a9c94277f07d19d3eb14f199c3e93491aa3eae0e
Author: Markus Armbruster 
Date:   Wed Jun 22 19:11:19 2016 +0200

Use #include "..." for our own headers, <...> for others

Tracked down with an ugly, brittle and probably buggy Perl script.

Also move includes converted to <...> up so they get included before
ours where that's obviously okay.

Thanks,
Laurent

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

Re: [Xen-devel] [RFC PATCH 11/12] hvmloader: use libacpi to build MCFG table

[Jan Beulich]

>>> On 19.03.18 at 22:20,  wrote:
> On Mon, 19 Mar 2018 17:49:09 +
> Roger Pau Monné  wrote:
>>On Tue, Mar 13, 2018 at 04:33:56AM +1000, Alexey Gerasimenko wrote:
>>> --- a/tools/firmware/hvmloader/util.c
>>> +++ b/tools/firmware/hvmloader/util.c
>>> @@ -782,6 +782,69 @@ int get_pc_machine_type(void)
>>>  return machine_type;
>>>  }
>>>  
>>> +#define PCIEXBAR_ADDR_MASK_64MB (~((1ULL << 26) - 1))
>>> +#define PCIEXBAR_ADDR_MASK_128MB(~((1ULL << 27) - 1))
>>> +#define PCIEXBAR_ADDR_MASK_256MB(~((1ULL << 28) - 1))
>>> +#define PCIEXBAR_LENGTH_BITS(reg)   (((reg) >> 1) & 3)
>>> +#define PCIEXBAREN  1  
>>
>>PCIEXBAR_ENABLE maybe?
> 
> PCIEXBAREN is just an official name of this bit from the
> Intel datasheet. :) OK, will rename it to PCIEXBAR_ENABLE.

I think using names from the datasheet (where they exist) is
preferable in cases like this one.

>>> +switch (PCIEXBAR_LENGTH_BITS(reg))
>>> +{
>>> +case 0:
>>> +base &= PCIEXBAR_ADDR_MASK_256MB;
>>> +break;
>>> +case 1:
>>> +base &= PCIEXBAR_ADDR_MASK_128MB;
>>> +break;
>>> +case 2:
>>> +base &= PCIEXBAR_ADDR_MASK_64MB;
>>> +break;  
>>
>>Missing newlines, plus this looks like it wants to use the defines
>>introduced in patch 7 (PCIEXBAR_{64,128,256}_BUSES). Also any reason
>>this patch and patch 7 cannot be put sequentially?
> 
> I think all these #defines should find a way to pci_regs.h, it seems
> like an appropriate place for them.

I don't think device specific defines belong into pci_regs.h.

Jan

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

Re: [Xen-devel] TLB flushing

[Jan Beulich]

>>> On 20.03.18 at 10:29,  wrote:
> On 20/03/18 10:19, Jan Beulich wrote:
> On 20.03.18 at 09:50,  wrote:
>>> While hunting a strange bug in my PCID patch series hinting at some
>>> TLB invalidation problem I discovered a piece of code looking rather
>>> fishy to me.
>>>
>>> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
>>> of FLUSH_TLB_GLOBAL?
>>>
>>> While not being a problem in current code as both will flush all TLB
>>> entries my series will change that by using invpcid to flush only the
>>> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
>>>
>>> I can send a patch if anyone can confirm that using FLUSH_TLB only is
>>> wrong.
>> 
>> I think this shouldn't be a separate patch, but an integral part of the
>> one introducing the distinction between "all" and non-global flushes.
>> This is because
>> - right now it doesn't make a difference (we do "all" flushes anyway),
>> - back in the 32-bit days it didn't matter because guest mappings
>>   would never have been allowed to be global, and transient Xen
>>   mappings also would never have had the G bit set.
>> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
>> this would need to become FLUSH_TLB_GLOBAL at the point the
>> kind of flush gets altered, while without it could remain at FLUSH_TLB.
> 
> Really? Aren't global hypervisor mappings affected by this, too?

Yes and no. The timestamp here is needed to know whether to
flush when a page gets recycled. As long as G is never set on
guest controlled mappings for pages which may be recycled,
there's no issue. In particular, the G bits in the 1:1 mappings are
of no interest here (and in fact anything Xen maintains which no
guest can control), as those mappings never go away (or if they
did, e.g. when a page needs to be offlined for causing #MC, an
explicit global flush for that page would be required).

>> Perhaps it is worthwhile to retain this distinction just for
>> documentation purposes (in case a future change wants to turn off
>> that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).
> 
> I think as long as FLUSH_TLB_GLOBAL is being used in the code
> new_tlbflush_clock_period() should do so, too. In case there is no need
> to do TLB flushes including global pages, FLUSH_TLB_GLOBAL can be
> modified to do only non-global flushing (with a comment explaining why
> this is secure).

No - an explicit global flush request (as per above explanation)
must never be converted down to a non-global one. Such a
"conversion" would only be valid when CR4.PGE is clear at all
times (but then it's not really a conversion anymore).

Jan


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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 10:01:24AM +, Peter Maydell wrote:
> On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> > We can follow what autoconf does, and add a check to configure to see if
> > there are generated files left in the source dir, when configuring with
> > builddir != srcdir, and exit with error, telling user to clean their
> > src dir first.
> 
> We already do this in our makefile...it just doesn't check every
> single generated file.

Ah yes, indeed:

$ make
Makefile:59: *** This is an out of tree build but your source tree
(/home/berrange/src/virt/qemu) seems to have been used for an in-tree
build. You can fix this by running "make distclean && rm -rf *-linux-user
 *-softmmu" in your source tree.  Stop.


It is checking for existance of config-host.mak.

We have a convenient list of generated files in $(GENERATED_FILES), so
I wonder if there's a practical way to check all of those too.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH v2 00/45] New VGIC(-v2) implementation

[Andre Przywara]

Hi,

On 20/03/18 08:30, Julien Grall wrote:
> Hi,
> 
> On 03/15/2018 08:30 PM, Andre Przywara wrote:
>> tl;dr: Coarse changelog below, individual patches have changelogs as
>> well. git branch:
>> http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-new/v2
>>
>> git://linux-arm.org/xen-ap.git branch vgic-new/v2
>>
>> Another update, addressing the review comments. Nothing too
>> outstanding this
>> time, the most interesting changes include:
>> - removing the split-out preparatory patches, which are already merged
>> - changing the setting and clearing of _IRQ_INPROGRESS
>> - including Julien's LR access rework series
>> - restricting new level IRQ handling to the new VGIC
>> - fix multiple SGI handling (mimicing the recent Linux/KVM patch)
>> - ASSERTing that h/w IRQs stay connected to their virtual IRQs
>> - directly update h/w affinity, without taking the desc lock
>> - restrict 8K struct vcpu to new VGIC and ARM64
>> - use separate Makefile for vgic/ directory
>> - many minor changes to address whitespace issues and usage of unsigned,
>>    also extending comments
>>
>> A summarising changelog can be found below, each individual patch has
>> its own changelog as well.
>>
>> There are some things that have (still) not been covered yet:
>> - struct VCPU still allocates two pages on ARM64 when using the new
>> VGIC now.
>> We could try to look if we can allocate some parts of struct vcpu
>> instead of
>> embedding sub-structures into it.
>> - vGICv3 support is not implemented, but should be fairly
>> straight-forward to
>> add, as the design incorporated this already. Will look at this next.
>> - There is a possible DOS vector on the VCPU ap_list, which holds pending
>> vIRQs. A guest can make this list rather long, which forces the
>> hypervisor
>> to hold the list lock when iterating the list. This should be bounded by
>> the number of emulated vIRQs though, and there are ideas how to mitigate
>> this issue. Those fixes would be posted on top as fixes later.
>> - There is no ITS support, though the VGIC code itself is more ready
>> for that
>> than the old VGIC ever was. However due to differences between the Xen
>> and KVM architecture the ITS bits are not easy to port over to Xen.
>> - Do we need to call vgic_evtchn_irq_pending() in
>> local_event_needs_delivery_nomask()? The event channel IRQ should be
>> covered
>> by the VGIC already.
> 
> Thank you for summarising the open questions! I will log them on jira
> once the series is merged.

Good idea, thanks!

> Meanwhile, I have committed patches #1 - #8.

Many thanks for that! Only 37 to go ;-)
Will try to send a v3 later today.

Cheers,
Andre.

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

Re: [Xen-devel] [PATCH v5 04/16] xen/arm: mm: Remove unused M2P code

[Julien Grall]

On 03/15/2018 07:20 AM, Alan Robinson wrote:

Hi Julien,


Hi Alan,


At the same time move the remaining M2P define just above just above
set_gpfn_from_mfn to keep all the dummy helpers for M2P together.


   At the same time move the remaining M2P define just above
   set_gpfn_from_mfn to keep all the dummy helpers for M2P together.


Thank you for looking at the patch. I will update the commit message on 
the next version.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 08/12] libxl: Q35 support (new option device_model_machine)

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 08:11:49AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 17:01:18 +
> Roger Pau Monné  wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:53AM +1000, Alexey Gerasimenko wrote:
> >> Provide a new domain config option to select the emulated machine
> >> type, device_model_machine. It has following possible values:
> >> - "i440" - i440 emulation (default)
> >> - "q35" - emulate a Q35 machine. By default, the storage interface
> >> is AHCI.  
> >
> >I would rather name this machine_chipset or device_model_chipset.
> 
> device_model_ prefix is a must I think -- multiple device model related
> options have names starting with device_model_.
> 
> device_model_chipset... well, maybe, but we're actually specifying a
> QEMU machine here. In QEMU mailing list there was even a suggestion
> to allow to pass a machine version number here, like "pc-q35-2.10".
> I think some opinions are needed here.

I'm not sure what a 'machine' is in QEMU speak, but in my mind I would
consider PC a machine (vs ARM for example).

I think 'chipset' is clearer, but again others should express their
opinion.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH 3/3] x86/pv: Minor tweaks to {, compat_}register_guest_callback()

[Jan Beulich]

>>> On 15.03.18 at 12:58,  wrote:
> --- a/xen/arch/x86/pv/callback.c
> +++ b/xen/arch/x86/pv/callback.c
> @@ -71,10 +71,11 @@ static void unregister_guest_nmi_callback(void)
>  memset(t, 0, sizeof(*t));
>  }
>  
> -static long register_guest_callback(struct callback_register *reg)
> +static int register_guest_callback(struct callback_register *reg)
>  {
> -long ret = 0;
>  struct vcpu *curr = current;
> +struct pv_vcpu *pv = >arch.pv_vcpu;
> +int ret = 0;

Instead of altering ret's type, why don't you drop it altogether?
Same actually for the unregister function in patch 2. Preferably
with that done
Reviewed-by: Jan Beulich 

Jan


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

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 120037
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 120037
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 120037
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 120037
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 120037
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 120037
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 120037
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 120037
 test-amd64-amd64-xl-qemut-ws16-amd64 16 guest-localmigrate/x10 fail like 120037
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 120037
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  never pass
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass

version targeted for testing:
 xen  0012ae8afb4a6e76f2847119f2c6850fbf41d9b7
baseline version:
 xen  a823a5280f25ad19a751dd9a41044f556471e61a

Last test of basis   120037  2018-02-26 13:16:29 Z   21 days
Failing since120076  2018-02-27 20:33:32 Z   20 days   11 attempts
Testing same since   120859  2018-03-17 04:13:38 Z3 days2 attempts


People who touched revisions under test:
  Alexandru Isaila 
  Andre 

Re: [Xen-devel] [PATCH 28/57] ARM: new VGIC: Add acccessor to new struct vgic_irq instance

[Julien Grall]

Hi,

Sorry for the formatting.

On 20 Mar 2018 19:01, "Andre Przywara"  wrote:


I am happy to add a comment saying that a list is not the right data
structure.


Fair enough. I won't argue more, let's keep the list and add a comment. I
do want to see that series merged in Xen 4.11.

Cheers,


Cheers,
Andre.

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

Re: [Xen-devel] [PATCH v10 07/11] vpci/bars: add handlers to map the BARs

[Jan Beulich]

>>> On 20.03.18 at 12:12,  wrote:
> On Mon, Mar 19, 2018 at 10:18:34AM -0600, Jan Beulich wrote:
>> >>> On 16.03.18 at 14:30,  wrote:
>> > +static int modify_bars(const struct pci_dev *pdev, bool map, bool 
>> > rom_only)
>> > +{
>> > +struct vpci_header *header = >vpci->header;
>> > +struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>> > +struct pci_dev *tmp, *dev = NULL;
>> > +unsigned int i;
>> > +int rc;
>> > +
>> > +if ( !mem )
>> > +return -ENOMEM;
>> > +
>> > +/*
>> > + * Create a rangeset that represents the current device BARs memory 
>> > region
>> > + * and compare it against all the currently active BAR memory 
>> > regions. If
>> > + * an overlap is found, subtract it from the region to be 
>> > mapped/unmapped.
>> > + *
>> > + * First fill the rangeset with all the BARs of this device or with 
>> > the ROM
>> > + * BAR only, depending on whether the guest is toggling the memory 
>> > decode
>> > + * bit of the command register, or the enable bit of the ROM BAR 
>> > register.
>> > + */
>> > +for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> > +{
>> > +const struct vpci_bar *bar = >bars[i];
>> > +
>> > +if ( !MAPPABLE_BAR(bar) ||
>> > + (rom_only ? bar->type != VPCI_BAR_ROM
>> > +   : (bar->type == VPCI_BAR_ROM && 
>> > !header->rom_enabled)) )
>> > +continue;
>> > +
>> > +rc = rangeset_add_range(mem, PFN_DOWN(bar->addr),
>> > +PFN_DOWN(bar->addr + bar->size - 1));
>> > +if ( rc )
>> > +{
>> > +printk(XENLOG_G_WARNING
>> > +   "Failed to add [%" PRI_gfn ", %" PRI_gfn "]: %d\n",
>> > +   PFN_DOWN(bar->addr), PFN_DOWN(bar->addr + bar->size - 
>> > 1),
>> > +   rc);
>> > +rangeset_destroy(mem);
>> > +return rc;
>> > +}
>> > +}
>> > +
>> > +/*
>> > + * Check for overlaps with other BARs. Note that only BARs that are
>> > + * currently mapped (enabled) are checked for overlaps.
>> > + */
>> > +list_for_each_entry(tmp, >domain->arch.pdev_list, domain_list)
>> > +{
>> > +if ( tmp == pdev )
>> > +{
>> > +/*
>> > + * Need to store the device so it's not constified and
>> > + * maybe_defer_map can modify it in case of error.
>> > + */
>> > +dev = tmp;
>> 
>> There's no maybe_defer_map anymore.
>> 
>> And then I'm having a problem with this comment on const-ness:
>> apply_map() only wants to hand the device to modify_decoding(),
>> whose respective argument is const. defer_map() stores the
>> pointer, but afaics again only for vpci_process_pending() to hand
>> it on to modify_decoding(); the lock the function takes is in a
>> structure pointed to from the device, so unaffected by the const.
> 
> vpci_process_pending calls vpci_remove_device which needs a
> non-constified pdev, since it sets pdev->vpci = NULL.

Oh, I see. Still means that apply_map() could have its parameter
constified.

>> > + * memory has been added or removed from the p2m (because the
>> > + * actual p2m changes are deferred in maybe_defer_map) and 
>> > the ROM
>> > + * enable bit has not been changed, so leave everything as-is,
>> > + * hoping the guest will realize and try again.
>> > + */
>> > +return;
>> > +}
>> > +else
>> > +{
>> > +header->rom_enabled = new_enabled;
>> > +pci_conf_write32(pdev->seg, pdev->bus, slot, func, reg, val);
>> > +}
>> > +
>> > +if ( !new_enabled )
>> > +rom->addr = val & PCI_ROM_ADDRESS_MASK;
>> 
>> I'm struggling to understand this update, not the least seeing the
>> other update further up.
> 
> I've added some comments now, but the point is that when mapping the
> ROM BAR we should update the addr field first, so that the correct p2m
> mappings are established
> 
> In the unmap case however (!new_enabled) the addr needs to be updated
> after calling modify_bars, or else the wrong address might be
> unmapped.
> 
> Does this address your concern?

Yes. It was largely the asymmetry with bar_write() which did
confuse me, but I can see now why that one doesn't have such
ordering constraints.

Jan

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

Re: [Xen-devel] [PATCH v2 00/45] New VGIC(-v2) implementation

[Julien Grall]

Hi,

On 03/15/2018 08:30 PM, Andre Przywara wrote:

tl;dr: Coarse changelog below, individual patches have changelogs as
well. git branch:
http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/vgic-new/v2
git://linux-arm.org/xen-ap.git branch vgic-new/v2

Another update, addressing the review comments. Nothing too outstanding this
time, the most interesting changes include:
- removing the split-out preparatory patches, which are already merged
- changing the setting and clearing of _IRQ_INPROGRESS
- including Julien's LR access rework series
- restricting new level IRQ handling to the new VGIC
- fix multiple SGI handling (mimicing the recent Linux/KVM patch)
- ASSERTing that h/w IRQs stay connected to their virtual IRQs
- directly update h/w affinity, without taking the desc lock
- restrict 8K struct vcpu to new VGIC and ARM64
- use separate Makefile for vgic/ directory
- many minor changes to address whitespace issues and usage of unsigned,
   also extending comments

A summarising changelog can be found below, each individual patch has
its own changelog as well.

There are some things that have (still) not been covered yet:
- struct VCPU still allocates two pages on ARM64 when using the new VGIC now.
We could try to look if we can allocate some parts of struct vcpu instead of
embedding sub-structures into it.
- vGICv3 support is not implemented, but should be fairly straight-forward to
add, as the design incorporated this already. Will look at this next.
- There is a possible DOS vector on the VCPU ap_list, which holds pending
vIRQs. A guest can make this list rather long, which forces the hypervisor
to hold the list lock when iterating the list. This should be bounded by
the number of emulated vIRQs though, and there are ideas how to mitigate
this issue. Those fixes would be posted on top as fixes later.
- There is no ITS support, though the VGIC code itself is more ready for that
than the old VGIC ever was. However due to differences between the Xen
and KVM architecture the ITS bits are not easy to port over to Xen.
- Do we need to call vgic_evtchn_irq_pending() in
local_event_needs_delivery_nomask()? The event channel IRQ should be covered
by the VGIC already.


Thank you for summarising the open questions! I will log them on jira 
once the series is merged.


Meanwhile, I have committed patches #1 - #8.

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 05/20] arm/boot: Mark construct_dom0() as __init

[Julien Grall]

On 03/20/2018 03:40 AM, Julien Grall wrote:

Hi Andrew,

On 03/19/2018 07:13 PM, Andrew Cooper wrote:

Its sole caller, start_xen(), is __init.


Actually, most of that files could benefits of __init as this is domain 
only build.


In any case, this patch is already a good start. So:

Acked-by: Julien Grall 


Committed.

Cheers,



Cheers,



Signed-off-by: Andrew Cooper 
---
CC: Stefano Stabellini 
CC: Julien Grall 
---
  xen/arch/arm/domain_build.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 28ee876..9ef9030 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2094,7 +2094,7 @@ static void __init find_gnttab_region(struct 
domain *d,
 kinfo->gnttab_start, kinfo->gnttab_start + 
kinfo->gnttab_size);

  }
-int construct_dom0(struct domain *d)
+int __init construct_dom0(struct domain *d)
  {
  struct kernel_info kinfo = {};
  struct vcpu *saved_current;





--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 07/12] hvmloader: allocate MMCONFIG area in the MMIO hole + minor code refactoring

[Paul Durrant]

> -Original Message-
> From: Roger Pau Monne
> Sent: 20 March 2018 08:51
> To: Alexey G 
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> ; Ian Jackson ; Jan
> Beulich ; Wei Liu ; Paul Durrant
> 
> Subject: Re: [Xen-devel] [RFC PATCH 07/12] hvmloader: allocate MMCONFIG
> area in the MMIO hole + minor code refactoring
> 
> On Tue, Mar 20, 2018 at 05:49:22AM +1000, Alexey G wrote:
> > On Mon, 19 Mar 2018 15:58:02 +
> > Roger Pau Monné  wrote:
> >
> > >On Tue, Mar 13, 2018 at 04:33:52AM +1000, Alexey Gerasimenko wrote:
> > >> Much like normal PCI BARs or other chipset-specific memory-mapped
> > >> resources, MMCONFIG area needs space in MMIO hole, so we must
> > >> allocate it manually.
> > >>
> > >> The actual MMCONFIG size depends on a number of PCI buses available
> > >> which should be covered by ECAM. Possible options are 64MB, 128MB
> > >> and 256MB. As we are limited to the bus 0 currently, thus using
> > >> lowest possible setting (64MB), #defined via PCI_MAX_MCFG_BUSES in
> > >> hvmloader/config.h. When multiple PCI buses support for Xen will be
> > >> implemented, PCI_MAX_MCFG_BUSES may be changed to calculation
> of the
> > >> number of buses according to results of the PCI devices enumeration.
> > >>
> > >> The way to allocate MMCONFIG range in MMIO hole is similar to how
> > >> other PCI BARs are allocated. The patch extends 'bars' structure to
> > >> make it universal for any arbitrary BAR type -- either IO, MMIO, ROM
> > >> or a chipset-specific resource.
> > >
> > >I'm not sure this is fully correct. The IOREQ interface can
> > >differentiate PCI devices and forward config space accesses to
> > >different emulators (see IOREQ_TYPE_PCI_CONFIG). With this change
> you
> > >will forward all MCFG accesses to QEMU, which will likely be wrong if
> > >there are multiple PCI-device emulators for the same domain.
> > >
> > >Ie: AFAICT Xen needs to know about the MCFG emulation and detect
> > >accesses to it in order to forward them to the right emulators.
> > >
> > >Adding Paul who knows more about all this.
> >
> > In which use cases multiple PCI-device emulators are used for a single
> > HVM domain? Is it a proprietary setup?
> 
> Likely. I think XenGT might be using it. It's a feature of the IOREQ
> implementation in Xen.
> 

Multiple ioreq servers are a supported use-case for Xen, if only experimental 
at this point. And indeed xengt is one such use-case.

> Traditional PCI config space accesses are not IO port space accesses.
> The IOREQ code in Xen detects accesses to ports 0xcf8/0xcfc and IOREQ
> servers can register devices they would like to receive configuration
> space accesses for. QEMU is already making use of this, see for
> example xen_map_pcidev in the QEMU code.
> 
> By treating MCFG accesses as MMIO you are bypassing the IOREQ PCI
> layer, and thus a IOREQ server could register a PCI device and only
> receive PCI configuration accesses from the IO port space, while MCFG
> accesses would be forwarded somewhere else.
> 
> I think you need to make the IOREQ code aware of the MCFG area and
> XEN_DMOP_IO_RANGE_PCI needs to forward both IO space and MCFG
> accesses
> to the right IOREQ server.

Yes, Xen must intercept all accesses to PCI config space and route them 
accordingly.

> 
> > I assume it is somehow related to this code in xen-hvm.c:
> > /* Fake a write to port 0xCF8 so that
> >  * the config space access will target the
> >  * correct device model.
> >  */
> > val = (1u << 31) | ((req->addr & 0x0f00) <...>
> > do_outp(0xcf8, 4, val);
> > if yes, similar thing can be made for IOREQ_TYPE_COPY accesses to
> > the emulated MMCONFIG if needed.
> 
> I have to admit I don't know that much about QEMU, and I have no idea
> what the chunk above is supposed to accomplish.
> 

The easiest way to make QEMU behave appropriately when dealing with a config 
space ioreq was indeed to make it appear as a write to cf8 followed by a read 
or write to cfc.

> >
> > In HVM+QEMU case we are not limited to merely passed through devices,
> > most of the observable PCI config space devices belong to one particular
> > QEMU instance. This dictates the overall emulated MMCONFIG layout
> > for a domain which should be in sync to what QEMU emulates via
> CF8h/CFCh
> > accesses... and between multiple device model instances (if there are
> > any, still not sure what multiple PCI-device emulators you mentioned
> > really are).
> 
> In newer versions of Xen (>4.5 IIRC, Paul knows more), QEMU doesn't
> directly trap accesses to the 0xcf8/0xcfc IO ports, it's Xen instead
> the one that detects and decodes such accesses, and then forwards them
> to the IOREQ server that has been registered to handle them.
> 

Correct.

> You cannot simply forward all 

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 09:58:23AM +0100, Laurent Vivier wrote:
> Le 20/03/2018 à 02:54, Michael S. Tsirkin a écrit :
> > QEMU coding style at the moment asks for all non-system
> > include files to be used with #include "foo.h".
> > However this rule actually does not make sense and
> > creates issues for when the included file is generated.
> 
> If you change that, we can have issue when a system include has the same
> name as our local include. With "", system header are taken first.

> > In C, include "file" means look in current directory,
> > then on include search path. Current directory here
> > means the source file directory.
> > By comparison include  means look on include search path.
> 
> Not exactly, there is the notion of "system header" too.
> 
> https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
> 
> #include 
> This variant is used for system header files. It searches for a file
> named file in a standard list of system directories. You can prepend
> directories to this list with the -I option (see Invocation).
> 
> #include "file"
> This variant is used for header files of your own program. It searches
> for a file named file first in the directory containing the current
> file, then in the quote directories and then the same directories used
> for . You can prepend directories to the list of quote directories
> with the -iquote option.
> 
> > As generated files are not in the search directory (unless the build
> > directory happens to match the source directory), it does not make sense
> > to include them with "" - doing so is merely more work for preprocessor
> > and a source or errors if a stale file happens to exist in the source
> > directory.
> 
> I agree there is a problem with stale files. But linux, for instance,
> asks for a "make mrproper" to avoid this.

We can follow what autoconf does, and add a check to configure to see if
there are generated files left in the source dir, when configuring with
builddir != srcdir, and exit with error, telling user to clean their
src dir first.

> > This changes include directives for all generated files, across the
> > tree. The idea is to avoid sending a huge amount of email.  But when
> > merging, the changes will be split with one commit per file, e.g. for
> > ease of bisect in case of build failures, and to ease merging.
> > 
> > Note that should some generated files be missed by this tree-wide
> > refactoring, it isn't a big deal - this merely maintains the status quo,
> > and this can be addressed by a separate patch on top.
> > 
> > Signed-off-by: Michael S. Tsirkin 
> 
> I think your idea conflicts with what Markus has started to do:

Yes, I don't think we should revert what Markus started.   Both ways of
referencing QEMU headers have downsides, but I think "..." has fewer
downsides that "<">.

The problem Michael is tackling should be pretty rare, because moist
developers aren't frequently switching between srcdir==builddir and
srcdir!=biulddir setups - they have their preference for which to use
and stick with it. As long as we get ./configure to warn about the
dirty srcdir it should be good enough

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

Re: [Xen-devel] [PATCH 7/7] x86: add iommu_ops to map and unmap pages, and also to flush the IOTLB

[Jan Beulich]

>>> On 20.03.18 at 10:32,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 20 March 2018 08:12
>> 
>> >>> On 19.03.18 at 17:57,  wrote:
>> > What I meant was that safety (against underflow) is predicated on
>> > iommu_lookup_page() failing if the mapping was not established through
>> an
>> > iommu op hypercall. So, the only things that should be valid in the iommu
>> > (and hence that iommu_lookup_page() would succeed for) at the point
>> where the
>> > guest starts to boot must all fall within reserved regions, so thay they 
>> > are
>> > ruled out by the earlier check.
>> 
>> Ah, I see. What I don't see is how you want to arrange for that.
>> The tool stack wouldn't know ahead of time whether the guest
>> wants to use the PV IOMMU interfaces, would it? IOW rather than
>> guaranteeing said state at start of guest, shouldn't you blow away
>> all non-special mappings the first time a PV IOMMU request is made?
>> 
> 
> I suspect we want both. Kevin suggested a 'big switch' when the domain 
> boots, in which I could blow away all non-reserved mappings. But, for 
> performance sake, I think it would also be worth a Xen command line option to 
> avoid populating the IOMMU mappings for dom0 in the first place (so when it 
> pulls the 'big switch' it's a no-op). Non-aware dom0s will, of course, 
> probably 
> fail to boot but whoever is setting the command line for Xen should know what 
> their dom0 is capable of. As for other domains, it may be worth adding a 
> similar domain create option to the toolstack but that could be done at a 
> later date.

Oh, yes, options to avoid the entire teardown are certainly going
to be a good thing to have.

Jan


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

Re: [Xen-devel] [PATCH v3] new config option vtsc_tolerance_khz to avoid TSC emulation

[Olaf Hering]

So, how do we address the performance regression fixed by this patch?

Olaf

Am Mon, 12 Mar 2018 10:15:08 +0100
schrieb Olaf Hering :

> Am Tue, 6 Mar 2018 11:07:54 +
> schrieb Andrew Cooper :
> 
> > > Add a new domctl XEN_DOMCTL_set_vtsc_tolerance_khz to adjust the
> > > tolerance value of a running domU that is supposed to be migrated.
> > Please can we not proliferate the domctls.
> > This looks like it should be part of the set_tsc_info hypercall, not a
> > separate hypercall.  
> 
> How should this approach be implemented? To me it looks like set_tsc_info
> should be called only once for a not-yet-active domU, but I'm not familiar
> with timekeeping.
> If it can be just called once, and there should be a way to adjust the 
> tolerance value, then XEN_DOMCTL_settscinfo would need to be changed to 
> have subcommands.


pgpXHgJRsm8RL.pgp
Description: Digitale Signatur von OpenPGP
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Peter Maydell]

On 20 March 2018 at 09:44, Daniel P. Berrangé  wrote:
> We can follow what autoconf does, and add a check to configure to see if
> there are generated files left in the source dir, when configuring with
> builddir != srcdir, and exit with error, telling user to clean their
> src dir first.

We already do this in our makefile...it just doesn't check every
single generated file.

thanks
-- PMM

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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Jan Beulich]

>>> On 15.03.18 at 13:07,  wrote:
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -2448,27 +2448,24 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs *regs,
>  break;
>  case EXIT_REASON_CR_ACCESS:
>  {
> -unsigned long exit_qualification;
> -int cr, write;
> +cr_access_qual_t qual;
>  u32 mask = 0;
>  
> -__vmread(EXIT_QUALIFICATION, _qualification);
> -cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
> -write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
> +__vmread(EXIT_QUALIFICATION, );
>  /* also according to guest exec_control */
>  ctrl = __n2_exec_control(v);
>  
> -if ( cr == 3 )
> +if ( qual.cr == 3 )
>  {
> -mask = write? CPU_BASED_CR3_STORE_EXITING:
> -  CPU_BASED_CR3_LOAD_EXITING;
> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
> +: CPU_BASED_CR3_LOAD_EXITING;

I realize the old code has the same problem, but is this correct?
Only type 1 is a read from the CR, types 0, 2, and 3 are writes.
At least have an assertion here that types 2 and 3 can't occur?

>  if ( ctrl & mask )
>  nvcpu->nv_vmexit_pending = 1;
>  }
> -else if ( cr == 8 )
> +else if ( qual.cr == 8 )
>  {
> -mask = write? CPU_BASED_CR8_STORE_EXITING:
> -  CPU_BASED_CR8_LOAD_EXITING;
> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
> +: CPU_BASED_CR3_LOAD_EXITING;

Copy-and-paste mistake (ought to be CR8 here).

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -232,18 +232,25 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc)
>  /*
>   * Exit Qualifications for MOV for Control Register Access
>   */
> - /* 3:0 - control register number (CRn) */
> -#define VMX_CONTROL_REG_ACCESS_NUM(eq)  ((eq) & 0xf)
> - /* 5:4 - access type (CR write, CR read, CLTS, LMSW) */
> -#define VMX_CONTROL_REG_ACCESS_TYPE(eq) (((eq) >> 4) & 0x3)
> -# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_TO_CR   0
> -# define VMX_CONTROL_REG_ACCESS_TYPE_MOV_FROM_CR 1
> -# define VMX_CONTROL_REG_ACCESS_TYPE_CLTS2
> -# define VMX_CONTROL_REG_ACCESS_TYPE_LMSW3
> - /* 11:8 - general purpose register operand */
> -#define VMX_CONTROL_REG_ACCESS_GPR(eq)  (((eq) >> 8) & 0xf)
> - /* 31:16 - LMSW source data */
> -#define VMX_CONTROL_REG_ACCESS_DATA(eq)  ((uint32_t)(eq) >> 16)
> +enum {
> +VMX_CR_ACCESS_TYPE_MOV_TO_CR,
> +VMX_CR_ACCESS_TYPE_MOV_FROM_CR,
> +VMX_CR_ACCESS_TYPE_CLTS,
> +VMX_CR_ACCESS_TYPE_LMSW,

The trailing comma is sort of pointless here with the field being
just 2 bits wide.

> +};
> +typedef union cr_access_qual {
> +unsigned long raw;
> +struct {
> +uint16_t cr:4,
> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
> + :1,
> + gpr:4,
> + :4;
> +uint16_t lmsw_data;
> +uint32_t :32;

Strictly speaking this doesn't belong here, as it doesn't exist for
32-bit VMX implementations.

Jan


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

Re: [Xen-devel] [PATCH 28/57] ARM: new VGIC: Add acccessor to new struct vgic_irq instance

[Andre Przywara]

Hi,

On 19/03/18 21:53, Julien Grall wrote:
> 
> 
> On 03/19/2018 05:32 PM, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>> On 06/03/18 18:13, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 05/03/18 16:03, Andre Przywara wrote:
 The new VGIC implementation centers around a struct vgic_irq instance
 per virtual IRQ.
 Provide a function to retrieve the right instance for a given IRQ
 number and (in case of private interrupts) the right VCPU.
 This also includes the corresponding put function, which does nothing
 for private interrupts and SPIs, but handles the ref-counting for LPIs.

 This is based on Linux commit 64a959d66e47, written by Christoffer
 Dall.

 Signed-off-by: Andre Przywara 
 ---
 Changelog RFC ... v1:
 - add kernel-doc comments to exported functions
 - adapt to previous changes (new_vgic.h, arch_vcpu member name)
 - use ASSERT_UNREACHABLE

    xen/arch/arm/vgic/vgic.c | 124
 +++
    xen/arch/arm/vgic/vgic.h |  41 
    2 files changed, 165 insertions(+)
    create mode 100644 xen/arch/arm/vgic/vgic.c
    create mode 100644 xen/arch/arm/vgic/vgic.h

 diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
 new file mode 100644
 index 00..ace30f78d0
 --- /dev/null
 +++ b/xen/arch/arm/vgic/vgic.c
 @@ -0,0 +1,124 @@
 +/*
 + * Copyright (C) 2015, 2016 ARM Ltd.
 + * Imported from Linux ("new" KVM VGIC) and heavily adapted to Xen.
 + *
 + * 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 "vgic.h"
 +
 +/*
 + * Iterate over the VM's list of mapped LPIs to find the one with a
 + * matching interrupt ID and return a reference to the IRQ structure.
 + */
 +static struct vgic_irq *vgic_get_lpi(struct domain *d, u32 intid)
 +{
 +    struct vgic_dist *dist = >arch.vgic;
 +    struct vgic_irq *irq = NULL;
 +
 +    spin_lock(>lpi_list_lock);
 +
 +    list_for_each_entry( irq, >lpi_list_head, lpi_list )
>>>
>>> I am still not a big fan of the list solution. Strictly speaking nobody
>>> is populating that list and likely going to be too slow in Xen case (I
>>> am thinking for the hardware domain). So I think I would prefer to see
>>> the LPI related code disappear for this cut. This could easily be added
>>> back as they are standalone.
>>
>> I was thinking about that, but dismissed the idea:
>> Considering LPIs as first class citizens is a crucial property of the
>> new VGIC and actually the main driver for its creation: the refcounting
>> is solely in for that purpose, and the per-IRQ data structure and its
>> lock are mainly driven by it. So removing the LPI support completely
>> will make the refcounting look very awkward (since useless and unused).
>> Consequently one would need to remove that as well. In the worst case we
>> run into issues with unused functions, like vgic_get_irq_kref().
>>
>> I already removed a lot of code from the KVM VGIC, which I feel we will
>> need back one day. So I really like to keep this one in, and be it just
>> for documentation how the refcounting is supposed to be used.
>> I will add a comment to this respect.
>> And I believe replacing the list_for_each_entry() with something more
>> sophisticated is the least of our problems later on.
> 
> Using a list for LPIs means the search is O(n). That search will be done
> each time you want to get the LPI information.
> 
> While in guest you may have limited LPIs, this will not be the case for
> the hardware domain. Indeed that domain has access to all devices and
> likely going to use a lot of MSIs.
> 
> So I still can't understand why you dismiss that problem.

I think you got me wrong. I am happy with eventually using another data
structure, but want to keep the current list stub in as documentation,
as it motivates a crucial parts of the VGIC, namely refcounting and the
possible dynamic allocation of struct vgic_irq's. If that would be left
out, anyone reading the code would seriously scratch their head why we
have all this code, possibly removing it.
Actually the list we are talking about consists of a root pointer and
this single "list_for_each_entry()" call in 

Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

[Jan Beulich]

>>> On 16.03.18 at 14:30,  wrote:
> +int vpci_msix_arch_print(const struct vpci_msix *msix)
> +{
> +unsigned int i;
> +
> +for ( i = 0; i < msix->max_entries; i++ )
> +{
> +const struct vpci_msix_entry *entry = >entries[i];
> +
> +printk("%6u vec=%02x%7s%6s%3sassert%5s%7s dest_id=%lu mask=%u pirq: 
> %d\n",
> +   i, MASK_EXTR(entry->data, MSI_DATA_VECTOR_MASK),
> +   entry->data & MSI_DATA_DELIVERY_LOWPRI ? "lowest" : "fixed",
> +   entry->data & MSI_DATA_TRIGGER_LEVEL ? "level" : "edge",
> +   entry->data & MSI_DATA_LEVEL_ASSERT ? "" : "de",
> +   entry->addr & MSI_ADDR_DESTMODE_LOGIC ? "log" : "phys",
> +   entry->addr & MSI_ADDR_REDIRECTION_LOWPRI ? "lowest" : 
> "fixed",
> +   MASK_EXTR(entry->addr, MSI_ADDR_DEST_ID_MASK),
> +   entry->masked, entry->arch.pirq);
> +if ( i && !(i % 64) )
> +{
> +struct pci_dev *pdev = msix->pdev;
> +
> +spin_unlock(>pdev->vpci->lock);
> +process_pending_softirqs();
> +/* NB: we assume that pdev cannot go away for an alive domain. */
> +if ( !pdev->vpci || !spin_trylock(>vpci->lock) )
> +return -EBUSY;
> +msix = pdev->vpci->msix;

I disagree with resuming with a potentially changed msix here: This
can only lead to confusion of the consumer of the produced output.

> @@ -231,6 +232,23 @@ static int modify_bars(const struct pci_dev *pdev, bool 
> map, bool rom_only)
>  }
>  }
>  
> +/* Remove any MSIX regions if present. */
> +for ( i = 0; msix && i < ARRAY_SIZE(msix->tables); i++ )
> +{
> +paddr_t start = vmsix_table_addr(pdev->vpci, i);
> +paddr_t end = start + vmsix_table_size(pdev->vpci, i) - 1;
> +
> +rc = rangeset_remove_range(mem, PFN_DOWN(start), PFN_DOWN(end));
> +if ( rc )
> +{
> +printk(XENLOG_G_WARNING
> +   "Failed to remove MSIX table [%" PRI_gfn ", %" PRI_gfn 
> "]: %d\n",
> +   PFN_DOWN(start), PFN_DOWN(end), rc);

In cases like this (where you don't use plain start/end anywhere,
but you do use the same calculation on them twice each), it's
certainly more efficient for the local variables to be frame numbers
right away.

Considering that I didn't notice this earlier, I won't insist on the
latter change to be made, i.e. with at least the former issue
addressed
Reviewed-by: Jan Beulich 

Jan


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

Re: [Xen-devel] [PATCH v5 04/16] xen/arm: mm: Remove unused M2P code

[Julien Grall]

Hi George,

On 03/15/2018 04:25 PM, George Dunlap wrote:

On Wed, Mar 14, 2018 at 6:19 PM,   wrote:

From: Julien Grall 

Arm does not have an M2P and very unlikely to get one in the future,
therefore don't keep defines that are not necessary in the common code.

At the same time move the remaining M2P define just above just above
set_gpfn_from_mfn to keep all the dummy helpers for M2P together.

Signed-off-by: Julien Grall 

---

Cc: Stefano Stabellini 

 Changes in v4:
 - Patch added.
---
  xen/include/asm-arm/mm.h | 25 -
  1 file changed, 4 insertions(+), 21 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 4d5563b0ce..c03f4ad674 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -320,33 +320,16 @@ static inline void *page_to_virt(const struct page_info 
*pg)
  struct page_info *get_page_from_gva(struct vcpu *v, vaddr_t va,
  unsigned long flags);

-/*
- * The MPT (machine->physical mapping table) is an array of word-sized
- * values, indexed on machine frame number. It is expected that guest OSes
- * will use it to store a "physical" frame number to give the appearance of
- * contiguous (or near contiguous) physical memory.
- */
-#undef  machine_to_phys_mapping
-#define machine_to_phys_mapping  ((unsigned long *)RDWR_MPT_VIRT_START)
-#define INVALID_M2P_ENTRY(~0UL)
-#define VALID_M2P(_e)(!((_e) & (1UL<<(BITS_PER_LONG-1
-#define SHARED_M2P_ENTRY (~0UL - 1UL)
-#define SHARED_M2P(_e)   ((_e) == SHARED_M2P_ENTRY)
-
-#define _set_gpfn_from_mfn(mfn, pfn) ({\
-struct domain *d = page_get_owner(__mfn_to_page(mfn)); \
-if(d && (d == dom_cow))\
-machine_to_phys_mapping[(mfn)] = SHARED_M2P_ENTRY; \
-else   \
-machine_to_phys_mapping[(mfn)] = (pfn);\
-})
-
  static inline void put_gfn(struct domain *d, unsigned long gfn) {}
  static inline int relinquish_shared_pages(struct domain *d)
  {
  return 0;
  }

+#define INVALID_M2P_ENTRY(~0UL)
+#define SHARED_M2P_ENTRY (~0UL - 1UL)
+#define SHARED_M2P(_e)   ((_e) == SHARED_M2P_ENTRY)


I think I might add a comment here like this:

"ARM doesn't have an M2P, but common code expects a handful of
M2P-related defines and functions.  Provide dummy versions of these."


I will add that in the next version.



Other than that looks good:

Reviewed-by: George Dunlap 


Thank you!

Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC PATCH 06/12] hvmloader: add basic Q35 support

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 09:44:33AM +1000, Alexey G wrote:
> On Mon, 19 Mar 2018 15:30:14 +
> Roger Pau Monné  wrote:
> 
> >On Tue, Mar 13, 2018 at 04:33:51AM +1000, Alexey Gerasimenko wrote:
> >> +{
> >> +case 0x0300:  
> >
> >All this values need to be defines documented somewhere.
> 
> Agree... although it was not me who introduced all these hardcoded PCI
> class values. :) I'll change these numbers into newly added pci_regs.h
> #defines in the non-functional patch.

Right. I've realized that later. If you place this code moment in a
separate patch without any other modifications I won't complain about
the lack of defines (although it would be nice to have them :)).

> >> +{
> >> +pci_writeb(PCI_ICH9_LPC_DEVFN, 0x60 + link, isa_irq);
> >> +
> >> +/* PIRQE..PIRQH are unused */
> >> +pci_writeb(PCI_ICH9_LPC_DEVFN, 0x68 + link, 0x80);  
> >
> >According to the spec 0x80 is the default value for this registers, do
> >you really need to write it?
> >
> >Is maybe QEMU not correctly setting the default value?
> 
> Won't agree here. We're initializing PIRQ[n] routing in this
> fragment, it's better not to rely on any values but simply initialize
> all PIRQ[n]_ROUT registers, this makes it explicit.
> 
> Even if it is unnecessary due to defaults it's more obvious to set
> these registers to our own values than to force a reader to either look
> up their emulation in QEMU code or read the ICH9 pdf to confirm
> assumptions.

But if you start doing this, you should do it for all the registers.
Why is PIRQE..PIRQH routing special that you need to re-write the
default value? But not SIRQ_CNTL for example?

I think a comment noting that the default value for those registers is
what we expect (0x80 - Interrupt Routing Disabled) would be better.

Thanks, Roger.

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

Re: [Xen-devel] TLB flushing

[Jan Beulich]

>>> On 20.03.18 at 09:50,  wrote:
> While hunting a strange bug in my PCID patch series hinting at some
> TLB invalidation problem I discovered a piece of code looking rather
> fishy to me.
> 
> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
> of FLUSH_TLB_GLOBAL?
> 
> While not being a problem in current code as both will flush all TLB
> entries my series will change that by using invpcid to flush only the
> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
> 
> I can send a patch if anyone can confirm that using FLUSH_TLB only is
> wrong.

I think this shouldn't be a separate patch, but an integral part of the
one introducing the distinction between "all" and non-global flushes.
This is because
- right now it doesn't make a difference (we do "all" flushes anyway),
- back in the 32-bit days it didn't matter because guest mappings
  would never have been allowed to be global, and transient Xen
  mappings also would never have had the G bit set.
IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
this would need to become FLUSH_TLB_GLOBAL at the point the
kind of flush gets altered, while without it could remain at FLUSH_TLB.
Perhaps it is worthwhile to retain this distinction just for
documentation purposes (in case a future change wants to turn off
that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).

Jan


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

Re: [Xen-devel] TLB flushing

[Juergen Gross]

On 20/03/18 10:19, Jan Beulich wrote:
 On 20.03.18 at 09:50,  wrote:
>> While hunting a strange bug in my PCID patch series hinting at some
>> TLB invalidation problem I discovered a piece of code looking rather
>> fishy to me.
>>
>> Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
>> of FLUSH_TLB_GLOBAL?
>>
>> While not being a problem in current code as both will flush all TLB
>> entries my series will change that by using invpcid to flush only the
>> non-global entries if FLUSH_TLB_GLOBAL wasn't set.
>>
>> I can send a patch if anyone can confirm that using FLUSH_TLB only is
>> wrong.
> 
> I think this shouldn't be a separate patch, but an integral part of the
> one introducing the distinction between "all" and non-global flushes.
> This is because
> - right now it doesn't make a difference (we do "all" flushes anyway),
> - back in the 32-bit days it didn't matter because guest mappings
>   would never have been allowed to be global, and transient Xen
>   mappings also would never have had the G bit set.
> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
> this would need to become FLUSH_TLB_GLOBAL at the point the
> kind of flush gets altered, while without it could remain at FLUSH_TLB.

Really? Aren't global hypervisor mappings affected by this, too?

> Perhaps it is worthwhile to retain this distinction just for
> documentation purposes (in case a future change wants to turn off
> that USER_MAPPINGS_ARE_GLOBAL behavior for whatever reason).

I think as long as FLUSH_TLB_GLOBAL is being used in the code
new_tlbflush_clock_period() should do so, too. In case there is no need
to do TLB flushes including global pages, FLUSH_TLB_GLOBAL can be
modified to do only non-global flushing (with a comment explaining why
this is secure).


Juergen

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

[Xen-devel] [PATCH v2] hvm/svm: Implement Debug events

[Alexandru Isaila]

At this moment the Debug events for the AMD architecture are not
forwarded to the monitor layer.

This patch adds the Debug event to the common capabilities, adds
the VMEXIT_ICEBP then forwards the event to the monitor layer.

Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1
exception generated by the single byte INT1
instruction (also known as ICEBP) does not trigger the #DB
intercept. Software should use the dedicated ICEBP
intercept to intercept ICEBP"

---
Changes since V1:
- Get inst_len from __get_instruction_length()
- Updated __get_instruction_length() for the INSTR_ICEBP
  instruction

Signed-off-by: Alexandru Isaila 
---
 xen/arch/x86/hvm/svm/emulate.c|  1 +
 xen/arch/x86/hvm/svm/svm.c| 37 +--
 xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
 xen/include/asm-x86/hvm/svm/emulate.h |  1 +
 xen/include/asm-x86/monitor.h |  4 ++--
 5 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
index e1a1581..172369e 100644
--- a/xen/arch/x86/hvm/svm/emulate.c
+++ b/xen/arch/x86/hvm/svm/emulate.c
@@ -80,6 +80,7 @@ static const struct {
 [INSTR_RDTSC]   = { X86EMUL_OPC(0x0f, 0x31) },
 [INSTR_RDMSR]   = { X86EMUL_OPC(0x0f, 0x32) },
 [INSTR_CPUID]   = { X86EMUL_OPC(0x0f, 0xa2) },
+[INSTR_ICEBP]   = { X86EMUL_OPC(   0, 0xf1) },
 };
 
 int __get_instruction_length_from_list(struct vcpu *v,
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c34f5b5..d4f2290 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v)
 {
 struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 bool debug_state = (v->domain->debugger_attached ||
-v->domain->arch.monitor.software_breakpoint_enabled);
+v->domain->arch.monitor.software_breakpoint_enabled ||
+v->domain->arch.monitor.debug_exception_enabled);
 bool_t vcpu_guestmode = 0;
 struct vlapic *vlapic = vcpu_vlapic(v);
 
@@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, 
struct x86_event *info)
 return true;
 }
 
-static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len)
+static void svm_propagate_intr(unsigned long insn_len, int16_t vector, uint8_t 
type)
 {
-struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 struct x86_event event = {
-.vector = vmcb->eventinj.fields.type,
-.type = vmcb->eventinj.fields.type,
-.error_code = vmcb->exitinfo1,
+.vector = vector,
+.type = type,
+.error_code = X86_EVENT_NO_EC,
+.insn_len = insn_len,
 };
 
-event.insn_len = insn_len;
 hvm_inject_event();
 }
 
@@ -2655,10 +2655,27 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
 /* Asynchronous event, handled when we STGI'd after the VMEXIT. */
 HVMTRACE_0D(SMI);
 break;
-
+case VMEXIT_ICEBP:
 case VMEXIT_EXCEPTION_DB:
 if ( !v->domain->debugger_attached )
-hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
+{
+int rc;
+unsigned long trap_type = exit_reason == VMEXIT_ICEBP ?
+X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION;
+
+inst_len = 0;
+
+if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
+inst_len = __get_instruction_length(v, INSTR_ICEBP);
+
+rc = hvm_monitor_debug(regs->rip,
+   HVM_MONITOR_DEBUG_EXCEPTION,
+   trap_type, inst_len);
+if ( rc < 0 )
+goto unexpected_exit_type;
+if ( !rc )
+svm_propagate_intr(inst_len, TRAP_debug, trap_type);
+}
 else
 domain_pause_for_debugger();
 break;
@@ -2687,7 +2704,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
if ( rc < 0 )
goto unexpected_exit_type;
if ( !rc )
-   svm_propagate_intr(v, inst_len);
+   svm_propagate_intr(inst_len, TRAP_int3, 
X86_EVENTTYPE_SW_EXCEPTION);
 }
 break;
 
diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index ae60d8d..06920d3 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -73,7 +73,7 @@ static int construct_vmcb(struct vcpu *v)
 GENERAL2_INTERCEPT_STGI| GENERAL2_INTERCEPT_CLGI|
 GENERAL2_INTERCEPT_SKINIT  | GENERAL2_INTERCEPT_MWAIT   |
 GENERAL2_INTERCEPT_WBINVD  | GENERAL2_INTERCEPT_MONITOR |
-GENERAL2_INTERCEPT_XSETBV;
+GENERAL2_INTERCEPT_XSETBV  | GENERAL2_INTERCEPT_ICEBP;
 
 /* Intercept all debug-register writes. */
 vmcb->_dr_intercepts = ~0u;
diff --git 

Re: [Xen-devel] [PATCH 1/3] x86/alternatives: fully leverage automatic NOP filling

[Jan Beulich]

>>> On 15.03.18 at 13:43,  wrote:
> On 14/03/18 08:29, Jan Beulich wrote:
>> As of commit 4008c71d7a ("x86/alt: Support for automatic padding
>> calculations") there's no point having explict ASM_NOPn instances in
>> alternatives anymore - drop them. As a result also drop the asm/nops.h
>> inclusion from alternative.h, adding explicit inclusions in the two
>> remaining C files needing them.
>>
>> While touching it also move the CR4_PV32_RESTORE definition out of the
>> SMAP-specific conditional into a more general one.
>>
>> Signed-off-by: Jan Beulich 
> 
> I had considered doing this, but decided against it.  At the moment, the
> majority of hardware Xen runs on (being Intel pre-broadwell) doesn't
> need to touch the STAC/CLAC patch sites, and this change means we will
> always touch those sites.
> 
> OTOH, (following up from later discussion) doing this does mean that we
> would end up replacing with K8 nops when appropriate.
> 
> I'd like the toolchain-nops change to go in first, so we don't regress
> the majority status quo, but I have no other issues with this change.

So what are the perspectives of this happening, not the least in
light of our disagreement on the usefulness of that change? I
continue to be unconvinced of the "avoid patching whenever
possible" argument - if patching is broken, we're hosed anyway.
I don't think there's a common configuration nowadays in which
no patching whatsoever would be happening (and the amount
of patching we do is going to grow anyway).

Jan


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

Re: [Xen-devel] [PATCH 11/20] xen/domctl: Merge set_gnttab_limits into createdomain

[Andrew Cooper]

On 19/03/2018 21:43, Christian Lindig wrote:
>
>> On 19. Mar 2018, at 19:13, Andrew Cooper  wrote:
>>
>> +max_grant_frames: int32;
>> +max_maptrack_frames: int32;
> As part of:
>
>> +type domctl_create_config =
>> +{
>> +   ssidref: int32;
>> +   handle: string;
>> +   flags: domain_create_flag list;
>> +   max_vcpus: int32;
>> +   max_evtchn_port: int32;
>> +   max_grant_frames: int32;
>> +   max_maptrack_frames: int32;
>> +   arch: arch_domainconfig;
>> +}
> This is a minor point: in OCaml, values of type int32 and int64 are 
> represented as pointers to a memory block containing the value. This is 
> unlike an int, which is represented simply as part of a memory block that 
> represents the record value. Because OCaml uses one bit as a tag, the range 
> of int and int32 (on a 32-bit system) is different. Most of the time the 
> range is large enough and people use int rather than int32 or int64 for that 
> reason. I would expect that an int is large enough, especially on a 64-bit 
> system. However, if int32 makes it easier to align this with the C code, this 
> is fine, especially because there are not many values of omctl_create_config 
> to be expected.

I'm aware of of in32/64 being properly tagged objects, and did consider
whether they were the sensible value to use here.  I sided with the
ssidref type for simplicity.

That said, while ssidref might plausibly need a full 32 bits of range
(and even then, I'm not entirely sure, but it is an opaque handle at the
end of the day), none of the max_* fields do.  vcpus is currently capped
at 128, grant frames at 64, maptrack frames at 1024 (both configurable
on the Xen command line).  evtchn_port is effectively capped at the ABI
limit which is 1024/4096 or 2^27 depending on the guests setup.

I'll swap all the max_* fields to being a plain int.

~Andrew

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

Re: [Xen-devel] [PATCH v10 01/11] vpci: introduce basic handlers to trap accesses to the PCI config space

[Jan Beulich]

>>> On 16.03.18 at 14:29,  wrote:
> This functionality is going to reside in vpci.c (and the corresponding
> vpci.h header), and should be arch-agnostic. The handlers introduced
> in this patch setup the basic functionality required in order to trap
> accesses to the PCI config space, and allow decoding the address and
> finding the corresponding handler that should handle the access
> (although no handlers are implemented).
> 
> Note that the traps to the PCI IO ports registers (0xcf8/0xcfc) are
> setup inside of a x86 HVM file, since that's not shared with other
> arches.
> 
> A new XEN_X86_EMU_VPCI x86 domain flag is added in order to signal Xen
> whether a domain should use the newly introduced vPCI handlers, this
> is only enabled for PVH Dom0 at the moment.
> 
> A very simple user-space test is also provided, so that the basic
> functionality of the vPCI traps can be asserted. This has been proven
> quite helpful during development, since the logic to handle partial
> accesses or accesses that expand across multiple registers is not
> trivial.
> 
> The handlers for the registers are added to a linked list that's keep
> sorted at all times. Both the read and write handlers support accesses
> that expand across multiple emulated registers and contain gaps not
> emulated.
> 
> Signed-off-by: Roger Pau Monné 
> Reviewed-by: Jan Beulich 
> [IO parts]
> Reviewed-by: Paul Durrant 

Stefano, Julien,

any chance of getting an ack for the smallish ARM side change
here (assuming your earlier concerns have been addressed)?

Ian, Wei,

along those lines (iirc there were no prior concerns) for the tiny
libxl part? (I think the new test code doesn't strictly belong under
tools/ maintainership, just like the x86_emulator one doesn't.)

I'd really like to get the first half of this series in.

Thanks, Jan

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

Re: [Xen-devel] [PATCH RESEND v2 1/2] drm/xen-front: Add support for Xen PV display frontend

[Daniel Vetter]

On Tue, Mar 20, 2018 at 01:58:01PM +0200, Oleksandr Andrushchenko wrote:
> On 03/19/2018 05:28 PM, Daniel Vetter wrote:
> > There should be no difference between immediate removal and delayed
> > removal of the drm_device from the xenbus pov. The lifetimes of the
> > front-end (drm_device) and backend (the xen bus thing) are entirely
> > decoupled:
> Well, they are not decoupled for simplicity of handling,
> please see below
> > 
> > So for case 2 you only have 1 case:
> > 
> > - drm_dev_unplug
> > - tear down the entire xenbus backend completely
> > - all xenbus access will be caught with drm_dev_entre/exit (well right
> > now drm_dev_is_unplugged) checks, including any access to your private
> > drm_device data
> > - once drm_device->open_count == 0 the core will tear down the
> > drm_device instance and call your optional drm_driver->release
> > callback.
> > 
> > So past drm_dev_unplug the drm_device is in zombie state and the only
> > thing that will happen is a) it rejects all ioctls and anything else
> > userspace might ask it to do and b) gets releases once the last
> > userspace reference is gone.
> I have re-worked the driver with this in mind [1]
> So, I now use drm_dev_unplug and destroy the DRM device
> on drm_driver.release.
> In context of unplug work I also merged xen_drm_front_drv.c and
> xen_drm_front.c as these are too coupled together now.
> 
> Could you please take a look and tell me if this is what you mean?
> > 
> > If the backend comes up again, you create a _new_ drm_device instance
> > (while the other one is still in the process of eventually getting
> > released).
> We only have a single xenbus instance, so this way I'll need
> to handle list of such zombies. For that reason I prefer to
> wait until the DRM device is destroyed, telling the backend
> to hold on until then (via going into XenbusStateReconfiguring state).

Why exactly do you need to keep track of your drm_devices from the xenbus?
Once unplugged, there should be no connection with the "hw" for your
device, in neither direction. Maybe I need to look again, but this still
smells funny and not like something you should ever do.

> Another drawback of such approach is that I'll have different
> minors at run-time, e.g. card0, card1, etc.
> For software which has /dev/dri/card0 hardcoded it may be a problem.
> But this is minor, IMO

Fix userspace :-)

But yeah unlikely this is a problem, hotplugging is fairly old thing.

> > In short, your driver code should never have a need to look at
> > drm_device->open_count. I hope this explains it a bit better.
> > -Daniel
> > 
> Yes, you are correct: at [1] I am not touching drm_device->open_count
> anymore and everything just happens synchronously

> [1] https://github.com/andr2000/linux/commits/drm_tip_pv_drm_v3

Please just resend, makes it easier to comment inline.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 02:46:46PM +0100, Thomas Huth wrote:
> On 20.03.2018 14:32, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >>> So for these, we should use "".  None of these are generated files though.
> >>
> >> That leads to crazy inconsistent message for developers where 50% of QEMU
> >> header files must use <> and the other 50% of header files must use "".
> > 
> > The rules are pretty simple though:
> > 
> >(1) Headers which are generated use <>.
> >(2) Headers which are in include/ use <>.
> >(3) Headers sitting in the same directory as the source files use "".
> 
> Ugh, no. Please don't. The normal way of including header files in QEMU

Stress on "in QEMU".

> is to use "" - also for headers that are not in the same directory. For
> example just do a
> 
>  grep -r '^#include.*hw/' hw/
> 
> from the top directory and you'll see what I mean. Changing that rule is
> crazy.
> So please, let's just fix the configure script to detect some
> more stale files in the source tree, and we're done.
> 
>  Thomas

The rule probably served a useful purpose when all
headers where in the same directory as the source.
we moved them out so that is no longer the case.

-- 
MST

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

Re: [Xen-devel] Spectre Mitigations in Xen 4.6

[Jan Beulich]

>>> On 20.03.18 at 13:58,  wrote:
> I've been experimenting with Linux 4.14 on Xen 4.6.  Now that Intel
> microcode is generally
> available, I'm starting to exercise the new mitigation code paths.
> 
> For Xen 4.6-4.8, microcode loading happens after
> init_speculation_mitigations, so Xen only
> detects the boot firmware features.  The early microcode loading
> f97838bbd980 ("x86: Move
> microcode loading earlier") can be cherry-picked, though small fix ups
> are needed for
> bool/true/false -> bool_t/1/0 and smpboot.c:smp_store_cpu_info() to
> retain "struct
> cpuinfo_x86 *c = cpu_data + id;".

Oh, I see - yes, I'll need to pull that in. I didn't notice it's missing
because the test box already had suitable microcode (by way of a
BIOS update done in January).

> With that in place, I'm seeing Dom0 receive a general protection fault on 
> boot
> 
> [   25.460035] general protection fault:  [#1] SMP
> [   25.460292] EIP: switch_mm_irqs_off+0xbe/0x600
> 
> switch_mm_irqs_off+0xbe is the inlined
> indirect_branch_prediction_barrier(void)
> {
> alternative_msr_write(MSR_IA32_PRED_CMD, PRED_CMD_IBPB,
>   X86_FEATURE_USE_IBPB);
> }
> 
> The system boots when dom0 disables IBPB manipulation with
> nospectre_v2 on the kernel
> command line.
> 
> I think Xen ends up here in xen/arch/x86/traps.c:emulate_privileged_op(),
> case MSR_PRED_CMD:
> domain_cpuid(currd, 7, 0, , , , );
> domain_cpuid(currd, 0x8008, 0, , , , );
> if ( !(edx & cpufeat_mask(X86_FEATURE_IBRSB)) &&
>  !(ebx & cpufeat_mask(X86_FEATURE_IBPB)) )
> goto fail; /* MSR available? */
> 
> /*
>  * The only defined behaviour is when writing PRED_CMD_IBPB.  In
>  * practice, real hardware accepts any value without faulting.
>  */
> if ( eax & PRED_CMD_IBPB )
> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
> break;
> 
> ...but Dom0 doesn't have a cpuid policy configured, so the IBRSB/IBPB
> check fails and we GP.
> Did I read that correctly?  If that is the case, how should Dom0 be handled?

Hmm, using pv_cpuid() instead isn't really a good option, as that
wants struct cpu_user_regs passed in. So I guess we'll have to
mimic what pv_cpuid() does for Dom0 in the priv-op emulation
code. I'll cook up a patch...

Jan


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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Jan Beulich]

>>> On 20.03.18 at 15:28,  wrote:
> On 20/03/18 10:51, Jan Beulich wrote:
> On 15.03.18 at 13:07,  wrote:
>>> +typedef union cr_access_qual {
>>> +unsigned long raw;
>>> +struct {
>>> +uint16_t cr:4,
>>> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
>>> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
>>> + :1,
>>> + gpr:4,
>>> + :4;
>>> +uint16_t lmsw_data;
>>> +uint32_t :32;
>> Strictly speaking this doesn't belong here, as it doesn't exist for
>> 32-bit VMX implementations.
> 
> It is only here to keep clang happy.  See c/s ac6e7fd7a482

Oh, I didn't recall that.

> As an alternative, we could see about not using transparent unions, and
> explicitly casting in the function calls.

Let's rather not - transparent unions are quite nice an aid to avoid
casts.

Jan


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

Re: [Xen-devel] [PATCH v10 11/11] vpci/msix: add MSI-X handlers

[Roger Pau Monné]

On Tue, Mar 20, 2018 at 07:10:39AM -0600, Jan Beulich wrote:
> >>> On 20.03.18 at 12:43,  wrote:
> > Since you only had comments on patch 7 and 11 and there's the extra
> > fix for the test harness, should I just send those and provide you
> > with a git branch that contains the rest?
> 
> Well, if it's not too much hassle for you I'd prefer if you resent the
> full series (or, see below, whatever is left at the time you get
> around to send it).
> 
> > I can also wait if you want to commit the start of the series
> > (provided I get all the relevant Acks) and rebase on top of that.
> 
> I'd leave that up to you; it looks like both tool stack maintainers
> are not around right now, so their ack for patch 1 may take a few
> more days to arrive.

I guess I will resend the full series later so they can review the latest
version.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] x86/vtx: Introduce a typed union for CR access exit information

[Andrew Cooper]

On 20/03/18 10:51, Jan Beulich wrote:
 On 15.03.18 at 13:07,  wrote:
>> --- a/xen/arch/x86/hvm/vmx/vvmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
>> @@ -2448,27 +2448,24 @@ int nvmx_n2_vmexit_handler(struct cpu_user_regs 
>> *regs,
>>  break;
>>  case EXIT_REASON_CR_ACCESS:
>>  {
>> -unsigned long exit_qualification;
>> -int cr, write;
>> +cr_access_qual_t qual;
>>  u32 mask = 0;
>>  
>> -__vmread(EXIT_QUALIFICATION, _qualification);
>> -cr = VMX_CONTROL_REG_ACCESS_NUM(exit_qualification);
>> -write = VMX_CONTROL_REG_ACCESS_TYPE(exit_qualification);
>> +__vmread(EXIT_QUALIFICATION, );
>>  /* also according to guest exec_control */
>>  ctrl = __n2_exec_control(v);
>>  
>> -if ( cr == 3 )
>> +if ( qual.cr == 3 )
>>  {
>> -mask = write? CPU_BASED_CR3_STORE_EXITING:
>> -  CPU_BASED_CR3_LOAD_EXITING;
>> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
>> +: CPU_BASED_CR3_LOAD_EXITING;
> I realize the old code has the same problem, but is this correct?
> Only type 1 is a read from the CR, types 0, 2, and 3 are writes.
> At least have an assertion here that types 2 and 3 can't occur?

If we trust hardware not to give us junk here, then types 2 and 3 can't
occur.  I'll add an assert.

>
>>  if ( ctrl & mask )
>>  nvcpu->nv_vmexit_pending = 1;
>>  }
>> -else if ( cr == 8 )
>> +else if ( qual.cr == 8 )
>>  {
>> -mask = write? CPU_BASED_CR8_STORE_EXITING:
>> -  CPU_BASED_CR8_LOAD_EXITING;
>> +mask = qual.access_type ? CPU_BASED_CR3_STORE_EXITING
>> +: CPU_BASED_CR3_LOAD_EXITING;
> Copy-and-paste mistake (ought to be CR8 here).

Oops.

>
>> +};
>> +typedef union cr_access_qual {
>> +unsigned long raw;
>> +struct {
>> +uint16_t cr:4,
>> + access_type:2,  /* VMX_CR_ACCESS_TYPE_* */
>> + lmsw_op_type:1, /* 0 => reg, 1 => mem   */
>> + :1,
>> + gpr:4,
>> + :4;
>> +uint16_t lmsw_data;
>> +uint32_t :32;
> Strictly speaking this doesn't belong here, as it doesn't exist for
> 32-bit VMX implementations.

It is only here to keep clang happy.  See c/s ac6e7fd7a482

As an alternative, we could see about not using transparent unions, and
explicitly casting in the function calls.

~Andrew

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

Re: [Xen-devel] [PATCH] xsm:schedop: introduce vcpuinfo permissions verification

[Daniel De Graaf]

On 02/15/2018 05:51 AM, Andrii Anisov wrote:

From: Andrii Anisov 

Introduce per-vcpu scheduler operations permission verification.
As long as Xvcpuinfo are in fact scheduler configuration manipulations
there is no need to introduce specific access vectors.

Signed-off-by: Andrii Anisov 


Acked-by: Daniel De Graaf 

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Thomas Huth]

On 20.03.2018 14:32, Gerd Hoffmann wrote:
>   Hi,
> 
>>> So for these, we should use "".  None of these are generated files though.
>>
>> That leads to crazy inconsistent message for developers where 50% of QEMU
>> header files must use <> and the other 50% of header files must use "".
> 
> The rules are pretty simple though:
> 
>(1) Headers which are generated use <>.
>(2) Headers which are in include/ use <>.
>(3) Headers sitting in the same directory as the source files use "".

Ugh, no. Please don't. The normal way of including header files in QEMU
is to use "" - also for headers that are not in the same directory. For
example just do a

 grep -r '^#include.*hw/' hw/

from the top directory and you'll see what I mean. Changing that rule is
crazy. So please, let's just fix the configure script to detect some
more stale files in the source tree, and we're done.

 Thomas

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Max Reitz]

On 2018-03-20 14:41, Daniel P. Berrangé wrote:
> On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
 So for these, we should use "".  None of these are generated files though.
>>>
>>> That leads to crazy inconsistent message for developers where 50% of QEMU
>>> header files must use <> and the other 50% of header files must use "".
>>
>> The rules are pretty simple though:
>>
>>(1) Headers which are generated use <>.
>>(2) Headers which are in include/ use <>.
>>(3) Headers sitting in the same directory as the source files use "".
> 
> We have 1200 header files in QEMU - a developer might just reasonably remember
> which header file is a QEMU header, vs which is a 3rd party system header.
> Expecting devs to remember which of those 3 buckets each QEMU header falls
> under is unreasonable IMHO. 

I agree with this in principle, but I don't find it unreasonable for
someone to look up whether a header file exists in the same directory.
I find that much simpler than to look up whether it is a system header,
actually.

And I have to agree with Michael that his rule when to use "" (if the
file is in the same directory) and when to use <> (otherwise) is
actually what every C developer might do by instinct.  (I guess it's
also easier to check in a script than the original guideline?)

However, I also think that if any problem arises because "" was used for
a generated file and that then uses a stale file, the bug is somewhere
else.  And I think that while Michael's proposal is the more intuitive
(C) way, it is a tiny bit more complicated to explain than 'Use "" for
qemu headers, <> for everything else'.

But I guess the main advantage with using this rule I see is that it's
better for people reading the code.  It's just nice to know whether a
file belongs to qemu or not by just looking at the #include statement.
(Note that this implies that it is indeed more difficult to determine
whether a header belongs to qemu than whether it sits in the same
directory as the C file, though!)  So I think the old (current) rule is
better for reading code, Michael's rule would be better for writing
code.  I think reading code is what should be easier.

But since that may be eaten up by build breakages due to stale files, I
don't have a strong opinion either way.  I just wanted to chime in
because in my opinion 'Use "" for headers in the same directory, <> for
everything else' is by no means an unreasonably complicated rule for
people writing code.

Max



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] TLB flushing

[Juergen Gross]

On 20/03/18 10:56, Jan Beulich wrote:
 On 20.03.18 at 10:29,  wrote:
>> On 20/03/18 10:19, Jan Beulich wrote:
>> On 20.03.18 at 09:50,  wrote:
 While hunting a strange bug in my PCID patch series hinting at some
 TLB invalidation problem I discovered a piece of code looking rather
 fishy to me.

 Is it correct for new_tlbflush_clock_period() to use FLUSH_TLB instead
 of FLUSH_TLB_GLOBAL?

 While not being a problem in current code as both will flush all TLB
 entries my series will change that by using invpcid to flush only the
 non-global entries if FLUSH_TLB_GLOBAL wasn't set.

 I can send a patch if anyone can confirm that using FLUSH_TLB only is
 wrong.
>>>
>>> I think this shouldn't be a separate patch, but an integral part of the
>>> one introducing the distinction between "all" and non-global flushes.
>>> This is because
>>> - right now it doesn't make a difference (we do "all" flushes anyway),
>>> - back in the 32-bit days it didn't matter because guest mappings
>>>   would never have been allowed to be global, and transient Xen
>>>   mappings also would never have had the G bit set.
>>> IOW with what used to be named USER_MAPPINGS_ARE_GLOBAL
>>> this would need to become FLUSH_TLB_GLOBAL at the point the
>>> kind of flush gets altered, while without it could remain at FLUSH_TLB.
>>
>> Really? Aren't global hypervisor mappings affected by this, too?
> 
> Yes and no. The timestamp here is needed to know whether to
> flush when a page gets recycled. As long as G is never set on
> guest controlled mappings for pages which may be recycled,
> there's no issue. In particular, the G bits in the 1:1 mappings are
> of no interest here (and in fact anything Xen maintains which no
> guest can control), as those mappings never go away (or if they
> did, e.g. when a page needs to be offlined for causing #MC, an
> explicit global flush for that page would be required).

I just verified that there is at least one problem in the hypervisor
TLB flushing logic: using invpcid it is possible to flush the non-global
entries only. If I do that in case of FLUSH_TLB_GLOBAL not being set I
get segfaults in user mode of the guest.


Juergen

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

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Daniel P . Berrangé]

On Tue, Mar 20, 2018 at 03:50:02PM +0200, Michael S. Tsirkin wrote:
> On Tue, Mar 20, 2018 at 01:41:17PM +, Daniel P. Berrangé wrote:
> > On Tue, Mar 20, 2018 at 02:32:16PM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > > So for these, we should use "".  None of these are generated files 
> > > > > though.
> > > > 
> > > > That leads to crazy inconsistent message for developers where 50% of 
> > > > QEMU
> > > > header files must use <> and the other 50% of header files must use "".
> > > 
> > > The rules are pretty simple though:
> > > 
> > >(1) Headers which are generated use <>.
> > >(2) Headers which are in include/ use <>.
> > >(3) Headers sitting in the same directory as the source files use "".
> > 
> > We have 1200 header files in QEMU - a developer might just reasonably 
> > remember
> > which header file is a QEMU header, vs which is a 3rd party system header.
> > Expecting devs to remember which of those 3 buckets each QEMU header falls
> > under is unreasonable IMHO. 
> 
> That's the C language for you though.  For better or worse, these are
> the rules that K came up with.

No one seriously tries to keep compliance with original K C these days,
things have moved on massively since then.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Xen-devel] [qemu-mainline test] 120947: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-qemuu-nested-intel 14 xen-boot/l1   fail REGR. vs. 120095
 test-amd64-amd64-qemuu-nested-amd 14 xen-boot/l1 fail REGR. vs. 120095

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-libvirt-xsm 14 saverestore-support-checkfail  like 120095
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 120095
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 120095
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 120095
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 120095
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 120095
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass
 test-amd64-amd64-xl-pvhv2-amd 12 guest-start  fail  never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-xsm  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass

version targeted for testing:
 qemuue1e44a9916b4318e943aecd669e096222cb3eaeb
baseline version:
 qemuu6697439794f72b3501ee16bb95d16854f9981421

Last test of basis   120095  2018-02-28 13:46:33 Z   19 days
Failing since120146  2018-03-02 10:10:57 Z   18 days   10 attempts
Testing same since   120947  2018-03-19 00:46:18 Z1 days1 attempts


People who touched revisions under test:
  Alberto Garcia 
  Alex Bennée 
  Alex Bennée 
  Alex Williamson 
  Alexey Kardashevskiy 
  Alistair Francis 
  Alistair Francis 
  Andrey Smirnov 
  Anton Nefedov 
 

Re: [Xen-devel] [PATCH] qemu: include generated files with <> and not ""

[Michael S. Tsirkin]

On Tue, Mar 20, 2018 at 01:58:32PM +, Daniel P. Berrangé wrote:
> > That's the C language for you though.  For better or worse, these are
> > the rules that K came up with.
> 
> No one seriously tries to keep compliance with original K C these days,
> things have moved on massively since then.

Not where the preprocessor is involved.

> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

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

[Xen-devel] [PATCH v11 11/12] vpci/msix: add MSI-X handlers

[Roger Pau Monne]

Add handlers for accesses to the MSI-X message control field on the
PCI configuration space, and traps for accesses to the memory region
that contains the MSI-X table and PBA. This traps detect attempts from
the guest to configure MSI-X interrupts and properly sets them up.

Note that accesses to the Table Offset, Table BIR, PBA Offset and PBA
BIR are not trapped by Xen at the moment.

Finally, turn the panic in the Dom0 PVH builder into a warning.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Jan Beulich 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Paul Durrant 
---
Changes since v10:
 - Do not continue to print msix entries if the MSIX struct has
   changed it's address while processing softirqs.
 - Use unsigned long to store the frame numbers in modify_bars.
 - Use lu to print frame values in modify_bars.

Changes since v9:
 - Unlock/lock when calling process_pending_softirqs.
 - Change vpci_msix_arch_print to return int in order to signal
   failure to continue after having processed softirqs.
 - Use a power of 2 to do the module.
 - Use PFN_DOWN in order to calculate the end of the MSI-X memory
   areas for the rangeset.

Changes since v8:
 - Call process_pending_softirqs between printing MSI-X entries.
 - Free msix struct in vpci_add_handlers.
 - Print only MSI or MSI-X if they are enabled.
 - Fix comment in update_entry.

Changes since v7:
 - Switch vpci.h macros to inline functions.
 - Change vpci_msix_arch_print_entry into vpci_msix_arch_print and
   make it print all the entries.
 - Add a log message if rangeset_remove_range fails to remove the BAR
   MSI-related range.
 - Introduce a new update_entry to disable and enable a MSIX entry in
   order to either update or set it up. This removes open coding it in
   two different places.
 - Unify access checks in access_allowed.
 - Add newlines between switch cases.
 - Expand max_entries to 12 bits.

Changes since v6:
 - Reduce the output of the debug keys.
 - Fix comments and code to match in vpci_msix_control_write.
 - Optimize size of the MSIX structure.
 - Convert 'tables[]' to a uint32_t in order to reduce the size of
   vpci_msix. Introduce some macros to make it easier to get the MSIX
   tables related data.
 - Limit size of the bool fields to 1 bit.
 - Remove the 'nr' field of vpci_msix_entry. The position can be
   calculated from the base of the entries array.
 - Drop the 'vpci_' prefix from the functions in msix.c, they are all
   static.
 - Remove the val local variable in control_read.
 - Initialize new_masked and new_enabled at declaration.
 - Recalculate the msix control value before writing it.
 - Remove the seg and bus local variables and use pdev->seg and
   pdev->bus instead.
 - Initialize msix at declaration in msix_{write/read}.
 - Add the must_check attribute to
   vpci_msix_arch_{enable/disable}_entry.

Changes since v5:
 - Update lock usage.
 - Unbind/unmap PIRQs when MSIX is disabled.
 - Share the arch-specific MSIX code with the MSI functions.
 - Do not reference the MSIX memory areas from the PCI BARs fields,
   instead fetch the BIR and offset each time needed.
 - Add the '_entry' suffix to the MSIX arch functions.
 - Prefix the vMSIX macros with 'V'.
 - s/gdprintk/gprintk/ in msix.c
 - Make vpci_msix_access_check return bool, and change it's name to
   vpci_msix_access_allowed.
 - Join the first two ifs in vpci_msix_{read/write} into a single one.
 - Allow Dom0 to write to the PBA area.
 - Add a note that reads from the PBA area will need to be translated
   if the PBA it's not identity mapped.

Changes since v4:
 - Remove parentheses around offsetof.
 - Add "being" to MSI-X enabling comment.
 - Use INVALID_PIRQ.
 - Add a simple sanity check to vpci_msix_arch_enable in order to
   detect wrong MSI-X entries more quickly.
 - Constify vpci_msix_arch_print entry argument.
 - s/cpu/fixed/ in vpci_msix_arch_print.
 - Dump the MSI-X info together with the MSI info.
 - Fix vpci_msix_control_write to take into account changes to the
   address and data fields when switching the function mask bit.
 - Only disable/enable the entries if the address or data fields have
   been updated.
 - Usew the BAR enable field to check if a BAR is mapped or not
   (instead of reading the command register for each device).
 - Fix error path in vpci_msix_read to set the return data to ~0.
 - Simplify mask usage in vpci_msix_write.
 - Cast data to uint64_t when shifting it 32 bits.
 - Fix writes to the table entry control register to take into account
   if the mask-all bit is set.
 - Add some comments to clarify the intended behavior of the code.
 - 

Re: [Xen-devel] [PATCH v2] hvm/svm: Implement Debug events

[Andrew Cooper]

On 20/03/18 09:40, Alexandru Isaila wrote:
> At this moment the Debug events for the AMD architecture are not
> forwarded to the monitor layer.
>
> This patch adds the Debug event to the common capabilities, adds
> the VMEXIT_ICEBP then forwards the event to the monitor layer.
>
> Chapter 2: SVM Processor and Platform Extensions: "Note: A vector 1
> exception generated by the single byte INT1
> instruction (also known as ICEBP) does not trigger the #DB
> intercept. Software should use the dedicated ICEBP
> intercept to intercept ICEBP"
>
> ---
> Changes since V1:
>   - Get inst_len from __get_instruction_length()
>   - Updated __get_instruction_length() for the INSTR_ICEBP
> instruction
>
> Signed-off-by: Alexandru Isaila 
> ---
>  xen/arch/x86/hvm/svm/emulate.c|  1 +
>  xen/arch/x86/hvm/svm/svm.c| 37 
> +--
>  xen/arch/x86/hvm/svm/vmcb.c   |  2 +-
>  xen/include/asm-x86/hvm/svm/emulate.h |  1 +
>  xen/include/asm-x86/monitor.h |  4 ++--
>  5 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/emulate.c b/xen/arch/x86/hvm/svm/emulate.c
> index e1a1581..172369e 100644
> --- a/xen/arch/x86/hvm/svm/emulate.c
> +++ b/xen/arch/x86/hvm/svm/emulate.c
> @@ -80,6 +80,7 @@ static const struct {
>  [INSTR_RDTSC]   = { X86EMUL_OPC(0x0f, 0x31) },
>  [INSTR_RDMSR]   = { X86EMUL_OPC(0x0f, 0x32) },
>  [INSTR_CPUID]   = { X86EMUL_OPC(0x0f, 0xa2) },
> +[INSTR_ICEBP]   = { X86EMUL_OPC(   0, 0xf1) },

This list is currently sorted by opcode.  The new addition should be
between INT3 and HLT.

>  };
>  
>  int __get_instruction_length_from_list(struct vcpu *v,
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index c34f5b5..d4f2290 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1109,7 +1109,8 @@ static void noreturn svm_do_resume(struct vcpu *v)
>  {
>  struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>  bool debug_state = (v->domain->debugger_attached ||
> -v->domain->arch.monitor.software_breakpoint_enabled);
> +v->domain->arch.monitor.software_breakpoint_enabled 
> ||
> +v->domain->arch.monitor.debug_exception_enabled);
>  bool_t vcpu_guestmode = 0;
>  struct vlapic *vlapic = vcpu_vlapic(v);
>  
> @@ -2438,16 +2439,15 @@ static bool svm_get_pending_event(struct vcpu *v, 
> struct x86_event *info)
>  return true;
>  }
>  
> -static void svm_propagate_intr(struct vcpu *v, unsigned long insn_len)
> +static void svm_propagate_intr(unsigned long insn_len, int16_t vector, 
> uint8_t type)

Hmm - not sure where the old unsigned long came from, but it isn't
really correct.  Also, as this function no longer propagates the
contents of the vmcb, it is now mis-named.

Please could you delete this function and use:

diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 2376ed6..843dafe 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -407,6 +407,19 @@ void hvm_migrate_pirqs(struct vcpu *v);
 
 void hvm_inject_event(const struct x86_event *event);
 
+static inline void hvm_inject_exception(
+unsigned int vector, unsigned int type, unsigned int insn_len)
+{
+struct x86_event event = {
+.vector = vector,
+.type = type,
+.insn_len = insn_len,
+.error_code = X86_EVENT_NO_EC,
+};
+
+hvm_inject_event();
+}
+
 static inline void hvm_inject_hw_exception(unsigned int vector, int errcode)
 {
 struct x86_event event = {

as a new common helper.  (I'm not terribly happy with the name, but I
can't think of a better alternative, seeing as it is needed for both
software and hardware exceptions.)

>  {
> -struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>  struct x86_event event = {
> -.vector = vmcb->eventinj.fields.type,
> -.type = vmcb->eventinj.fields.type,
> -.error_code = vmcb->exitinfo1,
> +.vector = vector,
> +.type = type,
> +.error_code = X86_EVENT_NO_EC,
> +.insn_len = insn_len,
>  };
>  
> -event.insn_len = insn_len;
>  hvm_inject_event();
>  }
>  
> @@ -2655,10 +2655,27 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>  /* Asynchronous event, handled when we STGI'd after the VMEXIT. */
>  HVMTRACE_0D(SMI);
>  break;
> -

Please retain this newline.

> +case VMEXIT_ICEBP:
>  case VMEXIT_EXCEPTION_DB:
>  if ( !v->domain->debugger_attached )
> -hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +{
> +int rc;
> +unsigned long trap_type = exit_reason == VMEXIT_ICEBP ?

unsigned int.

> +X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION;
> +
> +inst_len = 0;
> +
> +if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> 

Re: [Xen-devel] [PATCH v2 45/45] ARM: VGIC: wire new VGIC(-v2) files into Xen build system

[Andre Przywara]

Hi,

On 20/03/18 03:13, Julien Grall wrote:
> Hi Andre,
> 
> On 03/15/2018 08:30 PM, Andre Przywara wrote:
>> diff --git a/xen/arch/arm/vgic/Makefile b/xen/arch/arm/vgic/Makefile
>> new file mode 100644
>> index 00..806826948e
>> --- /dev/null
>> +++ b/xen/arch/arm/vgic/Makefile
>> @@ -0,0 +1,5 @@
>> +obj-y += vgic.o
>> +obj-y += vgic-v2.o
>> +obj-y += vgic-mmio.o
>> +obj-y += vgic-mmio-v2.o
>> +obj-y += vgic-init.o
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index 4b9664f313..342b95be31 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -968,6 +968,16 @@ unsigned int vgic_max_vcpus(const struct domain *d)
>>   return min_t(unsigned int, MAX_VIRT_CPUS, vgic_vcpu_limit);
>>   }
>>   +#ifdef CONFIG_HAS_GICV3
>> +void vgic_v3_setup_hw(paddr_t dbase,
>> +  unsigned int nr_rdist_regions,
>> +  const struct rdist_region *regions,
>> +  unsigned int intid_bits)
>> +{
>> +    /* Dummy implementation to allow building without actual vGICv3
>> support. */
>> +}
>> +#endif
> 
> Why not just avoid selecting HAS_GICV3?

Because "config ARM_64" selects HAS_GICV3, and I didn't dare to touch
this. Shouldn't be around for long anyways.

Cheers,
Andre.


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

[Xen-devel] [PATCH v11 00/12] vpci: PCI config space emulation

[Roger Pau Monne]

Hello,

The following series contain an implementation of handlers for the PCI
configuration space inside of Xen. This allows Xen to detect accesses
to the PCI configuration space and react accordingly.

Why is this needed? IMHO, there are two main points of doing all this
emulation inside of Xen, the first one is to prevent adding a bunch of
duplicated Xen PV specific code to each OS we want to support in PVH
mode. This just promotes Xen code duplication amongst OSes, which
leads to a higher maintainership burden.

The second reason would be that this code (or it's functionality to be
more precise) already exists in QEMU (and pciback to a degree), and
it's code that we already support and maintain. By moving it into the
hypervisor itself every guest type can make use of it, and should be
shared between them all. I know that the code in this series is not
yet suitable for DomU HVM guests in it's current state, but it should
be in due time.

As usual, each patch contains a changeset summary between versions,
I'm not going to copy the list of changes here.

The branch containing the patches can be found at:

git://xenbits.xen.org/people/royger/xen.git vpci_v11

Note that this is only safe to use for the hardware domain (that's
trusted), any non-trusted domain will need a lot more handlers before it
can freely access the PCI configuration space.

Roger Pau Monne (12):
  vpci: introduce basic handlers to trap accesses to the PCI config
space
  x86/mmcfg: add handlers for the PVH Dom0 MMCFG areas
  x86/physdev: enable PHYSDEVOP_pci_mmcfg_reserved for PVH Dom0
  pci: split code to size BARs from pci_add_device
  pci: add support to size ROM BARs to pci_size_mem_bar
  xen: introduce rangeset_consume_ranges
  vpci: add header handlers
  x86/pt: mask MSI vectors on unbind
  vpci/msi: add MSI handlers
  vpci: add a priority parameter to the vPCI register initializer
  vpci/msix: add MSI-X handlers
  vpci: do not expose unneeded functions to the user-space test harness

 .gitignore|   3 +
 tools/libxl/libxl_x86.c   |   2 +-
 tools/tests/Makefile  |   1 +
 tools/tests/vpci/Makefile |  33 +++
 tools/tests/vpci/emul.h   | 134 +
 tools/tests/vpci/main.c   | 309 +
 xen/arch/arm/xen.lds.S|  14 +
 xen/arch/x86/Kconfig  |   1 +
 xen/arch/x86/domain.c |   6 +-
 xen/arch/x86/hvm/dom0_build.c |  23 +-
 xen/arch/x86/hvm/hvm.c|   7 +
 xen/arch/x86/hvm/hypercall.c  |   5 +
 xen/arch/x86/hvm/io.c | 293 
 xen/arch/x86/hvm/ioreq.c  |   4 +
 xen/arch/x86/hvm/vmsi.c   | 246 +
 xen/arch/x86/msi.c|   3 +
 xen/arch/x86/physdev.c|  11 +
 xen/arch/x86/setup.c  |   2 +-
 xen/arch/x86/x86_64/mmconfig.h|   4 -
 xen/arch/x86/xen.lds.S|  14 +
 xen/common/rangeset.c |  28 ++
 xen/drivers/Kconfig   |   3 +
 xen/drivers/Makefile  |   1 +
 xen/drivers/passthrough/io.c  |  15 +
 xen/drivers/passthrough/pci.c | 107 ---
 xen/drivers/vpci/Makefile |   1 +
 xen/drivers/vpci/header.c | 567 ++
 xen/drivers/vpci/msi.c| 349 +++
 xen/drivers/vpci/msix.c   | 458 ++
 xen/drivers/vpci/vpci.c   | 482 
 xen/include/asm-x86/domain.h  |   1 +
 xen/include/asm-x86/hvm/domain.h  |   7 +
 xen/include/asm-x86/hvm/io.h  |  20 ++
 xen/include/asm-x86/msi.h |   3 +
 xen/include/asm-x86/pci.h |   6 +
 xen/include/public/arch-x86/xen.h |   5 +-
 xen/include/xen/irq.h |   1 +
 xen/include/xen/pci.h |   9 +
 xen/include/xen/pci_regs.h|   8 +
 xen/include/xen/rangeset.h|  10 +
 xen/include/xen/sched.h   |   4 +
 xen/include/xen/vpci.h| 225 +++
 42 files changed, 3379 insertions(+), 46 deletions(-)
 create mode 100644 tools/tests/vpci/Makefile
 create mode 100644 tools/tests/vpci/emul.h
 create mode 100644 tools/tests/vpci/main.c
 create mode 100644 xen/drivers/vpci/Makefile
 create mode 100644 xen/drivers/vpci/header.c
 create mode 100644 xen/drivers/vpci/msi.c
 create mode 100644 xen/drivers/vpci/msix.c
 create mode 100644 xen/drivers/vpci/vpci.c
 create mode 100644 xen/include/xen/vpci.h

-- 
2.16.2


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

[Xen-devel] [PATCH v11 09/12] vpci/msi: add MSI handlers

[Roger Pau Monne]

Add handlers for the MSI control, address, data and mask fields in
order to detect accesses to them and setup the interrupts as requested
by the guest.

Note that the pending register is not trapped, and the guest can
freely read/write to it.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Jan Beulich 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Paul Durrant 
---
Changes since v8:
 - Add a FIXME about the lack of testing and a comment regarding the
   lack of cleaning done in the init_msi error path.
 - Free msi struct when cleaning up if an init function failed.
 - Remove the 'error' label of init_msi, the caller will already
   perform the cleaning.

Changes since v7:
 - Don't store pci segment/bus on local variables.
 - Add an error label to init_msi.
 - Don't trap accesses to the PBA.
 - Fix msi_pending_bits_reg macro so it matches coding style.
 - Move the position of vectors in the vpci_msi struct.
 - Add a comment to clarify the expected state of vectors after
   pt_irq_create_bind and use XEN_DOMCTL_VMSI_X86_UNMASKED.

Changes since v6:
 - Use domain_spin_lock_irq_desc instead of open coding it.
 - Reduce the size of printed debug messages.
 - Constify domain in vpci_dump_msi.
 - Lock domlist_read_lock before iterating over the list of domains.
 - Make max_vectors and vectors uint8_t.
 - Drop the vpci_ prefix from the static functions in msi.c.
 - Turn the booleans in vpci_msi into bitfields.
 - Apply the mask bits to all vectors when enabling msi.
 - Remove the pos field.
 - Remove the usage of __msi_set_{enable/disable}.
 - Update the bindings when the message or data fields are updated.
 - Make vpci_msi_arch_disable return void, it wasn't returning any
   error.
 - Prevent the guest from writing to the pending bits field, it's read
   only as defined in the spec.
 - Add the must_check attribute to vpci_msi_arch_enable.

Changes since v5:
 - Update to new lock usage.
 - Change handlers to match the new type.
 - s/msi_flags/msi_gflags/, remove the local variables and use the new
   DOMCTL_VMSI_* defines.
 - Change the MSI arch function to take a vpci_msi instead of a
   vpci_arch_msi as parameter.
 - Fix the calculation of the guest vector for MSI injection to take
   into account the number of bits that can be modified.
 - Use INVALID_PIRQ everywhere.
 - Simplify exit path of vpci_msi_disable.
 - Remove the conditional when setting address64 and masking fields.
 - Add a process_pending_softirqs to the MSI dump loop.
 - Place the prototypes for the MSI arch-specific functions in
   xen/vpci.h.
 - Add parentheses around the INVALID_PIRQ definition.

Changes since v4:
 - Fix commit message.
 - Change the ASSERTs in vpci_msi_arch_mask into ifs.
 - Introduce INVALID_PIRQ.
 - Destroy the partially created bindings in case of failure in
   vpci_msi_arch_enable.
 - Just take the pcidevs lock once in vpci_msi_arch_disable.
 - Print an error message in case of failure of pt_irq_destroy_bind.
 - Make vpci_msi_arch_init return void.
 - Constify the arch parameter of vpci_msi_arch_print.
 - Use fixed instead of cpu for msi redirection.
 - Separate the header includes in vpci/msi.c between xen and asm.
 - Store the number of configured vectors even if MSI is not enabled
   and always return it in vpci_msi_control_read.
 - Fix/add comments in vpci_msi_control_write to clarify intended
   behavior.
 - Simplify usage of masks in vpci_msi_address_{upper_}write.
 - Add comment to vpci_msi_mask_{read/write}.
 - Don't use MASK_EXTR in vpci_msi_mask_write.
 - s/msi_offset/pos/ in vpci_init_msi.
 - Move control variable setup closer to it's usage.
 - Use d%d in vpci_dump_msi.
 - Fix printing of bitfield mask in vpci_dump_msi.
 - Fix definition of MSI_ADDR_REDIRECTION_MASK.
 - Shuffle the layout of vpci_msi to minimize gaps.
 - Remove the error label in vpci_init_msi.

Changes since v3:
 - Propagate changes from previous versions: drop xen_ prefix, drop
   return value from handlers, use the new vpci_val fields.
 - Use MASK_EXTR.
 - Remove the usage of GENMASK.
 - Add GFLAGS_SHIFT_DEST_ID and use it in msi_flags.
 - Add "arch" to the MSI arch specific functions.
 - Move the dumping of vPCI MSI information to dump_msi (key 'M').
 - Remove the guest_vectors field.
 - Allow the guest to change the number of active vectors without
   having to disable and enable MSI.
 - Check the number of active vectors when parsing the disable
   mask.
 - Remove the debug messages from vpci_init_msi.
 - Move the arch-specific part of the dump handler to x86/hvm/vmsi.c.
 - Use trylock in the dump handler to get the vpci lock.

Changes since 

[Xen-devel] [PATCH v11 05/12] pci: add support to size ROM BARs to pci_size_mem_bar

[Roger Pau Monne]

Signed-off-by: Roger Pau Monné 
Reviewed-by: Jan Beulich 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
---
Changes since v6:
 - Remove the rom local variable.

Changes since v5:
 - Use the flags field.
 - Introduce a mask local variable.
 - Simplify return.

Changes since v4:
 - New in this version.
---
 xen/drivers/passthrough/pci.c | 28 ++--
 xen/include/xen/pci.h |  1 +
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 190515b3c6..1751c66e34 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -610,11 +610,16 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned 
int pos,
 uint32_t hi = 0, bar = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
sbdf.func, pos);
 uint64_t size;
-
-ASSERT((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
+bool is64bits = !(flags & PCI_BAR_ROM) &&
+(bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64;
+uint32_t mask = (flags & PCI_BAR_ROM) ? (uint32_t)PCI_ROM_ADDRESS_MASK
+  : 
(uint32_t)PCI_BASE_ADDRESS_MEM_MASK;
+
+ASSERT(!((flags & PCI_BAR_VF) && (flags & PCI_BAR_ROM)));
+ASSERT((flags & PCI_BAR_ROM) ||
+   (bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_MEMORY);
 pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos, ~0);
-if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
- PCI_BASE_ADDRESS_MEM_TYPE_64 )
+if ( is64bits )
 {
 if ( flags & PCI_BAR_LAST )
 {
@@ -628,10 +633,9 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned 
int pos,
 hi = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4);
 pci_conf_write32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos + 4, ~0);
 }
-size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func, pos) &
-   PCI_BASE_ADDRESS_MEM_MASK;
-if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
- PCI_BASE_ADDRESS_MEM_TYPE_64 )
+size = pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev, sbdf.func,
+   pos) & mask;
+if ( is64bits )
 {
 size |= (uint64_t)pci_conf_read32(sbdf.seg, sbdf.bus, sbdf.dev,
   sbdf.func, pos + 4) << 32;
@@ -643,14 +647,10 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned 
int pos,
 size = -size;
 
 if ( paddr )
-*paddr = (bar & PCI_BASE_ADDRESS_MEM_MASK) | ((uint64_t)hi << 32);
+*paddr = (bar & mask) | ((uint64_t)hi << 32);
 *psize = size;
 
-if ( (bar & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==
- PCI_BASE_ADDRESS_MEM_TYPE_64 )
-return 2;
-
-return 1;
+return is64bits ? 2 : 1;
 }
 
 int pci_add_device(u16 seg, u8 bus, u8 devfn,
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 2f171a8dcc..4cfa774615 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -191,6 +191,7 @@ const char *parse_pci_seg(const char *, unsigned int *seg, 
unsigned int *bus,
 
 #define PCI_BAR_VF  (1u << 0)
 #define PCI_BAR_LAST(1u << 1)
+#define PCI_BAR_ROM (1u << 2)
 unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos,
   uint64_t *paddr, uint64_t *psize,
   unsigned int flags);
-- 
2.16.2


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

[Xen-devel] [PATCH v11 07/12] vpci: add header handlers

[Roger Pau Monne]

Introduce a set of handlers that trap accesses to the PCI BARs and the
command register, in order to snoop BAR sizing and BAR relocation.

The command handler is used to detect changes to bit 2 (response to
memory space accesses), and maps/unmaps the BARs of the device into
the guest p2m. A rangeset is used in order to figure out which memory
to map/unmap. This makes it easier to keep track of the possible
overlaps with other BARs, and will also simplify MSI-X support, where
certain regions of a BAR might be used for the MSI-X table or PBA.

The BAR register handlers are used to detect attempts by the guest to
size or relocate the BARs.

Note that the long running BAR mapping and unmapping operations are
deferred to be performed by hvm_io_pending, so that they can be safely
preempted.

Signed-off-by: Roger Pau Monné 
---
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Julien Grall 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Paul Durrant 
---
Changes since v10:
 - Fix indirect function call in map_range.
 - Use rom->addr instead of fetching it from the ROM BAR register in
   modify_decoding.
 - Remove ternary operator from modify_decoding.
 - Simply apply_map to have a single return.
 - Constify pci_dev parameter of apply_map.
 - Remove references to maybe_defer_map.
 - Use pdev (const) or dev (non-const) consistently in modify_bars.
 - Invert part of the logic in rom_write to remove one indentation
   level.
 - Add comments in rom_write to clarify why rom->addr is updated in
   two different places.
 - Use lx to print frame numbers in modify_bars.
 - Add start/end local variables in the first modify_bars loop.

Changes since v9:
 - Expand comments to clarify the code.
 - Rename rom to rom_only in the vpci_cpu struct.
 - Change definition style of dummy vpci_cpu.
 - Replace incorrect usage of PFN_UP.
 - Use system_state in order to check if the mapping functions are
   being called from Dom0 builder context.
 - Split the maybe_defer_map into two functions and place the Dom0
   builder one in the init section.

Changes since v8:
 - Do not pretend to support ARM in the map_range function. Explain
   the required changes in the comment.
 - Introduce PCI_HEADER_{NORMAL/BRIDGE}_NR_BARS defines.
 - Rename 'rom' boolean variable to 'rom_only', which is more
   descriptive of it's meaning.
 - Introduce vpci_remove_device which removes all handlers for a
   device.
 - Simplify error handling when modifying BARs mapping. Any error will
   cause the device to be unplugged (by calling vpci_remove_device).
 - Return an error code in modify_bars. Add comments describing why
   the error is sometimes ignored.

Changes since v7:
 - Order includes.
 - Add newline between switch cases.
 - Fix typo in comment (hopping).
 - Wrap ternary conditional in parentheses.
 - Remove CONFIG_HAS_PCI gueard from sched.h vpci_vcpu usage.
 - Add comment regarding vpci_vcpu usage.
 - Move rom_enabled from BAR struct to header.
 - Do not protect vpci_vcpu with __XEN__ guards.

Changes since v6:
 - s/vpci_check_pending/vpci_process_pending/.
 - Improve error handling in vpci_process_pending.
 - Add a comment that explains how vpci_check_bar_overlap works.
 - Add error messages to vpci_modify_bars and vpci_modify_rom.
 - Introduce vpci_hw_read16/32, in order to passthrough reads to
   the underlying hw.
 - Print BAR number on error in vpci_bar_write.
 - Place the CONFIG_HAS_PCI guards inside the vpci.h header and
   provide an empty vpci_vcpu structure for the !CONFIG_HAS_PCI case.
 - Define CONFIG_HAS_PCI in the test harness emul.h header before
   including vpci.h
 - Add ARM TODOs and an ARM-specific bodge to vpci_map_range due to
   the lack of preemption in {un}map_mmio_regions.
 - Make vpci_maybe_defer_map void.
 - Set rom_enabled in vpci_init_bars.
 - Defer enabling/disabling the memory decoding (or the ROM enable
   bit) until the memory has been mapped/unmapped.
 - Remove vpci_ prefix from static functions.
 - Use the same code in order to map the general BARs and the ROM
   BARs.
 - Remove the seg/bus local variables and use pdev->{seg,bus} instead.
 - Convert the bools in the BAR related structs into bool bitfields.
 - Add the must_check attribute to vpci_process_pending.
 - Open code check_bar_overlap inside modify_bars, which was it's only
   user.

Changes since v5:
 - Switch to the new handler type.
 - Use pci_sbdf_t to size the BARs.
 - Use a single return for vpci_modify_bar.
 - Do not return an error code from vpci_modify_bars, just log the
   failure.
 - Remove the 'sizing' parameter. Instead just let the guest write
   directly to the BAR, and read the value back. This simplifies the
   BAR register 

[Xen-devel] [PATCH v11 10/12] vpci: add a priority parameter to the vPCI register initializer

[Roger Pau Monne]

This is needed for MSI-X, since MSI-X will need to be initialized
before parsing the BARs, so that the header BAR handlers are aware of
the MSI-X related holes and make sure they are not mapped in order for
the trap handlers to work properly.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Jan Beulich 
---
Cc: Stefano Stabellini 
Cc: Julien Grall 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Tim Deegan 
Cc: Wei Liu 
---
Changes since v4:
 - Add a middle priority and add the PCI header to it.

Changes since v3:
 - Add a numerial suffix to the section used to store the pointer to
   each initializer function, and sort them at link time.
---
 xen/arch/arm/xen.lds.S| 4 ++--
 xen/arch/x86/xen.lds.S| 4 ++--
 xen/drivers/vpci/header.c | 2 +-
 xen/drivers/vpci/msi.c| 2 +-
 xen/include/xen/vpci.h| 8 ++--
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 49cae2af71..245a0e0e85 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -69,7 +69,7 @@ SECTIONS
 #if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
. = ALIGN(POINTER_ALIGN);
__start_vpci_array = .;
-   *(.data.vpci)
+   *(SORT(.data.vpci.*))
__end_vpci_array = .;
 #endif
   } :text
@@ -182,7 +182,7 @@ SECTIONS
 #if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
. = ALIGN(POINTER_ALIGN);
__start_vpci_array = .;
-   *(.data.vpci)
+   *(SORT(.data.vpci.*))
__end_vpci_array = .;
 #endif
   } :text
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 7bd6fb51c3..70afedd31d 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -139,7 +139,7 @@ SECTIONS
 #if defined(CONFIG_HAS_VPCI) && defined(CONFIG_LATE_HWDOM)
. = ALIGN(POINTER_ALIGN);
__start_vpci_array = .;
-   *(.data.vpci)
+   *(SORT(.data.vpci.*))
__end_vpci_array = .;
 #endif
   } :text
@@ -246,7 +246,7 @@ SECTIONS
 #if defined(CONFIG_HAS_VPCI) && !defined(CONFIG_LATE_HWDOM)
. = ALIGN(POINTER_ALIGN);
__start_vpci_array = .;
-   *(.data.vpci)
+   *(SORT(.data.vpci.*))
__end_vpci_array = .;
 #endif
   } :text
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index d7c220a452..8d9d6f43f3 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -535,7 +535,7 @@ static int init_bars(struct pci_dev *pdev)
 
 return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
 }
-REGISTER_VPCI_INIT(init_bars);
+REGISTER_VPCI_INIT(init_bars, VPCI_PRIORITY_MIDDLE);
 
 /*
  * Local variables:
diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c
index c3c69ec453..de4ddf562e 100644
--- a/xen/drivers/vpci/msi.c
+++ b/xen/drivers/vpci/msi.c
@@ -267,7 +267,7 @@ static int init_msi(struct pci_dev *pdev)
 
 return 0;
 }
-REGISTER_VPCI_INIT(init_msi);
+REGISTER_VPCI_INIT(init_msi, VPCI_PRIORITY_LOW);
 
 void vpci_dump_msi(void)
 {
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 116b93f519..7266c17679 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -15,9 +15,13 @@ typedef void vpci_write_t(const struct pci_dev *pdev, 
unsigned int reg,
 
 typedef int vpci_register_init_t(struct pci_dev *dev);
 
-#define REGISTER_VPCI_INIT(x)   \
+#define VPCI_PRIORITY_HIGH  "1"
+#define VPCI_PRIORITY_MIDDLE"5"
+#define VPCI_PRIORITY_LOW   "9"
+
+#define REGISTER_VPCI_INIT(x, p)\
   static vpci_register_init_t *const x##_entry  \
-   __used_section(".data.vpci") = x
+   __used_section(".data.vpci." p) = x
 
 /* Add vPCI handlers to device. */
 int __must_check vpci_add_handlers(struct pci_dev *dev);
-- 
2.16.2


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

[Xen-devel] [PATCH v11 08/12] x86/pt: mask MSI vectors on unbind

[Roger Pau Monne]

When a MSI device with per-vector masking capabilities is detected or
added to Xen all the vectors are masked when initializing it. This
implies that the first time the interrupt is bound to a domain it's
masked.

This however only applies to the first time the interrupt is bound
because neither the unbind nor the pirq unmap will mask the vector
again. In order to fix this re-mask the interrupt when unbinding it
from a guest. This makes sure that pairs of bind/unbind will always
get the same masking state.

Note that no issues have been reported regarding this behavior because
QEMU always uses the newly introduced XEN_PT_GFLAGSSHIFT_UNMASKED when
binding interrupts, so it's always unmasked.

Signed-off-by: Roger Pau Monné 
Reviewed-by: Jan Beulich 
---
Cc: Jan Beulich 
---
Changes since v7:
 - New in this version.
---
 xen/drivers/passthrough/io.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/xen/drivers/passthrough/io.c b/xen/drivers/passthrough/io.c
index 8f16e6c0a5..bab3aa349a 100644
--- a/xen/drivers/passthrough/io.c
+++ b/xen/drivers/passthrough/io.c
@@ -645,7 +645,22 @@ int pt_irq_destroy_bind(
 }
 break;
 case PT_IRQ_TYPE_MSI:
+{
+unsigned long flags;
+struct irq_desc *desc = domain_spin_lock_irq_desc(d, machine_gsi,
+  );
+
+if ( !desc )
+return -EINVAL;
+/*
+ * Leave the MSI masked, so that the state when calling
+ * pt_irq_create_bind is consistent across bind/unbinds.
+ */
+guest_mask_msi_irq(desc, true);
+spin_unlock_irqrestore(>lock, flags);
 break;
+}
+
 default:
 return -EOPNOTSUPP;
 }
-- 
2.16.2


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

[Xen-devel] [PATCH v11 12/12] vpci: do not expose unneeded functions to the user-space test harness

[Roger Pau Monne]

Some functions in vpci.c (vpci_remove_device and vpci_add_handlers)
are not used by the user-space test harness, so guard them with
__XEN__ in order to avoid exposing them to the user-space test
harness.

Requested-by: Jan Beulich 
Signed-off-by: Roger Pau Monné 
---
 tools/tests/vpci/Makefile |  8 ++--
 xen/drivers/vpci/vpci.c   | 10 ++
 xen/include/xen/vpci.h|  6 +-
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/tools/tests/vpci/Makefile b/tools/tests/vpci/Makefile
index e45fcb5cd9..5075bc2be2 100644
--- a/tools/tests/vpci/Makefile
+++ b/tools/tests/vpci/Makefile
@@ -24,12 +24,8 @@ distclean: clean
 install:
 
 vpci.c: $(XEN_ROOT)/xen/drivers/vpci/vpci.c
-   # Trick the compiler so it doesn't complain about missing symbols
-   sed -e '/#include/d' \
-   -e '1s;^;#include "emul.h"\
-vpci_register_init_t *const __start_vpci_array[1]\;\
-vpci_register_init_t *const __end_vpci_array[1]\;\
-;' <$< >$@
+   # Remove includes and add the test harness header
+   sed -e '/#include/d' -e '1s/^/#include "emul.h"/' <$< >$@
 
 list.h: $(XEN_ROOT)/xen/include/xen/list.h
 vpci.h: $(XEN_ROOT)/xen/include/xen/vpci.h
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 8ec9c916ea..2913b56500 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -20,10 +20,6 @@
 #include 
 #include 
 
-extern vpci_register_init_t *const __start_vpci_array[];
-extern vpci_register_init_t *const __end_vpci_array[];
-#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
-
 /* Internal struct to store the emulated PCI registers. */
 struct vpci_register {
 vpci_read_t *read;
@@ -34,6 +30,11 @@ struct vpci_register {
 struct list_head node;
 };
 
+#ifdef __XEN__
+extern vpci_register_init_t *const __start_vpci_array[];
+extern vpci_register_init_t *const __end_vpci_array[];
+#define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
+
 void vpci_remove_device(struct pci_dev *pdev)
 {
 spin_lock(>vpci->lock);
@@ -80,6 +81,7 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
 
 return rc;
 }
+#endif /* __XEN__ */
 
 static int vpci_register_cmp(const struct vpci_register *r1,
  const struct vpci_register *r2)
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index fc47163ba6..cb39e0ebea 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -90,11 +90,9 @@ struct vpci {
 bool rom_enabled  : 1;
 /* FIXME: currently there's no support for SR-IOV. */
 } header;
-#endif
 
 /* MSI data. */
 struct vpci_msi {
-#ifdef __XEN__
   /* Address. */
 uint64_t address;
 /* Mask bitfield. */
@@ -113,12 +111,10 @@ struct vpci {
 uint8_t vectors : 5;
 /* Arch-specific data. */
 struct vpci_arch_msi arch;
-#endif
 } *msi;
 
 /* MSI-X data. */
 struct vpci_msix {
-#ifdef __XEN__
 struct pci_dev *pdev;
 /* List link. */
 struct list_head next;
@@ -141,8 +137,8 @@ struct vpci {
 bool updated : 1;
 struct vpci_arch_msix_entry arch;
 } entries[];
-#endif
 } *msix;
+#endif
 };
 
 struct vpci_vcpu {
-- 
2.16.2


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

[Xen-devel] [PATCH v1.5 20/20] xen/domain: Allocate d->vcpu[] in arch_domain_create()

[Andrew Cooper]

On the ARM side, audit config->max_vcpus after the vgic has been initialised,
at which point we have a real upper bound to test against.  This allows for
the removal of the vgic_max_vcpus() juggling to cope with the call from
evtchn_init() before the vgic settings are known.

For each arch's dom0's, drop the temporary max_vcpus parameter, and allocation
of dom0->vcpu.

With arch_domain_create() now in charge of auditing config->max_vcpus, the
per-arch domain_max_vcpus() can be dropped.  Finally, evtchn_init() can be
updated to allocate a poll mask suitable for the domain, rather than suitable
for the worst case setting.

From this point on, d->max_vcpus and d->vcpus[] are valid for any domain which
can be looked up by ID.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Stefano Stabellini 
CC: Julien Grall 

v2:
 * Retain vgic_max_vcpus() on the ARM side, but remove the NULL special case.
---
 xen/arch/arm/domain.c| 11 +++
 xen/arch/arm/domain_build.c  |  8 +---
 xen/arch/arm/setup.c |  2 +-
 xen/arch/arm/vgic.c  | 11 +--
 xen/arch/x86/dom0_build.c|  8 +---
 xen/arch/x86/domain.c| 11 +++
 xen/arch/x86/setup.c |  2 +-
 xen/common/domctl.c  | 14 --
 xen/common/event_channel.c   |  4 ++--
 xen/include/asm-arm/domain.h |  6 --
 xen/include/asm-x86/domain.h |  2 --
 xen/include/xen/domain.h |  2 +-
 12 files changed, 30 insertions(+), 51 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 57a2b8b..7abe766 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -646,6 +646,17 @@ int arch_domain_create(struct domain *d,
 if ( (rc = domain_vtimer_init(d, >arch)) != 0 )
 goto fail;
 
+rc = -EINVAL;
+/* On ARM, the number of VCPUs is limited by the type of GIC emulated. */
+if ( (config->max_vcpus < 1) || (config->max_vcpus > vgic_max_vcpus(d)) )
+goto fail;
+
+rc = -ENOMEM;
+d->vcpu = xzalloc_array(struct vcpu *, config->max_vcpus);
+if ( !d->vcpu )
+goto fail;
+d->max_vcpus = config->max_vcpus;
+
 update_domain_wallclock_time(d);
 
 /*
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2e145d9..b13c47e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -76,14 +76,8 @@ unsigned int __init dom0_max_vcpus(void)
 return opt_dom0_max_vcpus;
 }
 
-struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0,
- unsigned int max_vcpus)
+struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 {
-dom0->vcpu = xzalloc_array(struct vcpu *, max_vcpus);
-if ( !dom0->vcpu )
-return NULL;
-dom0->max_vcpus = max_vcpus;
-
 return alloc_vcpu(dom0, 0, 0);
 }
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index be24f20..0ada4d5 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -859,7 +859,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 dom0_cfg.max_vcpus = dom0_max_vcpus();
 
 dom0 = domain_create(0, _cfg);
-if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0, dom0_cfg.max_vcpus) == NULL) )
+if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
 panic("Error creating domain 0");
 
 dom0->is_privileged = 1;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 3fafdd0..4d2b00c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -669,16 +669,7 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
 
 unsigned int vgic_max_vcpus(const struct domain *d)
 {
-/*
- * Since evtchn_init would call domain_max_vcpus for poll_mask
- * allocation when the vgic_ops haven't been initialised yet,
- * we return MAX_VIRT_CPUS if d->arch.vgic.handler is null.
- */
-if ( !d->arch.vgic.handler )
-return MAX_VIRT_CPUS;
-else
-return min_t(unsigned int, MAX_VIRT_CPUS,
- d->arch.vgic.handler->max_vcpus);
+return min_t(unsigned int, MAX_VIRT_CPUS, d->arch.vgic.handler->max_vcpus);
 }
 
 /*
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e82bc48..4c25789 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -200,17 +200,11 @@ unsigned int __init dom0_max_vcpus(void)
 return max_vcpus;
 }
 
-struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0,
- unsigned int max_vcpus)
+struct vcpu *__init alloc_dom0_vcpu0(struct domain *dom0)
 {
 dom0->node_affinity = dom0_nodes;
 dom0->auto_node_affinity = !dom0_nr_pxms;
 
-dom0->vcpu = xzalloc_array(struct vcpu *, max_vcpus);
-if ( !dom0->vcpu )
-return NULL;
-dom0->max_vcpus = max_vcpus;
-
 return dom0_setup_vcpu(dom0, 0,
cpumask_last(_cpus) /* so it wraps around to 
first pcpu */);
 }
diff --git 

Re: [Xen-devel] [PATCH v3 1/4] x86/PVHv2: Add memory map pointer to hvm_start_info struct

[Konrad Rzeszutek Wilk]

On Fri, Mar 16, 2018 at 10:00:54AM -0700, Maran Wilson wrote:
> On 3/16/2018 4:11 AM, Roger Pau Monné wrote:
> > On Thu, Mar 15, 2018 at 02:33:09PM -0700, Maran Wilson wrote:
> > > The start info structure that is defined as part of the x86/HVM direct 
> > > boot
> > > ABI and used for starting Xen PVH guests would be more versatile if it 
> > > also
> > > included a way to pass information about the memory map to the guest. This
> > > would allow KVM guests to share the same entry point.
> > > 
> > > Signed-off-by: Maran Wilson 
> > > ---
> > >   xen/include/public/arch-x86/hvm/start_info.h | 66 
> > > +++-
> > >   1 file changed, 65 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/include/public/arch-x86/hvm/start_info.h 
> > > b/xen/include/public/arch-x86/hvm/start_info.h
> > > index 6484159..f2e8ba6 100644
> > > --- a/xen/include/public/arch-x86/hvm/start_info.h
> > > +++ b/xen/include/public/arch-x86/hvm/start_info.h
> > > @@ -33,8 +33,9 @@
> > >*| magic  | Contains the magic value 
> > > XEN_HVM_START_MAGIC_VALUE
> > >*|| ("xEn3" with the 0x80 bit of the "E" set).
> > >*  4 ++
> > > - *| version| Version of this structure. Current version is 
> > > 0. New
> > > + *| version| Version of this structure. Current version is 
> > > 1. New
> > >*|| versions are guaranteed to be 
> > > backwards-compatible.
> > > + *|| For PV guests only 0 allowed, for PVH 0 or 1 
> > > allowed.
> > Why are you adding the above sentence? PV guest never used or will use
> > the hvm_start_info structure (note the hvm_ prefix).
> 
> Thanks for the feed back Roger,
> 
> As you noticed, my first version did not contain that comment. Konrad
> suggested adding that particular line (in reply to the Linux tree version of
> this patch) and no one seemed to object at the time so I went ahead and
> added it.
> 
> Konrad, do you care to weigh in here?

Could point about the hvm. My thought was more if somebody starts comparing
the 'start_info' structures they may get confused.

It totally is fine to ditch my idea.

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