[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management |
Date: |
Wed, 29 Apr 2009 20:08:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 19:30 +0200, Jan Kiszka wrote:
>> Hollis Blanchard wrote:
>>> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>>>> Liu Yu-B13201 wrote:
>>>>>> -----Original Message-----
>>>>>> From: address@hidden
>>>>>> [mailto:address@hidden
>>>>>> On Behalf Of Jan Kiszka
>>>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>>>> To: address@hidden
>>>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to
>>>>>> slot management
>>>>>>
>>>>>> Fail loudly if we run out of memory slot.
>>>>>>
>>>>>> Make sure that dirty log start/stop works with consistent
>>>>>> memory regions
>>>>>> by reporting invalid parameters. This reveals several
>>>>>> inconsistencies in
>>>>>> the vga code, patch to fix them follows later in this series.
>>>>>>
>>>>>> And, for simplicity reasons, also catch and report unaligned memory
>>>>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
>>>>>>
>>>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
>>>>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic
>>>>> initializing mmio regions.
>>>> What is the alignment of those regions then? None? And do regions of
>>>> different types overlap even on the same page? Maybe the check reveals
>>>> some deeper conflict /wrt KVM. Can you point me to the involved code files?
>>> These PCI controllers make separate calls to
>>> cpu_register_physical_memory() for separate callbacks. Reading
>>> ppce500_pci_init(), for example:
>>> 0xe0008000 -> CFGADDR (4 bytes)
>>> 0xe0008004 -> CFGDATA (4 bytes)
>>> 0xe0008c00 -> other registers
>>>
>>> The loop in cpu_register_physical_memory_offset() handles "subpage"
>>> registration. However, kvm_set_phys_mem() is called outside that loop,
>>> so it gets the non-page-aligned addresses.
>>>
>> Half-blind shot:
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 32cd636..c2c760e 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
>> int err;
>>
>> if (start_addr & ~TARGET_PAGE_MASK) {
>> + if (flags >= IO_MEM_UNASSIGNED) {
>> + return;
>> + }
>> fprintf(stderr, "Only page-aligned memory slots supported\n");
>> abort();
>> }
>>
>> If it works, it likely needs a cleaner approach to handle all cases.
>
> I don't understand the point. kvm_set_phys_mem() already works without
> this new abort() check.
This new check is there to catch those cases where someone tries to
register regions that are actually incompatible with KVM. IO-MEM regions
do not belong into this category (unless they would split existing KVM
slots in a non-align way), and so the test likely overshoots here.
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 0/7] kvm: fix system reset & rework slot management, Anthony Liguori, 2009/04/17