Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events
On 07/08/16 05:31, Tamas K Lengyel wrote:
> This patch implements sending notification to a monitor subscriber when an
> x86/vmx guest executes the CPUID instruction.
>
> Signed-off-by: Tamas K Lengyel
> ---
> Cc: Ian Jackson
> Cc: Wei Liu
> Cc: Razvan Cojocaru
> Cc: Jan Beulich
> Cc: Andrew Cooper
> Cc: Jun Nakajima
> Cc: Kevin Tian
> ---
> tools/libxc/include/xenctrl.h | 1 +
> tools/libxc/xc_monitor.c| 13 +
> tools/tests/xen-access/xen-access.c | 33 -
> xen/arch/x86/hvm/monitor.c | 16
> xen/arch/x86/hvm/vmx/vmx.c | 23 +++
> xen/arch/x86/monitor.c | 13 +
> xen/include/asm-x86/domain.h| 1 +
> xen/include/asm-x86/hvm/monitor.h | 1 +
> xen/include/asm-x86/monitor.h | 3 ++-
> xen/include/public/domctl.h | 1 +
> xen/include/public/vm_event.h | 8
> 11 files changed, 107 insertions(+), 6 deletions(-)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 4a85b4a..e904bd5 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2167,6 +2167,7 @@ int xc_monitor_guest_request(xc_interface *xch, domid_t
> domain_id,
> bool enable, bool sync);
> int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
> bool enable, bool sync);
> +int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
> /**
> * This function enables / disables emulation for each REP for a
> * REP-compatible instruction.
> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
> index 264992c..4298813 100644
> --- a/tools/libxc/xc_monitor.c
> +++ b/tools/libxc/xc_monitor.c
> @@ -172,6 +172,19 @@ int xc_monitor_debug_exceptions(xc_interface *xch,
> domid_t domain_id,
> return do_domctl(xch, &domctl);
> }
>
> +int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_CPUID;
> +
> +return do_domctl(xch, &domctl);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/tools/tests/xen-access/xen-access.c
> b/tools/tests/xen-access/xen-access.c
> index 02655d5..d525b82 100644
> --- a/tools/tests/xen-access/xen-access.c
> +++ b/tools/tests/xen-access/xen-access.c
> @@ -337,7 +337,7 @@ void usage(char* progname)
> {
> fprintf(stderr, "Usage: %s [-m] write|exec", progname);
> #if defined(__i386__) || defined(__x86_64__)
> -fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug");
> +fprintf(stderr,
> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
> #endif
> fprintf(stderr,
> "\n"
> @@ -364,6 +364,7 @@ int main(int argc, char *argv[])
> int shutting_down = 0;
> int altp2m = 0;
> int debug = 0;
> +int cpuid = 1;
Should this be on by default? All the rest of the options are 0, and ...
> uint16_t altp2m_view_id = 0;
>
> char* progname = argv[0];
> @@ -426,6 +427,10 @@ int main(int argc, char *argv[])
> {
> debug = 1;
> }
> +else if ( !strcmp(argv[0], "cpuid") )
> +{
> +cpuid = 1;
> +}
> #endif
... you also set it to 1 here.
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
>>> On 06.07.16 at 17:50, wrote:
The title of this patch keeps confusing me - which code motion is
being relocated here?
> +void vmx_vm_event_update_cr3_traps(struct domain *d)
Is there anything in this function preventing the parameter to be
const?
> +{
> +struct vcpu *v;
> +struct arch_vmx_struct *avmx;
> +unsigned int cr3_bitmask;
> +bool_t cr3_vmevent, cr3_ldexit;
> +
> +/* domain must be paused */
> +ASSERT(atomic_read(&d->pause_count));
Comment style.
> +/* non-hap domains trap CR3 writes unconditionally */
> +if ( !paging_mode_hap(d) )
> +{
> +#ifndef NDEBUG
> +for_each_vcpu ( d, v )
> +ASSERT(v->arch.hvm_vmx.exec_control &
> CPU_BASED_CR3_LOAD_EXITING);
> +#endif
> +return;
> +}
> +
> +cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
> +cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
> +
> +for_each_vcpu ( d, v )
> +{
> +avmx = &v->arch.hvm_vmx;
> +cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
> +
> +if ( cr3_vmevent == cr3_ldexit )
> +continue;
> +
> +/*
> + * If CR0.PE=0, CR3 load exiting must remain enabled.
> + * See vmx_update_guest_cr code motion for cr = 0.
> + */
Same as for the title - what code motion is this referring to? In a
code comment you clearly shouldn't be referring to anything the
patch effects, only to its result.
> +if ( cr3_ldexit && !hvm_paging_enabled(v) &&
> !vmx_unrestricted_guest(v) )
> +continue;
The first sentence of the comment should be brought in line with
this condition.
> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
> index)
Unless there is a particular reason for this uint8_t, please convert to
unsigned int.
> +{
> +/* vmx only */
> +ASSERT(cpu_has_vmx);
Comment style (more below). Should perhaps also get "for now" or
some such added.
> +static inline void write_ctrlreg_disable_traps(struct domain *d)
> +{
> +unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
> +d->arch.monitor.write_ctrlreg_enabled = 0;
> +
> +if ( old )
> +{
> +/* vmx only */
> +ASSERT(cpu_has_vmx);
Wouldn't this better move ahead of the if()?
> +/* was CR3 load-exiting enabled due to monitoring? */
> +if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
And then this if() alone would suffice.
> @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
> void arch_monitor_cleanup_domain(struct domain *d)
> {
> xfree(d->arch.monitor.msr_bitmap);
> -
> +write_ctrlreg_disable_traps(d);
> memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
> memset(&d->monitor, 0, sizeof(d->monitor));
> }
Rather than deleting the blank line, perhaps better to insert another
one after your addition?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
>>> On 06.07.16 at 17:51, wrote:
> @@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v)
>
> void vcpu_destroy(struct vcpu *v)
> {
> -xfree(v->arch.vm_event);
> -v->arch.vm_event = NULL;
> +if ( unlikely(v->arch.vm_event) )
> +{
> +xfree(v->arch.vm_event->emul_read_data);
> +xfree(v->arch.vm_event);
> +v->arch.vm_event = NULL;
> +}
Considering the repeat of this pattern ...
> @@ -52,8 +58,25 @@ void vm_event_cleanup_domain(struct domain *d)
>
> for_each_vcpu ( d, v )
> {
> -xfree(v->arch.vm_event);
> -v->arch.vm_event = NULL;
> +if ( likely(!v->arch.vm_event) )
> +continue;
> +
> +/*
> + * Only xfree the entire arch_vm_event if write_data was fully
> handled.
> + * Otherwise defer entire xfree until domain/vcpu destroyal.
> + */
> +if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
> +{
> +xfree(v->arch.vm_event->emul_read_data);
> +xfree(v->arch.vm_event);
> +v->arch.vm_event = NULL;
> +continue;
> +}
... here, please consider making this another helper (inline?) function.
> +/* write_data not fully handled, only xfree emul_read_data */
Comment style again (and more below).
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v,
> unsigned int port)
> /* Clean up on domain destruction */
> void vm_event_cleanup(struct domain *d)
> {
> +struct vcpu *v;
> +
> #ifdef CONFIG_HAS_MEM_PAGING
> if ( d->vm_event->paging.ring_page )
> {
> @@ -560,6 +562,16 @@ void vm_event_cleanup(struct domain *d)
> (void)vm_event_disable(d, &d->vm_event->share);
> }
> #endif
> +
> +for_each_vcpu ( d, v )
> +{
> +if ( unlikely(v->arch.vm_event) )
> +{
> +/* vm_event->emul_read_data freed in vm_event_cleanup_domain */
Perhaps worthwhile adding a respective ASSERT()?
> +static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
> +{
> +return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
> +}
> +
> +static inline bool_t vm_event_domain_initialised(struct domain *d)
> +{
> +return (d->max_vcpus && d->vcpu[0] &&
> +vm_event_vcpu_initialised(d->vcpu[0]));
> +}
Both functions' parameters should be const. Pointless parentheses
in both return statements.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [libvirt test] 96778: regressions - FAIL
flight 96778 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/96778/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt-qcow2 9 debian-di-install fail REGR. vs. 96664 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass version targeted for testing: libvirt 8f390596beeba94b691687a4fe3697ebf08858c7 baseline version: libvirt 660468b1a32d8f669c4718358bff4d7aa5082d0e Last test of basis96664 2016-07-05 04:22:11 Z3 days Testing same since96778 2016-07-08 04:20:52 Z0 days1 attempts People who touched revisions under test: Daniel P. Berrange Michal Privoznik Paolo Bonzini Peter Krempa jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-armhf-armhf-libvirt-xsm fail test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-armhf-armhf-libvirt fail test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-armhf-armhf-libvirt-qcow2 fail test-armhf-armhf-libvirt-raw fail test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master 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 8f390596beeba94b691687a4fe3697ebf08858c7 Author: Daniel P. Berrange Date: Fri Jul 1 17:42:37 2016 +0100 virtlogd: increase max file size to 2 MB People debugging guest OS boot processes and reported that the default 128 KB size is too small to capture an entire boot up sequence. Increase the default size to 2 MB which should allow capturing a full boot up even with verbose debugging. Signed-off-by: Daniel P. Berrange commit 24aacfa8e84af95cae78e23ee51a7b23e8a0d03a Author: Daniel P. Berrange Date: Fri Jul 1 17:40:55 2016 +0100 virtlogd: make max file size & number of backups configurable Currently virtlogd has a
[Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor
Xen hypervisor drivers might replace native OS drivers. The result is
that some important clocks that are enabled by the OS in the non-Xen
case are not properly enabled in the presence of Xen. The clocks
property enumerates the clocks that must be enabled by the Xen clock
consumer.
An example is a serial driver enabled by the hypervisor. Xen must
consume and enable these clocks in the OS to ensure behavior continues
after firmware configures the UART hardware and corresponding clock
harder.
Up to now, the workaround for this has been to use the Linux kernel
command line parameter 'clk_ignore_unused'. See Xen bug
http://bugs.xenproject.org/xen/bug/45
too.
To fix this, we will add the "unused" clocks in Xen to the hypervisor
node. The OS has to consume and enable the clocks from the hypervisor
node, then.
Therefore, check if there is a "clocks" entry in the hypervisor node
and if so consume and enable the given clocks. This prevents the clocks
from being disabled by the OS.
Signed-off-by: Dirk Behme
---
Changes in v3: Use the xen.txt description proposed by Michael. Thanks!
Changes in v2: Drop the Linux implementation details like clk_disable_unused
in xen.txt.
Documentation/devicetree/bindings/arm/xen.txt | 13
arch/arm/xen/enlighten.c | 47 +++
2 files changed, 60 insertions(+)
diff --git a/Documentation/devicetree/bindings/arm/xen.txt
b/Documentation/devicetree/bindings/arm/xen.txt
index c9b9321..00f2165 100644
--- a/Documentation/devicetree/bindings/arm/xen.txt
+++ b/Documentation/devicetree/bindings/arm/xen.txt
@@ -17,6 +17,19 @@ the following properties:
A GIC node is also required.
This property is unnecessary when booting Dom0 using ACPI.
+Optional properties:
+
+clocks: one or more clocks to be enabled
+ Xen hypervisor drivers might replace native OS drivers. The result is
+ that some important clocks that are enabled by the OS in the non-Xen
+ case are not properly enabled in the presence of Xen. The clocks
+ property enumerates the clocks that must be enabled by the Xen clock
+ consumer.
+ An example is a serial driver enabled by the hypervisor. Xen must
+ consume and enable these clocks in the OS to ensure behavior continues
+ after firmware configures the UART hardware and corresponding clock
+ harder.
+
To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node
under /hypervisor with following parameters:
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index 47acb36..5c546d0 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
}
late_initcall(xen_pm_init);
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+ struct clk *clk;
+ struct device_node *xen_node;
+ unsigned int i, count;
+ int ret = 0;
+
+ xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+ if (!xen_node) {
+ pr_err("Xen support was detected before, but it has
disappeared\n");
+ return -EINVAL;
+ }
+
+ count = of_clk_get_parent_count(xen_node);
+ if (!count)
+ goto out;
+
+ for (i = 0; i < count; i++) {
+ clk = of_clk_get(xen_node, i);
+ if (IS_ERR(clk)) {
+ pr_err("Xen failed to register clock %i. Error: %li\n",
+ i, PTR_ERR(clk));
+ ret = PTR_ERR(clk);
+ goto out;
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret < 0) {
+ pr_err("Xen failed to enable clock %i. Error: %i\n",
+ i, ret);
+ goto out;
+ }
+ }
+
+ ret = 0;
+
+out:
+ of_node_put(xen_node);
+ return ret;
+}
+late_initcall(xen_arm_register_clks);
/* empty stubs */
void xen_arch_pre_suspend(void) { }
--
2.8.0
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 7/8] minor fixes (formatting, comments, unused includes etc.)
>>> On 06.07.16 at 17:55, wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -31,7 +31,6 @@ > #include > #include > #include > -#include > #include > #include > #include > @@ -63,11 +62,9 @@ > #include > #include > #include > -#include > #include > #include > #include > -#include > #include > #include > #include The inclusion of asm/vm_event.h should really be removed in patch 2, I had to drop it here, to get the patch to apply ahead of the earlier ones in this series. > --- a/xen/arch/x86/monitor.c > +++ b/xen/arch/x86/monitor.c > @@ -21,7 +21,6 @@ > > #include > #include > -#include > #include > > static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t > index) Similarly I had to drop this hunk, which suggests that one of the earlier patches needlessly adds that #include (or it gets validly added and then a later patch makes it unnecessary again). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] xsm: rework policy_buffer globals
>>> On 07.07.16 at 17:35, wrote:
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1815,7 +1815,7 @@ static struct xsm_operations flask_ops = {
> .xen_version = flask_xen_version,
> };
>
> -__init void flask_init(void)
> +__init void flask_init(const void *policy_buffer, size_t policy_size)
Please take the opportunity and move the __init to its more
conventional place:
void __init flask_init(const void *policy_buffer, size_t policy_size)
> --- a/xen/xsm/xsm_policy.c
> +++ b/xen/xsm/xsm_policy.c
> @@ -28,13 +28,12 @@
> # include
> #endif
>
> -char *__initdata policy_buffer = NULL;
> -u32 __initdata policy_size = 0;
> -
> #ifdef CONFIG_MULTIBOOT
> int __init xsm_multiboot_policy_init(unsigned long *module_map,
> const multiboot_info_t *mbi,
> - void *(*bootstrap_map)(const module_t
> *))
> + void *(*bootstrap_map)(const module_t
> *),
> + void **policy_buffer,
> + size_t *policy_size)
> {
> int i;
> module_t *mod = (module_t *)__va(mbi->mods_addr);
> @@ -56,8 +55,8 @@ int __init xsm_multiboot_policy_init(unsigned long
> *module_map,
>
> if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
> {
> -policy_buffer = (char *)_policy_start;
> -policy_size = _policy_len;
> +*policy_buffer = (char *)_policy_start;
> +*policy_size = _policy_len;
With *policy_buffer now being of type void * I don't think this cast is
necessary anymore (or if it is, then it certainly shouldn't needlessly
go through char *).
With at least this latter aspect adjusted
Reviewed-by: Jan Beulich
Thanks for adding this patch,
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 2/2] xsm: add a default policy to .init.data
>>> On 07.07.16 at 17:35, wrote: > This adds a Kconfig option and support for including the XSM policy from > tools/flask/policy in the hypervisor so that the bootloader does not > need to provide a policy to get sane behavior from an XSM-enabled > hypervisor. The policy provided by the bootloader, if present, will > override the built-in policy. > > The XSM policy is not moved out of tools because that remains the > primary location for installing and configuring the policy. > > Signed-off-by: Daniel De Graaf > Reviewed-by: Konrad Rzeszutek Wilk > --- > > Changes since v4: > - Fixed clean target in xsm/flask/Makefile > - Dropped now-unneeded const-dropping cast of policy_buffer Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 1/2] xsm: rework policy_buffer globals
(Re-sending, as I got two bounces)
>>> On 07.07.16 at 17:35, wrote:
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1815,7 +1815,7 @@ static struct xsm_operations flask_ops = {
> .xen_version = flask_xen_version,
> };
>
> -__init void flask_init(void)
> +__init void flask_init(const void *policy_buffer, size_t policy_size)
Please take the opportunity and move the __init to its more
conventional place:
void __init flask_init(const void *policy_buffer, size_t policy_size)
> --- a/xen/xsm/xsm_policy.c
> +++ b/xen/xsm/xsm_policy.c
> @@ -28,13 +28,12 @@
> # include
> #endif
>
> -char *__initdata policy_buffer = NULL;
> -u32 __initdata policy_size = 0;
> -
> #ifdef CONFIG_MULTIBOOT
> int __init xsm_multiboot_policy_init(unsigned long *module_map,
> const multiboot_info_t *mbi,
> - void *(*bootstrap_map)(const module_t
> *))
> + void *(*bootstrap_map)(const module_t
> *),
> + void **policy_buffer,
> + size_t *policy_size)
> {
> int i;
> module_t *mod = (module_t *)__va(mbi->mods_addr);
> @@ -56,8 +55,8 @@ int __init xsm_multiboot_policy_init(unsigned long
> *module_map,
>
> if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
> {
> -policy_buffer = (char *)_policy_start;
> -policy_size = _policy_len;
> +*policy_buffer = (char *)_policy_start;
> +*policy_size = _policy_len;
With *policy_buffer now being of type void * I don't think this cast is
necessary anymore (or if it is, then it certainly shouldn't needlessly
go through char *).
With at least this latter aspect adjusted
Reviewed-by: Jan Beulich
Thanks for adding this patch,
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
On 05/07/16 15:28, Corneliu ZUZU wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack
> user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.
>
> But this can yield unexpected behavior since if a CR-write was awaiting to be
> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
> before xc_monitor_disable is called, then the domain CR write is wrongfully
> ignored, which of course, in these cases, can easily render a domain crash.
>
> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
> arch_vcpu.vm_event structure, which with this patch will only be freed on
> vcpu/domain destroyal.
>
> Signed-off-by: Corneliu ZUZU
> Acked-by: Razvan Cojocaru
[snip]
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 16733a4..6616626 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1642,7 +1642,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
> v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
>
> if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +*v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> }
> }
>
On the basis of Razvan's ack:
Acked-by: George Dunlap
> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 80f84d6..0e25e4d 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -30,12 +30,18 @@ int vm_event_init_domain(struct domain *d)
>
> for_each_vcpu ( d, v )
> {
> -if ( v->arch.vm_event )
> +if ( likely(!v->arch.vm_event) )
> +{
> +v->arch.vm_event = xzalloc(struct arch_vm_event);
> +if ( !v->arch.vm_event )
> +return -ENOMEM;
> +}
> +else if ( unlikely(v->arch.vm_event->emul_read_data) )
> continue;
>
> -v->arch.vm_event = xzalloc(struct arch_vm_event);
> -
> -if ( !v->arch.vm_event )
> +v->arch.vm_event->emul_read_data =
> +xzalloc(struct vm_event_emul_read_data);
> +if ( !v->arch.vm_event->emul_read_data )
> return -ENOMEM;
> }
>
> @@ -52,8 +58,12 @@ void vm_event_cleanup_domain(struct domain *d)
>
> for_each_vcpu ( d, v )
> {
> -xfree(v->arch.vm_event);
> -v->arch.vm_event = NULL;
> +if ( likely(!v->arch.vm_event) )
> +continue;
> +
> +v->arch.vm_event->emulate_flags = 0;
> +xfree(v->arch.vm_event->emul_read_data);
> +v->arch.vm_event->emul_read_data = NULL;
> }
>
> d->arch.mem_access_emulate_each_rep = 0;
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 17d2716..75bbbab 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v,
> unsigned int port)
> /* Clean up on domain destruction */
> void vm_event_cleanup(struct domain *d)
> {
> +struct vcpu *v;
> +
> #ifdef CONFIG_HAS_MEM_PAGING
> if ( d->vm_event->paging.ring_page )
> {
> @@ -560,6 +562,15 @@ void vm_event_cleanup(struct domain *d)
> (void)vm_event_disable(d, &d->vm_event->share);
> }
> #endif
> +
> +for_each_vcpu ( d, v )
> +{
> +if ( unlikely(v->arch.vm_event) )
> +{
> +xfree(v->arch.vm_event);
> +v->arch.vm_event = NULL;
> +}
> +}
> }
>
> int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0611681..a094c0d 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
>
> #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>
> @@ -48,7 +49,8 @@ int arch_monitor_domctl_op(struct domain *d, struct
> xen_domctl_monitor_op *mop)
> * Enabling mem_access_emulate_each_rep without a vm_event subscriber
> * is meaningless.
> */
> -if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )
> +if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event &&
> + d->vcpu[0]->arch.vm_event->emul_read_data )
> d->arch.mem_access_emulate_each_rep = !!mop->event;
> else
> rc = -EINVAL;
> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
> index 026f42e..557f353 100644
> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -28,7 +28,7 @@
> */
> struct arch_vm_event {
> uint32_t emulate_flag
Re: [Xen-devel] [RFC] Hypervisor, x86 emulation deprivileged
At 07:59 + on 06 Jul (1467791951), Paul Durrant wrote: > > -Original Message- > > From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of > > Andrew Cooper > > Sent: 05 July 2016 18:26 > > To: Anthony Perard; xen-devel@lists.xen.org; Jan Beulich > > Subject: Re: [Xen-devel] [RFC] Hypervisor, x86 emulation deprivileged > > > > On 05/07/16 12:22, Anthony PERARD wrote: > > > Hi, > > > > > > I've taken over the work from Ben to have a deprivileged mode in the > > > hypervisor, but I'm unsure about which direction to take. > > > > You should begin with an evaluation of available options, identifying > > which issues are mitigated, those which are not, and which which new > > risks are introduced. > > > > Personally I can't see why we want to invent a new way of executing > code in the hypervisor. What's wrong with modelling each emulator as a > PV unikernel, running with all the usual domain and vcpu constructs? It would be useful to compare a unikernel/minios implementation of this toy service to the other three. I'd expect it to be slower because of the extra context switch, but maybe directed yields could help with that? Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
Hi Dirk, On 08/07/16 06:51, Dirk Behme wrote: On 08.07.2016 04:50, Michael Turquette wrote: Quoting Dirk Behme (2016-07-07 00:32:34) The two issues to resolve are: 1) does consuming these hardware resources from the Linux kernel fit correctly with the Xen model? I think so. Julien, what do you think? I don't think it fits the Xen model. I will give more details on the v3 as your commit message give an example with is contradictory to the model. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events
On 08/07/16 03:31, Tamas K Lengyel wrote: > This patch implements sending notification to a monitor subscriber when an > x86/vmx guest executes the CPUID instruction. > > Signed-off-by: Tamas K Lengyel Is it wise having an on/off control without any further filtering? (I suppose that it is at least a fine first start). cpuid is usually the serialising instruction used with rdtsc for timing loops. This is bad enough in VMs because of the VMExit, but becomes even worse if there is a monitor delay as well. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor
Hi Dirk, On 08/07/16 08:44, Dirk Behme wrote: Xen hypervisor drivers might replace native OS drivers. The result is that some important clocks that are enabled by the OS in the non-Xen case are not properly enabled in the presence of Xen. The clocks property enumerates the clocks that must be enabled by the Xen clock consumer. An example is a serial driver enabled by the hypervisor. Xen must I would say "An example is the UART used by the hypervisor." consume and enable these clocks in the OS to ensure behavior continues after firmware configures the UART hardware and corresponding clock harder. What do you mean by "harder"? Also, relying on DOM0 to enable the clock looks very wrong to me and you give an example which prove that. The UART will be used before hand by Xen, however it will not be possible to use it if you expect DOM0 to enable the clock (or even modify the clock frequency). The clock should be enabled either by the firmware or Xen. But not DOM0. DOM0 should not touch this clock at all. Furthermore, this property could be used for clock associated to device that will be passthrough-ed to a guest. In this case, the clock would be enabled even if the device is not in use which will result more power consumption. Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 too. To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The OS has to consume and enable the clocks from the hypervisor node, then. Therefore, check if there is a "clocks" entry in the hypervisor node and if so consume and enable the given clocks. This prevents the clocks from being disabled by the OS. Signed-off-by: Dirk Behme --- Changes in v3: Use the xen.txt description proposed by Michael. Thanks! Changes in v2: Drop the Linux implementation details like clk_disable_unused in xen.txt. Documentation/devicetree/bindings/arm/xen.txt | 13 arch/arm/xen/enlighten.c | 47 +++ 2 files changed, 60 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt index c9b9321..00f2165 100644 --- a/Documentation/devicetree/bindings/arm/xen.txt +++ b/Documentation/devicetree/bindings/arm/xen.txt @@ -17,6 +17,19 @@ the following properties: A GIC node is also required. This property is unnecessary when booting Dom0 using ACPI. +Optional properties: + +clocks: one or more clocks to be enabled + Xen hypervisor drivers might replace native OS drivers. The result is "native OS" has no meaning on Xen. This seems to be a cumbersome way to say that the device will be used by the hypervisor and hidden to DOM0 (aka hardware domain). + that some important clocks that are enabled by the OS in the non-Xen + case are not properly enabled in the presence of Xen. The clocks + property enumerates the clocks that must be enabled by the Xen clock + consumer. + An example is a serial driver enabled by the hypervisor. Xen must + consume and enable these clocks in the OS to ensure behavior continues + after firmware configures the UART hardware and corresponding clock + harder. + To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node under /hypervisor with following parameters: Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2] xen/arm: register clocks used by the hypervisor
On 08/07/16 07:48, Dirk Behme wrote:
#include
#include
#include
@@ -444,6 +445,52 @@ static int __init xen_pm_init(void)
}
late_initcall(xen_pm_init);
+/*
+ * Check if we want to register some clocks, that they
+ * are not freed because unused by clk_disable_unused().
+ * E.g. the serial console clock.
+ */
+static int __init xen_arm_register_clks(void)
+{
+ struct clk *clk;
+ struct device_node *xen_node;
+ unsigned int i, count;
+ int ret = 0;
+
+ xen_node = of_find_compatible_node(NULL, NULL, "xen,xen");
+ if (!xen_node) {
+ pr_err("Xen support was detected before, but it has
disappeared\n");
+ return -EINVAL;
+ }
+
+ count = of_clk_get_parent_count(xen_node);
+ if (!count)
+ goto out;
+
+ for (i = 0; i < count; i++) {
+ clk = of_clk_get(xen_node, i);
Is there a struct device we can use here?
It doesn't look so. Julien?
We don't have a struct device. Maybe we can create a dummy one if it
simplifies the logic?
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 01/20] hvmloader: Provide hvmloader_acpi_build_tables()
>>> On 05.07.16 at 21:05, wrote: > In preparation for moving out ACPI builder make all > BIOSes call hvmloader_acpi_build_tables() instead of > calling ACPI code directly. > > No functional changes. > > Signed-off-by: Boris Ostrovsky Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 06/14] xen: Move the hvm_start_info C representation from libxc to public/xen.h
Hi Wei, On 07/07/16 16:28, Wei Liu wrote: On Thu, Jul 07, 2016 at 09:07:59AM -0600, Jan Beulich wrote: On 07.07.16 at 16:55, wrote: Cc HV maintainers I'm of course fine with moving this structure somewhere else. On Wed, Jun 22, 2016 at 06:15:37PM +0100, Anthony PERARD wrote: diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h I suspect it would make more sense to move it to public/arch-x86/xen.h. The question is whether we really mean this to remain x86 specific: The way it's now there's nothing inherently x86-ish there. But if that's the plan (which the conditional around it supports) then the suggested alternative resting place seems appropriate to me. For one, ARM doesn't distinguish PV vs HVM vs PVH (yet). Calling it HVM for ARM would be wrong IMHO. Correct, if you would have to do a comparison with x86 it would be PVH. However, this structure looks useful only if we lack a way to describe those parameters in the firmware. This is not the case for ARM as this could be described easily by the device tree with the generic bindings. Regards, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 96783: tolerable all pass - PUSHED
flight 96783 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96783/
Failures :-/ but no regressions.
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass
test-armhf-armhf-xl 12 migrate-support-checkfail never pass
test-armhf-armhf-xl 13 saverestore-support-checkfail never pass
version targeted for testing:
xen 5200cead3ec350e9fc0a2991383775bf5dd0624a
baseline version:
xen 730bdfa418fc8c809695ff5d96bc6f7a3b8827ba
Last test of basis96770 2016-07-07 19:00:59 Z0 days
Testing same since96783 2016-07-08 08:02:11 Z0 days1 attempts
People who touched revisions under test:
Corneliu ZUZU
Jan Beulich
Julien Grall
Razvan Cojocaru
Tamas K Lengyel
jobs:
build-amd64 pass
build-armhf pass
build-amd64-libvirt pass
test-armhf-armhf-xl pass
test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
test-amd64-amd64-libvirt pass
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Pushing revision :
+ branch=xen-unstable-smoke
+ revision=5200cead3ec350e9fc0a2991383775bf5dd0624a
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke
5200cead3ec350e9fc0a2991383775bf5dd0624a
+ branch=xen-unstable-smoke
+ revision=5200cead3ec350e9fc0a2991383775bf5dd0624a
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' x5200cead3ec350e9fc0a2991383775bf5dd0624a = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
getconfig GitCacheProxy
perl -e '
use Osstest;
readglobalconfig();
print $c{"GitCacheProxy"} or die $!;
'
+++ local cache=git://cache:9419/
+++ '[' xg
Re: [Xen-devel] [PATCH 00/19] Assorted fixes and improvements to Credit2
On Fri, Jun 17, 2016 at 6:32 PM, Dario Faggioli wrote: > Hi everyone, > > Here you go a collection of pseudo-random fixes and improvement to Credit2. > > In the process of working on Soft Affinity and Caps support, I stumbled upon > them, one after the other, and decided to take care. > > It's been hard to test and run benchmark, due to the "time goes backwards" bug > I uncovered [1], and this is at least part of the reason why the code for > affinity and caps is still missing. I've got it already, but need to refine a > couple of things, after double checking benchmark results. So, now that we > have > Jan's series [2] (thanks! [*]), and that I managed to indeed run some tests on > this preliminary set of patches, I decided I better set this first group free, > while working on finishing the rest. > > The various patches do a wide range of different things, so, please, refer to > Dario Faggioli (19): I've pushed the following patches: > xen: sched: make the 'tickled' perf counter clearer > xen: credit2: insert and tickle don't need a cpu parameter > xen: credit2: kill useless helper function choose_cpu > xen: credit2: do not warn if calling burn_credits more than once > xen: credit2: when tickling, check idle cpus first > xen: credit2: avoid calling __update_svc_load() multiple times on the > same vcpu > xen: credit2: use non-atomic cpumask and bit operations The ones below either have outstanding comments, or don't apply without patches which haven't been applied. > xen: sched: leave CPUs doing tasklet work alone. > xen: credit2: read NOW() with the proper runq lock held > xen: credit2: prevent load balancing to go mad if time goes backwards > xen: credit2: rework load tracking logic > tools: tracing: adapt Credit2 load tracking events to new format > xen: credit2: make the code less experimental > xen: credit2: add yet some more tracing > xen: credit2: only marshall trace point arguments if tracing enabled > tools: tracing: deal with new Credit2 events > xen: credit2: the private scheduler lock can be an rwlock. > xen: credit2: implement SMT support independent runq arrangement > xen: credit2: use cpumask_first instead of cpumask_any when choosing cpu -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
>>> On 05.07.16 at 21:05, wrote:
> --- a/tools/firmware/hvmloader/acpi/acpi2_0.h
> +++ b/tools/firmware/hvmloader/acpi/acpi2_0.h
> @@ -449,17 +449,6 @@ struct acpi_20_slit {
> #define ACPI_2_0_SRAT_REVISION 0x01
> #define ACPI_2_0_SLIT_REVISION 0x01
>
> -#pragma pack ()
This must not be removed from here.
> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config,
> unsigned int physical)
> offsetof(struct acpi_20_rsdp, extended_checksum),
> sizeof(struct acpi_20_rsdp));
>
> -if ( !new_vm_gid(acpi_info) )
> +if ( !new_vm_gid(&config->ainfo) )
> goto oom;
>
> -acpi_info->com1_present = uart_exists(0x3f8);
> -acpi_info->com2_present = uart_exists(0x2f8);
> -acpi_info->lpt1_present = lpt_exists(0x378);
> -acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
> -acpi_info->pci_min = pci_mem_start;
> -acpi_info->pci_len = pci_mem_end - pci_mem_start;
> -if ( pci_hi_mem_end > pci_hi_mem_start )
> -{
> -acpi_info->pci_hi_min = pci_hi_mem_start;
> -acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
> -}
> +*(struct acpi_info *)config->ainfop = config->ainfo;
With your new separation of responsibilities - does this really
belong here rather than in the caller? And if it is to stay here, then
I think the pointer field should be made of pointer type, leaving it
to the caller to do whatever casting is necessary when filling in that
field.
> --- /dev/null
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -0,0 +1,80 @@
> +/**
> + * libacpi.h
> + *
> + * libacpi interfaces
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense,
> and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +
> +#ifndef __LIBACPI_H__
> +#define __LIBACPI_H__
> +
> +#pragma pack ()
I guess this is going to be unneeded once the removal from acpi2_0.h
gets undone?
> +/*
> + * This must match the Field("BIOS") definition in the DSDT.
> + */
This is a single line comment. And considering what this comment
says I wonder whether the following structure definition wouldn't
better be wrapped in a #pragma pack(1) / #pragma pack() pair,
with ...
> +struct acpi_info {
> +uint8_t com1_present:1;/* 0[0] - System has COM1? */
> +uint8_t com2_present:1;/* 0[1] - System has COM2? */
> +uint8_t lpt1_present:1;/* 0[2] - System has LPT1? */
> +uint8_t hpet_present:1;/* 0[3] - System has HPET? */
explicit (unnamed) padding added here.
> +struct acpi_config {
> +const unsigned char *dsdt_anycpu;
> +unsigned int dsdt_anycpu_len;
> +const unsigned char *dsdt_15cpu;
> +unsigned int dsdt_15cpu_len;
> +
> +/* May have some fields updated by acpi_build_table() */
> +struct acpi_info ainfo;
> +/*
> + * Address where acpi_info should be placed.
> + * This must match the OperationRegion(BIOS, SystemMemory, )
> + * definition in the DSDT
> + */
> +unsigned int ainfop;
What is the "a" prefix of these two fields good for? Considering the
name of the structure I don't see a need for any prefix. But if you
absolutely want one, then acpi_ please.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 03/20] acpi/hvmloader: Initialize vm_gid data outside ACPI code
>>> On 05.07.16 at 21:05, wrote:
> This way ACPI code won't use xenstore-read() and hvm_param_set()
> which are private to hvmloader.
>
> Signed-off-by: Boris Ostrovsky
> ---
>
> This is one patch that modifies config->ainfo.vm_gid_addr in build.c, with
> that change
> consumed by the caller (when it sets VM_PARAM_VM_GENERATION_ID_ADDR). We could
> make the caller fill out config->ainfo.vm_gid_addr to avoid this as suggested
> by Jan but
> I think logically this belongs in the ACPI builder.
Yeah, looking at the patch I think this is fine as is.
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -446,32 +446,24 @@ static int construct_secondary_tables(unsigned long
> *table_ptrs,
> *
> * Return 0 if memory failure, != 0 if success
> */
> -static int new_vm_gid(struct acpi_info *acpi_info)
> +static int new_vm_gid(struct acpi_config *config)
> {
> -uint64_t vm_gid[2], *buf;
> -const char * s;
> -char *end;
> -
> -acpi_info->vm_gid_addr = 0;
> -
> -/* read ID and check for 0 */
> -s = xenstore_read("platform/generation-id", "0:0");
> -vm_gid[0] = strtoll(s, &end, 0);
> -vm_gid[1] = 0;
> -if ( end && end[0] == ':' )
> -vm_gid[1] = strtoll(end+1, NULL, 0);
> -if ( !vm_gid[0] && !vm_gid[1] )
> +uint64_t *buf;
Afaict you no-where deref that pointer, so I can't see why it can't
be void *.
> +config->ainfo.vm_gid_addr = 0;
> +
> +/* check for 0 ID*/
> +if ( !config->vm_gid[0] && !config->vm_gid[1] )
> return 1;
>
> /* copy to allocate BIOS memory */
> -buf = (uint64_t *) mem_alloc(sizeof(vm_gid), 8);
> +buf = (uint64_t *) mem_alloc(sizeof(config->vm_gid), 8);
In any event this cast is pointless (and has been before).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] xenbus: don't bail early from xenbus_dev_request_and_reply()
On 07/07/16 14:35, Jan Beulich wrote:
On 07.07.16 at 15:22, wrote:
>> On 07/07/16 14:13, David Vrabel wrote:
>>> On 07/07/16 13:23, Jan Beulich wrote:
>>> On 07.07.16 at 14:17, wrote:
> On 07/07/16 13:09, Jan Beulich wrote:
> On 07.07.16 at 13:36, wrote:
>>> On 07/07/16 08:32, Jan Beulich wrote:
We must not skip the transaction_end() call for a failed
XS_TRANSACTION_START. The removed code fragment got introduced by
commit 027bd7e899 ("xen/xenbus: Avoid synchronous wait on XenBus
stalling shutdown/restart") without its description really indicating
why it was added (and hence I can't identify whether a more complex
change might be needed here).
>>>
>>> If sending the XS_TRANSACTION_END message failed, then the transaction
>>> is still open and transaction_end() should not be called.
>>>
>>> However, if sending an XS_TRANSACTION_START failed, then
>>> transaction_end() should be called.
>>>
>>> So, yes a more complex fix is needed here.
>>
>> Well, both of the things you name are what happens with the patch
>> in place. So if those two conditions are all that needs to be satisfied,
>> then no more complex change is needed afaict (and was the behavior
>> before the cross referenced commit) - the question really is whether
>> that other commit meant to deal with something _beyond_ those two
>> things.
>
> You call transaction_end() if msg->type == XS_TRANSACTION_END, even if
> xb_write() returned an error.
When xb_write() returns an error, msg->type gets set to XS_ERROR.
>>>
>>> So?
>>>
>>> if ((msg->type == XS_TRANSACTION_END) ||
>>> (...))
>>> transaction_end();
>>>
>>> We don't check msg->type for XS_TRANSACTION_END messages.
>>
>> Sorry, being stupid. Yeah, the change is fine but it needs a better
>> commit message.
>
> I can certainly omit the part in parentheses. I don't think I should
> omit the reference to the original commit having introduced the issue.
> And without a more specific hint I also don't know what else may
> need changing. I'm sorry, I know I'm not doing very well in writing
> commit messages to your liking.
I rewrote it as:
xenbus: don't bail early from xenbus_dev_request_and_reply()
xenbus_dev_request_and_reply() needs to track whether a transaction is
open. For XS_TRANSACTION_START messages it calls transaction_start()
and for XS_TRANSACTION_END messages it calls transaction_end().
If sending an XS_TRANSACTION_START message fails or responds with an
an error, the transaction is not open and transaction_end() must be
called.
If sending an XS_TRANSACTION_END message fails, the transaction is
still open, but if an error response is returned the transaction is
closed.
Commit 027bd7e89906 ("xen/xenbus: Avoid synchronous wait on XenBus
stalling shutdown/restart") introduced a regression where failed
XS_TRANSACTION_START messages were leaving the transaction open. This
can cause problems with suspend (and migration) as all transaction must
be closed before suspending.
It appears that the problematic change was added accidentally, so just
remove it.
David
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 10:21 AM, Jan Beulich wrote:
On 06.07.16 at 17:50, wrote:
The title of this patch keeps confusing me - which code motion is
being relocated here?
As the commit message clearly states, the code motions that are being
relocated are:
1) handling of monitor_write_data @ hvm_do_resume
2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting
CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
/* Trap CR3 updates if CR3 memory events are enabled. */
and what's removed from under it.
By 'relocation' I meant making that code vm-event specific (moving it to
vm-event specific files).
+void vmx_vm_event_update_cr3_traps(struct domain *d)
Is there anything in this function preventing the parameter to be
const?
Nope, will do.
+{
+struct vcpu *v;
+struct arch_vmx_struct *avmx;
+unsigned int cr3_bitmask;
+bool_t cr3_vmevent, cr3_ldexit;
+
+/* domain must be paused */
+ASSERT(atomic_read(&d->pause_count));
Comment style.
As in change to "/* Domain must be paused. */"?
+/* non-hap domains trap CR3 writes unconditionally */
+if ( !paging_mode_hap(d) )
+{
+#ifndef NDEBUG
+for_each_vcpu ( d, v )
+ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
+#endif
+return;
+}
+
+cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
+cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
+
+for_each_vcpu ( d, v )
+{
+avmx = &v->arch.hvm_vmx;
+cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
+
+if ( cr3_vmevent == cr3_ldexit )
+continue;
+
+/*
+ * If CR0.PE=0, CR3 load exiting must remain enabled.
+ * See vmx_update_guest_cr code motion for cr = 0.
+ */
Same as for the title - what code motion is this referring to? In a
code comment you clearly shouldn't be referring to anything the
patch effects, only to its result.
The "vmx_update_guest_cr code motion for cr = 0", that's what's
referring to.
'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
In other words, see what's happening in the function
'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand
why CR3 load-exiting must remain enabled when CR0.PE=0.
+if ( cr3_ldexit && !hvm_paging_enabled(v) &&
!vmx_unrestricted_guest(v) )
+continue;
The first sentence of the comment should be brought in line with
this condition.
Would this do (aligned with the above observation):
"
/*
* If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
* enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
*/
"
?
+static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)
Unless there is a particular reason for this uint8_t, please convert to
unsigned int.
The particular reason is cr-indexes being uint8_t typed (see
typeof(xen_domctl_monitor_op.mov_to_cr.index)).
But I will change it to unsigned int if you prefer (maybe you could
explain the preference though).
+{
+/* vmx only */
+ASSERT(cpu_has_vmx);
Comment style (more below). Should perhaps also get "for now" or
some such added.
As in "/* For now, VMX only. */"?
+static inline void write_ctrlreg_disable_traps(struct domain *d)
+{
+unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
+d->arch.monitor.write_ctrlreg_enabled = 0;
+
+if ( old )
+{
+/* vmx only */
+ASSERT(cpu_has_vmx);
Wouldn't this better move ahead of the if()?
+/* was CR3 load-exiting enabled due to monitoring? */
+if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
And then this if() alone would suffice.
No, it would be wrong because that ASSERT may not hold if "old == 0",
i.e. we only ASSERT the implication "CR-write vm-events can be enabled
-> vmx domain", but since the function is called by
arch_monitor_cleanup_domain, putting the ASSERT before the if() would
change that implication to "(any) monitor vm-events available -> vmx
domain", assertion which wouldn't be proper TBD here.
@@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
void arch_monitor_cleanup_domain(struct domain *d)
{
xfree(d->arch.monitor.msr_bitmap);
-
+write_ctrlreg_disable_traps(d);
memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
memset(&d->monitor, 0, sizeof(d->monitor));
}
Rather than deleting the blank line, perhaps better to insert another
one after your addition?
Jan
Ack.
Thanks,
Corneliu.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [ovmf test] 96773: regressions - FAIL
flight 96773 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/96773/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 test-amd64-amd64-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. vs. 94748 version targeted for testing: ovmf 1d32246161af275d7a2d17ade4a8fa53ea37a556 baseline version: ovmf dc99315b8732b6e3032d01319d3f534d440b43d0 Last test of basis94748 2016-05-24 22:43:25 Z 44 days Failing since 94750 2016-05-25 03:43:08 Z 44 days 97 attempts Testing same since96773 2016-07-07 22:46:07 Z0 days1 attempts People who touched revisions under test: Anandakrishnan Loganathan Ard Biesheuvel Bi, Dandan Bret Barkelew Bruce Cran Bruce Cran Chao Zhang Cinnamon Shia Cohen, Eugene Dandan Bi Darbin Reyes david wei Eric Dong Eugene Cohen Evan Lloyd Evgeny Yakovlev Feng Tian Fu Siyuan Fu, Siyuan Gary Li Gary Lin Giri P Mudusuru Graeme Gregory Hao Wu Hegde Nagaraj P Hegde, Nagaraj P hegdenag Heyi Guo Jan D?bro? Jan Dabros Jeff Fan Jiaxin Wu Jiewen Yao Joe Zhou Jordan Justen Katie Dellaquila Laszlo Ersek Liming Gao Lu, ShifeiX A lushifex Marcin Wojtas Mark Rutland Marvin H?user Marvin Haeuser Maurice Ma Michael Zimmermann Mudusuru, Giri P Ni, Ruiyu Qiu Shumin Ruiyu Ni Ryan Harkin Sami Mujawar Satya Yarlagadda Shannon Zhao Sriram Subramanian Star Zeng Subramanian, Sriram (EG Servers Platform SW) Sunny Wang Tapan Shah Thomas Palmer Yarlagadda, Satya P Yonghong Zhu Zhang Lubo Zhang, Chao B 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 fail test-amd64-i386-xl-qemuu-ovmf-amd64 fail 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 8610 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 04/20] acpi/hvmloader: Decide which SSDTs to install in hvmloader
>>> On 05.07.16 at 21:05, wrote:
> @@ -890,6 +895,13 @@ void hvmloader_acpi_build_tables(struct acpi_config
> *config,
> config->vm_gid[1] = strtoll(end+1, NULL, 0);
> }
>
> +if ( battery_port_exists() )
> +config->table_flags |= ACPI_BUILD_SSDT_PM;
> +if ( !strncmp(xenstore_read("platform/acpi_s3", "1"), "1", 1) )
> +config->table_flags |= ACPI_BUILD_SSDT_S3;
> +if ( !strncmp(xenstore_read("platform/acpi_s4", "1"), "1", 1) )
> +config->table_flags |= ACPI_BUILD_SSDT_S4;
I think that both here and at the consuming side this would look
better if you used bit fields. But even in its current form
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
On 7/8/2016 10:35 AM, Jan Beulich wrote:
On 06.07.16 at 17:51, wrote:
@@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v)
void vcpu_destroy(struct vcpu *v)
{
-xfree(v->arch.vm_event);
-v->arch.vm_event = NULL;
+if ( unlikely(v->arch.vm_event) )
+{
+xfree(v->arch.vm_event->emul_read_data);
+xfree(v->arch.vm_event);
+v->arch.vm_event = NULL;
+}
Considering the repeat of this pattern ...
@@ -52,8 +58,25 @@ void vm_event_cleanup_domain(struct domain *d)
for_each_vcpu ( d, v )
{
-xfree(v->arch.vm_event);
-v->arch.vm_event = NULL;
+if ( likely(!v->arch.vm_event) )
+continue;
+
+/*
+ * Only xfree the entire arch_vm_event if write_data was fully handled.
+ * Otherwise defer entire xfree until domain/vcpu destroyal.
+ */
+if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
+{
+xfree(v->arch.vm_event->emul_read_data);
+xfree(v->arch.vm_event);
+v->arch.vm_event = NULL;
+continue;
+}
... here, please consider making this another helper (inline?) function.
Yeah, I'm sending a separate patch today which will invalidate some of
these changes (then a v4 above that one).
+/* write_data not fully handled, only xfree emul_read_data */
Comment style again (and more below).
Ack, assuming you mean 'capital letter, end with dot'.
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v,
unsigned int port)
/* Clean up on domain destruction */
void vm_event_cleanup(struct domain *d)
{
+struct vcpu *v;
+
#ifdef CONFIG_HAS_MEM_PAGING
if ( d->vm_event->paging.ring_page )
{
@@ -560,6 +562,16 @@ void vm_event_cleanup(struct domain *d)
(void)vm_event_disable(d, &d->vm_event->share);
}
#endif
+
+for_each_vcpu ( d, v )
+{
+if ( unlikely(v->arch.vm_event) )
+{
+/* vm_event->emul_read_data freed in vm_event_cleanup_domain */
Perhaps worthwhile adding a respective ASSERT()?
Good point, ack.
+static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
+{
+return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
+}
+
+static inline bool_t vm_event_domain_initialised(struct domain *d)
+{
+return (d->max_vcpus && d->vcpu[0] &&
+vm_event_vcpu_initialised(d->vcpu[0]));
+}
Both functions' parameters should be const. Pointless parentheses
in both return statements.
Jan
Ack (although the parenthesis were there strictly for aesthetics, but
that's subjective).
Thanks,
Corneliu.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] Infrastructure Maintenance Window: July 12. 2016 12:00 UTC to July 13, 2016 9:00 UTC
Hi everyone, just a quick note that we will perform the following maintenance activities from July 12. 2016 12:00 UTC to July 13, 2016 9:00 UTC (which is 13:00 BST and 10:00 BST respectively). The following work will be performed: - Various HW maintenance activities to the Xen Project Test Infrastructure, which means that OSSTEST will be down for some or most of this time period - OS Upgrade of blog.xenproject.org to Debian Jessie (this will take 1-2 hours) - OS Upgrade of bugs.xenproject.org to Debian Jessie (this will take 1-2 hours) Best Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 7/8] minor fixes (formatting, comments, unused includes etc.)
On 7/8/2016 10:56 AM, Jan Beulich wrote: On 06.07.16 at 17:55, wrote: --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -31,7 +31,6 @@ #include #include #include -#include #include #include #include @@ -63,11 +62,9 @@ #include #include #include -#include #include #include #include -#include #include #include #include The inclusion of asm/vm_event.h should really be removed in patch 2, I had to drop it here, to get the patch to apply ahead of the earlier ones in this series. --- a/xen/arch/x86/monitor.c +++ b/xen/arch/x86/monitor.c @@ -21,7 +21,6 @@ #include #include -#include #include static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Similarly I had to drop this hunk, which suggests that one of the earlier patches needlessly adds that #include (or it gets validly added and then a later patch makes it unnecessary again). Jan Thanks for applying these ahead, it spares me having to include them in future series. As for the issue, yeah that's probably what happened along the way although it was to be expected (applying patches from a series 'unsequentially') so again, thanks for the effort. Will keep in mind having to take care of the changes that didn't make it in a next series. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
>>> On 08.07.16 at 12:22, wrote:
> On 7/8/2016 10:21 AM, Jan Beulich wrote:
> On 06.07.16 at 17:50, wrote:
>> The title of this patch keeps confusing me - which code motion is
>> being relocated here?
>
> As the commit message clearly states, the code motions that are being
> relocated are:
Again this sentence makes no sense to me: I can't see how
"code motions" can be "relocated", just like I don't see how you
could move a move. But maybe it's just me...
> 1) handling of monitor_write_data @ hvm_do_resume
> 2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting
> CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
> /* Trap CR3 updates if CR3 memory events are enabled. */
> and what's removed from under it.
>
> By 'relocation' I meant making that code vm-event specific (moving it to
> vm-event specific files).
Yes, that what I've guessed.
>>> +{
>>> +struct vcpu *v;
>>> +struct arch_vmx_struct *avmx;
>>> +unsigned int cr3_bitmask;
>>> +bool_t cr3_vmevent, cr3_ldexit;
>>> +
>>> +/* domain must be paused */
>>> +ASSERT(atomic_read(&d->pause_count));
>> Comment style.
>
> As in change to "/* Domain must be paused. */"?
Yes, as mandated by ./CODING_STYLE.
>>> +/* non-hap domains trap CR3 writes unconditionally */
>>> +if ( !paging_mode_hap(d) )
>>> +{
>>> +#ifndef NDEBUG
>>> +for_each_vcpu ( d, v )
>>> +ASSERT(v->arch.hvm_vmx.exec_control &
>>> CPU_BASED_CR3_LOAD_EXITING);
>>> +#endif
>>> +return;
>>> +}
>>> +
>>> +cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
>>> +cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
>>> +
>>> +for_each_vcpu ( d, v )
>>> +{
>>> +avmx = &v->arch.hvm_vmx;
>>> +cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
>>> +
>>> +if ( cr3_vmevent == cr3_ldexit )
>>> +continue;
>>> +
>>> +/*
>>> + * If CR0.PE=0, CR3 load exiting must remain enabled.
>>> + * See vmx_update_guest_cr code motion for cr = 0.
>>> + */
>> Same as for the title - what code motion is this referring to? In a
>> code comment you clearly shouldn't be referring to anything the
>> patch effects, only to its result.
>
> The "vmx_update_guest_cr code motion for cr = 0", that's what's
> referring to.
So I guess my problem really is that I don't understand what a
"code motion" is (other than the act of moving code from one
place to another).
> 'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
> In other words, see what's happening in the function
> 'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand
> why CR3 load-exiting must remain enabled when CR0.PE=0.
>
>>
>>> +if ( cr3_ldexit && !hvm_paging_enabled(v) &&
>>> !vmx_unrestricted_guest(v) )
>>> +continue;
>> The first sentence of the comment should be brought in line with
>> this condition.
>
> Would this do (aligned with the above observation):
>
> "
>
> /*
> * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
> * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
> */
>
> "
> ?
Not really: The condition checks whether paging is enabled and
whether it's an unrestricted guest. The comment talks about
protected mode being enabled.
>>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
> index)
>> Unless there is a particular reason for this uint8_t, please convert to
>> unsigned int.
>
> The particular reason is cr-indexes being uint8_t typed (see
> typeof(xen_domctl_monitor_op.mov_to_cr.index)).
> But I will change it to unsigned int if you prefer (maybe you could
> explain the preference though).
No use of fixed width types when fixed width types aren't really
required. Generally generated code is less efficient when having
to deal with fixed width types.
>>> +{
>>> +/* vmx only */
>>> +ASSERT(cpu_has_vmx);
>> Comment style (more below). Should perhaps also get "for now" or
>> some such added.
>
> As in "/* For now, VMX only. */"?
For example, yes.
>>> +static inline void write_ctrlreg_disable_traps(struct domain *d)
>>> +{
>>> +unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
>>> +d->arch.monitor.write_ctrlreg_enabled = 0;
>>> +
>>> +if ( old )
>>> +{
>>> +/* vmx only */
>>> +ASSERT(cpu_has_vmx);
>> Wouldn't this better move ahead of the if()?
>>
>>> +/* was CR3 load-exiting enabled due to monitoring? */
>>> +if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
>> And then this if() alone would suffice.
>
> No, it would be wrong because that ASSERT may not hold if "old == 0",
> i.e. we only ASSERT the implication "CR-write vm-events can be enabled
> -> vmx domain", but since the function is called by
> arch_monitor_cleanup_domain, putting the ASSERT before the if() would
> change t
Re: [Xen-devel] [PATCH v3 3/8] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup
>>> On 08.07.16 at 12:28, wrote: > On 7/8/2016 10:35 AM, Jan Beulich wrote: > On 06.07.16 at 17:51, wrote: >>> +/* write_data not fully handled, only xfree emul_read_data */ >> Comment style again (and more below). > > Ack, assuming you mean 'capital letter, end with dot'. Actually in this particular case I meant only the missing full stop, as the first word - afaict - refers to a structure field name (and hence should remain lower case so e.g. a grep would catch it). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor
Hi Michael and Julien, On 08.07.2016 11:34, Julien Grall wrote: Hi Dirk, On 08/07/16 08:44, Dirk Behme wrote: Xen hypervisor drivers might replace native OS drivers. The result is that some important clocks that are enabled by the OS in the non-Xen case are not properly enabled in the presence of Xen. The clocks property enumerates the clocks that must be enabled by the Xen clock consumer. An example is a serial driver enabled by the hypervisor. Xen must I would say "An example is the UART used by the hypervisor." consume and enable these clocks in the OS to ensure behavior continues after firmware configures the UART hardware and corresponding clock harder. What do you mean by "harder"? Also, relying on DOM0 to enable the clock looks very wrong to me and you give an example which prove that. The UART will be used before hand by Xen, however it will not be possible to use it if you expect DOM0 to enable the clock (or even modify the clock frequency). The clock should be enabled either by the firmware or Xen. But not DOM0. DOM0 should not touch this clock at all. Furthermore, this property could be used for clock associated to device that will be passthrough-ed to a guest. In this case, the clock would be enabled even if the device is not in use which will result more power consumption. I took the description directly from Michael's proposal http://www.spinics.net/lists/arm-kernel/msg516576.html Would it be possible that you two experts agree on the exact wording you like to see? Best regards Dirk Up to now, the workaround for this has been to use the Linux kernel command line parameter 'clk_ignore_unused'. See Xen bug http://bugs.xenproject.org/xen/bug/45 too. To fix this, we will add the "unused" clocks in Xen to the hypervisor node. The OS has to consume and enable the clocks from the hypervisor node, then. Therefore, check if there is a "clocks" entry in the hypervisor node and if so consume and enable the given clocks. This prevents the clocks from being disabled by the OS. Signed-off-by: Dirk Behme --- Changes in v3: Use the xen.txt description proposed by Michael. Thanks! Changes in v2: Drop the Linux implementation details like clk_disable_unused in xen.txt. Documentation/devicetree/bindings/arm/xen.txt | 13 arch/arm/xen/enlighten.c | 47 +++ 2 files changed, 60 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/xen.txt b/Documentation/devicetree/bindings/arm/xen.txt index c9b9321..00f2165 100644 --- a/Documentation/devicetree/bindings/arm/xen.txt +++ b/Documentation/devicetree/bindings/arm/xen.txt @@ -17,6 +17,19 @@ the following properties: A GIC node is also required. This property is unnecessary when booting Dom0 using ACPI. +Optional properties: + +clocks: one or more clocks to be enabled + Xen hypervisor drivers might replace native OS drivers. The result is "native OS" has no meaning on Xen. This seems to be a cumbersome way to say that the device will be used by the hypervisor and hidden to DOM0 (aka hardware domain). + that some important clocks that are enabled by the OS in the non-Xen + case are not properly enabled in the presence of Xen. The clocks + property enumerates the clocks that must be enabled by the Xen clock + consumer. + An example is a serial driver enabled by the hypervisor. Xen must + consume and enable these clocks in the OS to ensure behavior continues + after firmware configures the UART hardware and corresponding clock + harder. + To support UEFI on Xen ARM virtual platforms, Xen populates the FDT "uefi" node under /hypervisor with following parameters: Regards, ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 06/17] libxl/arm: Estimate the size of ACPI tables
On Thu, Jul 07, 2016 at 05:39:38PM +0100, Julien Grall wrote:
> Hi Wei,
>
> On 07/07/16 17:07, Wei Liu wrote:
> >On Tue, Jul 05, 2016 at 11:12:36AM +0800, Shannon Zhao wrote:
> >> int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> >> libxl__domain_build_state *state,
> >> struct xc_dom_image *dom)
> >> {
> >> const libxl_version_info *vers;
> >>+int rc;
> >>+struct acpitable acpitables[NUMS];
> >>+
> >>+/* convenience aliases */
> >>+xc_domain_configuration_t *xc_config = &state->config;
> >>
> >
> >Julien seemed to have suggested this structure shouldn't be used. Did I
> >misremember?
>
> On the previous version, I said we should not extend this structure for
> parameter which is not used by the hypervisor (e.g the hypervisor does not
> need to know whether the guest will use ACPI).
>
> However, this structure have to be used to get the version of the GIC that
> will be emulated for the guest. This is because the hypervisor might choose
> the version if the user did not specify one.
>
OK. Makes sense.
Wei.
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 05/20] acpi/hvmloader: Move passthrough initialization from ACPI code
>>> On 05.07.16 at 21:05, wrote:
> Initialize it in hvmloader, avoiding ACPI code's use of xenstore_read()
>
> Signed-off-by: Boris Ostrovsky
> ---
>
> Changes in v1:
> * Rename acpi_pt_length/addr to pt_length/addr
And stopped half way? I ask because ...
> @@ -280,19 +280,11 @@ static int construct_passthrough_tables(unsigned long
> *table_ptrs,
> uint32_t total = 0;
> uint8_t *buffer;
>
> -s = xenstore_read(HVM_XS_ACPI_PT_ADDRESS, NULL);
> -if ( s == NULL )
> -return 0;
> -
> -acpi_pt_addr = (uint8_t*)(uint32_t)strtoll(s, NULL, 0);
> -if ( acpi_pt_addr == NULL )
> -return 0;
> -
> -s = xenstore_read(HVM_XS_ACPI_PT_LENGTH, NULL);
> -if ( s == NULL )
> +if ( config->pt.pt_addr == 0 )
> return 0;
>
> -acpi_pt_length = (uint32_t)strtoll(s, NULL, 0);
> +acpi_pt_addr = config->pt.pt_addr;
> +acpi_pt_length = config->pt.pt_length;
... expressions like the right side ones here look like K&R C to me -
the pt_ prefixes are redundant too. With that taken care of and ...
> @@ -895,6 +896,16 @@ void hvmloader_acpi_build_tables(struct acpi_config
> *config,
> config->vm_gid[1] = strtoll(end+1, NULL, 0);
> }
>
> +s = xenstore_read(HVM_XS_ACPI_PT_ADDRESS, NULL);
> +if ( s )
> +{
> +config->pt.pt_addr = strtoll(s, NULL, 0);
> +
> +s = xenstore_read(HVM_XS_ACPI_PT_LENGTH, NULL);
> +if ( s )
... the double blank here reduced to just one
Reviewed-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 17/17] libxl/arm: Initialize domain param HVM_PARAM_CALLBACK_IRQ
On Thu, Jul 07, 2016 at 05:57:09PM +0100, Julien Grall wrote:
> Hi Wei,
>
> On 07/07/16 17:15, Wei Liu wrote:
> >On Tue, Jul 05, 2016 at 11:12:47AM +0800, Shannon Zhao wrote:
> >>From: Shannon Zhao
> >>
> >>The guest kernel will get the event channel interrupt information via
> >>domain param HVM_PARAM_CALLBACK_IRQ. Initialize it here.
> >>
> >>Signed-off-by: Shannon Zhao
> >>---
> >> tools/libxl/libxl_arm.c | 11 +++
> >> 1 file changed, 11 insertions(+)
> >>
> >>diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >>index bc38318..acacba0 100644
> >>--- a/tools/libxl/libxl_arm.c
> >>+++ b/tools/libxl/libxl_arm.c
> >>@@ -900,8 +900,19 @@ int libxl__arch_domain_init_hw_description(libxl__gc
> >>*gc,
> >> struct xc_dom_image *dom)
> >> {
> >> int rc;
> >>+uint64_t val;
> >>
> >> assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> >>+
> >>+/* Set the value of domain param HVM_PARAM_CALLBACK_IRQ. */
> >>+val = (uint64_t)HVM_PARAM_CALLBACK_TYPE_PPI << 56;
> >>+val |= (2 << 8); /* Active-low level-sensitive */
> >
> >Please avoid using magic numbers here -- 56, 2 and 8.
>
> The magic numbers are described in public/hvm/params.h however there is no
> defines associated to them.
>
> The public header would need to be updated if we don't want the value
> hardcoded in libxl.
>
Either update the public header or have some local #defines plus
appropriate comments on the what the actual source of those numbers is.
FWIW I certainly prefer the formal option.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3] xen/arm: enable clocks used by the hypervisor
On 08/07/16 11:40, Dirk Behme wrote: Hi Michael and Julien, On 08.07.2016 11:34, Julien Grall wrote: Hi Dirk, On 08/07/16 08:44, Dirk Behme wrote: Xen hypervisor drivers might replace native OS drivers. The result is that some important clocks that are enabled by the OS in the non-Xen case are not properly enabled in the presence of Xen. The clocks property enumerates the clocks that must be enabled by the Xen clock consumer. An example is a serial driver enabled by the hypervisor. Xen must I would say "An example is the UART used by the hypervisor." consume and enable these clocks in the OS to ensure behavior continues after firmware configures the UART hardware and corresponding clock harder. What do you mean by "harder"? Also, relying on DOM0 to enable the clock looks very wrong to me and you give an example which prove that. The UART will be used before hand by Xen, however it will not be possible to use it if you expect DOM0 to enable the clock (or even modify the clock frequency). The clock should be enabled either by the firmware or Xen. But not DOM0. DOM0 should not touch this clock at all. Furthermore, this property could be used for clock associated to device that will be passthrough-ed to a guest. In this case, the clock would be enabled even if the device is not in use which will result more power consumption. I took the description directly from Michael's proposal http://www.spinics.net/lists/arm-kernel/msg516576.html Would it be possible that you two experts agree on the exact wording you like to see? I think the wording suggested by Mark in [1] represents better what we would like to achieve with this property. Regards, [1] https://www.spinics.net/lists/arm-kernel/msg516158.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote:
> > This patch use xc_dom_alloc_segment() to allocate the memory space for the
> > ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> > placement of 1MB (+ extra for MB alignement) after the hvmloader image.
> >
> > This patch can help if one add extra ACPI table and hvmloader contain
> > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
> > easily be loaded past the address 4MB, but hvmloader use a range of
> > memory from 4MB to 10MB to perform tests and in the process, clears the
> > memory, before loading the modules.
> >
> > Signed-off-by: Anthony PERARD
> > ---
> > Changes in V5:
> > - rewrite patch description
> >
> > Changes in V4:
> > - check that guest_addr_out have a reasonable value.
> >
> > New patch in V3.
> > ---
> > tools/libxc/xc_dom_hvmloader.c | 131
> > -
> > 1 file changed, 38 insertions(+), 93 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_hvmloader.c b/tools/libxc/xc_dom_hvmloader.c
> > index 330d5e8..da8b995 100644
> > --- a/tools/libxc/xc_dom_hvmloader.c
> > +++ b/tools/libxc/xc_dom_hvmloader.c
> > @@ -129,98 +129,52 @@ static elf_errorstatus xc_dom_parse_hvm_kernel(struct
> > xc_dom_image *dom)
> > return rc;
> > }
> >
> > -static int modules_init(struct xc_dom_image *dom,
> > -uint64_t vend, struct elf_binary *elf,
> > -uint64_t *mstart_out, uint64_t *mend_out)
> > +static int module_init_one(struct xc_dom_image *dom,
> > + struct xc_hvm_firmware_module *module,
> > + char *name)
> > {
> > -#define MODULE_ALIGN 1UL << 7
> > -#define MB_ALIGN 1UL << 20
> > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> > -uint64_t total_len = 0, offset1 = 0;
> > +struct xc_dom_seg seg;
> > +void *dest;
> >
> > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> > -return 0;
> > -
> > -/* Find the total length for the firmware modules with a reasonable
> > large
> > - * alignment size to align each the modules.
> > - */
> > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> > -offset1 = total_len;
> > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> > -
> > -/* Want to place the modules 1Mb+change behind the loader image. */
> > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> > -*mend_out = *mstart_out + total_len;
> > -
> > -if ( *mend_out > vend )
> > -return -1;
> > -
> > -if ( dom->acpi_module.length != 0 )
> > -dom->acpi_module.guest_addr_out = *mstart_out;
> > -if ( dom->smbios_module.length != 0 )
> > -dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> > +if ( module->length )
> > +{
> > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> > +goto err;
> > +dest = xc_dom_seg_to_ptr(dom, &seg);
> > +if ( dest == NULL )
> > +{
> > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> > + __FUNCTION__);
> > +goto err;
> > +}
> > +memcpy(dest, module->data, module->length);
> > +module->guest_addr_out = seg.vstart;
> > +if ( module->guest_addr_out > UINT32_MAX ||
> > + module->guest_addr_out + module->length > UINT32_MAX )
> > +{
> > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> > + __FUNCTION__, name);
> > +goto err;
> > +}
>
> One question:
>
> Can this check also account for MMIO hole below 4G? Maybe use
> dom->mmio_size?
Yes, I guess I can check against dom->mmio_start. Should I also check
that mmio_start have reasonable value? (<4G, and not 0x0) Or is
mmio_start is already supposed to have a good value?
> Other than this, this patch looks good.
Thanks,
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 20/20] libxl/acpi: Build ACPI tables for HVMlite guests
On Tue, Jul 05, 2016 at 03:05:19PM -0400, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky
> ---
>
> Changes in v1:
> * Move to libxl
> * Added populate_acpi_pages()
> * Stash location/size of tables in xc_dom_image (to be used in constructing
> e820 map)
> * Use libxl allocator
> * Only set XEN_X86_EMU_LAPIC flag if 'apic' option is set.
> * Make acpi_build_tables() return error code
>
> .gitignore |4 +
> tools/libacpi/build.c|7 +-
> tools/libacpi/libacpi.h | 15 ++-
> tools/libxl/Makefile | 17 +++-
> tools/libxl/libxl_arch.h |3 +
> tools/libxl/libxl_dom.c |1 +
> tools/libxl/libxl_x86.c | 29 +++--
> tools/libxl/libxl_x86_acpi.c | 292
> ++
> tools/libxl/libxl_x86_acpi.h | 21 +++
> 9 files changed, 373 insertions(+), 16 deletions(-)
> create mode 100644 tools/libxl/libxl_x86_acpi.c
> create mode 100644 tools/libxl/libxl_x86_acpi.h
>
> diff --git a/.gitignore b/.gitignore
> index 9dd2086..d4da37f 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -179,6 +179,10 @@ tools/libxl/testenum.c
> tools/libxl/tmp.*
> tools/libxl/_libxl.api-for-check
> tools/libxl/*.api-ok
> +tools/libxl/mk_dsdt
> +tools/libxl/dsdt*.c
> +tools/libxl/dsdt_*.asl
> +tools/libxl/ssdt_*.h
Please sort these alphabetically.
> tools/misc/cpuperf/cpuperf-perfcntr
> tools/misc/cpuperf/cpuperf-xen
> tools/misc/xc_shadow
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index 290f005..a6ddf53 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -23,6 +23,7 @@
> #include "ssdt_tpm.h"
> #include "ssdt_pm.h"
> #include "x86.h"
> +#include
> #include
> #include
>
[...]
> +int libxl__dom_load_acpi(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_dom_image *dom);
Indentation.
> #endif
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index ba3472e..7e4e289 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -657,6 +657,7 @@ static int libxl__build_dom(libxl__gc *gc, uint32_t domid,
> LOGE(ERROR, "xc_dom_build_image failed");
> goto out;
> }
> +
Stray blank line.
> if ( (ret = xc_dom_boot_image(dom)) != 0 ) {
> LOGE(ERROR, "xc_dom_boot_image failed");
> goto out;
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 32ce1d2..7201dbb 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
[...]
> +int libxl__dom_load_acpi(libxl__gc *gc,
> + libxl_domain_build_info *info,
> + struct xc_dom_image *dom)
> +{
> +struct acpi_config config = {0};
> +struct acpi_ctxt ctxt;
> +uint32_t domid = dom->guest_domid;
> +xc_interface *xch = dom->xch;
> +int rc, i, acpi_pages_num;
> +xen_pfn_t extent, *extents;
> +void *acpi_pages, *guest_acpi_pages = NULL;
> +unsigned long page_mask;
> +
> +if ((info->type != LIBXL_DOMAIN_TYPE_HVM) ||
> +(info->device_model_version != LIBXL_DEVICE_MODEL_VERSION_NONE))
> +return 0;
> +
> +ctxt.page_size = XC_DOM_PAGE_SIZE(dom);
> +ctxt.page_shift = XC_DOM_PAGE_SHIFT(dom);
> +page_mask = (1UL << ctxt.page_shift) - 1;
> +
> +ctxt.mem_ops.alloc = mem_alloc;
> +ctxt.mem_ops.v2p = virt_to_phys;
> +
> +rc = init_acpi_config(gc, dom, info, &config);
> +if (rc) {
> +LOG(ERROR, "%s: init_acpi_config failed (rc=%d)", __FUNCTION__, rc);
> +return rc;
> +}
> +
> +/* Map page that will hold RSDP */
> +extent = RSDP_ADDRESS >> ctxt.page_shift;
> +rc = populate_acpi_pages(dom, &extent, 1, &ctxt);
> +if (rc) {
> +LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d",
> +__FUNCTION__, rc);
> +goto out;
> +}
> +config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid,
> + ctxt.page_size,
> + PROT_READ | PROT_WRITE,
> + RSDP_ADDRESS >>
> ctxt.page_shift);
I think with Anthony's on-going work you should be more flexible for all
you tables.
I haven't really looked into details for this patch. Let's sort out
the linkage issue between GPLv2 cod and LGPL code first.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 02/14] libxc: Prepare a start info structure for hvmloader
On Thu, Jul 07, 2016 at 03:55:29PM +0100, Wei Liu wrote: > On Wed, Jun 22, 2016 at 06:15:33PM +0100, Anthony PERARD wrote: > [...] > > * Allocate and clear additional ioreq server pages. The default > > * server will use the IOREQ and BUFIOREQ special pages above. > > @@ -672,6 +675,14 @@ static int alloc_magic_pages_hvm(struct xc_dom_image > > *dom) > > NR_IOREQ_SERVER_PAGES); > > } > > > > +rc = xc_dom_alloc_segment(dom, &dom->start_info_seg, > > + "HVMlite start info", 0, start_info_size); > > Should we just call this "HVM start info"? It is not restricted to > hvmlite anymore. Yes, I think that would make more sense. I'll change that. -- Anthony PERARD ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 0/2] xenbus: xenbus_dev_request_and_reply() adjustments
On 07/07/16 08:25, Jan Beulich wrote: > 1: don't bail early from xenbus_dev_request_and_reply() > 2: simplify xenbus_dev_request_and_reply() Applied to for-linus-4.7b, thanks. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable baseline-only test] 66531: trouble: broken/preparing/queued
This run is configured for baseline tests only. flight 66531 xen-unstable running [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/66531/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt-xsm queued build-armhf-libvirt queued test-armhf-armhf-xl-xsm queued test-amd64-i386-rumpuserxen-i386 queued test-amd64-amd64-rumpuserxen-amd64 queued test-armhf-armhf-xl-rtds queued test-armhf-armhf-xl-multivcpu queued test-armhf-armhf-libvirt queued test-armhf-armhf-libvirt-qcow2 queued test-armhf-armhf-xl-vhd queued test-armhf-armhf-xl-midwayqueued test-armhf-armhf-xl queued test-armhf-armhf-libvirt-raw queued test-armhf-armhf-xl-credit2 queued build-amd64-libvirt queued build-i386-libvirtqueued test-amd64-amd64-xl-pvh-amd queued test-amd64-amd64-xl-pvh-intel queued build-i386-rumpuserxenqueued test-amd64-i386-qemut-rhel6hvm-intel queued build-amd64-rumpuserxen queued test-amd64-i386-libvirt-xsm queued test-amd64-i386-pair queued test-amd64-amd64-xl-credit2 queued test-amd64-amd64-migrupgrade queued test-amd64-i386-libvirt queued test-amd64-amd64-xl-multivcpu queued test-amd64-amd64-libvirt-xsm queued test-amd64-amd64-xl-xsm queued test-amd64-amd64-libvirt queued test-amd64-i386-xl-xsmqueued test-amd64-i386-qemut-rhel6hvm-amd queued test-amd64-i386-qemuu-rhel6hvm-amd queued test-amd64-amd64-pygrub queued test-amd64-amd64-xl-rtds queued test-amd64-i386-freebsd10-i386 queued test-amd64-amd64-xl-qemut-stubdom-debianhvm-amd64-xsmqueued test-amd64-i386-freebsd10-amd64 queued test-amd64-amd64-xl queued test-amd64-amd64-qemuu-nested-amd queued test-amd64-i386-qemuu-rhel6hvm-intel queued test-amd64-amd64-amd64-pvgrub queued test-amd64-amd64-xl-qemut-debianhvm-amd64queued test-amd64-amd64-i386-pvgrub queued test-amd64-i386-libvirt-pair queued test-amd64-amd64-libvirt-pair queued test-amd64-amd64-xl-qcow2 queued test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm queued test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-xl-qemut-debianhvm-amd64 queued test-amd64-i386-xl-rawqueued test-amd64-i386-xl-qemut-debianhvm-amd64-xsm queued test-amd64-amd64-xl-qemut-win7-amd64 queued test-amd64-i386-xl-qemuu-win7-amd64 queued test-amd64-amd64-xl-qemuu-win7-amd64 queued test-amd64-i386-xl-qemut-win7-amd64 queued test-amd64-i386-xl-qemut-winxpsp3-vcpus1 queued test-amd64-i386-xl-qemuu-debianhvm-amd64 queued test-amd64-amd64-pair queued test-amd64-i386-migrupgrade queued test-amd64-amd64-xl-qemuu-ovmf-amd64 queued test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm queued test-amd64-i386-xl-qemuu-ovmf-amd64 queued test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm queued test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-xlqueued test-amd64-amd64-qemuu-nested-intel queued test-amd64-amd64-xl-qemuu-debianhvm-amd64queued test-amd64-amd64-xl-qemut-debianhvm-amd64-xsmqueued test-amd64-amd64-libvirt-vhd queued test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 queued test-amd64-amd64-xl-qemuu-winxpsp3 queued test-amd64-i386-xl-qemuu-winxpsp3 queued test-amd64-i386-xl-qemut-winxpsp3 queued test-amd64-amd64-xl-qemut-winxpsp3 queued build-armhf 2 hosts-allocate running build-armhf-xsm 2 hosts-allocate running build-armhf-pvops 2 hosts-allocate running build-amd64-oldkern 2 hosts-allocate running build-amd64-prev 2 hosts-allocate running build-amd64-pvops 2 hosts-al
[Xen-devel] [seabios baseline-only test] 66543: trouble: broken/preparing/queued
This run is configured for baseline tests only. flight 66543 seabios running [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/66543/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirtqueued build-amd64-libvirt queued test-amd64-i386-qemuu-rhel6hvm-amd queued test-amd64-amd64-qemuu-nested-amd queued test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-xl-qemuu-debianhvm-amd64 queued test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm queued test-amd64-amd64-xl-qemuu-win7-amd64 queued test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-qemuu-rhel6hvm-intel queued test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm queued test-amd64-i386-xl-qemuu-win7-amd64 queued test-amd64-amd64-xl-qemuu-debianhvm-amd64queued test-amd64-i386-xl-qemuu-ovmf-amd64 queued test-amd64-amd64-qemuu-nested-intel queued test-amd64-amd64-xl-qemuu-ovmf-amd64 queued test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 queued test-amd64-i386-xl-qemuu-winxpsp3 queued test-amd64-amd64-xl-qemuu-winxpsp3 queued build-amd64 2 hosts-allocate running build-i386-pvops 2 hosts-allocate running build-amd64-xsm 2 hosts-allocate running build-i386-xsm2 hosts-allocate running build-i3862 hosts-allocate running build-amd64-pvops 2 hosts-allocate running version targeted for testing: seabios 13213a252286372efa5f72b4119faafd5dff5db1 baseline version: seabios 20f83d5c7c0f9ae5f775b6701c205349abe003fb Last test of basis66515 2016-07-03 16:19:58 Z4 days Testing same since0 1970-01-01 00:00:00 Z 16990 days0 attempts People who touched revisions under test: Kevin O'Connor jobs: build-amd64-xsm preparing build-i386-xsm preparing build-amd64 broken build-i386 preparing build-amd64-libvirt queued build-i386-libvirt queued build-amd64-pvopspreparing build-i386-pvops preparing test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm queued test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmqueued test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm queued test-amd64-amd64-qemuu-nested-amdqueued test-amd64-i386-qemuu-rhel6hvm-amd queued test-amd64-amd64-xl-qemuu-debianhvm-amd64queued test-amd64-i386-xl-qemuu-debianhvm-amd64 queued test-amd64-amd64-xl-qemuu-ovmf-amd64 queued test-amd64-i386-xl-qemuu-ovmf-amd64 queued test-amd64-amd64-xl-qemuu-win7-amd64 queued test-amd64-i386-xl-qemuu-win7-amd64 queued test-amd64-amd64-qemuu-nested-intel queued test-amd64-i386-qemuu-rhel6hvm-intel queued test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 queued test-amd64-amd64-xl-qemuu-winxpsp3 queued test-amd64-i386-xl-qemuu-winxpsp3queued sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary broken-job build-i386-libvirt queued broken-job build-amd64-libvirt queued broken-job test-amd64-i386-qemuu-rhel6hvm-amd queued broken-job test-amd64-amd64-qemuu-nested-amd queued broken-job test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm queued broken-job test-amd64-i386-xl-qemuu-debianhvm-amd64 queued broken-job test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm queued broken-job test-amd64-amd64-xl-qemuu-win7-amd64 queued broken-job test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm queued broken-job test-amd64-i386-qemuu-rhel6hvm-intel queued broken-job tes
[Xen-devel] [distros-debian-jessie test] 66544: trouble: broken/preparing/queued
flight 66544 distros-debian-jessie running [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/66544/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-amd64-jessie-netboot-pvgrub queued test-amd64-amd64-i386-jessie-netboot-pygrub queued test-amd64-i386-i386-jessie-netboot-pvgrub queued test-armhf-armhf-armhf-jessie-netboot-pygrub queued test-amd64-i386-amd64-jessie-netboot-pygrub queued build-armhf 2 hosts-allocate running build-amd64-pvops 2 hosts-allocate running build-i3862 hosts-allocate running build-amd64 2 hosts-allocate running build-armhf-pvops 2 hosts-allocate running build-i386-pvops 2 hosts-allocate running baseline version: flight 44412 jobs: build-amd64 broken build-armhf preparing build-i386 preparing build-amd64-pvopspreparing build-armhf-pvopspreparing build-i386-pvops preparing test-amd64-amd64-amd64-jessie-netboot-pvgrub queued test-amd64-i386-i386-jessie-netboot-pvgrub queued test-amd64-i386-amd64-jessie-netboot-pygrub queued test-armhf-armhf-armhf-jessie-netboot-pygrub queued test-amd64-amd64-i386-jessie-netboot-pygrub queued sg-report-flight on osstest.xs.citrite.net logs: /home/osstest/logs images: /home/osstest/images Logs, config files, etc. are available at http://osstest.xs.citrite.net/~osstest/testlogs/logs Test harness code can be found at http://xenbits.xensource.com/gitweb?p=osstest.git;a=summary Push not applicable. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline baseline-only test] 66532: trouble: broken/preparing/queued
This run is configured for baseline tests only. flight 66532 qemu-mainline running [real] http://osstest.xs.citrite.net/~osstest/testlogs/logs/66532/ Failures and problems with tests :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt-xsm queued test-armhf-armhf-xl-xsm queued build-armhf-libvirt queued test-armhf-armhf-xl-rtds queued test-armhf-armhf-xl-multivcpu queued test-armhf-armhf-xl-vhd queued test-armhf-armhf-xl-midwayqueued test-armhf-armhf-libvirt queued test-armhf-armhf-libvirt-qcow2 queued test-armhf-armhf-xl-credit2 queued test-armhf-armhf-xl queued test-armhf-armhf-libvirt-raw queued build-amd64-libvirt queued build-i386-libvirtqueued test-amd64-amd64-xl-pvh-intel queued test-amd64-amd64-xl-pvh-amd queued test-amd64-amd64-xl queued test-amd64-i386-qemuu-rhel6hvm-intel queued test-amd64-i386-xlqueued test-amd64-amd64-xl-multivcpu queued test-amd64-amd64-xl-credit2 queued test-amd64-i386-xl-xsmqueued test-amd64-amd64-xl-rtds queued test-amd64-amd64-libvirt queued test-amd64-amd64-libvirt-xsm queued test-amd64-i386-libvirt queued test-amd64-amd64-xl-xsm queued test-amd64-i386-qemuu-rhel6hvm-amd queued test-amd64-i386-libvirt-xsm queued test-amd64-i386-xl-qemuu-ovmf-amd64 queued test-amd64-amd64-xl-qemuu-ovmf-amd64 queued test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmqueued test-amd64-amd64-qemuu-nested-intel queued test-amd64-i386-freebsd10-i386 queued test-amd64-amd64-i386-pvgrub queued test-amd64-i386-freebsd10-amd64 queued test-amd64-amd64-qemuu-nested-amd queued test-amd64-amd64-pair queued test-amd64-amd64-amd64-pvgrub queued test-amd64-amd64-pygrub queued test-amd64-i386-pair queued test-amd64-amd64-libvirt-pair queued test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm queued test-amd64-amd64-libvirt-vhd queued test-amd64-i386-xl-rawqueued test-amd64-amd64-xl-qemuu-debianhvm-amd64queued test-amd64-i386-xl-qemuu-winxpsp3 queued test-amd64-i386-xl-qemuu-win7-amd64 queued test-amd64-amd64-xl-qemuu-win7-amd64 queued test-amd64-i386-libvirt-pair queued test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm queued test-amd64-amd64-xl-qcow2 queued test-amd64-i386-xl-qemuu-debianhvm-amd64 queued test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsmqueued test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 queued test-amd64-amd64-xl-qemuu-winxpsp3 queued build-armhf 2 hosts-allocate running build-armhf-xsm 2 hosts-allocate running build-armhf-pvops 2 hosts-allocate running build-amd64 2 hosts-allocate running build-amd64-pvops 2 hosts-allocate running build-i3862 hosts-allocate running build-amd64-xsm 2 hosts-allocate running build-i386-pvops 2 hosts-allocate running build-i386-xsm2 hosts-allocate running version targeted for testing: qemuu91d35509903464c7f4b9ed56be223d7370d3597c baseline version: qemuud6550e9ed2e1a60d889dfb721de00d9a4e3bafbe Last test of basis66485 2016-07-01 10:47:40 Z7 days Testing same since0 1970-01-01 00:00:00 Z 16990 days0 attempts People who touched revisions under test: Aaron Larson Alberto Garcia Aleksandar Markovic Alex Bennée Alex Bligh Alex Williamson Alexander Graf Alexander Shopov Alexey Kardashevskiy Alistair Francis Amit Shah Andrea Arcangeli Andreas Färber Andrew Jeffery Andrew Jones Andrey Smirnov Aneesh Kumar K.V Anthony PERARD Anton Blanchard Ard Biesheuvel Artyom Tarasenko Ashijeet Acharya Aurelien Jarno Bastian Koppelmann Benjamin Herrenschmidt Bharata B Rao Bret
Re: [Xen-devel] Kernel panic on Xen virtualisation in Debian
On Wed, Jul 06, 2016 at 03:14:15PM +0100, George Dunlap wrote: > On Mon, Jul 4, 2016 at 7:06 PM, Jan Prunk wrote: > > Hello ! > > > > I am posting Xen virtualisation bug links to this e-mail address, > > because I wasn't able to find the Xen specific bugtracker list. > > This bug has been discovered in 2015 and so far it hasn't been > > resolved through the Debian/Kernel bug lists. I submit the > > links to bug reports for you. > > > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804079 > > The serial log at the bottom looks like there was a crash in the ipv6 > handling as the result of a packet delivery, perhaps? David / Wei, do > you have any ideas? Not sure who else has worked on the netback side > of things. > The original bug report showed that there was a exception in the middle of memcpy instruction while the latest log showed that the exception could potentially be somewhere else. Both logs showed that the exception took place when ipv6 was involved. If Jan can come up with a reliable repro I might be able to have a look. Wei. > -George ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [xen-unstable baseline-only test] 66531: trouble: broken/preparing/queued
Platform Team regression test user writes ("[xen-unstable baseline-only test]
66531: trouble: broken/preparing/queued"):
> This run is configured for baseline tests only.
>
> flight 66531 xen-unstable running [real]
> http://osstest.xs.citrite.net/~osstest/testlogs/logs/66531/
>
> Failures and problems with tests :-(
>
> Tests which did not succeed and are blocking,
> including tests which could not be run:
> test-armhf-armhf-libvirt-xsm queued
> build-armhf-libvirt queued
> test-armhf-armhf-xl-xsm queued
Sorry about all this noise. The Cambridge osstest instance is not in
very good shape and I am trying to sort it out.
Thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [DRAFT 1] XenSock protocol design document
Hi all,
as promised, this is the design document for the XenSock protocol I
mentioned here:
http://marc.info/?l=xen-devel&m=146520572428581
It is still in its early days but should give you a good idea of how it
looks like and how it is supposed to work. Let me know if you find gaps
in the document and I'll fill them in the next version.
You can find prototypes of the Linux frontend and backend drivers here:
git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git xensock-1
To use them, make sure to enable CONFIG_XENSOCK in your kernel config
and add "xensock=1" to the command line of your DomU Linux kernel. You
also need the toolstack to create the initial xenstore nodes for the
protocol. To do that, please apply the attached patch to libxl (the
patch is based on Xen 4.7.0-rc3) and add "xensock=1" to your DomU config
file.
Feel free to try them out! Please be kind, they are only prototypes with
a few known issues :-) But they should work well enough to run simple
tests. If you find something missing, let me know or, even better, write
a patch!
I'll follow up with a separate document to cover the design of my
particular implementation of the protocol.
Cheers,
Stefano
---
# XenSocks Protocol v1
## Rationale
XenSocks is a paravirtualized protocol for the POSIX socket API.
The purpose of XenSocks is to allow the implementation of a specific set
of POSIX calls to be done in a domain other than your own. It allows
connect, accept, bind, release, listen, poll, recvmsg and sendmsg to be
implemented in another domain.
XenSocks provides the following benefits:
* guest networking works out of the box with VPNs, wireless networks and
any other complex configurations on the host
* guest services listen on ports bound directly to the backend domain IP
addresses
* localhost becomes a secure namespace for intra-VMs communications
* full visibility of the guest behavior on the backend domain, allowing
for inexpensive filtering and manipulation of any guest calls
* excellent performance
## Design
### Xenstore
The frontend and the backend connect to each other exchanging information via
xenstore. The toolstack creates front and back nodes with state
XenbusStateInitialising. There can only be one XenSock frontend per domain.
Frontend XenBus Nodes
port
Values:
The identifier of the Xen event channel used to signal activity
in the ring buffer.
ring-ref
Values:
The Xen grant reference granting permission for the backend to map
the sole page in a single page sized ring buffer.
State Machine
**Front** **Back**
XenbusStateInitialising XenbusStateInitialising
- Query virtual device- Query backend device
properties. identification data.
- Setup OS device instance. |
- Allocate and initialize the|
request ring. V
- Publish transport parametersXenbusStateInitWait
that will be in effect during
this connection.
|
|
V
XenbusStateInitialised
- Query frontend transport parameters.
- Connect to the request ring and
event channel.
|
|
V
XenbusStateConnected
- Query backend device properties.
- Finalize OS virtual device
instance.
|
|
V
XenbusStateConnected
Once frontend and backend are connected, they have a shared page, which
will is used to exchange messages over a ring, and an event channel,
which is used to send notifications.
### Commands Ring
The shared ring is used by the frontend to forward socket API calls to the
backend. I'll refer to this ring as **commands ring** to distinguish it from
other rings which will be created later in the lifecycle of the protocol (data
rings). The ring format is defined using the familiar `DEFINE_RING_TYPES` macro
(`xen/include/public/io/ring.h`). Frontend requests are allocated on the ring
using the `RING_GET_REQUEST` macro.
The format is defined as follows:
#define XENSOCK_DATARING_ORDER 6
#define XENSOCK_DATARING_PAGES (1 << XENSOCK_DATARING_ORDER)
#define XENSOCK_DATARING_SIZE (XENSOCK_DATARING_PAGES << PAGE_SHIFT)
#define XENSOCK_CONNECT0
#define XENSOCK_RELEASE3
#define XENSOCK_BIND 4
#define XENSOCK_LISTEN 5
#define XENSOCK_ACCEPT 6
#define XENSOCK_POLL 7
struct xen_xensock_request {
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote:
> On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote:
> > > This patch use xc_dom_alloc_segment() to allocate the memory space for the
> > > ACPI modules and the SMBIOS modules. This is to replace the arbitrary
> > > placement of 1MB (+ extra for MB alignement) after the hvmloader image.
> > >
> > > This patch can help if one add extra ACPI table and hvmloader contain
> > > OVMF (OVMF is a 2MB binary), as in that case the extra ACPI table could
> > > easily be loaded past the address 4MB, but hvmloader use a range of
> > > memory from 4MB to 10MB to perform tests and in the process, clears the
> > > memory, before loading the modules.
> > >
> > > Signed-off-by: Anthony PERARD
> > > ---
> > > Changes in V5:
> > > - rewrite patch description
> > >
> > > Changes in V4:
> > > - check that guest_addr_out have a reasonable value.
> > >
> > > New patch in V3.
> > > ---
> > > tools/libxc/xc_dom_hvmloader.c | 131
> > > -
> > > 1 file changed, 38 insertions(+), 93 deletions(-)
> > >
> > > diff --git a/tools/libxc/xc_dom_hvmloader.c
> > > b/tools/libxc/xc_dom_hvmloader.c
> > > index 330d5e8..da8b995 100644
> > > --- a/tools/libxc/xc_dom_hvmloader.c
> > > +++ b/tools/libxc/xc_dom_hvmloader.c
> > > @@ -129,98 +129,52 @@ static elf_errorstatus
> > > xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> > > return rc;
> > > }
> > >
> > > -static int modules_init(struct xc_dom_image *dom,
> > > -uint64_t vend, struct elf_binary *elf,
> > > -uint64_t *mstart_out, uint64_t *mend_out)
> > > +static int module_init_one(struct xc_dom_image *dom,
> > > + struct xc_hvm_firmware_module *module,
> > > + char *name)
> > > {
> > > -#define MODULE_ALIGN 1UL << 7
> > > -#define MB_ALIGN 1UL << 20
> > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) - 1))
> > > -uint64_t total_len = 0, offset1 = 0;
> > > +struct xc_dom_seg seg;
> > > +void *dest;
> > >
> > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length == 0 )
> > > -return 0;
> > > -
> > > -/* Find the total length for the firmware modules with a reasonable
> > > large
> > > - * alignment size to align each the modules.
> > > - */
> > > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> > > -offset1 = total_len;
> > > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> > > -
> > > -/* Want to place the modules 1Mb+change behind the loader image. */
> > > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> > > -*mend_out = *mstart_out + total_len;
> > > -
> > > -if ( *mend_out > vend )
> > > -return -1;
> > > -
> > > -if ( dom->acpi_module.length != 0 )
> > > -dom->acpi_module.guest_addr_out = *mstart_out;
> > > -if ( dom->smbios_module.length != 0 )
> > > -dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> > > +if ( module->length )
> > > +{
> > > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> > > +goto err;
> > > +dest = xc_dom_seg_to_ptr(dom, &seg);
> > > +if ( dest == NULL )
> > > +{
> > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> > > + __FUNCTION__);
> > > +goto err;
> > > +}
> > > +memcpy(dest, module->data, module->length);
> > > +module->guest_addr_out = seg.vstart;
> > > +if ( module->guest_addr_out > UINT32_MAX ||
> > > + module->guest_addr_out + module->length > UINT32_MAX )
> > > +{
> > > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> > > + __FUNCTION__, name);
> > > +goto err;
> > > +}
> >
> > One question:
> >
> > Can this check also account for MMIO hole below 4G? Maybe use
> > dom->mmio_size?
>
> Yes, I guess I can check against dom->mmio_start. Should I also check
> that mmio_start have reasonable value? (<4G, and not 0x0) Or is
> mmio_start is already supposed to have a good value?
>
mmio_start should already have a sane value here -- or at least I hope
so. The sanity of mmio_start should be checked where it is assigned.
Wei.
> > Other than this, this patch looks good.
>
> Thanks,
>
> --
> Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 1:37 PM, Jan Beulich wrote:
On 08.07.16 at 12:22, wrote:
On 7/8/2016 10:21 AM, Jan Beulich wrote:
On 06.07.16 at 17:50, wrote:
The title of this patch keeps confusing me - which code motion is
being relocated here?
As the commit message clearly states, the code motions that are being
relocated are:
Again this sentence makes no sense to me: I can't see how
"code motions" can be "relocated", just like I don't see how you
could move a move. But maybe it's just me...
Hah, sorry, I'm not very good expressivity-wise, a weakness I'm aware of
and which makes me pick up expressions I notice other people use (in
this case those of maintainers).
I think you were the one I noticed using the expression back in an older
patch-series I've sent and I thought by "code-motion" you meant simply
"that which some code does, tries to accomplish and the code itself".
1) handling of monitor_write_data @ hvm_do_resume
2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting
CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
/* Trap CR3 updates if CR3 memory events are enabled. */
and what's removed from under it.
By 'relocation' I meant making that code vm-event specific (moving it to
vm-event specific files).
Yes, that what I've guessed.
+{
+struct vcpu *v;
+struct arch_vmx_struct *avmx;
+unsigned int cr3_bitmask;
+bool_t cr3_vmevent, cr3_ldexit;
+
+/* domain must be paused */
+ASSERT(atomic_read(&d->pause_count));
Comment style.
As in change to "/* Domain must be paused. */"?
Yes, as mandated by ./CODING_STYLE.
+/* non-hap domains trap CR3 writes unconditionally */
+if ( !paging_mode_hap(d) )
+{
+#ifndef NDEBUG
+for_each_vcpu ( d, v )
+ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
+#endif
+return;
+}
+
+cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
+cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
+
+for_each_vcpu ( d, v )
+{
+avmx = &v->arch.hvm_vmx;
+cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
+
+if ( cr3_vmevent == cr3_ldexit )
+continue;
+
+/*
+ * If CR0.PE=0, CR3 load exiting must remain enabled.
+ * See vmx_update_guest_cr code motion for cr = 0.
+ */
Same as for the title - what code motion is this referring to? In a
code comment you clearly shouldn't be referring to anything the
patch effects, only to its result.
The "vmx_update_guest_cr code motion for cr = 0", that's what's
referring to.
So I guess my problem really is that I don't understand what a
"code motion" is (other than the act of moving code from one
place to another).
Again, sorry, will try to rephrase all of this properly :-).
'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
In other words, see what's happening in the function
'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand
why CR3 load-exiting must remain enabled when CR0.PE=0.
+if ( cr3_ldexit && !hvm_paging_enabled(v) &&
!vmx_unrestricted_guest(v) )
+continue;
The first sentence of the comment should be brought in line with
this condition.
Would this do (aligned with the above observation):
"
/*
* If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
* enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
*/
"
?
Not really: The condition checks whether paging is enabled and
whether it's an unrestricted guest. The comment talks about
protected mode being enabled.
Hah you're right, I only now notice, that comment has actually been
adopted (although I don't remember from where, I wonder if it was
meantime removed and I only now see), I always thought it said "CR0.PG =
0"...
So...
"
/*
* If domain paging is disabled (CR0.PG=0) and
* the domain is not in real-mode, then CR3 load-exiting
* must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
*/
"
?
+static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
index)
Unless there is a particular reason for this uint8_t, please convert to
unsigned int.
The particular reason is cr-indexes being uint8_t typed (see
typeof(xen_domctl_monitor_op.mov_to_cr.index)).
But I will change it to unsigned int if you prefer (maybe you could
explain the preference though).
No use of fixed width types when fixed width types aren't really
required. Generally generated code is less efficient when having
to deal with fixed width types.
Strange, I would have thought the compiler would properly (and easily)
deal with such efficiency issues.
+{
+/* vmx only */
+ASSERT(cpu_has_vmx);
Comment style (more below). Should perhaps also get "for now" or
some such added.
As in "/* For now, VMX only. */"?
For example, yes.
+static inline void write_ctrlreg
Re: [Xen-devel] [PATCH v2] xen: arm64: Add support for Renesas RCar Gen3 H3 Salvator-X platform
Hi Andre, On 05.07.2016 16:29, Andre Przywara wrote: Hi, On 05/07/16 15:22, Dirk Behme wrote: On 05.07.2016 15:45, Andre Przywara wrote: Hi, On 05/07/16 14:34, Julien Grall wrote: (CC Andre) On 05/07/16 14:04, Dirk Behme wrote: On 05.07.2016 14:45, Julien Grall wrote: On 05/07/16 13:09, Dirk Behme wrote: Hi Julien, On 05.07.2016 13:39, Julien Grall wrote: Hi Dirk, On 05/07/16 07:37, Dirk Behme wrote: Signed-off-by: Dirk Behme This patch looks good to me, however I would like to see the documentation on the wiki page (see [1]) before giving any formal ack. Ok, many thanks for your review! Yes, I understood that something more is needed. I just wonder if we keep the patch on the mailing list for a moment. With this it's publically available and we can see how's the public interest for this board. Additionally, getting Xen running on this board and describe all this in the wiki isn't that easy. You either need to modify the flashed firmware. Or, like me, load everything via a JTAG debugger ... Can you details why you think it is not easy? Why do you have to modify the firmware? Is it because it does not boot the hypervisor in EL2? The board boots via ATF, which starts U-Boot in EL1, then. You have to find a way to load Xen into memory (e.g. U-Boot TFTP) and then switch to EL2 to start Xen. But ATF is running in EL3, right? If so, can ATF just starts U-boot in EL2? I have a board at home (pine64) which also uses ATF and U-Boot and is able to boot Xen without any SMC call because the U-Boot is entered in EL2. From having a very quick look into the rcar ATF port on github[1] I see: +#elif (RCAR_BL33_EXECUTION_EL == BL33_EL2) +return (uint32_t)SPSR_64(MODE_EL2, MODE_SP_ELX, DISABLE_ALL_EXCEPTIONS); So if you rebuild ATF with RCAR_BL33_EXECUTION_EL set to BL33_EL2 that should enter U-Boot in EL2. The fix for the Pine64 was equally simple and works fine since then. This is an other solution most probably I meant when talking about "modifying the firmware" ;) I'm somehow unsure about the pros and cons running U-Boot at EL1 versus EL2, though. It shouldn't matter, really. U-Boot on AArch64 (called armv8 here) is able to run in any exception level (in contrast to ARMv7). Actually running in EL2 is the architecturally recommended way, even with ATF (by default it drops into EL2 when returning to non-secure world). On Juno (and the Pine64) is works fine this way. As U-Boot only uses an identity mapping, many differences between EL2 and EL1 don't matter. An understanding question regarding this: We found that running U-Boot at EL2 works fine and starting Xen works fine, then, too. So many thanks for that hint! But we found that the Linux native boot case (without Xen) does fail, then. I.e. running U-Boot in EL1 starts the native Linux kernel successfully. While running U-Boot in EL2 starting the native Linux kernel fails, then. Any idea regarding this? Who is responsible to switch to EL1 for the native Linux boot case if U-Boot runs at EL2? Best regards Dirk ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).
Signed-off-by: Corneliu ZUZU
---
Changed since v2:
---
xen/arch/x86/hvm/vmx/vmx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index df19579..8ab074f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
+uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
-vmx_update_cpu_exec_control(v);
+if ( old_ctls != v->arch.hvm_vmx.exec_control )
+vmx_update_cpu_exec_control(v);
}
if ( !nestedhvm_vcpu_in_guestmode(v) )
Jan,
I'm wondering if you could ack this as well (if there's nothing wrong w/
it) and push it to be done with it. :-)
Thanks,
Corneliu.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
>>> On 08.07.16 at 13:39, wrote:
> On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
>> Minor optimization @ vmx_update_guest_cr: checks if
> v->arch.hvm_vmx.exec_control
>> was modified before actually calling vmx_update_cpu_exec_control(v).
>>
>> Signed-off-by: Corneliu ZUZU
>> ---
>> Changed since v2:
>> ---
>> xen/arch/x86/hvm/vmx/vmx.c | 5 -
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>> index df19579..8ab074f 100644
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v,
> unsigned int cr)
>> if ( paging_mode_hap(v->domain) )
>> {
>> /* Manage GUEST_CR3 when CR0.PE=0. */
>> +uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
>> uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
>>CPU_BASED_CR3_STORE_EXITING);
>> +
>> v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
>> if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>> v->arch.hvm_vmx.exec_control |= cr3_ctls;
>> @@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v,
> unsigned int cr)
>>monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
>> v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
>>
>> -vmx_update_cpu_exec_control(v);
>> +if ( old_ctls != v->arch.hvm_vmx.exec_control )
>> +vmx_update_cpu_exec_control(v);
>> }
>>
>> if ( !nestedhvm_vcpu_in_guestmode(v) )
>
> I'm wondering if you could ack this as well (if there's nothing wrong w/
> it) and push it to be done with it. :-)
Well, if you had pinged the patch at least once, I probably would.
I don't, however, recall having seen any such ping (only resends).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
>>> On 08.07.16 at 13:33, wrote: > On 7/8/2016 1:37 PM, Jan Beulich wrote: > On 08.07.16 at 12:22, wrote: >>> On 7/8/2016 10:21 AM, Jan Beulich wrote: >>> On 06.07.16 at 17:50, wrote: > +/* > + * If CR0.PE=0, CR3 load exiting must remain enabled. > + * See vmx_update_guest_cr code motion for cr = 0. > + */ > +if ( cr3_ldexit && !hvm_paging_enabled(v) && > !vmx_unrestricted_guest(v) ) > +continue; The first sentence of the comment should be brought in line with this condition. >>> Would this do (aligned with the above observation): >>> >>> " >>> >>> /* >>>* If CR3 load-exiting was enabled and CR0.PE=0, then it must >>> remain >>>* enabled (see vmx_update_guest_cr(v, cr) function when cr = 0). >>>*/ >>> >>> " >>> ? >> Not really: The condition checks whether paging is enabled and >> whether it's an unrestricted guest. The comment talks about >> protected mode being enabled. > > Hah you're right, I only now notice, that comment has actually been > adopted (although I don't remember from where, I wonder if it was > meantime removed and I only now see), I always thought it said "CR0.PG = > 0"... > So... > " > > /* > * If domain paging is disabled (CR0.PG=0) and > * the domain is not in real-mode, then CR3 load-exiting > * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0). > */ > " > ? Looks reasonable. > +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t >>> index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. >>> The particular reason is cr-indexes being uint8_t typed (see >>> typeof(xen_domctl_monitor_op.mov_to_cr.index)). >>> But I will change it to unsigned int if you prefer (maybe you could >>> explain the preference though). >> No use of fixed width types when fixed width types aren't really >> required. Generally generated code is less efficient when having >> to deal with fixed width types. > > Strange, I would have thought the compiler would properly (and easily) > deal with such efficiency issues. In this case the compiler may well do (as the function is static inline), but in other cases it's simply not allowed to. In order to not misguide people cloning existing code we should thus try to limit the number of bad examples (which in particular mean not introducing any new ones). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/8/2016 2:48 PM, Jan Beulich wrote:
On 08.07.16 at 13:39, wrote:
On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
Minor optimization @ vmx_update_guest_cr: checks if
v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).
Signed-off-by: Corneliu ZUZU
---
Changed since v2:
---
xen/arch/x86/hvm/vmx/vmx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index df19579..8ab074f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v,
unsigned int cr)
if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
+uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v,
unsigned int cr)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
-vmx_update_cpu_exec_control(v);
+if ( old_ctls != v->arch.hvm_vmx.exec_control )
+vmx_update_cpu_exec_control(v);
}
if ( !nestedhvm_vcpu_in_guestmode(v) )
I'm wondering if you could ack this as well (if there's nothing wrong w/
it) and push it to be done with it. :-)
Well, if you had pinged the patch at least once, I probably would.
I don't, however, recall having seen any such ping (only resends).
Jan
So is a 'ping' (haven't done that before) still necessary? Is a ping a
simple 'reply-to-all' including 'Ping: ' in the subject? :-)
Thanks,
Corneliu.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately
On 7/8/2016 2:53 PM, Jan Beulich wrote: On 08.07.16 at 13:33, wrote: On 7/8/2016 1:37 PM, Jan Beulich wrote: On 08.07.16 at 12:22, wrote: On 7/8/2016 10:21 AM, Jan Beulich wrote: On 06.07.16 at 17:50, wrote: +/* + * If CR0.PE=0, CR3 load exiting must remain enabled. + * See vmx_update_guest_cr code motion for cr = 0. + */ +if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) ) +continue; The first sentence of the comment should be brought in line with this condition. Would this do (aligned with the above observation): " /* * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0). */ " ? Not really: The condition checks whether paging is enabled and whether it's an unrestricted guest. The comment talks about protected mode being enabled. Hah you're right, I only now notice, that comment has actually been adopted (although I don't remember from where, I wonder if it was meantime removed and I only now see), I always thought it said "CR0.PG = 0"... So... " /* * If domain paging is disabled (CR0.PG=0) and * the domain is not in real-mode, then CR3 load-exiting * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0). */ " ? Looks reasonable. +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index) Unless there is a particular reason for this uint8_t, please convert to unsigned int. The particular reason is cr-indexes being uint8_t typed (see typeof(xen_domctl_monitor_op.mov_to_cr.index)). But I will change it to unsigned int if you prefer (maybe you could explain the preference though). No use of fixed width types when fixed width types aren't really required. Generally generated code is less efficient when having to deal with fixed width types. Strange, I would have thought the compiler would properly (and easily) deal with such efficiency issues. In this case the compiler may well do (as the function is static inline), but in other cases it's simply not allowed to. In order to not misguide people cloning existing code we should thus try to limit the number of bad examples (which in particular mean not introducing any new ones). Jan Ack. Corneliu. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
>>> On 08.07.16 at 13:55, wrote:
> On 7/8/2016 2:48 PM, Jan Beulich wrote:
> On 08.07.16 at 13:39, wrote:
>>> On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
Minor optimization @ vmx_update_guest_cr: checks if
>>> v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).
Signed-off-by: Corneliu ZUZU
---
Changed since v2:
---
xen/arch/x86/hvm/vmx/vmx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index df19579..8ab074f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v,
>>> unsigned int cr)
if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
+uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v,
>>> unsigned int cr)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |=
CPU_BASED_CR3_LOAD_EXITING;
-vmx_update_cpu_exec_control(v);
+if ( old_ctls != v->arch.hvm_vmx.exec_control )
+vmx_update_cpu_exec_control(v);
}
if ( !nestedhvm_vcpu_in_guestmode(v) )
>>> I'm wondering if you could ack this as well (if there's nothing wrong w/
>>> it) and push it to be done with it. :-)
>> Well, if you had pinged the patch at least once, I probably would.
>> I don't, however, recall having seen any such ping (only resends).
>
> So is a 'ping' (haven't done that before) still necessary? Is a ping a
> simple 'reply-to-all' including 'Ping: ' in the subject? :-)
It's not very much formalized. What I do in such a case is, as you
say, prefix the subject with Ping:, but change addressing so that
the people expected to reply end up in To:, while everyone else in
the original To: list (i.e. including xen-devel) would get moved to
Cc:.
And while, with the adjustment to addressees in my earlier reply, I
already tried to do kind of a ping to them, I think it wouldn't hurt if
you did another explicit one.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [DRAFT 1] XenSock protocol design document
On 08/07/16 13:23, Stefano Stabellini wrote: > Hi all, > > as promised, this is the design document for the XenSock protocol I > mentioned here: > > http://marc.info/?l=xen-devel&m=146520572428581 > > It is still in its early days but should give you a good idea of how it > looks like and how it is supposed to work. Let me know if you find gaps > in the document and I'll fill them in the next version. > > You can find prototypes of the Linux frontend and backend drivers here: > > git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git xensock-1 > > To use them, make sure to enable CONFIG_XENSOCK in your kernel config > and add "xensock=1" to the command line of your DomU Linux kernel. You > also need the toolstack to create the initial xenstore nodes for the > protocol. To do that, please apply the attached patch to libxl (the > patch is based on Xen 4.7.0-rc3) and add "xensock=1" to your DomU config > file. > > Feel free to try them out! Please be kind, they are only prototypes with > a few known issues :-) But they should work well enough to run simple > tests. If you find something missing, let me know or, even better, write > a patch! > > I'll follow up with a separate document to cover the design of my > particular implementation of the protocol. > > Cheers, > > Stefano > > --- > > # XenSocks Protocol v1 > > ## Rationale > > XenSocks is a paravirtualized protocol for the POSIX socket API. > > The purpose of XenSocks is to allow the implementation of a specific set > of POSIX calls to be done in a domain other than your own. It allows > connect, accept, bind, release, listen, poll, recvmsg and sendmsg to be > implemented in another domain. > > XenSocks provides the following benefits: > * guest networking works out of the box with VPNs, wireless networks and > any other complex configurations on the host > * guest services listen on ports bound directly to the backend domain IP > addresses > * localhost becomes a secure namespace for intra-VMs communications > * full visibility of the guest behavior on the backend domain, allowing > for inexpensive filtering and manipulation of any guest calls > * excellent performance > > > ## Design > > ### Xenstore > > The frontend and the backend connect to each other exchanging information via > xenstore. The toolstack creates front and back nodes with state > XenbusStateInitialising. There can only be one XenSock frontend per domain. > > Frontend XenBus Nodes > > port > Values: > > The identifier of the Xen event channel used to signal activity > in the ring buffer. > > ring-ref > Values: > > The Xen grant reference granting permission for the backend to map > the sole page in a single page sized ring buffer. > > > State Machine > > **Front** **Back** > XenbusStateInitialising XenbusStateInitialising > - Query virtual device- Query backend device > properties. identification data. > - Setup OS device instance. | > - Allocate and initialize the| > request ring. V > - Publish transport parametersXenbusStateInitWait > that will be in effect during > this connection. > | > | > V >XenbusStateInitialised > > - Query frontend transport > parameters. > - Connect to the request ring and > event channel. > | > | > V > XenbusStateConnected > > - Query backend device properties. > - Finalize OS virtual device >instance. > | > | > V > XenbusStateConnected > > Once frontend and backend are connected, they have a shared page, which > will is used to exchange messages over a ring, and an event channel, > which is used to send notifications. > > > ### Commands Ring > > The shared ring is used by the frontend to forward socket API calls to the > backend. I'll refer to this ring as **commands ring** to distinguish it from > other rings which will be created later in the lifecycle of the protocol (data > rings). The ring format is defined using the familiar `DEFINE_RING_TYPES` > macro > (`xen/include/public/io/ring.h`). Frontend requests are allocated on the ring > using the `RING_GET_REQUEST` macro. > > The format is defined as follows: > > #define XENSOCK_DATARING_ORDER 6 > #define XENSOCK_DATARING_PAGES (1 << XENSOCK_DATARING_ORDER) > #define XENSOCK_DAT
[Xen-devel] [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and
CPUID[0x8007].EDX[7] anymore, causing the driver to fail to load on
both Intel and AMD systems. Doing any kind of hardware capability
checks in the driver as a prerequisite was wrong anyway: With the
hypervisor being in charge, all such checking should be done by it. If
ACPI data gets uploaded despite some missing capability, the hypervisor
is free to ignore part or all of that data.
Ditch the entire check_prereq() function, and do the only valid check
(xen_initial_domain()) in the caller in its place.
Signed-off-by: Jan Beulich
---
drivers/xen/xen-acpi-processor.c | 35 +++
1 file changed, 3 insertions(+), 32 deletions(-)
--- 4.7-rc6/drivers/xen/xen-acpi-processor.c
+++ 4.7-rc6-xen-acpi-processor-prereqs/drivers/xen/xen-acpi-processor.c
@@ -423,36 +423,7 @@ upload:
return 0;
}
-static int __init check_prereq(void)
-{
- struct cpuinfo_x86 *c = &cpu_data(0);
-
- if (!xen_initial_domain())
- return -ENODEV;
-
- if (!acpi_gbl_FADT.smi_command)
- return -ENODEV;
- if (c->x86_vendor == X86_VENDOR_INTEL) {
- if (!cpu_has(c, X86_FEATURE_EST))
- return -ENODEV;
-
- return 0;
- }
- if (c->x86_vendor == X86_VENDOR_AMD) {
- /* Copied from powernow-k8.h, can't include ../cpufreq/powernow
-* as we get compile warnings for the static functions.
-*/
-#define CPUID_FREQ_VOLT_CAPABILITIES0x8007
-#define USE_HW_PSTATE 0x0080
- u32 eax, ebx, ecx, edx;
- cpuid(CPUID_FREQ_VOLT_CAPABILITIES, &eax, &ebx, &ecx, &edx);
- if ((edx & USE_HW_PSTATE) != USE_HW_PSTATE)
- return -ENODEV;
- return 0;
- }
- return -ENODEV;
-}
/* acpi_perf_data is a pointer to percpu data. */
static struct acpi_processor_performance __percpu *acpi_perf_data;
@@ -509,10 +480,10 @@ struct notifier_block xen_acpi_processor
static int __init xen_acpi_processor_init(void)
{
unsigned int i;
- int rc = check_prereq();
+ int rc;
- if (rc)
- return rc;
+ if (!xen_initial_domain())
+ return -ENODEV;
nr_acpi_bits = get_max_acpi_id() + 1;
acpi_ids_done = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned
long), GFP_KERNEL);
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions
On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
> On 29/06/16 14:52, Andrew Cooper wrote:
> > On 29/06/16 13:44, Juergen Gross wrote:
> >> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
> >>/* Tell the kernel we're up and running. */
> >>xenbus_notify_running();
> >>
> >> -#if defined(XEN_SYSTEMD_ENABLED)
> >> - if (systemd) {
> >> - sd_notify(1, "READY=1");
> >> - fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> >> - }
> >> -#endif
> >
> > Getting rid of the socket configuration for systemd is ok, but we should
> > keep the sd_notify() calls for when the daemon is started by systemd.
> >
> > Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> > still required if we don't want systemd to treat xenstored as a legacy
> > unix daemon.
>
> So what is the downside of xenstored being treated as a legacy daemon?
> This question is especially interesting for the case of patch 2 being
> considered: xenstored is no longer started by systemd, but by a wrapper
> script which might decide to start the xenstore domain instead.
>
> Another problem: today xenstored decides whether to call sd_notify()
> by testing the xenstore sockets being specified via systemd. This will
> no longer work. So how to do it now?
>
Not sure I follow.
See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
to be able to tell if cxenstored is started by systemd or not.
sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
miss anything?
Wei.
>
> Juergen
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] Ping: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization
On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).
Signed-off-by: Corneliu ZUZU
---
Changed since v2:
---
xen/arch/x86/hvm/vmx/vmx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index df19579..8ab074f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
if ( paging_mode_hap(v->domain) )
{
/* Manage GUEST_CR3 when CR0.PE=0. */
+uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
CPU_BASED_CR3_STORE_EXITING);
+
v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned
int cr)
monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
-vmx_update_cpu_exec_control(v);
+if ( old_ctls != v->arch.hvm_vmx.exec_control )
+vmx_update_cpu_exec_control(v);
}
if ( !nestedhvm_vcpu_in_guestmode(v) )
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [qemu-mainline test] 96776: regressions - FAIL
flight 96776 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/96776/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-debianhvm-amd64 15 guest-localmigrate/x10 fail REGR. vs. 96732 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 15 guest-start/debian.repeat fail REGR. vs. 96732 test-amd64-amd64-xl-rtds 9 debian-install fail blocked in 96732 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-stop fail like 96732 test-amd64-i386-xl-qemuu-win7-amd64 16 guest-stop fail like 96732 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass test-armhf-armhf-libvirt 14 guest-saverestorefail never pass test-armhf-armhf-libvirt 12 migrate-support-checkfail never pass test-amd64-i386-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 16 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check fail never pass test-armhf-armhf-xl-credit2 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 12 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail never pass test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass test-armhf-armhf-xl 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 13 guest-saverestorefail never pass test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-xsm 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-xsm 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 12 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 12 migrate-support-checkfail never pass version targeted for testing: qemuu4f4a9ca4a4386c137301b3662faba076455ff15a baseline version: qemuu91d35509903464c7f4b9ed56be223d7370d3597c Last test of basis96732 2016-07-06 18:12:54 Z1 days Failing since 96765 2016-07-07 10:44:12 Z1 days2 attempts Testing same since96776 2016-07-08 00:42:28 Z0 days1 attempts People who touched revisions under test: Jason Wang Jean-Christophe Dubois KONRAD Frederic Paolo Bonzini Peter Maydell Shannon Zhao jobs: build-amd64-xsm pass build-armhf-xsm pass build-i386-xsm pass build-amd64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-xl pass
[Xen-devel] [PATCH v2 0/4] xen: prefer xenbus_scanf() over xenbus_gather()
For single items being collected this should be preferred as being more typesafe (as the compiler can check format string and to-be-written-to variable match) and more efficient (requiring one less parameter to be passed). 1: xenbus: prefer xenbus_scanf() over xenbus_gather() 2: xen-blkback: prefer xenbus_scanf() over xenbus_gather() 3: xen-blkfront: prefer xenbus_scanf() over xenbus_gather() 4: xen-netback: prefer xenbus_scanf() over xenbus_gather() Signed-off-by: Jan Beulich --- v2: Avoid commit messages to continue from subjects. Group into a series. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] Kernel panic on Xen virtualisation in Debian
Hello, Please also send a CC: to 804...@bugs.debian.org for future reference. The administrator of the server is Andreas Ziegler , maybe he will be able to log/reproduce the bug, I was only an initial reporter. Kind regards, Jan On Fri, Jul 8, 2016 at 1:14 PM, Wei Liu wrote: > On Wed, Jul 06, 2016 at 03:14:15PM +0100, George Dunlap wrote: > > On Mon, Jul 4, 2016 at 7:06 PM, Jan Prunk wrote: > > > Hello ! > > > > > > I am posting Xen virtualisation bug links to this e-mail address, > > > because I wasn't able to find the Xen specific bugtracker list. > > > This bug has been discovered in 2015 and so far it hasn't been > > > resolved through the Debian/Kernel bug lists. I submit the > > > links to bug reports for you. > > > > > > https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=804079 > > > > The serial log at the bottom looks like there was a crash in the ipv6 > > handling as the result of a packet delivery, perhaps? David / Wei, do > > you have any ideas? Not sure who else has worked on the netback side > > of things. > > > > The original bug report showed that there was a exception in the middle > of memcpy instruction while the latest log showed that the exception > could potentially be somewhere else. Both logs showed that the > exception took place when ipv6 was involved. > > If Jan can come up with a reliable repro I might be able to have a look. > > Wei. > > > -George > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 1/4] xenbus: prefer xenbus_scanf() over xenbus_gather()
For single items being collected this should be preferred as being more
typesafe (as the compiler can check format string and to-be-written-to
variable match) and more efficient (requiring one less parameter to be
passed).
Signed-off-by: Jan Beulich
---
v2: Avoid commit message to continue from subject.
---
drivers/xen/xenbus/xenbus_client.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- 4.7-rc6-prefer-xenbus_scanf.orig/drivers/xen/xenbus/xenbus_client.c
+++ 4.7-rc6-prefer-xenbus_scanf/drivers/xen/xenbus/xenbus_client.c
@@ -926,9 +926,9 @@ EXPORT_SYMBOL_GPL(xenbus_unmap_ring);
*/
enum xenbus_state xenbus_read_driver_state(const char *path)
{
- enum xenbus_state result;
- int err = xenbus_gather(XBT_NIL, path, "state", "%d", &result, NULL);
- if (err)
+ int result;
+
+ if (xenbus_scanf(XBT_NIL, path, "state", "%d", &result) != 1)
result = XenbusStateUnknown;
return result;
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 2/4] xen-blkback: prefer xenbus_scanf() over xenbus_gather()
For single items being collected this should be preferred as being more
typesafe (as the compiler can check format string and to-be-written-to
variable match) and more efficient (requiring one less parameter to be
passed).
Signed-off-by: Jan Beulich
Acked-by: Roger Pau Monné
---
v2: Avoid commit message to continue from subject.
---
drivers/block/xen-blkback/xenbus.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
--- 4.7-rc6-prefer-xenbus_scanf.orig/drivers/block/xen-blkback/xenbus.c
+++ 4.7-rc6-prefer-xenbus_scanf/drivers/block/xen-blkback/xenbus.c
@@ -1022,9 +1022,9 @@ static int connect_ring(struct backend_i
pr_debug("%s %s\n", __func__, dev->otherend);
be->blkif->blk_protocol = BLKIF_PROTOCOL_DEFAULT;
- err = xenbus_gather(XBT_NIL, dev->otherend, "protocol",
- "%63s", protocol, NULL);
- if (err)
+ err = xenbus_scanf(XBT_NIL, dev->otherend, "protocol",
+ "%63s", protocol);
+ if (err <= 0)
strcpy(protocol, "unspecified, assuming default");
else if (0 == strcmp(protocol, XEN_IO_PROTO_ABI_NATIVE))
be->blkif->blk_protocol = BLKIF_PROTOCOL_NATIVE;
@@ -1036,10 +1036,9 @@ static int connect_ring(struct backend_i
xenbus_dev_fatal(dev, err, "unknown fe protocol %s", protocol);
return -ENOSYS;
}
- err = xenbus_gather(XBT_NIL, dev->otherend,
- "feature-persistent", "%u",
- &pers_grants, NULL);
- if (err)
+ err = xenbus_scanf(XBT_NIL, dev->otherend,
+ "feature-persistent", "%u", &pers_grants);
+ if (err <= 0)
pers_grants = 0;
be->blkif->vbd.feature_gnt_persistent = pers_grants;
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 3/4] xen-blkfront: prefer xenbus_scanf() over xenbus_gather()
For single items being collected this should be preferred as being more typesafe (as the compiler can check format string and to-be-written-to variable match) and more efficient (requiring one less parameter to be passed). Signed-off-by: Jan Beulich Acked-by: Roger Pau Monné --- v2: Avoid commit message to continue from subject. --- drivers/block/xen-blkfront.c | 43 +++ 1 file changed, 19 insertions(+), 24 deletions(-) --- 4.7-rc6-prefer-xenbus_scanf.orig/drivers/block/xen-blkfront.c +++ 4.7-rc6-prefer-xenbus_scanf/drivers/block/xen-blkfront.c @@ -2208,10 +2208,9 @@ static void blkfront_setup_discard(struc info->discard_granularity = discard_granularity; info->discard_alignment = discard_alignment; } - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "discard-secure", "%d", &discard_secure, - NULL); - if (!err) + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "discard-secure", "%u", &discard_secure); + if (err > 0) info->feature_secdiscard = !!discard_secure; } @@ -2310,9 +2309,8 @@ static void blkfront_gather_backend_feat info->feature_flush = 0; - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "feature-barrier", "%d", &barrier, - NULL); + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "feature-barrier", "%d", &barrier); /* * If there's no "feature-barrier" defined, then it means @@ -2321,38 +2319,35 @@ static void blkfront_gather_backend_feat * * If there are barriers, then we use flush. */ - if (!err && barrier) + if (err > 0 && barrier) info->feature_flush = REQ_FLUSH | REQ_FUA; /* * And if there is "feature-flush-cache" use that above * barriers. */ - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "feature-flush-cache", "%d", &flush, - NULL); + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "feature-flush-cache", "%d", &flush); - if (!err && flush) + if (err > 0 && flush) info->feature_flush = REQ_FLUSH; - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "feature-discard", "%d", &discard, - NULL); + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "feature-discard", "%d", &discard); - if (!err && discard) + if (err > 0 && discard) blkfront_setup_discard(info); - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "feature-persistent", "%u", &persistent, - NULL); - if (err) + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "feature-persistent", "%d", &persistent); + if (err <= 0) info->feature_persistent = 0; else info->feature_persistent = persistent; - err = xenbus_gather(XBT_NIL, info->xbdev->otherend, - "feature-max-indirect-segments", "%u", &indirect_segments, - NULL); - if (err) + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend, + "feature-max-indirect-segments", "%u", + &indirect_segments); + if (err <= 0) info->max_indirect_segments = 0; else info->max_indirect_segments = min(indirect_segments, ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH v2 4/4] xen-netback: prefer xenbus_scanf() over xenbus_gather()
For single items being collected this should be preferred as being more
typesafe (as the compiler can check format string and to-be-written-to
variable match) and more efficient (requiring one less parameter to be
passed).
Signed-off-by: Jan Beulich
---
v2: Avoid commit message to continue from subject.
---
drivers/net/xen-netback/xenbus.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
--- 4.7-rc6-prefer-xenbus_scanf.orig/drivers/net/xen-netback/xenbus.c
+++ 4.7-rc6-prefer-xenbus_scanf/drivers/net/xen-netback/xenbus.c
@@ -842,16 +842,16 @@ static int connect_ctrl_ring(struct back
unsigned int evtchn;
int err;
- err = xenbus_gather(XBT_NIL, dev->otherend,
- "ctrl-ring-ref", "%u", &val, NULL);
- if (err)
+ err = xenbus_scanf(XBT_NIL, dev->otherend,
+ "ctrl-ring-ref", "%u", &val);
+ if (err <= 0)
goto done; /* The frontend does not have a control ring */
ring_ref = val;
- err = xenbus_gather(XBT_NIL, dev->otherend,
- "event-channel-ctrl", "%u", &val, NULL);
- if (err) {
+ err = xenbus_scanf(XBT_NIL, dev->otherend,
+ "event-channel-ctrl", "%u", &val);
+ if (err <= 0) {
xenbus_dev_fatal(dev, err,
"reading %s/event-channel-ctrl",
dev->otherend);
@@ -872,7 +872,7 @@ done:
return 0;
fail:
- return err;
+ return err ?: -ENODATA;
}
static void connect(struct backend_info *be)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
On 08/07/16 13:15, Jan Beulich wrote: > As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and > CPUID[0x8007].EDX[7] anymore, causing the driver to fail to load on > both Intel and AMD systems. Doing any kind of hardware capability > checks in the driver as a prerequisite was wrong anyway: With the > hypervisor being in charge, all such checking should be done by it. If > ACPI data gets uploaded despite some missing capability, the hypervisor > is free to ignore part or all of that data. > > Ditch the entire check_prereq() function, and do the only valid check > (xen_initial_domain()) in the caller in its place. Thanks, but I'm not sure this is sufficient. I think the generic ACPI code needs to know the full capabilities in order to generate the correct tables, or you won't get (for example) turbo mode working. We had to fake the EST feature back in. --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) if ((cx & xsave_mask) != xsave_mask) cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE */ if (xen_check_mwait()) - cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); + cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) + | 1 << (X86_FEATURE_EST % 32)); } static void xen_set_debugreg(int reg, unsigned long val) David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions
On 08/07/16 14:15, Wei Liu wrote:
> On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
>> On 29/06/16 14:52, Andrew Cooper wrote:
>>> On 29/06/16 13:44, Juergen Gross wrote:
@@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
/* Tell the kernel we're up and running. */
xenbus_notify_running();
-#if defined(XEN_SYSTEMD_ENABLED)
- if (systemd) {
- sd_notify(1, "READY=1");
- fprintf(stderr, SD_NOTICE "xenstored is ready\n");
- }
-#endif
>>>
>>> Getting rid of the socket configuration for systemd is ok, but we should
>>> keep the sd_notify() calls for when the daemon is started by systemd.
>>>
>>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
>>> still required if we don't want systemd to treat xenstored as a legacy
>>> unix daemon.
>>
>> So what is the downside of xenstored being treated as a legacy daemon?
>> This question is especially interesting for the case of patch 2 being
>> considered: xenstored is no longer started by systemd, but by a wrapper
>> script which might decide to start the xenstore domain instead.
>>
>> Another problem: today xenstored decides whether to call sd_notify()
>> by testing the xenstore sockets being specified via systemd. This will
>> no longer work. So how to do it now?
>>
>
> Not sure I follow.
>
> See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
> to be able to tell if cxenstored is started by systemd or not.
> sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
> miss anything?
Did you have a look at systemd_checkin()? It expects systemd does know
exactly the number of sockets xenstored needs. Only if systemd knows
about those sockets xenstored is regarded to have been started via
systemd.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
>>> On 08.07.16 at 14:29, wrote: > On 08/07/16 13:15, Jan Beulich wrote: >> As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and >> CPUID[0x8007].EDX[7] anymore, causing the driver to fail to load on >> both Intel and AMD systems. Doing any kind of hardware capability >> checks in the driver as a prerequisite was wrong anyway: With the >> hypervisor being in charge, all such checking should be done by it. If >> ACPI data gets uploaded despite some missing capability, the hypervisor >> is free to ignore part or all of that data. >> >> Ditch the entire check_prereq() function, and do the only valid check >> (xen_initial_domain()) in the caller in its place. > > Thanks, but I'm not sure this is sufficient. I think the generic ACPI > code needs to know the full capabilities in order to generate the > correct tables, or you won't get (for example) turbo mode working. > > We had to fake the EST feature back in. > > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) > if ((cx & xsave_mask) != xsave_mask) > cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE > */ > if (xen_check_mwait()) > - cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); > + cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) > +| 1 << (X86_FEATURE_EST % 32)); > } > > static void xen_set_debugreg(int reg, unsigned long val) Hmm, interesting. I admit I only tested on an AMD system, so I can't exclude the above is necessary. Otoh going over generic ACPI code the only use of X86_FEATURE_EST controls the logging of a message. Plus there's a use in arch_acpi_set_pdc_bits() - perhaps that's the one you mean? There's certainly no use of X86_FEATURE_HW_PSTATE anywhere in relevant code, so the AMD side would appear to be fine (which matches my testing). So I think the patch is fine as is (also avoiding cross component adjustments), and the part you suggest may then better be a separate patch? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [xen-unstable-smoke test] 96790: tolerable all pass - PUSHED
flight 96790 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/96790/
Failures :-/ but no regressions.
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 12 migrate-support-checkfail never pass
test-armhf-armhf-xl 12 migrate-support-checkfail never pass
test-armhf-armhf-xl 13 saverestore-support-checkfail never pass
version targeted for testing:
xen 1e2d167d8faae843b80487e5026b07a135cf4147
baseline version:
xen 5200cead3ec350e9fc0a2991383775bf5dd0624a
Last test of basis96783 2016-07-08 08:02:11 Z0 days
Testing same since96790 2016-07-08 11:00:55 Z0 days1 attempts
People who touched revisions under test:
Dario Faggioli
George Dunlap
jobs:
build-amd64 pass
build-armhf pass
build-amd64-libvirt pass
test-armhf-armhf-xl pass
test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
test-amd64-amd64-libvirt pass
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Pushing revision :
+ branch=xen-unstable-smoke
+ revision=1e2d167d8faae843b80487e5026b07a135cf4147
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke
1e2d167d8faae843b80487e5026b07a135cf4147
+ branch=xen-unstable-smoke
+ revision=1e2d167d8faae843b80487e5026b07a135cf4147
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
getconfig Repos
perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.7-testing
+ '[' x1e2d167d8faae843b80487e5026b07a135cf4147 = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
getconfig GitCacheProxy
perl -e '
use Osstest;
readglobalconfig();
print $c{"GitCacheProxy"} or die $!;
'
+++ local cache=git://cache:9419/
+++ '[' xgit://cache:9419/ '!=' x ']'
+++ echo
'git://cache:
Re: [Xen-devel] [PATCH 1/2] tools: remove systemd xenstore socket definitions
On Fri, Jul 08, 2016 at 02:32:31PM +0200, Juergen Gross wrote:
> On 08/07/16 14:15, Wei Liu wrote:
> > On Wed, Jun 29, 2016 at 03:00:41PM +0200, Juergen Gross wrote:
> >> On 29/06/16 14:52, Andrew Cooper wrote:
> >>> On 29/06/16 13:44, Juergen Gross wrote:
> @@ -2068,13 +1964,6 @@ int main(int argc, char *argv[])
> /* Tell the kernel we're up and running. */
> xenbus_notify_running();
>
> -#if defined(XEN_SYSTEMD_ENABLED)
> -if (systemd) {
> -sd_notify(1, "READY=1");
> -fprintf(stderr, SD_NOTICE "xenstored is ready\n");
> -}
> -#endif
> >>>
> >>> Getting rid of the socket configuration for systemd is ok, but we should
> >>> keep the sd_notify() calls for when the daemon is started by systemd.
> >>>
> >>> Socket activiation and sd_notify() are orthogonal, and sd_notify() is
> >>> still required if we don't want systemd to treat xenstored as a legacy
> >>> unix daemon.
> >>
> >> So what is the downside of xenstored being treated as a legacy daemon?
> >> This question is especially interesting for the case of patch 2 being
> >> considered: xenstored is no longer started by systemd, but by a wrapper
> >> script which might decide to start the xenstore domain instead.
> >>
> >> Another problem: today xenstored decides whether to call sd_notify()
> >> by testing the xenstore sockets being specified via systemd. This will
> >> no longer work. So how to do it now?
> >>
> >
> > Not sure I follow.
> >
> > See 81d758afca7c3c1e3ccbd78154b33d64fd7757fb. I expect systemd_checkin
> > to be able to tell if cxenstored is started by systemd or not.
> > sd_listen_fds doesn't seem to involve testing xenstore sockets. Do I
> > miss anything?
>
> Did you have a look at systemd_checkin()? It expects systemd does know
> exactly the number of sockets xenstored needs. Only if systemd knows
> about those sockets xenstored is regarded to have been started via
> systemd.
>
>
Ah, I see what you meant. I will let someone who is more familiar with
systemd to comment.
Wei.
> Juergen
>
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] xenbits "official" repo for XTF (was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)
On 07/07/16 18:17, Lars Kurth wrote: > Alright, > > it appears we are at an impasse here. Not hosting the code on xenbits as > suggested by David, seems to be the worst solution and will benefit > no-one. > >> If we can't get consensus on something like this, the sensible thing >> to do would be to vote. Our governance docs don't really cope with >> this kind of multi-answer question; they only do yes/no. > I am not convinced that we need a formal process in this case. Our > governance has the mechanism to referee, when there is disagreement. For > code changes the referee would be the maintainer/committer which owns a > piece of code and the mechanism would work by withholding an ACK. For > unowned changes the referee would be the project lead: but we have none > and in fact we want none. > > The next level up is the Advisory Board: but I really don't want to go to > the AB with a bike-shed issue like this. > > > In particular as WE DO ACTUALLY HAVE CONSENSUS for a compromise by the two > main people disagreeing. > >> On 20/06/16 18:03, Ian Jackson wrote: >> I could live with "xtf", although I think it's rather too short. > > >> On 07/07/2016 12:26, "Andrew Cooper" wrote: >>> On 07/07/16 12:10, Lars Kurth wrote: >>> @Andrew: would something like test/xtf.git work >> It would, although given a straight choice I would prefer >> xen-test-framework.git over its abbreviation. > > So let's just go with "./xtf.git" and make use of the "Description" field > in http://xenbits.xen.org/gitweb/ to add a bit more verbosity. Adding > something such as "Xen Test Framework and Suite for creating > microkernel-based tests". This is accurate and searchable. > > It is no worse than "raisin.git", "osstest.git", and other top-level repos. > > Maybe we can make improve the description for "./osstest.git": something > along the lines of "Xen Test Framework and Suite, used for Open Source Xen > Continuous Integration that also acts as push gate" or something like it. > That would be more accurate than what we have now. > > Compromise > A.1) Create "xtf.git" and use "Xen Test Framework and Suite for creating > microkernel-based tests" in Description field > > A.2) Update description for osstest.git to "Xen Test Framework and Suite, > used for Open Source Xen Continuous Integration that also acts as push > gate" > >> Out of curiosity, I searched for it on google, and found my written >> documentation as the top hit. >> >> >> https://www.google.co.uk/search?q=xen+test+framework&ie=utf-8&oe=utf-8&gws >> _rd=cr&ei=InxeV52HDYHc-QG0-qN4 > That has nothing to do with the repo name. The reason why google finds > this page is because > http://xenbits.xen.org/people/andrewcoop/xen-test-framework/ exists, but > no equivalent page exists for OSSTEST. > > > Improvements to web searchability for "xen test framework" to ensure that > searches for both frameworks lead somewhere sensible > B.1) http://xenbits.xen.org/people/andrewcoop/xen-test-framework/ should > be move under docs and re-named to "XTF: Xen Test Framework and Suite for > creating microkernel-based tests" > > B.2) Add a similar page under docs for OSSTEST with a similarly verbose > title, e.g. "OSSTEST: Xen Test Framework and Suite for Open Source Xen > Continuous Integration" > > That should address everyones concern, as far as I can tell from the the > e-mail thread. If anyone disagrees, please shout within the next few days. > > Best Regards > Lars > P.S.: I moved fixing some of our governance issues towards the top of my > TODO list I have no problem with Ian's earlier suggestion: > "CI (continuous integration)" is the keyword that many people will > have for osstest. > > I would suggest > > (This is not the Xen Project's CI / Continuous Integration / >automated push gate system. For that, see >osstest.) > > or something. Adding something like that to the XTF documentation is perfectly fine. I also have no problem with the other xtf changes in descriptions/etc suggested. However, OSSTest has always been known as OSSTest (including all references in the automated emails), and not as a xen test framework. Taking any steps to make OSSTest retroactively searchable as a xen test framework is a dumb move, which will only confuse users. I fully admit that had OSSTest been named differently then I might not have chosen XTF as a name, but that didn't happen. Trying to rewrite history isn't the answer. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v3 1/2] Interface for grant copy operation in libs.
To unblock Paulina on her series, I would be ok with the cast provided there is compile-time check to ensure the user-space structure is identical to the ioctl structure. That would involve: 1. Introducing BUILD_BUG_ON, offsetof, alignof to libs/ if they are not already available. 2. BUILD_BUG_ON(sizeof(A) != sizeof(B)) 3. BUILD_BUG_ON(offsetof(A, f1) != offsetof(B, f1)) (enumerate all fields) 4. BUILD_BUG_ON(alignof(A) != alignof(B)) Paulina, let me know if you would be interested in doing #1. Normally this requires reading compiler manuals and some coding. I can give you more details if you're up for the task, otherwise I will try to find some time to do it myself. Wei. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v5 01/14] libxc: Rework extra module initialisation
On Fri, Jul 08, 2016 at 12:29:36PM +0100, Wei Liu wrote:
> On Fri, Jul 08, 2016 at 11:52:08AM +0100, Anthony PERARD wrote:
> > On Thu, Jul 07, 2016 at 03:55:23PM +0100, Wei Liu wrote:
> > > On Wed, Jun 22, 2016 at 06:15:32PM +0100, Anthony PERARD wrote:
> > > > diff --git a/tools/libxc/xc_dom_hvmloader.c
> > > > b/tools/libxc/xc_dom_hvmloader.c
> > > > index 330d5e8..da8b995 100644
> > > > --- a/tools/libxc/xc_dom_hvmloader.c
> > > > +++ b/tools/libxc/xc_dom_hvmloader.c
> > > > @@ -129,98 +129,52 @@ static elf_errorstatus
> > > > xc_dom_parse_hvm_kernel(struct xc_dom_image *dom)
> > > > return rc;
> > > > }
> > > >
> > > > -static int modules_init(struct xc_dom_image *dom,
> > > > -uint64_t vend, struct elf_binary *elf,
> > > > -uint64_t *mstart_out, uint64_t *mend_out)
> > > > +static int module_init_one(struct xc_dom_image *dom,
> > > > + struct xc_hvm_firmware_module *module,
> > > > + char *name)
> > > > {
> > > > -#define MODULE_ALIGN 1UL << 7
> > > > -#define MB_ALIGN 1UL << 20
> > > > -#define MKALIGN(x, a) (((uint64_t)(x) + (a) - 1) & ~(uint64_t)((a) -
> > > > 1))
> > > > -uint64_t total_len = 0, offset1 = 0;
> > > > +struct xc_dom_seg seg;
> > > > +void *dest;
> > > >
> > > > -if ( dom->acpi_module.length == 0 && dom->smbios_module.length ==
> > > > 0 )
> > > > -return 0;
> > > > -
> > > > -/* Find the total length for the firmware modules with a
> > > > reasonable large
> > > > - * alignment size to align each the modules.
> > > > - */
> > > > -total_len = MKALIGN(dom->acpi_module.length, MODULE_ALIGN);
> > > > -offset1 = total_len;
> > > > -total_len += MKALIGN(dom->smbios_module.length, MODULE_ALIGN);
> > > > -
> > > > -/* Want to place the modules 1Mb+change behind the loader image. */
> > > > -*mstart_out = MKALIGN(elf->pend, MB_ALIGN) + (MB_ALIGN);
> > > > -*mend_out = *mstart_out + total_len;
> > > > -
> > > > -if ( *mend_out > vend )
> > > > -return -1;
> > > > -
> > > > -if ( dom->acpi_module.length != 0 )
> > > > -dom->acpi_module.guest_addr_out = *mstart_out;
> > > > -if ( dom->smbios_module.length != 0 )
> > > > -dom->smbios_module.guest_addr_out = *mstart_out + offset1;
> > > > +if ( module->length )
> > > > +{
> > > > +if ( xc_dom_alloc_segment(dom, &seg, name, 0, module->length) )
> > > > +goto err;
> > > > +dest = xc_dom_seg_to_ptr(dom, &seg);
> > > > +if ( dest == NULL )
> > > > +{
> > > > +DOMPRINTF("%s: xc_dom_seg_to_ptr(dom, &seg) => NULL",
> > > > + __FUNCTION__);
> > > > +goto err;
> > > > +}
> > > > +memcpy(dest, module->data, module->length);
> > > > +module->guest_addr_out = seg.vstart;
> > > > +if ( module->guest_addr_out > UINT32_MAX ||
> > > > + module->guest_addr_out + module->length > UINT32_MAX )
> > > > +{
> > > > +DOMPRINTF("%s: Module %s would be loaded abrove 4GB",
> > > > + __FUNCTION__, name);
> > > > +goto err;
> > > > +}
> > >
> > > One question:
> > >
> > > Can this check also account for MMIO hole below 4G? Maybe use
> > > dom->mmio_size?
> >
> > Yes, I guess I can check against dom->mmio_start. Should I also check
> > that mmio_start have reasonable value? (<4G, and not 0x0) Or is
> > mmio_start is already supposed to have a good value?
> >
>
> mmio_start should already have a sane value here -- or at least I hope
> so. The sanity of mmio_start should be checked where it is assigned.
Ok, I'll use dom->mmio_start instead of UINT32_MAX, and probably add an
assert() on the values expected in mmio_start.
Thanks,
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 06/20] acpi/hvmloader: Collect processor and NUMA info in hvmloader
>>> On 05.07.16 at 21:05, wrote:
> No need for ACPI code to rely on hvm_info.
Perhaps better "... on the hvm_info variable"?
> @@ -118,9 +118,9 @@ static struct acpi_20_madt *construct_madt(struct
> acpi_info *info)
> io_apic->ioapic_addr = IOAPIC_BASE_ADDRESS;
>
> lapic = (struct acpi_20_madt_lapic *)(io_apic + 1);
> -info->nr_cpus = hvm_info->nr_vcpus;
> -info->madt_lapic0_addr = (uint32_t)lapic;
> -for ( i = 0; i < hvm_info->nr_vcpus; i++ )
> +config->ainfo.nr_cpus =config-> hvminfo->nr_vcpus;
Misplaced space.
> memory = (struct acpi_20_srat_memory *)processor;
> -for ( i = 0; i < nr_vmemranges; i++ )
> +for ( i = 0; i < config->numa.nr_vmemranges; i++ )
> {
> memory->type = ACPI_MEMORY_AFFINITY;
> memory->length= sizeof(*memory);
> -memory->domain= vmemrange[i].nid;
> +memory->domain= config->numa.vmemrange[i].nid;
> memory->flags = ACPI_MEM_AFFIN_ENABLED;
> -memory->base_address = vmemrange[i].start;
> -memory->mem_length= vmemrange[i].end - vmemrange[i].start;
> +memory->base_address = config->numa.vmemrange[i].start;
> +memory->mem_length= config->numa.vmemrange[i].end -
> +config->numa.vmemrange[i].start;
I'd prefer for the two config-> of this expression to align with one
another.
> --- a/tools/firmware/hvmloader/acpi/libacpi.h
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -28,6 +28,8 @@
> #ifndef __LIBACPI_H__
> #define __LIBACPI_H__
>
> +#include
I think this can be avoided if ...
> @@ -51,6 +53,14 @@ struct acpi_info {
> uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
> };
>
> +struct acpi_numa {
> +uint32_t nr_vmemranges;
> +uint32_t nr_vnodes;
> +unsigned int *vcpu_to_vnode;
> +unsigned int *vdistance;
> +xen_vmemrange_t *vmemrange;
... you use struct xen_vmemrange * here.
Also I think the two pointed to types can and should be const
qualified.
And then - any reason not to put this ...
> @@ -66,6 +76,9 @@ struct acpi_config {
> uint32_t pt_length;
> } pt;
>
> +struct acpi_numa numa;
... right here, perhaps even omitting the structure tag?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 07/20] acpi/hvmloader: Set TIS header address in hvmloader
>>> On 05.07.16 at 21:05, wrote:
> @@ -79,6 +80,8 @@ struct acpi_config {
> struct acpi_numa numa;
> struct hvm_info_table *hvminfo;
>
> +uint16_t *tis_hdr;
const?
With that adjusted (unless impossible)
Reviewed-by: Jan Beulich
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 08/20] acpi/hvmloader: Make providing IOAPIC in MADT optional
>>> On 05.07.16 at 21:05, wrote: > Signed-off-by: Boris Ostrovsky Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] xenbits "official" repo for XTF (was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)
On 08/07/2016 14:06, "Andrew Cooper" wrote: >On 07/07/16 18:17, Lars Kurth wrote: >> Alright, >> >> it appears we are at an impasse here. Not hosting the code on xenbits as >> suggested by David, seems to be the worst solution and will benefit >> no-one. >> >>... >> That should address everyones concern, as far as I can tell from the the >> e-mail thread. If anyone disagrees, please shout within the next few >>days. >> >> Best Regards >> Lars >> P.S.: I moved fixing some of our governance issues towards the top of my >> TODO list > >I have no problem with Ian's earlier suggestion: Alright. Ian confirmed on IRC that he has no problem either. Which means the main issue is unblocked and the repo can be created. > >> "CI (continuous integration)" is the keyword that many people will >> have for osstest. >> >> I would suggest >> >> (This is not the Xen Project's CI / Continuous Integration / >>automated push gate system. For that, see >>osstest.) >> >> or something. > >Adding something like that to the XTF documentation is perfectly fine. >I also have no problem with the other xtf changes in descriptions/etc >suggested. That makes perfect sense (but also see below). >However, OSSTest has always been known as OSSTest (including all >references in the automated emails), and not as a xen test framework. >Taking any steps to make OSSTest retroactively searchable as a xen test >framework is a dumb move, which will only confuse users. > >I fully admit that had OSSTest been named differently then I might not >have chosen XTF as a name, but that didn't happen. Trying to rewrite >history isn't the answer. I am not trying to re-write history. Ultimately I don't mind what the exact description of OSSTEST says, as long as it is accurate, which is why I said "something along the lines of ...". Right now there is no wiki or other xenbits documentation about OSSTEST which is outside the git tree (except for a few blog posts). Which is mainly why I raised this. Of course you are right that OSSTest is established and people who have been around for a while know that. The key question is what newcomers who don't know would search for: and to be honest I don't know. But it seems reasonable to me that the "OSSTEST" page should come up when googling for a combination/sub-set of the following keywords: "test", "xen", "Continuous Integration", "CI", "suite", "push-gate" and maybe "framework". Lars ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 09/20] acpi/hvmloader: Build WAET optionally
>>> On 05.07.16 at 21:05, wrote:
> --- a/tools/firmware/hvmloader/acpi/build.c
> +++ b/tools/firmware/hvmloader/acpi/build.c
> @@ -342,9 +342,12 @@ static int construct_secondary_tables(unsigned long
> *table_ptrs,
> }
>
> /* WAET. */
> -waet = construct_waet();
> -if (!waet) return -1;
> -table_ptrs[nr_tables++] = (unsigned long)waet;
> +if ( config->table_flags & ACPI_BUILD_WAET )
> +{
> +waet = construct_waet();
> +if ( !waet ) return -1;
Now that you touch it, this should become two lines. With that
Reviewed-by: Jan Beulich
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] xenbits "official" repo for XTF (was Re: [PATCH 0/2] xtf: add launcher (+1 bugfix)
On 08/07/16 14:42, Lars Kurth wrote: > > On 08/07/2016 14:06, "Andrew Cooper" wrote: > >> On 07/07/16 18:17, Lars Kurth wrote: >>> Alright, >>> >>> it appears we are at an impasse here. Not hosting the code on xenbits as >>> suggested by David, seems to be the worst solution and will benefit >>> no-one. >>> >>> ... >>> That should address everyones concern, as far as I can tell from the the >>> e-mail thread. If anyone disagrees, please shout within the next few >>> days. >>> >>> Best Regards >>> Lars >>> P.S.: I moved fixing some of our governance issues towards the top of my >>> TODO list >> I have no problem with Ian's earlier suggestion: > Alright. Ian confirmed on IRC that he has no problem either. Which means > the main issue is unblocked and the repo can be created. > >>> "CI (continuous integration)" is the keyword that many people will >>> have for osstest. >>> >>> I would suggest >>> >>> (This is not the Xen Project's CI / Continuous Integration / >>>automated push gate system. For that, see >>>osstest.) >>> >>> or something. >> Adding something like that to the XTF documentation is perfectly fine. >> I also have no problem with the other xtf changes in descriptions/etc >> suggested. > That makes perfect sense (but also see below). > >> However, OSSTest has always been known as OSSTest (including all >> references in the automated emails), and not as a xen test framework. >> Taking any steps to make OSSTest retroactively searchable as a xen test >> framework is a dumb move, which will only confuse users. >> >> I fully admit that had OSSTest been named differently then I might not >> have chosen XTF as a name, but that didn't happen. Trying to rewrite >> history isn't the answer. > I am not trying to re-write history. Ultimately I don't mind what the > exact description of OSSTEST says, as long as it is accurate, which is why > I said "something along the lines of ...". Right now there is no wiki or > other xenbits documentation about OSSTEST which is outside the git tree > (except for a few blog posts). Which is mainly why I raised this. > > Of course you are right that OSSTest is established and people who have > been around for a while know that. The key question is what newcomers who > don't know would search for: and to be honest I don't know. But it seems > reasonable to me that the "OSSTEST" page should come up when googling for > a combination/sub-set of the following keywords: "test", "xen", > "Continuous Integration", "CI", "suite", "push-gate" and maybe > "framework". I think the suggested changes to XTF documentation/description should be able to cover these cases. I don't expect anyone to be blindly trying to find things like this; I would expect that searches would be based on information from the xen-devel list to start with. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7
On 08/07/16 13:53, Jan Beulich wrote: On 08.07.16 at 14:29, wrote: >> On 08/07/16 13:15, Jan Beulich wrote: >>> As of Xen 4.7 PV CPUID doesn't expose either of CPUID[1].ECX[7] and >>> CPUID[0x8007].EDX[7] anymore, causing the driver to fail to load on >>> both Intel and AMD systems. Doing any kind of hardware capability >>> checks in the driver as a prerequisite was wrong anyway: With the >>> hypervisor being in charge, all such checking should be done by it. If >>> ACPI data gets uploaded despite some missing capability, the hypervisor >>> is free to ignore part or all of that data. >>> >>> Ditch the entire check_prereq() function, and do the only valid check >>> (xen_initial_domain()) in the caller in its place. >> >> Thanks, but I'm not sure this is sufficient. I think the generic ACPI >> code needs to know the full capabilities in order to generate the >> correct tables, or you won't get (for example) turbo mode working. >> >> We had to fake the EST feature back in. >> >> --- a/arch/x86/xen/enlighten.c >> +++ b/arch/x86/xen/enlighten.c >> @@ -448,7 +448,8 @@ static void __init xen_init_cpuid_mask(void) >> if ((cx & xsave_mask) != xsave_mask) >> cpuid_leaf1_ecx_mask &= ~xsave_mask; /* disable XSAVE & OSXSAVE >> */ >> if (xen_check_mwait()) >> -cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32)); >> +cpuid_leaf1_ecx_set_mask = (1 << (X86_FEATURE_MWAIT % 32) >> + | 1 << (X86_FEATURE_EST % 32)); >> } >> >> static void xen_set_debugreg(int reg, unsigned long val) > > Hmm, interesting. I admit I only tested on an AMD system, so I > can't exclude the above is necessary. Otoh going over generic > ACPI code the only use of X86_FEATURE_EST controls the > logging of a message. Plus there's a use in > arch_acpi_set_pdc_bits() - perhaps that's the one you mean? > > There's certainly no use of X86_FEATURE_HW_PSTATE anywhere > in relevant code, so the AMD side would appear to be fine (which > matches my testing). So I think the patch is fine as is (also avoiding > cross component adjustments), and the part you suggest may then > better be a separate patch? It's also possible that I'm misremembering why we went with the above hack. I've applied your patch to for-linus-3.7b, thanks. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] xen-blkback: constify instance of "struct attribute_group"
On Thu, Jul 07, 2016 at 09:57:10AM -0600, Jan Beulich wrote: > >>> On 07.07.16 at 17:37, wrote: > > On Thu, Jul 07, 2016 at 10:52:18AM +0100, Andrew Cooper wrote: > >> On 07/07/16 10:45, Roger Pau Monne wrote: > >> > On Thu, Jul 07, 2016 at 01:38:58AM -0600, Jan Beulich wrote: > >> >> The functions such get passed to have been taking pointers to const > >> >> since at least 2.6.16. > >> >> > >> >> Signed-off-by: Jan Beulich > >> > Acked-by: Roger Pau Monné > >> > > >> > Although the wording in the commit message looks weird to me, but I'm > >> > not a > > > >> > native speaker anyway. > >> > >> As a native speaker, I can't parse it either. > >> > >> I think s/such/these/ is needed. > > > > The functions such as these have been taking pointers to const > > since at least 2.6.16. > > I had taken Andrew's suggestion literally and changed it to "The > functions these get passed to have been taking pointers to const > since at least 2.6.16" for a possible (if needed) resubmission. No need to resubmit. I will use Andrew's suggestion. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 10/20] acpi/hvmloader: Replace mem_alloc() and virt_to_phys() with memory ops
>>> On 05.07.16 at 21:05, wrote:
> Components that wish to use ACPI builder will need to provide their own
> mem_alloc() and virt_to_phys() routines. Pointers to these routines will
> be passed to the builder as memory ops.
>
> Signed-off-by: Boris Ostrovsky
> ---
>
> Changes in v1:
> * Keep memory ops seprate from acpi_config, in struct acpi_context.
>
> Jan requested adding a free() op to struct acpi_mem_ops. I couldn't see who
> might want to
> use those. The builder uses (should use) mem_alloc() to allocate memory for
> tables, not as a
> general-purpose allocator.
In addition to what I said back then, did you think of error cleanup
paths here? Not all errors mean the guest has to die.
> @@ -453,18 +462,18 @@ static int new_vm_gid(struct acpi_config *config)
> return 1;
>
> /* copy to allocate BIOS memory */
> -buf = (uint64_t *) mem_alloc(sizeof(config->vm_gid), 8);
> +buf = (uint64_t *) ctxt->mem_ops.alloc(ctxt, sizeof(config->vm_gid), 8);
Once again this appears to be a cast that already before was
unnecessary. So it (and in any event the stray blank) should
perhaps already get deleted when you touch this line the first
time (in one of the earlier patches).
> --- a/tools/firmware/hvmloader/acpi/libacpi.h
> +++ b/tools/firmware/hvmloader/acpi/libacpi.h
> @@ -64,6 +64,13 @@ struct acpi_numa {
> xen_vmemrange_t *vmemrange;
> };
>
> +struct acpi_ctxt {
> +struct acpi_mem_ops {
> +void *(*alloc)(struct acpi_ctxt *ctxt, uint32_t size, uint32_t
> align);
> +unsigned long (*v2p)(struct acpi_ctxt *ctxt, void *v);
> +} mem_ops;
> +};
> +
> struct acpi_config {
> const unsigned char *dsdt_anycpu;
> unsigned int dsdt_anycpu_len;
While you mention this in the revision log, I don't see the reason
for keeping this fully separate. Quite a few of the changes you
do here could be avoided if the new structure got pointed to by a
field in struct acpi_config.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
[Xen-devel] [GIT PULL] xen: bug fixes for 4.7-rc6
Linus, Please git pull the following tag: git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-4.7b-rc6-tag xen: bug fixes for 4.7-rc6 - Fix two bugs in the handling of xenbus transactions. - Make the xen acpi driver compatible with Xen 4.7. Thanks. David drivers/xen/xen-acpi-processor.c | 35 +++- drivers/xen/xenbus/xenbus_dev_frontend.c | 14 +++-- drivers/xen/xenbus/xenbus_xs.c | 10 +++-- 3 files changed, 14 insertions(+), 45 deletions(-) Jan Beulich (4): xenbus: don't BUG() on user mode induced condition xenbus: don't bail early from xenbus_dev_request_and_reply() xenbus: simplify xenbus_dev_request_and_reply() xen/acpi: allow xen-acpi-processor driver to load on Xen 4.7 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [DRAFT 1] XenSock protocol design document
Hi Juergen, many thanks for the fast and very useful review! On Fri, 8 Jul 2016, Juergen Gross wrote: > On 08/07/16 13:23, Stefano Stabellini wrote: > > Hi all, > > > > as promised, this is the design document for the XenSock protocol I > > mentioned here: > > > > http://marc.info/?l=xen-devel&m=146520572428581 > > > > It is still in its early days but should give you a good idea of how it > > looks like and how it is supposed to work. Let me know if you find gaps > > in the document and I'll fill them in the next version. > > > > You can find prototypes of the Linux frontend and backend drivers here: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/sstabellini/xen.git xensock-1 > > > > To use them, make sure to enable CONFIG_XENSOCK in your kernel config > > and add "xensock=1" to the command line of your DomU Linux kernel. You > > also need the toolstack to create the initial xenstore nodes for the > > protocol. To do that, please apply the attached patch to libxl (the > > patch is based on Xen 4.7.0-rc3) and add "xensock=1" to your DomU config > > file. > > > > Feel free to try them out! Please be kind, they are only prototypes with > > a few known issues :-) But they should work well enough to run simple > > tests. If you find something missing, let me know or, even better, write > > a patch! > > > > I'll follow up with a separate document to cover the design of my > > particular implementation of the protocol. > > > > Cheers, > > > > Stefano > > > > --- > > > > # XenSocks Protocol v1 > > > > ## Rationale > > > > XenSocks is a paravirtualized protocol for the POSIX socket API. > > > > The purpose of XenSocks is to allow the implementation of a specific set > > of POSIX calls to be done in a domain other than your own. It allows > > connect, accept, bind, release, listen, poll, recvmsg and sendmsg to be > > implemented in another domain. > > > > XenSocks provides the following benefits: > > * guest networking works out of the box with VPNs, wireless networks and > > any other complex configurations on the host > > * guest services listen on ports bound directly to the backend domain IP > > addresses > > * localhost becomes a secure namespace for intra-VMs communications > > * full visibility of the guest behavior on the backend domain, allowing > > for inexpensive filtering and manipulation of any guest calls > > * excellent performance > > > > > > ## Design > > > > ### Xenstore > > > > The frontend and the backend connect to each other exchanging information > > via > > xenstore. The toolstack creates front and back nodes with state > > XenbusStateInitialising. There can only be one XenSock frontend per domain. > > > > Frontend XenBus Nodes > > > > port > > Values: > > > > The identifier of the Xen event channel used to signal activity > > in the ring buffer. > > > > ring-ref > > Values: > > > > The Xen grant reference granting permission for the backend to map > > the sole page in a single page sized ring buffer. > > > > > > State Machine > > > > **Front** **Back** > > XenbusStateInitialising XenbusStateInitialising > > - Query virtual device- Query backend device > > properties. identification data. > > - Setup OS device instance. | > > - Allocate and initialize the| > > request ring. V > > - Publish transport parametersXenbusStateInitWait > > that will be in effect during > > this connection. > > | > > | > > V > >XenbusStateInitialised > > > > - Query frontend transport > > parameters. > > - Connect to the request ring and > > event channel. > > | > > | > > V > > XenbusStateConnected > > > > - Query backend device properties. > > - Finalize OS virtual device > >instance. > > | > > | > > V > > XenbusStateConnected > > > > Once frontend and backend are connected, they have a shared page, which > > will is used to exchange messages over a ring, and an event channel, > > which is used to send notifications. > > > > > > ### Commands Ring > > > > The shared ring is used by the frontend to forward socket API calls to the > > backend. I'll refer to this ring as **commands ring** to distinguish it from > > other rings which will be created later in the lifecycle of the protocol > > (data > > rings
Re: [Xen-devel] [PATCH v2 0/4] xen: prefer xenbus_scanf() over xenbus_gather()
On Fri, Jul 08, 2016 at 06:21:52AM -0600, Jan Beulich wrote: > For single items being collected this should be preferred as being more > typesafe (as the compiler can check format string and to-be-written-to > variable match) and more efficient (requiring one less parameter to be > passed). > > 1: xenbus: prefer xenbus_scanf() over xenbus_gather() > 2: xen-blkback: prefer xenbus_scanf() over xenbus_gather() > 3: xen-blkfront: prefer xenbus_scanf() over xenbus_gather() > 4: xen-netback: prefer xenbus_scanf() over xenbus_gather() > > Signed-off-by: Jan Beulich > --- > v2: Avoid commit messages to continue from subjects. Group into a series. To confuse this, Roger and I are the block sub-maintainers, which when we are happy, I send to Jens, while the rest go through Boris,David, and Juergen. Anyhow, I've already committed and tested for regressions these: 79ef83a xen-blkback: constify instance of "struct attribute_group" 5e4d659 xen-blkfront: prefer xenbus_scanf() over xenbus_gather() e9d1ebe xen-blkback: prefer xenbus_scanf() over xenbus_gather() 5b3b1db xen-blkback: really don't leak mode property And plan to send them to Jens. They will shortly be at my git tree under 'stable/for-jens-4.8'. > > > > ___ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [DRAFT 1] XenSock protocol design document
On 08/07/16 16:16, Stefano Stabellini wrote:
> Hi Juergen,
>
> many thanks for the fast and very useful review!
>
>
> On Fri, 8 Jul 2016, Juergen Gross wrote:
>> On 08/07/16 13:23, Stefano Stabellini wrote:
>>> #define XENSOCK_DATARING_ORDER 6
>>> #define XENSOCK_DATARING_PAGES (1 << XENSOCK_DATARING_ORDER)
>>> #define XENSOCK_DATARING_SIZE (XENSOCK_DATARING_PAGES << PAGE_SHIFT)
>>>
>>> #define XENSOCK_CONNECT0
>>> #define XENSOCK_RELEASE3
>>> #define XENSOCK_BIND 4
>>> #define XENSOCK_LISTEN 5
>>> #define XENSOCK_ACCEPT 6
>>> #define XENSOCK_POLL 7
>>>
>>> struct xen_xensock_request {
>>> uint32_t id; /* private to guest, echoed in response */
>>> uint32_t cmd;/* command to execute */
>>> uint64_t sockid; /* id of the socket */
>>> union {
>>> struct xen_xensock_connect {
>>> uint8_t addr[28];
>>> uint32_t len;
>>> uint32_t flags;
>>> grant_ref_t ref[XENSOCK_DATARING_PAGES];
>>> uint32_t evtchn;
>>> } connect;
>>> struct xen_xensock_bind {
>>> uint8_t addr[28]; /* ipv6 ready */
>>> uint32_t len;
>>> } bind;
>>> struct xen_xensock_accept {
>>> uint64_t sockid;
>>> grant_ref_t ref[XENSOCK_DATARING_PAGES];
>>> uint32_t evtchn;
>>> } accept;
>>> } u;
>>> };
>>
>> Below you write the data ring is flexible and can support different
>> ring sizes. This is in contradiction to the definition above: as soon
>> as you modify the ring size you change the interface. You'd have to
>> modify all guests and the host at the same time.
>
> Yeah, I meant at compile time (which I understand it is not useful for
> anything other than embedded use cases). But you are right that it would
> be nice to be able to choose the ring size at runtime.
>
>
>> The flexibility should be kept, so I suggest ring size negotiation via
>> xenstore: the backend should indicate the maximum supported size and
>> the frontend should tell which size it is using. In the beginning I'd
>> see no problem with accepting connection only if both values are
>> XENSOCK_DATARING_PAGES.
>
> I'll look into it.
>
>
>> The connect and accept calls should reference only one page (possibly
>> with an offset into that page) holding the grant_ref_t array of the
>> needed size.
>
> It would be nice to send the refs as part of the request as done here,
> but I imagine that it would be an issue with a variable number of refs
> because everything in the request struct needs to be sized up at compile
> time. That's the reason why you are suggesting to send them separatly,
> right?
Correct.
>>> The data ring format will be described in the following section.
>>>
>>> Fields:
>>>
>>> - **cmd** value: 0
>>> - additional fields:
>>> - **addr**: address to connect to, in struct sockaddr format
>>
>> So you expect only Linux guests with the current sockaddr layout?
>> Please specify the structure in the interface.
>
> I meant sockaddr as defined by POSIX (the Open Group standard):
>
> http://pubs.opengroup.org/onlinepubs/7908799/xns/syssocket.h.html
Neither the size of sa_family_t nor the numeric values are defined
there.
>> Which value? I've found systems with: 57, 76, 107, 134 or 235 (just to
>> make clear that even an errno name isn't optimal).
>
> I naively assumed that the error codes were also defined by POSIX, but
> it doesn't seem to be the case. If they are not standard, I'll have to
> include a numeric representation of those error names and possibly do
> conversions. I'll get to it in the next version. I think I makes sense
> to use the existing xen/include/public/errno.h (credits to Roger for the
> suggestion on IRC).
Sure, xen/include/public/errno.h is fine.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 11/20] acpi/hvmloader: Translate all addresses when assigning addresses in ACPI tables
>>> On 05.07.16 at 21:05, wrote: > Non-hvmloader users may be building tables in virtual address space > and therefore we need to make sure that values that end up in tables > are physical addresses. > > Signed-off-by: Boris Ostrovsky Reviewed-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
On 07/08/2016 06:10 AM, Jan Beulich wrote:
>
>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config,
>> unsigned int physical)
>> offsetof(struct acpi_20_rsdp, extended_checksum),
>> sizeof(struct acpi_20_rsdp));
>>
>> -if ( !new_vm_gid(acpi_info) )
>> +if ( !new_vm_gid(&config->ainfo) )
>> goto oom;
>>
>> -acpi_info->com1_present = uart_exists(0x3f8);
>> -acpi_info->com2_present = uart_exists(0x2f8);
>> -acpi_info->lpt1_present = lpt_exists(0x378);
>> -acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>> -acpi_info->pci_min = pci_mem_start;
>> -acpi_info->pci_len = pci_mem_end - pci_mem_start;
>> -if ( pci_hi_mem_end > pci_hi_mem_start )
>> -{
>> -acpi_info->pci_hi_min = pci_hi_mem_start;
>> -acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>> -}
>> +*(struct acpi_info *)config->ainfop = config->ainfo;
> With your new separation of responsibilities - does this really
> belong here rather than in the caller?
I think it should be done here: when the call returns all tables are
already in memory. If the caller wants to load those tables separately
(as probably the toolstack will) then it can simply copy it as a blob.
>
>> +struct acpi_config {
>> +const unsigned char *dsdt_anycpu;
>> +unsigned int dsdt_anycpu_len;
>> +const unsigned char *dsdt_15cpu;
>> +unsigned int dsdt_15cpu_len;
>> +
>> +/* May have some fields updated by acpi_build_table() */
>> +struct acpi_info ainfo;
>> +/*
>> + * Address where acpi_info should be placed.
>> + * This must match the OperationRegion(BIOS, SystemMemory, )
>> + * definition in the DSDT
>> + */
>> +unsigned int ainfop;
> What is the "a" prefix of these two fields good for? Considering the
> name of the structure I don't see a need for any prefix. But if you
> absolutely want one, then acpi_ please.
You requested 'acpi_' prefix to be dropped earlier. I added 'a' (and
'pt_' in patch 5) to be a bit more cscope-friendly: names like info,
addr and length will result in lots of unnecessary hits.
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 20/20] libxl/acpi: Build ACPI tables for HVMlite guests
On 07/08/2016 06:55 AM, Wei Liu wrote:
>
>> +
>> +/* Map page that will hold RSDP */
>> +extent = RSDP_ADDRESS >> ctxt.page_shift;
>> +rc = populate_acpi_pages(dom, &extent, 1, &ctxt);
>> +if (rc) {
>> +LOG(ERROR, "%s: populate_acpi_pages for rsdp failed with %d",
>> +__FUNCTION__, rc);
>> +goto out;
>> +}
>> +config.rsdp = (unsigned long)xc_map_foreign_range(xch, domid,
>> + ctxt.page_size,
>> + PROT_READ |
>> PROT_WRITE,
>> + RSDP_ADDRESS >>
>> ctxt.page_shift);
> I think with Anthony's on-going work you should be more flexible for all
> you tables.
Not sure I understand what you mean here. You want the address
(RSDP_ADDRESS) to be a variable as opposed to a macro?
>
> I haven't really looked into details for this patch. Let's sort out
> the linkage issue between GPLv2 cod and LGPL code first.
Ugh.. Yes, this one didn't even cross my mind until you brought it up
yesterday.
What are the options? Can we dual-license those files as GPLv2/LGPL,
assuming copyright holders --- Keir (or Citrix?) and Intel --- agree?
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 12/20] acpi/hvmloader: Link ACPI object files directly
>>> On 05.07.16 at 21:05, wrote: > @@ -95,7 +96,15 @@ all: subdirs-all > ovmf.o rombios.o seabios.o hvmloader.o: roms.inc > smbios.o: CFLAGS += -D__SMBIOS_DATE__="\"$(SMBIOS_REL_DATE)\"" > > -hvmloader: $(OBJS) acpi/acpi.a > +ACPI_PATH = $(XEN_ROOT)/tools/firmware/hvmloader/acpi I think I did say so already on the RFC series: I'd much prefer if we could stick to relative references, so trees have a better chances of being movable. > +ACPI_FILES = dsdt_anycpu.c dsdt_15cpu.c dsdt_anycpu_qemu_xen.c build.c > static_tables.c > +ACPI_OBJS = $(patsubst %.c,%.o,$(ACPI_FILES)) > +$(ACPI_OBJS): CFLAGS += -I$(ACPI_PATH) -I. What is the -I. needed for? The ACPI files shouldn't be referencing other hvmloader files, and generated .h files should be included using "" instead of <>. > +vpath build.c $(ACPI_PATH)/ > +vpath static_tables.c $(ACPI_PATH)/ I don't think the trailing slashes are needed here. > --- a/tools/firmware/hvmloader/acpi/Makefile > +++ b/tools/firmware/hvmloader/acpi/Makefile > @@ -17,36 +17,41 @@ > XEN_ROOT = $(CURDIR)/../../../.. > include $(XEN_ROOT)/tools/firmware/Rules.mk > > -C_SRC = build.c dsdt_anycpu.c dsdt_15cpu.c static_tables.c > dsdt_anycpu_qemu_xen.c > -OBJS = $(patsubst %.c,%.o,$(C_SRC)) > +MK_DSDT = $(ACPI_BUILD_DIR)/mk_dsdt > > -CFLAGS += $(CFLAGS_xeninclude) > +# Sources to be generated > +C_SRC = $(ACPI_BUILD_DIR)/dsdt_anycpu.c $(ACPI_BUILD_DIR)/dsdt_15cpu.c > $(ACPI_BUILD_DIR)/dsdt_anycpu_qemu_xen.c > +H_SRC = $(ACPI_BUILD_DIR)/ssdt_s3.h $(ACPI_BUILD_DIR)/ssdt_s4.h > $(ACPI_BUILD_DIR)/ssdt_pm.h $(ACPI_BUILD_DIR)/ssdt_tpm.h Use $(addprefix ) to limit line length? > vpath iasl $(PATH) > -all: acpi.a > +all: $(C_SRC) $(H_SRC) > > -ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h: %.h: %.asl iasl > +$(H_SRC): $(ACPI_BUILD_DIR)/%.h: %.asl iasl > + cd $(ACPI_BUILD_DIR) > iasl -vs -p $* -tc $< > sed -e 's/AmlCode/$*/g' $*.hex >$@ > rm -f $*.hex $*.aml > + cd $(CURDIR) cd in a make rule feel wrong. Could you prefix most of the $* with $(ACPI_BUILD_DIR)/ instead? > @@ -56,14 +61,8 @@ iasl: > @echo > @exit 1 > > -build.o: ssdt_s3.h ssdt_s4.h ssdt_pm.h ssdt_tpm.h > - > -acpi.a: $(OBJS) > - $(AR) rc $@ $(OBJS) > - > clean: > - rm -rf *.a *.o $(IASL_VER) $(IASL_VER).tar.gz $(DEPS) > - rm -rf ssdt_*.h dsdt*.c *~ *.aml *.hex mk_dsdt dsdt_*.asl > + rm -fr $(C_SRC) $(H_SRC) $(MK_DSDT) $(patsubst %.c,%.asl,$(C_SRC)) It seems to me that this would better done in from the consumer's clean rule. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 0/4] xen: prefer xenbus_scanf() over xenbus_gather()
>>> On 08.07.16 at 16:17, wrote: > On Fri, Jul 08, 2016 at 06:21:52AM -0600, Jan Beulich wrote: >> For single items being collected this should be preferred as being more >> typesafe (as the compiler can check format string and to-be-written-to >> variable match) and more efficient (requiring one less parameter to be >> passed). >> >> 1: xenbus: prefer xenbus_scanf() over xenbus_gather() >> 2: xen-blkback: prefer xenbus_scanf() over xenbus_gather() >> 3: xen-blkfront: prefer xenbus_scanf() over xenbus_gather() >> 4: xen-netback: prefer xenbus_scanf() over xenbus_gather() >> >> Signed-off-by: Jan Beulich >> --- >> v2: Avoid commit messages to continue from subjects. Group into a series. > > To confuse this, Roger and I are the block sub-maintainers, which > when we are happy, I send to Jens, while the rest go through Boris,David, > and Juergen. Which is why originally I had sent all of these separately. Yet David was pretty unhappy about that. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
On Thu, Jul 07, 2016 at 01:45:05PM -0400, Boris Ostrovsky wrote: > On 07/07/2016 01:15 PM, Wei Liu wrote: > > On Thu, Jul 07, 2016 at 01:09:28PM -0400, Boris Ostrovsky wrote: > >> On 07/07/2016 12:58 PM, Ian Jackson wrote: > >>> There are two serious problems with this. > >>> > >>> 1. You have dropped the copyright attribution to Intel and Xensource. > >>> > >>> 2. You have changed the licence to BSD-style, even though the original > >>>was GPLv2-only. > >> > >> I meant this to be a GPLv2 license. And I made the same mistake in a > >> couple of other files. > >> > > The other question is, will this GPLv2 file be linked against other > > toolstack components (libxl is LGPL)? If so, how should we deal with > > different licences? > > Two new libxl files will be LGPL and but libacpi files will be GPLv2 > (because most of the files there are already GPLv2, I just added a > couple of new ones). > > Which would mean that a non-GPL users cannot link against libxl anymore, > right? Having different licenses will invite the lawyers in the conversation which can drag things out. A quick read says one can add an exception to GPLv2 license to allow it to be linked (see https://www.gnu.org/licenses/gpl-faq.en.html#GPLIncompatibleLibs) but that would require Copyright OK from the original holders. It would be far easier to ask the copyright holders: Andrew Cooper Anthony PERARD David Vrabel Dexuan Cui Eddie Dong Ian Campbell John Levon Keir Fraser Keir Fraser Liu, Jinsong Paul Durrant Qing He Stefan Berger Tim Deegan Tobias Geiger Xiaowei Yang If they would be OK making the code (this is from tools/firmware/hvmloader/acpi/acpi2_0.h) lGPL. Or is there some other technical way around this? I can't recall whether the 'dlopen' (so runtime loading vs linking) of an GPL library is from Lesser GPL is OK. (so proprietary code linking with libxl, and libxl dlopen'ing the libacpi code'). Sounds like we may need to consult the lawyers after all. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 06/20] acpi/hvmloader: Collect processor and NUMA info in hvmloader
On 07/08/2016 09:36 AM, Jan Beulich wrote:
>
>> @@ -51,6 +53,14 @@ struct acpi_info {
>> uint64_t pci_hi_min, pci_hi_len; /* 24, 32 - PCI I/O hole boundaries */
>> };
>>
>> +struct acpi_numa {
>> +uint32_t nr_vmemranges;
>> +uint32_t nr_vnodes;
>> +unsigned int *vcpu_to_vnode;
>> +unsigned int *vdistance;
>> +xen_vmemrange_t *vmemrange;
> ... you use struct xen_vmemrange * here.
>
> Also I think the two pointed to types can and should be const
> qualified.
>
> And then - any reason not to put this ...
>
>> @@ -66,6 +76,9 @@ struct acpi_config {
>> uint32_t pt_length;
>> } pt;
>>
>> +struct acpi_numa numa;
> ... right here, perhaps even omitting the structure tag?
I'd like to have a data type for better readability. See patch 20,
init_acpi_config().
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 02/20] acpi/hvmloader: Move acpi_info initialization out of ACPI code
>>> On 08.07.16 at 16:39, wrote:
> On 07/08/2016 06:10 AM, Jan Beulich wrote:
>>
>>> @@ -615,20 +593,10 @@ void acpi_build_tables(struct acpi_config *config,
> unsigned int physical)
>>> offsetof(struct acpi_20_rsdp, extended_checksum),
>>> sizeof(struct acpi_20_rsdp));
>>>
>>> -if ( !new_vm_gid(acpi_info) )
>>> +if ( !new_vm_gid(&config->ainfo) )
>>> goto oom;
>>>
>>> -acpi_info->com1_present = uart_exists(0x3f8);
>>> -acpi_info->com2_present = uart_exists(0x2f8);
>>> -acpi_info->lpt1_present = lpt_exists(0x378);
>>> -acpi_info->hpet_present = hpet_exists(ACPI_HPET_ADDRESS);
>>> -acpi_info->pci_min = pci_mem_start;
>>> -acpi_info->pci_len = pci_mem_end - pci_mem_start;
>>> -if ( pci_hi_mem_end > pci_hi_mem_start )
>>> -{
>>> -acpi_info->pci_hi_min = pci_hi_mem_start;
>>> -acpi_info->pci_hi_len = pci_hi_mem_end - pci_hi_mem_start;
>>> -}
>>> +*(struct acpi_info *)config->ainfop = config->ainfo;
>> With your new separation of responsibilities - does this really
>> belong here rather than in the caller?
>
> I think it should be done here: when the call returns all tables are
> already in memory. If the caller wants to load those tables separately
> (as probably the toolstack will) then it can simply copy it as a blob.
But this structure isn't part of the ACPI tables, and by not doing
it here (a) at least some of the intended callers may be able to
get away without the ugly cast and (b) the field now named
ainfop wouldn't be needed either afaict.
>>> +struct acpi_config {
>>> +const unsigned char *dsdt_anycpu;
>>> +unsigned int dsdt_anycpu_len;
>>> +const unsigned char *dsdt_15cpu;
>>> +unsigned int dsdt_15cpu_len;
>>> +
>>> +/* May have some fields updated by acpi_build_table() */
>>> +struct acpi_info ainfo;
>>> +/*
>>> + * Address where acpi_info should be placed.
>>> + * This must match the OperationRegion(BIOS, SystemMemory, )
>>> + * definition in the DSDT
>>> + */
>>> +unsigned int ainfop;
>> What is the "a" prefix of these two fields good for? Considering the
>> name of the structure I don't see a need for any prefix. But if you
>> absolutely want one, then acpi_ please.
>
> You requested 'acpi_' prefix to be dropped earlier. I added 'a' (and
> 'pt_' in patch 5) to be a bit more cscope-friendly: names like info,
> addr and length will result in lots of unnecessary hits.
I remember having that discussion once with Mukesh too. I continue
to think source code should not get uglified just because of some
random tool's limitations, maybe unless _everyone_ uses that tool.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 06/20] acpi/hvmloader: Collect processor and NUMA info in hvmloader
>>> On 08.07.16 at 17:08, wrote:
> On 07/08/2016 09:36 AM, Jan Beulich wrote:
>>> @@ -51,6 +53,14 @@ struct acpi_info {
>>> @@ -66,6 +76,9 @@ struct acpi_config {
>>> uint32_t pt_length;
>>> } pt;
>>>
>>> +struct acpi_numa numa;
>> ... right here, perhaps even omitting the structure tag?
>
> I'd like to have a data type for better readability. See patch 20,
> init_acpi_config().
Okay, if it becomes of use later on, fine with me.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 0/4] xen: prefer xenbus_scanf() over xenbus_gather()
On Fri, Jul 08, 2016 at 09:06:36AM -0600, Jan Beulich wrote: > >>> On 08.07.16 at 16:17, wrote: > > On Fri, Jul 08, 2016 at 06:21:52AM -0600, Jan Beulich wrote: > >> For single items being collected this should be preferred as being more > >> typesafe (as the compiler can check format string and to-be-written-to > >> variable match) and more efficient (requiring one less parameter to be > >> passed). > >> > >> 1: xenbus: prefer xenbus_scanf() over xenbus_gather() > >> 2: xen-blkback: prefer xenbus_scanf() over xenbus_gather() > >> 3: xen-blkfront: prefer xenbus_scanf() over xenbus_gather() > >> 4: xen-netback: prefer xenbus_scanf() over xenbus_gather() > >> > >> Signed-off-by: Jan Beulich > >> --- > >> v2: Avoid commit messages to continue from subjects. Group into a series. > > > > To confuse this, Roger and I are the block sub-maintainers, which > > when we are happy, I send to Jens, while the rest go through Boris,David, > > and Juergen. > > Which is why originally I had sent all of these separately. Yet David > was pretty unhappy about that. Aye! I am not critizing, just saying that this is getting complicated and you are in the unfortunate situation to have to deal with this - so want to apoligize for you having to go through this gauntlet. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 10/20] acpi/hvmloader: Replace mem_alloc() and virt_to_phys() with memory ops
On 07/08/2016 09:58 AM, Jan Beulich wrote:
On 05.07.16 at 21:05, wrote:
>> Components that wish to use ACPI builder will need to provide their own
>> mem_alloc() and virt_to_phys() routines. Pointers to these routines will
>> be passed to the builder as memory ops.
>>
>> Signed-off-by: Boris Ostrovsky
>> ---
>>
>> Changes in v1:
>> * Keep memory ops seprate from acpi_config, in struct acpi_context.
>>
>> Jan requested adding a free() op to struct acpi_mem_ops. I couldn't see who
>> might want to
>> use those. The builder uses (should use) mem_alloc() to allocate memory for
>> tables, not as a
>> general-purpose allocator.
> In addition to what I said back then, did you think of error cleanup
> paths here? Not all errors mean the guest has to die.
If there is an error and the builder decides to free up memory needed
for a table, how do we communicate to the caller which table has been
failed? Is it up to the builder to decide which tables are important and
which are not?
>
>>
>> +struct acpi_ctxt {
>> +struct acpi_mem_ops {
>> +void *(*alloc)(struct acpi_ctxt *ctxt, uint32_t size, uint32_t
>> align);
>> +unsigned long (*v2p)(struct acpi_ctxt *ctxt, void *v);
>> +} mem_ops;
>> +};
>> +
>> struct acpi_config {
>> const unsigned char *dsdt_anycpu;
>> unsigned int dsdt_anycpu_len;
> While you mention this in the revision log, I don't see the reason
> for keeping this fully separate. Quite a few of the changes you
> do here could be avoided if the new structure got pointed to by a
> field in struct acpi_config.
I kept them separate here because acpi_config is intended to pass data
about tables content and acpi_ctxt is needed for storing info used for
building (ops, and as will be seen in patch 20, certain allocator
information).
-boris
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v1 10/20] acpi/hvmloader: Replace mem_alloc() and virt_to_phys() with memory ops
>>> On 08.07.16 at 17:23, wrote: > On 07/08/2016 09:58 AM, Jan Beulich wrote: > On 05.07.16 at 21:05, wrote: >>> Components that wish to use ACPI builder will need to provide their own >>> mem_alloc() and virt_to_phys() routines. Pointers to these routines will >>> be passed to the builder as memory ops. >>> >>> Signed-off-by: Boris Ostrovsky >>> --- >>> >>> Changes in v1: >>> * Keep memory ops seprate from acpi_config, in struct acpi_context. >>> >>> Jan requested adding a free() op to struct acpi_mem_ops. I couldn't see who >>> might want to >>> use those. The builder uses (should use) mem_alloc() to allocate memory for >>> tables, not as a >>> general-purpose allocator. >> In addition to what I said back then, did you think of error cleanup >> paths here? Not all errors mean the guest has to die. > > If there is an error and the builder decides to free up memory needed > for a table, how do we communicate to the caller which table has been > failed? I don't understand - that's what the proposed free hook would be for. > Is it up to the builder to decide which tables are important and which > are not? I'm afraid that's not so easy to tell. If for example we can't fit the HPET table, the guest could be run without HPET unless a HPET was specifically requested (rather than just defaulted to). Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] vmx/monitor: CPUID events
On Fri, Jul 8, 2016 at 1:03 AM, Razvan Cojocaru
wrote:
> On 07/08/16 05:31, Tamas K Lengyel wrote:
>> This patch implements sending notification to a monitor subscriber when an
>> x86/vmx guest executes the CPUID instruction.
>>
>> Signed-off-by: Tamas K Lengyel
>> ---
>> Cc: Ian Jackson
>> Cc: Wei Liu
>> Cc: Razvan Cojocaru
>> Cc: Jan Beulich
>> Cc: Andrew Cooper
>> Cc: Jun Nakajima
>> Cc: Kevin Tian
>> ---
>> tools/libxc/include/xenctrl.h | 1 +
>> tools/libxc/xc_monitor.c| 13 +
>> tools/tests/xen-access/xen-access.c | 33 -
>> xen/arch/x86/hvm/monitor.c | 16
>> xen/arch/x86/hvm/vmx/vmx.c | 23 +++
>> xen/arch/x86/monitor.c | 13 +
>> xen/include/asm-x86/domain.h| 1 +
>> xen/include/asm-x86/hvm/monitor.h | 1 +
>> xen/include/asm-x86/monitor.h | 3 ++-
>> xen/include/public/domctl.h | 1 +
>> xen/include/public/vm_event.h | 8
>> 11 files changed, 107 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>> index 4a85b4a..e904bd5 100644
>> --- a/tools/libxc/include/xenctrl.h
>> +++ b/tools/libxc/include/xenctrl.h
>> @@ -2167,6 +2167,7 @@ int xc_monitor_guest_request(xc_interface *xch,
>> domid_t domain_id,
>> bool enable, bool sync);
>> int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
>> bool enable, bool sync);
>> +int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable);
>> /**
>> * This function enables / disables emulation for each REP for a
>> * REP-compatible instruction.
>> diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
>> index 264992c..4298813 100644
>> --- a/tools/libxc/xc_monitor.c
>> +++ b/tools/libxc/xc_monitor.c
>> @@ -172,6 +172,19 @@ int xc_monitor_debug_exceptions(xc_interface *xch,
>> domid_t domain_id,
>> return do_domctl(xch, &domctl);
>> }
>>
>> +int xc_monitor_cpuid(xc_interface *xch, domid_t domain_id, bool enable)
>> +{
>> +DECLARE_DOMCTL;
>> +
>> +domctl.cmd = XEN_DOMCTL_monitor_op;
>> +domctl.domain = domain_id;
>> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
>> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
>> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_CPUID;
>> +
>> +return do_domctl(xch, &domctl);
>> +}
>> +
>> /*
>> * Local variables:
>> * mode: C
>> diff --git a/tools/tests/xen-access/xen-access.c
>> b/tools/tests/xen-access/xen-access.c
>> index 02655d5..d525b82 100644
>> --- a/tools/tests/xen-access/xen-access.c
>> +++ b/tools/tests/xen-access/xen-access.c
>> @@ -337,7 +337,7 @@ void usage(char* progname)
>> {
>> fprintf(stderr, "Usage: %s [-m] write|exec", progname);
>> #if defined(__i386__) || defined(__x86_64__)
>> -fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug");
>> +fprintf(stderr,
>> "|breakpoint|altp2m_write|altp2m_exec|debug|cpuid");
>> #endif
>> fprintf(stderr,
>> "\n"
>> @@ -364,6 +364,7 @@ int main(int argc, char *argv[])
>> int shutting_down = 0;
>> int altp2m = 0;
>> int debug = 0;
>> +int cpuid = 1;
>
> Should this be on by default? All the rest of the options are 0, and ...
No, it's just a typo.
Thanks,
Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
