On 11/28/2014 5:57 PM, Jan Beulich wrote:
On 28.11.14 at 08:59, <yu.c.zh...@linux.intel.com> 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
gla,
* 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,
XEN_GUEST_HANDLE_PARAM(void) arg)
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;
else
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,
XEN_GUEST_HANDLE_PARAM(void) arg)
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,
XEN_GUEST_HANDLE_PARAM(void) arg)
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
P2M_RO_TYPES?
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.
Yu
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel