Re: [Xen-devel] [RFC PATCH V2 8/8] x86/hvm: factor out vm_event related functions into separate file

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

On Fri, Jan 23, 2015 at 10:03 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 23.01.15 at 09:56, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> On 01/22/2015 06:42 PM, Tamas K Lengyel wrote:
>>> On Thu, Jan 22, 2015 at 5:25 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 18.01.15 at 16:18, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>>>>> +void hvm_event_msr(unsigned long msr, unsigned long value)
>>>>> +{
>>>>> +    vm_event_request_t req = {
>>>>> +        .reason = VM_EVENT_REASON_MSR,
>>>>> +        .vcpu_id = current->vcpu_id,
>>>>> +        .msr_event.msr = msr,
>>>>> +        .msr_event.new_value = value,
>>>>> +    };
>>>>
>>>> The .msr_event sub-structure also has an old_value member - how
>>>> come this doesn't get filled (I realize the old code was this way,
>>>> but I now doubt earlier patches are all correct in the regard).
>>>
>>> Razvan might have more information on this side as I haven't really
>>> touched MSR events. I vaguely recall some issues with having access to
>>> the old MSR value?
>>
>> Indeed, getting the previous value would be a bit involved. Please see
>> xen/arch/x86/hvm/hvm.c, specifically hvm_msr_write_intercept(). At first
>> glance, you'd have to call hvm_msr_read_intercept() to get the previous
>> value, or create some sort of cache for previous values for all MSRs,
>> updated by hvm_msr_write_intercept().
>>
>> Since this has not had any real value for my use-case, and since my code
>> has only needed to keep track of a handful of MSRs, I took the route of
>> caching previous values for them in the userspace application.
>>
>> This is, of course, not to say that it can't be done at the HV level,
>> just that (at least IMHO) the HV-level costs have outweighed the benefits.
>
> In which case the respective interface structure field should have a
> comment attached saying that it currently doesn't hold what the field
> name promises (and, without having checked whether this may already
> be the case, it should be made hold a deterministic value, e.g. zero).
>
> Jan

Ack, I'll remove it as we don't need unused struct members to pollute
the landscape.

Tamas

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