|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH RFCv2 02/15] xen/arm: lpae: Use the generic helpers to defined the Xen PT helpers
Hi Stefano, On 13/07/2021 21:53, Stefano Stabellini wrote: On Sat, 3 Jul 2021, Julien Grall wrote:Hi Stefano, Sorry for the late answer. On 13/05/2021 23:44, Stefano Stabellini wrote:On Wed, 12 May 2021, Julien Grall wrote:Hi Stefano, On 12/05/2021 22:30, Stefano Stabellini wrote:On Wed, 12 May 2021, Julien Grall wrote:+#define LPAE_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) +#define LPAE_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) +#define LPAE_ENTRY_MASK LPAE_ENTRY_MASK_GS(PAGE_SHIFT) +#define LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) +#define LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) +#define LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) +#define LEVEL_MASK(lvl) (~(LEVEL_SIZE(lvl) - 1))I would avoid adding these 4 macros. It would be OK if they were just used within this file but lpae.h is a header: they could end up be used anywhere in the xen/ code and they have a very generic name. My suggestion would be to skip them and just do:Those macros will be used in follow-up patches. They are pretty useful to avoid introduce static array with the different information for each level. Would prefix them with XEN_ be better?Maybe. The concern I have is that there are multiple page granularities (4kb, 16kb, etc) and multiple page sizes (4kb, 2mb, etc). If I just see LEVEL_ORDER it is not immediately obvious what granularity and what size we are talking about.I am a bit puzzled with your answer. AFAIU, you are happy with the existing macros (THIRD_*, SECOND_*) but not with the new macros. In reality, there is no difference because THIRD_* doesn't tell you the exact size but only "this is a level 3 mapping". So can you clarify what you are after? IOW is it reworking the current naming scheme?You are right -- there is no real difference between THIRD_*, SECOND_* and LEVEL_*. The original reason for my comments is that I hadn't read the following patches, and the definition of LEVEL_* macros is simple, they could be open coded. It looked like they were only going to be used to make the definition of THIRD_*, SECOND_* a bit easier. So, at first, I was wondering if they were needed at all. Secondly, I realized that they were going to be used in *.c files by other patches. That's why they are there. But I started thinking whether we should find a way to make it a bit clearer that they are for Xen pages, currently at 4KB granularity. THIRD_*, SECOND_*, etc. are already generic names which don't convey the granularity or whether they are Xen pages at all. But LEVEL_* seem even more generic. As I mentioned, I don't have any good suggestions for changes to make here, so unless you can come up with a good idea let's keep it as is.I am thinking to use the following naming (diff on top of this patch): -#define LPAE_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) -#define LPAE_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) -#define LPAE_ENTRY_MASK LPAE_ENTRY_MASK_GS(PAGE_SHIFT) +#define XEN_PT_SHIFT LPAE_SHIFT_GS(PAGE_SHIFT) +#define XEN_PT_ENTRIES LPAE_ENTRIES_GS(PAGE_SHIFT) +#define XEN_PT_ENTRY_MASK LPAE_ENTRY_MASK_GS(PAGE_SHIFT) -#define LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) -#define LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) -#define LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) -#define LEVEL_MASK(lvl) (~(LEVEL_SIZE(lvl) - 1)) +#define XEN_PT_LEVEL_SHIFT(lvl) LEVEL_SHIFT_GS(PAGE_SHIFT, lvl) +#define XEN_PT_LEVEL_ORDER(lvl) LEVEL_ORDER_GS(PAGE_SHIFT, lvl) +#define XEN_PT_LEVEL_SIZE(lvl) LEVEL_SIZE_GS(PAGE_SHIFT, lvl) +#define XEN_PT_LEVEL_MASK(lvl) (~(LEVEL_SIZE(lvl) - 1)) /* Convenience aliases */ -#define THIRD_SHIFT LEVEL_SHIFT(3) -#define THIRD_ORDER LEVEL_ORDER(3) -#define THIRD_SIZE LEVEL_SIZE(3) -#define THIRD_MASK LEVEL_MASK(3) - -#define SECOND_SHIFT LEVEL_SHIFT(2) -#define SECOND_ORDER LEVEL_ORDER(2) -#define SECOND_SIZE LEVEL_SIZE(2) -#define SECOND_MASK LEVEL_MASK(2) - -#define FIRST_SHIFT LEVEL_SHIFT(1) -#define FIRST_ORDER LEVEL_ORDER(1) -#define FIRST_SIZE LEVEL_SIZE(1) -#define FIRST_MASK LEVEL_MASK(1) - -#define ZEROETH_SHIFT LEVEL_SHIFT(0) -#define ZEROETH_ORDER LEVEL_ORDER(0) -#define ZEROETH_SIZE LEVEL_SIZE(0) -#define ZEROETH_MASK LEVEL_MASK(0) +#define THIRD_SHIFT XEN_PT_LEVEL_SHIFT(3) +#define THIRD_ORDER XEN_PT_LEVEL_ORDER(3) +#define THIRD_SIZE XEN_PT_LEVEL_SIZE(3) +#define THIRD_MASK XEN_PT_LEVEL_MASK(3) + +#define SECOND_SHIFT XEN_PT_LEVEL_SHIFT(2) +#define SECOND_ORDER XEN_PT_LEVEL_ORDER(2) +#define SECOND_SIZE XEN_PT_LEVEL_SIZE(2) +#define SECOND_MASK XEN_PT_LEVEL_MASK(2) + +#define FIRST_SHIFT XEN_PT_LEVEL_SHIFT(1) +#define FIRST_ORDER XEN_PT_LEVEL_ORDER(1) +#define FIRST_SIZE XEN_PT_LEVEL_SIZE(1) +#define FIRST_MASK XEN_PT_LEVEL_MASK(1) + +#define ZEROETH_SHIFT XEN_PT_LEVEL_SHIFT(0) +#define ZEROETH_ORDER XEN_PT_LEVEL_ORDER(0) +#define ZEROETH_SIZE XEN_PT_LEVEL_SIZE(0) +#define ZEROETH_MASK XEN_PT_LEVEL_MASK(0) I don't plan to modify the nameing for ZEROETH*, FIRST*, SECOND*, THIRD*. Let me know if you prefer it over the currrent naming.Yes, I think it is better, thanks! Ok. I will try to respin the series soon. Cheers, -- Julien Grall
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |