* [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-09 15:04 ` Andrew Cooper
2016-11-15 8:34 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
` (9 subsequent siblings)
10 siblings, 2 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
This domctl is called when a VCPU is hot-(un)plugged to a guest (via
'xl vcpu-set'). While this currently is only intended to be needed by
PVH guests we will call this domctl for all (x86) guests for consistency.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
Changes in v2:
* Added comment in arch_do_domctl() stating that the domctl is only required
for PVH guests
tools/flask/policy/modules/dom0.te | 2 +-
tools/flask/policy/modules/xen.if | 4 ++--
tools/libxc/include/xenctrl.h | 5 +++++
tools/libxc/xc_dom_x86.c | 11 +++++++++++
tools/libxl/libxl.c | 10 +++++++++-
tools/libxl/libxl_arch.h | 4 ++++
tools/libxl/libxl_arm.c | 6 ++++++
tools/libxl/libxl_dom.c | 7 +++++++
tools/libxl/libxl_x86.c | 6 ++++++
xen/arch/x86/domctl.c | 17 +++++++++++++++++
xen/include/asm-x86/domain.h | 6 ++++++
xen/include/public/domctl.h | 9 +++++++++
xen/xsm/flask/hooks.c | 3 +++
xen/xsm/flask/policy/access_vectors | 2 ++
14 files changed, 88 insertions(+), 4 deletions(-)
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 2d982d9..fd60c39 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -38,7 +38,7 @@ allow dom0_t dom0_t:domain {
};
allow dom0_t dom0_t:domain2 {
set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
- get_vnumainfo psr_cmt_op psr_cat_op
+ get_vnumainfo psr_cmt_op psr_cat_op set_avail_vcpus
};
allow dom0_t dom0_t:resource { add remove };
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index eb646f5..afbedc2 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -52,7 +52,7 @@ define(`create_domain_common', `
settime setdomainhandle getvcpucontext };
allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
set_max_evtchn set_vnumainfo get_vnumainfo cacheflush
- psr_cmt_op psr_cat_op soft_reset };
+ psr_cmt_op psr_cat_op soft_reset set_avail_vcpus};
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
@@ -85,7 +85,7 @@ define(`manage_domain', `
getaddrsize pause unpause trigger shutdown destroy
setaffinity setdomainmaxmem getscheduler resume
setpodtarget getpodtarget };
- allow $1 $2:domain2 set_vnumainfo;
+ allow $1 $2:domain2 { set_vnumainfo set_avail_vcpus };
')
# migrate_domain_out(priv, target)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2c83544..49e9b9f 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1256,6 +1256,11 @@ int xc_domain_getvnuma(xc_interface *xch,
int xc_domain_soft_reset(xc_interface *xch,
uint32_t domid);
+int xc_domain_set_avail_vcpus(xc_interface *xch,
+ uint32_t domid,
+ unsigned int num_vcpus);
+
+
#if defined(__i386__) || defined(__x86_64__)
/*
* PC BIOS standard E820 types and structure.
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0eab8a7..7fcdee1 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -125,6 +125,17 @@ const char *xc_domain_get_native_protocol(xc_interface *xch,
return protocol;
}
+int xc_domain_set_avail_vcpus(xc_interface *xch,
+ uint32_t domid,
+ unsigned int num_vcpus)
+{
+ DECLARE_DOMCTL;
+ domctl.cmd = XEN_DOMCTL_set_avail_vcpus;
+ domctl.domain = (domid_t)domid;
+ domctl.u.avail_vcpus.num = num_vcpus;
+ return do_domctl(xch, &domctl);
+}
+
static int count_pgtables(struct xc_dom_image *dom, xen_vaddr_t from,
xen_vaddr_t to, xen_pfn_t pfn)
{
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 33c5e4c..9b94413 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5148,11 +5148,12 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
case LIBXL_DOMAIN_TYPE_HVM:
switch (libxl__device_model_version_running(gc, domid)) {
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
- case LIBXL_DEVICE_MODEL_VERSION_NONE:
rc = libxl__set_vcpuonline_xenstore(gc, domid, cpumap, &info);
break;
case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
rc = libxl__set_vcpuonline_qmp(gc, domid, cpumap, &info);
+ /* fallthrough */
+ case LIBXL_DEVICE_MODEL_VERSION_NONE:
break;
default:
rc = ERROR_INVAL;
@@ -5164,6 +5165,13 @@ int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
default:
rc = ERROR_INVAL;
}
+
+ if (!rc) {
+ rc = libxl__arch_set_vcpuonline(ctx, domid, maxcpus);
+ if (rc)
+ LOG(ERROR, "Couldn't set available vcpu count");
+ }
+
out:
libxl_dominfo_dispose(&info);
GC_FREE;
diff --git a/tools/libxl/libxl_arch.h b/tools/libxl/libxl_arch.h
index 5e1fc60..33b5e12 100644
--- a/tools/libxl/libxl_arch.h
+++ b/tools/libxl/libxl_arch.h
@@ -71,6 +71,10 @@ int libxl__arch_extra_memory(libxl__gc *gc,
const libxl_domain_build_info *info,
uint64_t *out);
+_hidden
+int libxl__arch_set_vcpuonline(libxl_ctx *ctx, uint32_t domid,
+ unsigned int vcpu_num);
+
#if defined(__i386__) || defined(__x86_64__)
#define LAPIC_BASE_ADDRESS 0xfee00000
diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index d842d88..18c79b0 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -1065,6 +1065,12 @@ void libxl__arch_domain_build_info_acpi_setdefault(
libxl_defbool_setdefault(&b_info->acpi, false);
}
+int libxl__arch_set_vcpuonline(libxl_ctx *ctx, uint32_t domid,
+ unsigned int vcpu_num)
+{
+ return 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d519c8d..e6e94bb 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -309,6 +309,13 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
return ERROR_FAIL;
}
+ rc = libxl__arch_set_vcpuonline(ctx, domid,
+ libxl_bitmap_count_set(&info->avail_vcpus));
+ if (rc) {
+ LOG(ERROR, "Couldn't set available vcpu count (error %d)", rc);
+ return ERROR_FAIL;
+ }
+
/*
* Check if the domain has any CPU or node affinity already. If not, try
* to build up the latter via automatic NUMA placement. In fact, in case
diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
index e1844c8..3a5264d 100644
--- a/tools/libxl/libxl_x86.c
+++ b/tools/libxl/libxl_x86.c
@@ -588,6 +588,12 @@ void libxl__arch_domain_build_info_acpi_setdefault(
libxl_defbool_setdefault(&b_info->acpi, true);
}
+int libxl__arch_set_vcpuonline(libxl_ctx *ctx, uint32_t domid,
+ unsigned int vcpu_num)
+{
+ return xc_domain_set_avail_vcpus(ctx->xch, domid, vcpu_num);
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 2a2fe04..b736d4c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1430,6 +1430,23 @@ long arch_do_domctl(
}
break;
+ case XEN_DOMCTL_set_avail_vcpus:
+ {
+ unsigned int num = domctl->u.avail_vcpus.num;
+
+ /*
+ * This is currently only needed by PVH guests (but
+ * any guest is free to make this call).
+ */
+ ret = -EINVAL;
+ if ( num > d->max_vcpus )
+ break;
+
+ d->arch.avail_vcpus = num;
+ ret = 0;
+ break;
+ }
+
default:
ret = iommu_do_domctl(domctl, d, u_domctl);
break;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index f6a40eb..a279e4a 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -414,6 +414,12 @@ struct arch_domain
/* Emulated devices enabled bitmap. */
uint32_t emulation_flags;
+
+ /*
+ * Number of VCPUs that were online during guest creation
+ * plus/minus any hot-(un)plugged VCPUs.
+ */
+ unsigned int avail_vcpus;
} __cacheline_aligned;
#define has_vlapic(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC))
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 177319d..f114645 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1010,6 +1010,13 @@ struct xen_domctl_vcpu_msrs {
};
typedef struct xen_domctl_vcpu_msrs xen_domctl_vcpu_msrs_t;
DEFINE_XEN_GUEST_HANDLE(xen_domctl_vcpu_msrs_t);
+
+/* XEN_DOMCTL_avail_vcpus */
+struct xen_domctl_avail_vcpus {
+ uint32_t num; /* available number of vcpus */
+};
+typedef struct xen_domctl_avail_vcpus xen_domctl_avail_vcpus_t;
+DEFINE_XEN_GUEST_HANDLE(xen_domctl_avail_vcpus_t);
#endif
/* XEN_DOMCTL_setvnumainfo: specifies a virtual NUMA topology for the guest */
@@ -1221,6 +1228,7 @@ struct xen_domctl {
#define XEN_DOMCTL_monitor_op 77
#define XEN_DOMCTL_psr_cat_op 78
#define XEN_DOMCTL_soft_reset 79
+#define XEN_DOMCTL_set_avail_vcpus 80
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1269,6 +1277,7 @@ struct xen_domctl {
struct xen_domctl_cpuid cpuid;
struct xen_domctl_vcpuextstate vcpuextstate;
struct xen_domctl_vcpu_msrs vcpu_msrs;
+ struct xen_domctl_avail_vcpus avail_vcpus;
#endif
struct xen_domctl_set_access_required access_required;
struct xen_domctl_audit_p2m audit_p2m;
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 177c11f..377549a 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -748,6 +748,9 @@ static int flask_domctl(struct domain *d, int cmd)
case XEN_DOMCTL_soft_reset:
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET);
+ case XEN_DOMCTL_set_avail_vcpus:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SET_AVAIL_VCPUS);
+
default:
return avc_unknown_permission("domctl", cmd);
}
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 49c9a9e..f8a5e6c 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -244,6 +244,8 @@ class domain2
mem_sharing
# XEN_DOMCTL_psr_cat_op
psr_cat_op
+# XEN_DOMCTL_set_avail_vcpus
+ set_avail_vcpus
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 14:39 ` [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus Boris Ostrovsky
@ 2016-11-09 15:04 ` Andrew Cooper
2016-11-09 15:29 ` Boris Ostrovsky
2016-11-15 8:34 ` Jan Beulich
1 sibling, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-09 15:04 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 09/11/16 14:39, Boris Ostrovsky wrote:
> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
> 'xl vcpu-set'). While this currently is only intended to be needed by
> PVH guests we will call this domctl for all (x86) guests for consistency.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
> Changes in v2:
> * Added comment in arch_do_domctl() stating that the domctl is only required
> for PVH guests
I am not happy with this change until we understand why it is needed.
Are we genuinely saying that there is no current enforcement in the
PV-hotplug mechanism?
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 2a2fe04..b736d4c 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
> }
> break;
>
> + case XEN_DOMCTL_set_avail_vcpus:
> + {
> + unsigned int num = domctl->u.avail_vcpus.num;
> +
> + /*
> + * This is currently only needed by PVH guests (but
> + * any guest is free to make this call).
> + */
> + ret = -EINVAL;
> + if ( num > d->max_vcpus )
> + break;
> +
> + d->arch.avail_vcpus = num;
> + ret = 0;
> + break;
> + }
What do you actually mean by "avail_vcpus"? What happens if a vcpu
higher than the new number is currently online and running? What
happens to the already-existing vcpus-at-startup value?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 15:04 ` Andrew Cooper
@ 2016-11-09 15:29 ` Boris Ostrovsky
2016-11-09 19:23 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 15:29 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 11/09/2016 10:04 AM, Andrew Cooper wrote:
> On 09/11/16 14:39, Boris Ostrovsky wrote:
>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>> 'xl vcpu-set'). While this currently is only intended to be needed by
>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> ---
>> Changes in v2:
>> * Added comment in arch_do_domctl() stating that the domctl is only required
>> for PVH guests
> I am not happy with this change until we understand why it is needed.
>
> Are we genuinely saying that there is no current enforcement in the
> PV-hotplug mechanism?
That's right. Don't call setup_cpu_watcher() in Linux and you will be
running with maxvcpus.
>
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 2a2fe04..b736d4c 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>> }
>> break;
>>
>> + case XEN_DOMCTL_set_avail_vcpus:
>> + {
>> + unsigned int num = domctl->u.avail_vcpus.num;
>> +
>> + /*
>> + * This is currently only needed by PVH guests (but
>> + * any guest is free to make this call).
>> + */
>> + ret = -EINVAL;
>> + if ( num > d->max_vcpus )
>> + break;
>> +
>> + d->arch.avail_vcpus = num;
>> + ret = 0;
>> + break;
>> + }
> What do you actually mean by "avail_vcpus"? What happens if a vcpu
> higher than the new number is currently online and running? What
> happens to the already-existing vcpus-at-startup value?
It shouldn't happen: we set avail_vcpus at domain creation time to
vcpus-at-startup.
The name is not great. It would have been better to have it online_vcpus
but that usually means that VPF_down is set (which can happen, for
example, when the guest offlines a VCPU).
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 15:29 ` Boris Ostrovsky
@ 2016-11-09 19:23 ` Andrew Cooper
2016-11-09 19:47 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-09 19:23 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 09/11/16 15:29, Boris Ostrovsky wrote:
> On 11/09/2016 10:04 AM, Andrew Cooper wrote:
>> On 09/11/16 14:39, Boris Ostrovsky wrote:
>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>> ---
>>> Changes in v2:
>>> * Added comment in arch_do_domctl() stating that the domctl is only required
>>> for PVH guests
>> I am not happy with this change until we understand why it is needed.
>>
>> Are we genuinely saying that there is no current enforcement in the
>> PV-hotplug mechanism?
> That's right. Don't call setup_cpu_watcher() in Linux and you will be
> running with maxvcpus.
/sigh - Quality engineering there...
Yes - lets take the time to actually do this properly.
>
>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>> index 2a2fe04..b736d4c 100644
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>>> }
>>> break;
>>>
>>> + case XEN_DOMCTL_set_avail_vcpus:
>>> + {
>>> + unsigned int num = domctl->u.avail_vcpus.num;
>>> +
>>> + /*
>>> + * This is currently only needed by PVH guests (but
>>> + * any guest is free to make this call).
>>> + */
>>> + ret = -EINVAL;
>>> + if ( num > d->max_vcpus )
>>> + break;
>>> +
>>> + d->arch.avail_vcpus = num;
>>> + ret = 0;
>>> + break;
>>> + }
>> What do you actually mean by "avail_vcpus"? What happens if a vcpu
>> higher than the new number is currently online and running? What
>> happens to the already-existing vcpus-at-startup value?
> It shouldn't happen: we set avail_vcpus at domain creation time to
> vcpus-at-startup.
>
> The name is not great. It would have been better to have it online_vcpus
> but that usually means that VPF_down is set (which can happen, for
> example, when the guest offlines a VCPU).
How about an availability bitmap instead, which always has max_vcpus
bits in it? Xen should consult the bitmap before allowing a VM to
online a new vcpu.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 19:23 ` Andrew Cooper
@ 2016-11-09 19:47 ` Boris Ostrovsky
2016-11-14 14:59 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 19:47 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 11/09/2016 02:23 PM, Andrew Cooper wrote:
> On 09/11/16 15:29, Boris Ostrovsky wrote:
>> On 11/09/2016 10:04 AM, Andrew Cooper wrote:
>>> On 09/11/16 14:39, Boris Ostrovsky wrote:
>>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>> ---
>>>> Changes in v2:
>>>> * Added comment in arch_do_domctl() stating that the domctl is only required
>>>> for PVH guests
>>> I am not happy with this change until we understand why it is needed.
>>>
>>> Are we genuinely saying that there is no current enforcement in the
>>> PV-hotplug mechanism?
>> That's right. Don't call setup_cpu_watcher() in Linux and you will be
>> running with maxvcpus.
> /sigh - Quality engineering there...
>
> Yes - lets take the time to actually do this properly.
>
>>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>>> index 2a2fe04..b736d4c 100644
>>>> --- a/xen/arch/x86/domctl.c
>>>> +++ b/xen/arch/x86/domctl.c
>>>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>>>> }
>>>> break;
>>>>
>>>> + case XEN_DOMCTL_set_avail_vcpus:
>>>> + {
>>>> + unsigned int num = domctl->u.avail_vcpus.num;
>>>> +
>>>> + /*
>>>> + * This is currently only needed by PVH guests (but
>>>> + * any guest is free to make this call).
>>>> + */
>>>> + ret = -EINVAL;
>>>> + if ( num > d->max_vcpus )
>>>> + break;
>>>> +
>>>> + d->arch.avail_vcpus = num;
>>>> + ret = 0;
>>>> + break;
>>>> + }
>>> What do you actually mean by "avail_vcpus"? What happens if a vcpu
>>> higher than the new number is currently online and running? What
>>> happens to the already-existing vcpus-at-startup value?
>> It shouldn't happen: we set avail_vcpus at domain creation time to
>> vcpus-at-startup.
>>
>> The name is not great. It would have been better to have it online_vcpus
>> but that usually means that VPF_down is set (which can happen, for
>> example, when the guest offlines a VCPU).
> How about an availability bitmap instead, which always has max_vcpus
> bits in it? Xen should consult the bitmap before allowing a VM to
> online a new vcpu.
We could indeed use bitmap (and then it will actually be easier to
handle io request as we can just copy appropriate bytes of the map
instead of going bit-by-bit). This will still require the new domctl.
I am not convinced though that we can start enforcing this new VCPU
count, at least for PV guests. They expect to start all max VCPUs and
then offline them. This, of course, can be fixed but all non-updated
kernels will stop booting.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 19:47 ` Boris Ostrovsky
@ 2016-11-14 14:59 ` Boris Ostrovsky
2016-11-14 17:17 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-14 14:59 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: wei.liu2, ian.jackson, jbeulich, roger.pau
On 11/09/2016 02:47 PM, Boris Ostrovsky wrote:
> On 11/09/2016 02:23 PM, Andrew Cooper wrote:
>> On 09/11/16 15:29, Boris Ostrovsky wrote:
>>> On 11/09/2016 10:04 AM, Andrew Cooper wrote:
>>>> On 09/11/16 14:39, Boris Ostrovsky wrote:
>>>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>>>>
>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>>> ---
>>>>> Changes in v2:
>>>>> * Added comment in arch_do_domctl() stating that the domctl is only required
>>>>> for PVH guests
>>>> I am not happy with this change until we understand why it is needed.
>>>>
>>>> Are we genuinely saying that there is no current enforcement in the
>>>> PV-hotplug mechanism?
>>> That's right. Don't call setup_cpu_watcher() in Linux and you will be
>>> running with maxvcpus.
>> /sigh - Quality engineering there...
>>
>> Yes - lets take the time to actually do this properly.
>>
>>>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>>>> index 2a2fe04..b736d4c 100644
>>>>> --- a/xen/arch/x86/domctl.c
>>>>> +++ b/xen/arch/x86/domctl.c
>>>>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>>>>> }
>>>>> break;
>>>>>
>>>>> + case XEN_DOMCTL_set_avail_vcpus:
>>>>> + {
>>>>> + unsigned int num = domctl->u.avail_vcpus.num;
>>>>> +
>>>>> + /*
>>>>> + * This is currently only needed by PVH guests (but
>>>>> + * any guest is free to make this call).
>>>>> + */
>>>>> + ret = -EINVAL;
>>>>> + if ( num > d->max_vcpus )
>>>>> + break;
>>>>> +
>>>>> + d->arch.avail_vcpus = num;
>>>>> + ret = 0;
>>>>> + break;
>>>>> + }
>>>> What do you actually mean by "avail_vcpus"? What happens if a vcpu
>>>> higher than the new number is currently online and running? What
>>>> happens to the already-existing vcpus-at-startup value?
>>> It shouldn't happen: we set avail_vcpus at domain creation time to
>>> vcpus-at-startup.
>>>
>>> The name is not great. It would have been better to have it online_vcpus
>>> but that usually means that VPF_down is set (which can happen, for
>>> example, when the guest offlines a VCPU).
>> How about an availability bitmap instead, which always has max_vcpus
>> bits in it? Xen should consult the bitmap before allowing a VM to
>> online a new vcpu.
> We could indeed use bitmap (and then it will actually be easier to
> handle io request as we can just copy appropriate bytes of the map
> instead of going bit-by-bit). This will still require the new domctl.
>
> I am not convinced though that we can start enforcing this new VCPU
> count, at least for PV guests. They expect to start all max VCPUs and
> then offline them. This, of course, can be fixed but all non-updated
> kernels will stop booting.
How about we don't clear _VPF_down if the bit in the availability bitmap
is not set?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-14 14:59 ` Boris Ostrovsky
@ 2016-11-14 17:17 ` Andrew Cooper
2016-11-14 17:48 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-14 17:17 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: wei.liu2, ian.jackson, jbeulich, roger.pau
On 14/11/16 14:59, Boris Ostrovsky wrote:
> On 11/09/2016 02:47 PM, Boris Ostrovsky wrote:
>> On 11/09/2016 02:23 PM, Andrew Cooper wrote:
>>> On 09/11/16 15:29, Boris Ostrovsky wrote:
>>>> On 11/09/2016 10:04 AM, Andrew Cooper wrote:
>>>>> On 09/11/16 14:39, Boris Ostrovsky wrote:
>>>>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>>>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>>>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>>>>>
>>>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>>>>> Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>>>>>> ---
>>>>>> Changes in v2:
>>>>>> * Added comment in arch_do_domctl() stating that the domctl is only required
>>>>>> for PVH guests
>>>>> I am not happy with this change until we understand why it is needed.
>>>>>
>>>>> Are we genuinely saying that there is no current enforcement in the
>>>>> PV-hotplug mechanism?
>>>> That's right. Don't call setup_cpu_watcher() in Linux and you will be
>>>> running with maxvcpus.
>>> /sigh - Quality engineering there...
>>>
>>> Yes - lets take the time to actually do this properly.
>>>
>>>>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>>>>> index 2a2fe04..b736d4c 100644
>>>>>> --- a/xen/arch/x86/domctl.c
>>>>>> +++ b/xen/arch/x86/domctl.c
>>>>>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>>>>>> }
>>>>>> break;
>>>>>>
>>>>>> + case XEN_DOMCTL_set_avail_vcpus:
>>>>>> + {
>>>>>> + unsigned int num = domctl->u.avail_vcpus.num;
>>>>>> +
>>>>>> + /*
>>>>>> + * This is currently only needed by PVH guests (but
>>>>>> + * any guest is free to make this call).
>>>>>> + */
>>>>>> + ret = -EINVAL;
>>>>>> + if ( num > d->max_vcpus )
>>>>>> + break;
>>>>>> +
>>>>>> + d->arch.avail_vcpus = num;
>>>>>> + ret = 0;
>>>>>> + break;
>>>>>> + }
>>>>> What do you actually mean by "avail_vcpus"? What happens if a vcpu
>>>>> higher than the new number is currently online and running? What
>>>>> happens to the already-existing vcpus-at-startup value?
>>>> It shouldn't happen: we set avail_vcpus at domain creation time to
>>>> vcpus-at-startup.
>>>>
>>>> The name is not great. It would have been better to have it online_vcpus
>>>> but that usually means that VPF_down is set (which can happen, for
>>>> example, when the guest offlines a VCPU).
>>> How about an availability bitmap instead, which always has max_vcpus
>>> bits in it? Xen should consult the bitmap before allowing a VM to
>>> online a new vcpu.
>> We could indeed use bitmap (and then it will actually be easier to
>> handle io request as we can just copy appropriate bytes of the map
>> instead of going bit-by-bit). This will still require the new domctl.
Given the lack of any enforcing mechanism, introducing one is clearly
needed. Please mention so in the commit message.
>>
>> I am not convinced though that we can start enforcing this new VCPU
>> count, at least for PV guests. They expect to start all max VCPUs and
>> then offline them. This, of course, can be fixed but all non-updated
>> kernels will stop booting.
>
> How about we don't clear _VPF_down if the bit in the availability bitmap
> is not set?
This is yet another PV mess. We clearly need to quirk PV guests as the
exception to sanity, given that they expect (and have been able to)
online all cpus at start-of-day.
To avoid race conditions, you necessarily need to be able to set a
reduced permitted map before asking the VM to unplug.
For HVM guests, we can set a proper permitted map at boot, and really
should do so.
For PV guests, we have to wait until it has completed its SMP bringup
before reducing the permitted set. Therefore, making the initial
set_avail_vcpus call could be deferred until the first unplug request?
It also occurs to me that you necessarily need a get_avail_vcpus
hypercall to be able to use this interface sensibly from the toolstack.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-14 17:17 ` Andrew Cooper
@ 2016-11-14 17:48 ` Boris Ostrovsky
2016-11-14 18:19 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-14 17:48 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: wei.liu2, ian.jackson, jbeulich, roger.pau
On 11/14/2016 12:17 PM, Andrew Cooper wrote:
>
>>> I am not convinced though that we can start enforcing this new VCPU
>>> count, at least for PV guests. They expect to start all max VCPUs and
>>> then offline them. This, of course, can be fixed but all non-updated
>>> kernels will stop booting.
>> How about we don't clear _VPF_down if the bit in the availability bitmap
>> is not set?
> This is yet another PV mess. We clearly need to quirk PV guests as the
> exception to sanity, given that they expect (and have been able to)
> online all cpus at start-of-day.
>
> To avoid race conditions, you necessarily need to be able to set a
> reduced permitted map before asking the VM to unplug.
>
> For HVM guests, we can set a proper permitted map at boot, and really
> should do so.
>
> For PV guests, we have to wait until it has completed its SMP bringup
> before reducing the permitted set. Therefore, making the initial
> set_avail_vcpus call could be deferred until the first unplug request?
I am not sure how we can determine in the hypervisor that a guest has
completed the bringup: I don't think we can rely on the last VCPU (which
is maxvcpu-1) doing VCPUOP_up. Just to mess up with the hypervisor the
guest may decide to only bring up (maxvcpus-2) VCPUs. In other words, we
can't assume a well-behaved guest.
And then, even if we do determine the point when (maxvcpus-1) VCPUs are
all up, when do we clamp them down to avail_vcpus? For the same reason,
we can't assume that the guest will VCPUOP_down all extra VCPUs.
>
> It also occurs to me that you necessarily need a get_avail_vcpus
> hypercall to be able to use this interface sensibly from the toolstack.
We could modify getdomaininfo but that would make set_avail_vcpus domctl
non-symmetrical.
And what would the use of this information be anyway?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-14 17:48 ` Boris Ostrovsky
@ 2016-11-14 18:19 ` Andrew Cooper
2016-11-14 18:44 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-14 18:19 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: wei.liu2, ian.jackson, jbeulich, roger.pau
On 14/11/16 17:48, Boris Ostrovsky wrote:
> On 11/14/2016 12:17 PM, Andrew Cooper wrote:
>>>> I am not convinced though that we can start enforcing this new VCPU
>>>> count, at least for PV guests. They expect to start all max VCPUs and
>>>> then offline them. This, of course, can be fixed but all non-updated
>>>> kernels will stop booting.
>>> How about we don't clear _VPF_down if the bit in the availability bitmap
>>> is not set?
>> This is yet another PV mess. We clearly need to quirk PV guests as the
>> exception to sanity, given that they expect (and have been able to)
>> online all cpus at start-of-day.
>>
>> To avoid race conditions, you necessarily need to be able to set a
>> reduced permitted map before asking the VM to unplug.
>>
>> For HVM guests, we can set a proper permitted map at boot, and really
>> should do so.
>>
>> For PV guests, we have to wait until it has completed its SMP bringup
>> before reducing the permitted set. Therefore, making the initial
>> set_avail_vcpus call could be deferred until the first unplug request?
> I am not sure how we can determine in the hypervisor that a guest has
> completed the bringup: I don't think we can rely on the last VCPU (which
> is maxvcpu-1) doing VCPUOP_up. Just to mess up with the hypervisor the
> guest may decide to only bring up (maxvcpus-2) VCPUs. In other words, we
> can't assume a well-behaved guest.
I wasn't suggesting relying on the guest. I was referring to the first
unplug request at the toolstack level.
> And then, even if we do determine the point when (maxvcpus-1) VCPUs are
> all up, when do we clamp them down to avail_vcpus? For the same reason,
> we can't assume that the guest will VCPUOP_down all extra VCPUs.
If at some point we observe all vcpus being up, then we could set the
restricted map then. However, I can't think of a useful way of
identifying this point.
>> It also occurs to me that you necessarily need a get_avail_vcpus
>> hypercall to be able to use this interface sensibly from the toolstack.
> We could modify getdomaininfo but that would make set_avail_vcpus domctl
> non-symmetrical.
>
> And what would the use of this information be anyway?
Well, for a start, this information needs to move in the migration
stream, or by migrating a VM you will lose its current availability
bitmap and reintroduce the problem we are trying to solve.
>
> -boris
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-14 18:19 ` Andrew Cooper
@ 2016-11-14 18:44 ` Boris Ostrovsky
2016-11-15 16:41 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-14 18:44 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: wei.liu2, ian.jackson, jbeulich, roger.pau
On 11/14/2016 01:19 PM, Andrew Cooper wrote:
> On 14/11/16 17:48, Boris Ostrovsky wrote:
>> On 11/14/2016 12:17 PM, Andrew Cooper wrote:
>>>>> I am not convinced though that we can start enforcing this new VCPU
>>>>> count, at least for PV guests. They expect to start all max VCPUs and
>>>>> then offline them. This, of course, can be fixed but all non-updated
>>>>> kernels will stop booting.
>>>> How about we don't clear _VPF_down if the bit in the availability bitmap
>>>> is not set?
>>> This is yet another PV mess. We clearly need to quirk PV guests as the
>>> exception to sanity, given that they expect (and have been able to)
>>> online all cpus at start-of-day.
>>>
>>> To avoid race conditions, you necessarily need to be able to set a
>>> reduced permitted map before asking the VM to unplug.
>>>
>>> For HVM guests, we can set a proper permitted map at boot, and really
>>> should do so.
>>>
>>> For PV guests, we have to wait until it has completed its SMP bringup
>>> before reducing the permitted set. Therefore, making the initial
>>> set_avail_vcpus call could be deferred until the first unplug request?
>> I am not sure how we can determine in the hypervisor that a guest has
>> completed the bringup: I don't think we can rely on the last VCPU (which
>> is maxvcpu-1) doing VCPUOP_up. Just to mess up with the hypervisor the
>> guest may decide to only bring up (maxvcpus-2) VCPUs. In other words, we
>> can't assume a well-behaved guest.
> I wasn't suggesting relying on the guest. I was referring to the first
> unplug request at the toolstack level.
I don't think waiting for toolstack's (un)plug request is going to help
much --- the request may never come and the guest will be able to use
all maxvcpus.
>
>> And then, even if we do determine the point when (maxvcpus-1) VCPUs are
>> all up, when do we clamp them down to avail_vcpus? For the same reason,
>> we can't assume that the guest will VCPUOP_down all extra VCPUs.
> If at some point we observe all vcpus being up, then we could set the
> restricted map then. However, I can't think of a useful way of
> identifying this point.
Exactly.
The question is then, if we can't do this for PV, should we still do it
for HVM?
>
>>> It also occurs to me that you necessarily need a get_avail_vcpus
>>> hypercall to be able to use this interface sensibly from the toolstack.
>> We could modify getdomaininfo but that would make set_avail_vcpus domctl
>> non-symmetrical.
>>
>> And what would the use of this information be anyway?
> Well, for a start, this information needs to move in the migration
> stream, or by migrating a VM you will lose its current availability
> bitmap and reintroduce the problem we are trying to solve.
Oh, right, of course.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-14 18:44 ` Boris Ostrovsky
@ 2016-11-15 16:41 ` Andrew Cooper
2016-11-15 17:04 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-15 16:41 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 14/11/16 18:44, Boris Ostrovsky wrote:
> On 11/14/2016 01:19 PM, Andrew Cooper wrote:
>> On 14/11/16 17:48, Boris Ostrovsky wrote:
>>> On 11/14/2016 12:17 PM, Andrew Cooper wrote:
>>>>>> I am not convinced though that we can start enforcing this new VCPU
>>>>>> count, at least for PV guests. They expect to start all max VCPUs and
>>>>>> then offline them. This, of course, can be fixed but all non-updated
>>>>>> kernels will stop booting.
>>>>> How about we don't clear _VPF_down if the bit in the availability bitmap
>>>>> is not set?
>>>> This is yet another PV mess. We clearly need to quirk PV guests as the
>>>> exception to sanity, given that they expect (and have been able to)
>>>> online all cpus at start-of-day.
>>>>
>>>> To avoid race conditions, you necessarily need to be able to set a
>>>> reduced permitted map before asking the VM to unplug.
>>>>
>>>> For HVM guests, we can set a proper permitted map at boot, and really
>>>> should do so.
>>>>
>>>> For PV guests, we have to wait until it has completed its SMP bringup
>>>> before reducing the permitted set. Therefore, making the initial
>>>> set_avail_vcpus call could be deferred until the first unplug request?
>>> I am not sure how we can determine in the hypervisor that a guest has
>>> completed the bringup: I don't think we can rely on the last VCPU (which
>>> is maxvcpu-1) doing VCPUOP_up. Just to mess up with the hypervisor the
>>> guest may decide to only bring up (maxvcpus-2) VCPUs. In other words, we
>>> can't assume a well-behaved guest.
>> I wasn't suggesting relying on the guest. I was referring to the first
>> unplug request at the toolstack level.
> I don't think waiting for toolstack's (un)plug request is going to help
> much --- the request may never come and the guest will be able to use
> all maxvcpus.
How does this currently work for PV guests, assuming there is no
explicit user input beyond the what was written in the domain
configuraiton file?
>
>
>>> And then, even if we do determine the point when (maxvcpus-1) VCPUs are
>>> all up, when do we clamp them down to avail_vcpus? For the same reason,
>>> we can't assume that the guest will VCPUOP_down all extra VCPUs.
>> If at some point we observe all vcpus being up, then we could set the
>> restricted map then. However, I can't think of a useful way of
>> identifying this point.
> Exactly.
>
> The question is then, if we can't do this for PV, should we still do it
> for HVM?
Absolutely. It is embarrassing that there isn't enforcement for PV.
Getting extra vcpu power for a short while is something that you
typically buy from a cloud provider.
>
>>>> It also occurs to me that you necessarily need a get_avail_vcpus
>>>> hypercall to be able to use this interface sensibly from the toolstack.
>>> We could modify getdomaininfo but that would make set_avail_vcpus domctl
>>> non-symmetrical.
>>>
>>> And what would the use of this information be anyway?
>> Well, for a start, this information needs to move in the migration
>> stream, or by migrating a VM you will lose its current availability
>> bitmap and reintroduce the problem we are trying to solve.
> Oh, right, of course.
Everyone forgets migrate.
When we eventually get a checklist for new features, migration will
definitely be on there somewhere.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-15 16:41 ` Andrew Cooper
@ 2016-11-15 17:04 ` Boris Ostrovsky
2016-11-15 17:30 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 17:04 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 11/15/2016 11:41 AM, Andrew Cooper wrote:
>
>>>>> It also occurs to me that you necessarily need a get_avail_vcpus
>>>>> hypercall to be able to use this interface sensibly from the toolstack.
>>>> We could modify getdomaininfo but that would make set_avail_vcpus domctl
>>>> non-symmetrical.
>>>>
>>>> And what would the use of this information be anyway?
>>> Well, for a start, this information needs to move in the migration
>>> stream, or by migrating a VM you will lose its current availability
>>> bitmap and reintroduce the problem we are trying to solve.
>> Oh, right, of course.
> Everyone forgets migrate.
(Or nested virt.)
As I was poking at this yesterday I realized that we may not need the
get interface ---- this information is available in xenstore via
cpu/available node (well, it actually doesn't fully function for HVM
guests but I think it should be fixed).
But if you still feel we should have it I'd rather add another field to
XEN_DOMCTL_getdomaininfo.
-boris
>
> When we eventually get a checklist for new features, migration will
> definitely be on there somewhere.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-15 17:04 ` Boris Ostrovsky
@ 2016-11-15 17:30 ` Andrew Cooper
0 siblings, 0 replies; 76+ messages in thread
From: Andrew Cooper @ 2016-11-15 17:30 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel; +Cc: ian.jackson, wei.liu2, jbeulich, roger.pau
On 15/11/16 17:04, Boris Ostrovsky wrote:
> On 11/15/2016 11:41 AM, Andrew Cooper wrote:
>>>>>> It also occurs to me that you necessarily need a get_avail_vcpus
>>>>>> hypercall to be able to use this interface sensibly from the toolstack.
>>>>> We could modify getdomaininfo but that would make set_avail_vcpus domctl
>>>>> non-symmetrical.
>>>>>
>>>>> And what would the use of this information be anyway?
>>>> Well, for a start, this information needs to move in the migration
>>>> stream, or by migrating a VM you will lose its current availability
>>>> bitmap and reintroduce the problem we are trying to solve.
>>> Oh, right, of course.
>> Everyone forgets migrate.
> (Or nested virt.)
or vnuma, PoD, ballooning. All examples of features which fail or blow
up spectacularly when combined with migration.
>
> As I was poking at this yesterday I realized that we may not need the
> get interface ---- this information is available in xenstore via
> cpu/available node (well, it actually doesn't fully function for HVM
> guests but I think it should be fixed).
Xenstore data isn't propagated on migrate, nor should it start to be.
(This isn't quite true. There is currently one piece of data which is,
because of a Qemu hackary, which needs to be dropped.)
>
> But if you still feel we should have it I'd rather add another field to
> XEN_DOMCTL_getdomaininfo.
There are two points to consider.
1) Where should the information live in an ideal migration stream which
includes hypervisor migration-v2.
As this is being implemented by a new piece of emulation in Xen, there
should be a new Xen record in the stream.
2) How are utility functions like `xl vcpu-info` going to find this
information out.
This just requires the information being available somehow in a
hypercall. Extending XEN_DOMCTL_getdomaininfo would be fine for this
purpose.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-09 14:39 ` [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus Boris Ostrovsky
2016-11-09 15:04 ` Andrew Cooper
@ 2016-11-15 8:34 ` Jan Beulich
2016-11-15 14:28 ` Boris Ostrovsky
1 sibling, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 8:34 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
> 'xl vcpu-set'). While this currently is only intended to be needed by
> PVH guests we will call this domctl for all (x86) guests for consistency.
The discussion on the actual change seems to have pointed out all
needs of change, but what I wasn't able to understand yet is why
this is needed in the first place. From hypervisor pov, so far it's been
up to the guest which CPUs get onlined/offlined, and the interface
to request offlining (not an issue for onlining) was - afaict - a purely
voluntary one. Why does this change with PVH? Any such ratonale
should be put in the commit message.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-15 8:34 ` Jan Beulich
@ 2016-11-15 14:28 ` Boris Ostrovsky
2016-11-15 14:33 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 14:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
On 11/15/2016 03:34 AM, Jan Beulich wrote:
>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>> 'xl vcpu-set'). While this currently is only intended to be needed by
>> PVH guests we will call this domctl for all (x86) guests for consistency.
> The discussion on the actual change seems to have pointed out all
> needs of change, but what I wasn't able to understand yet is why
> this is needed in the first place. From hypervisor pov, so far it's been
> up to the guest which CPUs get onlined/offlined, and the interface
> to request offlining (not an issue for onlining) was - afaict - a purely
> voluntary one. Why does this change with PVH? Any such ratonale
> should be put in the commit message.
If the question is why we need to have hypervisor interface for PVH
guests then it's because we need someone to send an SCI and set GPE
registers and there is noone but the hypervisor to do that for PVH (I
will add it to the commit message).
As for whether we want to enforce available VCPU count --- I think we
decided that we can't do this for PV and so the question is whether it's
worth doing only for some types of guests. And as you pointed out the
second question (or may be the first) is whether enforcing it is the
right thing in the first place.
(BTW, I am thinking to move the domctl from x86-specific to common code
since if we are no longer saying that it's PVH-only then ARM should have
it available too)
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-15 14:28 ` Boris Ostrovsky
@ 2016-11-15 14:33 ` Jan Beulich
2016-11-15 15:00 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 14:33 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 15.11.16 at 15:28, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 03:34 AM, Jan Beulich wrote:
>>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>> The discussion on the actual change seems to have pointed out all
>> needs of change, but what I wasn't able to understand yet is why
>> this is needed in the first place. From hypervisor pov, so far it's been
>> up to the guest which CPUs get onlined/offlined, and the interface
>> to request offlining (not an issue for onlining) was - afaict - a purely
>> voluntary one. Why does this change with PVH? Any such ratonale
>> should be put in the commit message.
>
> If the question is why we need to have hypervisor interface for PVH
> guests then it's because we need someone to send an SCI and set GPE
> registers and there is noone but the hypervisor to do that for PVH (I
> will add it to the commit message).
Yes, that was the primary question. And it took me until quite late
in the series until I've seen the purpose, so I appreciate you
extending the description, even if just slightly.
> As for whether we want to enforce available VCPU count --- I think we
> decided that we can't do this for PV and so the question is whether it's
> worth doing only for some types of guests. And as you pointed out the
> second question (or may be the first) is whether enforcing it is the
> right thing in the first place.
>
> (BTW, I am thinking to move the domctl from x86-specific to common code
> since if we are no longer saying that it's PVH-only then ARM should have
> it available too)
Well, without us wanting to enforce anything, would we still need the
domctl for anything?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
2016-11-15 14:33 ` Jan Beulich
@ 2016-11-15 15:00 ` Boris Ostrovsky
0 siblings, 0 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 15:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
On 11/15/2016 09:33 AM, Jan Beulich wrote:
>>>> On 15.11.16 at 15:28, <boris.ostrovsky@oracle.com> wrote:
>> On 11/15/2016 03:34 AM, Jan Beulich wrote:
>>>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>> The discussion on the actual change seems to have pointed out all
>>> needs of change, but what I wasn't able to understand yet is why
>>> this is needed in the first place. From hypervisor pov, so far it's been
>>> up to the guest which CPUs get onlined/offlined, and the interface
>>> to request offlining (not an issue for onlining) was - afaict - a purely
>>> voluntary one. Why does this change with PVH? Any such ratonale
>>> should be put in the commit message.
>> If the question is why we need to have hypervisor interface for PVH
>> guests then it's because we need someone to send an SCI and set GPE
>> registers and there is noone but the hypervisor to do that for PVH (I
>> will add it to the commit message).
> Yes, that was the primary question. And it took me until quite late
> in the series until I've seen the purpose, so I appreciate you
> extending the description, even if just slightly.
>
>> As for whether we want to enforce available VCPU count --- I think we
>> decided that we can't do this for PV and so the question is whether it's
>> worth doing only for some types of guests. And as you pointed out the
>> second question (or may be the first) is whether enforcing it is the
>> right thing in the first place.
>>
>> (BTW, I am thinking to move the domctl from x86-specific to common code
>> since if we are no longer saying that it's PVH-only then ARM should have
>> it available too)
> Well, without us wanting to enforce anything, would we still need the
> domctl for anything?
While we can send an SCI directly from the toolstack we need to set the
GPE registers for the guest and that's what domctl's primary use is (was).
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
2016-11-09 14:39 ` [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-09 14:48 ` Julien Grall
` (3 more replies)
2016-11-09 14:39 ` [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus Boris Ostrovsky
` (8 subsequent siblings)
10 siblings, 4 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, Julien Grall, Paul Durrant,
jbeulich, Boris Ostrovsky, roger.pau
ACPI hotplug-related IO accesses (to GPE0 block) are handled
by qemu for HVM guests. Since PVH guests don't have qemu these
accesses will need to be procesed by the hypervisor.
Because ACPI event model expects pm1a block to be present we
need to have the hypervisor emulate it as well.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
CC: Julien Grall <julien.grall@arm.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
Changes in v2:
* Added public macros for ACPI CPU bitmap (at 0xaf00). Note: PRST
region shrinks from 32 bytes to 16 (when 128 VCPUs are
supported).
tools/libacpi/mk_dsdt.c | 4 +++-
tools/libacpi/static_tables.c | 28 +++++++++++-----------------
xen/include/asm-x86/hvm/domain.h | 6 ++++++
xen/include/public/arch-arm.h | 11 ++++++++---
xen/include/public/hvm/ioreq.h | 13 +++++++++++++
5 files changed, 41 insertions(+), 21 deletions(-)
diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 4ae68bc..2b8234d 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -19,6 +19,7 @@
#include <stdbool.h>
#if defined(__i386__) || defined(__x86_64__)
#include <xen/hvm/hvm_info_table.h>
+#include <xen/hvm/ioreq.h>
#elif defined(__aarch64__)
#include <xen/arch-arm.h>
#endif
@@ -244,7 +245,8 @@ int main(int argc, char **argv)
#endif
/* Operation Region 'PRST': bitmask of online CPUs. */
- stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
+ stmt("OperationRegion", "PRST, SystemIO, 0x%x, %d",
+ ACPI_CPU_MAP, ACPI_CPU_MAP_LEN);
push_block("Field", "PRST, ByteAcc, NoLock, Preserve");
indent(); printf("PRS, %u\n", max_cpus);
pop_block();
diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
index 617bf68..413abcc 100644
--- a/tools/libacpi/static_tables.c
+++ b/tools/libacpi/static_tables.c
@@ -20,6 +20,8 @@
* Firmware ACPI Control Structure (FACS).
*/
+#define ACPI_REG_BIT_OFFSET 0
+
struct acpi_20_facs Facs = {
.signature = ACPI_2_0_FACS_SIGNATURE,
.length = sizeof(struct acpi_20_facs),
@@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
/*
* Fixed ACPI Description Table (FADT).
*/
-
-#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
-#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
-#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
-#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
-#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
-#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
-
struct acpi_20_fadt Fadt = {
.header = {
.signature = ACPI_2_0_FADT_SIGNATURE,
@@ -56,9 +50,9 @@ struct acpi_20_fadt Fadt = {
.pm1a_cnt_blk = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
.pm_tmr_blk = ACPI_PM_TMR_BLK_ADDRESS_V1,
.gpe0_blk = ACPI_GPE0_BLK_ADDRESS_V1,
- .pm1_evt_len = ACPI_PM1A_EVT_BLK_BIT_WIDTH / 8,
- .pm1_cnt_len = ACPI_PM1A_CNT_BLK_BIT_WIDTH / 8,
- .pm_tmr_len = ACPI_PM_TMR_BLK_BIT_WIDTH / 8,
+ .pm1_evt_len = ACPI_PM1A_EVT_BLK_LEN,
+ .pm1_cnt_len = ACPI_PM1A_CNT_BLK_LEN,
+ .pm_tmr_len = ACPI_PM_TMR_BLK_LEN,
.gpe0_blk_len = ACPI_GPE0_BLK_LEN_V1,
.p_lvl2_lat = 0x0fff, /* >100, means we do not support C2 state */
@@ -79,22 +73,22 @@ struct acpi_20_fadt Fadt = {
.x_pm1a_evt_blk = {
.address_space_id = ACPI_SYSTEM_IO,
- .register_bit_width = ACPI_PM1A_EVT_BLK_BIT_WIDTH,
- .register_bit_offset = ACPI_PM1A_EVT_BLK_BIT_OFFSET,
+ .register_bit_width = ACPI_PM1A_EVT_BLK_LEN * 8,
+ .register_bit_offset = ACPI_REG_BIT_OFFSET,
.address = ACPI_PM1A_EVT_BLK_ADDRESS_V1,
},
.x_pm1a_cnt_blk = {
.address_space_id = ACPI_SYSTEM_IO,
- .register_bit_width = ACPI_PM1A_CNT_BLK_BIT_WIDTH,
- .register_bit_offset = ACPI_PM1A_CNT_BLK_BIT_OFFSET,
+ .register_bit_width = ACPI_PM1A_CNT_BLK_LEN * 8,
+ .register_bit_offset = ACPI_REG_BIT_OFFSET,
.address = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
},
.x_pm_tmr_blk = {
.address_space_id = ACPI_SYSTEM_IO,
- .register_bit_width = ACPI_PM_TMR_BLK_BIT_WIDTH,
- .register_bit_offset = ACPI_PM_TMR_BLK_BIT_OFFSET,
+ .register_bit_width = ACPI_PM_TMR_BLK_LEN * 8,
+ .register_bit_offset = ACPI_REG_BIT_OFFSET,
.address = ACPI_PM_TMR_BLK_ADDRESS_V1,
}
};
diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
index f34d784..f492a2b 100644
--- a/xen/include/asm-x86/hvm/domain.h
+++ b/xen/include/asm-x86/hvm/domain.h
@@ -87,6 +87,12 @@ struct hvm_domain {
} ioreq_server;
struct hvm_ioreq_server *default_ioreq_server;
+ /* PVH guests */
+ struct {
+ uint8_t pm1a[ACPI_PM1A_EVT_BLK_LEN];
+ uint8_t gpe[ACPI_GPE0_BLK_LEN_V1];
+ } acpi_io;
+
/* Cached CF8 for guest PCI config cycles */
uint32_t pci_cf8;
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index bd974fb..b793774 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -383,6 +383,9 @@ typedef uint64_t xen_callback_t;
* should instead use the FDT.
*/
+/* Current supported guest VCPUs */
+#define GUEST_MAX_VCPUS 128
+
/* Physical Address Space */
/*
@@ -410,6 +413,11 @@ typedef uint64_t xen_callback_t;
#define GUEST_ACPI_BASE 0x20000000ULL
#define GUEST_ACPI_SIZE 0x02000000ULL
+/* Location of online VCPU bitmap. */
+#define ACPI_CPU_MAP 0xaf00
+#define ACPI_CPU_MAP_LEN ((GUEST_MAX_VCPUS / 8) + \
+ ((GUEST_MAX_VCPUS & 7) ? 1 : 0))
+
/*
* 16MB == 4096 pages reserved for guest to use as a region to map its
* grant table in.
@@ -435,9 +443,6 @@ typedef uint64_t xen_callback_t;
#define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
#define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
-/* Current supported guest VCPUs */
-#define GUEST_MAX_VCPUS 128
-
/* Interrupts */
#define GUEST_TIMER_VIRT_PPI 27
#define GUEST_TIMER_PHYS_S_PPI 29
diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
index 2e5809b..e3fa704 100644
--- a/xen/include/public/hvm/ioreq.h
+++ b/xen/include/public/hvm/ioreq.h
@@ -24,6 +24,8 @@
#ifndef _IOREQ_H_
#define _IOREQ_H_
+#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
+
#define IOREQ_READ 1
#define IOREQ_WRITE 0
@@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
#define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
#define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
+#define ACPI_PM1A_EVT_BLK_LEN 0x04
+#define ACPI_PM1A_CNT_BLK_LEN 0x02
+#define ACPI_PM_TMR_BLK_LEN 0x04
+
+/* Location of online VCPU bitmap. */
+#define ACPI_CPU_MAP 0xaf00
+#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
+ ((HVM_MAX_VCPUS & 7) ? 1 : 0))
+#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
+#error "ACPI_CPU_MAP is too big"
+#endif
#endif /* _IOREQ_H_ */
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
@ 2016-11-09 14:48 ` Julien Grall
2016-11-09 14:54 ` Boris Ostrovsky
2016-11-09 14:48 ` Paul Durrant
` (2 subsequent siblings)
3 siblings, 1 reply; 76+ messages in thread
From: Julien Grall @ 2016-11-09 14:48 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, Paul Durrant, jbeulich,
roger.pau
Hi Boris,
On 09/11/16 14:39, Boris Ostrovsky wrote:
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..b793774 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -383,6 +383,9 @@ typedef uint64_t xen_callback_t;
> * should instead use the FDT.
> */
>
> +/* Current supported guest VCPUs */
> +#define GUEST_MAX_VCPUS 128
> +
> /* Physical Address Space */
>
> /*
> @@ -410,6 +413,11 @@ typedef uint64_t xen_callback_t;
> #define GUEST_ACPI_BASE 0x20000000ULL
> #define GUEST_ACPI_SIZE 0x02000000ULL
>
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((GUEST_MAX_VCPUS / 8) + \
> + ((GUEST_MAX_VCPUS & 7) ? 1 : 0))
> +
I don't understand this change. PRST is not generated for ARM (see the
return 0 in mk_dsdt a bit before).
In any case, I am expecting CPU hotplug to be handle via PSCI on ARM.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:48 ` Julien Grall
@ 2016-11-09 14:54 ` Boris Ostrovsky
0 siblings, 0 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:54 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, Paul Durrant, jbeulich,
roger.pau
On 11/09/2016 09:48 AM, Julien Grall wrote:
> Hi Boris,
>
> On 09/11/16 14:39, Boris Ostrovsky wrote:
>> diff --git a/xen/include/public/arch-arm.h
>> b/xen/include/public/arch-arm.h
>> index bd974fb..b793774 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -383,6 +383,9 @@ typedef uint64_t xen_callback_t;
>> * should instead use the FDT.
>> */
>>
>> +/* Current supported guest VCPUs */
>> +#define GUEST_MAX_VCPUS 128
>> +
>> /* Physical Address Space */
>>
>> /*
>> @@ -410,6 +413,11 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_ACPI_BASE 0x20000000ULL
>> #define GUEST_ACPI_SIZE 0x02000000ULL
>>
>> +/* Location of online VCPU bitmap. */
>> +#define ACPI_CPU_MAP 0xaf00
>> +#define ACPI_CPU_MAP_LEN ((GUEST_MAX_VCPUS / 8) + \
>> + ((GUEST_MAX_VCPUS & 7) ? 1 : 0))
>> +
>
> I don't understand this change. PRST is not generated for ARM (see the
> return 0 in mk_dsdt a bit before).
Oh, right --- I missed it. Then this change needs to be dropped.
-boris
>
> In any case, I am expecting CPU hotplug to be handle via PSCI on ARM.
>
> Regards,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
2016-11-09 14:48 ` Julien Grall
@ 2016-11-09 14:48 ` Paul Durrant
2016-11-09 14:59 ` Andrew Cooper
2016-11-15 8:47 ` Jan Beulich
3 siblings, 0 replies; 76+ messages in thread
From: Paul Durrant @ 2016-11-09 14:48 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel@lists.xen.org
Cc: Wei Liu, Andrew Cooper, Julien Grall, jbeulich@suse.com,
Ian Jackson, Roger Pau Monne
> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 09 November 2016 14:40
> To: xen-devel@lists.xen.org
> Cc: jbeulich@suse.com; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Julien Grall <julien.grall@arm.com>; Paul
> Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
>
> ACPI hotplug-related IO accesses (to GPE0 block) are handled
> by qemu for HVM guests. Since PVH guests don't have qemu these
> accesses will need to be procesed by the hypervisor.
>
> Because ACPI event model expects pm1a block to be present we
> need to have the hypervisor emulate it as well.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Julien Grall <julien.grall@arm.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changes in v2:
> * Added public macros for ACPI CPU bitmap (at 0xaf00). Note: PRST
> region shrinks from 32 bytes to 16 (when 128 VCPUs are
> supported).
>
>
> tools/libacpi/mk_dsdt.c | 4 +++-
> tools/libacpi/static_tables.c | 28 +++++++++++-----------------
> xen/include/asm-x86/hvm/domain.h | 6 ++++++
> xen/include/public/arch-arm.h | 11 ++++++++---
> xen/include/public/hvm/ioreq.h | 13 +++++++++++++
> 5 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 4ae68bc..2b8234d 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -19,6 +19,7 @@
> #include <stdbool.h>
> #if defined(__i386__) || defined(__x86_64__)
> #include <xen/hvm/hvm_info_table.h>
> +#include <xen/hvm/ioreq.h>
> #elif defined(__aarch64__)
> #include <xen/arch-arm.h>
> #endif
> @@ -244,7 +245,8 @@ int main(int argc, char **argv)
> #endif
>
> /* Operation Region 'PRST': bitmask of online CPUs. */
> - stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
> + stmt("OperationRegion", "PRST, SystemIO, 0x%x, %d",
> + ACPI_CPU_MAP, ACPI_CPU_MAP_LEN);
> push_block("Field", "PRST, ByteAcc, NoLock, Preserve");
> indent(); printf("PRS, %u\n", max_cpus);
> pop_block();
> diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
> index 617bf68..413abcc 100644
> --- a/tools/libacpi/static_tables.c
> +++ b/tools/libacpi/static_tables.c
> @@ -20,6 +20,8 @@
> * Firmware ACPI Control Structure (FACS).
> */
>
> +#define ACPI_REG_BIT_OFFSET 0
> +
> struct acpi_20_facs Facs = {
> .signature = ACPI_2_0_FACS_SIGNATURE,
> .length = sizeof(struct acpi_20_facs),
> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
> /*
> * Fixed ACPI Description Table (FADT).
> */
> -
> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
> -
> struct acpi_20_fadt Fadt = {
> .header = {
> .signature = ACPI_2_0_FADT_SIGNATURE,
> @@ -56,9 +50,9 @@ struct acpi_20_fadt Fadt = {
> .pm1a_cnt_blk = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
> .pm_tmr_blk = ACPI_PM_TMR_BLK_ADDRESS_V1,
> .gpe0_blk = ACPI_GPE0_BLK_ADDRESS_V1,
> - .pm1_evt_len = ACPI_PM1A_EVT_BLK_BIT_WIDTH / 8,
> - .pm1_cnt_len = ACPI_PM1A_CNT_BLK_BIT_WIDTH / 8,
> - .pm_tmr_len = ACPI_PM_TMR_BLK_BIT_WIDTH / 8,
> + .pm1_evt_len = ACPI_PM1A_EVT_BLK_LEN,
> + .pm1_cnt_len = ACPI_PM1A_CNT_BLK_LEN,
> + .pm_tmr_len = ACPI_PM_TMR_BLK_LEN,
> .gpe0_blk_len = ACPI_GPE0_BLK_LEN_V1,
>
> .p_lvl2_lat = 0x0fff, /* >100, means we do not support C2 state */
> @@ -79,22 +73,22 @@ struct acpi_20_fadt Fadt = {
>
> .x_pm1a_evt_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM1A_EVT_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM1A_EVT_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM1A_EVT_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM1A_EVT_BLK_ADDRESS_V1,
> },
>
> .x_pm1a_cnt_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM1A_CNT_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM1A_CNT_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM1A_CNT_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
> },
>
> .x_pm_tmr_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM_TMR_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM_TMR_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM_TMR_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM_TMR_BLK_ADDRESS_V1,
> }
> };
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-
> x86/hvm/domain.h
> index f34d784..f492a2b 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -87,6 +87,12 @@ struct hvm_domain {
> } ioreq_server;
> struct hvm_ioreq_server *default_ioreq_server;
>
> + /* PVH guests */
> + struct {
> + uint8_t pm1a[ACPI_PM1A_EVT_BLK_LEN];
> + uint8_t gpe[ACPI_GPE0_BLK_LEN_V1];
> + } acpi_io;
> +
> /* Cached CF8 for guest PCI config cycles */
> uint32_t pci_cf8;
>
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..b793774 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -383,6 +383,9 @@ typedef uint64_t xen_callback_t;
> * should instead use the FDT.
> */
>
> +/* Current supported guest VCPUs */
> +#define GUEST_MAX_VCPUS 128
> +
> /* Physical Address Space */
>
> /*
> @@ -410,6 +413,11 @@ typedef uint64_t xen_callback_t;
> #define GUEST_ACPI_BASE 0x20000000ULL
> #define GUEST_ACPI_SIZE 0x02000000ULL
>
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((GUEST_MAX_VCPUS / 8) + \
> + ((GUEST_MAX_VCPUS & 7) ? 1 : 0))
> +
> /*
> * 16MB == 4096 pages reserved for guest to use as a region to map its
> * grant table in.
> @@ -435,9 +443,6 @@ typedef uint64_t xen_callback_t;
> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE,
> GUEST_RAM1_BASE }
> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE,
> GUEST_RAM1_SIZE }
>
> -/* Current supported guest VCPUs */
> -#define GUEST_MAX_VCPUS 128
> -
> /* Interrupts */
> #define GUEST_TIMER_VIRT_PPI 27
> #define GUEST_TIMER_PHYS_S_PPI 29
> diff --git a/xen/include/public/hvm/ioreq.h
> b/xen/include/public/hvm/ioreq.h
> index 2e5809b..e3fa704 100644
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -24,6 +24,8 @@
> #ifndef _IOREQ_H_
> #define _IOREQ_H_
>
> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
> +
> #define IOREQ_READ 1
> #define IOREQ_WRITE 0
>
> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>
> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
> +#define ACPI_PM_TMR_BLK_LEN 0x04
> +
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >=
> ACPI_GPE0_BLK_ADDRESS_V1
> +#error "ACPI_CPU_MAP is too big"
> +#endif
>
> #endif /* _IOREQ_H_ */
>
> --
> 2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
2016-11-09 14:48 ` Julien Grall
2016-11-09 14:48 ` Paul Durrant
@ 2016-11-09 14:59 ` Andrew Cooper
2016-11-09 15:14 ` Boris Ostrovsky
2016-11-15 8:47 ` Jan Beulich
3 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-09 14:59 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 09/11/16 14:39, Boris Ostrovsky wrote:
> ACPI hotplug-related IO accesses (to GPE0 block) are handled
> by qemu for HVM guests. Since PVH guests don't have qemu these
> accesses will need to be procesed by the hypervisor.
>
> Because ACPI event model expects pm1a block to be present we
> need to have the hypervisor emulate it as well.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Julien Grall <julien.grall@arm.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changes in v2:
> * Added public macros for ACPI CPU bitmap (at 0xaf00). Note: PRST
> region shrinks from 32 bytes to 16 (when 128 VCPUs are
> supported).
>
>
> tools/libacpi/mk_dsdt.c | 4 +++-
> tools/libacpi/static_tables.c | 28 +++++++++++-----------------
> xen/include/asm-x86/hvm/domain.h | 6 ++++++
> xen/include/public/arch-arm.h | 11 ++++++++---
> xen/include/public/hvm/ioreq.h | 13 +++++++++++++
> 5 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 4ae68bc..2b8234d 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -19,6 +19,7 @@
> #include <stdbool.h>
> #if defined(__i386__) || defined(__x86_64__)
> #include <xen/hvm/hvm_info_table.h>
> +#include <xen/hvm/ioreq.h>
> #elif defined(__aarch64__)
> #include <xen/arch-arm.h>
> #endif
> @@ -244,7 +245,8 @@ int main(int argc, char **argv)
> #endif
>
> /* Operation Region 'PRST': bitmask of online CPUs. */
> - stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
> + stmt("OperationRegion", "PRST, SystemIO, 0x%x, %d",
> + ACPI_CPU_MAP, ACPI_CPU_MAP_LEN);
> push_block("Field", "PRST, ByteAcc, NoLock, Preserve");
> indent(); printf("PRS, %u\n", max_cpus);
> pop_block();
> diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
> index 617bf68..413abcc 100644
> --- a/tools/libacpi/static_tables.c
> +++ b/tools/libacpi/static_tables.c
> @@ -20,6 +20,8 @@
> * Firmware ACPI Control Structure (FACS).
> */
>
> +#define ACPI_REG_BIT_OFFSET 0
> +
> struct acpi_20_facs Facs = {
> .signature = ACPI_2_0_FACS_SIGNATURE,
> .length = sizeof(struct acpi_20_facs),
> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
> /*
> * Fixed ACPI Description Table (FADT).
> */
> -
> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
> -
> struct acpi_20_fadt Fadt = {
> .header = {
> .signature = ACPI_2_0_FADT_SIGNATURE,
> @@ -56,9 +50,9 @@ struct acpi_20_fadt Fadt = {
> .pm1a_cnt_blk = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
> .pm_tmr_blk = ACPI_PM_TMR_BLK_ADDRESS_V1,
> .gpe0_blk = ACPI_GPE0_BLK_ADDRESS_V1,
> - .pm1_evt_len = ACPI_PM1A_EVT_BLK_BIT_WIDTH / 8,
> - .pm1_cnt_len = ACPI_PM1A_CNT_BLK_BIT_WIDTH / 8,
> - .pm_tmr_len = ACPI_PM_TMR_BLK_BIT_WIDTH / 8,
> + .pm1_evt_len = ACPI_PM1A_EVT_BLK_LEN,
> + .pm1_cnt_len = ACPI_PM1A_CNT_BLK_LEN,
> + .pm_tmr_len = ACPI_PM_TMR_BLK_LEN,
> .gpe0_blk_len = ACPI_GPE0_BLK_LEN_V1,
>
> .p_lvl2_lat = 0x0fff, /* >100, means we do not support C2 state */
> @@ -79,22 +73,22 @@ struct acpi_20_fadt Fadt = {
>
> .x_pm1a_evt_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM1A_EVT_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM1A_EVT_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM1A_EVT_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM1A_EVT_BLK_ADDRESS_V1,
> },
>
> .x_pm1a_cnt_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM1A_CNT_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM1A_CNT_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM1A_CNT_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM1A_CNT_BLK_ADDRESS_V1,
> },
>
> .x_pm_tmr_blk = {
> .address_space_id = ACPI_SYSTEM_IO,
> - .register_bit_width = ACPI_PM_TMR_BLK_BIT_WIDTH,
> - .register_bit_offset = ACPI_PM_TMR_BLK_BIT_OFFSET,
> + .register_bit_width = ACPI_PM_TMR_BLK_LEN * 8,
> + .register_bit_offset = ACPI_REG_BIT_OFFSET,
> .address = ACPI_PM_TMR_BLK_ADDRESS_V1,
> }
> };
> diff --git a/xen/include/asm-x86/hvm/domain.h b/xen/include/asm-x86/hvm/domain.h
> index f34d784..f492a2b 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -87,6 +87,12 @@ struct hvm_domain {
> } ioreq_server;
> struct hvm_ioreq_server *default_ioreq_server;
>
> + /* PVH guests */
> + struct {
> + uint8_t pm1a[ACPI_PM1A_EVT_BLK_LEN];
> + uint8_t gpe[ACPI_GPE0_BLK_LEN_V1];
> + } acpi_io;
> +
> /* Cached CF8 for guest PCI config cycles */
> uint32_t pci_cf8;
>
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index bd974fb..b793774 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -383,6 +383,9 @@ typedef uint64_t xen_callback_t;
> * should instead use the FDT.
> */
>
> +/* Current supported guest VCPUs */
> +#define GUEST_MAX_VCPUS 128
> +
> /* Physical Address Space */
>
> /*
> @@ -410,6 +413,11 @@ typedef uint64_t xen_callback_t;
> #define GUEST_ACPI_BASE 0x20000000ULL
> #define GUEST_ACPI_SIZE 0x02000000ULL
>
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((GUEST_MAX_VCPUS / 8) + \
> + ((GUEST_MAX_VCPUS & 7) ? 1 : 0))
> +
> /*
> * 16MB == 4096 pages reserved for guest to use as a region to map its
> * grant table in.
> @@ -435,9 +443,6 @@ typedef uint64_t xen_callback_t;
> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE }
> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE }
>
> -/* Current supported guest VCPUs */
> -#define GUEST_MAX_VCPUS 128
> -
> /* Interrupts */
> #define GUEST_TIMER_VIRT_PPI 27
> #define GUEST_TIMER_PHYS_S_PPI 29
> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
> index 2e5809b..e3fa704 100644
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -24,6 +24,8 @@
> #ifndef _IOREQ_H_
> #define _IOREQ_H_
>
> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
> +
> #define IOREQ_READ 1
> #define IOREQ_WRITE 0
>
> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>
> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
> +#define ACPI_PM_TMR_BLK_LEN 0x04
> +
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
> +#error "ACPI_CPU_MAP is too big"
> +#endif
Why is this in ioreq.h? It has nothing to do with ioreq's.
The current ACPI bits in here are to do with the qemu ACPI interface,
not the Xen ACPI interface.
Also, please can we avoid hard-coding the location of the map in the
hypervisor ABI. These constants make it impossible to ever extend the
number of HVM vcpus at a future date.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:59 ` Andrew Cooper
@ 2016-11-09 15:14 ` Boris Ostrovsky
2016-11-09 19:58 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 15:14 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>
>> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
>> index 2e5809b..e3fa704 100644
>> --- a/xen/include/public/hvm/ioreq.h
>> +++ b/xen/include/public/hvm/ioreq.h
>> @@ -24,6 +24,8 @@
>> #ifndef _IOREQ_H_
>> #define _IOREQ_H_
>>
>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>> +
>> #define IOREQ_READ 1
>> #define IOREQ_WRITE 0
>>
>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>
>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>> +
>> +/* Location of online VCPU bitmap. */
>> +#define ACPI_CPU_MAP 0xaf00
>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>> +#error "ACPI_CPU_MAP is too big"
>> +#endif
> Why is this in ioreq.h? It has nothing to do with ioreq's.
>
> The current ACPI bits in here are to do with the qemu ACPI interface,
> not the Xen ACPI interface.
>
> Also, please can we avoid hard-coding the location of the map in the
> hypervisor ABI. These constants make it impossible to ever extend the
> number of HVM vcpus at a future date.
The first three logically belong here because corresponding blocks'
addresses are defined right above.
ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
hypervisor (and qemu as well, although it is defined as
PIIX4_CPU_HOTPLUG_IO_BASE).
Where do you think it should go then?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 15:14 ` Boris Ostrovsky
@ 2016-11-09 19:58 ` Andrew Cooper
2016-11-09 21:01 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-09 19:58 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 09/11/16 15:14, Boris Ostrovsky wrote:
> On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>>> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
>>> index 2e5809b..e3fa704 100644
>>> --- a/xen/include/public/hvm/ioreq.h
>>> +++ b/xen/include/public/hvm/ioreq.h
>>> @@ -24,6 +24,8 @@
>>> #ifndef _IOREQ_H_
>>> #define _IOREQ_H_
>>>
>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>> +
>>> #define IOREQ_READ 1
>>> #define IOREQ_WRITE 0
>>>
>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>
>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>> +
>>> +/* Location of online VCPU bitmap. */
>>> +#define ACPI_CPU_MAP 0xaf00
>>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>>> +#error "ACPI_CPU_MAP is too big"
>>> +#endif
>> Why is this in ioreq.h? It has nothing to do with ioreq's.
>>
>> The current ACPI bits in here are to do with the qemu ACPI interface,
>> not the Xen ACPI interface.
>>
>> Also, please can we avoid hard-coding the location of the map in the
>> hypervisor ABI. These constants make it impossible to ever extend the
>> number of HVM vcpus at a future date.
> The first three logically belong here because corresponding blocks'
> addresses are defined right above.
They have no relationship to the ones above, other than their name.
>
> ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
> hypervisor (and qemu as well, although it is defined as
> PIIX4_CPU_HOTPLUG_IO_BASE).
>
> Where do you think it should go then?
This highlights a reoccurring problem in Xen which desperately needs
fixing, but still isn't high enough on my TODO list to tackle yet.
There is no central registration of claims on domain resources. This is
the root cause of memory accounting problems for HVM guests.
The way I planned to fix this was to have Xen maintain a registry of
domains physical resources which ends up looking very much like
/proc/io{mem,ports}. There will be a hypercall interface for querying
this information, and for a toolstack and device model to modify it.
The key point is that Xen needs to be authoritative source of
information pertaining to layout, rather than the current fiasco we have
of the toolstack, qemu and hvmloader all thinking they know and control
what's going on. This fixes several current unknowns which have caused
real problems, such as whether a domain was told about certain RMRRs
when it booted, or how many PXEROMs qemu tried to fit into the physmap.
This information (eventually, when I get Xen-level migration v2 sorted)
needs to move at the head of the migration stream.
The way I would envisage this working is that on domain create, Xen
makes a blank map indicating that all space is free. By selecting
X86_EMUL_APCI_*, Xen takes out an allocation when it wires up the ioport
handler.
Later, when constructing the ACPI tables, the toolstack reads the
current ioport allocations and can see exactly which ports are reserved
for what.
Now, I understand that lumbering you with this work as a prerequisite
would be unfair.
Therefore, I will accept an alternative of hiding all these definitions
behind __XEN_TOOLS__ so the longterm fix can be introduced in a
compatible manner in the future.
That said, I am still certain that they shouldn't live in ioreq.h, as
they have nothing to do with Qemu.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 19:58 ` Andrew Cooper
@ 2016-11-09 21:01 ` Boris Ostrovsky
2016-11-14 15:01 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 21:01 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 11/09/2016 02:58 PM, Andrew Cooper wrote:
> On 09/11/16 15:14, Boris Ostrovsky wrote:
>> On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>>>> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
>>>> index 2e5809b..e3fa704 100644
>>>> --- a/xen/include/public/hvm/ioreq.h
>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>> @@ -24,6 +24,8 @@
>>>> #ifndef _IOREQ_H_
>>>> #define _IOREQ_H_
>>>>
>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>> +
>>>> #define IOREQ_READ 1
>>>> #define IOREQ_WRITE 0
>>>>
>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>
>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>> +
>>>> +/* Location of online VCPU bitmap. */
>>>> +#define ACPI_CPU_MAP 0xaf00
>>>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>>>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>>>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>>>> +#error "ACPI_CPU_MAP is too big"
>>>> +#endif
>>> Why is this in ioreq.h? It has nothing to do with ioreq's.
>>>
>>> The current ACPI bits in here are to do with the qemu ACPI interface,
>>> not the Xen ACPI interface.
>>>
>>> Also, please can we avoid hard-coding the location of the map in the
>>> hypervisor ABI. These constants make it impossible to ever extend the
>>> number of HVM vcpus at a future date.
>> The first three logically belong here because corresponding blocks'
>> addresses are defined right above.
> They have no relationship to the ones above, other than their name.
They describe the same object --- for example
ACPI_PM1A_CNT_BLK_ADDRESS_V1 and (new) ACPI_PM1A_CNT_BLK_LEN describe
pm1a control.
As far as definitions being there for qemu interface only ---
ACPI_PM1A_CNT_BLK_ADDRESS_V1, for example, is used only by hvmloader and
libacpi.
>
>> ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
>> hypervisor (and qemu as well, although it is defined as
>> PIIX4_CPU_HOTPLUG_IO_BASE).
>>
>> Where do you think it should go then?
> This highlights a reoccurring problem in Xen which desperately needs
> fixing, but still isn't high enough on my TODO list to tackle yet.
>
> There is no central registration of claims on domain resources. This is
> the root cause of memory accounting problems for HVM guests.
>
>
> The way I planned to fix this was to have Xen maintain a registry of
> domains physical resources which ends up looking very much like
> /proc/io{mem,ports}. There will be a hypercall interface for querying
> this information, and for a toolstack and device model to modify it.
>
> The key point is that Xen needs to be authoritative source of
> information pertaining to layout, rather than the current fiasco we have
> of the toolstack, qemu and hvmloader all thinking they know and control
> what's going on. This fixes several current unknowns which have caused
> real problems, such as whether a domain was told about certain RMRRs
> when it booted, or how many PXEROMs qemu tried to fit into the physmap.
>
> This information (eventually, when I get Xen-level migration v2 sorted)
> needs to move at the head of the migration stream.
>
> The way I would envisage this working is that on domain create, Xen
> makes a blank map indicating that all space is free. By selecting
> X86_EMUL_APCI_*, Xen takes out an allocation when it wires up the ioport
> handler.
>
> Later, when constructing the ACPI tables, the toolstack reads the
> current ioport allocations and can see exactly which ports are reserved
> for what.
>
>
> Now, I understand that lumbering you with this work as a prerequisite
> would be unfair.
>
> Therefore, I will accept an alternative of hiding all these definitions
> behind __XEN_TOOLS__ so the longterm fix can be introduced in a
> compatible manner in the future.
__XEN_TOOLS__ or (__XEN__ || __XEN_TOOLS__) ? Because both the toolstack
and the hypervisor want to see them.
>
> That said, I am still certain that they shouldn't live in ioreq.h, as
> they have nothing to do with Qemu.
None of the existing files looks (to me) much better in terms of being
more appropriate. include/public/arch-x86/xen.h?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 21:01 ` Boris Ostrovsky
@ 2016-11-14 15:01 ` Boris Ostrovsky
2016-11-14 17:19 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-14 15:01 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 11/09/2016 04:01 PM, Boris Ostrovsky wrote:
> On 11/09/2016 02:58 PM, Andrew Cooper wrote:
>> On 09/11/16 15:14, Boris Ostrovsky wrote:
>>> On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>>>>> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
>>>>> index 2e5809b..e3fa704 100644
>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>> @@ -24,6 +24,8 @@
>>>>> #ifndef _IOREQ_H_
>>>>> #define _IOREQ_H_
>>>>>
>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>> +
>>>>> #define IOREQ_READ 1
>>>>> #define IOREQ_WRITE 0
>>>>>
>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>
>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>> +
>>>>> +/* Location of online VCPU bitmap. */
>>>>> +#define ACPI_CPU_MAP 0xaf00
>>>>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>>>>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>>>>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>>>>> +#error "ACPI_CPU_MAP is too big"
>>>>> +#endif
>>>> Why is this in ioreq.h? It has nothing to do with ioreq's.
>>>>
>>>> The current ACPI bits in here are to do with the qemu ACPI interface,
>>>> not the Xen ACPI interface.
>>>>
>>>> Also, please can we avoid hard-coding the location of the map in the
>>>> hypervisor ABI. These constants make it impossible to ever extend the
>>>> number of HVM vcpus at a future date.
>>> The first three logically belong here because corresponding blocks'
>>> addresses are defined right above.
>> They have no relationship to the ones above, other than their name.
> They describe the same object --- for example
> ACPI_PM1A_CNT_BLK_ADDRESS_V1 and (new) ACPI_PM1A_CNT_BLK_LEN describe
> pm1a control.
>
> As far as definitions being there for qemu interface only ---
> ACPI_PM1A_CNT_BLK_ADDRESS_V1, for example, is used only by hvmloader and
> libacpi.
>
>
>>> ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
>>> hypervisor (and qemu as well, although it is defined as
>>> PIIX4_CPU_HOTPLUG_IO_BASE).
>>>
>>> Where do you think it should go then?
>> This highlights a reoccurring problem in Xen which desperately needs
>> fixing, but still isn't high enough on my TODO list to tackle yet.
>>
>> There is no central registration of claims on domain resources. This is
>> the root cause of memory accounting problems for HVM guests.
>>
>>
>> The way I planned to fix this was to have Xen maintain a registry of
>> domains physical resources which ends up looking very much like
>> /proc/io{mem,ports}. There will be a hypercall interface for querying
>> this information, and for a toolstack and device model to modify it.
>>
>> The key point is that Xen needs to be authoritative source of
>> information pertaining to layout, rather than the current fiasco we have
>> of the toolstack, qemu and hvmloader all thinking they know and control
>> what's going on. This fixes several current unknowns which have caused
>> real problems, such as whether a domain was told about certain RMRRs
>> when it booted, or how many PXEROMs qemu tried to fit into the physmap.
>>
>> This information (eventually, when I get Xen-level migration v2 sorted)
>> needs to move at the head of the migration stream.
>>
>> The way I would envisage this working is that on domain create, Xen
>> makes a blank map indicating that all space is free. By selecting
>> X86_EMUL_APCI_*, Xen takes out an allocation when it wires up the ioport
>> handler.
>>
>> Later, when constructing the ACPI tables, the toolstack reads the
>> current ioport allocations and can see exactly which ports are reserved
>> for what.
>>
>>
>> Now, I understand that lumbering you with this work as a prerequisite
>> would be unfair.
>>
>> Therefore, I will accept an alternative of hiding all these definitions
>> behind __XEN_TOOLS__ so the longterm fix can be introduced in a
>> compatible manner in the future.
>
> __XEN_TOOLS__ or (__XEN__ || __XEN_TOOLS__) ? Because both the toolstack
> and the hypervisor want to see them.
>
>
>> That said, I am still certain that they shouldn't live in ioreq.h, as
>> they have nothing to do with Qemu.
> None of the existing files looks (to me) much better in terms of being
> more appropriate. include/public/arch-x86/xen.h?
Andrew, ping on these two questions.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-14 15:01 ` Boris Ostrovsky
@ 2016-11-14 17:19 ` Andrew Cooper
0 siblings, 0 replies; 76+ messages in thread
From: Andrew Cooper @ 2016-11-14 17:19 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel
Cc: wei.liu2, ian.jackson, Julien Grall, Paul Durrant, jbeulich,
roger.pau
On 14/11/16 15:01, Boris Ostrovsky wrote:
> On 11/09/2016 04:01 PM, Boris Ostrovsky wrote:
>> On 11/09/2016 02:58 PM, Andrew Cooper wrote:
>>> On 09/11/16 15:14, Boris Ostrovsky wrote:
>>>> On 11/09/2016 09:59 AM, Andrew Cooper wrote:
>>>>>> diff --git a/xen/include/public/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
>>>>>> index 2e5809b..e3fa704 100644
>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>> @@ -24,6 +24,8 @@
>>>>>> #ifndef _IOREQ_H_
>>>>>> #define _IOREQ_H_
>>>>>>
>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>> +
>>>>>> #define IOREQ_READ 1
>>>>>> #define IOREQ_WRITE 0
>>>>>>
>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>
>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>>> +
>>>>>> +/* Location of online VCPU bitmap. */
>>>>>> +#define ACPI_CPU_MAP 0xaf00
>>>>>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>>>>>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
>>>>>> +#if ACPI_CPU_MAP + ACPI_CPU_MAP_LEN >= ACPI_GPE0_BLK_ADDRESS_V1
>>>>>> +#error "ACPI_CPU_MAP is too big"
>>>>>> +#endif
>>>>> Why is this in ioreq.h? It has nothing to do with ioreq's.
>>>>>
>>>>> The current ACPI bits in here are to do with the qemu ACPI interface,
>>>>> not the Xen ACPI interface.
>>>>>
>>>>> Also, please can we avoid hard-coding the location of the map in the
>>>>> hypervisor ABI. These constants make it impossible to ever extend the
>>>>> number of HVM vcpus at a future date.
>>>> The first three logically belong here because corresponding blocks'
>>>> addresses are defined right above.
>>> They have no relationship to the ones above, other than their name.
>> They describe the same object --- for example
>> ACPI_PM1A_CNT_BLK_ADDRESS_V1 and (new) ACPI_PM1A_CNT_BLK_LEN describe
>> pm1a control.
>>
>> As far as definitions being there for qemu interface only ---
>> ACPI_PM1A_CNT_BLK_ADDRESS_V1, for example, is used only by hvmloader and
>> libacpi.
>>
>>
>>>> ACPI_CPU_MAP has to be seen by both the toolstack (libacpi) and the
>>>> hypervisor (and qemu as well, although it is defined as
>>>> PIIX4_CPU_HOTPLUG_IO_BASE).
>>>>
>>>> Where do you think it should go then?
>>> This highlights a reoccurring problem in Xen which desperately needs
>>> fixing, but still isn't high enough on my TODO list to tackle yet.
>>>
>>> There is no central registration of claims on domain resources. This is
>>> the root cause of memory accounting problems for HVM guests.
>>>
>>>
>>> The way I planned to fix this was to have Xen maintain a registry of
>>> domains physical resources which ends up looking very much like
>>> /proc/io{mem,ports}. There will be a hypercall interface for querying
>>> this information, and for a toolstack and device model to modify it.
>>>
>>> The key point is that Xen needs to be authoritative source of
>>> information pertaining to layout, rather than the current fiasco we have
>>> of the toolstack, qemu and hvmloader all thinking they know and control
>>> what's going on. This fixes several current unknowns which have caused
>>> real problems, such as whether a domain was told about certain RMRRs
>>> when it booted, or how many PXEROMs qemu tried to fit into the physmap.
>>>
>>> This information (eventually, when I get Xen-level migration v2 sorted)
>>> needs to move at the head of the migration stream.
>>>
>>> The way I would envisage this working is that on domain create, Xen
>>> makes a blank map indicating that all space is free. By selecting
>>> X86_EMUL_APCI_*, Xen takes out an allocation when it wires up the ioport
>>> handler.
>>>
>>> Later, when constructing the ACPI tables, the toolstack reads the
>>> current ioport allocations and can see exactly which ports are reserved
>>> for what.
>>>
>>>
>>> Now, I understand that lumbering you with this work as a prerequisite
>>> would be unfair.
>>>
>>> Therefore, I will accept an alternative of hiding all these definitions
>>> behind __XEN_TOOLS__ so the longterm fix can be introduced in a
>>> compatible manner in the future.
>> __XEN_TOOLS__ or (__XEN__ || __XEN_TOOLS__) ? Because both the toolstack
>> and the hypervisor want to see them.
(__XEN__ || __XEN_TOOLS__) is fine.
>>
>>
>>> That said, I am still certain that they shouldn't live in ioreq.h, as
>>> they have nothing to do with Qemu.
>> None of the existing files looks (to me) much better in terms of being
>> more appropriate. include/public/arch-x86/xen.h?
> Andrew, ping on these two questions.
Sorry for letting this slip through the cracks.
Leave them here for now. xen.h is not a better place for them to live.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
` (2 preceding siblings ...)
2016-11-09 14:59 ` Andrew Cooper
@ 2016-11-15 8:47 ` Jan Beulich
2016-11-15 14:47 ` Boris Ostrovsky
3 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 8:47 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> @@ -244,7 +245,8 @@ int main(int argc, char **argv)
> #endif
>
> /* Operation Region 'PRST': bitmask of online CPUs. */
> - stmt("OperationRegion", "PRST, SystemIO, 0xaf00, 32");
> + stmt("OperationRegion", "PRST, SystemIO, 0x%x, %d",
%#x
> --- a/tools/libacpi/static_tables.c
> +++ b/tools/libacpi/static_tables.c
> @@ -20,6 +20,8 @@
> * Firmware ACPI Control Structure (FACS).
> */
>
> +#define ACPI_REG_BIT_OFFSET 0
Can you completely exclude us ever wanting to support something
that's not on a byte boundary? I think there was a good reason ...
> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
> /*
> * Fixed ACPI Description Table (FADT).
> */
> -
> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
... these specified both width and offset.
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -87,6 +87,12 @@ struct hvm_domain {
> } ioreq_server;
> struct hvm_ioreq_server *default_ioreq_server;
>
> + /* PVH guests */
> + struct {
> + uint8_t pm1a[ACPI_PM1A_EVT_BLK_LEN];
> + uint8_t gpe[ACPI_GPE0_BLK_LEN_V1];
> + } acpi_io;
This is left unused in this patch - please add it once it gets used.
> --- a/xen/include/public/hvm/ioreq.h
> +++ b/xen/include/public/hvm/ioreq.h
> @@ -24,6 +24,8 @@
> #ifndef _IOREQ_H_
> #define _IOREQ_H_
>
> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
> +
> #define IOREQ_READ 1
> #define IOREQ_WRITE 0
>
> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>
> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
> +#define ACPI_PM_TMR_BLK_LEN 0x04
Just like ACPI_GPE0_BLK_LEN these should really go next to their
address definitions. Provided we really want to hard code further
values here in the first place, which I don't think we should. The
goal should rather be for all these hard coded values to go away
(which really should have happened when the V1 variants had
been added).
> +/* Location of online VCPU bitmap. */
> +#define ACPI_CPU_MAP 0xaf00
> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
((HVM_MAX_VCPUS + 7) / 8)
But as indicated elsewhere, this should be made less static anyway.
Also, no matter that there are (many, or should I say only) bad
examples in this file, please don't add new possible name space
conflicts: Any new constants should start with XEN_.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 8:47 ` Jan Beulich
@ 2016-11-15 14:47 ` Boris Ostrovsky
2016-11-15 15:13 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 14:47 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
On 11/15/2016 03:47 AM, Jan Beulich wrote:
>
>> --- a/tools/libacpi/static_tables.c
>> +++ b/tools/libacpi/static_tables.c
>> @@ -20,6 +20,8 @@
>> * Firmware ACPI Control Structure (FACS).
>> */
>>
>> +#define ACPI_REG_BIT_OFFSET 0
> Can you completely exclude us ever wanting to support something
> that's not on a byte boundary? I think there was a good reason ...
>
>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>> /*
>> * Fixed ACPI Description Table (FADT).
>> */
>> -
>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
> ... these specified both width and offset.
Since OFFSET is not used anywhere I kept it local to static_tables.c. I
can restore these macros per block and move them to public header but...
>> --- a/xen/include/public/hvm/ioreq.h
>> +++ b/xen/include/public/hvm/ioreq.h
>> @@ -24,6 +24,8 @@
>> #ifndef _IOREQ_H_
>> #define _IOREQ_H_
>>
>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>> +
>> #define IOREQ_READ 1
>> #define IOREQ_WRITE 0
>>
>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>
>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>> +#define ACPI_PM_TMR_BLK_LEN 0x04
> Just like ACPI_GPE0_BLK_LEN these should really go next to their
> address definitions.
... together with this, it will make it rather messy/unsightly to go
with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
> Provided we really want to hard code further
> values here in the first place, which I don't think we should. The
> goal should rather be for all these hard coded values to go away
> (which really should have happened when the V1 variants had
> been added).
How can we not hardcode this if the values should match those in FADT
(i.e. static_tables.c)?
>
>> +/* Location of online VCPU bitmap. */
>> +#define ACPI_CPU_MAP 0xaf00
>> +#define ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS / 8) + \
>> + ((HVM_MAX_VCPUS & 7) ? 1 : 0))
> ((HVM_MAX_VCPUS + 7) / 8)
>
> But as indicated elsewhere, this should be made less static anyway.
Right, we could size it by domain.max_vcpus.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 14:47 ` Boris Ostrovsky
@ 2016-11-15 15:13 ` Jan Beulich
2016-11-15 15:41 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 15:13 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>
>>> --- a/tools/libacpi/static_tables.c
>>> +++ b/tools/libacpi/static_tables.c
>>> @@ -20,6 +20,8 @@
>>> * Firmware ACPI Control Structure (FACS).
>>> */
>>>
>>> +#define ACPI_REG_BIT_OFFSET 0
>> Can you completely exclude us ever wanting to support something
>> that's not on a byte boundary? I think there was a good reason ...
>>
>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>> /*
>>> * Fixed ACPI Description Table (FADT).
>>> */
>>> -
>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>> ... these specified both width and offset.
>
> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
> can restore these macros per block and move them to public header but...
>
>>> --- a/xen/include/public/hvm/ioreq.h
>>> +++ b/xen/include/public/hvm/ioreq.h
>>> @@ -24,6 +24,8 @@
>>> #ifndef _IOREQ_H_
>>> #define _IOREQ_H_
>>>
>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>> +
>>> #define IOREQ_READ 1
>>> #define IOREQ_WRITE 0
>>>
>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>
>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>> address definitions.
>
> ... together with this, it will make it rather messy/unsightly to go
> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
Well, framing them that way is a good excuse for having them
separate from the others. In fact, however, the others also
should get hidden in the same way, just that we would need to
be more careful there (read: make the condition also check
__XEN_INTERFACE_VERSION__).
>> Provided we really want to hard code further
>> values here in the first place, which I don't think we should. The
>> goal should rather be for all these hard coded values to go away
>> (which really should have happened when the V1 variants had
>> been added).
>
> How can we not hardcode this if the values should match those in FADT
> (i.e. static_tables.c)?
By having the loading entity obtain the dynamic values and adjust
the table(s) accordingly.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 15:13 ` Jan Beulich
@ 2016-11-15 15:41 ` Boris Ostrovsky
2016-11-15 15:53 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 15:41 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>> --- a/tools/libacpi/static_tables.c
>>>> +++ b/tools/libacpi/static_tables.c
>>>> @@ -20,6 +20,8 @@
>>>> * Firmware ACPI Control Structure (FACS).
>>>> */
>>>>
>>>> +#define ACPI_REG_BIT_OFFSET 0
>>> Can you completely exclude us ever wanting to support something
>>> that's not on a byte boundary? I think there was a good reason ...
>>>
>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>> /*
>>>> * Fixed ACPI Description Table (FADT).
>>>> */
>>>> -
>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>> ... these specified both width and offset.
>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>> can restore these macros per block and move them to public header but...
>>
>>>> --- a/xen/include/public/hvm/ioreq.h
>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>> @@ -24,6 +24,8 @@
>>>> #ifndef _IOREQ_H_
>>>> #define _IOREQ_H_
>>>>
>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>> +
>>>> #define IOREQ_READ 1
>>>> #define IOREQ_WRITE 0
>>>>
>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>
>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>> address definitions.
>> ... together with this, it will make it rather messy/unsightly to go
>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
> Well, framing them that way is a good excuse for having them
> separate from the others. In fact, however, the others also
> should get hidden in the same way, just that we would need to
> be more careful there (read: make the condition also check
> __XEN_INTERFACE_VERSION__).
Sorry, I don't follow this. How can interface version help here?
>
>>> Provided we really want to hard code further
>>> values here in the first place, which I don't think we should. The
>>> goal should rather be for all these hard coded values to go away
>>> (which really should have happened when the V1 variants had
>>> been added).
>> How can we not hardcode this if the values should match those in FADT
>> (i.e. static_tables.c)?
> By having the loading entity obtain the dynamic values and adjust
> the table(s) accordingly.
And this. Which loading entity (ACPI builder?) and how would it adjust
the addresses? It still needs those addresses defined somewhere. And the
the hypervisor, which can't parse guest FADT, needs to get those addresses.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 15:41 ` Boris Ostrovsky
@ 2016-11-15 15:53 ` Jan Beulich
2016-11-15 16:23 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 15:53 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>> --- a/tools/libacpi/static_tables.c
>>>>> +++ b/tools/libacpi/static_tables.c
>>>>> @@ -20,6 +20,8 @@
>>>>> * Firmware ACPI Control Structure (FACS).
>>>>> */
>>>>>
>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>> Can you completely exclude us ever wanting to support something
>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>
>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>> /*
>>>>> * Fixed ACPI Description Table (FADT).
>>>>> */
>>>>> -
>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>> ... these specified both width and offset.
>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>> can restore these macros per block and move them to public header but...
>>>
>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>> @@ -24,6 +24,8 @@
>>>>> #ifndef _IOREQ_H_
>>>>> #define _IOREQ_H_
>>>>>
>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>> +
>>>>> #define IOREQ_READ 1
>>>>> #define IOREQ_WRITE 0
>>>>>
>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>
>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>> address definitions.
>>> ... together with this, it will make it rather messy/unsightly to go
>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>> Well, framing them that way is a good excuse for having them
>> separate from the others. In fact, however, the others also
>> should get hidden in the same way, just that we would need to
>> be more careful there (read: make the condition also check
>> __XEN_INTERFACE_VERSION__).
>
> Sorry, I don't follow this. How can interface version help here?
We can't outright remove existing definitions from the public interface,
but we can limit their exposure to old consumers.
>>>> Provided we really want to hard code further
>>>> values here in the first place, which I don't think we should. The
>>>> goal should rather be for all these hard coded values to go away
>>>> (which really should have happened when the V1 variants had
>>>> been added).
>>> How can we not hardcode this if the values should match those in FADT
>>> (i.e. static_tables.c)?
>> By having the loading entity obtain the dynamic values and adjust
>> the table(s) accordingly.
>
> And this. Which loading entity (ACPI builder?) and how would it adjust
> the addresses? It still needs those addresses defined somewhere. And the
> the hypervisor, which can't parse guest FADT, needs to get those addresses.
Didn't Andrew make quite clear that there needs to be a central
authority assigning guest resources? That's where the values
would come from, and they would need to be suitably propagated
to however is in need of knowing them.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 15:53 ` Jan Beulich
@ 2016-11-15 16:23 ` Boris Ostrovsky
2016-11-15 16:33 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 16:23 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>> @@ -20,6 +20,8 @@
>>>>>> * Firmware ACPI Control Structure (FACS).
>>>>>> */
>>>>>>
>>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>>> Can you completely exclude us ever wanting to support something
>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>
>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>> /*
>>>>>> * Fixed ACPI Description Table (FADT).
>>>>>> */
>>>>>> -
>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>>> ... these specified both width and offset.
>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>> can restore these macros per block and move them to public header but...
>>>>
>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>> @@ -24,6 +24,8 @@
>>>>>> #ifndef _IOREQ_H_
>>>>>> #define _IOREQ_H_
>>>>>>
>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>> +
>>>>>> #define IOREQ_READ 1
>>>>>> #define IOREQ_WRITE 0
>>>>>>
>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>
>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>> address definitions.
>>>> ... together with this, it will make it rather messy/unsightly to go
>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>> Well, framing them that way is a good excuse for having them
>>> separate from the others. In fact, however, the others also
>>> should get hidden in the same way, just that we would need to
>>> be more careful there (read: make the condition also check
>>> __XEN_INTERFACE_VERSION__).
>> Sorry, I don't follow this. How can interface version help here?
> We can't outright remove existing definitions from the public interface,
> but we can limit their exposure to old consumers.
But don't we need to support both V0 and V1 as long as qemu-trad is
supported? In other words, checking interface version won't limit the
scope at this point.
>
>>>>> Provided we really want to hard code further
>>>>> values here in the first place, which I don't think we should. The
>>>>> goal should rather be for all these hard coded values to go away
>>>>> (which really should have happened when the V1 variants had
>>>>> been added).
>>>> How can we not hardcode this if the values should match those in FADT
>>>> (i.e. static_tables.c)?
>>> By having the loading entity obtain the dynamic values and adjust
>>> the table(s) accordingly.
>> And this. Which loading entity (ACPI builder?) and how would it adjust
>> the addresses? It still needs those addresses defined somewhere. And the
>> the hypervisor, which can't parse guest FADT, needs to get those addresses.
> Didn't Andrew make quite clear that there needs to be a central
> authority assigning guest resources? That's where the values
> would come from, and they would need to be suitably propagated
> to however is in need of knowing them.
Oh, but that is still (way?) off at this point. From what I understood
about Andrew's proposal this will require fairly significant update of
how regions are registered.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 16:23 ` Boris Ostrovsky
@ 2016-11-15 16:33 ` Jan Beulich
2016-11-15 16:58 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 16:33 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
>>> On 15.11.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>>> @@ -20,6 +20,8 @@
>>>>>>> * Firmware ACPI Control Structure (FACS).
>>>>>>> */
>>>>>>>
>>>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>>>> Can you completely exclude us ever wanting to support something
>>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>>
>>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>>> /*
>>>>>>> * Fixed ACPI Description Table (FADT).
>>>>>>> */
>>>>>>> -
>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>>>> ... these specified both width and offset.
>>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>>> can restore these macros per block and move them to public header but...
>>>>>
>>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>>> @@ -24,6 +24,8 @@
>>>>>>> #ifndef _IOREQ_H_
>>>>>>> #define _IOREQ_H_
>>>>>>>
>>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>>> +
>>>>>>> #define IOREQ_READ 1
>>>>>>> #define IOREQ_WRITE 0
>>>>>>>
>>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>>
>>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>>> address definitions.
>>>>> ... together with this, it will make it rather messy/unsightly to go
>>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>>> Well, framing them that way is a good excuse for having them
>>>> separate from the others. In fact, however, the others also
>>>> should get hidden in the same way, just that we would need to
>>>> be more careful there (read: make the condition also check
>>>> __XEN_INTERFACE_VERSION__).
>>> Sorry, I don't follow this. How can interface version help here?
>> We can't outright remove existing definitions from the public interface,
>> but we can limit their exposure to old consumers.
>
> But don't we need to support both V0 and V1 as long as qemu-trad is
> supported? In other words, checking interface version won't limit the
> scope at this point.
Doesn't qemu-trad set __XEN_TOOLS__?
>>>>>> Provided we really want to hard code further
>>>>>> values here in the first place, which I don't think we should. The
>>>>>> goal should rather be for all these hard coded values to go away
>>>>>> (which really should have happened when the V1 variants had
>>>>>> been added).
>>>>> How can we not hardcode this if the values should match those in FADT
>>>>> (i.e. static_tables.c)?
>>>> By having the loading entity obtain the dynamic values and adjust
>>>> the table(s) accordingly.
>>> And this. Which loading entity (ACPI builder?) and how would it adjust
>>> the addresses? It still needs those addresses defined somewhere. And the
>>> the hypervisor, which can't parse guest FADT, needs to get those addresses.
>> Didn't Andrew make quite clear that there needs to be a central
>> authority assigning guest resources? That's where the values
>> would come from, and they would need to be suitably propagated
>> to however is in need of knowing them.
>
> Oh, but that is still (way?) off at this point. From what I understood
> about Andrew's proposal this will require fairly significant update of
> how regions are registered.
Well, perhaps. Yet I question whether it's a good idea to add another
fixed address right now, instead of switching over first.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 16:33 ` Jan Beulich
@ 2016-11-15 16:58 ` Boris Ostrovsky
2016-11-15 16:59 ` Jan Beulich
2016-11-15 18:31 ` Andrew Cooper
0 siblings, 2 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 16:58 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
On 11/15/2016 11:33 AM, Jan Beulich wrote:
>>>> On 15.11.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>> On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>>>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>>>> @@ -20,6 +20,8 @@
>>>>>>>> * Firmware ACPI Control Structure (FACS).
>>>>>>>> */
>>>>>>>>
>>>>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>>>>> Can you completely exclude us ever wanting to support something
>>>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>>>
>>>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>>>> /*
>>>>>>>> * Fixed ACPI Description Table (FADT).
>>>>>>>> */
>>>>>>>> -
>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>>>>> ... these specified both width and offset.
>>>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>>>> can restore these macros per block and move them to public header but...
>>>>>>
>>>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>> #ifndef _IOREQ_H_
>>>>>>>> #define _IOREQ_H_
>>>>>>>>
>>>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>>>> +
>>>>>>>> #define IOREQ_READ 1
>>>>>>>> #define IOREQ_WRITE 0
>>>>>>>>
>>>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>>>
>>>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>>>> address definitions.
>>>>>> ... together with this, it will make it rather messy/unsightly to go
>>>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>>>> Well, framing them that way is a good excuse for having them
>>>>> separate from the others. In fact, however, the others also
>>>>> should get hidden in the same way, just that we would need to
>>>>> be more careful there (read: make the condition also check
>>>>> __XEN_INTERFACE_VERSION__).
>>>> Sorry, I don't follow this. How can interface version help here?
>>> We can't outright remove existing definitions from the public interface,
>>> but we can limit their exposure to old consumers.
>> But don't we need to support both V0 and V1 as long as qemu-trad is
>> supported? In other words, checking interface version won't limit the
>> scope at this point.
> Doesn't qemu-trad set __XEN_TOOLS__?
Oh, so you meant that interface version would be an OR, in addition to
__XEN__ and __XEN_TOOLS__?
>
>>>>>>> Provided we really want to hard code further
>>>>>>> values here in the first place, which I don't think we should. The
>>>>>>> goal should rather be for all these hard coded values to go away
>>>>>>> (which really should have happened when the V1 variants had
>>>>>>> been added).
>>>>>> How can we not hardcode this if the values should match those in FADT
>>>>>> (i.e. static_tables.c)?
>>>>> By having the loading entity obtain the dynamic values and adjust
>>>>> the table(s) accordingly.
>>>> And this. Which loading entity (ACPI builder?) and how would it adjust
>>>> the addresses? It still needs those addresses defined somewhere. And the
>>>> the hypervisor, which can't parse guest FADT, needs to get those addresses.
>>> Didn't Andrew make quite clear that there needs to be a central
>>> authority assigning guest resources? That's where the values
>>> would come from, and they would need to be suitably propagated
>>> to however is in need of knowing them.
>> Oh, but that is still (way?) off at this point. From what I understood
>> about Andrew's proposal this will require fairly significant update of
>> how regions are registered.
> Well, perhaps. Yet I question whether it's a good idea to add another
> fixed address right now, instead of switching over first.
I think getting that framework in order would be out of the scope of
what this series is trying to achieve.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 16:58 ` Boris Ostrovsky
@ 2016-11-15 16:59 ` Jan Beulich
2016-11-15 18:31 ` Andrew Cooper
1 sibling, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 16:59 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Julien Grall,
Paul Durrant, roger.pau
>>> On 15.11.16 at 17:58, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 11:33 AM, Jan Beulich wrote:
>>>>> On 15.11.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>>>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>>>>> @@ -20,6 +20,8 @@
>>>>>>>>> * Firmware ACPI Control Structure (FACS).
>>>>>>>>> */
>>>>>>>>>
>>>>>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>>>>>> Can you completely exclude us ever wanting to support something
>>>>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>>>>
>>>>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>>>>> /*
>>>>>>>>> * Fixed ACPI Description Table (FADT).
>>>>>>>>> */
>>>>>>>>> -
>>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>>>>>> ... these specified both width and offset.
>>>>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>>>>> can restore these macros per block and move them to public header but...
>>>>>>>
>>>>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>> #ifndef _IOREQ_H_
>>>>>>>>> #define _IOREQ_H_
>>>>>>>>>
>>>>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>>>>> +
>>>>>>>>> #define IOREQ_READ 1
>>>>>>>>> #define IOREQ_WRITE 0
>>>>>>>>>
>>>>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>>>>
>>>>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>>>>> address definitions.
>>>>>>> ... together with this, it will make it rather messy/unsightly to go
>>>>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>>>>> Well, framing them that way is a good excuse for having them
>>>>>> separate from the others. In fact, however, the others also
>>>>>> should get hidden in the same way, just that we would need to
>>>>>> be more careful there (read: make the condition also check
>>>>>> __XEN_INTERFACE_VERSION__).
>>>>> Sorry, I don't follow this. How can interface version help here?
>>>> We can't outright remove existing definitions from the public interface,
>>>> but we can limit their exposure to old consumers.
>>> But don't we need to support both V0 and V1 as long as qemu-trad is
>>> supported? In other words, checking interface version won't limit the
>>> scope at this point.
>> Doesn't qemu-trad set __XEN_TOOLS__?
>
> Oh, so you meant that interface version would be an OR, in addition to
> __XEN__ and __XEN_TOOLS__?
Yes.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests
2016-11-15 16:58 ` Boris Ostrovsky
2016-11-15 16:59 ` Jan Beulich
@ 2016-11-15 18:31 ` Andrew Cooper
1 sibling, 0 replies; 76+ messages in thread
From: Andrew Cooper @ 2016-11-15 18:31 UTC (permalink / raw)
To: Boris Ostrovsky, Jan Beulich
Cc: wei.liu2, ian.jackson, xen-devel, Julien Grall, Paul Durrant,
roger.pau
On 15/11/16 16:58, Boris Ostrovsky wrote:
> On 11/15/2016 11:33 AM, Jan Beulich wrote:
>>>>> On 15.11.16 at 17:23, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 10:53 AM, Jan Beulich wrote:
>>>>>>> On 15.11.16 at 16:41, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 11/15/2016 10:13 AM, Jan Beulich wrote:
>>>>>>>>> On 15.11.16 at 15:47, <boris.ostrovsky@oracle.com> wrote:
>>>>>>> On 11/15/2016 03:47 AM, Jan Beulich wrote:
>>>>>>>>> --- a/tools/libacpi/static_tables.c
>>>>>>>>> +++ b/tools/libacpi/static_tables.c
>>>>>>>>> @@ -20,6 +20,8 @@
>>>>>>>>> * Firmware ACPI Control Structure (FACS).
>>>>>>>>> */
>>>>>>>>>
>>>>>>>>> +#define ACPI_REG_BIT_OFFSET 0
>>>>>>>> Can you completely exclude us ever wanting to support something
>>>>>>>> that's not on a byte boundary? I think there was a good reason ...
>>>>>>>>
>>>>>>>>> @@ -30,14 +32,6 @@ struct acpi_20_facs Facs = {
>>>>>>>>> /*
>>>>>>>>> * Fixed ACPI Description Table (FADT).
>>>>>>>>> */
>>>>>>>>> -
>>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_WIDTH 0x20
>>>>>>>>> -#define ACPI_PM1A_EVT_BLK_BIT_OFFSET 0x00
>>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_WIDTH 0x10
>>>>>>>>> -#define ACPI_PM1A_CNT_BLK_BIT_OFFSET 0x00
>>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_WIDTH 0x20
>>>>>>>>> -#define ACPI_PM_TMR_BLK_BIT_OFFSET 0x00
>>>>>>>> ... these specified both width and offset.
>>>>>>> Since OFFSET is not used anywhere I kept it local to static_tables.c. I
>>>>>>> can restore these macros per block and move them to public header but...
>>>>>>>
>>>>>>>>> --- a/xen/include/public/hvm/ioreq.h
>>>>>>>>> +++ b/xen/include/public/hvm/ioreq.h
>>>>>>>>> @@ -24,6 +24,8 @@
>>>>>>>>> #ifndef _IOREQ_H_
>>>>>>>>> #define _IOREQ_H_
>>>>>>>>>
>>>>>>>>> +#include "hvm_info_table.h" /* HVM_MAX_VCPUS */
>>>>>>>>> +
>>>>>>>>> #define IOREQ_READ 1
>>>>>>>>> #define IOREQ_WRITE 0
>>>>>>>>>
>>>>>>>>> @@ -124,6 +126,17 @@ typedef struct buffered_iopage buffered_iopage_t;
>>>>>>>>> #define ACPI_GPE0_BLK_ADDRESS ACPI_GPE0_BLK_ADDRESS_V0
>>>>>>>>> #define ACPI_GPE0_BLK_LEN ACPI_GPE0_BLK_LEN_V0
>>>>>>>>>
>>>>>>>>> +#define ACPI_PM1A_EVT_BLK_LEN 0x04
>>>>>>>>> +#define ACPI_PM1A_CNT_BLK_LEN 0x02
>>>>>>>>> +#define ACPI_PM_TMR_BLK_LEN 0x04
>>>>>>>> Just like ACPI_GPE0_BLK_LEN these should really go next to their
>>>>>>>> address definitions.
>>>>>>> ... together with this, it will make it rather messy/unsightly to go
>>>>>>> with Andrew's request to ifdef this with __XEN__/__XEN_TOOLS__.
>>>>>> Well, framing them that way is a good excuse for having them
>>>>>> separate from the others. In fact, however, the others also
>>>>>> should get hidden in the same way, just that we would need to
>>>>>> be more careful there (read: make the condition also check
>>>>>> __XEN_INTERFACE_VERSION__).
>>>>> Sorry, I don't follow this. How can interface version help here?
>>>> We can't outright remove existing definitions from the public interface,
>>>> but we can limit their exposure to old consumers.
>>> But don't we need to support both V0 and V1 as long as qemu-trad is
>>> supported? In other words, checking interface version won't limit the
>>> scope at this point.
>> Doesn't qemu-trad set __XEN_TOOLS__?
> Oh, so you meant that interface version would be an OR, in addition to
> __XEN__ and __XEN_TOOLS__?
>
>>>>>>>> Provided we really want to hard code further
>>>>>>>> values here in the first place, which I don't think we should. The
>>>>>>>> goal should rather be for all these hard coded values to go away
>>>>>>>> (which really should have happened when the V1 variants had
>>>>>>>> been added).
>>>>>>> How can we not hardcode this if the values should match those in FADT
>>>>>>> (i.e. static_tables.c)?
>>>>>> By having the loading entity obtain the dynamic values and adjust
>>>>>> the table(s) accordingly.
>>>>> And this. Which loading entity (ACPI builder?) and how would it adjust
>>>>> the addresses? It still needs those addresses defined somewhere. And the
>>>>> the hypervisor, which can't parse guest FADT, needs to get those addresses.
>>>> Didn't Andrew make quite clear that there needs to be a central
>>>> authority assigning guest resources? That's where the values
>>>> would come from, and they would need to be suitably propagated
>>>> to however is in need of knowing them.
>>> Oh, but that is still (way?) off at this point. From what I understood
>>> about Andrew's proposal this will require fairly significant update of
>>> how regions are registered.
>> Well, perhaps. Yet I question whether it's a good idea to add another
>> fixed address right now, instead of switching over first.
>
> I think getting that framework in order would be out of the scope of
> what this series is trying to achieve.
Yes - adding the central authority framework is out of scope, because it
is a lot of work on its own to do.
All I want to ensure is that no hardcoded numbers get published in a
stable place in the API. i.e. once the central management work is done,
I want these defines to disappear completely from the header file.
Stuff behind __TOOLS__ is safe to modify later.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
2016-11-09 14:39 ` [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus Boris Ostrovsky
2016-11-09 14:39 ` [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-11 19:57 ` Konrad Rzeszutek Wilk
2016-11-09 14:39 ` [PATCH v2 04/11] acpi: Make pmtimer optional in FADT Boris Ostrovsky
` (7 subsequent siblings)
10 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
ACPI builder marks VCPUS set in vcpu_online map as enabled in MADT.
With ACPI-based CPU hotplug we only want VCPUs that are started by
the guest to be marked as such. Remaining VCPUs will be set to
"enable" by AML code during hotplug.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* Clarified in the commit message that it's AML that updates MADT
tools/libxl/libxl_x86_acpi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
index ff0e2df..949f555 100644
--- a/tools/libxl/libxl_x86_acpi.c
+++ b/tools/libxl/libxl_x86_acpi.c
@@ -98,7 +98,7 @@ static int init_acpi_config(libxl__gc *gc,
uint32_t domid = dom->guest_domid;
xc_dominfo_t info;
struct hvm_info_table *hvminfo;
- int i, rc = 0;
+ int rc = 0;
config->dsdt_anycpu = config->dsdt_15cpu = dsdt_pvh;
config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_pvh_len;
@@ -144,8 +144,8 @@ static int init_acpi_config(libxl__gc *gc,
hvminfo->nr_vcpus = info.max_vcpu_id + 1;
}
- for (i = 0; i < hvminfo->nr_vcpus; i++)
- hvminfo->vcpu_online[i / 8] |= 1 << (i & 7);
+ memcpy(hvminfo->vcpu_online, b_info->avail_vcpus.map,
+ b_info->avail_vcpus.size);
config->hvminfo = hvminfo;
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus
2016-11-09 14:39 ` [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus Boris Ostrovsky
@ 2016-11-11 19:57 ` Konrad Rzeszutek Wilk
2016-11-12 15:40 ` Wei Liu
0 siblings, 1 reply; 76+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-11 19:57 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, jbeulich,
roger.pau
On Wed, Nov 09, 2016 at 09:39:51AM -0500, Boris Ostrovsky wrote:
> ACPI builder marks VCPUS set in vcpu_online map as enabled in MADT.
> With ACPI-based CPU hotplug we only want VCPUs that are started by
> the guest to be marked as such. Remaining VCPUs will be set to
> "enable" by AML code during hotplug.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Changes in v2:
> * Clarified in the commit message that it's AML that updates MADT
>
> tools/libxl/libxl_x86_acpi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_x86_acpi.c b/tools/libxl/libxl_x86_acpi.c
> index ff0e2df..949f555 100644
> --- a/tools/libxl/libxl_x86_acpi.c
> +++ b/tools/libxl/libxl_x86_acpi.c
> @@ -98,7 +98,7 @@ static int init_acpi_config(libxl__gc *gc,
> uint32_t domid = dom->guest_domid;
> xc_dominfo_t info;
> struct hvm_info_table *hvminfo;
> - int i, rc = 0;
> + int rc = 0;
>
> config->dsdt_anycpu = config->dsdt_15cpu = dsdt_pvh;
> config->dsdt_anycpu_len = config->dsdt_15cpu_len = dsdt_pvh_len;
> @@ -144,8 +144,8 @@ static int init_acpi_config(libxl__gc *gc,
> hvminfo->nr_vcpus = info.max_vcpu_id + 1;
> }
>
> - for (i = 0; i < hvminfo->nr_vcpus; i++)
> - hvminfo->vcpu_online[i / 8] |= 1 << (i & 7);
> + memcpy(hvminfo->vcpu_online, b_info->avail_vcpus.map,
> + b_info->avail_vcpus.size);
>
> config->hvminfo = hvminfo;
>
> --
> 2.7.4
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus
2016-11-11 19:57 ` Konrad Rzeszutek Wilk
@ 2016-11-12 15:40 ` Wei Liu
0 siblings, 0 replies; 76+ messages in thread
From: Wei Liu @ 2016-11-12 15:40 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, jbeulich,
Boris Ostrovsky, roger.pau
On Fri, Nov 11, 2016 at 02:57:58PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Nov 09, 2016 at 09:39:51AM -0500, Boris Ostrovsky wrote:
> > ACPI builder marks VCPUS set in vcpu_online map as enabled in MADT.
> > With ACPI-based CPU hotplug we only want VCPUs that are started by
> > the guest to be marked as such. Remaining VCPUs will be set to
> > "enable" by AML code during hotplug.
> >
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 04/11] acpi: Make pmtimer optional in FADT
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (2 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-15 8:49 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests Boris Ostrovsky
` (6 subsequent siblings)
10 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
PM timer is not supported by PVH guests.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/firmware/hvmloader/util.c | 3 ++-
tools/libacpi/build.c | 5 +++++
tools/libacpi/libacpi.h | 1 +
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 6e0cfe7..1d78973 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -948,7 +948,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
if ( !strncmp(xenstore_read("platform/acpi_s4", "1"), "1", 1) )
config->table_flags |= ACPI_HAS_SSDT_S4;
- config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC | ACPI_HAS_WAET);
+ config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
+ ACPI_HAS_WAET | ACPI_HAS_PMTIMER);
config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 47dae01..58822d3 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -574,6 +574,11 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
fadt = ctxt->mem_ops.alloc(ctxt, sizeof(struct acpi_20_fadt), 16);
if (!fadt) goto oom;
+ if ( !(config->table_flags & ACPI_HAS_PMTIMER) )
+ {
+ Fadt.pm_tmr_blk = 0;
+ memset(&Fadt.x_pm_tmr_blk, 0, sizeof(Fadt.x_pm_tmr_blk));
+ }
memcpy(fadt, &Fadt, sizeof(struct acpi_20_fadt));
fadt->dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
fadt->x_dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index 1d388f9..bda692e 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -30,6 +30,7 @@
#define ACPI_HAS_TCPA (1<<7)
#define ACPI_HAS_IOAPIC (1<<8)
#define ACPI_HAS_WAET (1<<9)
+#define ACPI_HAS_PMTIMER (1<<10)
struct xen_vmemrange;
struct acpi_numa {
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (3 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 04/11] acpi: Make pmtimer optional in FADT Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-11 19:56 ` Konrad Rzeszutek Wilk
2016-11-15 8:54 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 06/11] acpi: PVH guests need _E02 method Boris Ostrovsky
` (5 subsequent siblings)
10 siblings, 2 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* HVM guests continue having the buttons
tools/firmware/hvmloader/util.c | 3 ++-
tools/libacpi/build.c | 2 ++
tools/libacpi/libacpi.h | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
index 1d78973..a3f12fe 100644
--- a/tools/firmware/hvmloader/util.c
+++ b/tools/firmware/hvmloader/util.c
@@ -949,7 +949,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
config->table_flags |= ACPI_HAS_SSDT_S4;
config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
- ACPI_HAS_WAET | ACPI_HAS_PMTIMER);
+ ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
+ ACPI_HAS_BUTTONS);
config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
index 58822d3..5c4a818 100644
--- a/tools/libacpi/build.c
+++ b/tools/libacpi/build.c
@@ -579,6 +579,8 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
Fadt.pm_tmr_blk = 0;
memset(&Fadt.x_pm_tmr_blk, 0, sizeof(Fadt.x_pm_tmr_blk));
}
+ if ( !(config->table_flags & ACPI_HAS_BUTTONS) )
+ Fadt.flags |= (ACPI_PWR_BUTTON | ACPI_SLP_BUTTON);
memcpy(fadt, &Fadt, sizeof(struct acpi_20_fadt));
fadt->dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
fadt->x_dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
index bda692e..dd6ef8b 100644
--- a/tools/libacpi/libacpi.h
+++ b/tools/libacpi/libacpi.h
@@ -31,6 +31,7 @@
#define ACPI_HAS_IOAPIC (1<<8)
#define ACPI_HAS_WAET (1<<9)
#define ACPI_HAS_PMTIMER (1<<10)
+#define ACPI_HAS_BUTTONS (1<<11)
struct xen_vmemrange;
struct acpi_numa {
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests
2016-11-09 14:39 ` [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests Boris Ostrovsky
@ 2016-11-11 19:56 ` Konrad Rzeszutek Wilk
2016-11-15 8:54 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-11 19:56 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, jbeulich,
roger.pau
On Wed, Nov 09, 2016 at 09:39:53AM -0500, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Changes in v2:
> * HVM guests continue having the buttons
>
> tools/firmware/hvmloader/util.c | 3 ++-
> tools/libacpi/build.c | 2 ++
> tools/libacpi/libacpi.h | 1 +
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index 1d78973..a3f12fe 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -949,7 +949,8 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
> config->table_flags |= ACPI_HAS_SSDT_S4;
>
> config->table_flags |= (ACPI_HAS_TCPA | ACPI_HAS_IOAPIC |
> - ACPI_HAS_WAET | ACPI_HAS_PMTIMER);
> + ACPI_HAS_WAET | ACPI_HAS_PMTIMER |
> + ACPI_HAS_BUTTONS);
>
> config->tis_hdr = (uint16_t *)ACPI_TIS_HDR_ADDRESS;
>
> diff --git a/tools/libacpi/build.c b/tools/libacpi/build.c
> index 58822d3..5c4a818 100644
> --- a/tools/libacpi/build.c
> +++ b/tools/libacpi/build.c
> @@ -579,6 +579,8 @@ int acpi_build_tables(struct acpi_ctxt *ctxt, struct acpi_config *config)
> Fadt.pm_tmr_blk = 0;
> memset(&Fadt.x_pm_tmr_blk, 0, sizeof(Fadt.x_pm_tmr_blk));
> }
> + if ( !(config->table_flags & ACPI_HAS_BUTTONS) )
> + Fadt.flags |= (ACPI_PWR_BUTTON | ACPI_SLP_BUTTON);
> memcpy(fadt, &Fadt, sizeof(struct acpi_20_fadt));
> fadt->dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
> fadt->x_dsdt = ctxt->mem_ops.v2p(ctxt, dsdt);
> diff --git a/tools/libacpi/libacpi.h b/tools/libacpi/libacpi.h
> index bda692e..dd6ef8b 100644
> --- a/tools/libacpi/libacpi.h
> +++ b/tools/libacpi/libacpi.h
> @@ -31,6 +31,7 @@
> #define ACPI_HAS_IOAPIC (1<<8)
> #define ACPI_HAS_WAET (1<<9)
> #define ACPI_HAS_PMTIMER (1<<10)
> +#define ACPI_HAS_BUTTONS (1<<11)
>
> struct xen_vmemrange;
> struct acpi_numa {
> --
> 2.7.4
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests
2016-11-09 14:39 ` [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests Boris Ostrovsky
2016-11-11 19:56 ` Konrad Rzeszutek Wilk
@ 2016-11-15 8:54 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 8:54 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 06/11] acpi: PVH guests need _E02 method
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (4 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-11 19:58 ` Konrad Rzeszutek Wilk
2016-11-15 8:57 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses Boris Ostrovsky
` (4 subsequent siblings)
10 siblings, 2 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
This is the method that will get invoked on an SCI.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
tools/libacpi/mk_dsdt.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index 2b8234d..780783e 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -282,11 +282,6 @@ int main(int argc, char **argv)
pop_block();
- if (dm_version == QEMU_NONE) {
- pop_block();
- return 0;
- }
-
/* Define GPE control method. */
push_block("Scope", "\\_GPE");
push_block("Method",
@@ -294,6 +289,11 @@ int main(int argc, char **argv)
stmt("\\_SB.PRSC ()", NULL);
pop_block();
pop_block();
+
+ if (dm_version == QEMU_NONE) {
+ pop_block();
+ return 0;
+ }
/**** Processor end ****/
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 06/11] acpi: PVH guests need _E02 method
2016-11-09 14:39 ` [PATCH v2 06/11] acpi: PVH guests need _E02 method Boris Ostrovsky
@ 2016-11-11 19:58 ` Konrad Rzeszutek Wilk
2016-11-15 8:57 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-11 19:58 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, jbeulich,
roger.pau
On Wed, Nov 09, 2016 at 09:39:54AM -0500, Boris Ostrovsky wrote:
> This is the method that will get invoked on an SCI.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> tools/libacpi/mk_dsdt.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
> index 2b8234d..780783e 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -282,11 +282,6 @@ int main(int argc, char **argv)
>
> pop_block();
>
> - if (dm_version == QEMU_NONE) {
> - pop_block();
> - return 0;
> - }
> -
> /* Define GPE control method. */
> push_block("Scope", "\\_GPE");
> push_block("Method",
> @@ -294,6 +289,11 @@ int main(int argc, char **argv)
> stmt("\\_SB.PRSC ()", NULL);
> pop_block();
> pop_block();
> +
> + if (dm_version == QEMU_NONE) {
> + pop_block();
> + return 0;
> + }
> /**** Processor end ****/
>
>
> --
> 2.7.4
>
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 06/11] acpi: PVH guests need _E02 method
2016-11-09 14:39 ` [PATCH v2 06/11] acpi: PVH guests need _E02 method Boris Ostrovsky
2016-11-11 19:58 ` Konrad Rzeszutek Wilk
@ 2016-11-15 8:57 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 8:57 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> This is the method that will get invoked on an SCI.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (5 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 06/11] acpi: PVH guests need _E02 method Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-09 14:56 ` Paul Durrant
` (2 more replies)
2016-11-09 14:39 ` [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests Boris Ostrovsky
` (3 subsequent siblings)
10 siblings, 3 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, Paul Durrant, jbeulich,
Boris Ostrovsky, roger.pau
PVH guests will have ACPI accesses emulated by the hypervisor
as opposed to QEMU (as is the case for HVM guests)
Support for IOREQ server emulation of CPU hotplug is indicated
by XEN_X86_EMU_IOREQ_CPUHP flag.
Logic for the handler will be provided by a later patch.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
CC: Paul Durrant <paul.durrant@citrix.com>
---
Changes in v2:
* Introduce XEN_X86_EMU_IOREQ_CPUHP, don't set HVM_PARAM_NR_IOREQ_SERVER_PAGES
for PVH guests
* Register IO hadler for PVH from hvm_ioreq_init()
xen/arch/x86/hvm/ioreq.c | 18 ++++++++++++++++++
xen/include/asm-x86/domain.h | 2 ++
xen/include/public/arch-x86/xen.h | 5 ++++-
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index d2245e2..e6ff48f 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1380,6 +1380,12 @@ static int hvm_access_cf8(
return X86EMUL_UNHANDLEABLE;
}
+static int acpi_ioaccess(
+ int dir, unsigned int port, unsigned int bytes, uint32_t *val)
+{
+ return X86EMUL_OKAY;
+}
+
void hvm_ioreq_init(struct domain *d)
{
spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
@@ -1387,6 +1393,18 @@ void hvm_ioreq_init(struct domain *d)
if ( !is_pvh_domain(d) )
register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
+
+ if ( !has_ioreq_cpuhp(d) )
+ {
+ /* Online CPU map, see DSDT's PRST region. */
+ register_portio_handler(d, ACPI_CPU_MAP,
+ ACPI_CPU_MAP_LEN, acpi_ioaccess);
+
+ register_portio_handler(d, ACPI_GPE0_BLK_ADDRESS_V1,
+ ACPI_GPE0_BLK_LEN_V1, acpi_ioaccess);
+ register_portio_handler(d, ACPI_PM1A_EVT_BLK_ADDRESS_V1,
+ ACPI_PM1A_EVT_BLK_LEN, acpi_ioaccess);
+ }
}
/*
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index a279e4a..7aa736c 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -431,6 +431,8 @@ struct arch_domain
#define has_vvga(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_VGA))
#define has_viommu(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_IOMMU))
#define has_vpit(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_PIT))
+#define has_ioreq_cpuhp(d) (!!((d)->arch.emulation_flags & \
+ XEN_X86_EMU_IOREQ_CPUHP))
#define has_arch_pdevs(d) (!list_empty(&(d)->arch.pdev_list))
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index cdd93c1..350bc66 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -283,12 +283,15 @@ struct xen_arch_domainconfig {
#define XEN_X86_EMU_IOMMU (1U<<_XEN_X86_EMU_IOMMU)
#define _XEN_X86_EMU_PIT 8
#define XEN_X86_EMU_PIT (1U<<_XEN_X86_EMU_PIT)
+#define _XEN_X86_EMU_IOREQ_CPUHP 9
+#define XEN_X86_EMU_IOREQ_CPUHP (1U<<_XEN_X86_EMU_IOREQ_CPUHP)
#define XEN_X86_EMU_ALL (XEN_X86_EMU_LAPIC | XEN_X86_EMU_HPET | \
XEN_X86_EMU_PM | XEN_X86_EMU_RTC | \
XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC | \
XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | \
- XEN_X86_EMU_PIT)
+ XEN_X86_EMU_PIT | \
+ XEN_X86_EMU_IOREQ_CPUHP)
uint32_t emulation_flags;
};
#endif
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
2016-11-09 14:39 ` [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses Boris Ostrovsky
@ 2016-11-09 14:56 ` Paul Durrant
2016-11-11 20:01 ` Konrad Rzeszutek Wilk
2016-11-15 9:04 ` Jan Beulich
2 siblings, 0 replies; 76+ messages in thread
From: Paul Durrant @ 2016-11-09 14:56 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel@lists.xen.org
Cc: Andrew Cooper, Roger Pau Monne, Wei Liu, jbeulich@suse.com,
Ian Jackson
> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 09 November 2016 14:40
> To: xen-devel@lists.xen.org
> Cc: jbeulich@suse.com; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO
> accesses
>
> PVH guests will have ACPI accesses emulated by the hypervisor
> as opposed to QEMU (as is the case for HVM guests)
>
> Support for IOREQ server emulation of CPU hotplug is indicated
> by XEN_X86_EMU_IOREQ_CPUHP flag.
>
> Logic for the handler will be provided by a later patch.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changes in v2:
> * Introduce XEN_X86_EMU_IOREQ_CPUHP, don't set
> HVM_PARAM_NR_IOREQ_SERVER_PAGES
> for PVH guests
Is 'CPUHP' the right name? The same GPE block could be used for DIMM and PCI hotplug, right? That aside...
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> * Register IO hadler for PVH from hvm_ioreq_init()
>
> xen/arch/x86/hvm/ioreq.c | 18 ++++++++++++++++++
> xen/include/asm-x86/domain.h | 2 ++
> xen/include/public/arch-x86/xen.h | 5 ++++-
> 3 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index d2245e2..e6ff48f 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1380,6 +1380,12 @@ static int hvm_access_cf8(
> return X86EMUL_UNHANDLEABLE;
> }
>
> +static int acpi_ioaccess(
> + int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> +{
> + return X86EMUL_OKAY;
> +}
> +
> void hvm_ioreq_init(struct domain *d)
> {
> spin_lock_init(&d->arch.hvm_domain.ioreq_server.lock);
> @@ -1387,6 +1393,18 @@ void hvm_ioreq_init(struct domain *d)
>
> if ( !is_pvh_domain(d) )
> register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +
> + if ( !has_ioreq_cpuhp(d) )
> + {
> + /* Online CPU map, see DSDT's PRST region. */
> + register_portio_handler(d, ACPI_CPU_MAP,
> + ACPI_CPU_MAP_LEN, acpi_ioaccess);
> +
> + register_portio_handler(d, ACPI_GPE0_BLK_ADDRESS_V1,
> + ACPI_GPE0_BLK_LEN_V1, acpi_ioaccess);
> + register_portio_handler(d, ACPI_PM1A_EVT_BLK_ADDRESS_V1,
> + ACPI_PM1A_EVT_BLK_LEN, acpi_ioaccess);
> + }
> }
>
> /*
> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index a279e4a..7aa736c 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -431,6 +431,8 @@ struct arch_domain
> #define has_vvga(d) (!!((d)->arch.emulation_flags &
> XEN_X86_EMU_VGA))
> #define has_viommu(d) (!!((d)->arch.emulation_flags &
> XEN_X86_EMU_IOMMU))
> #define has_vpit(d) (!!((d)->arch.emulation_flags &
> XEN_X86_EMU_PIT))
> +#define has_ioreq_cpuhp(d) (!!((d)->arch.emulation_flags & \
> + XEN_X86_EMU_IOREQ_CPUHP))
>
> #define has_arch_pdevs(d) (!list_empty(&(d)->arch.pdev_list))
>
> diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-
> x86/xen.h
> index cdd93c1..350bc66 100644
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -283,12 +283,15 @@ struct xen_arch_domainconfig {
> #define XEN_X86_EMU_IOMMU (1U<<_XEN_X86_EMU_IOMMU)
> #define _XEN_X86_EMU_PIT 8
> #define XEN_X86_EMU_PIT (1U<<_XEN_X86_EMU_PIT)
> +#define _XEN_X86_EMU_IOREQ_CPUHP 9
> +#define XEN_X86_EMU_IOREQ_CPUHP
> (1U<<_XEN_X86_EMU_IOREQ_CPUHP)
>
> #define XEN_X86_EMU_ALL (XEN_X86_EMU_LAPIC |
> XEN_X86_EMU_HPET | \
> XEN_X86_EMU_PM | XEN_X86_EMU_RTC | \
> XEN_X86_EMU_IOAPIC | XEN_X86_EMU_PIC | \
> XEN_X86_EMU_VGA | XEN_X86_EMU_IOMMU | \
> - XEN_X86_EMU_PIT)
> + XEN_X86_EMU_PIT | \
> + XEN_X86_EMU_IOREQ_CPUHP)
> uint32_t emulation_flags;
> };
> #endif
> --
> 2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
2016-11-09 14:39 ` [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses Boris Ostrovsky
2016-11-09 14:56 ` Paul Durrant
@ 2016-11-11 20:01 ` Konrad Rzeszutek Wilk
2016-11-15 9:04 ` Jan Beulich
2 siblings, 0 replies; 76+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-11 20:01 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
jbeulich, roger.pau
On Wed, Nov 09, 2016 at 09:39:55AM -0500, Boris Ostrovsky wrote:
> PVH guests will have ACPI accesses emulated by the hypervisor
> as opposed to QEMU (as is the case for HVM guests)
>
> Support for IOREQ server emulation of CPU hotplug is indicated
> by XEN_X86_EMU_IOREQ_CPUHP flag.
>
> Logic for the handler will be provided by a later patch.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
2016-11-09 14:39 ` [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses Boris Ostrovsky
2016-11-09 14:56 ` Paul Durrant
2016-11-11 20:01 ` Konrad Rzeszutek Wilk
@ 2016-11-15 9:04 ` Jan Beulich
2 siblings, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 9:04 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1380,6 +1380,12 @@ static int hvm_access_cf8(
> return X86EMUL_UNHANDLEABLE;
> }
>
> +static int acpi_ioaccess(
> + int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> +{
> + return X86EMUL_OKAY;
How can you return success here without doing anything, not
even setting *val in case of a read?
> @@ -1387,6 +1393,18 @@ void hvm_ioreq_init(struct domain *d)
>
> if ( !is_pvh_domain(d) )
> register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +
> + if ( !has_ioreq_cpuhp(d) )
> + {
> + /* Online CPU map, see DSDT's PRST region. */
> + register_portio_handler(d, ACPI_CPU_MAP,
> + ACPI_CPU_MAP_LEN, acpi_ioaccess);
> +
> + register_portio_handler(d, ACPI_GPE0_BLK_ADDRESS_V1,
> + ACPI_GPE0_BLK_LEN_V1, acpi_ioaccess);
> + register_portio_handler(d, ACPI_PM1A_EVT_BLK_ADDRESS_V1,
> + ACPI_PM1A_EVT_BLK_LEN, acpi_ioaccess);
> + }
Isn't the condition inverted?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (6 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-09 14:58 ` Paul Durrant
` (2 more replies)
2016-11-09 14:39 ` [PATCH v2 09/11] events/x86: Define SCI virtual interrupt Boris Ostrovsky
` (2 subsequent siblings)
10 siblings, 3 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, Paul Durrant, jbeulich,
Boris Ostrovsky, roger.pau
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
CC: Paul Durrant <paul.durrant@citrix.com>
---
Changes in v2:
* Use 'true/false' values for bools
xen/arch/x86/hvm/ioreq.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index e6ff48f..3ef01cf 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1383,6 +1383,78 @@ static int hvm_access_cf8(
static int acpi_ioaccess(
int dir, unsigned int port, unsigned int bytes, uint32_t *val)
{
+ unsigned int i;
+ unsigned int bits = bytes * 8;
+ unsigned int idx = port & 3;
+ uint8_t *reg = NULL;
+ bool is_cpu_map = false;
+ struct domain *currd = current->domain;
+
+ BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
+ (ACPI_GPE0_BLK_LEN_V1 != 4));
+
+ if ( has_ioreq_cpuhp(currd) )
+ return X86EMUL_UNHANDLEABLE;
+
+ switch (port)
+ {
+ case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
+ (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + ACPI_PM1A_EVT_BLK_LEN - 1):
+ reg = currd->arch.hvm_domain.acpi_io.pm1a;
+ break;
+ case ACPI_GPE0_BLK_ADDRESS_V1 ...
+ (ACPI_GPE0_BLK_ADDRESS_V1 + ACPI_GPE0_BLK_LEN_V1 - 1):
+ reg = currd->arch.hvm_domain.acpi_io.gpe;
+ break;
+ case ACPI_CPU_MAP ... (ACPI_CPU_MAP + ACPI_CPU_MAP_LEN - 1):
+ is_cpu_map = true;
+ break;
+ default:
+ return X86EMUL_UNHANDLEABLE;
+ }
+
+ if ( bytes == 0 )
+ return X86EMUL_OKAY;
+
+ if ( dir == IOREQ_READ )
+ {
+ *val &= ~((1U << bits) - 1);
+
+ if ( is_cpu_map )
+ {
+ unsigned int first_bit, last_bit;
+
+ first_bit = (port - ACPI_CPU_MAP) * 8;
+ last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
+ for ( i = first_bit; i < last_bit; i++ )
+ *val |= (1U << (i - first_bit));
+ }
+ else
+ memcpy(val, ®[idx], bytes);
+ }
+ else
+ {
+ if ( is_cpu_map )
+ /*
+ * CPU map is only read by DSDT's PRSC method and should never
+ * be written by a guest.
+ */
+ return X86EMUL_UNHANDLEABLE;
+
+ /* Write either status or enable reegister. */
+ if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
+ return X86EMUL_UNHANDLEABLE;
+
+ if ( idx < 2 ) /* status, write 1 to clear. */
+ {
+ reg[idx] &= ~(*val & 0xff);
+ if ( bytes == 2 )
+ reg[idx + 1] &= ~((*val >> 8) & 0xff);
+ }
+ else /* enable */
+ memcpy(®[idx], val, bytes);
+ }
+
return X86EMUL_OKAY;
}
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-09 14:39 ` [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests Boris Ostrovsky
@ 2016-11-09 14:58 ` Paul Durrant
2016-11-11 20:07 ` Konrad Rzeszutek Wilk
2016-11-15 9:24 ` Jan Beulich
2 siblings, 0 replies; 76+ messages in thread
From: Paul Durrant @ 2016-11-09 14:58 UTC (permalink / raw)
To: Boris Ostrovsky, xen-devel@lists.xen.org
Cc: Andrew Cooper, Roger Pau Monne, Wei Liu, jbeulich@suse.com,
Ian Jackson
> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 09 November 2016 14:40
> To: xen-devel@lists.xen.org
> Cc: jbeulich@suse.com; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> Wei Liu <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Boris Ostrovsky
> <boris.ostrovsky@oracle.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changes in v2:
> * Use 'true/false' values for bools
>
>
> xen/arch/x86/hvm/ioreq.c | 72
> ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index e6ff48f..3ef01cf 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(
> static int acpi_ioaccess(
> int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> {
> + unsigned int i;
> + unsigned int bits = bytes * 8;
> + unsigned int idx = port & 3;
> + uint8_t *reg = NULL;
> + bool is_cpu_map = false;
> + struct domain *currd = current->domain;
> +
> + BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> + (ACPI_GPE0_BLK_LEN_V1 != 4));
> +
> + if ( has_ioreq_cpuhp(currd) )
> + return X86EMUL_UNHANDLEABLE;
> +
> + switch (port)
> + {
> + case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
> + (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + ACPI_PM1A_EVT_BLK_LEN - 1):
> + reg = currd->arch.hvm_domain.acpi_io.pm1a;
> + break;
> + case ACPI_GPE0_BLK_ADDRESS_V1 ...
> + (ACPI_GPE0_BLK_ADDRESS_V1 + ACPI_GPE0_BLK_LEN_V1 - 1):
> + reg = currd->arch.hvm_domain.acpi_io.gpe;
> + break;
> + case ACPI_CPU_MAP ... (ACPI_CPU_MAP + ACPI_CPU_MAP_LEN - 1):
> + is_cpu_map = true;
> + break;
> + default:
> + return X86EMUL_UNHANDLEABLE;
> + }
> +
> + if ( bytes == 0 )
> + return X86EMUL_OKAY;
> +
> + if ( dir == IOREQ_READ )
> + {
> + *val &= ~((1U << bits) - 1);
> +
> + if ( is_cpu_map )
> + {
> + unsigned int first_bit, last_bit;
> +
> + first_bit = (port - ACPI_CPU_MAP) * 8;
> + last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
> + for ( i = first_bit; i < last_bit; i++ )
> + *val |= (1U << (i - first_bit));
> + }
> + else
> + memcpy(val, ®[idx], bytes);
> + }
> + else
> + {
> + if ( is_cpu_map )
> + /*
> + * CPU map is only read by DSDT's PRSC method and should never
> + * be written by a guest.
> + */
> + return X86EMUL_UNHANDLEABLE;
> +
> + /* Write either status or enable reegister. */
> + if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
> + return X86EMUL_UNHANDLEABLE;
> +
> + if ( idx < 2 ) /* status, write 1 to clear. */
> + {
> + reg[idx] &= ~(*val & 0xff);
> + if ( bytes == 2 )
> + reg[idx + 1] &= ~((*val >> 8) & 0xff);
> + }
> + else /* enable */
> + memcpy(®[idx], val, bytes);
> + }
> +
> return X86EMUL_OKAY;
> }
>
> --
> 2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-09 14:39 ` [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests Boris Ostrovsky
2016-11-09 14:58 ` Paul Durrant
@ 2016-11-11 20:07 ` Konrad Rzeszutek Wilk
2016-11-15 9:24 ` Jan Beulich
2 siblings, 0 replies; 76+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-11-11 20:07 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
jbeulich, roger.pau
On Wed, Nov 09, 2016 at 09:39:56AM -0500, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
> Changes in v2:
> * Use 'true/false' values for bools
>
>
> xen/arch/x86/hvm/ioreq.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index e6ff48f..3ef01cf 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(
> static int acpi_ioaccess(
> int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> {
> + unsigned int i;
> + unsigned int bits = bytes * 8;
> + unsigned int idx = port & 3;
> + uint8_t *reg = NULL;
> + bool is_cpu_map = false;
> + struct domain *currd = current->domain;
> +
> + BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> + (ACPI_GPE0_BLK_LEN_V1 != 4));
> +
> + if ( has_ioreq_cpuhp(currd) )
> + return X86EMUL_UNHANDLEABLE;
> +
> + switch (port)
Spaces around 'port'
otherwise Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-09 14:39 ` [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests Boris Ostrovsky
2016-11-09 14:58 ` Paul Durrant
2016-11-11 20:07 ` Konrad Rzeszutek Wilk
@ 2016-11-15 9:24 ` Jan Beulich
2016-11-15 14:55 ` Boris Ostrovsky
2 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 9:24 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(static int acpi_ioaccess(
> int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> {
> + unsigned int i;
> + unsigned int bits = bytes * 8;
> + unsigned int idx = port & 3;
> + uint8_t *reg = NULL;
> + bool is_cpu_map = false;
> + struct domain *currd = current->domain;
> +
> + BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> + (ACPI_GPE0_BLK_LEN_V1 != 4));
> +
> + if ( has_ioreq_cpuhp(currd) )
> + return X86EMUL_UNHANDLEABLE;
Hmm, so it seems you indeed mean the flag to have the inverse sense
of what I would have expected, presumably in order for HVM guests
to continue to have all emulation flags set. I think that's a little
unfortunate, or at least the name of flag and predicate are somewhat
misleading (as there's no specific CPU hotplug related ioreq).
In any event, with the handler getting registered only when
!has_ioreq_cpuhp() I think this should be an ASSERT() - iirc the
emulation flags can be set only once at domain creation time.
> + switch (port)
> + {
> + case ACPI_PM1A_EVT_BLK_ADDRESS_V1 ...
> + (ACPI_PM1A_EVT_BLK_ADDRESS_V1 + ACPI_PM1A_EVT_BLK_LEN - 1):
Please align the opening parenthesis with the respective expression
on the previous line (an really the parentheses aren't needed here,
so you could just align the two starting A-s).
> + reg = currd->arch.hvm_domain.acpi_io.pm1a;
> + break;
> + case ACPI_GPE0_BLK_ADDRESS_V1 ...
> + (ACPI_GPE0_BLK_ADDRESS_V1 + ACPI_GPE0_BLK_LEN_V1 - 1):
> + reg = currd->arch.hvm_domain.acpi_io.gpe;
> + break;
> + case ACPI_CPU_MAP ... (ACPI_CPU_MAP + ACPI_CPU_MAP_LEN - 1):
> + is_cpu_map = true;
> + break;
> + default:
> + return X86EMUL_UNHANDLEABLE;
> + }
Blank lines between non-fall-through case statements please.
> + if ( bytes == 0 )
> + return X86EMUL_OKAY;
> +
> + if ( dir == IOREQ_READ )
> + {
> + *val &= ~((1U << bits) - 1);
Undefined behavior for bits == 32.
> + if ( is_cpu_map )
> + {
> + unsigned int first_bit, last_bit;
> +
> + first_bit = (port - ACPI_CPU_MAP) * 8;
> + last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
> + for ( i = first_bit; i < last_bit; i++ )
> + *val |= (1U << (i - first_bit));
> + }
> + else
> + memcpy(val, ®[idx], bytes);
> + }
> + else
> + {
> + if ( is_cpu_map )
> + /*
> + * CPU map is only read by DSDT's PRSC method and should never
> + * be written by a guest.
> + */
> + return X86EMUL_UNHANDLEABLE;
> +
> + /* Write either status or enable reegister. */
> + if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
> + return X86EMUL_UNHANDLEABLE;
> +
> + if ( idx < 2 ) /* status, write 1 to clear. */
> + {
> + reg[idx] &= ~(*val & 0xff);
> + if ( bytes == 2 )
> + reg[idx + 1] &= ~((*val >> 8) & 0xff);
> + }
> + else /* enable */
> + memcpy(®[idx], val, bytes);
> + }
Overall - how does this implementation match up with the following
requirements from the spec:
● Reserved or unimplemented bits always return zero (control or enable).
● Writes to reserved or unimplemented bits have no affect.
To me it looks as it at this point all bits are reserved/unimplemented.
> return X86EMUL_OKAY;
So regarding my comment on the previous patch: That one should
fail the call unconditionally, and the one here should switch to
returning success.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 9:24 ` Jan Beulich
@ 2016-11-15 14:55 ` Boris Ostrovsky
2016-11-15 15:17 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 14:55 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
On 11/15/2016 04:24 AM, Jan Beulich wrote:
>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(static int acpi_ioaccess(
>> int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>> {
>> + unsigned int i;
>> + unsigned int bits = bytes * 8;
>> + unsigned int idx = port & 3;
>> + uint8_t *reg = NULL;
>> + bool is_cpu_map = false;
>> + struct domain *currd = current->domain;
>> +
>> + BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> + (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> + if ( has_ioreq_cpuhp(currd) )
>> + return X86EMUL_UNHANDLEABLE;
> Hmm, so it seems you indeed mean the flag to have the inverse sense
> of what I would have expected, presumably in order for HVM guests
> to continue to have all emulation flags set. I think that's a little
> unfortunate, or at least the name of flag and predicate are somewhat
> misleading (as there's no specific CPU hotplug related ioreq).
The other option was XEN_X86_EMU_ACPI. Would it be better?
>
>> + if ( is_cpu_map )
>> + {
>> + unsigned int first_bit, last_bit;
>> +
>> + first_bit = (port - ACPI_CPU_MAP) * 8;
>> + last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
>> + for ( i = first_bit; i < last_bit; i++ )
>> + *val |= (1U << (i - first_bit));
>> + }
>> + else
>> + memcpy(val, ®[idx], bytes);
>> + }
>> + else
>> + {
>> + if ( is_cpu_map )
>> + /*
>> + * CPU map is only read by DSDT's PRSC method and should never
>> + * be written by a guest.
>> + */
>> + return X86EMUL_UNHANDLEABLE;
>> +
>> + /* Write either status or enable reegister. */
>> + if ( (bytes > 2) || ((bytes == 2) && (port & 1)) )
>> + return X86EMUL_UNHANDLEABLE;
>> +
>> + if ( idx < 2 ) /* status, write 1 to clear. */
>> + {
>> + reg[idx] &= ~(*val & 0xff);
>> + if ( bytes == 2 )
>> + reg[idx + 1] &= ~((*val >> 8) & 0xff);
>> + }
>> + else /* enable */
>> + memcpy(®[idx], val, bytes);
>> + }
> Overall - how does this implementation match up with the following
> requirements from the spec:
>
> ● Reserved or unimplemented bits always return zero (control or enable).
> ● Writes to reserved or unimplemented bits have no affect.
>
> To me it looks as it at this point all bits are reserved/unimplemented.
We do have one bit that we need --- bit 2 of GPE --- but the rest indeed
looks like it is unused. I'll check whether there are any required buts
to be supported and if not then only checking for bit 2 will make things
slightly simpler here.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 14:55 ` Boris Ostrovsky
@ 2016-11-15 15:17 ` Jan Beulich
2016-11-15 15:44 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 15:17 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
>>> On 15.11.16 at 15:55, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 04:24 AM, Jan Beulich wrote:
>>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/xen/arch/x86/hvm/ioreq.c
>>> +++ b/xen/arch/x86/hvm/ioreq.c
>>> @@ -1383,6 +1383,78 @@ static int hvm_access_cf8(static int acpi_ioaccess(
>>> int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>> {
>>> + unsigned int i;
>>> + unsigned int bits = bytes * 8;
>>> + unsigned int idx = port & 3;
>>> + uint8_t *reg = NULL;
>>> + bool is_cpu_map = false;
>>> + struct domain *currd = current->domain;
>>> +
>>> + BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>>> + (ACPI_GPE0_BLK_LEN_V1 != 4));
>>> +
>>> + if ( has_ioreq_cpuhp(currd) )
>>> + return X86EMUL_UNHANDLEABLE;
>> Hmm, so it seems you indeed mean the flag to have the inverse sense
>> of what I would have expected, presumably in order for HVM guests
>> to continue to have all emulation flags set. I think that's a little
>> unfortunate, or at least the name of flag and predicate are somewhat
>> misleading (as there's no specific CPU hotplug related ioreq).
>
> The other option was XEN_X86_EMU_ACPI. Would it be better?
As that's a little too wide (and I think someone else had also
disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
(for "fixed features"), or if that's still too wide, break things up
(PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 15:17 ` Jan Beulich
@ 2016-11-15 15:44 ` Boris Ostrovsky
2016-11-15 15:56 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 15:44 UTC (permalink / raw)
To: Jan Beulich
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
On 11/15/2016 10:17 AM, Jan Beulich wrote:
>> The other option was XEN_X86_EMU_ACPI. Would it be better?
> As that's a little too wide (and I think someone else had also
> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
> (for "fixed features"), or if that's still too wide, break things up
> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
I think this may be a bit too fine-grained. Fixed-features would be
good, but is GPE block considered part of fixed features?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 15:44 ` Boris Ostrovsky
@ 2016-11-15 15:56 ` Jan Beulich
2016-11-15 19:19 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 15:56 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: wei.liu2, andrew.cooper3, ian.jackson, xen-devel, Paul Durrant,
roger.pau
>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>> As that's a little too wide (and I think someone else had also
>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>> (for "fixed features"), or if that's still too wide, break things up
>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>
> I think this may be a bit too fine-grained. Fixed-features would be
> good, but is GPE block considered part of fixed features?
See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
text ahead of this makes it pretty clear that altogether they're
being called fixed hardware register blocks. So if you consider FF
misleading, FHRB would be another option.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 15:56 ` Jan Beulich
@ 2016-11-15 19:19 ` Andrew Cooper
2016-11-15 19:38 ` Boris Ostrovsky
2016-11-16 9:23 ` Jan Beulich
0 siblings, 2 replies; 76+ messages in thread
From: Andrew Cooper @ 2016-11-15 19:19 UTC (permalink / raw)
To: Jan Beulich, Boris Ostrovsky
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
On 15/11/16 15:56, Jan Beulich wrote:
>>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
>> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>>> As that's a little too wide (and I think someone else had also
>>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>>> (for "fixed features"), or if that's still too wide, break things up
>>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>> I think this may be a bit too fine-grained. Fixed-features would be
>> good, but is GPE block considered part of fixed features?
> See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
> text ahead of this makes it pretty clear that altogether they're
> being called fixed hardware register blocks. So if you consider FF
> misleading, FHRB would be another option.
Please can we also considering a naming appropriate for joint use with
HVM guests as well.
For PVH, (if enabled), Xen handles all (implemented) fixed function
registers.
For HVM, Xen already intercepts and interposes on the PM1a_STS and
PM1a_EN registers heading towards qemu, for the apparent purpose of
raising SCIs on behalf of qemu.
When we want to enable ACPI vcpu hotplug for HVM guests, Xen will have
to maintain the current intercept behaviour for qemu, but also take on
the PM1b role entirely.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 19:19 ` Andrew Cooper
@ 2016-11-15 19:38 ` Boris Ostrovsky
2016-11-15 20:07 ` Andrew Cooper
2016-11-16 9:23 ` Jan Beulich
1 sibling, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 19:38 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
On 11/15/2016 02:19 PM, Andrew Cooper wrote:
> On 15/11/16 15:56, Jan Beulich wrote:
>>>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>>>> As that's a little too wide (and I think someone else had also
>>>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>>>> (for "fixed features"), or if that's still too wide, break things up
>>>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>>> I think this may be a bit too fine-grained. Fixed-features would be
>>> good, but is GPE block considered part of fixed features?
>> See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
>> text ahead of this makes it pretty clear that altogether they're
>> being called fixed hardware register blocks. So if you consider FF
>> misleading, FHRB would be another option.
> Please can we also considering a naming appropriate for joint use with
> HVM guests as well.
>
> For PVH, (if enabled), Xen handles all (implemented) fixed function
> registers.
>
> For HVM, Xen already intercepts and interposes on the PM1a_STS and
> PM1a_EN registers heading towards qemu, for the apparent purpose of
> raising SCIs on behalf of qemu.
>
> When we want to enable ACPI vcpu hotplug for HVM guests,
What do you mean by "when"? We *are* doing ACPI hotplug for HVM guests,
aren't we?
Or are you thinking about moving this functionality to the hypervisor?
-boris
> Xen will have
> to maintain the current intercept behaviour for qemu, but also take on
> the PM1b role entirely.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 19:38 ` Boris Ostrovsky
@ 2016-11-15 20:07 ` Andrew Cooper
2016-11-15 20:20 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Andrew Cooper @ 2016-11-15 20:07 UTC (permalink / raw)
To: Boris Ostrovsky, Jan Beulich
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
On 15/11/16 19:38, Boris Ostrovsky wrote:
> On 11/15/2016 02:19 PM, Andrew Cooper wrote:
>> On 15/11/16 15:56, Jan Beulich wrote:
>>>>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
>>>> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>>>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>>>>> As that's a little too wide (and I think someone else had also
>>>>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>>>>> (for "fixed features"), or if that's still too wide, break things up
>>>>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>>>> I think this may be a bit too fine-grained. Fixed-features would be
>>>> good, but is GPE block considered part of fixed features?
>>> See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
>>> text ahead of this makes it pretty clear that altogether they're
>>> being called fixed hardware register blocks. So if you consider FF
>>> misleading, FHRB would be another option.
>> Please can we also considering a naming appropriate for joint use with
>> HVM guests as well.
>>
>> For PVH, (if enabled), Xen handles all (implemented) fixed function
>> registers.
>>
>> For HVM, Xen already intercepts and interposes on the PM1a_STS and
>> PM1a_EN registers heading towards qemu, for the apparent purpose of
>> raising SCIs on behalf of qemu.
>>
>> When we want to enable ACPI vcpu hotplug for HVM guests,
> What do you mean by "when"? We *are* doing ACPI hotplug for HVM guests,
> aren't we?
Are we? If so, how?
I don't see any toolstack or qemu code able to cope with APCI CPU
hotplug. I can definitely see ACPI PCI hotplug in qemu, but that does
make sense.
> Or are you thinking about moving this functionality to the hypervisor?
As an aside, we need to move some part of PCI hotplug into the
hypervisor longterm. At the moment, any new entity coming along and
attaching to an ioreq server still needs to negotiate with Qemu to make
the device appear. This is awkward but doable if all device models are
in dom0, but is far harder if the device models are in different domains.
As for CPU hotplug, (if I have indeed overlooked something), Qemu has no
business in this matter. The device model exists to be an
implementation of an LPC bridge, and is not responsible for any CPU
related functionality; Xen does all vcpu handling.
The Xen project and community have had a very rich history of hacking
things up in the past, an frankly, it shows. I want to ensure that
development progresses in an architecturally clean and appropriate
direction, especially if this enables us to remove some of the duck tape
holding pre-existing features together.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 20:07 ` Andrew Cooper
@ 2016-11-15 20:20 ` Boris Ostrovsky
2016-11-17 0:00 ` Boris Ostrovsky
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 20:20 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
On 11/15/2016 03:07 PM, Andrew Cooper wrote:
> On 15/11/16 19:38, Boris Ostrovsky wrote:
>> On 11/15/2016 02:19 PM, Andrew Cooper wrote:
>>> On 15/11/16 15:56, Jan Beulich wrote:
>>>>>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
>>>>> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>>>>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>>>>>> As that's a little too wide (and I think someone else had also
>>>>>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>>>>>> (for "fixed features"), or if that's still too wide, break things up
>>>>>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>>>>> I think this may be a bit too fine-grained. Fixed-features would be
>>>>> good, but is GPE block considered part of fixed features?
>>>> See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
>>>> text ahead of this makes it pretty clear that altogether they're
>>>> being called fixed hardware register blocks. So if you consider FF
>>>> misleading, FHRB would be another option.
>>> Please can we also considering a naming appropriate for joint use with
>>> HVM guests as well.
>>>
>>> For PVH, (if enabled), Xen handles all (implemented) fixed function
>>> registers.
>>>
>>> For HVM, Xen already intercepts and interposes on the PM1a_STS and
>>> PM1a_EN registers heading towards qemu, for the apparent purpose of
>>> raising SCIs on behalf of qemu.
>>>
>>> When we want to enable ACPI vcpu hotplug for HVM guests,
>> What do you mean by "when"? We *are* doing ACPI hotplug for HVM guests,
>> aren't we?
> Are we? If so, how?
>
> I don't see any toolstack or qemu code able to cope with APCI CPU
> hotplug. I can definitely see ACPI PCI hotplug in qemu, but that does
> make sense.
piix4_acpi_system_hot_add_init():
acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
PIIX4_CPU_HOTPLUG_IO_BASE);
>
>> Or are you thinking about moving this functionality to the hypervisor?
> As an aside, we need to move some part of PCI hotplug into the
> hypervisor longterm. At the moment, any new entity coming along and
> attaching to an ioreq server still needs to negotiate with Qemu to make
> the device appear. This is awkward but doable if all device models are
> in dom0, but is far harder if the device models are in different domains.
>
> As for CPU hotplug, (if I have indeed overlooked something), Qemu has no
> business in this matter.
Yes. And if we are going to do it for PVH we might as well do it for HVM
--- I think most of the code will be the same, save for how SCI is sent.
-boris
> The device model exists to be an
> implementation of an LPC bridge, and is not responsible for any CPU
> related functionality; Xen does all vcpu handling.
>
>
> The Xen project and community have had a very rich history of hacking
> things up in the past, an frankly, it shows. I want to ensure that
> development progresses in an architecturally clean and appropriate
> direction, especially if this enables us to remove some of the duck tape
> holding pre-existing features together.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 20:20 ` Boris Ostrovsky
@ 2016-11-17 0:00 ` Boris Ostrovsky
2016-11-17 0:08 ` Andrew Cooper
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-17 0:00 UTC (permalink / raw)
To: Andrew Cooper, Jan Beulich
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
>
> When we want to enable ACPI vcpu hotplug for HVM guests,
>>> What do you mean by "when"? We *are* doing ACPI hotplug for HVM guests,
>>> aren't we?
>> Are we? If so, how?
>>
>> I don't see any toolstack or qemu code able to cope with APCI CPU
>> hotplug. I can definitely see ACPI PCI hotplug in qemu, but that does
>> make sense.
> piix4_acpi_system_hot_add_init():
> acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> PIIX4_CPU_HOTPLUG_IO_BASE);
>
>
>>> Or are you thinking about moving this functionality to the hypervisor?
>> As an aside, we need to move some part of PCI hotplug into the
>> hypervisor longterm. At the moment, any new entity coming along and
>> attaching to an ioreq server still needs to negotiate with Qemu to make
>> the device appear. This is awkward but doable if all device models are
>> in dom0, but is far harder if the device models are in different domains.
>>
>> As for CPU hotplug, (if I have indeed overlooked something), Qemu has no
>> business in this matter.
> Yes. And if we are going to do it for PVH we might as well do it for HVM
> --- I think most of the code will be the same, save for how SCI is sent.
So I discovered that we actually cannot unplug an HVM VCPU with qemu,
there is no support for that via QMP (which is what we use).
'xl vcpu-set <domid> N' is nop when we unplug.
-boris
>
> -boris
>
>> The device model exists to be an
>> implementation of an LPC bridge, and is not responsible for any CPU
>> related functionality; Xen does all vcpu handling.
>>
>>
>> The Xen project and community have had a very rich history of hacking
>> things up in the past, an frankly, it shows. I want to ensure that
>> development progresses in an architecturally clean and appropriate
>> direction, especially if this enables us to remove some of the duck tape
>> holding pre-existing features together.
>>
>> ~Andrew
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-17 0:00 ` Boris Ostrovsky
@ 2016-11-17 0:08 ` Andrew Cooper
0 siblings, 0 replies; 76+ messages in thread
From: Andrew Cooper @ 2016-11-17 0:08 UTC (permalink / raw)
To: Boris Ostrovsky, Jan Beulich
Cc: ian.jackson, xen-devel, Paul Durrant, wei.liu2, roger.pau
On 17/11/2016 00:00, Boris Ostrovsky wrote:
>> When we want to enable ACPI vcpu hotplug for HVM guests,
>>>> What do you mean by "when"? We *are* doing ACPI hotplug for HVM guests,
>>>> aren't we?
>>> Are we? If so, how?
>>>
>>> I don't see any toolstack or qemu code able to cope with APCI CPU
>>> hotplug. I can definitely see ACPI PCI hotplug in qemu, but that does
>>> make sense.
>> piix4_acpi_system_hot_add_init():
>> acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
>> PIIX4_CPU_HOTPLUG_IO_BASE);
>>
>>
>>>> Or are you thinking about moving this functionality to the hypervisor?
>>> As an aside, we need to move some part of PCI hotplug into the
>>> hypervisor longterm. At the moment, any new entity coming along and
>>> attaching to an ioreq server still needs to negotiate with Qemu to make
>>> the device appear. This is awkward but doable if all device models are
>>> in dom0, but is far harder if the device models are in different domains.
>>>
>>> As for CPU hotplug, (if I have indeed overlooked something), Qemu has no
>>> business in this matter.
>> Yes. And if we are going to do it for PVH we might as well do it for HVM
>> --- I think most of the code will be the same, save for how SCI is sent.
>
> So I discovered that we actually cannot unplug an HVM VCPU with qemu,
> there is no support for that via QMP (which is what we use).
>
> 'xl vcpu-set <domid> N' is nop when we unplug.
Lovely!
Sounds like an even better reason to implement it properly when someone
has some TUITs.
Anyway, so long as the PVH implementation will be clean to reuse when
someone gets time to retrofit it to plain HVM guests, I am happy.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests
2016-11-15 19:19 ` Andrew Cooper
2016-11-15 19:38 ` Boris Ostrovsky
@ 2016-11-16 9:23 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-16 9:23 UTC (permalink / raw)
To: Andrew Cooper
Cc: wei.liu2, ian.jackson, xen-devel, Paul Durrant, Boris Ostrovsky,
roger.pau
>>> On 15.11.16 at 20:19, <andrew.cooper3@citrix.com> wrote:
> On 15/11/16 15:56, Jan Beulich wrote:
>>>>> On 15.11.16 at 16:44, <boris.ostrovsky@oracle.com> wrote:
>>> On 11/15/2016 10:17 AM, Jan Beulich wrote:
>>>>> The other option was XEN_X86_EMU_ACPI. Would it be better?
>>>> As that's a little too wide (and I think someone else had also
>>>> disliked it for that reason), how about XEN_X86_EMU_ACPI_FF
>>>> (for "fixed features"), or if that's still too wide, break things up
>>>> (PM1a, PM1b, PM2, TMR, GPE0, GPE1)?
>>> I think this may be a bit too fine-grained. Fixed-features would be
>>> good, but is GPE block considered part of fixed features?
>> See figure 4-12 in ACPI 6.1: GEP{0,1} are included there, and the
>> text ahead of this makes it pretty clear that altogether they're
>> being called fixed hardware register blocks. So if you consider FF
>> misleading, FHRB would be another option.
>
> Please can we also considering a naming appropriate for joint use with
> HVM guests as well.
>
> For PVH, (if enabled), Xen handles all (implemented) fixed function
> registers.
>
> For HVM, Xen already intercepts and interposes on the PM1a_STS and
> PM1a_EN registers heading towards qemu, for the apparent purpose of
> raising SCIs on behalf of qemu.
>
> When we want to enable ACPI vcpu hotplug for HVM guests, Xen will have
> to maintain the current intercept behaviour for qemu, but also take on
> the PM1b role entirely.
PM1b? There's no such thing right now, and it's also not being added
by Boris' series, so I don't know what you're thinking about making it
a requirement to have (and be handled in Xen). Yes, PM1a and PM1b
are intentionally split so that two distinct parties (on raw hardware:
chips) could each take on part of the combined functionality. But, if
at all, that should have been leveraged when the implementation was
done originally (to separate Xen's and qemu's parts); I don't see how
it matters now (unless you're again referring to some future overhaul).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 09/11] events/x86: Define SCI virtual interrupt
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (7 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-15 9:29 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event Boris Ostrovsky
2016-11-09 14:39 ` [PATCH v2 11/11] docs: Describe PVHv2's VCPU hotplug procedure Boris Ostrovsky
10 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
PVH guests do not have IOAPIC which typically generates an SCI. For
those guests SCI will be provided as a virtual interrupt.
We also move VIRQ_MCA definition out of xen-mca.h to
keep all x86-specific VIRQ_ARCH_* in one place.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
xen/include/asm-x86/event.h | 3 ++-
xen/include/public/arch-x86/xen-mca.h | 2 --
xen/include/public/arch-x86/xen.h | 3 +++
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index a82062e..9cad8e3 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -38,9 +38,10 @@ static inline void local_event_delivery_enable(void)
vcpu_info(current, evtchn_upcall_mask) = 0;
}
-/* No arch specific virq definition now. Default to global. */
static inline int arch_virq_is_global(uint32_t virq)
{
+ if ( virq == VIRQ_SCI )
+ return 0;
return 1;
}
diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h
index a97e821..b76c53c 100644
--- a/xen/include/public/arch-x86/xen-mca.h
+++ b/xen/include/public/arch-x86/xen-mca.h
@@ -91,8 +91,6 @@
#ifndef __ASSEMBLY__
-#define VIRQ_MCA VIRQ_ARCH_0 /* G. (DOM0) Machine Check Architecture */
-
/*
* Machine Check Architecure:
* structs are read-only and used to report all kinds of
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index 350bc66..4750ba7 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -296,6 +296,9 @@ struct xen_arch_domainconfig {
};
#endif
+#define VIRQ_MCA VIRQ_ARCH_0 /* G. (DOM0) Machine Check Architecture */
+#define VIRQ_SCI VIRQ_ARCH_1 /* V. (PVH) ACPI interrupt */
+
#endif /* !__ASSEMBLY__ */
/*
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 09/11] events/x86: Define SCI virtual interrupt
2016-11-09 14:39 ` [PATCH v2 09/11] events/x86: Define SCI virtual interrupt Boris Ostrovsky
@ 2016-11-15 9:29 ` Jan Beulich
0 siblings, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 9:29 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/include/asm-x86/event.h
> +++ b/xen/include/asm-x86/event.h
> @@ -38,9 +38,10 @@ static inline void local_event_delivery_enable(void)
> vcpu_info(current, evtchn_upcall_mask) = 0;
> }
>
> -/* No arch specific virq definition now. Default to global. */
> static inline int arch_virq_is_global(uint32_t virq)
> {
> + if ( virq == VIRQ_SCI )
> + return 0;
> return 1;
> }
Why is SCI per-CPU? (And if so, please use "return virq != VIRQ_SCI".)
It's certainly conceptually a global one in the native case.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (8 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 09/11] events/x86: Define SCI virtual interrupt Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
2016-11-15 9:36 ` Jan Beulich
2016-11-15 9:38 ` Jan Beulich
2016-11-09 14:39 ` [PATCH v2 11/11] docs: Describe PVHv2's VCPU hotplug procedure Boris Ostrovsky
10 siblings, 2 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
.. and update GPE0 registers.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* Use has_ioreq_cpuhp() instead of ioreq_gmfn.mask==0 as check
for PVH guest
xen/arch/x86/domctl.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b736d4c..07c8247 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1443,6 +1443,18 @@ long arch_do_domctl(
break;
d->arch.avail_vcpus = num;
+
+ /*
+ * For PVH guests we need to send an SCI and set enable/status
+ * bits in GPE block (DSDT specifies _E02, so it's bit 2).
+ */
+ if ( is_hvm_domain(d) && !has_ioreq_cpuhp(d) )
+ {
+ d->arch.hvm_domain.acpi_io.gpe[2] =
+ d->arch.hvm_domain.acpi_io.gpe[0] = 4;
+ send_guest_vcpu_virq(d->vcpu[0], VIRQ_SCI);
+ }
+
ret = 0;
break;
}
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event
2016-11-09 14:39 ` [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event Boris Ostrovsky
@ 2016-11-15 9:36 ` Jan Beulich
2016-11-15 14:57 ` Boris Ostrovsky
2016-11-15 9:38 ` Jan Beulich
1 sibling, 1 reply; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 9:36 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1443,6 +1443,18 @@ long arch_do_domctl(
> break;
>
> d->arch.avail_vcpus = num;
> +
> + /*
> + * For PVH guests we need to send an SCI and set enable/status
> + * bits in GPE block (DSDT specifies _E02, so it's bit 2).
> + */
> + if ( is_hvm_domain(d) && !has_ioreq_cpuhp(d) )
> + {
> + d->arch.hvm_domain.acpi_io.gpe[2] =
> + d->arch.hvm_domain.acpi_io.gpe[0] = 4;
> + send_guest_vcpu_virq(d->vcpu[0], VIRQ_SCI);
> + }
The use of d->vcpu[0] here supports this not being a per-vCPU
vIRQ. And it has two problems: you don't check it to be non-NULL
(see send_guest_global_virq(), which you want to make
non-static), and what do you do if vCPU0 is offline (i.e. you also
want to generalize send_guest_global_virq())?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event
2016-11-15 9:36 ` Jan Beulich
@ 2016-11-15 14:57 ` Boris Ostrovsky
2016-11-15 15:18 ` Jan Beulich
0 siblings, 1 reply; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-15 14:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
On 11/15/2016 04:36 AM, Jan Beulich wrote:
>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1443,6 +1443,18 @@ long arch_do_domctl(
>> break;
>>
>> d->arch.avail_vcpus = num;
>> +
>> + /*
>> + * For PVH guests we need to send an SCI and set enable/status
>> + * bits in GPE block (DSDT specifies _E02, so it's bit 2).
>> + */
>> + if ( is_hvm_domain(d) && !has_ioreq_cpuhp(d) )
>> + {
>> + d->arch.hvm_domain.acpi_io.gpe[2] =
>> + d->arch.hvm_domain.acpi_io.gpe[0] = 4;
>> + send_guest_vcpu_virq(d->vcpu[0], VIRQ_SCI);
>> + }
> The use of d->vcpu[0] here supports this not being a per-vCPU
> vIRQ. And it has two problems: you don't check it to be non-NULL
> (see send_guest_global_virq(), which you want to make
> non-static), and what do you do if vCPU0 is offline (i.e. you also
> want to generalize send_guest_global_virq())?
IIRC in Linux you can't offline BP but I guess in general it should be
supported.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event
2016-11-15 14:57 ` Boris Ostrovsky
@ 2016-11-15 15:18 ` Jan Beulich
0 siblings, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 15:18 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 15.11.16 at 15:57, <boris.ostrovsky@oracle.com> wrote:
> On 11/15/2016 04:36 AM, Jan Beulich wrote:
>>>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -1443,6 +1443,18 @@ long arch_do_domctl(
>>> break;
>>>
>>> d->arch.avail_vcpus = num;
>>> +
>>> + /*
>>> + * For PVH guests we need to send an SCI and set enable/status
>>> + * bits in GPE block (DSDT specifies _E02, so it's bit 2).
>>> + */
>>> + if ( is_hvm_domain(d) && !has_ioreq_cpuhp(d) )
>>> + {
>>> + d->arch.hvm_domain.acpi_io.gpe[2] =
>>> + d->arch.hvm_domain.acpi_io.gpe[0] = 4;
>>> + send_guest_vcpu_virq(d->vcpu[0], VIRQ_SCI);
>>> + }
>> The use of d->vcpu[0] here supports this not being a per-vCPU
>> vIRQ. And it has two problems: you don't check it to be non-NULL
>> (see send_guest_global_virq(), which you want to make
>> non-static), and what do you do if vCPU0 is offline (i.e. you also
>> want to generalize send_guest_global_virq())?
>
> IIRC in Linux you can't offline BP but I guess in general it should be
> supported.
I thought that has changed a year or two ago (see the
*_HOTPLUG_CPU0 config options).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event
2016-11-09 14:39 ` [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event Boris Ostrovsky
2016-11-15 9:36 ` Jan Beulich
@ 2016-11-15 9:38 ` Jan Beulich
1 sibling, 0 replies; 76+ messages in thread
From: Jan Beulich @ 2016-11-15 9:38 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: andrew.cooper3, xen-devel, wei.liu2, ian.jackson, roger.pau
>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1443,6 +1443,18 @@ long arch_do_domctl(
> break;
>
> d->arch.avail_vcpus = num;
> +
> + /*
> + * For PVH guests we need to send an SCI and set enable/status
> + * bits in GPE block (DSDT specifies _E02, so it's bit 2).
> + */
One more thing - this comment should be accompanied by an update
to the other side, so whoever touches one side has a reasonable
chance to find the other part that may also need changing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH v2 11/11] docs: Describe PVHv2's VCPU hotplug procedure
2016-11-09 14:39 [PATCH v2 00/11] PVH VCPU hotplug support Boris Ostrovsky
` (9 preceding siblings ...)
2016-11-09 14:39 ` [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event Boris Ostrovsky
@ 2016-11-09 14:39 ` Boris Ostrovsky
10 siblings, 0 replies; 76+ messages in thread
From: Boris Ostrovsky @ 2016-11-09 14:39 UTC (permalink / raw)
To: xen-devel
Cc: wei.liu2, andrew.cooper3, ian.jackson, jbeulich, Boris Ostrovsky,
roger.pau
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* New patch
docs/misc/hvmlite.markdown | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/docs/misc/hvmlite.markdown b/docs/misc/hvmlite.markdown
index 898b8ee..0045d22 100644
--- a/docs/misc/hvmlite.markdown
+++ b/docs/misc/hvmlite.markdown
@@ -75,3 +75,15 @@ info structure that's passed at boot time (field rsdp_paddr).
Description of paravirtualized devices will come from XenStore, just as it's
done for HVM guests.
+
+## VCPU hotplug ##
+
+VCPU hotplug (e.g. 'xl vcpu-set <domain> <num_vcpus>') for PVHv2 guests
+follows ACPI model where change in domain's number of VCPUS (stored in
+arch_domain.avail_vcpus) results in an SCI being sent to the guest. The
+guest then executes DSDT's PRSC method, updating MADT enable status for
+the affected VCPU.
+
+This is achieved by having the toolstack call XEN_DOMCTL_set_avail_vcpus
+which sets appropriate bits in ACPI GPE0 enable and status registers followed
+by sending VIRQ_SCI to the guest.
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 76+ messages in thread