[PATCH 0/2] Add API for making parts of a MMIO page R/O and use it in XHCI console

[PATCH 0/2] Add API for making parts of a MMIO page R/O and use it in XHCI console

[Marek Marczykowski-Górecki]

On older systems, XHCI xcap had a layout that no other (interesting) registers
were placed on the same page as the debug capability, so Linux was fine with
making the whole page R/O. But at least on Tiger Lake and Alder Lake, Linux
needs to write to some other registers on the same page too.

Add a generic API for making just parts of an MMIO page R/O and use it to fix
USB3 console with share=yes or share=hwdom options. More details in commit
messages.

Marek Marczykowski-Górecki (2):
  x86/mm: add API for marking only part of a MMIO page read only
  drivers/char: Use sub-page ro API to make just xhci dbc cap RO

 xen/arch/x86/hvm/emulate.c      |   2 +-
 xen/arch/x86/hvm/hvm.c          |   3 +-
 xen/arch/x86/include/asm/mm.h   |  22 ++++-
 xen/arch/x86/mm.c               | 181 +++++++++++++++++++++++++++++++++-
 xen/arch/x86/pv/ro-page-fault.c |   1 +-
 xen/drivers/char/xhci-dbc.c     |  38 ++++++-
 6 files changed, 243 insertions(+), 4 deletions(-)

base-commit: b1f11273d5a774cc88a3685c96c2e7cf6385e3b6
-- 
git-series 0.9.1

[PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Marek Marczykowski-Górecki]

In some cases, only few registers on a page needs to be write-protected.
Examples include USB3 console (64 bytes worth of registers) or MSI-X's
PBA table (which doesn't need to span the whole table either).
Current API allows only marking whole pages pages read-only, which
sometimes may cover other registers that guest may need to write into.

Currently, when a guest tries to write to an MMIO page on the
mmio_ro_ranges, it's either immediately crashed on EPT violation - if
that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
from userspace (like, /dev/mem), it will try to fixup by updating page
tables (that Xen again will force to read-only) and will hit that #PF
again (looping endlessly). Both behaviors are undesirable if guest could
actually be allowed the write.

Introduce an API that allows marking part of a page read-only. Since
sub-page permissions are not a thing in page tables, do this via
emulation (or simply page fault handler for PV) that handles writes that
are supposed to be allowed. Those writes require the page to be mapped
to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
page. The page needs to be added to mmio_ro_ranges, first anyway.
Sub-page ranges are stored using rangeset for each added page, and those
pages are stored on a plain list (as there isn't supposed to be many
pages needing this precise r/o control).

The mechanism this API is plugged in is slightly different for PV and
HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
it's already called for #PF on read-only MMIO page. For HVM however, EPT
violation on p2m_mmio_direct page results in a direct domain_crash().
To reach mmio_ro_emulated_write(), change how write violations for
p2m_mmio_direct are handled - specifically, treat them similar to
p2m_ioreq_server. This makes relevant ioreq handler being called,
that finally end up calling mmio_ro_emulated_write().
Both of those paths need an MFN to which guest tried to write (to check
which part of the page is supposed to be read-only, and where
the page is mapped for writes). This information currently isn't
available directly in mmio_ro_emulated_write(), but in both cases it is
already resolved somewhere higher in the call tree. Pass it down to
mmio_ro_emulated_write() via new mmio_ro_emulate_ctxt.mfn field.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Shadow mode is not tested, but I don't expect it to work differently than
HAP in areas related to this patch.
The used locking should make it safe to use similar to mmio_ro_ranges,
but frankly the only use (introduced in the next patch) could go without
locking at all, as subpage_mmio_ro_add() is called only before any
domain is constructed and subpage_mmio_ro_remove() is never called.
---
 xen/arch/x86/hvm/emulate.c      |   2 +-
 xen/arch/x86/hvm/hvm.c          |   3 +-
 xen/arch/x86/include/asm/mm.h   |  22 ++++-
 xen/arch/x86/mm.c               | 181 +++++++++++++++++++++++++++++++++-
 xen/arch/x86/pv/ro-page-fault.c |   1 +-
 5 files changed, 207 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 95364deb1996..311102724dea 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2733,7 +2733,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
         .write      = mmio_ro_emulated_write,
         .validate   = hvmemul_validate,
     };
-    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
+    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = _mfn(mfn) };
     struct hvm_emulate_ctxt ctxt;
     const struct x86_emulate_ops *ops;
     unsigned int seg, bdf;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d326fa1c0136..f1c928e3e4ee 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1942,7 +1942,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
      */
     if ( (p2mt == p2m_mmio_dm) ||
          (npfec.write_access &&
-          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
+          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
+           p2mt == p2m_mmio_direct)) )
     {
         if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
index db29e3e2059f..91937d556bac 100644
--- a/xen/arch/x86/include/asm/mm.h
+++ b/xen/arch/x86/include/asm/mm.h
@@ -522,9 +522,31 @@ extern struct rangeset *mmio_ro_ranges;
 void memguard_guard_stack(void *p);
 void memguard_unguard_stack(void *p);
 
+/*
+ * Add more precise r/o marking for a MMIO page. Bytes range specified here
+ * will still be R/O, but the rest of the page (nor marked as R/O via another
+ * call) will have writes passed through. The write passthrough requires
+ * providing fixmap entry by the caller.
+ * Since multiple callers can mark different areas of the same page, they might
+ * provide different fixmap entries (although that's very unlikely in
+ * practice). Only the one provided by the first caller will be used. Return value
+ * indicates whether this fixmap entry will be used, or a different one
+ * provided earlier (in which case the caller might decide to release it).
+ *
+ * Return values:
+ *  - negative: error
+ *  - 0: success, fixmap entry is claimed
+ *  - 1: success, fixmap entry set earlier will be used
+ */
+int subpage_mmio_ro_add(mfn_t mfn, unsigned long offset_s,
+                        unsigned long offset_e, int fixmap_idx);
+int subpage_mmio_ro_remove(mfn_t mfn, unsigned long offset_s,
+                           unsigned long offset_e);
+
 struct mmio_ro_emulate_ctxt {
         unsigned long cr2;
         unsigned int seg, bdf;
+        mfn_t mfn;
 };
 
 int cf_check mmio_ro_emulated_write(
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 0fe14faa5fa7..b50bdee40b6b 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -165,6 +165,19 @@ bool __read_mostly machine_to_phys_mapping_valid;
 
 struct rangeset *__read_mostly mmio_ro_ranges;
 
+/* Handling sub-page read-only MMIO regions */
+struct subpage_ro_range {
+    struct list_head list;
+    mfn_t mfn;
+    int fixmap_idx;
+    struct rangeset *ro_bytes;
+    struct rcu_head rcu;
+};
+
+static LIST_HEAD(subpage_ro_ranges);
+static DEFINE_RCU_READ_LOCK(subpage_ro_rcu);
+static DEFINE_SPINLOCK(subpage_ro_lock);
+
 static uint32_t base_disallow_mask;
 /* Global bit is allowed to be set on L1 PTEs. Intended for user mappings. */
 #define L1_DISALLOW_MASK ((base_disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL)
@@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     return 0;
 }
 
+int subpage_mmio_ro_add(
+    mfn_t mfn,
+    unsigned long offset_s,
+    unsigned long offset_e,
+    int fixmap_idx)
+{
+    struct subpage_ro_range *entry = NULL, *iter;
+    int rc;
+
+    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
+    ASSERT(offset_s < PAGE_SIZE);
+    ASSERT(offset_e < PAGE_SIZE);
+
+    spin_lock(&subpage_ro_lock);
+
+    list_for_each_entry( iter, &subpage_ro_ranges, list )
+    {
+        if ( mfn_eq(iter->mfn, mfn) )
+        {
+            entry = iter;
+            break;
+        }
+    }
+    if ( !entry )
+    {
+        /* iter==NULL marks it was a newly allocated entry */
+        iter = NULL;
+        entry = xmalloc(struct subpage_ro_range);
+        rc = -ENOMEM;
+        if ( !entry )
+            goto err_unlock;
+        entry->mfn = mfn;
+        entry->fixmap_idx = fixmap_idx;
+        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
+                                       RANGESETF_prettyprint_hex);
+        rc = -ENOMEM;
+        if ( !entry->ro_bytes )
+            goto err_unlock;
+    }
+
+    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
+    if ( rc < 0 )
+        goto err_unlock;
+
+    if ( !iter )
+        list_add_rcu(&entry->list, &subpage_ro_ranges);
+
+    spin_unlock(&subpage_ro_lock);
+
+    if ( !iter || entry->fixmap_idx == fixmap_idx )
+        return 0;
+    else
+        return 1;
+
+err_unlock:
+    spin_unlock(&subpage_ro_lock);
+    if ( !iter )
+    {
+        if ( entry )
+        {
+            if ( entry->ro_bytes )
+                rangeset_destroy(entry->ro_bytes);
+            xfree(entry);
+        }
+    }
+    return rc;
+}
+
+static void subpage_mmio_ro_free(struct rcu_head *rcu)
+{
+    struct subpage_ro_range *entry = container_of(rcu, struct subpage_ro_range, rcu);
+
+    rangeset_destroy(entry->ro_bytes);
+    xfree(entry);
+}
+
+int subpage_mmio_ro_remove(
+    mfn_t mfn,
+    unsigned long offset_s,
+    unsigned long offset_e)
+{
+    struct subpage_ro_range *entry = NULL, *iter;
+    int rc;
+
+    ASSERT(offset_s < PAGE_SIZE);
+    ASSERT(offset_e < PAGE_SIZE);
+
+    spin_lock(&subpage_ro_lock);
+
+    list_for_each_entry_rcu( iter, &subpage_ro_ranges, list )
+    {
+        if ( mfn_eq(iter->mfn, mfn) )
+        {
+            entry = iter;
+            break;
+        }
+    }
+    rc = -ENOENT;
+    if ( !entry )
+        goto out_unlock;
+
+    rc = rangeset_remove_range(entry->ro_bytes, offset_s, offset_e);
+    if ( rc < 0 )
+        goto out_unlock;
+
+    rc = 0;
+
+    if ( !rangeset_is_empty(entry->ro_bytes) )
+        goto out_unlock;
+
+    list_del_rcu(&entry->list);
+    call_rcu(&entry->rcu, subpage_mmio_ro_free);
+
+out_unlock:
+    spin_unlock(&subpage_ro_lock);
+    return rc;
+}
+
+static void subpage_mmio_write_emulate(
+    mfn_t mfn,
+    unsigned long offset,
+    void *data,
+    unsigned int len)
+{
+    struct subpage_ro_range *entry;
+    void __iomem *addr;
+
+    rcu_read_lock(&subpage_ro_rcu);
+
+    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
+    {
+        if ( mfn_eq(entry->mfn, mfn) )
+        {
+            if ( rangeset_overlaps_range(entry->ro_bytes, offset, offset + len - 1) )
+                goto out_unlock;
+
+            addr = fix_to_virt(entry->fixmap_idx) + offset;
+            switch ( len )
+            {
+            case 1:
+                writeb(*(uint8_t*)data, addr);
+                break;
+            case 2:
+                writew(*(uint16_t*)data, addr);
+                break;
+            case 4:
+                writel(*(uint32_t*)data, addr);
+                break;
+            case 8:
+                writeq(*(uint64_t*)data, addr);
+                break;
+            default:
+                /* mmio_ro_emulated_write() already validated the size */
+                ASSERT_UNREACHABLE();
+            }
+            goto out_unlock;
+        }
+    }
+    gdprintk(XENLOG_WARNING,
+             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
+             mfn_x(mfn), offset, len);
+
+out_unlock:
+    rcu_read_unlock(&subpage_ro_rcu);
+}
+
 int cf_check mmio_ro_emulated_write(
     enum x86_segment seg,
     unsigned long offset,
@@ -4911,6 +5090,8 @@ int cf_check mmio_ro_emulated_write(
         return X86EMUL_UNHANDLEABLE;
     }
 
+    subpage_mmio_write_emulate(mmio_ro_ctxt->mfn, offset & (PAGE_SIZE - 1), p_data, bytes);
+
     return X86EMUL_OKAY;
 }
 
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index 5963f5ee2d51..91caa2c8f520 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -342,6 +342,7 @@ static int mmio_ro_do_page_fault(struct x86_emulate_ctxt *ctxt,
             return X86EMUL_UNHANDLEABLE;
     }
 
