From: Jan Beulich <JBeulich@suse.com>
To: Alexandru Stefan ISAILA <aisaila@bitdefender.com>
Cc: "kevin.tian@intel.com" <kevin.tian@intel.com>,
"tamas@tklengyel.com" <tamas@tklengyel.com>,
"rcojocaru@bitdefender.com" <rcojocaru@bitdefender.com>,
Wei Liu <wl@xen.org>,
"jun.nakajima@intel.com" <jun.nakajima@intel.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Tim Deegan <tim@xen.org>,
George Dunlap <George.Dunlap@eu.citrix.com>,
Paul Durrant <Paul.Durrant@citrix.com>,
"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
"brian.woods@amd.com" <brian.woods@amd.com>,
Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v5] x86/emulate: Send vm_event from emulate
Date: Mon, 1 Jul 2019 13:13:56 +0000 [thread overview]
Message-ID: <fb2a4d3e-b477-11a5-7b5e-4a236acd60dd@suse.com> (raw)
>>> On 04.06.19 at 13:49, <aisaila@bitdefender.com> wrote:
> This patch aims to have mem access vm events sent from the emulator.
> This is useful where we want to only emulate a page walk without
> checking the EPT, but we still want to check the EPT when emulating
> the instruction that caused the page walk. In this case, the original
> EPT fault is caused by the walk trying to set the accessed or dirty
> bits, but executing the instruction itself might also cause an EPT
> fault if permitted to run, and this second fault should not be lost.
I'm afraid I still can't translate this into what exactly is wanted and
why. While typically we don't use examples to demonstrate that is
wanted in commit messages, I think this is a rather good candidate
for actually using such an approach. This may then ...
> We use hvmemul_map_linear_addr() to intercept r/w access and
> __hvm_copy() to intercept exec access.
>
> First we try to send a vm event and if the event is sent then emulation
> returns X86EMUL_RETRY in order to stop emulation on instructions that
> use access protected pages. If the event is not sent then the
> emulation goes on as expected.
... also help understanding this part, which I continue to be confused
by, too.
> @@ -530,6 +532,57 @@ static int hvmemul_do_mmio_addr(paddr_t mmio_gpa,
> return hvmemul_do_io_addr(1, mmio_gpa, reps, size, dir, df, ram_gpa);
> }
>
> +bool hvm_emulate_send_vm_event(unsigned long gla, gfn_t gfn,
> + uint32_t pfec, bool send_event)
> +{
> + xenmem_access_t access;
> + vm_event_request_t req = {};
> + paddr_t gpa = ((gfn_x(gfn) << PAGE_SHIFT) | (gla & ~PAGE_MASK));
gfn_to_gaddr()
> + if ( !send_event || !pfec )
> + return false;
I think I've said before that the !pfec part need an explanation (in
a comment). Without such an explanation I'm inclined to say it
should be deleted. If otoh this is simply mean to be a shortcut,
then you should really just check the two bits you actually care
about further down.
Similarly I think I've said before that I'm not happy for the common
case to now be to call into here just to bail back out (when VM
events are disabled on a guest). IOW I don't think you should call
into this function in the first place when "send_event" is false.
> + if ( p2m_get_mem_access(current->domain, gfn, &access,
> + altp2m_vcpu_idx(current)) != 0 )
> + return false;
> +
> + switch ( access ) {
Style.
> + case XENMEM_access_x:
> + case XENMEM_access_rx:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags = MEM_ACCESS_R | MEM_ACCESS_W;
> + break;
> +
> + case XENMEM_access_w:
> + case XENMEM_access_rw:
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags = MEM_ACCESS_X;
> + break;
> +
> + case XENMEM_access_r:
> + case XENMEM_access_n:
> + if ( pfec & PFEC_write_access )
> + req.u.mem_access.flags |= MEM_ACCESS_R | MEM_ACCESS_W;
> + if ( pfec & PFEC_insn_fetch )
> + req.u.mem_access.flags |= MEM_ACCESS_X;
> + break;
> +
> + default:
> + return false;
I think it would be more future proof to not have a default case
here: When a new access enumerator gets introduced, most
compilers would tell the developer right away that this new
enumerator value needs actively handling here.
> + }
> +
> + if ( !req.u.mem_access.flags )
> + return false; /* no violation */
How is the "false" here (I think this is the one the description talks
about) matching up with the various other ones in the function?
> @@ -615,6 +669,13 @@ static void *hvmemul_map_linear_addr(
>
> if ( pfec & PFEC_write_access )
> {
> + if ( hvm_emulate_send_vm_event(addr, gfn, pfec,
> + hvmemul_ctxt->send_event) )
> + {
> + err = ERR_PTR(~X86EMUL_RETRY);
> + goto out;
> + }
How come this sits only on the write path?
> @@ -1115,7 +1176,8 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
> * clean up any interim state.
> */
> if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_READ, false) )
> - rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> + rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo,
> + hvmemul_ctxt->send_event);
I'm not very happy to see this new parameter/argument addition.
Did you consider putting the flag of interest elsewhere (into a
structure hanging off of current, or into pagefault_info_t)?
Furthermore, if the parameter is really unavoidable, then please
separate the mechanics of introducing it from the actual change
you're after.
> @@ -2629,7 +2692,7 @@ void hvm_emulate_init_per_insn(
> hvm_copy_from_guest_linear(hvmemul_ctxt->insn_buf, addr,
> sizeof(hvmemul_ctxt->insn_buf),
> pfec | PFEC_insn_fetch,
> - NULL) == HVMTRANS_okay) ?
> + NULL, false) == HVMTRANS_okay) ?
If you pass false here, what's the point of handling insn fetches
in the new function you add?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next reply other threads:[~2019-07-01 13:19 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-01 13:13 Jan Beulich [this message]
2019-07-01 14:45 ` [Xen-devel] [PATCH v5] x86/emulate: Send vm_event from emulate Alexandru Stefan ISAILA
2019-07-01 14:55 ` Jan Beulich
2019-07-01 15:36 ` Alexandru Stefan ISAILA
2019-07-01 15:53 ` Jan Beulich
2019-07-02 7:58 ` Alexandru Stefan ISAILA
2019-07-02 8:01 ` Jan Beulich
2019-07-02 8:10 ` Alexandru Stefan ISAILA
2019-07-01 15:13 ` Razvan Cojocaru
2019-07-01 15:32 ` Jan Beulich
-- strict thread matches above, loose matches on Subject: below --
2019-06-04 11:49 Alexandru Stefan ISAILA
2019-06-11 12:45 ` Paul Durrant
2019-06-12 9:27 ` Alexandru Stefan ISAILA
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fb2a4d3e-b477-11a5-7b5e-4a236acd60dd@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=Paul.Durrant@citrix.com \
--cc=aisaila@bitdefender.com \
--cc=andrew.cooper3@citrix.com \
--cc=boris.ostrovsky@oracle.com \
--cc=brian.woods@amd.com \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=rcojocaru@bitdefender.com \
--cc=roger.pau@citrix.com \
--cc=suravee.suthikulpanit@amd.com \
--cc=tamas@tklengyel.com \
--cc=tim@xen.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).