[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm

On 11/28/2014 5:57 PM, Jan Beulich wrote:
On 28.11.14 at 08:59, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2838,7 +2838,8 @@ int hvm_hap_nested_page_fault(paddr_t gpa, unsigned long 
       * to the mmio handler.
      if ( (p2mt == p2m_mmio_dm) ||
-         (npfec.write_access && (p2mt == p2m_ram_ro)) )
+         (npfec.write_access &&
+               ((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm))) )

Why are you corrupting indentation here?
Thanks for your comments, Jan.
The indentation here is to make sure the
((p2mt == p2m_ram_ro) || (p2mt == p2m_mmio_write_dm)) are grouped together. But I am not sure if this is correct according to xen coding style. I may have misunderstood your previous comments on Sep 3, which said "the indentation would need adjustment" in reply to "[Xen-devel] [PATCH v3 1/2] x86: add p2m_mmio_write_dm"

Furthermore the code you modify here suggests that p2m_ram_ro
already has the needed semantics - writes get passed to the DM.
None of the other changes you make, and none of the other uses
of p2m_ram_ro appear to be in conflict with your intentions, so
you'd really need to explain better why you need the new type.

Thanks Jan.
To my understanding, pages with p2m_ram_ro are not supposed to be modified by guest. So in __hvm_copy(), when p2m type of a page is p2m_ram_rom, no copy will occur. However, to our usage, we just wanna this page to be write protected, so that our device model can be triggered to do some emulation. The content written to this page is supposed not to be dropped. This way, if sequentially a read operation is performed by guest to this page, the guest will still see its anticipated value.

Maybe I need to update my commit message to explain this more clearly. :)

@@ -5922,6 +5923,8 @@ long do_hvm_op(unsigned long op, 
                  a.mem_type =  HVMMEM_ram_rw;
              else if ( p2m_is_grant(t) )
                  a.mem_type =  HVMMEM_ram_rw;
+            else if ( t == p2m_mmio_write_dm )
+                a.mem_type = HVMMEM_mmio_write_dm;
                  a.mem_type =  HVMMEM_mmio_dm;
              rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
@@ -5941,7 +5944,8 @@ long do_hvm_op(unsigned long op, 
          static const p2m_type_t memtype[] = {
              [HVMMEM_ram_rw]  = p2m_ram_rw,
              [HVMMEM_ram_ro]  = p2m_ram_ro,
-            [HVMMEM_mmio_dm] = p2m_mmio_dm
+            [HVMMEM_mmio_dm] = p2m_mmio_dm,
+            [HVMMEM_mmio_write_dm] = p2m_mmio_write_dm

          if ( copy_from_guest(&a, arg, 1) )
@@ -5987,14 +5991,17 @@ long do_hvm_op(unsigned long op, 
                  rc = -EAGAIN;
                  goto param_fail4;

Stray addition of a blank line?

              if ( !p2m_is_ram(t) &&
-                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) )
+                 (!p2m_is_hole(t) || a.hvmmem_type != HVMMEM_mmio_dm) &&
+                 t != p2m_mmio_write_dm )

Do you really want to permit e.g. transitions between mmio_dm and
mmio_write_dm? We should be as restrictive as possible here to not
open up paths to security problems.

                  put_gfn(d, pfn);
                  goto param_fail4;

              rc = p2m_change_type_one(d, pfn, t, memtype[a.hvmmem_type]);
              put_gfn(d, pfn);
              if ( rc )
                  goto param_fail4;

Another stray newline addition.

--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -72,6 +72,7 @@ typedef enum {
      p2m_ram_shared = 12,          /* Shared or sharable memory */
      p2m_ram_broken = 13,          /* Broken page, access cause domain crash */
      p2m_map_foreign  = 14,        /* ram pages from foreign domain */
+    p2m_mmio_write_dm = 15,       /* Read-only; writes go to the device model 
  } p2m_type_t;

  /* Modifiers to the query */

If the new type is really needed, shouldn't this get added to

Well, previsouly, I wished to differenciate the HVMMEM_ram_ro and the newly added HVMMEM_mmio_write_dm in the HVMOP_get_mem_type/HVMOP_set_mem_type hypercall. I'd rather think of this new type as write protected than read only.
But I'll take a look, to see if this can be added to P2M_RO_TYPES. Thanks.



Xen-devel mailing list

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.