All of lore.kernel.org
 help / color / mirror / Atom feed
From: Richard Henderson <richard.henderson@linaro.org>
To: "Alex Bennée" <alex.bennee@linaro.org>
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH] accel/tcg: Probe the proper permissions for atomic ops
Date: Mon, 14 Jun 2021 07:49:10 -0700	[thread overview]
Message-ID: <fb5f29d8-d343-f7ca-020c-b94c0f81fe34@linaro.org> (raw)
In-Reply-To: <87lf7ck9vp.fsf@linaro.org>

On 6/14/21 3:18 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> We had a single ATOMIC_MMU_LOOKUP macro that probed for
>> read+write on all atomic ops.  This is incorrect for
>> plain atomic load and atomic store.
>>
>> For user-only, we rely on the host page permissions.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/390
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
> <snip>
>>   
>> -/* Probe for a read-modify-write atomic operation.  Do not allow unaligned
>> - * operations, or io operations to proceed.  Return the host address.  */
>> +/*
>> + * Probe for an atomic operation.  Do not allow unaligned operations,
>> + * or io operations to proceed.  Return the host address.
>> + *
>> + * @prot may be PAGE_READ, PAGE_WRITE, or PAGE_READ|PAGE_WRITE.
>> + */
>>   static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>> -                               TCGMemOpIdx oi, uintptr_t retaddr)
>> +                               TCGMemOpIdx oi, int prot, uintptr_t retaddr)
>>   {
>>       size_t mmu_idx = get_mmuidx(oi);
>> -    uintptr_t index = tlb_index(env, mmu_idx, addr);
>> -    CPUTLBEntry *tlbe = tlb_entry(env, mmu_idx, addr);
>> -    target_ulong tlb_addr = tlb_addr_write(tlbe);
>>       MemOp mop = get_memop(oi);
>>       int a_bits = get_alignment_bits(mop);
>>       int s_bits = mop & MO_SIZE;
>> +    uintptr_t index;
>> +    CPUTLBEntry *tlbe;
>> +    target_ulong tlb_addr;
>>       void *hostaddr;
>>   
>>       /* Adjust the given return address.  */
>> @@ -1775,15 +1779,45 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
>>           goto stop_the_world;
>>       }
>>   
>> +    index = tlb_index(env, mmu_idx, addr);
>> +    tlbe = tlb_entry(env, mmu_idx, addr);
>> +
>>       /* Check TLB entry and enforce page permissions.  */
>> -    if (!tlb_hit(tlb_addr, addr)) {
>> -        if (!VICTIM_TLB_HIT(addr_write, addr)) {
>> -            tlb_fill(env_cpu(env), addr, 1 << s_bits, MMU_DATA_STORE,
>> -                     mmu_idx, retaddr);
>> -            index = tlb_index(env, mmu_idx, addr);
>> -            tlbe = tlb_entry(env, mmu_idx, addr);
>> +    if (prot & PAGE_WRITE) {
>> +        tlb_addr = tlb_addr_write(tlbe);
>> +        if (!tlb_hit(tlb_addr, addr)) {
>> +            if (!VICTIM_TLB_HIT(addr_write, addr)) {
>> +                tlb_fill(env_cpu(env), addr, 1 << s_bits,
>> +                         MMU_DATA_STORE, mmu_idx, retaddr);
>> +                index = tlb_index(env, mmu_idx, addr);
>> +                tlbe = tlb_entry(env, mmu_idx, addr);
>> +            }
>> +            tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK;
>> +        }
>> +
>> +        /* Let the guest notice RMW on a write-only page.  */
>> +        if ((prot & PAGE_READ) &&
>> +            unlikely(tlbe->addr_read != (tlb_addr & ~TLB_NOTDIRTY))) {
>> +            tlb_fill(env_cpu(env), addr, 1 << s_bits,
>> +                     MMU_DATA_LOAD, mmu_idx, retaddr);
>> +            /*
>> +             * Since we don't support reads and writes to different addresses,
>> +             * and we do have the proper page loaded for write, this shouldn't
>> +             * ever return.  But just in case, handle via stop-the-world.
>> +             */
>> +            goto stop_the_world;
>> +        }
>> +    } else /* if (prot & PAGE_READ) */ {
> 
> I guess the compiler doesn't have enough info to know that !PROT_WRITE
> implies PROT_READ and ends up doing an extra check it doesn't need to otherwise?

I didn't even look at the compiler output.

r~

> 
> Otherwise seems sane to me:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 



      reply	other threads:[~2021-06-14 14:51 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-13  0:25 Richard Henderson
2021-06-14 10:18 ` Alex Bennée
2021-06-14 14:49   ` Richard Henderson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fb5f29d8-d343-f7ca-020c-b94c0f81fe34@linaro.org \
    --to=richard.henderson@linaro.org \
    --cc=alex.bennee@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --subject='Re: [PATCH] accel/tcg: Probe the proper permissions for atomic ops' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.