+    mmio_ro_ctxt.mfn = mfn;
     ctxt->data = &mmio_ro_ctxt;
     if ( pci_ro_mmcfg_decode(mfn_x(mfn), &mmio_ro_ctxt.seg, &mmio_ro_ctxt.bdf) )
         return x86_emulate(ctxt, &mmcfg_intercept_ops);
-- 
git-series 0.9.1

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Roger Pau Monné]

On Mon, Mar 27, 2023 at 12:09:15PM +0200, Marek Marczykowski-Górecki wrote:
> In some cases, only few registers on a page needs to be write-protected.
> Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> PBA table (which doesn't need to span the whole table either).
> Current API allows only marking whole pages pages read-only, which
> sometimes may cover other registers that guest may need to write into.
> 
> Currently, when a guest tries to write to an MMIO page on the
> mmio_ro_ranges, it's either immediately crashed on EPT violation - if
> that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
> from userspace (like, /dev/mem), it will try to fixup by updating page
> tables (that Xen again will force to read-only) and will hit that #PF
> again (looping endlessly). Both behaviors are undesirable if guest could
> actually be allowed the write.
> 
> Introduce an API that allows marking part of a page read-only. Since
> sub-page permissions are not a thing in page tables, do this via
> emulation (or simply page fault handler for PV) that handles writes that
> are supposed to be allowed. Those writes require the page to be mapped
> to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
> page. The page needs to be added to mmio_ro_ranges, first anyway.
> Sub-page ranges are stored using rangeset for each added page, and those
> pages are stored on a plain list (as there isn't supposed to be many
> pages needing this precise r/o control).

Since mmio_ro_ranges is x86 only, it is possible to mutate
it to track address ranges instead of page frames.  The current type
is unsigned long, so that should be fine, and would avoid having to
create a per-page rangeset to just track offsets.

> The mechanism this API is plugged in is slightly different for PV and
> HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
> it's already called for #PF on read-only MMIO page. For HVM however, EPT
> violation on p2m_mmio_direct page results in a direct domain_crash().
> To reach mmio_ro_emulated_write(), change how write violations for
> p2m_mmio_direct are handled - specifically, treat them similar to
> p2m_ioreq_server. This makes relevant ioreq handler being called,
> that finally end up calling mmio_ro_emulated_write().
> Both of those paths need an MFN to which guest tried to write (to check
> which part of the page is supposed to be read-only, and where
> the page is mapped for writes). This information currently isn't
> available directly in mmio_ro_emulated_write(), but in both cases it is
> already resolved somewhere higher in the call tree. Pass it down to
> mmio_ro_emulated_write() via new mmio_ro_emulate_ctxt.mfn field.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Shadow mode is not tested, but I don't expect it to work differently than
> HAP in areas related to this patch.
> The used locking should make it safe to use similar to mmio_ro_ranges,
> but frankly the only use (introduced in the next patch) could go without
> locking at all, as subpage_mmio_ro_add() is called only before any
> domain is constructed and subpage_mmio_ro_remove() is never called.
> ---
>  xen/arch/x86/hvm/emulate.c      |   2 +-
>  xen/arch/x86/hvm/hvm.c          |   3 +-
>  xen/arch/x86/include/asm/mm.h   |  22 ++++-
>  xen/arch/x86/mm.c               | 181 +++++++++++++++++++++++++++++++++-
>  xen/arch/x86/pv/ro-page-fault.c |   1 +-
>  5 files changed, 207 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 95364deb1996..311102724dea 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2733,7 +2733,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
>          .write      = mmio_ro_emulated_write,
>          .validate   = hvmemul_validate,
>      };
> -    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
> +    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = _mfn(mfn) };
>      struct hvm_emulate_ctxt ctxt;
>      const struct x86_emulate_ops *ops;
>      unsigned int seg, bdf;
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d326fa1c0136..f1c928e3e4ee 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1942,7 +1942,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
>       */
>      if ( (p2mt == p2m_mmio_dm) ||
>           (npfec.write_access &&
> -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> +           p2mt == p2m_mmio_direct)) )
>      {
>          if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
>              hvm_inject_hw_exception(TRAP_gp_fault, 0);
> diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> index db29e3e2059f..91937d556bac 100644
> --- a/xen/arch/x86/include/asm/mm.h
> +++ b/xen/arch/x86/include/asm/mm.h
> @@ -522,9 +522,31 @@ extern struct rangeset *mmio_ro_ranges;
>  void memguard_guard_stack(void *p);
>  void memguard_unguard_stack(void *p);
>  
> +/*
> + * Add more precise r/o marking for a MMIO page. Bytes range specified here
> + * will still be R/O, but the rest of the page (nor marked as R/O via another
> + * call) will have writes passed through. The write passthrough requires
> + * providing fixmap entry by the caller.
> + * Since multiple callers can mark different areas of the same page, they might
> + * provide different fixmap entries (although that's very unlikely in
> + * practice). Only the one provided by the first caller will be used. Return value
> + * indicates whether this fixmap entry will be used, or a different one
> + * provided earlier (in which case the caller might decide to release it).

Why not use ioremap() to map the page instead of requiring a fixmap
entry?

> + *
> + * Return values:
> + *  - negative: error
> + *  - 0: success, fixmap entry is claimed
> + *  - 1: success, fixmap entry set earlier will be used
> + */
> +int subpage_mmio_ro_add(mfn_t mfn, unsigned long offset_s,
> +                        unsigned long offset_e, int fixmap_idx);
> +int subpage_mmio_ro_remove(mfn_t mfn, unsigned long offset_s,
> +                           unsigned long offset_e);
> +
>  struct mmio_ro_emulate_ctxt {
>          unsigned long cr2;
>          unsigned int seg, bdf;
> +        mfn_t mfn;
>  };
>  
>  int cf_check mmio_ro_emulated_write(
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index 0fe14faa5fa7..b50bdee40b6b 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -165,6 +165,19 @@ bool __read_mostly machine_to_phys_mapping_valid;
>  
>  struct rangeset *__read_mostly mmio_ro_ranges;
>  
> +/* Handling sub-page read-only MMIO regions */
> +struct subpage_ro_range {
> +    struct list_head list;
> +    mfn_t mfn;
> +    int fixmap_idx;
> +    struct rangeset *ro_bytes;
> +    struct rcu_head rcu;
> +};
> +
> +static LIST_HEAD(subpage_ro_ranges);
> +static DEFINE_RCU_READ_LOCK(subpage_ro_rcu);
> +static DEFINE_SPINLOCK(subpage_ro_lock);
> +
>  static uint32_t base_disallow_mask;
>  /* Global bit is allowed to be set on L1 PTEs. Intended for user mappings. */
>  #define L1_DISALLOW_MASK ((base_disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL)
> @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      return 0;
>  }
>  
> +int subpage_mmio_ro_add(
> +    mfn_t mfn,
> +    unsigned long offset_s,
> +    unsigned long offset_e,

Since those are page offset, you can likely use unsigned int rather
than long?

I also wonder why not use [paddr_t start, paddr_t end] (or start and
size) and drop the mfn parameter?  You can certainly get the mfn from
the full address, and it seems more natural that having the caller
pass an mfn and two offsets.

> +    int fixmap_idx)
> +{
> +    struct subpage_ro_range *entry = NULL, *iter;
> +    int rc;
> +
> +    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
> +    ASSERT(offset_s < PAGE_SIZE);
> +    ASSERT(offset_e < PAGE_SIZE);
> +
> +    spin_lock(&subpage_ro_lock);
> +
> +    list_for_each_entry( iter, &subpage_ro_ranges, list )
> +    {
> +        if ( mfn_eq(iter->mfn, mfn) )
> +        {
> +            entry = iter;
> +            break;
> +        }
> +    }
> +    if ( !entry )
> +    {
> +        /* iter==NULL marks it was a newly allocated entry */
> +        iter = NULL;
> +        entry = xmalloc(struct subpage_ro_range);
> +        rc = -ENOMEM;
> +        if ( !entry )
> +            goto err_unlock;
> +        entry->mfn = mfn;
> +        entry->fixmap_idx = fixmap_idx;
> +        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
> +                                       RANGESETF_prettyprint_hex);
> +        rc = -ENOMEM;

rc will already be -ENOMEM, albeit doing error handling this way is
tricky...

> +        if ( !entry->ro_bytes )
> +            goto err_unlock;
> +    }
> +
> +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
> +    if ( rc < 0 )
> +        goto err_unlock;
> +
> +    if ( !iter )
> +        list_add_rcu(&entry->list, &subpage_ro_ranges);
> +
> +    spin_unlock(&subpage_ro_lock);
> +
> +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> +        return 0;
> +    else
> +        return 1;
> +
> +err_unlock:
> +    spin_unlock(&subpage_ro_lock);
> +    if ( !iter )
> +    {
> +        if ( entry )
> +        {
> +            if ( entry->ro_bytes )
> +                rangeset_destroy(entry->ro_bytes);
> +            xfree(entry);
> +        }
> +    }
> +    return rc;
> +}
> +
> +static void subpage_mmio_ro_free(struct rcu_head *rcu)
> +{
> +    struct subpage_ro_range *entry = container_of(rcu, struct subpage_ro_range, rcu);
> +
> +    rangeset_destroy(entry->ro_bytes);
> +    xfree(entry);
> +}
> +
> +int subpage_mmio_ro_remove(
> +    mfn_t mfn,
> +    unsigned long offset_s,
> +    unsigned long offset_e)
> +{
> +    struct subpage_ro_range *entry = NULL, *iter;
> +    int rc;
> +
> +    ASSERT(offset_s < PAGE_SIZE);
> +    ASSERT(offset_e < PAGE_SIZE);
> +
> +    spin_lock(&subpage_ro_lock);
> +
> +    list_for_each_entry_rcu( iter, &subpage_ro_ranges, list )
> +    {
> +        if ( mfn_eq(iter->mfn, mfn) )
> +        {
> +            entry = iter;
> +            break;
> +        }
> +    }
> +    rc = -ENOENT;
> +    if ( !entry )
> +        goto out_unlock;
> +
> +    rc = rangeset_remove_range(entry->ro_bytes, offset_s, offset_e);
> +    if ( rc < 0 )
> +        goto out_unlock;
> +
> +    rc = 0;

You can use `if ( rc ) goto out_unlock;` and that avoids having to
explicitly set rc to 0.

> +
> +    if ( !rangeset_is_empty(entry->ro_bytes) )
> +        goto out_unlock;
> +
> +    list_del_rcu(&entry->list);
> +    call_rcu(&entry->rcu, subpage_mmio_ro_free);
> +
> +out_unlock:
> +    spin_unlock(&subpage_ro_lock);
> +    return rc;
> +}
> +
> +static void subpage_mmio_write_emulate(
> +    mfn_t mfn,
> +    unsigned long offset,
> +    void *data,
> +    unsigned int len)
> +{
> +    struct subpage_ro_range *entry;

const.

> +    void __iomem *addr;

Do we care about the access being aligned?

> +
> +    rcu_read_lock(&subpage_ro_rcu);
> +
> +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
> +    {
> +        if ( mfn_eq(entry->mfn, mfn) )
> +        {

You need to take the spinlock at this point, since the contents of
entry->ro_bytes are not protected by the RCU.  The RCU only provides
safe iteration over the list, but not the content of the elements on
the list.

Thanks, Roger.

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 13587 bytes --]

On Tue, Mar 28, 2023 at 04:04:47PM +0200, Roger Pau Monné wrote:
> On Mon, Mar 27, 2023 at 12:09:15PM +0200, Marek Marczykowski-Górecki wrote:
> > In some cases, only few registers on a page needs to be write-protected.
> > Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> > PBA table (which doesn't need to span the whole table either).
> > Current API allows only marking whole pages pages read-only, which
> > sometimes may cover other registers that guest may need to write into.
> > 
> > Currently, when a guest tries to write to an MMIO page on the
> > mmio_ro_ranges, it's either immediately crashed on EPT violation - if
> > that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
> > from userspace (like, /dev/mem), it will try to fixup by updating page
> > tables (that Xen again will force to read-only) and will hit that #PF
> > again (looping endlessly). Both behaviors are undesirable if guest could
> > actually be allowed the write.
> > 
> > Introduce an API that allows marking part of a page read-only. Since
> > sub-page permissions are not a thing in page tables, do this via
> > emulation (or simply page fault handler for PV) that handles writes that
> > are supposed to be allowed. Those writes require the page to be mapped
> > to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
> > page. The page needs to be added to mmio_ro_ranges, first anyway.
> > Sub-page ranges are stored using rangeset for each added page, and those
> > pages are stored on a plain list (as there isn't supposed to be many
> > pages needing this precise r/o control).
> 
> Since mmio_ro_ranges is x86 only, it is possible to mutate
> it to track address ranges instead of page frames.  The current type
> is unsigned long, so that should be fine, and would avoid having to
> create a per-page rangeset to just track offsets.

I was thinking about it, but rangeset doesn't allow attaching extra data
(fixmap index, or mapped address as you propose with ioremap()).
Changing all the places where mmio_ro_ranges is used will be a bit
tedious, but that isn't really a problem.

> > The mechanism this API is plugged in is slightly different for PV and
> > HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
> > it's already called for #PF on read-only MMIO page. For HVM however, EPT
> > violation on p2m_mmio_direct page results in a direct domain_crash().
> > To reach mmio_ro_emulated_write(), change how write violations for
> > p2m_mmio_direct are handled - specifically, treat them similar to
> > p2m_ioreq_server. This makes relevant ioreq handler being called,
> > that finally end up calling mmio_ro_emulated_write().
> > Both of those paths need an MFN to which guest tried to write (to check
> > which part of the page is supposed to be read-only, and where
> > the page is mapped for writes). This information currently isn't
> > available directly in mmio_ro_emulated_write(), but in both cases it is
> > already resolved somewhere higher in the call tree. Pass it down to
> > mmio_ro_emulated_write() via new mmio_ro_emulate_ctxt.mfn field.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > Shadow mode is not tested, but I don't expect it to work differently than
> > HAP in areas related to this patch.
> > The used locking should make it safe to use similar to mmio_ro_ranges,
> > but frankly the only use (introduced in the next patch) could go without
> > locking at all, as subpage_mmio_ro_add() is called only before any
> > domain is constructed and subpage_mmio_ro_remove() is never called.
> > ---
> >  xen/arch/x86/hvm/emulate.c      |   2 +-
> >  xen/arch/x86/hvm/hvm.c          |   3 +-
> >  xen/arch/x86/include/asm/mm.h   |  22 ++++-
> >  xen/arch/x86/mm.c               | 181 +++++++++++++++++++++++++++++++++-
> >  xen/arch/x86/pv/ro-page-fault.c |   1 +-
> >  5 files changed, 207 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index 95364deb1996..311102724dea 100644
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -2733,7 +2733,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
> >          .write      = mmio_ro_emulated_write,
> >          .validate   = hvmemul_validate,
> >      };
> > -    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
> > +    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = _mfn(mfn) };
> >      struct hvm_emulate_ctxt ctxt;
> >      const struct x86_emulate_ops *ops;
> >      unsigned int seg, bdf;
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index d326fa1c0136..f1c928e3e4ee 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -1942,7 +1942,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> >       */
> >      if ( (p2mt == p2m_mmio_dm) ||
> >           (npfec.write_access &&
> > -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> > +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> > +           p2mt == p2m_mmio_direct)) )
> >      {
> >          if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
> >              hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> > index db29e3e2059f..91937d556bac 100644
> > --- a/xen/arch/x86/include/asm/mm.h
> > +++ b/xen/arch/x86/include/asm/mm.h
> > @@ -522,9 +522,31 @@ extern struct rangeset *mmio_ro_ranges;
> >  void memguard_guard_stack(void *p);
> >  void memguard_unguard_stack(void *p);
> >  
> > +/*
> > + * Add more precise r/o marking for a MMIO page. Bytes range specified here
> > + * will still be R/O, but the rest of the page (nor marked as R/O via another
> > + * call) will have writes passed through. The write passthrough requires
> > + * providing fixmap entry by the caller.
> > + * Since multiple callers can mark different areas of the same page, they might
> > + * provide different fixmap entries (although that's very unlikely in
> > + * practice). Only the one provided by the first caller will be used. Return value
> > + * indicates whether this fixmap entry will be used, or a different one
> > + * provided earlier (in which case the caller might decide to release it).
> 
> Why not use ioremap() to map the page instead of requiring a fixmap
> entry?

In all the cases this feature is used (for now), I do have a fixmap
anyway. So, I don't need to worry if I can call ioremap() at that boot
stage (I think it's okay in console_init_postirq(), but that may not
be obvious in other places).

> > + *
> > + * Return values:
> > + *  - negative: error
> > + *  - 0: success, fixmap entry is claimed
> > + *  - 1: success, fixmap entry set earlier will be used
> > + */
> > +int subpage_mmio_ro_add(mfn_t mfn, unsigned long offset_s,
> > +                        unsigned long offset_e, int fixmap_idx);
> > +int subpage_mmio_ro_remove(mfn_t mfn, unsigned long offset_s,
> > +                           unsigned long offset_e);
> > +
> >  struct mmio_ro_emulate_ctxt {
> >          unsigned long cr2;
> >          unsigned int seg, bdf;
> > +        mfn_t mfn;
> >  };
> >  
> >  int cf_check mmio_ro_emulated_write(
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 0fe14faa5fa7..b50bdee40b6b 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -165,6 +165,19 @@ bool __read_mostly machine_to_phys_mapping_valid;
> >  
> >  struct rangeset *__read_mostly mmio_ro_ranges;
> >  
> > +/* Handling sub-page read-only MMIO regions */
> > +struct subpage_ro_range {
> > +    struct list_head list;
> > +    mfn_t mfn;
> > +    int fixmap_idx;
> > +    struct rangeset *ro_bytes;
> > +    struct rcu_head rcu;
> > +};
> > +
> > +static LIST_HEAD(subpage_ro_ranges);
> > +static DEFINE_RCU_READ_LOCK(subpage_ro_rcu);
> > +static DEFINE_SPINLOCK(subpage_ro_lock);
> > +
> >  static uint32_t base_disallow_mask;
> >  /* Global bit is allowed to be set on L1 PTEs. Intended for user mappings. */
> >  #define L1_DISALLOW_MASK ((base_disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL)
> > @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      return 0;
> >  }
> >  
> > +int subpage_mmio_ro_add(
> > +    mfn_t mfn,
> > +    unsigned long offset_s,
> > +    unsigned long offset_e,
> 
> Since those are page offset, you can likely use unsigned int rather
> than long?
> 
> I also wonder why not use [paddr_t start, paddr_t end] (or start and
> size) and drop the mfn parameter?  You can certainly get the mfn from
> the full address, and it seems more natural that having the caller
> pass an mfn and two offsets.

That would work for the function parameters indeed, regardless of what's
really stored.

> > +    int fixmap_idx)
> > +{
> > +    struct subpage_ro_range *entry = NULL, *iter;
> > +    int rc;
> > +
> > +    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
> > +    ASSERT(offset_s < PAGE_SIZE);
> > +    ASSERT(offset_e < PAGE_SIZE);
> > +
> > +    spin_lock(&subpage_ro_lock);
> > +
> > +    list_for_each_entry( iter, &subpage_ro_ranges, list )
> > +    {
> > +        if ( mfn_eq(iter->mfn, mfn) )
> > +        {
> > +            entry = iter;
> > +            break;
> > +        }
> > +    }
> > +    if ( !entry )
> > +    {
> > +        /* iter==NULL marks it was a newly allocated entry */
> > +        iter = NULL;
> > +        entry = xmalloc(struct subpage_ro_range);
> > +        rc = -ENOMEM;
> > +        if ( !entry )
> > +            goto err_unlock;
> > +        entry->mfn = mfn;
> > +        entry->fixmap_idx = fixmap_idx;
> > +        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
> > +                                       RANGESETF_prettyprint_hex);
> > +        rc = -ENOMEM;
> 
> rc will already be -ENOMEM, albeit doing error handling this way is
> tricky...
> 
> > +        if ( !entry->ro_bytes )
> > +            goto err_unlock;
> > +    }
> > +
> > +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
> > +    if ( rc < 0 )
> > +        goto err_unlock;
> > +
> > +    if ( !iter )
> > +        list_add_rcu(&entry->list, &subpage_ro_ranges);
> > +
> > +    spin_unlock(&subpage_ro_lock);
> > +
> > +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> > +        return 0;
> > +    else
> > +        return 1;
> > +
> > +err_unlock:
> > +    spin_unlock(&subpage_ro_lock);
> > +    if ( !iter )
> > +    {
> > +        if ( entry )
> > +        {
> > +            if ( entry->ro_bytes )
> > +                rangeset_destroy(entry->ro_bytes);
> > +            xfree(entry);
> > +        }
> > +    }
> > +    return rc;
> > +}
> > +
> > +static void subpage_mmio_ro_free(struct rcu_head *rcu)
> > +{
> > +    struct subpage_ro_range *entry = container_of(rcu, struct subpage_ro_range, rcu);
> > +
> > +    rangeset_destroy(entry->ro_bytes);
> > +    xfree(entry);
> > +}
> > +
> > +int subpage_mmio_ro_remove(
> > +    mfn_t mfn,
> > +    unsigned long offset_s,
> > +    unsigned long offset_e)
> > +{
> > +    struct subpage_ro_range *entry = NULL, *iter;
> > +    int rc;
> > +
> > +    ASSERT(offset_s < PAGE_SIZE);
> > +    ASSERT(offset_e < PAGE_SIZE);
> > +
> > +    spin_lock(&subpage_ro_lock);
> > +
> > +    list_for_each_entry_rcu( iter, &subpage_ro_ranges, list )
> > +    {
> > +        if ( mfn_eq(iter->mfn, mfn) )
> > +        {
> > +            entry = iter;
> > +            break;
> > +        }
> > +    }
> > +    rc = -ENOENT;
> > +    if ( !entry )
> > +        goto out_unlock;
> > +
> > +    rc = rangeset_remove_range(entry->ro_bytes, offset_s, offset_e);
> > +    if ( rc < 0 )
> > +        goto out_unlock;
> > +
> > +    rc = 0;
> 
> You can use `if ( rc ) goto out_unlock;` and that avoids having to
> explicitly set rc to 0.
> 
> > +
> > +    if ( !rangeset_is_empty(entry->ro_bytes) )
> > +        goto out_unlock;
> > +
> > +    list_del_rcu(&entry->list);
> > +    call_rcu(&entry->rcu, subpage_mmio_ro_free);
> > +
> > +out_unlock:
> > +    spin_unlock(&subpage_ro_lock);
> > +    return rc;
> > +}
> > +
> > +static void subpage_mmio_write_emulate(
> > +    mfn_t mfn,
> > +    unsigned long offset,
> > +    void *data,
> > +    unsigned int len)
> > +{
> > +    struct subpage_ro_range *entry;
> 
> const.
> 
> > +    void __iomem *addr;
> 
> Do we care about the access being aligned?

I don't think Xen cares about it when page is mapped R/W to the guest,
so why should it care when it's partially R/W only?

> 
> > +
> > +    rcu_read_lock(&subpage_ro_rcu);
> > +
> > +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
> > +    {
> > +        if ( mfn_eq(entry->mfn, mfn) )
> > +        {
> 
> You need to take the spinlock at this point, since the contents of
> entry->ro_bytes are not protected by the RCU.  The RCU only provides
> safe iteration over the list, but not the content of the elements on
> the list.

mfn is not supposed to change ever on the specific list element, and
IIUC, rangeset does locking itself. Am I missing something?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Roger Pau Monné]

On Tue, Mar 28, 2023 at 04:49:36PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, Mar 28, 2023 at 04:04:47PM +0200, Roger Pau Monné wrote:
> > On Mon, Mar 27, 2023 at 12:09:15PM +0200, Marek Marczykowski-Górecki wrote:
> > > In some cases, only few registers on a page needs to be write-protected.
> > > Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> > > PBA table (which doesn't need to span the whole table either).
> > > Current API allows only marking whole pages pages read-only, which
> > > sometimes may cover other registers that guest may need to write into.
> > > 
> > > Currently, when a guest tries to write to an MMIO page on the
> > > mmio_ro_ranges, it's either immediately crashed on EPT violation - if
> > > that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
> > > from userspace (like, /dev/mem), it will try to fixup by updating page
> > > tables (that Xen again will force to read-only) and will hit that #PF
> > > again (looping endlessly). Both behaviors are undesirable if guest could
> > > actually be allowed the write.
> > > 
> > > Introduce an API that allows marking part of a page read-only. Since
> > > sub-page permissions are not a thing in page tables, do this via
> > > emulation (or simply page fault handler for PV) that handles writes that
> > > are supposed to be allowed. Those writes require the page to be mapped
> > > to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
> > > page. The page needs to be added to mmio_ro_ranges, first anyway.
> > > Sub-page ranges are stored using rangeset for each added page, and those
> > > pages are stored on a plain list (as there isn't supposed to be many
> > > pages needing this precise r/o control).
> > 
> > Since mmio_ro_ranges is x86 only, it is possible to mutate
> > it to track address ranges instead of page frames.  The current type
> > is unsigned long, so that should be fine, and would avoid having to
> > create a per-page rangeset to just track offsets.
> 
> I was thinking about it, but rangeset doesn't allow attaching extra data
> (fixmap index, or mapped address as you propose with ioremap()).
> Changing all the places where mmio_ro_ranges is used will be a bit
> tedious, but that isn't really a problem.

Yes, you would still need to keep a separate structure in order to
store the maps, a simple list that contains the physical mfn and the
virtual address where it's mapped would suffice I think.  That would
avoid creating a separate rangeset for each mfn that you need to
track.

> > > The mechanism this API is plugged in is slightly different for PV and
> > > HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
> > > it's already called for #PF on read-only MMIO page. For HVM however, EPT
> > > violation on p2m_mmio_direct page results in a direct domain_crash().
> > > To reach mmio_ro_emulated_write(), change how write violations for
> > > p2m_mmio_direct are handled - specifically, treat them similar to
> > > p2m_ioreq_server. This makes relevant ioreq handler being called,
> > > that finally end up calling mmio_ro_emulated_write().
> > > Both of those paths need an MFN to which guest tried to write (to check
> > > which part of the page is supposed to be read-only, and where
> > > the page is mapped for writes). This information currently isn't
> > > available directly in mmio_ro_emulated_write(), but in both cases it is
> > > already resolved somewhere higher in the call tree. Pass it down to
> > > mmio_ro_emulated_write() via new mmio_ro_emulate_ctxt.mfn field.
> > > 
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > ---
> > > Shadow mode is not tested, but I don't expect it to work differently than
> > > HAP in areas related to this patch.
> > > The used locking should make it safe to use similar to mmio_ro_ranges,
> > > but frankly the only use (introduced in the next patch) could go without
> > > locking at all, as subpage_mmio_ro_add() is called only before any
> > > domain is constructed and subpage_mmio_ro_remove() is never called.
> > > ---
> > >  xen/arch/x86/hvm/emulate.c      |   2 +-
> > >  xen/arch/x86/hvm/hvm.c          |   3 +-
> > >  xen/arch/x86/include/asm/mm.h   |  22 ++++-
> > >  xen/arch/x86/mm.c               | 181 +++++++++++++++++++++++++++++++++-
> > >  xen/arch/x86/pv/ro-page-fault.c |   1 +-
> > >  5 files changed, 207 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > > index 95364deb1996..311102724dea 100644
> > > --- a/xen/arch/x86/hvm/emulate.c
> > > +++ b/xen/arch/x86/hvm/emulate.c
> > > @@ -2733,7 +2733,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
> > >          .write      = mmio_ro_emulated_write,
> > >          .validate   = hvmemul_validate,
> > >      };
> > > -    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
> > > +    struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla, .mfn = _mfn(mfn) };
> > >      struct hvm_emulate_ctxt ctxt;
> > >      const struct x86_emulate_ops *ops;
> > >      unsigned int seg, bdf;
> > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > index d326fa1c0136..f1c928e3e4ee 100644
> > > --- a/xen/arch/x86/hvm/hvm.c
> > > +++ b/xen/arch/x86/hvm/hvm.c
> > > @@ -1942,7 +1942,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long gla,
> > >       */
> > >      if ( (p2mt == p2m_mmio_dm) ||
> > >           (npfec.write_access &&
> > > -          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server))) )
> > > +          (p2m_is_discard_write(p2mt) || (p2mt == p2m_ioreq_server) ||
> > > +           p2mt == p2m_mmio_direct)) )
> > >      {
> > >          if ( !handle_mmio_with_translation(gla, gpa >> PAGE_SHIFT, npfec) )
> > >              hvm_inject_hw_exception(TRAP_gp_fault, 0);
> > > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> > > index db29e3e2059f..91937d556bac 100644
> > > --- a/xen/arch/x86/include/asm/mm.h
> > > +++ b/xen/arch/x86/include/asm/mm.h
> > > @@ -522,9 +522,31 @@ extern struct rangeset *mmio_ro_ranges;
> > >  void memguard_guard_stack(void *p);
> > >  void memguard_unguard_stack(void *p);
> > >  
> > > +/*
> > > + * Add more precise r/o marking for a MMIO page. Bytes range specified here
> > > + * will still be R/O, but the rest of the page (nor marked as R/O via another
> > > + * call) will have writes passed through. The write passthrough requires
> > > + * providing fixmap entry by the caller.
> > > + * Since multiple callers can mark different areas of the same page, they might
> > > + * provide different fixmap entries (although that's very unlikely in
> > > + * practice). Only the one provided by the first caller will be used. Return value
> > > + * indicates whether this fixmap entry will be used, or a different one
> > > + * provided earlier (in which case the caller might decide to release it).
> > 
> > Why not use ioremap() to map the page instead of requiring a fixmap
> > entry?
> 
> In all the cases this feature is used (for now), I do have a fixmap
> anyway. So, I don't need to worry if I can call ioremap() at that boot
> stage (I think it's okay in console_init_postirq(), but that may not
> be obvious in other places).

If we moved for a model where mmio_ro_ranges tracks at address
granularity I would suggest that the underlying addresses are only
mapped when there's a guest access to handle, so that we don't waste
fixmap entries for no reason.

> > > +        if ( !entry->ro_bytes )
> > > +            goto err_unlock;
> > > +    }
> > > +
> > > +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
> > > +    if ( rc < 0 )
> > > +        goto err_unlock;
> > > +
> > > +    if ( !iter )
> > > +        list_add_rcu(&entry->list, &subpage_ro_ranges);
> > > +
> > > +    spin_unlock(&subpage_ro_lock);
> > > +
> > > +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> > > +        return 0;
> > > +    else
> > > +        return 1;
> > > +
> > > +err_unlock:
> > > +    spin_unlock(&subpage_ro_lock);
> > > +    if ( !iter )
> > > +    {
> > > +        if ( entry )
> > > +        {
> > > +            if ( entry->ro_bytes )
> > > +                rangeset_destroy(entry->ro_bytes);
> > > +            xfree(entry);
> > > +        }
> > > +    }
> > > +    return rc;
> > > +}
> > > +
> > > +static void subpage_mmio_ro_free(struct rcu_head *rcu)
> > > +{
> > > +    struct subpage_ro_range *entry = container_of(rcu, struct subpage_ro_range, rcu);

Overly long line.

Out of precaution I would also add an extra
ASSERT(rangeset_is_empty(entry->ro_bytes)) here.

> > > +
> > > +    if ( !rangeset_is_empty(entry->ro_bytes) )
> > > +        goto out_unlock;
> > > +
> > > +    list_del_rcu(&entry->list);
> > > +    call_rcu(&entry->rcu, subpage_mmio_ro_free);
> > > +
> > > +out_unlock:
> > > +    spin_unlock(&subpage_ro_lock);
> > > +    return rc;
> > > +}
> > > +
> > > +static void subpage_mmio_write_emulate(
> > > +    mfn_t mfn,
> > > +    unsigned long offset,
> > > +    void *data,
> > > +    unsigned int len)
> > > +{
> > > +    struct subpage_ro_range *entry;
> > 
> > const.
> > 
> > > +    void __iomem *addr;
> > 
> > Do we care about the access being aligned?
> 
> I don't think Xen cares about it when page is mapped R/W to the guest,
> so why should it care when it's partially R/W only?

It's kind of the same issue we have with adjacent MSIX accesses: we
don't really know whether the device might choke on unaligned accesses
and generate some kind of exception that results in a NMI to the CPU.
Handling those it's likely more feasible if they happen in guest
context, rather than Xen context.

> > 
> > > +
> > > +    rcu_read_lock(&subpage_ro_rcu);
> > > +
> > > +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
> > > +    {
> > > +        if ( mfn_eq(entry->mfn, mfn) )
> > > +        {
> > 
> > You need to take the spinlock at this point, since the contents of
> > entry->ro_bytes are not protected by the RCU.  The RCU only provides
> > safe iteration over the list, but not the content of the elements on
> > the list.
> 
> mfn is not supposed to change ever on the specific list element, and
> IIUC, rangeset does locking itself. Am I missing something?

Oh, sorry, it was me being wrong, I didn't recall and didn't check that
rangesets do have internal locking.

Thanks, Roger.

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Jan Beulich]

On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> In some cases, only few registers on a page needs to be write-protected.
> Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> PBA table (which doesn't need to span the whole table either).

Yet like the MSI-X table the PBA is not permitted to share a page with
other registers (the table itself excluded). So a need there would
(again) only arise for devices violating the spec.

> Current API allows only marking whole pages pages read-only, which
> sometimes may cover other registers that guest may need to write into.
> 
> Currently, when a guest tries to write to an MMIO page on the
> mmio_ro_ranges, it's either immediately crashed on EPT violation - if
> that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
> from userspace (like, /dev/mem), it will try to fixup by updating page
> tables (that Xen again will force to read-only) and will hit that #PF
> again (looping endlessly). Both behaviors are undesirable if guest could
> actually be allowed the write.
> 
> Introduce an API that allows marking part of a page read-only. Since
> sub-page permissions are not a thing in page tables, do this via
> emulation (or simply page fault handler for PV) that handles writes that
> are supposed to be allowed. Those writes require the page to be mapped
> to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
> page. The page needs to be added to mmio_ro_ranges, first anyway.
> Sub-page ranges are stored using rangeset for each added page, and those
> pages are stored on a plain list (as there isn't supposed to be many
> pages needing this precise r/o control).

While, unlike Roger, I don't want to see mmio_ro_ranges' granularity
changed, I wonder if a bitmap isn't going to be easier to use (even
if perhaps requiring a little more memory, but whether that's the case
depends on intended granularity - I'm not convinced we need byte-
granular ranges). I'm also worried of this yet more heavily tying to
x86 of (as of the next patch) the USB3 debug console driver (i.e. as
opposed to Roger I wouldn't take anything that's x86-only as
justification for making wider changes).

As to sub-page permissions: EPT has, as one of its extensions, support
for this. It might be worth at least mentioning, even if the feature
isn't intended to be used right here.

Taking both remarks together, limiting granularity to 16(?) bytes
would allow using the advanced EPT functionality down the road, and
would at the same time limit the suggested bitmap to just 256 bits /
32 bytes, which I think gets us below what even an empty rangeset
would require. Plus lookup would also be quite a bit more lightweight.

> The mechanism this API is plugged in is slightly different for PV and
> HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
> it's already called for #PF on read-only MMIO page. For HVM however, EPT
> violation on p2m_mmio_direct page results in a direct domain_crash().
> To reach mmio_ro_emulated_write(), change how write violations for
> p2m_mmio_direct are handled - specifically, treat them similar to
> p2m_ioreq_server. This makes relevant ioreq handler being called,
> that finally end up calling mmio_ro_emulated_write().

I wonder whether that isn't too heavy a change to existing behavior.
I understand that taking this path is necessary for the purpose of the
patch, but I don't think it is necessary for all p2m_mmio_direct pages.
Checking at least mmio_ro_ranges right in hvm_hap_nested_page_fault()
looks reasonable to me.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -165,6 +165,19 @@ bool __read_mostly machine_to_phys_mapping_valid;
>  
>  struct rangeset *__read_mostly mmio_ro_ranges;
>  
> +/* Handling sub-page read-only MMIO regions */
> +struct subpage_ro_range {
> +    struct list_head list;
> +    mfn_t mfn;
> +    int fixmap_idx;
> +    struct rangeset *ro_bytes;
> +    struct rcu_head rcu;
> +};
> +
> +static LIST_HEAD(subpage_ro_ranges);
> +static DEFINE_RCU_READ_LOCK(subpage_ro_rcu);
> +static DEFINE_SPINLOCK(subpage_ro_lock);
> +
>  static uint32_t base_disallow_mask;
>  /* Global bit is allowed to be set on L1 PTEs. Intended for user mappings. */
>  #define L1_DISALLOW_MASK ((base_disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL)
> @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>      return 0;
>  }
>  
> +int subpage_mmio_ro_add(

As long as patch 2 is going to add the only users, __init please, and
there's no need for a "remove" counterpart.

> +    mfn_t mfn,
> +    unsigned long offset_s,
> +    unsigned long offset_e,
> +    int fixmap_idx)

enum fixed_addresses?

> +{
> +    struct subpage_ro_range *entry = NULL, *iter;
> +    int rc;
> +
> +    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
> +    ASSERT(offset_s < PAGE_SIZE);
> +    ASSERT(offset_e < PAGE_SIZE);
> +
> +    spin_lock(&subpage_ro_lock);
> +
> +    list_for_each_entry( iter, &subpage_ro_ranges, list )

Nit: Style (either you view list_for_each_entry as a [pseudo]keyword
for the purposes of what blanks should be present/absent, or you don't,
a mixture isn't reasonable; also elsewhere).

> +    {
> +        if ( mfn_eq(iter->mfn, mfn) )
> +        {
> +            entry = iter;
> +            break;
> +        }
> +    }
> +    if ( !entry )
> +    {
> +        /* iter==NULL marks it was a newly allocated entry */
> +        iter = NULL;
> +        entry = xmalloc(struct subpage_ro_range);
> +        rc = -ENOMEM;
> +        if ( !entry )
> +            goto err_unlock;
> +        entry->mfn = mfn;
> +        entry->fixmap_idx = fixmap_idx;
> +        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
> +                                       RANGESETF_prettyprint_hex);
> +        rc = -ENOMEM;
> +        if ( !entry->ro_bytes )
> +            goto err_unlock;
> +    }
> +
> +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
> +    if ( rc < 0 )
> +        goto err_unlock;
> +
> +    if ( !iter )
> +        list_add_rcu(&entry->list, &subpage_ro_ranges);
> +
> +    spin_unlock(&subpage_ro_lock);
> +
> +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> +        return 0;
> +    else
> +        return 1;

If this case is really needed, this special return value (as documented
in the header) probably needs specially handling in the callers - it's
not necessarily an error after all. But I wonder whether it wouldn't be
easier to check earlier on and fail right away (with e.g. -EBUSY), rather
than adding the range and _then_ (kind of, as per patch 2) failing.

> +err_unlock:

Nit: Style (labels indented by at least one space please, also elsewhere).

> +static void subpage_mmio_write_emulate(
> +    mfn_t mfn,
> +    unsigned long offset,

unsigned int

> +    void *data,

const

> +    unsigned int len)
> +{
> +    struct subpage_ro_range *entry;
> +    void __iomem *addr;
> +
> +    rcu_read_lock(&subpage_ro_rcu);
> +
> +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
> +    {
> +        if ( mfn_eq(entry->mfn, mfn) )
> +        {
> +            if ( rangeset_overlaps_range(entry->ro_bytes, offset, offset + len - 1) )
> +                goto out_unlock;

This case ...

> +            addr = fix_to_virt(entry->fixmap_idx) + offset;
> +            switch ( len )
> +            {
> +            case 1:
> +                writeb(*(uint8_t*)data, addr);
> +                break;
> +            case 2:
> +                writew(*(uint16_t*)data, addr);
> +                break;
> +            case 4:
> +                writel(*(uint32_t*)data, addr);
> +                break;
> +            case 8:
> +                writeq(*(uint64_t*)data, addr);
> +                break;
> +            default:
> +                /* mmio_ro_emulated_write() already validated the size */
> +                ASSERT_UNREACHABLE();

... as well as, in a release build, this one wants to ...

> +            }
> +            goto out_unlock;

... not use this path but ...

> +        }
> +    }
> +    gdprintk(XENLOG_WARNING,
> +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
> +             mfn_x(mfn), offset, len);

... make it here. (By implication: I'm not convinced this wants to be a
gdprintk(), as I think at least one such warning would better surface in
release builds as well.)

At the same time I don't think any message should be issued for write
attempts to pages which don't have part of it marked writable. We deal
with such silently right now, and this shouldn't change. In fact even
for ranges which don't overlap the writable portion of a page we may want
to consider remaining silent. The log message ought to be of interest
mainly for writes which we meant to permit, but which for whatever reason
we elect to suppress nevertheless.

As to the message - why do you split MFN from offset, rather than simply
logging an address ("... %" PRI_mfn "%03x ...")?

Like (iirc) Roger I think that misaligned accesses should be refused (and
hence also make it here).

Jan

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 11518 bytes --]

On Wed, Mar 29, 2023 at 10:50:20AM +0200, Jan Beulich wrote:
> On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> > In some cases, only few registers on a page needs to be write-protected.
> > Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> > PBA table (which doesn't need to span the whole table either).
> 
> Yet like the MSI-X table the PBA is not permitted to share a page with
> other registers (the table itself excluded). So a need there would
> (again) only arise for devices violating the spec.

For PBA, indeed (and due to not seeing device needing such hack, I don't
do that for PBA yet). But the XHCI spec doesn't include such limitation, so
this case is perfectly valid.

> > Current API allows only marking whole pages pages read-only, which
> > sometimes may cover other registers that guest may need to write into.
> > 
> > Currently, when a guest tries to write to an MMIO page on the
> > mmio_ro_ranges, it's either immediately crashed on EPT violation - if
> > that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
> > from userspace (like, /dev/mem), it will try to fixup by updating page
> > tables (that Xen again will force to read-only) and will hit that #PF
> > again (looping endlessly). Both behaviors are undesirable if guest could
> > actually be allowed the write.
> > 
> > Introduce an API that allows marking part of a page read-only. Since
> > sub-page permissions are not a thing in page tables, do this via
> > emulation (or simply page fault handler for PV) that handles writes that
> > are supposed to be allowed. Those writes require the page to be mapped
> > to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
> > page. The page needs to be added to mmio_ro_ranges, first anyway.
> > Sub-page ranges are stored using rangeset for each added page, and those
> > pages are stored on a plain list (as there isn't supposed to be many
> > pages needing this precise r/o control).
> 
> While, unlike Roger, I don't want to see mmio_ro_ranges' granularity
> changed, I wonder if a bitmap isn't going to be easier to use (even
> if perhaps requiring a little more memory, but whether that's the case
> depends on intended granularity - I'm not convinced we need byte-
> granular ranges). I'm also worried of this yet more heavily tying to
> x86 of (as of the next patch) the USB3 debug console driver (i.e. as
> opposed to Roger I wouldn't take anything that's x86-only as
> justification for making wider changes).

Well, it's still under the very same CONFIG_X86, and for the same
purpose, so I don't think it's "more heavily tying".

> As to sub-page permissions: EPT has, as one of its extensions, support
> for this. It might be worth at least mentioning, even if the feature
> isn't intended to be used right here.

Ah, ok.

> Taking both remarks together, limiting granularity to 16(?) bytes
> would allow using the advanced EPT functionality down the road, and
> would at the same time limit the suggested bitmap to just 256 bits /
> 32 bytes, which I think gets us below what even an empty rangeset
> would require. Plus lookup would also be quite a bit more lightweight.

Indeed, in that case it makes sense.

> > The mechanism this API is plugged in is slightly different for PV and
> > HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
> > it's already called for #PF on read-only MMIO page. For HVM however, EPT
> > violation on p2m_mmio_direct page results in a direct domain_crash().
> > To reach mmio_ro_emulated_write(), change how write violations for
> > p2m_mmio_direct are handled - specifically, treat them similar to
> > p2m_ioreq_server. This makes relevant ioreq handler being called,
> > that finally end up calling mmio_ro_emulated_write().
> 
> I wonder whether that isn't too heavy a change to existing behavior.
> I understand that taking this path is necessary for the purpose of the
> patch, but I don't think it is necessary for all p2m_mmio_direct pages.
> Checking at least mmio_ro_ranges right in hvm_hap_nested_page_fault()
> looks reasonable to me.

Before this change, domU was crashed on write EPT violation to
p2m_mmio_direct page (which, IIUC, can happen only for mmio_ro_ranges,
as otherwise page is mapped R/W), so I don't think there are many
accesses that would hit this path for other reasons.

> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -165,6 +165,19 @@ bool __read_mostly machine_to_phys_mapping_valid;
> >  
> >  struct rangeset *__read_mostly mmio_ro_ranges;
> >  
> > +/* Handling sub-page read-only MMIO regions */
> > +struct subpage_ro_range {
> > +    struct list_head list;
> > +    mfn_t mfn;
> > +    int fixmap_idx;
> > +    struct rangeset *ro_bytes;
> > +    struct rcu_head rcu;
> > +};
> > +
> > +static LIST_HEAD(subpage_ro_ranges);
> > +static DEFINE_RCU_READ_LOCK(subpage_ro_rcu);
> > +static DEFINE_SPINLOCK(subpage_ro_lock);
> > +
> >  static uint32_t base_disallow_mask;
> >  /* Global bit is allowed to be set on L1 PTEs. Intended for user mappings. */
> >  #define L1_DISALLOW_MASK ((base_disallow_mask | _PAGE_GNTTAB) & ~_PAGE_GLOBAL)
> > @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      return 0;
> >  }
> >  
> > +int subpage_mmio_ro_add(
> 
> As long as patch 2 is going to add the only users, __init please, and
> there's no need for a "remove" counterpart.

__init makes sense. But as for removing "remove" part, I'm not sure. I
realize it is a dead code now, but it's easier to introduce it now to
provide complete API, than later by somebody else who would want to use
it in other places. Is there some trick to make compiler/linker optimize
it out?

> 
> > +    mfn_t mfn,
> > +    unsigned long offset_s,
> > +    unsigned long offset_e,
> > +    int fixmap_idx)
> 
> enum fixed_addresses?

If that parameter stays, yes.

> > +{
> > +    struct subpage_ro_range *entry = NULL, *iter;
> > +    int rc;
> > +
> > +    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
> > +    ASSERT(offset_s < PAGE_SIZE);
> > +    ASSERT(offset_e < PAGE_SIZE);
> > +
> > +    spin_lock(&subpage_ro_lock);
> > +
> > +    list_for_each_entry( iter, &subpage_ro_ranges, list )
> 
> Nit: Style (either you view list_for_each_entry as a [pseudo]keyword
> for the purposes of what blanks should be present/absent, or you don't,
> a mixture isn't reasonable; also elsewhere).

Which version would be your preference for list_for_each_entry?

> > +    {
> > +        if ( mfn_eq(iter->mfn, mfn) )
> > +        {
> > +            entry = iter;
> > +            break;
> > +        }
> > +    }
> > +    if ( !entry )
> > +    {
> > +        /* iter==NULL marks it was a newly allocated entry */
> > +        iter = NULL;
> > +        entry = xmalloc(struct subpage_ro_range);
> > +        rc = -ENOMEM;
> > +        if ( !entry )
> > +            goto err_unlock;
> > +        entry->mfn = mfn;
> > +        entry->fixmap_idx = fixmap_idx;
> > +        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
> > +                                       RANGESETF_prettyprint_hex);
> > +        rc = -ENOMEM;
> > +        if ( !entry->ro_bytes )
> > +            goto err_unlock;
> > +    }
> > +
> > +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
> > +    if ( rc < 0 )
> > +        goto err_unlock;
> > +
> > +    if ( !iter )
> > +        list_add_rcu(&entry->list, &subpage_ro_ranges);
> > +
> > +    spin_unlock(&subpage_ro_lock);
> > +
> > +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> > +        return 0;
> > +    else
> > +        return 1;
> 
> If this case is really needed, this special return value (as documented
> in the header) probably needs specially handling in the callers - it's
> not necessarily an error after all. But I wonder whether it wouldn't be
> easier to check earlier on and fail right away (with e.g. -EBUSY), 

The idea is to allow multiple sub-ranges in a single page. Again, if
using ioremap() internally, instead of fixmap provided externally, this
case will go away.

> rather
> than adding the range and _then_ (kind of, as per patch 2) failing.

Right, I missed "!= 0" there.

> > +err_unlock:
> 
> Nit: Style (labels indented by at least one space please, also elsewhere).
> 
> > +static void subpage_mmio_write_emulate(
> > +    mfn_t mfn,
> > +    unsigned long offset,
> 
> unsigned int
> 
> > +    void *data,
> 
> const
> 
> > +    unsigned int len)
> > +{
> > +    struct subpage_ro_range *entry;
> > +    void __iomem *addr;
> > +
> > +    rcu_read_lock(&subpage_ro_rcu);
> > +
> > +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
> > +    {
> > +        if ( mfn_eq(entry->mfn, mfn) )
> > +        {
> > +            if ( rangeset_overlaps_range(entry->ro_bytes, offset, offset + len - 1) )
> > +                goto out_unlock;
> 
> This case ...
> 
> > +            addr = fix_to_virt(entry->fixmap_idx) + offset;
> > +            switch ( len )
> > +            {
> > +            case 1:
> > +                writeb(*(uint8_t*)data, addr);
> > +                break;
> > +            case 2:
> > +                writew(*(uint16_t*)data, addr);
> > +                break;
> > +            case 4:
> > +                writel(*(uint32_t*)data, addr);
> > +                break;
> > +            case 8:
> > +                writeq(*(uint64_t*)data, addr);
> > +                break;
> > +            default:
> > +                /* mmio_ro_emulated_write() already validated the size */
> > +                ASSERT_UNREACHABLE();
> 
> ... as well as, in a release build, this one wants to ...
> 
> > +            }
> > +            goto out_unlock;
> 
> ... not use this path but ...
> 
> > +        }
> > +    }
> > +    gdprintk(XENLOG_WARNING,
> > +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
> > +             mfn_x(mfn), offset, len);
> 
> ... make it here. (By implication: I'm not convinced this wants to be a
> gdprintk(), as I think at least one such warning would better surface in
> release builds as well.)

Right, gprintk() would make more sense indeed.

> At the same time I don't think any message should be issued for write
> attempts to pages which don't have part of it marked writable. We deal
> with such silently right now, and this shouldn't change.

At least for HVM domains, it isn't really silent. It's domain_crash()
(before my change, not reaching this function at all).
I think it is silent only for PV domains (or maybe even just hardware
domain?).

> In fact even
> for ranges which don't overlap the writable portion of a page we may want
> to consider remaining silent. The log message ought to be of interest
> mainly for writes which we meant to permit, but which for whatever reason
> we elect to suppress nevertheless.
> 
> As to the message - why do you split MFN from offset, rather than simply
> logging an address ("... %" PRI_mfn "%03x ...")?

I could use such formatting trick indeed.

> Like (iirc) Roger I think that misaligned accesses should be refused (and
> hence also make it here).

Ok, will change.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Jan Beulich]

On 29.03.2023 12:51, Marek Marczykowski-Górecki wrote:
> On Wed, Mar 29, 2023 at 10:50:20AM +0200, Jan Beulich wrote:
>> On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
>>> In some cases, only few registers on a page needs to be write-protected.
>>> Examples include USB3 console (64 bytes worth of registers) or MSI-X's
>>> PBA table (which doesn't need to span the whole table either).
>>
>> Yet like the MSI-X table the PBA is not permitted to share a page with
>> other registers (the table itself excluded). So a need there would
>> (again) only arise for devices violating the spec.
> 
> For PBA, indeed (and due to not seeing device needing such hack, I don't
> do that for PBA yet). But the XHCI spec doesn't include such limitation, so
> this case is perfectly valid.

My remark was merely because you mention PBA in the description.
Mentioning it is okay, but you would want to further qualify it, I
think.

>>> Current API allows only marking whole pages pages read-only, which
>>> sometimes may cover other registers that guest may need to write into.
>>>
>>> Currently, when a guest tries to write to an MMIO page on the
>>> mmio_ro_ranges, it's either immediately crashed on EPT violation - if
>>> that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
>>> from userspace (like, /dev/mem), it will try to fixup by updating page
>>> tables (that Xen again will force to read-only) and will hit that #PF
>>> again (looping endlessly). Both behaviors are undesirable if guest could
>>> actually be allowed the write.
>>>
>>> Introduce an API that allows marking part of a page read-only. Since
>>> sub-page permissions are not a thing in page tables, do this via
>>> emulation (or simply page fault handler for PV) that handles writes that
>>> are supposed to be allowed. Those writes require the page to be mapped
>>> to Xen, so subpage_mmio_ro_add() function takes fixmap index of the
>>> page. The page needs to be added to mmio_ro_ranges, first anyway.
>>> Sub-page ranges are stored using rangeset for each added page, and those
>>> pages are stored on a plain list (as there isn't supposed to be many
>>> pages needing this precise r/o control).
>>
>> While, unlike Roger, I don't want to see mmio_ro_ranges' granularity
>> changed, I wonder if a bitmap isn't going to be easier to use (even
>> if perhaps requiring a little more memory, but whether that's the case
>> depends on intended granularity - I'm not convinced we need byte-
>> granular ranges). I'm also worried of this yet more heavily tying to
>> x86 of (as of the next patch) the USB3 debug console driver (i.e. as
>> opposed to Roger I wouldn't take anything that's x86-only as
>> justification for making wider changes).
> 
> Well, it's still under the very same CONFIG_X86, and for the same
> purpose, so I don't think it's "more heavily tying".
> 
>> As to sub-page permissions: EPT has, as one of its extensions, support
>> for this. It might be worth at least mentioning, even if the feature
>> isn't intended to be used right here.
> 
> Ah, ok.
> 
>> Taking both remarks together, limiting granularity to 16(?) bytes
>> would allow using the advanced EPT functionality down the road, and
>> would at the same time limit the suggested bitmap to just 256 bits /
>> 32 bytes, which I think gets us below what even an empty rangeset
>> would require. Plus lookup would also be quite a bit more lightweight.
> 
> Indeed, in that case it makes sense.

Hmm, I've checked the SDM, and I was misremembering: Granularity is
128 bytes, which might be too large for the purposes here.

>>> @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      return 0;
>>>  }
>>>  
>>> +int subpage_mmio_ro_add(
>>
>> As long as patch 2 is going to add the only users, __init please, and
>> there's no need for a "remove" counterpart.
> 
> __init makes sense. But as for removing "remove" part, I'm not sure. I
> realize it is a dead code now, but it's easier to introduce it now to
> provide complete API, than later by somebody else who would want to use
> it in other places. Is there some trick to make compiler/linker optimize
> it out?

At the very least you could also mark it __init. There are also the .discard
and .discard.* sections we handle specially in the linker script. But no
matter what you do to retain the code without impacting the resulting binary,
iirc Misra tells us that there shouldn't be dead code.

>>> +    mfn_t mfn,
>>> +    unsigned long offset_s,
>>> +    unsigned long offset_e,
>>> +    int fixmap_idx)
>>
>> enum fixed_addresses?
> 
> If that parameter stays, yes.
> 
>>> +{
>>> +    struct subpage_ro_range *entry = NULL, *iter;
>>> +    int rc;
>>> +
>>> +    ASSERT(rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)));
>>> +    ASSERT(offset_s < PAGE_SIZE);
>>> +    ASSERT(offset_e < PAGE_SIZE);
>>> +
>>> +    spin_lock(&subpage_ro_lock);
>>> +
>>> +    list_for_each_entry( iter, &subpage_ro_ranges, list )
>>
>> Nit: Style (either you view list_for_each_entry as a [pseudo]keyword
>> for the purposes of what blanks should be present/absent, or you don't,
>> a mixture isn't reasonable; also elsewhere).
> 
> Which version would be your preference for list_for_each_entry?

I have no preference, and I'm sure we have ample examples of all forms
(including the mixed ones that we shouldn't have). There's a
for_each_online_cpu() in this file, using the non-keyword style, so
if anything this might be taken as reference. But I didn't check whether
there are any other uses of keyword-like identifiers which may use the
other style.

>>> +    {
>>> +        if ( mfn_eq(iter->mfn, mfn) )
>>> +        {
>>> +            entry = iter;
>>> +            break;
>>> +        }
>>> +    }
>>> +    if ( !entry )
>>> +    {
>>> +        /* iter==NULL marks it was a newly allocated entry */
>>> +        iter = NULL;
>>> +        entry = xmalloc(struct subpage_ro_range);
>>> +        rc = -ENOMEM;
>>> +        if ( !entry )
>>> +            goto err_unlock;
>>> +        entry->mfn = mfn;
>>> +        entry->fixmap_idx = fixmap_idx;
>>> +        entry->ro_bytes = rangeset_new(NULL, "subpage r/o mmio",
>>> +                                       RANGESETF_prettyprint_hex);
>>> +        rc = -ENOMEM;
>>> +        if ( !entry->ro_bytes )
>>> +            goto err_unlock;
>>> +    }
>>> +
>>> +    rc = rangeset_add_range(entry->ro_bytes, offset_s, offset_e);
>>> +    if ( rc < 0 )
>>> +        goto err_unlock;
>>> +
>>> +    if ( !iter )
>>> +        list_add_rcu(&entry->list, &subpage_ro_ranges);
>>> +
>>> +    spin_unlock(&subpage_ro_lock);
>>> +
>>> +    if ( !iter || entry->fixmap_idx == fixmap_idx )
>>> +        return 0;
>>> +    else
>>> +        return 1;
>>
>> If this case is really needed, this special return value (as documented
>> in the header) probably needs specially handling in the callers - it's
>> not necessarily an error after all. But I wonder whether it wouldn't be
>> easier to check earlier on and fail right away (with e.g. -EBUSY), 
> 
> The idea is to allow multiple sub-ranges in a single page. Again, if
> using ioremap() internally, instead of fixmap provided externally, this
> case will go away.
> 
>> rather
>> than adding the range and _then_ (kind of, as per patch 2) failing.
> 
> Right, I missed "!= 0" there.

Hmm, adding "!= 0" won't make a difference, will it? Adding "< 0" would.

>>> +err_unlock:
>>
>> Nit: Style (labels indented by at least one space please, also elsewhere).
>>
>>> +static void subpage_mmio_write_emulate(
>>> +    mfn_t mfn,
>>> +    unsigned long offset,
>>
>> unsigned int
>>
>>> +    void *data,
>>
>> const
>>
>>> +    unsigned int len)
>>> +{
>>> +    struct subpage_ro_range *entry;
>>> +    void __iomem *addr;
>>> +
>>> +    rcu_read_lock(&subpage_ro_rcu);
>>> +
>>> +    list_for_each_entry_rcu( entry, &subpage_ro_ranges, list )
>>> +    {
>>> +        if ( mfn_eq(entry->mfn, mfn) )
>>> +        {
>>> +            if ( rangeset_overlaps_range(entry->ro_bytes, offset, offset + len - 1) )
>>> +                goto out_unlock;
>>
>> This case ...
>>
>>> +            addr = fix_to_virt(entry->fixmap_idx) + offset;
>>> +            switch ( len )
>>> +            {
>>> +            case 1:
>>> +                writeb(*(uint8_t*)data, addr);
>>> +                break;
>>> +            case 2:
>>> +                writew(*(uint16_t*)data, addr);
>>> +                break;
>>> +            case 4:
>>> +                writel(*(uint32_t*)data, addr);
>>> +                break;
>>> +            case 8:
>>> +                writeq(*(uint64_t*)data, addr);
>>> +                break;
>>> +            default:
>>> +                /* mmio_ro_emulated_write() already validated the size */
>>> +                ASSERT_UNREACHABLE();
>>
>> ... as well as, in a release build, this one wants to ...
>>
>>> +            }
>>> +            goto out_unlock;
>>
>> ... not use this path but ...
>>
>>> +        }
>>> +    }
>>> +    gdprintk(XENLOG_WARNING,
>>> +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
>>> +             mfn_x(mfn), offset, len);
>>
>> ... make it here. (By implication: I'm not convinced this wants to be a
>> gdprintk(), as I think at least one such warning would better surface in
>> release builds as well.)
> 
> Right, gprintk() would make more sense indeed.
> 
>> At the same time I don't think any message should be issued for write
>> attempts to pages which don't have part of it marked writable. We deal
>> with such silently right now, and this shouldn't change.
> 
> At least for HVM domains, it isn't really silent. It's domain_crash()
> (before my change, not reaching this function at all).

Well, yes, but that's one instance in the lifetime of a domain.

Jan

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 5271 bytes --]

On Wed, Mar 29, 2023 at 02:39:22PM +0200, Jan Beulich wrote:
> On 29.03.2023 12:51, Marek Marczykowski-Górecki wrote:
> > On Wed, Mar 29, 2023 at 10:50:20AM +0200, Jan Beulich wrote:
> >> On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> >>> In some cases, only few registers on a page needs to be write-protected.
> >>> Examples include USB3 console (64 bytes worth of registers) or MSI-X's
> >>> PBA table (which doesn't need to span the whole table either).
> >>
> >> Yet like the MSI-X table the PBA is not permitted to share a page with
> >> other registers (the table itself excluded). So a need there would
> >> (again) only arise for devices violating the spec.
> > 
> > For PBA, indeed (and due to not seeing device needing such hack, I don't
> > do that for PBA yet). But the XHCI spec doesn't include such limitation, so
> > this case is perfectly valid.
> 
> My remark was merely because you mention PBA in the description.
> Mentioning it is okay, but you would want to further qualify it, I
> think.

Ah, ok.

> >> Taking both remarks together, limiting granularity to 16(?) bytes
> >> would allow using the advanced EPT functionality down the road, and
> >> would at the same time limit the suggested bitmap to just 256 bits /
> >> 32 bytes, which I think gets us below what even an empty rangeset
> >> would require. Plus lookup would also be quite a bit more lightweight.
> > 
> > Indeed, in that case it makes sense.
> 
> Hmm, I've checked the SDM, and I was misremembering: Granularity is
> 128 bytes, which might be too large for the purposes here.

Indeed, it seems so. In case of USB3 console, I want to protect 64 bytes
of registers...

I guess 16 bytes granularity would work, but it feels kinda arbitrary
without any good reason to choose this specific number. More logical
would be 4 bytes (as common register size), but it means 128 bytes for
the bitmask.

> >>> @@ -4893,6 +4906,172 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>      return 0;
> >>>  }
> >>>  
> >>> +int subpage_mmio_ro_add(
> >>
> >> As long as patch 2 is going to add the only users, __init please, and
> >> there's no need for a "remove" counterpart.
> > 
> > __init makes sense. But as for removing "remove" part, I'm not sure. I
> > realize it is a dead code now, but it's easier to introduce it now to
> > provide complete API, than later by somebody else who would want to use
> > it in other places. Is there some trick to make compiler/linker optimize
> > it out?
> 
> At the very least you could also mark it __init. There are also the .discard
> and .discard.* sections we handle specially in the linker script. But no
> matter what you do to retain the code without impacting the resulting binary,
> iirc Misra tells us that there shouldn't be dead code.

Well, if dropping remove, then I guess I could leave a comment
describing what it would need to do. Or maybe just hint in the
description that earlier version of the patch had remove implemented -
so if anybody needs it in the future, can do some mailing list
archaeology and have something to start with.

> >>> +    if ( !iter || entry->fixmap_idx == fixmap_idx )
> >>> +        return 0;
> >>> +    else
> >>> +        return 1;
> >>
> >> If this case is really needed, this special return value (as documented
> >> in the header) probably needs specially handling in the callers - it's
> >> not necessarily an error after all. But I wonder whether it wouldn't be
> >> easier to check earlier on and fail right away (with e.g. -EBUSY), 
> > 
> > The idea is to allow multiple sub-ranges in a single page. Again, if
> > using ioremap() internally, instead of fixmap provided externally, this
> > case will go away.
> > 
> >> rather
> >> than adding the range and _then_ (kind of, as per patch 2) failing.
> > 
> > Right, I missed "!= 0" there.
> 
> Hmm, adding "!= 0" won't make a difference, will it? Adding "< 0" would.

Right.

> >>> +        }
> >>> +    }
> >>> +    gdprintk(XENLOG_WARNING,
> >>> +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
> >>> +             mfn_x(mfn), offset, len);
> >>
> >> ... make it here. (By implication: I'm not convinced this wants to be a
> >> gdprintk(), as I think at least one such warning would better surface in
> >> release builds as well.)
> > 
> > Right, gprintk() would make more sense indeed.
> > 
> >> At the same time I don't think any message should be issued for write
> >> attempts to pages which don't have part of it marked writable. We deal
> >> with such silently right now, and this shouldn't change.
> > 
> > At least for HVM domains, it isn't really silent. It's domain_crash()
> > (before my change, not reaching this function at all).
> 
> Well, yes, but that's one instance in the lifetime of a domain.

What I mean is that it isn't a common or normal case that HVM domain
would exercise in normal operation. It's abnormal situation and as such
it should IMO get a log message.
And even for PV domain, such message would save me quite some time
debugging related issues...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/mm: add API for marking only part of a MMIO page read only

[Jan Beulich]

On 29.03.2023 15:27, Marek Marczykowski-Górecki wrote:
> On Wed, Mar 29, 2023 at 02:39:22PM +0200, Jan Beulich wrote:
>> On 29.03.2023 12:51, Marek Marczykowski-Górecki wrote:
>>> On Wed, Mar 29, 2023 at 10:50:20AM +0200, Jan Beulich wrote:
>>>> Taking both remarks together, limiting granularity to 16(?) bytes
>>>> would allow using the advanced EPT functionality down the road, and
>>>> would at the same time limit the suggested bitmap to just 256 bits /
>>>> 32 bytes, which I think gets us below what even an empty rangeset
>>>> would require. Plus lookup would also be quite a bit more lightweight.
>>>
>>> Indeed, in that case it makes sense.
>>
>> Hmm, I've checked the SDM, and I was misremembering: Granularity is
>> 128 bytes, which might be too large for the purposes here.
> 
> Indeed, it seems so. In case of USB3 console, I want to protect 64 bytes
> of registers...
> 
> I guess 16 bytes granularity would work, but it feels kinda arbitrary
> without any good reason to choose this specific number. More logical
> would be 4 bytes (as common register size), but it means 128 bytes for
> the bitmask.

Which is still acceptable, so I'd say 4 bytes it is then.

>>>>> +        }
>>>>> +    }
>>>>> +    gdprintk(XENLOG_WARNING,
>>>>> +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len %u\n",
>>>>> +             mfn_x(mfn), offset, len);
>>>>
>>>> ... make it here. (By implication: I'm not convinced this wants to be a
>>>> gdprintk(), as I think at least one such warning would better surface in
>>>> release builds as well.)
>>>
>>> Right, gprintk() would make more sense indeed.
>>>
>>>> At the same time I don't think any message should be issued for write
>>>> attempts to pages which don't have part of it marked writable. We deal
>>>> with such silently right now, and this shouldn't change.
>>>
>>> At least for HVM domains, it isn't really silent. It's domain_crash()
>>> (before my change, not reaching this function at all).
>>
>> Well, yes, but that's one instance in the lifetime of a domain.
> 
> What I mean is that it isn't a common or normal case that HVM domain
> would exercise in normal operation. It's abnormal situation and as such
> it should IMO get a log message.
> And even for PV domain, such message would save me quite some time
> debugging related issues...

Sure. I don't mind a single message. I also don't mind a few. But I'd
prefer if we could avoid relying on (general) rate limiting to bound
the volume, at least when it's the simple case of dropping writes
which don't overlap the intended to be writable region.

That said - if for HVM previously we crashed the domain in such cases
and this now changes, then this change in behavior would also need
mentioning / justifying.

Jan

[PATCH 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Marek Marczykowski-Górecki]

... not the whole page, which may contain other registers too. In fact
on Tiger Lake and newer (at least), this page do contain other registers
that Linux tries to use. And with share=yes, a domU would use them too.
Without this patch, PV dom0 would fail to initialize the controller,
while HVM would be killed on EPT violation.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/xhci-dbc.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 60b781f87202..df2524b0ca18 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1226,9 +1226,43 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
                          uart->dbc.xhc_dbc_offset),
                 PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
                        uart->dbc.xhc_dbc_offset +
-                sizeof(*uart->dbc.dbc_reg)) - 1) )
-        printk(XENLOG_INFO
+                sizeof(*uart->dbc.dbc_reg)) - 1) ) {
+        printk(XENLOG_WARNING
                "Error while adding MMIO range of device to mmio_ro_ranges\n");
+    }
+    else
+    {
+        unsigned long dbc_regs_start = (uart->dbc.bar_val &
+                PCI_BASE_ADDRESS_MEM_MASK) + uart->dbc.xhc_dbc_offset;
+        unsigned long dbc_regs_end = dbc_regs_start + sizeof(*uart->dbc.dbc_reg);
+
+        /* This being smaller than a page simplifies conditions below */
+        BUILD_BUG_ON(sizeof(*uart->dbc.dbc_reg) >= PAGE_SIZE - 1);
+        if ( dbc_regs_start & (PAGE_SIZE - 1) ||
+                PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) )
+        {
+            if ( subpage_mmio_ro_add(
+                        _mfn(PFN_DOWN(dbc_regs_start)),
+                        dbc_regs_start & (PAGE_SIZE - 1),
+                        PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end)
+                        ? dbc_regs_end & (PAGE_SIZE - 1)
+                        : PAGE_SIZE - 1,
+                        FIX_XHCI_END) )
+                printk(XENLOG_WARNING
+                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
+        }
+        if ( dbc_regs_end & (PAGE_SIZE - 1) &&
+                PFN_DOWN(dbc_regs_start) != PFN_DOWN(dbc_regs_end) )
+        {
+            if ( subpage_mmio_ro_add(
+                        _mfn(PFN_DOWN(dbc_regs_end)),
+                        0,
+                        dbc_regs_end & (PAGE_SIZE - 1),
+                        FIX_XHCI_END + PFN_DOWN(sizeof(*uart->dbc.dbc_reg))) )
+                printk(XENLOG_WARNING
+                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
+        }
+    }
 #endif
 }
 
