Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-26 Thread Stefano Stabellini 
On Sun, 25 Mar 2012, Avi Kivity wrote:
 On 03/23/2012 06:37 PM, Jan Kiszka wrote:
  On 2012-03-23 16:08, Julien Grall wrote:
   On 03/22/2012 05:44 PM, Jan Kiszka wrote:
  
 static void core_region_nop(MemoryListener *listener,
   diff --git a/ioport.c b/ioport.c
   index 78a3b89..073ed75 100644
   --- a/ioport.c
   +++ b/ioport.c
   @@ -28,6 +28,7 @@
 #include ioport.h
 #include trace.h
 #include memory.h
   +#include hw/xen.h
  
 /***/
 /* IO Port */
   @@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
   length, int size,
  i);
 ioport_opaque[i] = opaque;
 }
   +
   +if (xen_enabled()) {
   +xen_map_iorange(start, length, 0);
   +}
   +
 return 0;
 }
  
   @@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
   length, int size,
  i);
 ioport_opaque[i] = opaque;
 }
   +
   +if (xen_enabled()) {
   +xen_map_iorange(start, length, 0);
   +}
   +
 return 0;
   +
 }
  
 static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
   @@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
   length)
 ioport_destructor_table[start](ioport_opaque[start]);
 ioport_destructor_table[start] = NULL;
 }
   +
   +if (xen_enabled()) {
   +xen_unmap_iorange(start, length, 0);
   +}
   +
 for(i = start; i  start + length; i++) {
 ioport_read_table[0][i] = NULL;
 ioport_read_table[1][i] = NULL;

   memory_listener_register(xen_hooks, system_io)?
  
   QEMU doesn't seem to call region_add/region_del for ioport.
   Moreover, some of ioport are directly register without
   using memory hook (for example cirrus vga).
   
   What is the best way to do it ?
 
  I haven't looked at details. Maybe it is just a combination of use case
  not yet considered, but can easily be added and need to switch legacy
  code to new scheme. Then this still remains the better option than this
  hook. Avi?
 
 Just the second - region_add/del will be called, but only for ioports
 registered via the MemoryRegion APIs.

It looks like there are quite a few register_ioport_read/write left
around, especially in the following files:

hw/acpi_piix4.c
hw/cirrus_vga.c
hw/serial.c
hw/pckbd.c
hw/pc.c

I guess they should all be converted to memory_region_init_io, right?

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-26 Thread Avi Kivity 
On 03/26/2012 01:01 PM, Stefano Stabellini wrote:
 On Sun, 25 Mar 2012, Avi Kivity wrote:
  On 03/23/2012 06:37 PM, Jan Kiszka wrote:
   On 2012-03-23 16:08, Julien Grall wrote:
On 03/22/2012 05:44 PM, Jan Kiszka wrote:
   
  static void core_region_nop(MemoryListener *listener,
diff --git a/ioport.c b/ioport.c
index 78a3b89..073ed75 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
  #include ioport.h
  #include trace.h
  #include memory.h
+#include hw/xen.h
   
  /***/
  /* IO Port */
@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
length, int size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
  }
   
@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
length, int size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
+
  }
   
  static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
length)
  ioport_destructor_table[start](ioport_opaque[start]);
  ioport_destructor_table[start] = NULL;
  }
+
+if (xen_enabled()) {
+xen_unmap_iorange(start, length, 0);
+}
+
  for(i = start; i  start + length; i++) {
  ioport_read_table[0][i] = NULL;
  ioport_read_table[1][i] = NULL;
 
memory_listener_register(xen_hooks, system_io)?
   
QEMU doesn't seem to call region_add/region_del for ioport.
Moreover, some of ioport are directly register without
using memory hook (for example cirrus vga).

What is the best way to do it ?
  
   I haven't looked at details. Maybe it is just a combination of use case
   not yet considered, but can easily be added and need to switch legacy
   code to new scheme. Then this still remains the better option than this
   hook. Avi?
  
  Just the second - region_add/del will be called, but only for ioports
  registered via the MemoryRegion APIs.

 It looks like there are quite a few register_ioport_read/write left
 around, especially in the following files:

 hw/acpi_piix4.c
 hw/cirrus_vga.c
 hw/serial.c
 hw/pckbd.c
 hw/pc.c

 I guess they should all be converted to memory_region_init_io, right?

Right.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-26 Thread Julien Grall 

On 03/26/2012 12:02 PM, Avi Kivity wrote:

On 03/26/2012 01:01 PM, Stefano Stabellini wrote:
   

On Sun, 25 Mar 2012, Avi Kivity wrote:
 

On 03/23/2012 06:37 PM, Jan Kiszka wrote:
   

On 2012-03-23 16:08, Julien Grall wrote:
 

On 03/22/2012 05:44 PM, Jan Kiszka wrote:
   

   static void core_region_nop(MemoryListener *listener,
diff --git a/ioport.c b/ioport.c
index 78a3b89..073ed75 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
   #include ioport.h
   #include trace.h
   #include memory.h
+#include hw/xen.h

   /***/
   /* IO Port */
@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
length, int size,
i);
   ioport_opaque[i] = opaque;
   }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
   return 0;
   }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
length, int size,
i);
   ioport_opaque[i] = opaque;
   }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
   return 0;
+
   }

   static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
length)
   ioport_destructor_table[start](ioport_opaque[start]);
   ioport_destructor_table[start] = NULL;
   }
+
+if (xen_enabled()) {
+xen_unmap_iorange(start, length, 0);
+}
+
   for(i = start; i   start + length; i++) {
   ioport_read_table[0][i] = NULL;
   ioport_read_table[1][i] = NULL;

   

memory_listener_register(xen_hooks, system_io)?

 

QEMU doesn't seem to call region_add/region_del for ioport.
Moreover, some of ioport are directly register without
using memory hook (for example cirrus vga).

What is the best way to do it ?
   

I haven't looked at details. Maybe it is just a combination of use case
not yet considered, but can easily be added and need to switch legacy
code to new scheme. Then this still remains the better option than this
hook. Avi?
 

Just the second - region_add/del will be called, but only for ioports
registered via the MemoryRegion APIs.
   

It looks like there are quite a few register_ioport_read/write left
around, especially in the following files:

hw/acpi_piix4.c
hw/cirrus_vga.c
hw/serial.c
hw/pckbd.c
hw/pc.c

I guess they should all be converted to memory_region_init_io, right?
 

Right

I will modify theses files and send a different patch series.

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-26 Thread Avi Kivity 
On 03/26/2012 01:24 PM, Julien Grall wrote:
 It looks like there are quite a few register_ioport_read/write left
 around, especially in the following files:

 hw/acpi_piix4.c
 hw/cirrus_vga.c
 hw/serial.c
 hw/pckbd.c
 hw/pc.c

 I guess they should all be converted to memory_region_init_io, right?
  
 Right
I will modify theses files and send a different patch series.


Great.  Please post them as a separate series, they can go in relatively
quickly since they should be mostly straighforward.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-25 Thread Avi Kivity 
On 03/23/2012 06:37 PM, Jan Kiszka wrote:
 On 2012-03-23 16:08, Julien Grall wrote:
  On 03/22/2012 05:44 PM, Jan Kiszka wrote:
 
static void core_region_nop(MemoryListener *listener,
  diff --git a/ioport.c b/ioport.c
  index 78a3b89..073ed75 100644
  --- a/ioport.c
  +++ b/ioport.c
  @@ -28,6 +28,7 @@
#include ioport.h
#include trace.h
#include memory.h
  +#include hw/xen.h
 
/***/
/* IO Port */
  @@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
  length, int size,
 i);
ioport_opaque[i] = opaque;
}
  +
  +if (xen_enabled()) {
  +xen_map_iorange(start, length, 0);
  +}
  +
return 0;
}
 
  @@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
  length, int size,
 i);
ioport_opaque[i] = opaque;
}
  +
  +if (xen_enabled()) {
  +xen_map_iorange(start, length, 0);
  +}
  +
return 0;
  +
}
 
static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
  @@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
  length)
ioport_destructor_table[start](ioport_opaque[start]);
ioport_destructor_table[start] = NULL;
}
  +
  +if (xen_enabled()) {
  +xen_unmap_iorange(start, length, 0);
  +}
  +
for(i = start; i  start + length; i++) {
ioport_read_table[0][i] = NULL;
ioport_read_table[1][i] = NULL;
   
  memory_listener_register(xen_hooks, system_io)?
 
  QEMU doesn't seem to call region_add/region_del for ioport.
  Moreover, some of ioport are directly register without
  using memory hook (for example cirrus vga).
  
  What is the best way to do it ?

 I haven't looked at details. Maybe it is just a combination of use case
 not yet considered, but can easily be added and need to switch legacy
 code to new scheme. Then this still remains the better option than this
 hook. Avi?

Just the second - region_add/del will be called, but only for ioports
registered via the MemoryRegion APIs.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-23 Thread Julien Grall 

On 03/22/2012 05:44 PM, Jan Kiszka wrote:


  static void core_region_nop(MemoryListener *listener,
diff --git a/ioport.c b/ioport.c
index 78a3b89..073ed75 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
  #include ioport.h
  #include trace.h
  #include memory.h
+#include hw/xen.h

  /***/
  /* IO Port */
@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, int 
size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, 
int size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
+
  }

  static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
@@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int length)
  ioport_destructor_table[start](ioport_opaque[start]);
  ioport_destructor_table[start] = NULL;
  }
+
+if (xen_enabled()) {
+xen_unmap_iorange(start, length, 0);
+}
+
  for(i = start; i  start + length; i++) {
  ioport_read_table[0][i] = NULL;
  ioport_read_table[1][i] = NULL;
 

memory_listener_register(xen_hooks, system_io)?
   

QEMU doesn't seem to call region_add/region_del for ioport.
Moreover, some of ioport are directly register without
using memory hook (for example cirrus vga).

What is the best way to do it ?


Even if that is not yet powerful enough, tuning the hooks is usually
better than open-coding.

Jan

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-23 Thread Jan Kiszka 
On 2012-03-23 16:08, Julien Grall wrote:
 On 03/22/2012 05:44 PM, Jan Kiszka wrote:

   static void core_region_nop(MemoryListener *listener,
 diff --git a/ioport.c b/ioport.c
 index 78a3b89..073ed75 100644
 --- a/ioport.c
 +++ b/ioport.c
 @@ -28,6 +28,7 @@
   #include ioport.h
   #include trace.h
   #include memory.h
 +#include hw/xen.h

   /***/
   /* IO Port */
 @@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int
 length, int size,
i);
   ioport_opaque[i] = opaque;
   }
 +
 +if (xen_enabled()) {
 +xen_map_iorange(start, length, 0);
 +}
 +
   return 0;
   }

 @@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int
 length, int size,
i);
   ioport_opaque[i] = opaque;
   }
 +
 +if (xen_enabled()) {
 +xen_map_iorange(start, length, 0);
 +}
 +
   return 0;
 +
   }

   static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
 @@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int
 length)
   ioport_destructor_table[start](ioport_opaque[start]);
   ioport_destructor_table[start] = NULL;
   }
 +
 +if (xen_enabled()) {
 +xen_unmap_iorange(start, length, 0);
 +}
 +
   for(i = start; i  start + length; i++) {
   ioport_read_table[0][i] = NULL;
   ioport_read_table[1][i] = NULL;
  
 memory_listener_register(xen_hooks, system_io)?

 QEMU doesn't seem to call region_add/region_del for ioport.
 Moreover, some of ioport are directly register without
 using memory hook (for example cirrus vga).
 
 What is the best way to do it ?

I haven't looked at details. Maybe it is just a combination of use case
not yet considered, but can easily be added and need to switch legacy
code to new scheme. Then this still remains the better option than this
hook. Avi?

Jan



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-23 Thread Anthony Liguori 

On 03/22/2012 11:01 AM, Julien Grall wrote:

QEMU will now register all memory range (PIO and MMIO) in Xen.
We distinct two phases in memory registered :
   - initialization
   - running

For all range registered during the initialization, QEMU will
check with XenStore if it is authorized to use them.
After the initialization, QEMU can register all range. Indeed,
the new ranges will be for PCI Bar.

Signed-off-by: Julien Gralljulien.gr...@citrix.com
---
  exec.c|9 ++
  ioport.c  |   17 
  xen-all.c |   83 +
  3 files changed, 109 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 780f63f..42d8c56 100644
--- a/exec.c
+++ b/exec.c
@@ -3557,12 +3557,21 @@ static void core_commit(MemoryListener *listener)
  static void core_region_add(MemoryListener *listener,
  MemoryRegionSection *section)
  {
+if (xen_enabled()) {
+   xen_map_iorange(section-offset_within_address_space,
+   section-size, 1);
+}
+
  cpu_register_physical_memory_log(section, section-readonly);
  }

  static void core_region_del(MemoryListener *listener,
  MemoryRegionSection *section)
  {
+if (xen_enabled()) {
+   xen_unmap_iorange(section-offset_within_address_space,
+   section-size, 1);
+}
  }

  static void core_region_nop(MemoryListener *listener,
diff --git a/ioport.c b/ioport.c
index 78a3b89..073ed75 100644
--- a/ioport.c
+++ b/ioport.c
@@ -28,6 +28,7 @@
  #include ioport.h
  #include trace.h
  #include memory.h
+#include hw/xen.h

  /***/
  /* IO Port */
@@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, int 
size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
  }

@@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, 
int size,
   i);
  ioport_opaque[i] = opaque;
  }
