|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V2] vm_event: Allow subscribing to write events for specific MSR-s
On 13/04/16 10:47, Konrad Rzeszutek Wilk wrote:
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 1fec412..4c96968 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -22,6 +22,58 @@
>> #include <asm/monitor.h>
>> #include <public/vm_event.h>
>>
>> +static int arch_monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> + if ( !d->arch.monitor_msr_bitmap )
>> + return -EINVAL;
> I this was not set wouldn't we fail in vm_event_enable with -ENOMEM?
>
> I presume the user can still make this hypercall.. Ah yes.
>
> Perhaps -ENXIO?
>> +
>> + if ( msr <= 0x1fff )
>> + set_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
(It might help to read the following review before coming back here...)
It might be clearer to express monitor_msr_bitmap as a pointer to
struct monitor_msr_bitmap {
uint8_t low[1024];
uint8_t hypervisor[1024];
uint8_t high[1024];
};
which avoids the odd pointer arithmetic.
> The 0x000/BYTER_PER_LONG looks odd. Is it even needed?
>
>> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> + {
>> + msr &= 0x1fff;
>> + set_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
__set_bit(). I don't think you need a LOCK here.
>> + }
>> +
>> + hvm_enable_msr_interception(d, msr);
> And for MSRs above 0xc0001fff it is OK to enable the interception?
> Or between 0x1fff and 0xc0000000?
No real MSRs exist outside the [0..1fff] and [0xc0000000..0xc0001fff]
ranges, so will suffer a #GP. This is even reflected in how both VT-x
and SVM do their MSR interception bitmap, which is why I specifically
suggested using the same here.
However, this case wants a range between [0x40000000..0x40001fff]
>
> No need to filter them out? Or error on them?
>> +
>> + return 0;
>> +}
>> +
>> +static int arch_monitor_disable_msr(struct domain *d, u32 msr)
>> +{
>> + if ( !d->arch.monitor_msr_bitmap )
>> + return -EINVAL;
>> +
>> + if ( msr <= 0x1fff )
>> + clear_bit(msr, d->arch.monitor_msr_bitmap + 0x000/BYTES_PER_LONG);
>> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> + {
>> + msr &= 0x1fff;
>> + clear_bit(msr, d->arch.monitor_msr_bitmap + 0x400/BYTES_PER_LONG);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +bool_t arch_monitor_is_msr_enabled(const struct domain *d, u32 msr)
>> +{
>> + bool_t rc = 0;
>> +
>> + if ( !d->arch.monitor_msr_bitmap )
>> + return 0;
>> +
>> + if ( msr <= 0x1fff )
>> + rc = test_bit(msr, d->arch.monitor_msr_bitmap +
>> 0x000/BYTES_PER_LONG);
>> + else if ( (msr >= 0xc0000000) && (msr <= 0xc0001fff) )
>> + {
>> + msr &= 0x1fff;
>> + rc = test_bit(msr, d->arch.monitor_msr_bitmap +
>> 0x400/BYTES_PER_LONG);
>> + }
> And what if msr requested is above 0xc0001fff ? What then?
>
>> +
>> + return rc;
>> +}
>> +
>> int arch_monitor_domctl_event(struct domain *d,
>> struct xen_domctl_monitor_op *mop)
>> {
>> @@ -77,25 +129,28 @@ int arch_monitor_domctl_event(struct domain *d,
>>
>> case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
> Should this be renamed?
>> {
>> - bool_t old_status = ad->monitor.mov_to_msr_enabled;
>> + bool_t old_status;
>> + int rc;
>> + u32 msr = mop->u.mov_to_msr.msr;
>>
>> - if ( unlikely(old_status == requested_status) )
>> - return -EEXIST;
>> + domain_pause(d);
>>
>> - if ( requested_status && mop->u.mov_to_msr.extended_capture &&
>> - !hvm_enable_msr_exit_interception(d) )
>> - return -EOPNOTSUPP;
>> + old_status = arch_monitor_is_msr_enabled(d, msr);
>>
>> - domain_pause(d);
>> + if ( unlikely(old_status == requested_status) )
>> + {
>> + domain_unpause(d);
>> + return -EEXIST;
>> + }
>>
>> - if ( requested_status && mop->u.mov_to_msr.extended_capture )
>> - ad->monitor.mov_to_msr_extended = 1;
>> + if ( requested_status )
>> + rc = arch_monitor_enable_msr(d, msr);
>> else
>> - ad->monitor.mov_to_msr_extended = 0;
>> + rc = arch_monitor_disable_msr(d, msr);
>>
>> - ad->monitor.mov_to_msr_enabled = requested_status;
>> domain_unpause(d);
>> - break;
>> +
>> + return rc;
>> }
>>
>> case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 5635603..9b4267e 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -27,6 +27,13 @@ int vm_event_init_domain(struct domain *d)
>> {
>> struct vcpu *v;
>>
>> + d->arch.monitor_msr_bitmap = alloc_xenheap_page();
> How about using vzalloc?
vmap space is far more limited than general xenheap space. vmap()
should only be used when you need >4K allocations contiguously in
virtual address space.
>> +
>> + if ( !d->arch.monitor_msr_bitmap )
>> + return -ENOMEM;
>> +
>> + memset(d->arch.monitor_msr_bitmap, 0, PAGE_SIZE);
> Then you don't have to do that.
clear_page()
>
>> +
>> for_each_vcpu ( d, v )
>> {
>> if ( v->arch.vm_event )
>> @@ -55,6 +62,9 @@ void vm_event_cleanup_domain(struct domain *d)
>> v->arch.vm_event = NULL;
>> }
>>
>> + free_xenheap_page(d->arch.monitor_msr_bitmap);
> And this would be vfree.
>
>> + d->arch.monitor_msr_bitmap = NULL;
>> +
>> d->arch.mem_access_emulate_each_rep = 0;
>> memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>> memset(&d->monitor, 0, sizeof(d->monitor));
>> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
>> index 2457698..875c09a 100644
>> --- a/xen/include/public/domctl.h
>> +++ b/xen/include/public/domctl.h
>> @@ -1107,8 +1107,7 @@ struct xen_domctl_monitor_op {
>> } mov_to_cr;
>>
>> struct {
>> - /* Enable the capture of an extended set of MSRs */
>> - uint8_t extended_capture;
>> + uint32_t msr;
> Whoa there. Isn't it expanding the structure? Will this be backwards
> compatible? What if somebody is using an older version of xen-access
> against this hypervisor? Will they work?
Its a domctl. This is perfectly fine (within the rules) to do.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |