[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 2/7] xen/riscv: set up fixmap mappings
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. 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) }; } 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(). Am I missing something? ~ Oleksii
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |