[PATCH 00/10] PVH VCPU hotplug support

[PATCH 00/10] 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.


Boris Ostrovsky (10):
  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: Power and Sleep ACPI buttons are not emulated
  acpi: Make pmtimer optional in FADT
  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

 tools/firmware/hvmloader/util.c       |  3 +-
 tools/flask/policy/modules/dom0.te    |  2 +-
 tools/flask/policy/modules/xen.if     |  4 +-
 tools/libacpi/build.c                 |  5 +++
 tools/libacpi/libacpi.h               |  1 +
 tools/libacpi/mk_dsdt.c               | 10 ++---
 tools/libacpi/static_tables.c         | 31 ++++++-------
 tools/libxc/include/xenctrl.h         |  5 +++
 tools/libxc/xc_dom_x86.c              | 14 ++++++
 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                 | 25 +++++++++++
 xen/arch/x86/hvm/hvm.c                | 13 ++++--
 xen/arch/x86/hvm/ioreq.c              | 83 +++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/domain.h          |  6 +++
 xen/include/asm-x86/event.h           |  3 +-
 xen/include/asm-x86/hvm/domain.h      |  6 +++
 xen/include/asm-x86/hvm/ioreq.h       |  1 +
 xen/include/public/arch-x86/xen-mca.h |  2 -
 xen/include/public/arch-x86/xen.h     |  3 ++
 xen/include/public/domctl.h           |  9 ++++
 xen/include/public/hvm/ioreq.h        |  3 ++
 xen/xsm/flask/hooks.c                 |  3 ++
 xen/xsm/flask/policy/access_vectors   |  2 +
 28 files changed, 235 insertions(+), 38 deletions(-)

-- 
2.7.4


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

[PATCH 01/10] 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>
---
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 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               | 13 +++++++++++++
 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, 84 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 d83f031..0ac4c5b 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 };
 	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..78b7d4b 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1430,6 +1430,19 @@ long arch_do_domctl(
         }
         break;
 
+    case XEN_DOMCTL_set_avail_vcpus:
+    {
+        unsigned int num = domctl->u.avail_vcpus.num;
+
+        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

Re: [PATCH 01/10] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:34PM -0500, 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.

In light of that perhaps the change in domctl.h should also
include this comment?


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

Re: [PATCH 01/10] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Boris Ostrovsky]

On 11/07/2016 10:30 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:34PM -0500, 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.
> In light of that perhaps the change in domctl.h should also
> include this comment?
>

This is part of the interface so I am not sure whether use cases belong
in public headers. domctl.c may be a better place.

-boris


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

Re: [PATCH 01/10] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Daniel De Graaf]

On 11/06/2016 04:42 PM, 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>

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

[PATCH 02/10] 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>
---
 tools/libacpi/static_tables.c    | 28 +++++++++++-----------------
 xen/include/asm-x86/hvm/domain.h |  6 ++++++
 xen/include/public/hvm/ioreq.h   |  3 +++
 3 files changed, 20 insertions(+), 17 deletions(-)

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/hvm/ioreq.h b/xen/include/public/hvm/ioreq.h
index 2e5809b..c36dd0f 100644
--- a/xen/include/public/hvm/ioreq.h
+++ b/xen/include/public/hvm/ioreq.h
@@ -124,6 +124,9 @@ 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
 
 #endif /* _IOREQ_H_ */
 
-- 
2.7.4


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

[PATCH 03/10] 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 ACPI code during hotplug.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 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

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

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:36PM -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

Oh, so this is a bug then? We always marked them as enabled?
Is this mentioned somewhere in the ACPI docs?

But what about guests that don't want to start their CPUs using
ACPI but are using SMP MPtables?

> "enable" by ACPI code during hotplug.

. which ACPI code? The libacpi one? You may want to be specific.
Or the QEMU code?

> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  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 03/10] pvh: Set online VCPU map to avail_vcpus

[Boris Ostrovsky]

On 11/07/2016 10:36 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:36PM -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
> Oh, so this is a bug then? We always marked them as enabled?
> Is this mentioned somewhere in the ACPI docs?
>
> But what about guests that don't want to start their CPUs using
> ACPI but are using SMP MPtables?

This code is only applicable to PVH guests and it wasn't a bug because
until now I assumed PV-like CPU hotplug (where the guest would watch
xenstore). That required having all VCPUs marked as enabled in MADT and
the guest (well, Linux guest) would immediately offline them.

Now that we are going full-ACPI this is no longer the case.

>
>> "enable" by ACPI code during hotplug.
> . which ACPI code? The libacpi one? You may want to be specific.
> Or the QEMU code?

AML code in DSDT. I'll update the comment.

(No qemu --- this is PVH)

-boris

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

[PATCH 04/10] acpi: Power and Sleep ACPI buttons are not emulated

[Boris Ostrovsky]

