Re: [Xen-devel] [PATCH v1] x86/hvm: Generic instruction re-execution mechanism for execute faults

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.11.18 at 18:04, <roger.pau@xxxxxxxxxx> wrote:
> On Fri, Nov 16, 2018 at 10:06:36AM +0000, Alexandru Stefan ISAILA wrote:
>> @@ -377,6 +379,8 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>      p2m_access_t a;
>>      unsigned long gfn_l;
>>      long rc = 0;
>> +    struct vcpu *v;
>> +    int i;
>>  
>>      /* altp2m view 0 is treated as the hostp2m */
>>  #ifdef CONFIG_HVM
>> @@ -413,6 +417,16 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, 
>> uint32_t nr,
>>          if ( rc )
>>              break;
>>  
>> +        for_each_vcpu(d, v)
>> +        {
>> +            if ( !v->arch.rexec_level )
>> +                continue;
>> +
>> +            for ( i = v->arch.rexec_level - 1; i >= 0; i-- )
> 
> Is there any reason this has to be done backwards?
> 
> If you do it from 0 to v->arch.rexec_level you could use an unsigned
> int as the index.

And even if there's need for this going backwards the variable should
still be unsigned (using "for ( i = v->arch.rexec_level; i--; )" then,
presumably allowing the if() above to be dropped altogether).

>> +                if ( (v->arch.rexec_context[i].gpa >> PAGE_SHIFT) == 
>> gfn_x(gfn) )
> 
> PFN_DOWN instead of the right shift, and maybe use gfn_eq instead of
> converting gfn.

ITYM gaddr_to_gfn() instead of PFN_DOWN.

>> --- a/xen/include/asm-x86/hvm/hvm.h
>> +++ b/xen/include/asm-x86/hvm/hvm.h
>> @@ -167,6 +167,8 @@ struct hvm_function_table {
>>  
>>      int  (*cpu_up)(void);
>>      void (*cpu_down)(void);
>> +    int  (*start_reexecute_instruction)(struct vcpu *v, unsigned long gpa,
>> +                                        xenmem_access_t required_access);
> 
> I would name this reexecute_instruction, I don't think the start_
> prefix adds any value to the handler.

Or even just rexec_insn, to cut down on name length. I also
dislike the insertion point: This should live amidst the less "core"
hooks further down - there's already a block of three
introspection related hooks where this one would likely be a
good fit.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel