Xen project Mailing List

On Tue, Jul 7, 2015 at 9:09 AM, Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx> wrote:

On 07/07/2015 03:04 PM, Lengyel, Tamas wrote:
>
>
> On Tue, Jul 7, 2015 at 4:10 AM, Razvan Cojocaru

> <rcojocaru@xxxxxxxxxxxxxxx <mailto:rcojocaru@xxxxxxxxxxxxxxx>> 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 = ""> 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

Re: [Xen-devel] [PATCH V3 1/3] xen/mem_access: Support for memory-content hiding