.. for PVH guests. However, since emulating them for HVM guests
also doesn't seem useful we can have FADT disable those buttons
for both types of guests.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libacpi/static_tables.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
index 413abcc..ebe8ffe 100644
--- a/tools/libacpi/static_tables.c
+++ b/tools/libacpi/static_tables.c
@@ -61,7 +61,8 @@ struct acpi_20_fadt Fadt = {
     .flags = (ACPI_PROC_C1 |
               ACPI_WBINVD |
               ACPI_FIX_RTC | ACPI_TMR_VAL_EXT |
-              ACPI_USE_PLATFORM_CLOCK),
+              ACPI_USE_PLATFORM_CLOCK |
+              ACPI_PWR_BUTTON | ACPI_SLP_BUTTON),
 
     .reset_reg = {
         .address_space_id    = ACPI_SYSTEM_IO,
-- 
2.7.4


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

Re: [PATCH 04/10] acpi: Power and Sleep ACPI buttons are not emulated

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:37PM -0500, Boris Ostrovsky wrote:
> .. for PVH guests. However, since emulating them for HVM guests
> also doesn't seem useful we can have FADT disable those buttons
> for both types of guests.

Wait, we need S3 suspend for HVM guests (think Windows without PV drivers)!
And I this is how we "inject" the event to the guest.

CC-ing Annie and Adnan who I believe had to do something like this
in old version of OVS, maybe they can recall more details.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  tools/libacpi/static_tables.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
> index 413abcc..ebe8ffe 100644
> --- a/tools/libacpi/static_tables.c
> +++ b/tools/libacpi/static_tables.c
> @@ -61,7 +61,8 @@ struct acpi_20_fadt Fadt = {
>      .flags = (ACPI_PROC_C1 |
>                ACPI_WBINVD |
>                ACPI_FIX_RTC | ACPI_TMR_VAL_EXT |
> -              ACPI_USE_PLATFORM_CLOCK),
> +              ACPI_USE_PLATFORM_CLOCK |
> +              ACPI_PWR_BUTTON | ACPI_SLP_BUTTON),
>  
>      .reset_reg = {
>          .address_space_id    = ACPI_SYSTEM_IO,
> -- 
> 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 04/10] acpi: Power and Sleep ACPI buttons are not emulated

[Boris Ostrovsky]

On 11/07/2016 10:38 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:37PM -0500, Boris Ostrovsky wrote:
>> .. for PVH guests. However, since emulating them for HVM guests
>> also doesn't seem useful we can have FADT disable those buttons
>> for both types of guests.
> Wait, we need S3 suspend for HVM guests (think Windows without PV drivers)!
> And I this is how we "inject" the event to the guest.
>
> CC-ing Annie and Adnan who I believe had to do something like this
> in old version of OVS, maybe they can recall more details.

Andrew has already requested that HVM guests keep their buttons so this
change will only be implemented for PVH guests.

-boris


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

Re: [PATCH 04/10] acpi: Power and Sleep ACPI buttons are not emulated

[annie li]

On 11/7/2016 10:38 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:37PM -0500, Boris Ostrovsky wrote:
>> .. for PVH guests. However, since emulating them for HVM guests
>> also doesn't seem useful we can have FADT disable those buttons
>> for both types of guests.
> Wait, we need S3 suspend for HVM guests (think Windows without PV drivers)!
> And I this is how we "inject" the event to the guest.
>
> CC-ing Annie and Adnan who I believe had to do something like this
> in old version of OVS, maybe they can recall more details.
Our winpv supports S4(hibernation) which will be tested by WHQL, and 
S3(Sleep) is optional(no real process for it yet).

Thanks
Annie
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>   tools/libacpi/static_tables.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libacpi/static_tables.c b/tools/libacpi/static_tables.c
>> index 413abcc..ebe8ffe 100644
>> --- a/tools/libacpi/static_tables.c
>> +++ b/tools/libacpi/static_tables.c
>> @@ -61,7 +61,8 @@ struct acpi_20_fadt Fadt = {
>>       .flags = (ACPI_PROC_C1 |
>>                 ACPI_WBINVD |
>>                 ACPI_FIX_RTC | ACPI_TMR_VAL_EXT |
>> -              ACPI_USE_PLATFORM_CLOCK),
>> +              ACPI_USE_PLATFORM_CLOCK |
>> +              ACPI_PWR_BUTTON | ACPI_SLP_BUTTON),
>>   
>>       .reset_reg = {
>>           .address_space_id    = ACPI_SYSTEM_IO,
>> -- 
>> 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


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

[PATCH 05/10] 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>
---
 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

Re: [PATCH 05/10] acpi: Make pmtimer optional in FADT

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:38PM -0500, Boris Ostrovsky 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>
> ---
>  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

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

[PATCH 06/10] 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>
---
 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 4ae68bc..407386a 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -280,11 +280,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",
@@ -292,6 +287,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

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

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:39PM -0500, Boris Ostrovsky wrote:
> This is the method that will get invoked on an SCI.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


.. thought I wonder - do you want to also make the writes to
_GPE.DPT1 and _GPE.DPT2 be optional? Looking at the mk_dsdt.c
those are defined at 0xb044 and are done in:
 /**** PCI0 start ****/ 

aka:

  stmt("OperationRegion", "DG1, SystemIO, 0xb044, 0x04");                     
415                                                                                 
416     push_block("Field", "DG1, ByteAcc, NoLock, Preserve");                      
417     indent(); printf("DPT1, 8, DPT2, 8\n");                                     
418     pop_block();                                     

Which means if you run 'acpixtract' and 'acpixecute' to play with
the DSDTs you will get nasty warnings about the DPT1 and DPT2 not being
defined.

I am actually not sure what the kernel will do with undefind DPT1 and DPT2?
I would have thought the iasl compiler would throw an nasty fit?

> ---
>  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 4ae68bc..407386a 100644
> --- a/tools/libacpi/mk_dsdt.c
> +++ b/tools/libacpi/mk_dsdt.c
> @@ -280,11 +280,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",
> @@ -292,6 +287,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 06/10] acpi: PVH guests need _E02 method

[Boris Ostrovsky]

On 11/07/2016 10:45 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:39PM -0500, Boris Ostrovsky wrote:
>> This is the method that will get invoked on an SCI.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
> .. thought I wonder - do you want to also make the writes to
> _GPE.DPT1 and _GPE.DPT2 be optional? Looking at the mk_dsdt.c
> those are defined at 0xb044 and are done in:
>  /**** PCI0 start ****/ 
>
> aka:
>
>   stmt("OperationRegion", "DG1, SystemIO, 0xb044, 0x04");                     
> 415                                                                                 
> 416     push_block("Field", "DG1, ByteAcc, NoLock, Preserve");                      
> 417     indent(); printf("DPT1, 8, DPT2, 8\n");                                     
> 418     pop_block();                                     
>
> Which means if you run 'acpixtract' and 'acpixecute' to play with
> the DSDTs you will get nasty warnings about the DPT1 and DPT2 not being
> defined.
>
> I am actually not sure what the kernel will do with undefind DPT1 and DPT2?
> I would have thought the iasl compiler would throw an nasty fit?

Are these ACPI debug ports?

I don't see any warnings from acpixtract, unless it's a particular AML
that would cause it.

-boris


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

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

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2016 at 11:08:26AM -0500, Boris Ostrovsky wrote:
> On 11/07/2016 10:45 AM, Konrad Rzeszutek Wilk wrote:
> > On Sun, Nov 06, 2016 at 04:42:39PM -0500, Boris Ostrovsky wrote:
> >> This is the method that will get invoked on an SCI.
> >>
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >
> > .. thought I wonder - do you want to also make the writes to
> > _GPE.DPT1 and _GPE.DPT2 be optional? Looking at the mk_dsdt.c
> > those are defined at 0xb044 and are done in:
> >  /**** PCI0 start ****/ 
> >
> > aka:
> >
> >   stmt("OperationRegion", "DG1, SystemIO, 0xb044, 0x04");                     
> > 415                                                                                 
> > 416     push_block("Field", "DG1, ByteAcc, NoLock, Preserve");                      
> > 417     indent(); printf("DPT1, 8, DPT2, 8\n");                                     
> > 418     pop_block();                                     
> >
> > Which means if you run 'acpixtract' and 'acpixecute' to play with
> > the DSDTs you will get nasty warnings about the DPT1 and DPT2 not being
> > defined.
> >
> > I am actually not sure what the kernel will do with undefind DPT1 and DPT2?
> > I would have thought the iasl compiler would throw an nasty fit?
> 
> Are these ACPI debug ports?

Yes.
> 
> I don't see any warnings from acpixtract, unless it's a particular AML
> that would cause it.

Could you send the .dsl files over ?
> 
> -boris
> 

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

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

[Boris Ostrovsky]

No IOREQ server installed for an HVM guest (as indicated
by HVM_PARAM_NR_IOREQ_SERVER_PAGES being set to zero) implies
a PVH guest. These guests need to handle ACPI-related IO
accesses.

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>
---
 tools/libxc/xc_dom_x86.c        |  3 +++
 xen/arch/x86/hvm/hvm.c          | 13 +++++++++----
 xen/arch/x86/hvm/ioreq.c        | 17 +++++++++++++++++
 xen/include/asm-x86/hvm/ioreq.h |  1 +
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 7fcdee1..0017694 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -649,6 +649,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         /* Limited to one module. */
         if ( dom->ramdisk_blob )
             start_info_size += sizeof(struct hvm_modlist_entry);
+
+        /* No IOREQ server for PVH guests. */
+        xc_hvm_param_set(xch, domid, HVM_PARAM_NR_IOREQ_SERVER_PAGES, 0);
     }
     else
     {
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 704fd64..6f8439d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5206,14 +5206,19 @@ static int hvmop_set_param(
     {
         unsigned int i;
 
-        if ( a.value == 0 ||
-             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
+        if ( a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
         {
             rc = -EINVAL;
             break;
         }
-        for ( i = 0; i < a.value; i++ )
-            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
+
+        if ( a.value == 0 ) /* PVH guest */
+            acpi_ioreq_init(d);
+        else
+        {
+            for ( i = 0; i < a.value; i++ )
+                set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
+        }
 
         break;
     }
diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index d2245e2..171ea82 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1389,6 +1389,23 @@ void hvm_ioreq_init(struct domain *d)
         register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
 }
 
+static int acpi_ioaccess(
+    int dir, unsigned int port, unsigned int bytes, uint32_t *val)
+{
+    return X86EMUL_OKAY;
+}
+
+void acpi_ioreq_init(struct domain *d)
+{
+    /* Online CPU map, see DSDT's PRST region. */
+    register_portio_handler(d, 0xaf00, HVM_MAX_VCPUS/8, 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);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
index fbf2c74..e7b7f52 100644
--- a/xen/include/asm-x86/hvm/ioreq.h
+++ b/xen/include/asm-x86/hvm/ioreq.h
@@ -53,6 +53,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
 unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool_t buffered);
 
 void hvm_ioreq_init(struct domain *d);
+void acpi_ioreq_init(struct domain *d);
 
 #endif /* __ASM_X86_HVM_IOREQ_H__ */
 
-- 
2.7.4


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

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

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 06 November 2016 21:43
> 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 07/10] pvh/ioreq: Install handlers for ACPI-related PVH IO
> accesses
> 
> No IOREQ server installed for an HVM guest (as indicated
> by HVM_PARAM_NR_IOREQ_SERVER_PAGES being set to zero) implies
> a PVH guest. These guests need to handle ACPI-related IO
> accesses.
> 
> 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>
> ---
>  tools/libxc/xc_dom_x86.c        |  3 +++
>  xen/arch/x86/hvm/hvm.c          | 13 +++++++++----
>  xen/arch/x86/hvm/ioreq.c        | 17 +++++++++++++++++
>  xen/include/asm-x86/hvm/ioreq.h |  1 +
>  4 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 7fcdee1..0017694 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -649,6 +649,9 @@ static int alloc_magic_pages_hvm(struct
> xc_dom_image *dom)
>          /* Limited to one module. */
>          if ( dom->ramdisk_blob )
>              start_info_size += sizeof(struct hvm_modlist_entry);
> +
> +        /* No IOREQ server for PVH guests. */
> +        xc_hvm_param_set(xch, domid,
> HVM_PARAM_NR_IOREQ_SERVER_PAGES, 0);

I thought params defaulted to zero...

>      }
>      else
>      {
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 704fd64..6f8439d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5206,14 +5206,19 @@ static int hvmop_set_param(
>      {
>          unsigned int i;
> 
> -        if ( a.value == 0 ||
> -             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )

...and if they do then you should not need this change.

> +        if ( a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
>          {
>              rc = -EINVAL;
>              break;
>          }
> -        for ( i = 0; i < a.value; i++ )
> -            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> +
> +        if ( a.value == 0 ) /* PVH guest */
> +            acpi_ioreq_init(d);
> +        else
> +        {
> +            for ( i = 0; i < a.value; i++ )
> +                set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> +        }

This looks quite wrong. Initializing the acpi io hander should be done directly in hvm_domain_initialise() IMO and not as an obscure side effect of setting a parameter to its default value.

  Paul

> 
>          break;
>      }
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index d2245e2..171ea82 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1389,6 +1389,23 @@ void hvm_ioreq_init(struct domain *d)
>          register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
>  }
> 
> +static int acpi_ioaccess(
> +    int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> +{
> +    return X86EMUL_OKAY;
> +}
> +
> +void acpi_ioreq_init(struct domain *d)
> +{
> +    /* Online CPU map, see DSDT's PRST region. */
> +    register_portio_handler(d, 0xaf00, HVM_MAX_VCPUS/8, 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);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-
> x86/hvm/ioreq.h
> index fbf2c74..e7b7f52 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -53,6 +53,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s,
> ioreq_t *proto_p,
>  unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool_t buffered);
> 
>  void hvm_ioreq_init(struct domain *d);
> +void acpi_ioreq_init(struct domain *d);
> 
>  #endif /* __ASM_X86_HVM_IOREQ_H__ */
> 
> --
> 2.7.4


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

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

[Boris Ostrovsky]

On 11/07/2016 04:39 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
>> Sent: 06 November 2016 21:43
>> 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 07/10] pvh/ioreq: Install handlers for ACPI-related PVH IO
>> accesses
>>
>> No IOREQ server installed for an HVM guest (as indicated
>> by HVM_PARAM_NR_IOREQ_SERVER_PAGES being set to zero) implies
>> a PVH guest. These guests need to handle ACPI-related IO
>> accesses.
>>
>> 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>
>> ---
>>  tools/libxc/xc_dom_x86.c        |  3 +++
>>  xen/arch/x86/hvm/hvm.c          | 13 +++++++++----
>>  xen/arch/x86/hvm/ioreq.c        | 17 +++++++++++++++++
>>  xen/include/asm-x86/hvm/ioreq.h |  1 +
>>  4 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 7fcdee1..0017694 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -649,6 +649,9 @@ static int alloc_magic_pages_hvm(struct
>> xc_dom_image *dom)
>>          /* Limited to one module. */
>>          if ( dom->ramdisk_blob )
>>              start_info_size += sizeof(struct hvm_modlist_entry);
>> +
>> +        /* No IOREQ server for PVH guests. */
>> +        xc_hvm_param_set(xch, domid,
>> HVM_PARAM_NR_IOREQ_SERVER_PAGES, 0);
> I thought params defaulted to zero...
>
>>      }
>>      else
>>      {
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 704fd64..6f8439d 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5206,14 +5206,19 @@ static int hvmop_set_param(
>>      {
>>          unsigned int i;
>>
>> -        if ( a.value == 0 ||
>> -             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
> ...and if they do then you should not need this change.
>
>> +        if ( a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
>>          {
>>              rc = -EINVAL;
>>              break;
>>          }
>> -        for ( i = 0; i < a.value; i++ )
>> -            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>> +
>> +        if ( a.value == 0 ) /* PVH guest */
>> +            acpi_ioreq_init(d);
>> +        else
>> +        {
>> +            for ( i = 0; i < a.value; i++ )
>> +                set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
>> +        }
> This looks quite wrong. Initializing the acpi io hander should be done directly in hvm_domain_initialise() IMO and not as an obscure side effect of setting a parameter to its default value.

Right, this call indeed is used to tell the hypervisor that it should
handle IO accesses itself. It was either this or adding another
emulation flag (which is what Andrew prefers).

-boris


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

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

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 07 November 2016 14:01
> To: Paul Durrant <Paul.Durrant@citrix.com>; 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>
> Subject: Re: [PATCH 07/10] pvh/ioreq: Install handlers for ACPI-related PVH
> IO accesses
> 
> On 11/07/2016 04:39 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> >> Sent: 06 November 2016 21:43
> >> 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 07/10] pvh/ioreq: Install handlers for ACPI-related PVH
> IO
> >> accesses
> >>
> >> No IOREQ server installed for an HVM guest (as indicated
> >> by HVM_PARAM_NR_IOREQ_SERVER_PAGES being set to zero) implies
> >> a PVH guest. These guests need to handle ACPI-related IO
> >> accesses.
> >>
> >> 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>
> >> ---
> >>  tools/libxc/xc_dom_x86.c        |  3 +++
> >>  xen/arch/x86/hvm/hvm.c          | 13 +++++++++----
> >>  xen/arch/x86/hvm/ioreq.c        | 17 +++++++++++++++++
> >>  xen/include/asm-x86/hvm/ioreq.h |  1 +
> >>  4 files changed, 30 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> >> index 7fcdee1..0017694 100644
> >> --- a/tools/libxc/xc_dom_x86.c
> >> +++ b/tools/libxc/xc_dom_x86.c
> >> @@ -649,6 +649,9 @@ static int alloc_magic_pages_hvm(struct
> >> xc_dom_image *dom)
> >>          /* Limited to one module. */
> >>          if ( dom->ramdisk_blob )
> >>              start_info_size += sizeof(struct hvm_modlist_entry);
> >> +
> >> +        /* No IOREQ server for PVH guests. */
> >> +        xc_hvm_param_set(xch, domid,
> >> HVM_PARAM_NR_IOREQ_SERVER_PAGES, 0);
> > I thought params defaulted to zero...
> >
> >>      }
> >>      else
> >>      {
> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >> index 704fd64..6f8439d 100644
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -5206,14 +5206,19 @@ static int hvmop_set_param(
> >>      {
> >>          unsigned int i;
> >>
> >> -        if ( a.value == 0 ||
> >> -             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
> > ...and if they do then you should not need this change.
> >
> >> +        if ( a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
> >>          {
> >>              rc = -EINVAL;
> >>              break;
> >>          }
> >> -        for ( i = 0; i < a.value; i++ )
> >> -            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> >> +
> >> +        if ( a.value == 0 ) /* PVH guest */
> >> +            acpi_ioreq_init(d);
> >> +        else
> >> +        {
> >> +            for ( i = 0; i < a.value; i++ )
> >> +                set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
> >> +        }
> > This looks quite wrong. Initializing the acpi io hander should be done
> directly in hvm_domain_initialise() IMO and not as an obscure side effect of
> setting a parameter to its default value.
> 
> Right, this call indeed is used to tell the hypervisor that it should
> handle IO accesses itself. It was either this or adding another
> emulation flag (which is what Andrew prefers).

Yes, another emulation flag seems like the right thing.

  Paul

> 
> -boris


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

[PATCH 08/10] 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>
---
 xen/arch/x86/hvm/ioreq.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
index 171ea82..ced7c92 100644
--- a/xen/arch/x86/hvm/ioreq.c
+++ b/xen/arch/x86/hvm/ioreq.c
@@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
 static int acpi_ioaccess(
     int dir, unsigned int port, unsigned int bytes, uint32_t *val)
 {
+    unsigned int i;
+    unsigned int bits = bytes * 8;
+    uint8_t *reg = NULL;
+    unsigned idx = port & 3;
+    bool is_cpu_map = 0;
+    struct domain *currd = current->domain;
+
+    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
+                 (ACPI_GPE0_BLK_LEN_V1 != 4));
+
+    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
+        is_cpu_map = 1;
+        break;
+    default:
+        return X86EMUL_UNHANDLEABLE;
+    }
+
+    if ( bytes == 0 )
+        return X86EMUL_OKAY;
+
+    if ( dir == IOREQ_READ )
+    {
+        *val &= ~((1U << bits) - 1);
+
+        if ( is_cpu_map )
+        {
+            unsigned first_bit, last_bit;
+
+            first_bit = (port - 0xaf00) * 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 should not be written. */
+            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 08/10] pvh/acpi: Handle ACPI accesses for PVH guests

[Paul Durrant]

> -----Original Message-----
> From: Boris Ostrovsky [mailto:boris.ostrovsky@oracle.com]
> Sent: 06 November 2016 21:43
> 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 08/10] pvh/acpi: Handle ACPI accesses for PVH guests
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/arch/x86/hvm/ioreq.c | 66
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 171ea82..ced7c92 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>  static int acpi_ioaccess(
>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>  {
> +    unsigned int i;
> +    unsigned int bits = bytes * 8;
> +    uint8_t *reg = NULL;
> +    unsigned idx = port & 3;
> +    bool is_cpu_map = 0;

Shouldn't we be using false instead of 0 now that we are using proper bool types?

> +    struct domain *currd = current->domain;
> +
> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
> +
> +    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
> +        is_cpu_map = 1;

s/1/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 first_bit, last_bit;

unsigned int

> +
> +            first_bit = (port - 0xaf00) * 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 should not be written. */
> +            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);

idx should be strictly == 2 in the else case shouldn't it (since it = port & 3) so would it not be more efficient to use direct assignment rather than resorting to a call to memcpy?

  Paul

> +    }
> +
>      return X86EMUL_OKAY;
>  }
> 
> --
> 2.7.4


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

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

[Boris Ostrovsky]

>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index 171ea82..ced7c92 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>>  static int acpi_ioaccess(
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>> +    unsigned int i;
>> +    unsigned int bits = bytes * 8;
>> +    uint8_t *reg = NULL;
>> +    unsigned idx = port & 3;
>> +    bool is_cpu_map = 0;
>
> Shouldn't we be using false instead of 0 now that we are using proper bool types?
>
>> +    struct domain *currd = current->domain;
>> +
>> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> +    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
>> +        is_cpu_map = 1;
>
> s/1/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 first_bit, last_bit;
>
> unsigned int
>
>> +
>> +            first_bit = (port - 0xaf00) * 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 should not be written. */
>> +            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);
>
> idx should be strictly == 2 in the else case shouldn't it (since it = port & 3) so would it not be more efficient to use direct assignment rather than resorting to a call to memcpy?


Why do you think idx can't be 3? Reading 1 byte from index 3 should be 
possible.

-boris

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

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

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:41PM -0500, Boris Ostrovsky wrote:
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> CC: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/arch/x86/hvm/ioreq.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 171ea82..ced7c92 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>  static int acpi_ioaccess(
>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>  {
> +    unsigned int i;
> +    unsigned int bits = bytes * 8;
> +    uint8_t *reg = NULL;
> +    unsigned idx = port & 3;
> +    bool is_cpu_map = 0;
> +    struct domain *currd = current->domain;
> +
> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
> +
> +    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):

That may need some documentation or a #define perhaps?

Also just in case somebody decided it was funny and compile Xen with
HVM_MAX_VCPUS set to say 4, won't this go in 0xfffffff region?

You may want to add a BUILD_BUG_ON for the HVM_MAX_VCPUS, like:

BUILD_BUGON(HVM_MAX_VCPUS > 8)?

> +        is_cpu_map = 1;
> +        break;
> +    default:
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    if ( bytes == 0 )
> +        return X86EMUL_OKAY;

Should you also check for other odd sizes? Say 3?

> +
> +    if ( dir == IOREQ_READ )
> +    {
> +        *val &= ~((1U << bits) - 1);
> +
> +        if ( is_cpu_map )
> +        {
> +            unsigned first_bit, last_bit;
> +
> +            first_bit = (port - 0xaf00) * 8;

Oh, that needs a define. 
> +            last_bit = min(currd->arch.avail_vcpus, first_bit + bits);
> +            for (i = first_bit; i < last_bit; i++)

I think you need spaces here.
> +                *val |= (1U << (i - first_bit));
> +        }
> +        else
> +            memcpy(val, &reg[idx], bytes);
> +    }
> +    else
> +    {
> +        if ( is_cpu_map )
> +            /* CPU map should not be written. */

It shouldn't? Then who updates this? Oh we do it via the the hypercall.
You may want to mention in this function how this all is suppose to work?

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

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

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

[Boris Ostrovsky]

On 11/07/2016 10:55 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:41PM -0500, Boris Ostrovsky wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> ---
>>  xen/arch/x86/hvm/ioreq.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>>
>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index 171ea82..ced7c92 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>>  static int acpi_ioaccess(
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>> +    unsigned int i;
>> +    unsigned int bits = bytes * 8;
>> +    uint8_t *reg = NULL;
>> +    unsigned idx = port & 3;
>> +    bool is_cpu_map = 0;
>> +    struct domain *currd = current->domain;
>> +
>> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> +    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
> That may need some documentation or a #define perhaps?

This will need to get into public header
(xen/include/public/hvm/ioreq.h) since mk_dsdt.c is the other user.


>
> Also just in case somebody decided it was funny and compile Xen with
> HVM_MAX_VCPUS set to say 4, won't this go in 0xfffffff region?
>
> You may want to add a BUILD_BUG_ON for the HVM_MAX_VCPUS, like:
>
> BUILD_BUGON(HVM_MAX_VCPUS > 8)?

OK (although I'd think compiler would flag the case statment like that)


>
>> +        is_cpu_map = 1;
>> +        break;
>> +    default:
>> +        return X86EMUL_UNHANDLEABLE;
>> +    }
>> +
>> +    if ( bytes == 0 )
>> +        return X86EMUL_OKAY;
> Should you also check for other odd sizes? Say 3?

I think 3-byte read are OK, unless they are not allowed by higher-level
ioreq code.

(And writes are already not allowed)

>
>> +        if ( is_cpu_map )
>> +            /* CPU map should not be written. */
> It shouldn't? Then who updates this? Oh we do it via the the hypercall.
> You may want to mention in this function how this all is suppose to work?

This is updated by the hypervisor's ACPI access handler (or qemu for HVM
guests) and read by AML. I'll add a comment to this effect.

-boris


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

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

[Jan Beulich]

>>> On 07.11.16 at 16:55, <konrad.wilk@oracle.com> wrote:
> On Sun, Nov 06, 2016 at 04:42:41PM -0500, Boris Ostrovsky wrote:
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> ---
>>  xen/arch/x86/hvm/ioreq.c | 66 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>> 
>> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
>> index 171ea82..ced7c92 100644
>> --- a/xen/arch/x86/hvm/ioreq.c
>> +++ b/xen/arch/x86/hvm/ioreq.c
>> @@ -1392,6 +1392,72 @@ void hvm_ioreq_init(struct domain *d)
>>  static int acpi_ioaccess(
>>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>>  {
>> +    unsigned int i;
>> +    unsigned int bits = bytes * 8;
>> +    uint8_t *reg = NULL;
>> +    unsigned idx = port & 3;
>> +    bool is_cpu_map = 0;
>> +    struct domain *currd = current->domain;
>> +
>> +    BUILD_BUG_ON((ACPI_PM1A_EVT_BLK_LEN != 4) ||
>> +                 (ACPI_GPE0_BLK_LEN_V1 != 4));
>> +
>> +    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 0xaf00 ... (0xaf00 + HVM_MAX_VCPUS/8 - 1):
> 
> That may need some documentation or a #define perhaps?
> 
> Also just in case somebody decided it was funny and compile Xen with
> HVM_MAX_VCPUS set to say 4, won't this go in 0xfffffff region?
> 
> You may want to add a BUILD_BUG_ON for the HVM_MAX_VCPUS, like:
> 
> BUILD_BUGON(HVM_MAX_VCPUS > 8)?

No. The expression wants to be 0xaf00 + (HVM_MAX_VCPUS - 1) / 8.
And a BUILD_BUG_ON() should check the range doesn't spill into
anything later (presumably starting at 0xb000).

Jan


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

[PATCH 09/10] 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 cdd93c1..bffa3e0 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -293,6 +293,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

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

[Konrad Rzeszutek Wilk]

On Sun, Nov 06, 2016 at 04:42:42PM -0500, Boris Ostrovsky wrote:
> 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.

I somehow assumed that the ACPI MADT would be in the PVH guest so the
SCI would be enumerated in there (the INT_SVC_OVR) along with the magic
way one can figure this out.

But this implies that VIRQ17 is the SCI one? But how does
that connect to the SCI being at 9 (or 20)?

I know we don't have the LAPIC in there but going forward we may really
want it. Could you kindly explain how this VIRQ ends transumuting itself
in the MADT value? Or would there be an event(virq)->emulated GSI 
to be set?

Sorry for not following that.

Also you may want to expand the PVH markdown as well to document how the
CPU hotplug mechanism will work (or where you thinking to do that once
you get feedback on this, in which case pls ignore me).
> 
> 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 cdd93c1..bffa3e0 100644
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -293,6 +293,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

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

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

[Boris Ostrovsky]

On 11/07/2016 11:05 AM, Konrad Rzeszutek Wilk wrote:
> On Sun, Nov 06, 2016 at 04:42:42PM -0500, Boris Ostrovsky wrote:
>> 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.
> I somehow assumed that the ACPI MADT would be in the PVH guest so the
> SCI would be enumerated in there (the INT_SVC_OVR) along with the magic
> way one can figure this out.
>
> But this implies that VIRQ17 is the SCI one? But how does
> that connect to the SCI being at 9 (or 20)?
>
> I know we don't have the LAPIC in there but going forward we may really
> want it. Could you kindly explain how this VIRQ ends transumuting itself
> in the MADT value? Or would there be an event(virq)->emulated GSI 
> to be set?
>
> Sorry for not following that.

The IRQ is whatever FADT specifies (in libacpi/static_tables.c), the
guest shouldn't care. The expectation is that the guest will them bind
VIRQ_SCI to that IRQ.

So in Linux this will be something like

    if (xen_pvh_domain())
               bind_virq_to_irq(VIRQ_SCI, 0,
acpi_gbl_FADT.sci_interrupt, 0);

(and then bind_virq_to_irq() is modified to take IRQ number as argument,
if available).


>
> Also you may want to expand the PVH markdown as well to document how the
> CPU hotplug mechanism will work (or where you thinking to do that once
> you get feedback on this, in which case pls ignore me).


I'll add a comment there.

-boris



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

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

[Boris Ostrovsky]

.. and update GPE0 registers.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 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 78b7d4b..8151fd7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1439,6 +1439,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) && d->arch.hvm_domain.ioreq_gmfn.mask == 0 )
+        {
+            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

Re: [PATCH 00/10] PVH VCPU hotplug support

[Andrew Cooper]

On 06/11/16 21:42, Boris Ostrovsky wrote:
> 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.

Thankyou for doing this.  Getting APCI hotplug working has been a low
item on my TODO list for while now.

Some queries and comments however.

This series is currently very PVH centric, to the point of making it
unusable for plain HVM guests.  While I won't insist on you implementing
this for HVM (there are some particularly awkward migration problems to
be considered), I do insist that its implementation isn't tied
implicitly to being PVH.

The first part of this will be controlling the hypervisor emulation of
the PM1* blocks with an XEN_X86_EMU_* flag just like all other emulation.

>
>
> Boris Ostrovsky (10):
>   x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

Why is this necessary?  Given that a paravirtual hotplug mechanism
already exists, why isn't its equivalent mechanism suitable?

>   acpi: Define ACPI IO registers for PVH guests

Can Xen use pm1b, or does there have to be a pm1a available to the guest?

>   pvh: Set online VCPU map to avail_vcpus
>   acpi: Power and Sleep ACPI buttons are not emulated

PVH might not want power/sleep, but you cannot assume that HVM guests
have a paravirtual mechnism of shutting down.

>   acpi: Make pmtimer optional in FADT
>   acpi: PVH guests need _E02 method

Patch 6 Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>   pvh/ioreq: Install handlers for ACPI-related PVH IO accesses

Do not make any assumptions about PVHness based on IOREQ servers.  It
will not be true for usecases such as vGPU.

~Andrew

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

Re: [PATCH 00/10] PVH VCPU hotplug support

[Boris Ostrovsky]

On 11/07/2016 06:41 AM, Andrew Cooper wrote:
> On 06/11/16 21:42, Boris Ostrovsky wrote:
>> 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.
> Thankyou for doing this.  Getting APCI hotplug working has been a low
> item on my TODO list for while now.
>
> Some queries and comments however.
>
> This series is currently very PVH centric, to the point of making it
> unusable for plain HVM guests.  While I won't insist on you implementing
> this for HVM (there are some particularly awkward migration problems to
> be considered), I do insist that its implementation isn't tied
> implicitly to being PVH.
>
> The first part of this will be controlling the hypervisor emulation of
> the PM1* blocks with an XEN_X86_EMU_* flag just like all other emulation.

Something like XEN_X86_EMU_ACPI?

That would also eliminate the need for explicitly setting
HVM_PARAM_NR_IOREQ_SERVER_PAGES to zero which I used as indication that
we should have IO handler in the hypervisor. Paul (copied) didn't like that.


>
>>
>> Boris Ostrovsky (10):
>>   x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
> Why is this necessary?  Given that a paravirtual hotplug mechanism
> already exists, why isn't its equivalent mechanism suitable?

PV guests register a xenstore watch and the toolstack updates cpu's 
"available" entry. And ACPI codepath (at least for Linux guest) is not
involved at all.

I don't think we can use anything like that in hypervisor.


>
>>   acpi: Define ACPI IO registers for PVH guests
> Can Xen use pm1b, or does there have to be a pm1a available to the guest?

pm1a is a required block (unlike pm1b). ACPICA, for example, always
first checks pm1a when handling an SCI.

(And how having pm1b only would have helped?)

>
>>   pvh: Set online VCPU map to avail_vcpus
>>   acpi: Power and Sleep ACPI buttons are not emulated
> PVH might not want power/sleep, but you cannot assume that HVM guests
> have a paravirtual mechnism of shutting down.

AFAIK they don't rely on a button-initiated codepath. At least Linux.

I don't know Windows path though. I can add ACPI_HAS_BUTTONS.

>
>>   acpi: Make pmtimer optional in FADT
>>   acpi: PVH guests need _E02 method
> Patch 6 Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
>>   pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
> Do not make any assumptions about PVHness based on IOREQ servers.  It
> will not be true for usecases such as vGPU.

Is this comment related to the last patch or is it a general one?  If
it's the latter and we use XEN_X86_EMU_ACPI then I think this will not
be an issue.

-boris



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

Re: [PATCH 00/10] PVH VCPU hotplug support

[Andrew Cooper]

On 07/11/16 14:19, Boris Ostrovsky wrote:
> On 11/07/2016 06:41 AM, Andrew Cooper wrote:
>> On 06/11/16 21:42, Boris Ostrovsky wrote:
>>> 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.
>> Thankyou for doing this.  Getting APCI hotplug working has been a low
>> item on my TODO list for while now.
>>
>> Some queries and comments however.
>>
>> This series is currently very PVH centric, to the point of making it
>> unusable for plain HVM guests.  While I won't insist on you implementing
>> this for HVM (there are some particularly awkward migration problems to
>> be considered), I do insist that its implementation isn't tied
>> implicitly to being PVH.
>>
>> The first part of this will be controlling the hypervisor emulation of
>> the PM1* blocks with an XEN_X86_EMU_* flag just like all other emulation.
> Something like XEN_X86_EMU_ACPI?

Sounds good.

>
> That would also eliminate the need for explicitly setting
> HVM_PARAM_NR_IOREQ_SERVER_PAGES to zero which I used as indication that
> we should have IO handler in the hypervisor. Paul (copied) didn't like that.

Definitely an improvement.

>>> Boris Ostrovsky (10):
>>>   x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
>> Why is this necessary?  Given that a paravirtual hotplug mechanism
>> already exists, why isn't its equivalent mechanism suitable?
> PV guests register a xenstore watch and the toolstack updates cpu's 
> "available" entry. And ACPI codepath (at least for Linux guest) is not
> involved at all.
>
> I don't think we can use anything like that in hypervisor.

There must be something in the hypervisor; what currently prevents the
PV path ignoring xenstore and onlining CPUs themselves?

Or do we currently have nothing... ?

>
>
>>>   acpi: Define ACPI IO registers for PVH guests
>> Can Xen use pm1b, or does there have to be a pm1a available to the guest?
> pm1a is a required block (unlike pm1b). ACPICA, for example, always
> first checks pm1a when handling an SCI.
>
> (And how having pm1b only would have helped?)

For the HVM case, I think we are going to need to one pm1 block
belonging to qemu, and one belonging to Xen.

>
>>>   pvh: Set online VCPU map to avail_vcpus
>>>   acpi: Power and Sleep ACPI buttons are not emulated
>> PVH might not want power/sleep, but you cannot assume that HVM guests
>> have a paravirtual mechnism of shutting down.
> AFAIK they don't rely on a button-initiated codepath. At least Linux.
>
> I don't know Windows path though. I can add ACPI_HAS_BUTTONS.

Windows very definitely does respond to button presses (although not in
a helpful way).  Please keep them enabled by default for HVM guests,
even if we disallow their use with PVH.

>
>>>   acpi: Make pmtimer optional in FADT
>>>   acpi: PVH guests need _E02 method
>> Patch 6 Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>>>   pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
>> Do not make any assumptions about PVHness based on IOREQ servers.  It
>> will not be true for usecases such as vGPU.
> Is this comment related to the last patch or is it a general one?  If
> it's the latter and we use XEN_X86_EMU_ACPI then I think this will not
> be an issue.

It was about that patch specifically, but XEN_X86_EMU_ACPI is definitely
the better way to go.

The only question is whether there might be other things APCI things we
wish to emulate in the future (PCI hotplug by any chance?), in which
case, should we be slightly more specific than just ACPI in the name label?

~Andrew

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

Re: [PATCH 00/10] PVH VCPU hotplug support

[Boris Ostrovsky]

>>>> Boris Ostrovsky (10):
>>>>   x86/domctl: Add XEN_DOMCTL_set_avail_vcpus
>>> Why is this necessary?  Given that a paravirtual hotplug mechanism
>>> already exists, why isn't its equivalent mechanism suitable?
>> PV guests register a xenstore watch and the toolstack updates cpu's 
>> "available" entry. And ACPI codepath (at least for Linux guest) is not
>> involved at all.
>>
>> I don't think we can use anything like that in hypervisor.
> There must be something in the hypervisor; what currently prevents the
> PV path ignoring xenstore and onlining CPUs themselves?
>
> Or do we currently have nothing... ?


I don't think we have anything. libxl__set_vcpuonline_xenstore() is the
only thing that the toolstack does.

HVM is *possibly* more strict in that onlining involves qemu but I am
not sure even about that (especially qemu-trad, which also triggers
hotplug via xenstore watch).


>
>>
>>>>   acpi: Define ACPI IO registers for PVH guests
>>> Can Xen use pm1b, or does there have to be a pm1a available to the guest?
>> pm1a is a required block (unlike pm1b). ACPICA, for example, always
>> first checks pm1a when handling an SCI.
>>
>> (And how having pm1b only would have helped?)
> For the HVM case, I think we are going to need to one pm1 block
> belonging to qemu, and one belonging to Xen.

The only place we use pm1 block in the hypervisor is for pmtimer (and I
am actually not sure I see how qemu uses it for Xen guests).

>
>>>>   acpi: Make pmtimer optional in FADT
>>>>   acpi: PVH guests need _E02 method
>>> Patch 6 Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>>>   pvh/ioreq: Install handlers for ACPI-related PVH IO accesses
>>> Do not make any assumptions about PVHness based on IOREQ servers.  It
>>> will not be true for usecases such as vGPU.
>> Is this comment related to the last patch or is it a general one?  If
>> it's the latter and we use XEN_X86_EMU_ACPI then I think this will not
>> be an issue.
> It was about that patch specifically, but XEN_X86_EMU_ACPI is definitely
> the better way to go.
>
> The only question is whether there might be other things APCI things we
> wish to emulate in the future (PCI hotplug by any chance?), in which
> case, should we be slightly more specific than just ACPI in the name label?

The flag  would be meant to say that no ACPI accesses are emulated by
qemu and that would be true for any accesses by PVH guests --- either
CPU or PCI-related.

But the name is somewhat misleading, even without considering other
APCI-related things: we do emulate ACPI, but in the hypervisor and not
in qemu. (As ridiculous as it sounds that actually was one of the
reasons why I didn't use a flag). EMU_NO_DM? But that's the whole PVH thing.


-boris


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