Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly. Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2 L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+ TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS 32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}
This looks pretty frightening. What is it needed for?
I think we should stick with range operations, but maybe I misunderstood
something here.
Otherwise, I like this patchset.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:
On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly. Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2 L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+ TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS 32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}
This looks pretty frightening. What is it needed for?
The point is that clients can be registered at any point.
A client that registered when memory is present needs to
be notified about it.
I think we should stick with range operations, but maybe I misunderstood
something here.
Otherwise, I like this patchset.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On 01/18/2010 03:52 PM, Michael S. Tsirkin wrote:
On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:
On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly. Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2 L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+ TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS 32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}
This looks pretty frightening. What is it needed for?
The point is that clients can be registered at any point.
A client that registered when memory is present needs to
be notified about it.
It looks very expensive. Maybe we mandate clients be registered at
init-time?
Long term we need to move to a range based memory description.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On Mon, Jan 18, 2010 at 03:58:51PM +0200, Avi Kivity wrote:
On 01/18/2010 03:52 PM, Michael S. Tsirkin wrote:
On Mon, Jan 18, 2010 at 03:02:59PM +0200, Avi Kivity wrote:
On 01/04/2010 09:49 PM, Michael S. Tsirkin wrote:
This adds notifiers for phys memory changes: a set of callbacks that
vhost can register and update kernel accordingly. Down the road, kvm
code can be switched to use these as well, instead of calling kvm code
directly from exec.c as is done now.
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+ CPUPhysMemoryClient *client)
+{
+PhysPageDesc *pd;
+int l1, l2;
+
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+pd = phys_map[l1];
+if (!pd) {
+continue;
+}
+for (l2 = 0; l2 L2_SIZE; ++l2) {
+if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+continue;
+}
+client-set_memory(client, pd[l2].region_offset,
+ TARGET_PAGE_SIZE, pd[l2].phys_offset);
+}
+}
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS 32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+void **phys_map = (void **)l1_phys_map;
+int l1;
+if (!l1_phys_map) {
+return;
+}
+for (l1 = 0; l1 L1_SIZE; ++l1) {
+if (phys_map[l1]) {
+phys_page_for_each_in_l1_map(phys_map[l1], client);
+}
+}
+#else
+if (!l1_phys_map) {
+return;
+}
+phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}
This looks pretty frightening. What is it needed for?
The point is that clients can be registered at any point.
A client that registered when memory is present needs to
be notified about it.
It looks very expensive.
Shouldn't be hard to optimize ...
Maybe we mandate clients be registered at init-time?
This might be tricky - vhost currently only registers when the
first device is hot-added.
Long term we need to move to a range based memory description.
--
error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote: The point is that clients can be registered at any point. A client that registered when memory is present needs to be notified about it. It looks very expensive. Shouldn't be hard to optimize ... It's O(memory size), which can be very big. Maybe we mandate clients be registered at init-time? This might be tricky - vhost currently only registers when the first device is hot-added. I see. Maybe coalesce adjacent pages and call the callback with the ranges? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On Mon, Jan 18, 2010 at 04:52:10PM +0200, Avi Kivity wrote: On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote: The point is that clients can be registered at any point. A client that registered when memory is present needs to be notified about it. It looks very expensive. Shouldn't be hard to optimize ... It's O(memory size), which can be very big. Maybe we mandate clients be registered at init-time? This might be tricky - vhost currently only registers when the first device is hot-added. I see. Maybe coalesce adjacent pages and call the callback with the ranges? Yes, I'll do that.
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On Mon, Jan 18, 2010 at 04:52:10PM +0200, Avi Kivity wrote: On 01/18/2010 04:44 PM, Michael S. Tsirkin wrote: The point is that clients can be registered at any point. A client that registered when memory is present needs to be notified about it. It looks very expensive. Shouldn't be hard to optimize ... It's O(memory size), which can be very big. cpu_register_physical_memory_offset already is O(memory size) btw. Maybe we mandate clients be registered at init-time? This might be tricky - vhost currently only registers when the first device is hot-added. I see. Maybe coalesce adjacent pages and call the callback with the ranges? Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ... Can't we just rely on callback to do the coalescing? -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On 01/18/2010 05:45 PM, Michael S. Tsirkin wrote: cpu_register_physical_memory_offset already is O(memory size) btw. Right, but we'd like to replace it with a range API. Maybe we mandate clients be registered at init-time? This might be tricky - vhost currently only registers when the first device is hot-added. I see. Maybe coalesce adjacent pages and call the callback with the ranges? Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ... Can't we just rely on callback to do the coalescing? If the callback can do the coalescing, surely the caller can as well? This way we don't introduce a new per-page API. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCHv2 1/3] qemu: memory notifiers
On Mon, Jan 18, 2010 at 06:04:40PM +0200, Avi Kivity wrote: On 01/18/2010 05:45 PM, Michael S. Tsirkin wrote: cpu_register_physical_memory_offset already is O(memory size) btw. Right, but we'd like to replace it with a range API. So, when we do the implementation of notifiers can follow? Maybe we mandate clients be registered at init-time? This might be tricky - vhost currently only registers when the first device is hot-added. I see. Maybe coalesce adjacent pages and call the callback with the ranges? Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ... Can't we just rely on callback to do the coalescing? If the callback can do the coalescing, surely the caller can as well? The callback calls qemu_ram_ptr and coalesces when the virtual memory pointers are matching. caller can do this as well but it looks ugly: we don't know this is what caller does ... This way we don't introduce a new per-page API. What do you mean by new per-page API? The API is range-based, implementation currently scans all pages. -- error compiling committee.c: too many arguments to function
