qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Rebecca Cran
Subject: Re: [PATCH v3 1/3] target/arm: Add support for FEAT_TLBIRANGE
Date: Wed, 10 Mar 2021 14:59:48 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

On 3/10/21 12:24 PM, Richard Henderson wrote:
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.

Thanks for the comments. I'll continue working on the full/proper implementation (including changing the interface to remove num_pages) and send out a v4.

--
Rebecca Cran



reply via email to

[Prev in Thread] Current Thread [Next in Thread]