Re: [Spice-devel] [PATCH spice-server] Use (u)intptr_t for virtual addresses

Sun, 11 Aug 2019 08:12:37 -0700 


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.



    Snir.

        Signed-off-by: Frediano Ziglio<[email protected]>
        ---
          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


_______________________________________________
Spice-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/spice-devel






















					Reply via email to