On 22.03.2023 14:29, Julien Grall wrote: > On 22/03/2023 06:59, Jan Beulich wrote: >> On 21.03.2023 19:33, Ayan Kumar Halder wrote: >>> On 21/03/2023 16:53, Jan Beulich wrote: >>>> On 21.03.2023 17:15, Ayan Kumar Halder wrote: >>>>> On 21/03/2023 14:22, Jan Beulich wrote: >>>>>> (Using "unsigned long" for a 32-bit paddr_t is of >>>>>> course suspicious as well - this ought to be uint32_t.) >>>>> The problem with using uint32_t for paddr_t is that there are instances >>>>> where the paddr_t is modified with PAGE_MASK or PAGE_ALIGN. >>>>> >>>>> For eg , handle_passthrough_prop() >>>>> >>>>> printk(XENLOG_ERR "Unable to permit to dom%d access to" >>>>> " 0x%"PRIpaddr" - 0x%"PRIpaddr"\n", >>>>> kinfo->d->domain_id, >>>>> mstart & PAGE_MASK, PAGE_ALIGN(mstart + size) - 1); >>>>> >>>>> And in xen/include/xen/page-size.h, >>>>> >>>>> #define PAGE_SIZE (_AC(1,L) << PAGE_SHIFT) >>>>> #define PAGE_MASK (~(PAGE_SIZE-1)) >>>>> >>>>> Thus, the resulting types are unsigned long. This cannot be printed >>>>> using %u for PRIpaddr. >>>> Is there anything wrong with making PAGE_SIZE expand to (1 << PAGE_SHIFT) >>>> when physical addresses are only 32 bits wide? >>> >>> I don't have a strong objection except that this is similar to what >>> linux is doing today. >>> >>> https://elixir.bootlin.com/linux/latest/source/arch/arm/include/asm/page.h#L12 >>> >>>> >>>>> I remember some discussion (or comment) that the physical addresses >>>>> should be represented using 'unsigned long'. >>>> A reference would be helpful. >>> >>> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00305.html >> >> I see. I guess this will be okay as long as only 32-bit arches elect to >> use 32-bit physical addresses. Maybe there should be a BUILD_BUG_ON() >> somewhere, accompanied by a suitable comment? > > Hmmm... We definitely have 40-bits physical address space on Arm32. In > fact, my suggestion was not to define paddr_t as unsigned long for > everyone but only when PHYS_ADDR_T_32 is selected (AFAICT this is what > is done in this patch). > > This is to avoid having to add cast everywhere we are using PAGE_* on > paddr_t and print it. > > That said, I realize this means that for 64-bit, we would still use > 64-bit integer. I view it as a less a problem (at least on Arm) because > registers are always 64-bit. I am open to other suggestion.
It simply struck me as odd to use a 64-bit type for something that was explicitly said is only going to be 32 bits wide. I would therefore prefer if we could limit 32-bit paddr_t to 32-bit architectures for now, as expressed before when asking for a respective BUILD_BUG_ON(). Especially if, as intended, the type definition moves to xen/types.h (and hence isn't Arm-specific anymore). Jan
