[PATCH 0/2] x86/vMCE: address handling related adjustments

[PATCH 0/2] x86/vMCE: address handling related adjustments

[Jan Beulich]

While going through uses of get_gpfn_from_mfn(), I've noticed
some anomalies here (but of course there are more left). Patch
2 is specifically RFC, for altering the public interface.

1: adjustments to unmmap_broken_page()
2: change address space for incident reporting

Jan

[PATCH 1/2] x86/vMCE: adjustments to unmmap_broken_page()

[Jan Beulich]

There's no need for more than an assertion as to the passed in MFN's
validity, as the caller's prior call to offline_page() would not have
succeeded on an invalid one.

There's no use in checking both is_hvm_domain() and paging_mode_hap(),
as the latter implies the former.

Extend the P2M manipulation that's there also to PVH Dom0, merely having
it using the prior PV Dom0 related behavioral assumption when the page
type cannot be changed (yet).

There's no point in P2M_UNMAP_TYPES including p2m_mmio_direct. The
respective comment is bogus afaict, there are no RAM pages getting
mapped with that type for the purpose of becoming UC. The sole RAM page
getting mapped with this attribute is the (now global) APIC access MFN.
(This page, if it went bad, shouldn't have any effect on the system
anyway, as it never really gets accessed; it's only its address which
matters.)

Make the last function parameter type-safe.

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

--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -91,7 +91,7 @@ mc_memerr_dhandler(struct mca_binfo *bin
                 ASSERT(d);
                 gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
 
-                if ( unmmap_broken_page(d, mfn, gfn) )
+                if ( unmmap_broken_page(d, mfn, _gfn(gfn)) )
                 {
                     printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
                            mfn_x(mfn), d->domain_id);
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -502,11 +502,9 @@ int fill_vmsr_data(struct mcinfo_bank *m
     return ret;
 }
 
-/* It's said some ram is setup as mmio_direct for UC cache attribute */
-#define P2M_UNMAP_TYPES (p2m_to_mask(p2m_ram_rw) \
-                                | p2m_to_mask(p2m_ram_logdirty) \
-                                | p2m_to_mask(p2m_ram_ro)       \
-                                | p2m_to_mask(p2m_mmio_direct))
+#define P2M_UNMAP_TYPES (p2m_to_mask(p2m_ram_rw) | \
+                         p2m_to_mask(p2m_ram_logdirty) | \
+                         p2m_to_mask(p2m_ram_ro))
 
 /*
  * Currently all CPUs are redenzevous at the MCE softirq handler, no
@@ -515,30 +513,25 @@ int fill_vmsr_data(struct mcinfo_bank *m
  * XXX following situation missed:
  * PoD, Foreign mapped, Granted, Shared
  */
-int unmmap_broken_page(struct domain *d, mfn_t mfn, unsigned long gfn)
+int unmmap_broken_page(struct domain *d, mfn_t mfn, gfn_t gfn)
 {
-    mfn_t r_mfn;
     p2m_type_t pt;
     int rc;
 
-    /* Always trust dom0's MCE handler will prevent future access */
-    if ( is_hardware_domain(d) )
-        return 0;
-
-    if ( !mfn_valid(mfn) )
-        return -EINVAL;
-
-    if ( !is_hvm_domain(d) || !paging_mode_hap(d) )
-        return -EOPNOTSUPP;
-
-    rc = -1;
-    r_mfn = get_gfn_query(d, gfn, &pt);
-    if ( p2m_to_mask(pt) & P2M_UNMAP_TYPES)
-    {
-        ASSERT(mfn_eq(r_mfn, mfn));
-        rc = p2m_change_type_one(d, gfn, pt, p2m_ram_broken);
-    }
-    put_gfn(d, gfn);
+    if ( !paging_mode_hap(d) )
+        /* Always trust Dom0's MCE handler will prevent further access. */
+        return is_hardware_domain(d) ? 0 : -EOPNOTSUPP;
+
+    ASSERT(mfn_valid(mfn));
+
+    if ( !mfn_eq(get_gfn_query(d, gfn_x(gfn), &pt), mfn) )
+        rc = -EAGAIN;
+    else if ( p2m_to_mask(pt) & P2M_UNMAP_TYPES )
+        rc = p2m_change_type_one(d, gfn_x(gfn), pt, p2m_ram_broken);
+    else
+        /* Always trust Dom0's MCE handler will prevent further access. */
+        rc = is_hardware_domain(d) ? 0 : -EOPNOTSUPP;
+    put_gfn(d, gfn_x(gfn));
 
     return rc;
 }
--- a/xen/arch/x86/cpu/mcheck/vmce.h
+++ b/xen/arch/x86/cpu/mcheck/vmce.h
@@ -9,7 +9,7 @@ int vmce_init(struct cpuinfo_x86 *c);
     (hardware_domain && \
      evtchn_virq_enabled(domain_vcpu(hardware_domain, 0), VIRQ_MCA))
 
