On 16.09.2021 00:13, Oleksandr wrote: > On 15.09.21 13:06, Jan Beulich wrote: >> On 14.09.2021 22:44, Oleksandr Tyshchenko wrote: >>> --- a/xen/include/asm-arm/mm.h >>> +++ b/xen/include/asm-arm/mm.h >>> @@ -98,9 +98,18 @@ struct page_info >>> #define PGT_writable_page PG_mask(1, 1) /* has writable mappings? >>> */ >>> #define PGT_type_mask PG_mask(1, 1) /* Bits 31 or 63. >>> */ >>> >>> - /* Count of uses of this frame as its current type. */ >>> -#define PGT_count_width PG_shift(2) >>> -#define PGT_count_mask ((1UL<<PGT_count_width)-1) >>> + /* 3-bit count of uses of this frame as its current type. */ >>> +#define PGT_count_base PG_shift(4) >>> +#define PGT_count_mask PG_mask(7, 4) >>> + >>> +/* >>> + * Stored in bits [27:0] or [59:0] GFN if page is used for grant table >>> frame. >> I don't know enough Arm details to tell whether this is properly >> one bit more than the maximum number of physical address bits. >> Without the extra bit you wouldn't be able to tell apart a >> guest specified GFN matching the value of PGT_INVALID_FRAME_GFN >> from an entry which was set from INVALID_GFN. > Really good point. > > 1. On Arm64 the p2m_ipa_bits could (theoretically) be 64-bit which, I > assume, corresponds to the maximum guest physical address (1 << 64) - 1 > = 0xFFFFFFFFFFFFFFFF. > To store that GFN we need 52-bit. But, we provide 60-bit field which is > more than enough, I think. Practically, the maximum supported > p2m_ipa_bits is 48-bit, so the maximum supported GFN will occupy 36-bit > only. Everything is ok here. > 2. On Arm32 the p2m_ipa_bits is 40-bit which, I assume, corresponds to > the maximum guest physical address (1 << 40) - 1 = 0xFFFFFFFFFF. To > store that GFN we need 28-bit. If I did the calculation correctly, what > we have on Arm32 is that PGT_INVALID_FRAME_GFN == maximum guest physical > address and it looks like we need and extra bit on Arm32. Do you think > we need to borrow one more bit from the count portion to stay on the > safe side?
I think so, unless there are further restrictions on the GFN range that I'm unaware of. For 64-bit, if you only need 52 bits, why do you make the field 60 bits wide? I'd recommend against "wasting" bits. Better keep the count field as wide as possible. >>> + * This only valid for the xenheap pages. >>> + */ >>> +#define PGT_gfn_width PG_shift(4) >>> +#define PGT_gfn_mask ((1UL<<PGT_gfn_width)-1) >> Any reason you don't use PG_mask() here? Any open-coding is prone >> to people later making mistakes. > I failed to come up with idea how to do that without #ifdef. As GFN > starts at bit 0 different first parameter would be needed for PG_mask on > 32-bit and 64-bit systems. > I wonder whether PGC_count_mask/PGT_count_mask are open-coded on Arm/x86 > because of the same reason. Hmm, that pre-existing open-coding isn't nice, but is perhaps a good enough reason to keep what you have. (Personally I wouldn't be afraid of adding an #ifdef here, but I realize views there may differ.) Jan
