Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 12/2/2014 7:40 PM, Tim Deegan wrote: At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote: On 12/1/2014 8:13 PM, Tim Deegan wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. And I suppose in that latter case the new type could be made part of P2M_RO_TYPES()? Yes indeed, as P2M_RO_TYPES is defined as must have the _PAGE_RW bit clear in their PTEs. Thanks Tim. Following are my understanding of the P2M_RO_TYPES and your comments. Not sure if I get it right. Please correct me if anything wrong: 1 The P2M_RO_TYPES now bears 2 meanings: one is w bit is clear in the pte; and another being to discard the write operations; 2 We better define another class to bear the second meaning. Yes, that's what I meant. Answering your other questions in reverse order: 2 p2m_grant_map_ro is also supposed to be discarded? Will handling of this type of pages goes into __hvm_copy()/__hvm_clear(), or should? I think so, yes. At the moment we inject #GP when the guest writes to a read-only grant, which is OK: the guest really ought to know better. But I think we'll probably end up with neater code if we handle read-only grants the same way as p2m_ram_ro. Anyone else have an opinion on the right thing to do here? Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, and the new predicates, say p2m_is_discard_write: 1 You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard the write ops, yet I also noticed many other places using the p2m_is_readonly, or only the p2mt == p2m_ram_ro judgement(in __hvm_copy/__hvm_clear). Among all these other places, is there any ones also supposed to use the p2m_is_discard_write? I've just had a look through them all, and I can see exactly four places that should be using the new p2m_is_discard_write() test: - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode guests never have p2m_ram_shared or p2m_ram_logdirty mappings.) - __hvm_copy() - __hvm_clear() and - hvm_hap_nested_page_fault() (where you should also remove the explicit handling of p2m_grant_map_ro below.) Looking through that turned up a few other oddities, which I'm listing here to remind myself to look at them later (i.e. you don't need to worry about them for this patch): - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle p2m_ram_logdirty or they might spuriously fail duiring live migration. - __hvm_copy() and __hvm_clear are probably over-strict in their failure to handle grant types. Hi Tim. Sorry to bother you. :) I just noticed that in __hvm_copy()/__hvm_clear(), the grant types are handled before the p2m_ram_ro - will return HVMCOPY_unhandleable. So if p2m_is_discard_write() is supposed to replace the handling of p2m_ram_ro, handling of p2m_grant_map_ro will still return HVMCOPY_unhandleable, before the p2m_is_discard_write() predicate. Even we move the testing of p2m_is_discard_write() before the handling of grant types, it seems not quite clean. By over-strict in their failure to handle grant types., do you also mean this? Thanks Yu - P2M_UNMAP_TYPES in vmce.c is a mess. It's not the right place to define this, since it definitely won't be seen my anyone adding a new type, and it already has an 'XXX' comment that says it doesn't cover a lot of cases. :( I'll have a look at those another time. Cheers, Tim. ___ 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
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
Hi, At 17:01 +0800 on 04 Dec (1417708878), Yu, Zhang wrote: I just noticed that in __hvm_copy()/__hvm_clear(), the grant types are handled before the p2m_ram_ro - will return HVMCOPY_unhandleable. So if p2m_is_discard_write() is supposed to replace the handling of p2m_ram_ro, handling of p2m_grant_map_ro will still return HVMCOPY_unhandleable, before the p2m_is_discard_write() predicate. Even we move the testing of p2m_is_discard_write() before the handling of grant types, it seems not quite clean. By over-strict in their failure to handle grant types., do you also mean this? Yes, that's the sort of thing I meant. I'll try to write a patch for that later today or next week -- in the meantime I think you should ignore it. :) An unrelated thought: when you send your next version can you send it as a two-patch series, where the first patch does the p2m_is_discard_write() changes and the second adds your new type? Cheers, Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 02.12.14 at 08:48, yu.c.zh...@linux.intel.com wrote: On 12/1/2014 8:31 PM, Jan Beulich wrote: On 01.12.14 at 13:13, t...@xen.org wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. Sorry, I was unclear -- there isn't an inconsistency; both internal and public headers currently say that writes are discarded and AFAICT that is what the code does. Not for hvm_hap_nested_page_fault() afaict - the forwarding to DM there contradicts the writes are discarded model that other code paths follow. Thanks, Jan. By inconsistency, do you mean the p2m_ram_ro shall not trigger the handle_mmio_with_translation() in hvm_hap_nested_page_fault()? Yes, pending Tim's agreement. I'm also a bit confused with the writes are discarded/dropped comments in the code. Does this mean writes to the p2m_ram_ro pages should be abandoned without going to the dm, or going to the dm and ignored later? The code seems to be the second one. And it would seem to me that it should be the former. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
At 12:31 + on 01 Dec (1417433464), Jan Beulich wrote: On 01.12.14 at 13:13, t...@xen.org wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. Sorry, I was unclear -- there isn't an inconsistency; both internal and public headers currently say that writes are discarded and AFAICT that is what the code does. Not for hvm_hap_nested_page_fault() afaict - the forwarding to DM there contradicts the writes are discarded model that other code paths follow. It calls handle_mmio() to emulate the instruction for it, but handle_mmio() ought not to send anything to the DM. hvmemul_write() will call hvm_copy(), which will drop the write and report success. The shadow code has its own emulator framework for emulting PT writes, which does much the same thing. Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 02.12.14 at 11:37, t...@xen.org wrote: At 12:31 + on 01 Dec (1417433464), Jan Beulich wrote: On 01.12.14 at 13:13, t...@xen.org wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. Sorry, I was unclear -- there isn't an inconsistency; both internal and public headers currently say that writes are discarded and AFAICT that is what the code does. Not for hvm_hap_nested_page_fault() afaict - the forwarding to DM there contradicts the writes are discarded model that other code paths follow. It calls handle_mmio() to emulate the instruction for it, but handle_mmio() ought not to send anything to the DM. hvmemul_write() will call hvm_copy(), which will drop the write and report success. Oh, of course - it's really just the wording of the comment there that mislead me to assume the operation gets passed on for actual handling. Thanks for clarifying! Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote: On 12/1/2014 8:13 PM, Tim Deegan wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. And I suppose in that latter case the new type could be made part of P2M_RO_TYPES()? Yes indeed, as P2M_RO_TYPES is defined as must have the _PAGE_RW bit clear in their PTEs. Thanks Tim. Following are my understanding of the P2M_RO_TYPES and your comments. Not sure if I get it right. Please correct me if anything wrong: 1 The P2M_RO_TYPES now bears 2 meanings: one is w bit is clear in the pte; and another being to discard the write operations; 2 We better define another class to bear the second meaning. Yes, that's what I meant. Answering your other questions in reverse order: 2 p2m_grant_map_ro is also supposed to be discarded? Will handling of this type of pages goes into __hvm_copy()/__hvm_clear(), or should? I think so, yes. At the moment we inject #GP when the guest writes to a read-only grant, which is OK: the guest really ought to know better. But I think we'll probably end up with neater code if we handle read-only grants the same way as p2m_ram_ro. Anyone else have an opinion on the right thing to do here? Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, and the new predicates, say p2m_is_discard_write: 1 You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard the write ops, yet I also noticed many other places using the p2m_is_readonly, or only the p2mt == p2m_ram_ro judgement(in __hvm_copy/__hvm_clear). Among all these other places, is there any ones also supposed to use the p2m_is_discard_write? I've just had a look through them all, and I can see exactly four places that should be using the new p2m_is_discard_write() test: - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode guests never have p2m_ram_shared or p2m_ram_logdirty mappings.) - __hvm_copy() - __hvm_clear() and - hvm_hap_nested_page_fault() (where you should also remove the explicit handling of p2m_grant_map_ro below.) Looking through that turned up a few other oddities, which I'm listing here to remind myself to look at them later (i.e. you don't need to worry about them for this patch): - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle p2m_ram_logdirty or they might spuriously fail duiring live migration. - __hvm_copy() and __hvm_clear are probably over-strict in their failure to handle grant types. - P2M_UNMAP_TYPES in vmce.c is a mess. It's not the right place to define this, since it definitely won't be seen my anyone adding a new type, and it already has an 'XXX' comment that says it doesn't cover a lot of cases. :( I'll have a look at those another time. Cheers, Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 02.12.14 at 12:40, t...@xen.org wrote: At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote: 2 p2m_grant_map_ro is also supposed to be discarded? Will handling of this type of pages goes into __hvm_copy()/__hvm_clear(), or should? I think so, yes. At the moment we inject #GP when the guest writes to a read-only grant, which is OK: the guest really ought to know better. But I think we'll probably end up with neater code if we handle read-only grants the same way as p2m_ram_ro. Anyone else have an opinion on the right thing to do here? I actually came to the same conclusion. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 12/2/2014 7:40 PM, Tim Deegan wrote: At 15:38 +0800 on 02 Dec (1417531126), Yu, Zhang wrote: On 12/1/2014 8:13 PM, Tim Deegan wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. And I suppose in that latter case the new type could be made part of P2M_RO_TYPES()? Yes indeed, as P2M_RO_TYPES is defined as must have the _PAGE_RW bit clear in their PTEs. Thanks Tim. Following are my understanding of the P2M_RO_TYPES and your comments. Not sure if I get it right. Please correct me if anything wrong: 1 The P2M_RO_TYPES now bears 2 meanings: one is w bit is clear in the pte; and another being to discard the write operations; 2 We better define another class to bear the second meaning. Yes, that's what I meant. Answering your other questions in reverse order: 2 p2m_grant_map_ro is also supposed to be discarded? Will handling of this type of pages goes into __hvm_copy()/__hvm_clear(), or should? I think so, yes. At the moment we inject #GP when the guest writes to a read-only grant, which is OK: the guest really ought to know better. But I think we'll probably end up with neater code if we handle read-only grants the same way as p2m_ram_ro. Anyone else have an opinion on the right thing to do here? Also some questions for the new p2m class, say P2M_DISCARD_WRITE_TYPES, and the new predicates, say p2m_is_discard_write: 1 You mentioned emulate_gva_to_mfn() and __hvm_copy() should discard the write ops, yet I also noticed many other places using the p2m_is_readonly, or only the p2mt == p2m_ram_ro judgement(in __hvm_copy/__hvm_clear). Among all these other places, is there any ones also supposed to use the p2m_is_discard_write? I've just had a look through them all, and I can see exactly four places that should be using the new p2m_is_discard_write() test: - emulate_gva_to_mfn() (Though in fact it's a no-op as shadow-mode guests never have p2m_ram_shared or p2m_ram_logdirty mappings.) - __hvm_copy() - __hvm_clear() and - hvm_hap_nested_page_fault() (where you should also remove the explicit handling of p2m_grant_map_ro below.) Thank you, Tim Jan. To give a summary for all the comments: 1 new p2m type p2m_mmio_write_dm is to be added; 2 new p2m type need to be added to P2M_RO_TYPES class; 3 new p2m class, say P2M_DISCARD_WRITE_TYPES(which only include p2m_ram_ro and p2m_grant_map_ro), and the new predicates, say p2m_is_discard_write are needed to in these 4 places to discard the write op; 4 and of cause hvm_hap_nested_page_fault() do not need the special handling for p2m_grant_map_ro anymore; 5 coding style changes pointed out by Jan 6 clear the commit message I'll prepare the patch and thanks! :) Yu Looking through that turned up a few other oddities, which I'm listing here to remind myself to look at them later (i.e. you don't need to worry about them for this patch): - nsvm_get_nvmcb_page() and nestedhap_walk_L0_p2m() need to handle p2m_ram_logdirty or they might spuriously fail duiring live migration. - __hvm_copy() and __hvm_clear are probably over-strict in their failure to handle grant types. - P2M_UNMAP_TYPES in vmce.c is a mess. It's not the right place to define this, since it definitely won't be seen my anyone adding a new type, and it already has an 'XXX' comment that says it doesn't cover a lot of cases. :( I'll have a look at those another time. Cheers, Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
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.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
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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 Getting the alignment right is needed, but no via using hard tabs. 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? Maybe I need to update my commit message to explain this more clearly. :) At the very least, yes. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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 Getting the alignment right is needed, but no via using hard tabs. 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. Cheers, Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. And I suppose in that latter case the new type could be made part of P2M_RO_TYPES()? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. Sorry, I was unclear -- there isn't an inconsistency; both internal and public headers currently say that writes are discarded and AFAICT that is what the code does. But yes, we ought to follow the established hypercall interface, and so we need the new type. During this bit of archaeology I realised that either this new type should _not_ be made part of P2M_RO_TYPES, or, better, we need a new class of P2M types (P2M_DISCARD_WRITE_TYPES, say) that should be used for these paths in emulate_gva_to_mfn() and __hvm_copy(), containing just p2m_ram_ro and p2m_grant_map_ro. And I suppose in that latter case the new type could be made part of P2M_RO_TYPES()? Yes indeed, as P2M_RO_TYPES is defined as must have the _PAGE_RW bit clear in their PTEs. Cheers, Tim. ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
On 01.12.14 at 13:13, t...@xen.org wrote: At 11:17 + on 01 Dec (1417429027), Jan Beulich wrote: On 01.12.14 at 11:30, t...@xen.org wrote: At 09:32 + on 01 Dec (1417422746), Jan Beulich wrote: On 01.12.14 at 09:49, yu.c.zh...@linux.intel.com wrote: 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. __hvm_copy() is only a helper function, and doesn't write to mmio_dm space either; instead its (indirect) callers would invoke hvmemul_do_mmio() upon seeing HVMCOPY_bad_gfn_to_mfn returns. The question hence is about the apparent inconsistency resulting from writes to ram_ro being dropped here but getting passed to the DM by hvm_hap_nested_page_fault(). Tim - is that really intentional? No - and AFAICT it shouldn't be happening. It _is_ how it was implemented originally, because it involved fewer moving parts and didn't need to be efficient (and after all, writes to entirely missing addresses go to the device model too). But the code was later updated to log and discard writes to read-only memory (see 4d8aa29 from Trolle Selander). Early version of p2m_ram_ro were documented in the internal headers as sending the writes to the DM, but the public interface (HVMMEM_ram_ro) has always said that writes are discarded. Hmm, so which way do you recommend resolving the inconsistency then - match what the public interface says or what the apparent original intention for the internal type was? Presumably we need to follow the public interface mandated model, and hence require the new type to be introduced. Sorry, I was unclear -- there isn't an inconsistency; both internal and public headers currently say that writes are discarded and AFAICT that is what the code does. Not for hvm_hap_nested_page_fault() afaict - the forwarding to DM there contradicts the writes are discarded model that other code paths follow. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v4] x86: add p2m_mmio_write_dm
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?
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.
@@ -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?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
