On 04.04.2025 10:43, Oleksii Kurochko wrote: > > On 4/4/25 9:52 AM, Jan Beulich wrote: >> On 04.04.2025 09:31, Oleksii Kurochko wrote: >>> On 4/4/25 8:56 AM, Jan Beulich wrote: >>>> On 03.04.2025 18:20, Oleksii Kurochko wrote: >>>>> On 4/1/25 6:04 PM, Jan Beulich wrote: >>>>>> On 01.04.2025 17:58, Oleksii Kurochko wrote: >>>>>>> On 3/31/25 6:14 PM, Jan Beulich wrote: >>>>>>>> On 31.03.2025 17:20, Oleksii Kurochko wrote: >>>>>>>>> + _AC(XEN_VIRT_START, UL) >> vpn1_shift; >>>>>>>>> + const unsigned long xen_virt_end_vpn = >>>>>>>>> + xen_virt_starn_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1); >>>>>>>>> + >>>>>>>>> if ((va >= DIRECTMAP_VIRT_START) && >>>>>>>>> (va <= DIRECTMAP_VIRT_END)) >>>>>>>>> return directmapoff_to_maddr(va - directmap_virt_start); >>>>>>>>> >>>>>>>>> - BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2)); >>>>>>>>> - ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == >>>>>>>>> - (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + >>>>>>>>> PAGE_SHIFT))); >>>>>>>>> + BUILD_BUG_ON(XEN_VIRT_SIZE != MB(8)); >>>>>>>> Is it necessary to be != ? Won't > suffice? >>>>>>> It could be just > MB(2). Or perphaps >=. >>>>>>> = would make the build fail, wouldn't it? >>>>> I just realized that BUILD_BUG_ON() condition is compared to zero so >>>>> actually everything what >>>>> will make the condition true will cause a build fail as inside it used >>>>> !(condition). >>>> ??? >>> |BUILD_BUG_ON()| forces a compilation error if the given condition is true. >>> Therefore, if the condition >>> |XEN_VIRT_SIZE != MB(2)| is changed to|XEN_VIRT_SIZE > MB(2)|, the >>> condition will always evaluate to true >>> (assuming|XEN_VIRT_SIZE| is greater than 2 MB), which will result in a >>> compilation error. >> Well, it was you who used MB(2) in a reply, when previously talk was of >> MB(8), >> and that to grow to MB(16). The BUILD_BUG_ON() is - aiui - about you having >> set >> aside enough page table space. Quite possibly the need for this >> BUILD_BUG_ON() >> then disappears altogether when XEN_VIRT_SIZE is properly taken into account >> for the number-of-page-tables calculation. In no event do I see why the MB(2) >> boundary would be relevant for anything going forward. > > Also, doesn’t|BUILD_BUG_ON()| affect how the|ASSERT()| that follows it is > written? > > The changes, at the moment, look like: > + const unsigned int vpn1_shift = PAGETABLE_ORDER + PAGE_SHIFT; > + const unsigned long va_vpn = va >> vpn1_shift; > + const unsigned long xen_virt_start_vpn = > + _AC(XEN_VIRT_START, UL) >> vpn1_shift; > + const unsigned long xen_virt_end_vpn = > + xen_virt_start_vpn + ((XEN_VIRT_SIZE >> vpn1_shift) - 1); > + > if ((va >= DIRECTMAP_VIRT_START) && > (va <= DIRECTMAP_VIRT_END)) > return directmapoff_to_maddr(va - directmap_virt_start); > > - BUILD_BUG_ON(XEN_VIRT_SIZE != MB(2)); > - ASSERT((va >> (PAGETABLE_ORDER + PAGE_SHIFT)) == > - (_AC(XEN_VIRT_START, UL) >> (PAGETABLE_ORDER + PAGE_SHIFT))); > + BUILD_BUG_ON(XEN_VIRT_SIZE != MB(16)); > + ASSERT((va_vpn >= xen_virt_start_vpn) && (va_vpn <= xen_virt_end_vpn)); > > > If|XEN_VIRT_SIZE| is greater than|GB(1)|, then|xen_virt_end_vpn| may be > calculated > incorrectly. > > For example, if|XEN_VIRT_START| is|0xFFFFFFFF80000000| and|XEN_VIRT_SIZE| > is|0x40200000|, > then|(XEN_VIRT_SIZE >> vpn1_shift)| equals 513, whereas|va_vpn| is always in > the range [0, 511], > but xen_virt_end_vpn will be greater then 511. > > So shouldn't it be checked before ASSERT() that XEN_VIRT_SIZE is <= GB(1): > BUILD_BUG_ON(XEN_VIRT_SIZE <= GB(1))?
Yes, that would make sense. Jan
