Re: [Qemu-devel] [PATCH v6 07/11] dump: Add API to write dump_bitmap

[Laszlo Ersek]

On 01/07/14 15:49, Laszlo Ersek wrote:
> 
> On 01/05/14 08:27, Qiao Nuohan wrote:

>> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
>> index 9e47b4c..b5eaf8d 100644
>> --- a/include/sysemu/dump.h
>> +++ b/include/sysemu/dump.h
>> @@ -27,11 +27,18 @@
>>  #define DUMP_DH_COMPRESSED_LZO      (0x2)
>>  #define DUMP_DH_COMPRESSED_SNAPPY   (0x4)
>>  
>> +#define PAGE_SIZE                   (4096)
>>  #define KDUMP_SIGNATURE             "KDUMP   "
>>  #define SIG_LEN                     (sizeof(KDUMP_SIGNATURE) - 1)
>>  #define PHYS_BASE                   (0)
>>  #define DUMP_LEVEL                  (1)
>>  #define DISKDUMP_HEADER_BLOCKS      (1)
>> +#define BUFSIZE_BITMAP              (PAGE_SIZE)
>> +#define PFN_BUFBITMAP               (CHAR_BIT * BUFSIZE_BITMAP)
>> +#define ARCH_PFN_OFFSET             (0)
>> +
>> +#define paddr_to_pfn(X, page_shift) \
>> +    (((unsigned long long)(X) >> (page_shift)) - ARCH_PFN_OFFSET)
>>  
>>  typedef struct ArchDumpInfo {
>>      int d_machine;  /* Architecture */
>>
> 
> I think these magic constants are somewhat tied to x86, and therefore
> should be in an arch-specific file rather than a common file, but
> whoever wants to extend this to another architecture can do that.

Stressing the argument a bit more for PAGE_SIZE specifically:
- we already have TARGET_PAGE_SIZE, maybe that would be a better choice,
- PAGE_SIZE is defined *as* TARGET_PAGE_SIZE in kvm-all.c. There's no
actual conflict, but the mental conflict is bad enough.

Anyway my R-b stands.

Laszlo