[PATCH v5 2/3] server: Add USD support with timestamp updates.

[Rémi Bernon]

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>