-- 
git-series 0.9.1

Re: [PATCH 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Jan Beulich]

On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> ... not the whole page, which may contain other registers too. In fact
> on Tiger Lake and newer (at least), this page do contain other registers
> that Linux tries to use. And with share=yes, a domU would use them too.
> Without this patch, PV dom0 would fail to initialize the controller,
> while HVM would be killed on EPT violation.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  xen/drivers/char/xhci-dbc.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> index 60b781f87202..df2524b0ca18 100644
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1226,9 +1226,43 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
>                           uart->dbc.xhc_dbc_offset),
>                  PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
>                         uart->dbc.xhc_dbc_offset +
> -                sizeof(*uart->dbc.dbc_reg)) - 1) )
> -        printk(XENLOG_INFO
> +                sizeof(*uart->dbc.dbc_reg)) - 1) ) {

Nit: No need for a brace here (and certainly not a misplaced one).

> +        printk(XENLOG_WARNING

This log level change looks kind of unrelated.

>                 "Error while adding MMIO range of device to mmio_ro_ranges\n");
> +    }
> +    else
> +    {
> +        unsigned long dbc_regs_start = (uart->dbc.bar_val &
> +                PCI_BASE_ADDRESS_MEM_MASK) + uart->dbc.xhc_dbc_offset;
> +        unsigned long dbc_regs_end = dbc_regs_start + sizeof(*uart->dbc.dbc_reg);
> +
> +        /* This being smaller than a page simplifies conditions below */
> +        BUILD_BUG_ON(sizeof(*uart->dbc.dbc_reg) >= PAGE_SIZE - 1);

Why PAGE_SIZE - 1 (or why >= instead of > )? If there is a reason, then
the comment wants to be in sync.

> +        if ( dbc_regs_start & (PAGE_SIZE - 1) ||

Nit: Please parenthesize the & against the || (similarly again below).

Like asked by Roger for patch 1 (iirc), here and below please use
PAGE_OFFSET() in favor of (kind of) open-coding it.

> +                PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) )

Nit: Style (indentation).

> +        {
> +            if ( subpage_mmio_ro_add(
> +                        _mfn(PFN_DOWN(dbc_regs_start)),
> +                        dbc_regs_start & (PAGE_SIZE - 1),
> +                        PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end)
> +                        ? dbc_regs_end & (PAGE_SIZE - 1)
> +                        : PAGE_SIZE - 1,
> +                        FIX_XHCI_END) )

Nit: I think this is too deep a level of indentation; it should be a
single level (4 blanks) from the start of the function name (also
again another time below).

> +                printk(XENLOG_WARNING
> +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");

Nit: Style (indentation).

> +        }
> +        if ( dbc_regs_end & (PAGE_SIZE - 1) &&
> +                PFN_DOWN(dbc_regs_start) != PFN_DOWN(dbc_regs_end) )
> +        {
> +            if ( subpage_mmio_ro_add(
> +                        _mfn(PFN_DOWN(dbc_regs_end)),
> +                        0,
> +                        dbc_regs_end & (PAGE_SIZE - 1),
> +                        FIX_XHCI_END + PFN_DOWN(sizeof(*uart->dbc.dbc_reg))) )
> +                printk(XENLOG_WARNING
> +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
> +        }
> +    }

Seeing the uses it occurs to me that the interface is somewhat odd: It
adds a r/o range to a page that is already recorded to be r/o. It would
imo be more logical the other way around: To add an exception (writable)
range. The only alternative would be to include the call to
rangeset_add_range(mmio_ro_ranges, ...) as part of the new function, and
reduce accordingly the range passed earlier in the function. But I think
this would needlessly complicate the code there.

Jan

Re: [PATCH 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 5135 bytes --]

On Wed, Mar 29, 2023 at 11:14:52AM +0200, Jan Beulich wrote:
> On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> > ... not the whole page, which may contain other registers too. In fact
> > on Tiger Lake and newer (at least), this page do contain other registers
> > that Linux tries to use. And with share=yes, a domU would use them too.
> > Without this patch, PV dom0 would fail to initialize the controller,
> > while HVM would be killed on EPT violation.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  xen/drivers/char/xhci-dbc.c | 38 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> > index 60b781f87202..df2524b0ca18 100644
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -1226,9 +1226,43 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
> >                           uart->dbc.xhc_dbc_offset),
> >                  PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> >                         uart->dbc.xhc_dbc_offset +
> > -                sizeof(*uart->dbc.dbc_reg)) - 1) )
> > -        printk(XENLOG_INFO
> > +                sizeof(*uart->dbc.dbc_reg)) - 1) ) {
> 
> Nit: No need for a brace here (and certainly not a misplaced one).
> 
> > +        printk(XENLOG_WARNING
> 
> This log level change looks kind of unrelated.
> 
> >                 "Error while adding MMIO range of device to mmio_ro_ranges\n");
> > +    }
> > +    else
> > +    {
> > +        unsigned long dbc_regs_start = (uart->dbc.bar_val &
> > +                PCI_BASE_ADDRESS_MEM_MASK) + uart->dbc.xhc_dbc_offset;
> > +        unsigned long dbc_regs_end = dbc_regs_start + sizeof(*uart->dbc.dbc_reg);
> > +
> > +        /* This being smaller than a page simplifies conditions below */
> > +        BUILD_BUG_ON(sizeof(*uart->dbc.dbc_reg) >= PAGE_SIZE - 1);
> 
> Why PAGE_SIZE - 1 (or why >= instead of > )? If there is a reason, then
> the comment wants to be in sync.

Indeed looks like off-by-one.

> > +        if ( dbc_regs_start & (PAGE_SIZE - 1) ||
> 
> Nit: Please parenthesize the & against the || (similarly again below).
> 
> Like asked by Roger for patch 1 (iirc), here and below please use
> PAGE_OFFSET() in favor of (kind of) open-coding it.
> 
> > +                PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) )
> 
> Nit: Style (indentation).
> 
> > +        {
> > +            if ( subpage_mmio_ro_add(
> > +                        _mfn(PFN_DOWN(dbc_regs_start)),
> > +                        dbc_regs_start & (PAGE_SIZE - 1),
> > +                        PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end)
> > +                        ? dbc_regs_end & (PAGE_SIZE - 1)
> > +                        : PAGE_SIZE - 1,
> > +                        FIX_XHCI_END) )
> 
> Nit: I think this is too deep a level of indentation; it should be a
> single level (4 blanks) from the start of the function name (also
> again another time below).
> 
> > +                printk(XENLOG_WARNING
> > +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
> 
> Nit: Style (indentation).
> 
> > +        }
> > +        if ( dbc_regs_end & (PAGE_SIZE - 1) &&
> > +                PFN_DOWN(dbc_regs_start) != PFN_DOWN(dbc_regs_end) )
> > +        {
> > +            if ( subpage_mmio_ro_add(
> > +                        _mfn(PFN_DOWN(dbc_regs_end)),
> > +                        0,
> > +                        dbc_regs_end & (PAGE_SIZE - 1),
> > +                        FIX_XHCI_END + PFN_DOWN(sizeof(*uart->dbc.dbc_reg))) )
> > +                printk(XENLOG_WARNING
> > +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
> > +        }
> > +    }
> 
> Seeing the uses it occurs to me that the interface is somewhat odd: It
> adds a r/o range to a page that is already recorded to be r/o. It would
> imo be more logical the other way around: To add an exception (writable)
> range. The only alternative would be to include the call to
> rangeset_add_range(mmio_ro_ranges, ...) as part of the new function, and
> reduce accordingly the range passed earlier in the function. But I think
> this would needlessly complicate the code there.

I'm trying to make the interface safe against multiple calls making
different parts of the page R/O. If it would mark an exception, then
handling a page where multiple regions need to be protected would be
rather cumbersome. It isn't the case for XHCI driver, but for example it
could be in MSI-X (if I'd actually use this API there).

Making subpage_mmio_ro_add() to call mmio_ro_ranges() on its own,
together with Roger's suggestion of using ioremap() internally instead
of using fixmap would make it a bit nicer (if mapping the same page with
ioremap() in addition to fixmap isn't a problem).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Jan Beulich]

On 29.03.2023 12:21, Marek Marczykowski-Górecki wrote:
> Making subpage_mmio_ro_add() to call mmio_ro_ranges() on its own,
> together with Roger's suggestion of using ioremap() internally instead
> of using fixmap would make it a bit nicer (if mapping the same page with
> ioremap() in addition to fixmap isn't a problem).

I don't see any problem there, as long as we don't gain many of these
(at which point wasting address space may become an issue).

Jan