Re: [Xen-devel] [PATCH v2 2/4] xen/x86: Drop unnecessary barriers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/08/17 16:23, Jan Beulich wrote:
>>>> On 16.08.17 at 13:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>> x86's current implementation of wmb() is a compiler barrier.  As a result, 
>> the
>> only change in this patch is to remove an mfence instruction from
>> cpuidle_disable_deep_cstate().
>>
>> None of these barriers serve any purpose.  Most aren't aren't synchronising
>> with any remote cpus, where as the mcetelem barriers are redundant with
>> spin_unlock(), which already has full read/write barrier semantics.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> For the relevant parts
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> For the parts the ack doesn't extend to, however:
>
>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -3112,7 +3112,6 @@ static int sh_page_fault(struct vcpu *v,
>>       * will make sure no inconsistent mapping being translated into
>>       * shadow page table. */
>>      version = atomic_read(&d->arch.paging.shadow.gtable_dirty_version);
>> -    rmb();
>>      walk_ok = sh_walk_guest_tables(v, va, &gw, error_code);
> Isn't this supposed to make sure version is being read first? I.e.
> doesn't this at least need to be barrier()?

atomic_read() is not free to be reordered by the compiler.  It is an asm
volatile with a volatile memory reference.

>
>> index a459e99..d5b6049 100644
>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -558,7 +558,6 @@ static void parse_event_log_entry(struct amd_iommu 
>> *iommu, u32 entry[])
>>              return;
>>          }
>>          udelay(1);
>> -        rmb();
>>          code = get_field_from_reg_u32(entry[1], IOMMU_EVENT_CODE_MASK,
>>                                        IOMMU_EVENT_CODE_SHIFT);
>>      }
>> @@ -663,7 +662,6 @@ void parse_ppr_log_entry(struct amd_iommu *iommu, u32 
>> entry[])
>>              return;
>>          }
>>          udelay(1);
>> -        rmb();
>>          code = get_field_from_reg_u32(entry[1], IOMMU_PPR_LOG_CODE_MASK,
>>                                        IOMMU_PPR_LOG_CODE_SHIFT);
>>      }
> With these fully removed, what keeps the compiler from moving
> the entry[1] reads out of the loop? Implementation details of
> udelay() don't count...

It is a write to the control variable which is derived from a non-local
non-constant object.  It can't be hoisted at all.

Consider this simplified version:

while ( count == 0 )
    count = entry[1];

If entry were const, the compiler would be free to expect that the value
doesn't change on repeated reads, but that is not the case here.

~Andrew

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