On 11.04.2024 01:47, Stefano Stabellini wrote:
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -415,7 +415,7 @@ struct hvm_hw_pci_irqs {
> * Indexed by: device*4 + INTx#.
> */
> union {
> - unsigned long i[16 / sizeof (unsigned long)]; /* DECLARE_BITMAP(i,
> 32*4); */
> + xen_ulong_t i[16 / sizeof (xen_ulong_t)]; /* DECLARE_BITMAP(i,
> 32*4); */
> uint64_t pad[2];
> };
> };
> @@ -428,7 +428,7 @@ struct hvm_hw_isa_irqs {
> * Indexed by ISA IRQ (assumes no ISA-device IRQ sharing).
> */
> union {
> - unsigned long i[1]; /* DECLARE_BITMAP(i, 16); */
> + xen_ulong_t i[1]; /* DECLARE_BITMAP(i, 16); */
> uint64_t pad[1];
> };
> };
Taking just this as example: Why is this not part of patch 3? If there was
"x86" somewhere in the title, I could probably see a reason for such
separation.
> --- a/xen/include/public/arch-x86/xen-x86_32.h
> +++ b/xen/include/public/arch-x86/xen-x86_32.h
> @@ -68,7 +68,7 @@
> #define MACH2PHYS_VIRT_END xen_mk_ulong(__MACH2PHYS_VIRT_END)
> #define MACH2PHYS_NR_ENTRIES ((MACH2PHYS_VIRT_END-MACH2PHYS_VIRT_START)>>2)
> #ifndef machine_to_phys_mapping
> -#define machine_to_phys_mapping ((unsigned long *)MACH2PHYS_VIRT_START)
> +#define machine_to_phys_mapping ((xen_ulong_t *)MACH2PHYS_VIRT_START)
While making this (and the corresponding 64-bit) adjustment, we ought to
at least consider to also add const. The overall change is at risk of being
disruptive to consumers anyway, in case they use less lax compilers than
e.g. gcc and Clang are: "unsigned int" and "uint32_t", even if both 32 bits
wide, don't necessarily need to be treated as the same type by the compiler.
In fact portability of code would be increased if compilers insisted on
clearly distinguishing the two.
In fact because of this it may want considering to split the conversions of
"long" from those of "int", just to cover the remote possibility of the
latter needing reverting from some reason.
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -141,7 +141,7 @@ struct trap_info {
> uint8_t vector; /* exception vector
> */
> uint8_t flags; /* 0-3: privilege level; 4: clear event enable?
> */
> uint16_t cs; /* code selector
> */
> - unsigned long address; /* code offset
> */
> + xen_ulong_t address; /* code offset
> */
> };
Nit: Please add two padding blanks to retain columnar arrangement of field
names.
> @@ -174,36 +174,36 @@ struct vcpu_guest_context {
> #define VGCF_syscall_disables_events (1<<_VGCF_syscall_disables_events)
> #define _VGCF_online 5
> #define VGCF_online (1<<_VGCF_online)
> - unsigned long flags; /* VGCF_* flags
> */
> - struct cpu_user_regs user_regs; /* User-level CPU registers
> */
> - struct trap_info trap_ctxt[256]; /* Virtual IDT
> */
> - unsigned long ldt_base, ldt_ents; /* LDT (linear address, # ents)
> */
> - unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, # ents)
> */
> - unsigned long kernel_ss, kernel_sp; /* Virtual TSS (only SS1/SP1)
> */
> + xen_ulong_t flags; /* VGCF_* flags */
> + struct cpu_user_regs user_regs; /* User-level CPU registers */
> + struct trap_info trap_ctxt[256]; /* Virtual IDT */
Please can these two lines remain untouched, and other lines instead have
padding blanks added to keep comments aligned?
> @@ -228,7 +228,7 @@ struct arch_shared_info {
> * is to be used.
> */
> xen_pfn_t pfn_to_mfn_frame_list_list;
> - unsigned long nmi_reason;
> + xen_ulong_t nmi_reason;
Nit: Here there again want to be two padding blanks.
Jan
