[Spice-devel] [PATCH spice-server] Use (u)intptr_t for virtual addresses
Frediano Ziglio
fziglio at redhat.com
Sun Aug 11 16:41:39 UTC 2019
> 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?
> > > 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/20190811/5920f3a8/attachment-0001.html>
More information about the Spice-devel
mailing list