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

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

>> 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.

Well, from the guest's perspective it is still pre-write as the guest
hasn't executed yet with the new CR value set. It's post-write only
from the hypervisor's perspective right now. So where we actually send
the event is somewhat arbitrary IMHO as the response processing would
have to call hvm_set_cr3 again if the deny/change_value flag is set.

>> 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).

You can have a response flag for it to tell Xen to look at the
new_value. What I meant is why restrict the feature to be DENY only.
You might as well let the user choose the value he wants to see in the
register. The flag would still be necessary to be defined for the
response as to avoid accidentally changing new_value without realizing
the consequence of it.

Cheers,
Tamas

>
>
> Thanks,
> Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel