[PATCH v2 00/11] PVH VCPU hotplug support

[PATCH v2 00/11] PVH VCPU hotplug support

[Boris Ostrovsky]

This series adds support for ACPI-based VCPU hotplug for unprivileged
PVH guests.

New XEN_DOMCTL_set_avail_vcpus is introduced and is called during
guest creation and in response to 'xl vcpu-set' command. This domctl
updates GPE0's status and enable registers and sends an SCI to the
guest using (newly added) VIRQ_SCI.

Main changes in V2:
* Add XEN_X86_EMU_IOREQ_CPUHP flag to indicate need to handle
  ACPI accesses in hypervisor
* Add ACPI_CPU_MAP[_LEN] for 0xafee address block


Boris Ostrovsky (11):
  x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
  acpi: Define ACPI IO registers for PVH guests
  pvh: Set online VCPU map to avail_vcpus
  acpi: Make pmtimer optional in FADT
  acpi: Power and Sleep ACPI buttons are not emulated for PVH guests
  acpi: PVH guests need _E02 method
  pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
  pvh/acpi: Handle ACPI accesses for PVH guests
  events/x86: Define SCI virtual interrupt
  pvh: Send an SCI on VCPU hotplug event
  docs: Describe PVHv2's VCPU hotplug procedure

 docs/misc/hvmlite.markdown            | 12 +++++
 tools/firmware/hvmloader/util.c       |  4 +-
 tools/flask/policy/modules/dom0.te    |  2 +-
 tools/flask/policy/modules/xen.if     |  4 +-
 tools/libacpi/build.c                 |  7 +++
 tools/libacpi/libacpi.h               |  2 +
 tools/libacpi/mk_dsdt.c               | 14 +++---
 tools/libacpi/static_tables.c         | 28 +++++------
 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 +++
 tools/libxl/libxl_x86_acpi.c          |  6 +--
 xen/arch/x86/domctl.c                 | 29 +++++++++++
 xen/arch/x86/hvm/ioreq.c              | 90 +++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/domain.h          |  8 ++++
 xen/include/asm-x86/event.h           |  3 +-
 xen/include/asm-x86/hvm/domain.h      |  6 +++
 xen/include/public/arch-arm.h         | 11 +++--
 xen/include/public/arch-x86/xen-mca.h |  2 -
 xen/include/public/arch-x86/xen.h     |  8 +++-
 xen/include/public/domctl.h           |  9 ++++
 xen/include/public/hvm/ioreq.h        | 13 +++++
 xen/xsm/flask/hooks.c                 |  3 ++
 xen/xsm/flask/policy/access_vectors   |  2 +
 28 files changed, 274 insertions(+), 38 deletions(-)

-- 
2.7.4


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

[PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

[PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

[PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus

[Boris Ostrovsky]

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

[PATCH v2 04/11] acpi: Make pmtimer optional in FADT

[Boris Ostrovsky]

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

[PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests

[Boris Ostrovsky]

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

[PATCH v2 06/11] acpi: PVH guests need _E02 method

[Boris Ostrovsky]

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

[PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

[Boris Ostrovsky]

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

[PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

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, &reg[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(&reg[idx], val, bytes);
+    }
+
     return X86EMUL_OKAY;
 }
 
-- 
2.7.4


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

[PATCH v2 09/11] events/x86: Define SCI virtual interrupt

[Boris Ostrovsky]

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

[PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event

[Boris Ostrovsky]

.. 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

[PATCH v2 11/11] docs: Describe PVHv2's VCPU hotplug procedure

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Julien Grall]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Paul Durrant]

> -----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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

[Paul Durrant]

> -----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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Paul Durrant]

> -----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, &reg[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(&reg[idx], val, bytes);
> +    }
> +
>      return X86EMUL_OKAY;
>  }
> 
> --
> 2.7.4


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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests

[Konrad Rzeszutek Wilk]

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

Re: [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus

[Konrad Rzeszutek Wilk]

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

Re: [PATCH v2 06/11] acpi: PVH guests need _E02 method

[Konrad Rzeszutek Wilk]

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

Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

[Konrad Rzeszutek Wilk]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Konrad Rzeszutek Wilk]

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

Re: [PATCH v2 03/11] pvh: Set online VCPU map to avail_vcpus

[Wei Liu]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Jan Beulich]

>>> 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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 04/11] acpi: Make pmtimer optional in FADT

[Jan Beulich]

>>> On 09.11.16 at 15:39, <boris.ostrovsky@oracle.com> wrote:
> 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>

Acked-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v2 05/11] acpi: Power and Sleep ACPI buttons are not emulated for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 06/11] acpi: PVH guests need _E02 method

[Jan Beulich]

>>> 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

Re: [PATCH v2 07/11] pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

[Jan Beulich]

>>> 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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Jan Beulich]

>>> 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, &reg[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(&reg[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

Re: [PATCH v2 09/11] events/x86: Define SCI virtual interrupt

[Jan Beulich]

>>> 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

Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event

[Jan Beulich]

>>> 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

Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event

[Jan Beulich]

>>> 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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Jan Beulich]

>>> 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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

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, &reg[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(&reg[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

Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 10/11] pvh: Send an SCI on VCPU hotplug event

[Jan Beulich]

>>> 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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

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

Re: [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper]

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

Re: [PATCH v2 02/11] acpi: Define ACPI IO registers for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Andrew Cooper]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Jan Beulich]

>>> 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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Boris Ostrovsky]

>
> 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

Re: [PATCH v2 08/11] pvh/acpi: Handle ACPI accesses for PVH guests

[Andrew Cooper]

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