[PATCH v5 2/3] server: Add USD support with timestamp updates.
Rémi Bernon
rbernon at codeweavers.com
Thu May 7 16:07:46 CDT 2020
On 5/7/20 10:45 PM, Alexandre Julliard wrote:
> Rémi Bernon <rbernon at codeweavers.com> writes:
>
>> +#if defined(__GNUC__) && ((__GNUC__ > 4) || ((__GNUC__ == 4) && (__GNUC_MINOR__ >= 7)))
>> + __atomic_store_n(&ptr->SystemTime.High2Time, system_time_high, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->SystemTime.LowPart, system_time_low, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->SystemTime.High1Time, system_time_high, __ATOMIC_SEQ_CST);
>> +
>> + __atomic_store_n(&ptr->InterruptTime.High2Time, interrupt_time_high, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->InterruptTime.LowPart, interrupt_time_low, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->InterruptTime.High1Time, interrupt_time_high, __ATOMIC_SEQ_CST);
>> +
>> + __atomic_store_n(&ptr->TickCount.High2Time, tick_count_high, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->TickCount.LowPart, tick_count_low, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->TickCount.High1Time, tick_count_high, __ATOMIC_SEQ_CST);
>> + __atomic_store_n(&ptr->TickCountLowDeprecated, tick_count_low, __ATOMIC_SEQ_CST);
>
> Is this gcc-specific code really necessary?
>
I'm not sure it is strictly necessary but then it depends what
guarantees we want to have. The volatile qualifier only acts as a
compiler barrier and will make sure that the stores aren't optimized or
reordered by the compiler. However it doesn't enforce anything on the
CPU. I believe that X86 has global store ordering guarantees, but a
quick search tells me that ARM or POWER don't, so the stores to the
various members may be seen out of order from another CPU depending on
the architecture.
Then, if it is about the __atomic builtins usage rather than __sync
builtins that are sometimes used elsewhere, it's because these builtins
provide cleaner semantics, as well as read-only and write-only
operations -- whereas __sync only offers read+write operations. The page
is mapped write-only in the server, so there was no guarantee that it
was possible to read it. It's maybe a bit overkill and PROT_WRITE also
somtimes implies PROT_READ, so maybe it should go this way and use
__sync builtins instead?
>> +obj_handle_t get_usd_handle( const void *usd_init, data_size_t usd_size )
>> +{
>> + /* keep it the same as user_shared_data_size in ntdll */
>> + size_t size = 0x10000;
>> + struct fd *fd;
>> +
>> + if (sizeof(*kusd) != usd_size)
>> + {
>> + set_error( STATUS_INVALID_PARAMETER );
>> + return 0;
>> + }
>> +
>> + if (!kusd_mapping)
>> + {
>> + if (!(kusd_mapping = create_mapping( NULL, NULL, 0, size, SEC_COMMIT, 0, 0, NULL )))
>> + return 0;
>> + make_object_static( kusd_mapping );
>> +
>> + if (!(fd = mapping_get_fd( kusd_mapping )))
>> + return 0;
>> +
>> + if ((kusd = mmap( NULL, size, PROT_WRITE, MAP_SHARED, get_unix_fd( fd ), 0 )) != MAP_FAILED)
>> + {
>> + memcpy( kusd, usd_init, usd_size );
>> + kusd_set_current_time( NULL );
>> + }
>> + release_object( fd );
>> + }
>> +
>> + if (kusd == MAP_FAILED)
>> + {
>> + set_error( STATUS_NO_MEMORY );
>> + return 0;
>> + }
>> +
>> + return alloc_handle( current->process, kusd_mapping, SECTION_QUERY|SECTION_MAP_READ, 0 );
>> +}
>
> I still think it would be cleaner to use standard APIs for this,
> something like NtCreateSection("__wine_shared_user_data"), on
> STATUS_SUCCESS initialize it and tell the server about it, and on
> STATUS_OBJECT_NAME_EXISTS simply map it.
>
Alright.
--
Rémi Bernon <rbernon at codeweavers.com>
More information about the wine-devel
mailing list