RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1029 bytes --]



> -----Original Message-----
> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> On Behalf Of Jan Kiszka
> Sent: Sunday, April 12, 2009 1:20 AM
> To: qemu-devel@nongnu.org
> 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.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¤¾oé¥ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Liu Yu-B13201 wrote:
> 
>> -----Original Message-----
>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>> On Behalf Of Jan Kiszka
>> Sent: Sunday, April 12, 2009 1:20 AM
>> To: qemu-devel@nongnu.org
>> 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?

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1800 bytes --]

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
> Sent: Wednesday, April 29, 2009 6:38 PM
> To: Liu Yu-B13201
> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
> to slot management
> 
> Liu Yu-B13201 wrote:
> > 
> >> -----Original Message-----
> >> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >> On Behalf Of Jan Kiszka
> >> Sent: Sunday, April 12, 2009 1:20 AM
> >> To: qemu-devel@nongnu.org
> >> 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?
I think so.

> Maybe the check reveals
> some deeper conflict /wrt KVM. Can you point me to the 
> involved code files?
> 

hw/ppc4xx_pci.c	ppc4xx_pci_init()
hw/ppce500_pci.c	ppce500_pci_init()
hw/openpic.c		mpic_init()
hw/ppce500_mpc8544ds.c    serial_mm_init()

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¤¾oé¥ÏâžØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&£ûàz¿äz¹Þ—ú+€Ê+zf£¢·hšˆ§~†­†Ûiÿÿïêÿ‘êçz_è®\x0fæj:+v‰¨þ)ߣøm

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Liu Yu-B13201 wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
>> Sent: Wednesday, April 29, 2009 6:38 PM
>> To: Liu Yu-B13201
>> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
>> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
>> to slot management
>>
>> Liu Yu-B13201 wrote:
>>>> -----Original Message-----
>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>> On Behalf Of Jan Kiszka
>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>> To: qemu-devel@nongnu.org
>>>> 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?
> I think so.
> 
>> Maybe the check reveals
>> some deeper conflict /wrt KVM. Can you point me to the 
>> involved code files?
>>
> 
> hw/ppc4xx_pci.c	ppc4xx_pci_init()
> hw/ppce500_pci.c	ppce500_pci_init()
> hw/openpic.c		mpic_init()
> hw/ppce500_mpc8544ds.c    serial_mm_init()
> 

Hmm, too bad that I have no platform at hand to test this. Could you
instrument kvm_set_phys_mem (on an older, working version), dumping its
arguments and post this trace?

TIA,
Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot

[Hollis Blanchard]

On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> Liu Yu-B13201 wrote:
> > 
> >> -----Original Message-----
> >> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >> On Behalf Of Jan Kiszka
> >> Sent: Sunday, April 12, 2009 1:20 AM
> >> To: qemu-devel@nongnu.org
> >> 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.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>> Liu Yu-B13201 wrote:
>>>> -----Original Message-----
>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>> On Behalf Of Jan Kiszka
>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>> To: qemu-devel@nongnu.org
>>>> 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.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot

[Hollis Blanchard]

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: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >>>> On Behalf Of Jan Kiszka
> >>>> Sent: Sunday, April 12, 2009 1:20 AM
> >>>> To: qemu-devel@nongnu.org
> >>>> 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.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

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: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>>>> On Behalf Of Jan Kiszka
>>>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>>>> To: qemu-devel@nongnu.org
>>>>>> 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 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
> Sent: Thursday, April 30, 2009 1:31 AM
> To: Hollis Blanchard
> Cc: Liu Yu-B13201; qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
> to slot management
> 
> Hollis Blanchard wrote:
> > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> >> Liu Yu-B13201 wrote:
> >>>> -----Original Message-----
> >>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >>>> On Behalf Of Jan Kiszka
> >>>> Sent: Sunday, April 12, 2009 1:20 AM
> >>>> To: qemu-devel@nongnu.org
> >>>> 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.
> 

It works for me.

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Anthony Liguori]

Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>   
>> 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
>   

That's goofy.  If the single device owns the entire region, it should 
region the entire region instead of relying on subpage functionality.

If just requires a switch() on the address to dispatch to the 
appropriate functions.  It should be easy enough to fix.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot

[Hollis Blanchard]

On Wed, 2009-04-29 at 12:38 -0500, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> >   
> >> 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
> >   
> 
> That's goofy.  If the single device owns the entire region, it should 
> region the entire region instead of relying on subpage functionality.
> 
> If just requires a switch() on the address to dispatch to the 
> appropriate functions.  It should be easy enough to fix.

There are two cases that share this code path:
1) same driver registers multiple regions in the same page
2) different drivers register regions in the same page

This is case 1, and as you say, we could add a switch statement to
handle it. I did not look closely to see how many other callers fall
into this category.

However, are you suggesting that case 2 is also "goofy" and will never
work with KVM? It works in qemu today. As long as case 2 works, case 1
will work too, so why change anything?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot

[Blue Swirl]

On 4/29/09, Hollis Blanchard <hollisb@us.ibm.com> wrote:
> On Wed, 2009-04-29 at 12:38 -0500, Anthony Liguori wrote:
>  > Hollis Blanchard wrote:
>  > > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>  > >
>  > >> 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
>  > >
>  >
>  > That's goofy.  If the single device owns the entire region, it should
>  > region the entire region instead of relying on subpage functionality.
>  >
>  > If just requires a switch() on the address to dispatch to the
>  > appropriate functions.  It should be easy enough to fix.
>
>  There are two cases that share this code path:
>  1) same driver registers multiple regions in the same page
>  2) different drivers register regions in the same page
>
>  This is case 1, and as you say, we could add a switch statement to
>  handle it. I did not look closely to see how many other callers fall
>  into this category.
>
>  However, are you suggesting that case 2 is also "goofy" and will never
>  work with KVM? It works in qemu today. As long as case 2 works, case 1
>  will work too, so why change anything?

I don't see why it would be wrong to register multiple regions within
the same page. It means that you can catch accesses to unassigned
addresses between the regions.

There are two instances of Sparc32 DMA controller, one to serve ESP
and the other for Lance. These are at addresses dma_base and dma_base
+ 16. Before subpage, this was handled with a switch, but now we rely
on the subpage mechanism instead.