[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 2/7] xen/riscv: set up fixmap mappings
On 30.08.2024 13:55, oleksii.kurochko@xxxxxxxxx wrote: > On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote: >>> @@ -81,6 +82,18 @@ static inline void flush_page_to_ram(unsigned >>> long mfn, bool sync_icache) >>> BUG_ON("unimplemented"); >>> } >>> >>> +/* Write a pagetable entry. */ >>> +static inline void write_pte(pte_t *p, pte_t pte) >>> +{ >>> + write_atomic(p, pte); >>> +} >>> + >>> +/* Read a pagetable entry. */ >>> +static inline pte_t read_pte(pte_t *p) >>> +{ >>> + return read_atomic(p); >> >> This only works because of the strange type trickery you're playing >> in >> read_atomic(). Look at x86 code - there's a strict expectation that >> the >> type can be converted to/from unsigned long. And page table accessors >> are written with that taken into consideration. Same goes for >> write_pte() >> of course, with the respective comment on the earlier patch in mind. >> >> Otoh I see that Arm does something very similar. If you have a strong >> need / desire to follow that, then please at least split the two >> entirely separate aspects that patch 1 presently changes both in one >> go. > I am not 100% sure that type trick could be dropped easily for RISC-V: > 1. I still need the separate C function for proper #ifdef-ing: > #ifndef CONFIG_RISCV_32 > case 8: *(uint32_t *)res = readq_cpu(p); break; > #endif > > 2. Because of the point 1 the change should be as following: > -#define read_atomic(p) ({ \ > - union { typeof(*(p)) val; char c[sizeof(*(p))]; } x_; \ > - read_atomic_size(p, x_.c, sizeof(*(p))); \ > - x_.val; \ > +#define read_atomic(p) ({ \ > + unsigned long x_; \ > + read_atomic_size(p, &x_, sizeof(*(p))); \ > + (typeof(*(p)))x_; \ > }) > But after that I think it will be an error: "conversion to non-scalar > type requested" in the last line as *p points to pte_t. > > and we can't just in read_pte() change to: > static inline pte_t read_pte(pte_t *p) > { > return read_atomic(&p->pte); > } > As in this cases it started it will return unsigned long but function > expects pte_t. Of course. > As an option read_pte() can be updated to: > /* Read a pagetable entry. */ > static inline pte_t read_pte(pte_t *p) > { > return (pte_t) { .pte = read_atomic(&p->pte) }; > } That's what's needed. > But I am not sure that it is better then just have a union trick inside > read_atomic() and then just have read_atomic(p) for read_pte(). It's largely up to you. My main request is that things end up / remain consistent. Which way round is secondary, and often merely a matter of suitably justifying the choice made. Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |