Re: [PATCH v1] xen/riscv: Increase XEN_VIRT_SIZE

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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))?

~ Oleksii