On Tue, Jul 7, 2015 at 9:09 AM, Razvan Cojocaru <rcojoc...@bitdefender.com> wrote:
> On 07/07/2015 03:04 PM, Lengyel, Tamas wrote: > > > > > > On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru > > <rcojoc...@bitdefender.com <mailto:rcojoc...@bitdefender.com>> wrote: > > > > On 07/06/2015 09:30 PM, Lengyel, Tamas wrote: > > > If you'd prefer that I do some ground work for the future > > (i.e. rename > > > MEM_ACCESS constants, etc.), please let me know. > > > > > > > > > Yeap, that sounds reasonable to me. > > > > Any objections to this renaming? > > > > 151 #define MEM_ACCESS_EMULATE_NOWRITE (1 << 7) > > 152 /* > > 153 * Data is being sent back to the hypervisor in the event > response, > > to be > > 154 * returned by the read function when emulating an instruction. > > 155 * This flag is only useful when combined with > MEM_ACCESS_EMULATE or > > 156 * MEM_ACCESS_EMULATE_NOWRITE. > > 157 * The flag has been defined here because it is used with > mem_access > > 158 * events, and so should not accidentaly overwrite one of the > above. > > 159 */ > > 160 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 8) > > > > Are there any other small changes you'd like to see in this patch? > > > > > > That should suffice - with the flag being move to the VM_EVENT_FLAG list > > and the bitshift adjusted accordingly. > > Actually, thinking about this more, there is a small problem with the > vm_event.h code: > > 1417 void p2m_mem_access_emulate_check(struct vcpu *v, > 1418 const vm_event_response_t *rsp) > 1419 { > 1420 /* Mark vcpu for skipping one instruction upon rescheduling. */ > 1421 if ( rsp->flags & MEM_ACCESS_EMULATE ) > 1422 { > 1423 xenmem_access_t access; > 1424 bool_t violation = 1; > 1425 const struct vm_event_mem_access *data = &rsp->u.mem_access; > 1426 > 1427 if ( p2m_get_mem_access(v->domain, data->gfn, &access) == 0 ) > 1428 { > 1429 switch ( access ) > 1430 { > 1431 case XENMEM_access_n: > 1432 case XENMEM_access_n2rwx: > 1433 default: > 1434 violation = data->flags & MEM_ACCESS_RWX; > > So, in p2m_mem_access_emulate_check() MEM_ACCESS_EMULATE is checked for > in rsp->flags (_not_ data->flags), but then in vm_event.h: > > 42 /* > 43 * VCPU_PAUSED in a request signals that the vCPU triggering the > event has been > 44 * paused > 45 * VCPU_PAUSED in a response signals to unpause the vCPU > 46 */ > 47 #define VM_EVENT_FLAG_VCPU_PAUSED (1 << 0) > 48 /* Flags to aid debugging mem_event */ > 49 #define VM_EVENT_FLAG_FOREIGN (1 << 1) > > [...] > > 126 /* > 127 * mem_access flag definitions > 128 * > 129 * These flags are set only as part of a mem_event request. > 130 * > 131 * R/W/X: Defines the type of violation that has triggered the event > 132 * Multiple types can be set in a single violation! > 133 * GLA_VALID: If the gla field holds a guest VA associated with the > event > 134 * FAULT_WITH_GLA: If the violation was triggered by accessing gla > 135 * FAULT_IN_GPT: If the violation was triggered during translating gla > 136 */ > 137 #define MEM_ACCESS_R (1 << 0) > 138 #define MEM_ACCESS_W (1 << 1) > 139 #define MEM_ACCESS_X (1 << 2) > 140 #define MEM_ACCESS_RWX (MEM_ACCESS_R | MEM_ACCESS_W > | MEM_ACCESS_X) > 141 #define MEM_ACCESS_RW (MEM_ACCESS_R | MEM_ACCESS_W) > 142 #define MEM_ACCESS_RX (MEM_ACCESS_R | MEM_ACCESS_X) > 143 #define MEM_ACCESS_WX (MEM_ACCESS_W | MEM_ACCESS_X) > 144 #define MEM_ACCESS_GLA_VALID (1 << 3) > 145 #define MEM_ACCESS_FAULT_WITH_GLA (1 << 4) > 146 #define MEM_ACCESS_FAULT_IN_GPT (1 << 5) > 147 /* > 148 * The following flags can be set in the response. > 149 * > 150 * Emulate the fault-causing instruction (if set in the event > response flags). > 151 * This will allow the guest to continue execution without lifting > the page > 152 * access restrictions. > 153 */ > 154 #define MEM_ACCESS_EMULATE (1 << 6) > 155 /* > 156 * Same as MEM_ACCESS_EMULATE, but with write operations or operations > 157 * potentially having side effects (like memory mapped or port I/O) > disabled. > 158 */ > 159 #define MEM_ACCESS_EMULATE_NOWRITE (1 << 7) > > So VM_EVENT_FLAG_FOREIGN (1 << 1), and then MEM_ACCESS_EMULATE (1 << 6). > Now you're adding VM_EVENT_FLAG_TOGGLE_SINGLESTEP (1 << 2), and if we're > not very careful, slowly but surely the VM_EVENT_FLAG_ constants will > crawl towards (1 << 6) and start overlapping with MEM_ACCESS_EMULATE, > MEM_ACCESS_EMULATE_NOWRITE and so on, because they're not clearly > #defined right after the rest of the VM_EVENT_FLAG_ ones, are called > MEM_ACCESS_<something> just like the mem_access event specific flags, > and they use bit shifts relative to the MEM_ACCESS_ ones as well. This > is in part what has caused my confusion here. > > What do you think? > > Thanks, > Razvan > Well, this is clearly an oversight that we need to fix. MEM_ACCESS_* flags should be only set on the mem_access flags field. Tamas
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
