Re: [PATCH v3 1/3] target/arm: Add support for FEAT_TLBIRANGE

[Richard Henderson]

On 3/9/21 6:29 PM, Rebecca Cran wrote:
> +void tlb_flush_page_range_by_mmuidx(CPUState *cpu, target_ulong addr,
> +                                    unsigned int num_pages, uint16_t idxmap)

I am not keen on this interface.  I think you should take either start+end 
addresses (inclusive) or start+length (in bytes).

Using num_pages, and as an unsigned int, seems too easy to fail when applied to 
a different guest.

> +{
> +  /*
> +   * We currently do a full flush, but for performance this should be
> +   * updated to only flush the requested pages, taking TBI into account.
> +   */
> +    tlb_flush_by_mmuidx(cpu, idxmap);
> +}

And if you're going to cop-out like this, you might as well just do it in 
target/arm and not add these new interfaces at all.

> +#ifdef TARGET_AARCH64
> +static unsigned int tlbi_aa64_range_get_num_pages(CPUARMState *env,
> +                                                  uint64_t value,
> +                                                  uint64_t addr)
> +{
> +    unsigned int page_size;
> +    unsigned int page_shift;
> +    unsigned int page_size_granule;
> +    uint64_t num;
> +    uint64_t scale;
> +    uint64_t exponent;
> +    uint64_t high_addr;
> +
> +    num = (value >> 39) & 0xF;
> +    scale = (value >> 44) & 0x3;
> +    page_size_granule = (value >> 46) & 0x3;

extract64()

> +
> +    switch (page_size_granule) {
> +    case 1:
> +      page_size = 4096;
> +      page_shift = 12;
> +      break;
> +    case 2:
> +      page_size = 16384;
> +      page_shift = 14;
> +      break;
> +    case 3:
> +      page_size = 65536;
> +      page_shift = 16;
> +      break;
> +    default:
> +      qemu_log_mask(LOG_GUEST_ERROR, "Invalid page size granule %d\n",
> +                    page_size_granule);
> +
> +      raise_exception(env, EXCP_UDEF, syn_uncategorized(),
> +                      exception_target_el(env));

You can't raise an exception from here, because you don't have all of the 
context for unwinding the tcg state.  Which you cannot access from within the 
callback of an ARMCPRegInfo.

The manual says that if TG does not correspond to the granule size of the 
actual translation then "the architecture does not require that the instruction 
invalidates any entries".  "Reserved" can be safely assumed to "not 
correspond", so I think you could just as easily return 0 here, after logging 
the guest error.


> +    high_addr = addr + (((num + 1) << exponent) * page_size);
> +
> +    return (high_addr - addr) >> page_shift;

I'll note that it would be much easier for you to return a byte value for the 
length, and that you don't actually need to pass in addr at all.

> +    uint64_t addr = (value & 0xFFFFFFFFFUL) << TARGET_PAGE_BITS;

The manual does not explicitly say, but I'm certain that this should be a 
signed address, when regime_has_2_ranges().  Otherwise it would be impossible 
to flush a range of kernel addresses.

But all of this is moot if we're just going to flush all pages.  At which point 
you might as well simply re-use tlbi_aa64_vmalle1_write et al.  Place your TODO 
comment in front of tlbirange_reginfo[] instead of buried n-levels further down.


r~