[PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Jan Beulich]

--- 2010-01-06.orig/qemu/hw/xen_common.h	2009-01-27 08:32:14.000000000 +0100
+++ 2010-01-06/qemu/hw/xen_common.h	2010-01-08 17:19:07.000000000 +0100
@@ -30,5 +30,10 @@
 # define xen_rmb() rmb()
 # define xen_wmb() wmb()
 #endif
+#if __XEN_LATEST_INTERFACE_VERSION__ < 0x0003020a
+void * __attribute__((__weak__))
+xc_map_foreign_bulk(int xc_handle, uint32_t dom, int prot,
+                    const xen_pfn_t *, int *err, unsigned int num);
+#endif
 
 #endif /* QEMU_HW_XEN_COMMON_H */
--- 2010-01-06.orig/qemu/hw/xen_machine_fv.c	2009-05-19 16:24:10.000000000 +0200
+++ 2010-01-06/qemu/hw/xen_machine_fv.c	2010-01-08 17:24:10.000000000 +0100
@@ -109,6 +109,34 @@ static void qemu_remap_bucket(struct map
     for (i = 0; i < MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT; i++)
         pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
 
+    if (xc_map_foreign_bulk != NULL) {
+        int err[MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT];
+
+        vaddr_base = xc_map_foreign_bulk(xc_handle, domid,
+                                         PROT_READ|PROT_WRITE, pfns, err,
+                                         MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT);
+        if (vaddr_base == NULL) {
+            fprintf(logfile, "xc_map_foreign_bulk error %d\n", errno);
+            exit(-1);
+        }
+
+        entry->vaddr_base  = vaddr_base;
+        entry->paddr_index = address_index;
+
+        for (i = 0; i < MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT;
+             i += BITS_PER_LONG) {
+            unsigned long word = 0;
+            j = ((i + BITS_PER_LONG) > (MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT)) ?
+                (MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT) % BITS_PER_LONG :
+                BITS_PER_LONG;
+            while (j > 0)
+                word = (word << 1) | !err[i + --j];
+            entry->valid_mapping[i / BITS_PER_LONG] = word;
+        }
+
+        return;
+    }
+
     vaddr_base = xc_map_foreign_batch(xc_handle, domid, PROT_READ|PROT_WRITE,
                                       pfns, MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT);
     if (vaddr_base == NULL) {
@@ -347,11 +375,11 @@ static void xen_init_fv(ram_addr_t ram_s
                 (STORE_PAGE_START >> XC_PAGE_SHIFT); 
     }
 
-    phys_ram_base = xc_map_foreign_batch(xc_handle, domid,
+    phys_ram_base = xc_map_foreign_pages(xc_handle, domid,
                                          PROT_READ|PROT_WRITE,
                                          page_array, nr_pages);
     if (phys_ram_base == 0) {
-        fprintf(logfile, "xc_map_foreign_batch returned error %d\n", errno);
+        fprintf(logfile, "xc_map_foreign_pages returned error %d\n", errno);
         exit(-1);
     }
     free(page_array);

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Ian Jackson]

I don't have any objections to this in principle, but:

> +#if __XEN_LATEST_INTERFACE_VERSION__ < 0x0003020a
> +void * __attribute__((__weak__))
> +xc_map_foreign_bulk(int xc_handle, uint32_t dom, int prot,
> +                    const xen_pfn_t *, int *err, unsigned int num);
> +#endif

This is pretty horrible.

This should be done by using the #ifdef arround the call to
xc_map_foreign_bulk, not by having a weak symbol compared to NULL.

You might consider whether the ability to use the old call is intended
to last forever.  Probably not, soi you should mark it deprecated.

Jan Beulich writes ("[Xen-devel] [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl"):
> -    phys_ram_base = xc_map_foreign_batch(xc_handle, domid,
> +    phys_ram_base = xc_map_foreign_pages(xc_handle, domid,
>                                           PROT_READ|PROT_WRITE,
>                                           page_array, nr_pages);

I'm not sure I understand this bug I haven't looked at it in any kind
of detail so it may be correct.

Ian.

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Jan Beulich]

>>> Ian Jackson <Ian.Jackson@eu.citrix.com> 12.01.10 18:25 >>>
>> +#if __XEN_LATEST_INTERFACE_VERSION__ < 0x0003020a
>> +void * __attribute__((__weak__))
>> +xc_map_foreign_bulk(int xc_handle, uint32_t dom, int prot,
>> +                    const xen_pfn_t *, int *err, unsigned int num);
>> +#endif
>
>This is pretty horrible.
>
>This should be done by using the #ifdef arround the call to
>xc_map_foreign_bulk, not by having a weak symbol compared to NULL.

While indeed I wasn't sure about how (or if at all) to put in
backward compatibility, this seemed to be the consolidated place.

Using and #ifdef in the source file doesn't seem nice though, as it
would tie a qemu built against older headers to using the old
interface. But since I'm not sure about the compatibility needs in
the first place, I will listen to whatever you say you deem
appropriate.

>You might consider whether the ability to use the old call is intended
>to last forever.  Probably not, soi you should mark it deprecated.

It is being marked deprecated in a comment; using the respective
__attribute_((__deprecated__)) didn't seem a proper thing in a
header that ought to be consumable by non-GNU compilers.

>> -    phys_ram_base = xc_map_foreign_batch(xc_handle, domid,
>> +    phys_ram_base = xc_map_foreign_pages(xc_handle, domid,
>>                                           PROT_READ|PROT_WRITE,
>>                                           page_array, nr_pages);
>
>I'm not sure I understand this bug I haven't looked at it in any kind
>of detail so it may be correct.

This just follows the libxc changes: Any call to xc_map_foreign_batch()
where the array wasn't looked at for per-page errors just wasn't
correct, and now gets converted to use xc_map_foreign_pages()
instead (which does the error checking on behalf of the caller).

Jan

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Keir Fraser]

On 13/01/2010 07:50, "Jan Beulich" <JBeulich@novell.com> wrote:

>> This should be done by using the #ifdef arround the call to
>> xc_map_foreign_bulk, not by having a weak symbol compared to NULL.
> 
> While indeed I wasn't sure about how (or if at all) to put in
> backward compatibility, this seemed to be the consolidated place.
> 
> Using and #ifdef in the source file doesn't seem nice though, as it
> would tie a qemu built against older headers to using the old
> interface. But since I'm not sure about the compatibility needs in
> the first place, I will listen to whatever you say you deem
> appropriate.

Qemu is deemed part of the tollstack matched set, so compatibility with
older libxc is not an issue. The primary concern is to provide continued
compatibility with older dom0 kernels, which I believe you entirely handle
within libxc.

 -- Keir

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Jan Beulich]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 13.01.10 08:59 >>>
>Qemu is deemed part of the tollstack matched set, so compatibility with
>older libxc is not an issue. The primary concern is to provide continued
>compatibility with older dom0 kernels, which I believe you entirely handle
>within libxc.

Yes, that's correct. Should I re-spin the patch with the compatibility
handling entirely removed then?

Jan

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Keir Fraser]

