Re: [Xen-devel] [PATCH V2] x86/emulate: synchronize LOCKed instruction emulation

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 03/15/2017 06:30 PM, Jan Beulich wrote:
>>>> On 15.03.17 at 17:04, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> ---
>> Changes since V1:
>>  - Added Andrew Cooper's credit, as he's kept the patch current
>>    througout non-trivial code changes since the initial patch.
>>  - Significantly more patch testing (with XenServer).
>>  - Restricted lock scope.
> 
> Not by much, as it seems. In particular you continue to take the
> lock even for instructions not accessing memory at all.

I'll take a closer look.

> Also, by "reworked" I did assume you mean converted to at least the
> cmpxchg based model.

I haven't been able to follow the latest emulator changes closely, could
you please clarify what you mean by "the cmpxchg model"? Thanks.

>> --- a/tools/tests/x86_emulator/test_x86_emulator.c
>> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
>> @@ -283,6 +283,14 @@ static int read_msr(
>>      return X86EMUL_UNHANDLEABLE;
>>  }
>>  
>> +static void smp_lock(bool locked)
>> +{
>> +}
>> +
>> +static void smp_unlock(bool locked)
>> +{
>> +}
> 
> I don't think the hooks should be a requirement, and hence these
> shouldn't be needed.

I'll make them optional.

>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -529,6 +529,8 @@ int arch_domain_create(struct domain *d, unsigned int 
>> domcr_flags,
>>      if ( config == NULL && !is_idle_domain(d) )
>>          return -EINVAL;
>>  
>> +    percpu_rwlock_resource_init(&d->arch.emulate_lock, 
>> emulate_locked_rwlock);
> 
> This should move into the same file as where the lock and the hook
> functions live, so that the variable can be static. I'm not sure ...
> 
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -24,6 +24,8 @@
>>  #include <asm/hvm/svm/svm.h>
>>  #include <asm/vm_event.h>
>>  
>> +DEFINE_PERCPU_RWLOCK_GLOBAL(emulate_locked_rwlock);
> 
> ... this is the right file, though, considering the wide (including PV)
> use of it.

I'll hunt for a better place for it.

>> @@ -3065,6 +3065,8 @@ x86_emulate(
>>      d = state.desc;
>>  #define state (&state)
>>  
>> +    ops->smp_lock(lock_prefix);
> 
> There's a "goto complete_insn" upwards from here, which therefore
> bypasses the acquire, but goes through the release path. Also this is
> still too early to take the lock.

True, it appears that there have been changes in staging since the last
test. I'll need to follow the locking patch carefully.

>> @@ -7925,8 +7932,11 @@ x86_emulate(
>>      ctxt->regs->eflags &= ~X86_EFLAGS_RF;
>>  
>>   done:
>> +    ops->smp_unlock(lock_prefix);
> 
> And this, imo, is too late (except for covering error exits coming
> here). I don't think you can avoid having a local tracking variable.

Fair enough. Will use one.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -448,6 +448,14 @@ struct x86_emulate_ops
>>      /* vmfunc: Emulate VMFUNC via given set of EAX ECX inputs */
>>      int (*vmfunc)(
>>          struct x86_emulate_ctxt *ctxt);
>> +
>> +    /* smp_lock: Take a write lock if locked, read lock otherwise. */
>> +    void (*smp_lock)(
>> +        bool locked);
>> +
>> +    /* smp_unlock: Write unlock if locked, read unlock otherwise. */
>> +    void (*smp_unlock)(
>> +        bool locked);
>>  };
> 
> All hooks should take a ctxt pointer.

I'll try to adapt them.


Thanks,
Razvan

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