Re: [PATCH 4/5] xen/vm_event: Deny MSR writes if refused by vm_event reply

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 05/08/2015 07:23 PM, Jan Beulich wrote:
>>>> On 06.05.15 at 19:12, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -518,6 +518,11 @@ struct arch_vcpu
>>          struct vm_event_emul_read_data emul_read_data;
>>      } vm_event;
>>  
>> +    struct {
>> +        bool_t do_write;
>> +        uint64_t msr;
>> +        uint64_t value;
>> +    } msr_write;
>>  };
> 
> Again a growth of struct vcpu by 24 bytes for everyone even though
> quite likely only very few VMs would actually need this. To be honest
> I'd even be hesitant to accept a pointer addition here. Perhaps this
> should be a suitably sized, dynamically allocated array hanging off of
> struct domain?

Sorry, I don't follow the dynamically allocated _array_ part. Could you
please give a small example of what you mean?

>> --- a/xen/include/public/vm_event.h
>> +++ b/xen/include/public/vm_event.h
>> @@ -158,6 +158,11 @@ struct vm_event_regs_x86 {
>>   * MEM_ACCESS_EMULATE_NOWRITE.
>>   */
>>  #define MEM_ACCESS_SET_EMUL_READ_DATA   (1 << 8)
>> + /*
>> +  * If mov_to_msr events are enabled, setting this flag in the vm_event
>> +  * response denies the MSR write that triggered the event.
>> +  */
>> +#define MEM_ACCESS_SKIP_MSR_WRITE       (1 << 9)
> 
>>From an interface point of view - does this need to be MSR-
> specific? I.e. can't this just be a flag to deny whatever the
> operation was (not necessarily supported/valid for all events,
> but possibly for more than just MSR writes)?

Yes, that's a good idea - it could just be a DENY flag, and the actual
action to be rejected can be inferred from the response type.


Thanks,
Razvan