On 13/01/2010 08:03, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> Keir Fraser <keir.fraser@eu.citrix.com> 13.01.10 08:59 >>>
>> Qemu is deemed part of the tollstack matched set, so compatibility with
>> older libxc is not an issue. The primary concern is to provide continued
>> compatibility with older dom0 kernels, which I believe you entirely handle
>> within libxc.
> 
> Yes, that's correct. Should I re-spin the patch with the compatibility
> handling entirely removed then?

Yes please. I've committed the other patches by the way (not yet pushed
though). Re-spin the qemu one and we'll get that in for -rc2 as well.

 Thanks,
 Keir

Re: [PATCH, RFC 3/4] qemu: use new (replacement) mmap-batch ioctl

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

>>> Keir Fraser <keir.fraser@eu.citrix.com> 13.01.10 09:28 >>>
>On 13/01/2010 08:03, "Jan Beulich" <JBeulich@novell.com> wrote:
>> Yes, that's correct. Should I re-spin the patch with the compatibility
>> handling entirely removed then?
>
>Yes please. I've committed the other patches by the way (not yet pushed
>though). Re-spin the qemu one and we'll get that in for -rc2 as well.

Here we go. Thanks!
Jan


[-- Attachment #2: xen-qemu-privcmd-mmap-new.patch --]
[-- Type: text/plain, Size: 2514 bytes --]

Subject: qemu: use new (replacement) mmap-batch ioctl

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- 2010-01-06.orig/qemu/hw/xen_machine_fv.c	2010-01-13 09:45:24.000000000 +0100
+++ 2010-01-06/qemu/hw/xen_machine_fv.c	2010-01-13 09:49:22.000000000 +0100
@@ -95,7 +95,8 @@ static void qemu_remap_bucket(struct map
                               unsigned long address_index)
 {
     uint8_t *vaddr_base;
-    unsigned long pfns[MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT];
+    xen_pfn_t pfns[MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT];
+    int err[MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT];
     unsigned int i, j;
 
     if (entry->vaddr_base != NULL) {
@@ -109,10 +110,11 @@ static void qemu_remap_bucket(struct map
     for (i = 0; i < MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT; i++)
         pfns[i] = (address_index << (MCACHE_BUCKET_SHIFT-XC_PAGE_SHIFT)) + i;
 
-    vaddr_base = xc_map_foreign_batch(xc_handle, domid, PROT_READ|PROT_WRITE,
-                                      pfns, MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT);
+    vaddr_base = xc_map_foreign_bulk(xc_handle, domid, PROT_READ|PROT_WRITE,
+                                     pfns, err,
+                                     MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT);
     if (vaddr_base == NULL) {
-        fprintf(logfile, "xc_map_foreign_batch error %d\n", errno);
+        fprintf(logfile, "xc_map_foreign_bulk error %d\n", errno);
         exit(-1);
     }
 
@@ -124,7 +126,7 @@ static void qemu_remap_bucket(struct map
         j = ((i + BITS_PER_LONG) > (MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT)) ?
             (MCACHE_BUCKET_SIZE >> XC_PAGE_SHIFT) % BITS_PER_LONG : BITS_PER_LONG;
         while (j > 0)
-            word = (word << 1) | (((pfns[i + --j] >> 28) & 0xf) != 0xf);
+            word = (word << 1) | !err[i + --j];
         entry->valid_mapping[i / BITS_PER_LONG] = word;
     }
 }
@@ -347,11 +349,11 @@ static void xen_init_fv(ram_addr_t ram_s
                 (STORE_PAGE_START >> XC_PAGE_SHIFT); 
     }
 
-    phys_ram_base = xc_map_foreign_batch(xc_handle, domid,
+    phys_ram_base = xc_map_foreign_pages(xc_handle, domid,
                                          PROT_READ|PROT_WRITE,
                                          page_array, nr_pages);
     if (phys_ram_base == 0) {
-        fprintf(logfile, "xc_map_foreign_batch returned error %d\n", errno);
+        fprintf(logfile, "xc_map_foreign_pages returned error %d\n", errno);
         exit(-1);
     }
     free(page_array);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel