[Spice-devel] [PATCH spice-server] Use (u)intptr_t for virtual addresses
Snir Sheriber
ssheribe at redhat.com
Mon Aug 12 06:34:15 UTC 2019
On 8/11/19 7:41 PM, Frediano Ziglio wrote:
>
>
> On 8/11/19 3:12 PM, Frediano Ziglio wrote:
>
>
>
> Hi
>
> On 7/23/19 11:22 AM, Frediano Ziglio wrote:
>
>
> When to Say "a" or "an" | Pronunciation |
> EnglishClub
>
> https://www.englishclub.com/pronunciation/a-an.htm
> On LLP64 platforms (like Windows) a virtual address cannot
> be represented by a "unsigned long" type, so use uintptr_t
> which is defined as a integral type large like a pointer.
>
>
> This sentence sounds a bit odd to me
>
> should be integer?
>
> C/C++ documentation classify these types as integral, not integer.
>
> also s/a/an
>
> fixed, thanks
>
>
>
> "address_delta" is a difference of pointers so use same
> type size.
>
>
> Not a big deal but wouldn't be preferable to be consistent
> with addr_delta?
>
> It sounds reasonable, although the value came from
> "addr_delta" field of QXLDevMemSlot
> which is part of SPICE ABI (so cannot be changed).
>
>
> Actually I thought to change address_delta to match everywhere to
> uint64_t of addr_delta
> can this delta be actually negative? seems not since it's coming
> from addr_delta,wouldn't be
> better to keep it unsigned
>
> Snir.
>
> The value is used only for addition and we support only two's
> complement architectures
> so no difference between signed or unsigned in this case (only
> additions means no extension,
> division or others).
> Moving to uin64_t instead of intptr_t or uintptr_t would cause 64 bit
> arithmetic even on
> 32 bit architectures for no reasons.
> Would be uintptr_t (all unsigned but 32 bit) fine?
Yes, sounds good.
Snir.
>
>
> Snir.
>
> Signed-off-by: Frediano Ziglio<fziglio at redhat.com>
> ---
> server/memslot.c | 22 ++++++++++++----------
> server/memslot.h | 20 ++++++++++----------
> server/red-parse-qxl.c | 2 +-
> server/spice-qxl.h | 4 ++--
> 4 files changed, 25 insertions(+), 23 deletions(-)
>
> diff --git a/server/memslot.c b/server/memslot.c
> index 2a1771b02..182d2b7e9 100644
> --- a/server/memslot.c
> +++ b/server/memslot.c
> @@ -21,7 +21,7 @@
>
> #include "memslot.h"
>
> -static unsigned long __get_clean_virt(RedMemSlotInfo *info, QXLPHYSICAL addr)
> +static uintptr_t __get_clean_virt(RedMemSlotInfo *info, QXLPHYSICAL addr)
> {
> return addr & info->memslot_clean_virt_mask;
> }
> @@ -37,7 +37,8 @@ static void print_memslots(RedMemSlotInfo *info)
> !info->mem_slots[i][x].virt_end_addr) {
> continue;
> }
> - printf("id %d, group %d, virt start %lx, virt end %lx, generation %u, delta %lx\n",
> + printf("id %d, group %d, virt start %" PRIxPTR ", virt end %" PRIxPTR ", generation %u,"
> + " delta %" PRIxPTR "\n",
> x, i, info->mem_slots[i][x].virt_start_addr,
> info->mem_slots[i][x].virt_end_addr, info->mem_slots[i][x].generation,
> info->mem_slots[i][x].address_delta);
> @@ -46,7 +47,7 @@ static void print_memslots(RedMemSlotInfo *info)
> }
>
> /* return 1 if validation successfull, 0 otherwise */
> -int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
> +int memslot_validate_virt(RedMemSlotInfo *info, uintptr_t virt, int slot_id,
> uint32_t add_size, uint32_t group_id)
> {
> MemSlot *slot;
> @@ -60,8 +61,9 @@ int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
> if (virt < slot->virt_start_addr || (virt + add_size) > slot->virt_end_addr) {
> print_memslots(info);
> spice_warning("virtual address out of range"
> - " virt=0x%lx+0x%x slot_id=%d group_id=%d\n"
> - " slot=0x%lx-0x%lx delta=0x%lx",
> + " virt=0x%" G_GINTPTR_MODIFIER "x+0x%x slot_id=%d group_id=%d\n"
> + " slot=0x%" G_GINTPTR_MODIFIER "x-0x%" G_GINTPTR_MODIFIER "x"
> + " delta=0x%" G_GINTPTR_MODIFIER "x",
> virt, add_size, slot_id, group_id,
> slot->virt_start_addr, slot->virt_end_addr, slot->address_delta);
> return 0;
> @@ -69,9 +71,9 @@ int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
> return 1;
> }
>
> -unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
> - unsigned long virt, int slot_id,
> - uint32_t group_id)
> +uintptr_t memslot_max_size_virt(RedMemSlotInfo *info,
> + uintptr_t virt, int slot_id,
> + uint32_t group_id)
> {
> MemSlot *slot;
>
> @@ -91,7 +93,7 @@ void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size
> {
> int slot_id;
> int generation;
> - unsigned long h_virt;
> + uintptr_t h_virt;
>
> MemSlot *slot;
>
> @@ -171,7 +173,7 @@ void memslot_info_destroy(RedMemSlotInfo *info)
> }
>
> void memslot_info_add_slot(RedMemSlotInfo *info, uint32_t slot_group_id, uint32_t slot_id,
> - uint64_t addr_delta, unsigned long virt_start, unsigned long virt_end,
> + uint64_t addr_delta, uintptr_t virt_start, uintptr_t virt_end,
> uint32_t generation)
> {
> spice_assert(info->num_memslots_groups > slot_group_id);
> diff --git a/server/memslot.h b/server/memslot.h
> index 00728c4b6..45381feb9 100644
> --- a/server/memslot.h
> +++ b/server/memslot.h
> @@ -25,9 +25,9 @@
>
> typedef struct MemSlot {
> int generation;
> - unsigned long virt_start_addr;
> - unsigned long virt_end_addr;
> - long address_delta;
> + uintptr_t virt_start_addr;
> + uintptr_t virt_end_addr;
> + intptr_t address_delta;
> } MemSlot;
>
> typedef struct RedMemSlotInfo {
> @@ -39,8 +39,8 @@ typedef struct RedMemSlotInfo {
> uint8_t memslot_id_shift;
> uint8_t memslot_gen_shift;
> uint8_t internal_groupslot_id;
> - unsigned long memslot_gen_mask;
> - unsigned long memslot_clean_virt_mask;
> + uintptr_t memslot_gen_mask;
> + uintptr_t memslot_clean_virt_mask;
> } RedMemSlotInfo;
>
> static inline int memslot_get_id(RedMemSlotInfo *info, uint64_t addr)
> @@ -53,11 +53,11 @@ static inline int memslot_get_generation(RedMemSlotInfo *info, uint64_t addr)
> return (addr >> info->memslot_gen_shift) & info->memslot_gen_mask;
> }
>
> -int memslot_validate_virt(RedMemSlotInfo *info, unsigned long virt, int slot_id,
> +int memslot_validate_virt(RedMemSlotInfo *info, uintptr_t virt, int slot_id,
> uint32_t add_size, uint32_t group_id);
> -unsigned long memslot_max_size_virt(RedMemSlotInfo *info,
> - unsigned long virt, int slot_id,
> - uint32_t group_id);
> +uintptr_t memslot_max_size_virt(RedMemSlotInfo *info,
> + uintptr_t virt, int slot_id,
> + uint32_t group_id);
> void *memslot_get_virt(RedMemSlotInfo *info, QXLPHYSICAL addr, uint32_t add_size,
> int group_id);
>
> @@ -68,7 +68,7 @@ void memslot_info_init(RedMemSlotInfo *info,
> uint8_t internal_groupslot_id);
> void memslot_info_destroy(RedMemSlotInfo *info);
> void memslot_info_add_slot(RedMemSlotInfo *info, uint32_t slot_group_id, uint32_t slot_id,
> - uint64_t addr_delta, unsigned long virt_start, unsigned long virt_end,
> + uint64_t addr_delta, uintptr_t virt_start, uintptr_t virt_end,
> uint32_t generation);
> void memslot_info_del_slot(RedMemSlotInfo *info, uint32_t slot_group_id, uint32_t slot_id);
> void memslot_info_reset(RedMemSlotInfo *info);
> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
> index eb2c0b538..01fd60580 100644
> --- a/server/red-parse-qxl.c
> +++ b/server/red-parse-qxl.c
> @@ -1335,7 +1335,7 @@ static bool red_get_message(QXLInstance *qxl_instance, RedMemSlotInfo *slots, in
> {
> QXLMessage *qxl;
> int memslot_id;
> - unsigned long len;
> + uintptr_t len;
> uint8_t *end;
>
> /*
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index 2f47910b9..5349d9275 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -187,8 +187,8 @@ struct QXLDevMemSlot {
> uint32_t slot_group_id;
> uint32_t slot_id;
> uint32_t generation;
> - unsigned long virt_start;
> - unsigned long virt_end;
> + uintptr_t virt_start;
> + uintptr_t virt_end;
> uint64_t addr_delta;
> uint32_t qxl_ram_size;
> };
>
> Frediano
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20190812/fcd8fc7e/attachment.html>
More information about the Spice-devel
mailing list