Re: [PATCH 5/5] xen: Write CR0, CR3 and CR4 in arch_set_info_guest()

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/17/2015 09:32 PM, Tamas K Lengyel wrote:
>>
>> I took the suggestion to mean at the time that we should have something
>> like EVENT_CR3_PRE and EVENT_CR3_POST, where basically all we needed was
>> for all events for which this applicable to be pre-write events. IMHO
>> that's simpler and sufficient: just send out an event when you know
>> that, unless you deny it, simply responding to it / unblocking the VCPU
>> will perform the write. So you know that the write is about to happen,
>> and by no denying it, that it will happen (or at least, that it's
>> extremely likely to happen - since some HV check can still fail somewhere).
>>
>> So, again, just IMHO, the simpler modification would just be to turn all
>> events where this is applicable into pre-write events.
> 
> Isn't the event from the guest's perspective guaranteed to be
> pre-write already? IMHO there is not much point in having two distinct

The CR events are not pre-write:

3289 int hvm_set_cr3(unsigned long value)
3290 {
3291     struct vcpu *v = current;
3292     struct page_info *page;
3293     unsigned long old;
3294
3295     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
3296          (value != v->arch.hvm_vcpu.guest_cr[3]) )
3297     {
3298         /* Shadow-mode CR3 change. Check PDBR and update refcounts. */
3299         HVM_DBG_LOG(DBG_LEVEL_VMMU, "CR3 value = %lx", value);
3300         page = get_page_from_gfn(v->domain, value >> PAGE_SHIFT,
3301                                  NULL, P2M_ALLOC);
3302         if ( !page )
3303             goto bad_cr3;
3304
3305         put_page(pagetable_get_page(v->arch.guest_table));
3306         v->arch.guest_table = pagetable_from_page(page);
3307
3308         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", value);
3309     }
3310
3311     old=v->arch.hvm_vcpu.guest_cr[3];
3312     v->arch.hvm_vcpu.guest_cr[3] = value;
3313     paging_update_cr3(v);
3314     hvm_event_cr(VM_EVENT_X86_CR3, value, old);
3315     return X86EMUL_OKAY;
3316
3317  bad_cr3:
3318     gdprintk(XENLOG_ERR, "Invalid CR3\n");
3319     domain_crash(v->domain);
3320     return X86EMUL_UNHANDLEABLE;
3321 }

The line numbers assume my patch has been applied, but the only relevant
change here is that hvm_event_cr(VM_EVENT_X86_CR3, value, old); replaced
the old hvm_event_cr3(value, old); at line 3314.

As you can see, first CR3 is being updated, and the events is being sent
afterwards. This applies to CR4, etc. also.

> event types (PRE/POST). I'm not particularly happy with the "deny
> change" flag idea either. Why not let the user specify what value he
> wants to set the register to in such a case? It could the one that was
> already there (old_value) in "deny change" style, but it might as well
> be something else. I'm thinking we could keep the existing vm_event
> hooks in place and simply let the vm_event response specify the value
> the register should be set to (in the new_value field).

You mean not have a distinct DENY vm_event response flag, but if rsp's
new_value != req's new value set that one? Sure, that'll work, but it's
less explicit, and thus, IMHO, more error-prone (it's easy for a
vm_event consumer to just create the response on the stack, forget (or
not know) that this might happen, and have the guest just write garbage
to some register).


Thanks,
Razvan