On 5/20/25 5:04 PM, Jan Beulich wrote:
On 09.05.2025 17:57, Oleksii Kurochko wrote:
Refactor pte_t to be a union which hold page table entry plus
pt_t and pt_walk_t structures to simpilfy p2m functions.
Is this really simplifying things? I really view ...
Also, introduce some helpers which are using pt_walk_t.
... these helpers as confusing things, by using the wrong part of the
union relative to what their names are. (I'll re-phrase this some at
the bottom.)
With the union it's also unclear to me how to know which part of the
union is the one that's valid to use, when there's no auxiliary info
available.
Everything is valid to use and depends on the context if it is convenient
or not. It is hard to come up with a rule when and what should be used.
--- a/xen/arch/riscv/include/asm/page.h
+++ b/xen/arch/riscv/include/asm/page.h
@@ -99,15 +99,67 @@
#endif
-/* Page Table entry */
typedef struct {
+ unsigned long v:1;
+ unsigned long r:1;
+ unsigned long w:1;
+ unsigned long x:1;
+ unsigned long u:1;
+ unsigned long g:1;
+ unsigned long a:1;
+ unsigned long d:1;
+ unsigned long rsw:2;
+#if RV_STAGE1_MODE == SATP_MODE_SV39
+ unsigned long ppn0:9;
+ unsigned long ppn1:9;
+ unsigned long ppn2:26;
+ unsigned long rsw2:7;
+ unsigned long pbmt:2;
+ unsigned long n:1;
+#elif RV_STAGE1_MODE == SATP_MODE_SV48
+ unsigned long ppn0:9;
+ unsigned long ppn1:9;
+ unsigned long ppn2:9;
+ unsigned long ppn3:17;
+ unsigned long rsw2:7;
+ unsigned long pbmt:2;
+ unsigned long n:1;
+#else
Imo you want to settle on whether you want to use bitfields or #define-s
to manipulate page table entries. Using a mix is going to be confusing
(or worse).
Generally, I am okay to use something one.
But technically it is the same things from result point of view, just
different is access of a field by using a union or do a bit manipulation
operations.
+#error "Add proper bits for SATP_MODE"
+#endif
+} pt_t;
+
+typedef struct {
+ unsigned long rsw:10;
+#if RV_STAGE1_MODE == SATP_MODE_SV39 || RV_STAGE1_MODE == SATP_MODE_SV48
+ unsigned long ppn: 44;
Nit: Why's there a blank after the colon here when there's none anywhere else?
+static inline void pte_set_mfn(pte_t *pte, mfn_t mfn)
+{
+ pte->walk.ppn = mfn_x(mfn);
+}
+
+static inline mfn_t pte_get_mfn(pte_t pte)
+{
+ return _mfn(pte.walk.ppn);
+}
Following to my remark at the top - if you do it this way, what use are the
ppn<N> fields?
|ppn<N>| isn't actually used; it was added only to follow the PTE format
specified
in the architecture spec. Technically,|ppn<N>| could be merged into|ppn|,
but IMO, keeping|ppn<N>| improves self-documentation of the page table format.
~ Oleksii