-int unmmap_broken_page(struct domain *d, mfn_t mfn, unsigned long gfn);
+int unmmap_broken_page(struct domain *d, mfn_t mfn, gfn_t gfn);
 
 int vmce_intel_rdmsr(const struct vcpu *, uint32_t msr, uint64_t *val);
 int vmce_intel_wrmsr(struct vcpu *, uint32_t msr, uint64_t val);

[PATCH 2/2] x86/vMCE: change address space for incident reporting

[Jan Beulich]

PFNs are purely a software construct. In particular their association
with MFNs can change at any time. Switch to reporting back GFNs through
the hypercall interface (but stick to PFNs / paddr for the MSR one).
(Note that unmmap_broken_page() validly expects a GFN anyway.)

While doing the adjustments, replace an open-coded instance of
PAGE_OFFSET().

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC because of the change to the hypercall interface, for which the
address space is not documented anywhere anyway. Aiui the main purpose
is to get things logged, and in a (system wide, even if maintained by
Dom0) log system wide meaningful addresses are surely of more use.

--- a/xen/arch/x86/cpu/mcheck/mcaction.c
+++ b/xen/arch/x86/cpu/mcheck/mcaction.c
@@ -1,5 +1,6 @@
 #include <xen/types.h>
 #include <xen/sched.h>
+#include <asm/p2m.h>
 #include "mcaction.h"
 #include "vmce.h"
 #include "mce.h"
@@ -43,7 +44,6 @@ mc_memerr_dhandler(struct mca_binfo *bin
     struct mcinfo_global *global = binfo->mig;
     struct domain *d;
     mfn_t mfn;
-    unsigned long gfn;
     uint32_t status;
     int vmce_vcpuid;
     unsigned int mc_vcpuid;
@@ -87,11 +87,13 @@ mc_memerr_dhandler(struct mca_binfo *bin
             BUG_ON( bank->mc_domid == DOMID_COW );
             if ( bank->mc_domid != DOMID_XEN )
             {
+                gfn_t gfn;
+
                 d = rcu_lock_domain_by_id(bank->mc_domid);
                 ASSERT(d);
-                gfn = get_gpfn_from_mfn((bank->mc_addr) >> PAGE_SHIFT);
 
-                if ( unmmap_broken_page(d, mfn, _gfn(gfn)) )
+                gfn = mfn_to_gfn(d, mfn);
+                if ( unmmap_broken_page(d, mfn, gfn) )
                 {
                     printk("Unmap broken memory %"PRI_mfn" for DOM%d failed\n",
                            mfn_x(mfn), d->domain_id);
@@ -115,8 +117,7 @@ mc_memerr_dhandler(struct mca_binfo *bin
                 else
                     vmce_vcpuid = mc_vcpuid;
 
-                bank->mc_addr = gfn << PAGE_SHIFT |
-                                (bank->mc_addr & (PAGE_SIZE - 1));
+                bank->mc_addr = gfn_to_gaddr(gfn) | PAGE_OFFSET(bank->mc_addr);
                 if ( fill_vmsr_data(bank, d, global->mc_gstatus, vmce_vcpuid) )
                 {
                     mce_printk(MCE_QUIET, "Fill vMCE# data for DOM%d "
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -465,6 +465,7 @@ int fill_vmsr_data(struct mcinfo_bank *m
 {
     struct vcpu *v = d->vcpu[0];
     bool broadcast = (vmce_vcpuid == VMCE_INJECT_BROADCAST);
+    paddr_t addr = mc_bank->mc_addr;
     int ret, err;
 
     if ( mc_bank->mc_domid == DOMID_INVALID )
@@ -479,6 +480,14 @@ int fill_vmsr_data(struct mcinfo_bank *m
     }
 
     /*
+     * Provide a PADDR through the MSR interface, for historical reasons. What
+     * we are being passed is a GADDR (i.e. MADDR for PV and PADDR for HVM).
+     */
+    if ( !paging_mode_translate(d) )
+        addr = pfn_to_paddr(get_gpfn_from_mfn(mfn_x(maddr_to_mfn(addr)))) |
+               PAGE_OFFSET(addr);
+
+    /*
      * vMCE with the actual error information is injected to vCPU0,
      * and, if broadcast is required, we choose to inject less severe
      * vMCEs to other vCPUs. Thus guest can always get the severest
@@ -487,7 +496,7 @@ int fill_vmsr_data(struct mcinfo_bank *m
      * vCPUs will not prevent guest from recovering on those vCPUs.
      */
     ret = vcpu_fill_mc_msrs(v, gstatus, mc_bank->mc_status,
-                            mc_bank->mc_addr, mc_bank->mc_misc);
+                            addr, mc_bank->mc_misc);
     if ( broadcast )
         for_each_vcpu ( d, v )
         {

Ping: [PATCH 0/2] x86/vMCE: address handling related adjustments

[Jan Beulich]

On 28.06.2021 13:56, Jan Beulich wrote:
> While going through uses of get_gpfn_from_mfn(), I've noticed
> some anomalies here (but of course there are more left). Patch
> 2 is specifically RFC, for altering the public interface.
> 
> 1: adjustments to unmmap_broken_page()
> 2: change address space for incident reporting

Anyone?

Thanks, Jan