Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
On Tue, Sep 23, 2008 at 10:35:24AM +0300, Avi Kivity wrote: > Glauber Costa wrote: >> On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote: >> >>> Glauber Costa wrote: >>> is_allocated_mem is a function that checks if every relevant aspect of the memory slot match (start and size). Replace it with a more generic function that checks if a memory region is totally contained into another. The former case is also covered. >>> I think enabling dirty page tracking requires the slot to match exactly. >>> >> >> The registering function was the only caller for that. As dirty tracking was >> happening >> _inside_ it, so we're not really losing anything here. >> >> That said, I believe in the future, it will. (would be a logical next step, >> and start >> addressing the problem that Jan raised). Maybe we can keep the function that >> checks for >> exact matching, but another alternative is to pass a phys_addr, and turn >> dirty tracking on/off >> for whatever slot that contains that phys_addr. What do you think? >> > > Exact matching would avoid surprises (catch errors early). Agreed, you're right. However, as I said, this is for future reference, because right now, we don't change slots properties via the memory registration infrastructure once it's in place. > > -- > error compiling committee.c: too many arguments to function > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
Glauber Costa wrote: On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote: Glauber Costa wrote: is_allocated_mem is a function that checks if every relevant aspect of the memory slot match (start and size). Replace it with a more generic function that checks if a memory region is totally contained into another. The former case is also covered. I think enabling dirty page tracking requires the slot to match exactly. The registering function was the only caller for that. As dirty tracking was happening _inside_ it, so we're not really losing anything here. That said, I believe in the future, it will. (would be a logical next step, and start addressing the problem that Jan raised). Maybe we can keep the function that checks for exact matching, but another alternative is to pass a phys_addr, and turn dirty tracking on/off for whatever slot that contains that phys_addr. What do you think? Exact matching would avoid surprises (catch errors early). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
On Sat, Sep 20, 2008 at 11:33:56AM -0700, Avi Kivity wrote: > Glauber Costa wrote: >> is_allocated_mem is a function that checks if every relevant aspect of the >> memory slot >> match (start and size). Replace it with a more generic function that checks >> if a memory >> region is totally contained into another. The former case is also covered. >> > > I think enabling dirty page tracking requires the slot to match exactly. The registering function was the only caller for that. As dirty tracking was happening _inside_ it, so we're not really losing anything here. That said, I believe in the future, it will. (would be a logical next step, and start addressing the problem that Jan raised). Maybe we can keep the function that checks for exact matching, but another alternative is to pass a phys_addr, and turn dirty tracking on/off for whatever slot that contains that phys_addr. What do you think? > > -- > I have a truly marvellous patch that fixes the bug which this > signature is too narrow to contain. > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
Glauber Costa wrote: is_allocated_mem is a function that checks if every relevant aspect of the memory slot match (start and size). Replace it with a more generic function that checks if a memory region is totally contained into another. The former case is also covered. I think enabling dirty page tracking requires the slot to match exactly. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
is_allocated_mem is a function that checks if every relevant aspect of the memory slot match (start and size). Replace it with a more generic function that checks if a memory region is totally contained into another. The former case is also covered. Signed-off-by: Glauber Costa <[EMAIL PROTECTED]> --- libkvm/libkvm.c | 34 +- libkvm/libkvm.h |2 +- qemu/qemu-kvm.c |4 ++-- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c index fa65c30..a5e20bb 100644 --- a/libkvm/libkvm.c +++ b/libkvm/libkvm.c @@ -136,6 +136,27 @@ int get_intersecting_slot(unsigned long phys_addr) return -1; } +/* Returns -1 if this slot is not totally contained on any other, + * and the number of the slot otherwise */ +int get_container_slot(uint64_t phys_addr, unsigned long size) +{ + int i; + + for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i) + if (slots[i].len && slots[i].phys_addr <= phys_addr && + (slots[i].phys_addr + slots[i].len) >= phys_addr + size) + return i; + return -1; +} + +int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size) +{ + int slot = get_container_slot(phys_addr, size); + if (slot == -1) + return 0; + return 1; +} + /* * dirty pages logging control */ @@ -423,19 +444,6 @@ int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start) return get_intersecting_slot(phys_start) != -1; } -int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start, -unsigned long len) -{ - int slot; - - slot = get_slot(phys_start); - if (slot == -1) - return 0; - if (slots[slot].len == len) - return 1; - return 0; -} - int kvm_register_phys_mem(kvm_context_t kvm, unsigned long phys_start, void *userspace_addr, unsigned long len, int log) diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h index 79dd769..1e89993 100644 --- a/libkvm/libkvm.h +++ b/libkvm/libkvm.h @@ -454,7 +454,7 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start, unsigned long len, int log, int writable); void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, unsigned long len); -int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start); +int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size); int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start, unsigned long len); int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start, diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c index cff04c5..e0b114a 100644 --- a/qemu/qemu-kvm.c +++ b/qemu/qemu-kvm.c @@ -778,10 +778,10 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr, int r = 0; phys_offset &= ~IO_MEM_ROM; -r = kvm_is_allocated_mem(kvm_context, start_addr, size); +r = kvm_is_containing_region(kvm_context, start_addr, size); if (r) return; -r = kvm_is_intersecting_mem(kvm_context, start_addr); +r = kvm_is_intersecting_mem(kvm_context, start_addr); if (r) { printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size); } else -- 1.5.5.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] substitute is_allocated_mem with more general is_containing_region
is_allocated_mem is a function that checks if every relevant aspect of the memory slot match (start and size). Replace it with a more generic function that checks if a memory region is totally contained into another. The former case is also covered. Signed-off-by: Glauber Costa <[EMAIL PROTECTED]> --- libkvm/libkvm.c | 34 +- libkvm/libkvm.h |2 +- qemu/qemu-kvm.c |4 ++-- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c index fa65c30..a5e20bb 100644 --- a/libkvm/libkvm.c +++ b/libkvm/libkvm.c @@ -136,6 +136,27 @@ int get_intersecting_slot(unsigned long phys_addr) return -1; } +/* Returns -1 if this slot is not totally contained on any other, + * and the number of the slot otherwise */ +int get_container_slot(uint64_t phys_addr, unsigned long size) +{ + int i; + + for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i) + if (slots[i].len && slots[i].phys_addr <= phys_addr && + (slots[i].phys_addr + slots[i].len) >= phys_addr + size) + return i; + return -1; +} + +int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size) +{ + int slot = get_container_slot(phys_addr, size); + if (slot == -1) + return 0; + return 1; +} + /* * dirty pages logging control */ @@ -423,19 +444,6 @@ int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start) return get_intersecting_slot(phys_start) != -1; } -int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start, -unsigned long len) -{ - int slot; - - slot = get_slot(phys_start); - if (slot == -1) - return 0; - if (slots[slot].len == len) - return 1; - return 0; -} - int kvm_register_phys_mem(kvm_context_t kvm, unsigned long phys_start, void *userspace_addr, unsigned long len, int log) diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h index 79dd769..1e89993 100644 --- a/libkvm/libkvm.h +++ b/libkvm/libkvm.h @@ -454,7 +454,7 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start, unsigned long len, int log, int writable); void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, unsigned long len); -int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start); +int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size); int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start, unsigned long len); int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start, diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c index cff04c5..e0b114a 100644 --- a/qemu/qemu-kvm.c +++ b/qemu/qemu-kvm.c @@ -778,10 +778,10 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr, int r = 0; phys_offset &= ~IO_MEM_ROM; -r = kvm_is_allocated_mem(kvm_context, start_addr, size); +r = kvm_is_containing_region(kvm_context, start_addr, size); if (r) return; -r = kvm_is_intersecting_mem(kvm_context, start_addr); +r = kvm_is_intersecting_mem(kvm_context, start_addr); if (r) { printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size); } else -- 1.5.5.1 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html