+
+if (xen_enabled()) {
+xen_map_iorange(start, length, 0);
+}
+
  return 0;
+
  }



This is the opposite direction we need to head.

I really don't think this series is the right way to handle things.  I don't 
want to see random hooks throughout QEMU to intercept for APIs affectively 
disabling large chunks of QEMU in the process.


You should look at (1) creating only the devices you want (2) use a clean 
interface to interact with those devices.


That would mean having a Xen specific AddressSpaceOps for ioports or something 
like that.  Not having hooks in areas of code like this.


Regards,

Anthony Liguori

Re: [Qemu-devel] [QEMU][RFC PATCH 3/6] memory: Add xen memory hook

2012-03-22 Thread Jan Kiszka 
On 2012-03-22 17:01, Julien Grall wrote:
 QEMU will now register all memory range (PIO and MMIO) in Xen.
 We distinct two phases in memory registered :
   - initialization
   - running
 
 For all range registered during the initialization, QEMU will
 check with XenStore if it is authorized to use them.
 After the initialization, QEMU can register all range. Indeed,
 the new ranges will be for PCI Bar.
 
 Signed-off-by: Julien Grall julien.gr...@citrix.com
 ---
  exec.c|9 ++
  ioport.c  |   17 
  xen-all.c |   83 
 +
  3 files changed, 109 insertions(+), 0 deletions(-)
 
 diff --git a/exec.c b/exec.c
 index 780f63f..42d8c56 100644
 --- a/exec.c
 +++ b/exec.c
 @@ -3557,12 +3557,21 @@ static void core_commit(MemoryListener *listener)
  static void core_region_add(MemoryListener *listener,
  MemoryRegionSection *section)
  {
 +if (xen_enabled()) {
 +   xen_map_iorange(section-offset_within_address_space,
 +   section-size, 1);
 +}
 +
  cpu_register_physical_memory_log(section, section-readonly);
  }
  
  static void core_region_del(MemoryListener *listener,
  MemoryRegionSection *section)
  {
 +if (xen_enabled()) {
 +   xen_unmap_iorange(section-offset_within_address_space,
 +   section-size, 1);
 +}
  }

memory_listener_register(xen_io_hooks, system_memory)?

  
  static void core_region_nop(MemoryListener *listener,
 diff --git a/ioport.c b/ioport.c
 index 78a3b89..073ed75 100644
 --- a/ioport.c
 +++ b/ioport.c
 @@ -28,6 +28,7 @@
  #include ioport.h
  #include trace.h
  #include memory.h
 +#include hw/xen.h
  
  /***/
  /* IO Port */
 @@ -155,6 +156,11 @@ int register_ioport_read(pio_addr_t start, int length, 
 int size,
   i);
  ioport_opaque[i] = opaque;
  }
 +
 +if (xen_enabled()) {
 +xen_map_iorange(start, length, 0);
 +}
 +
  return 0;
  }
  
 @@ -175,7 +181,13 @@ int register_ioport_write(pio_addr_t start, int length, 
 int size,
   i);
  ioport_opaque[i] = opaque;
  }
 +
 +if (xen_enabled()) {
 +xen_map_iorange(start, length, 0);
 +}
 +
  return 0;
 +
  }
  
  static uint32_t ioport_readb_thunk(void *opaque, uint32_t addr)
 @@ -260,6 +272,11 @@ void isa_unassign_ioport(pio_addr_t start, int length)
  ioport_destructor_table[start](ioport_opaque[start]);
  ioport_destructor_table[start] = NULL;
  }
 +
 +if (xen_enabled()) {
 +xen_unmap_iorange(start, length, 0);
 +}
 +
  for(i = start; i  start + length; i++) {
  ioport_read_table[0][i] = NULL;
  ioport_read_table[1][i] = NULL;

memory_listener_register(xen_hooks, system_io)?

Even if that is not yet powerful enough, tuning the hooks is usually
better than open-coding.

Jan



signature.asc
Description: OpenPGP digital signature