Xen devel - Aug 2012 - [PATCH V2] libxl/xl: implement support for guest iooprt and irq permissions

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1346428441 -3600
# Node ID ddde6c2c45de8e60518aafa077f0f3867ff68e17
# Parent  ccbee5bcb31b72706497725381f4e6836b9df657
libxl/xl: implement support for guest iooprt and irq permissions.

This is useful for passing legacy ISA devices (e.g. com ports,
parallel ports) to guests.

Supported syntax is as described in
http://cmrg.fifthhorseman.net/wiki/xen#grantingaccesstoserialhardwaretoadomU

I tested this using Xen''s ''q'' key handler which
prints out the I/O
port and IRQ ranges allowed for each domain. e.g.:

(XEN) Rangesets belonging to domain 31:
(XEN)     I/O Ports  { 2e8-2ef, 2f8-2ff }
(XEN)     Interrupts { 3, 5-6 }

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
Handle additional error conditions:
- (*ep!=0 && *ep!=''-'')
- *ep2!=0
- calloc failure
- values which are too big for a uint32_t
s/parsion/parsing/

diff -r ccbee5bcb31b -r ddde6c2c45de docs/man/xl.cfg.pod.5
--- a/docs/man/xl.cfg.pod.5	Fri Aug 31 12:03:55 2012 +0100
+++ b/docs/man/xl.cfg.pod.5	Fri Aug 31 16:54:01 2012 +0100
@@ -402,6 +402,30 @@ for more information on the "permissive"
 
 =back
 
+=item B<ioports=[ "IOPORT_RANGE", "IOPORT_RANGE", ...
]>
+
+=over 4
+
+Allow guest to access specific legacy I/O ports. Each B<IOPORT_RANGE>
+is given in hexadecimal and may either a span e.g. C<2f8-2ff>
+(inclusive) or a single I/O port C<2f8>.
+
+It is recommended to use this option only for trusted VMs under
+administrator control.
+
+=back
+
+=item B<irqs=[ NUMBER, NUMBER, ... ]>
+
+=over 4
+
+Allow a guest to access specific physical IRQs.
+
+It is recommended to use this option only for trusted VMs under
+administrator control.
+
+=back
+
 =head2 Paravirtualised (PV) Guest Specific Options
 
 The following options apply only to Paravirtual guests.
diff -r ccbee5bcb31b -r ddde6c2c45de tools/libxl/libxl_create.c
--- a/tools/libxl/libxl_create.c	Fri Aug 31 12:03:55 2012 +0100
+++ b/tools/libxl/libxl_create.c	Fri Aug 31 16:54:01 2012 +0100
@@ -933,6 +933,36 @@ static void domcreate_launch_dm(libxl__e
         LOG(ERROR, "unable to add disk devices");
         goto error_out;
     }
+
+    for (i = 0; i < d_config->b_info.num_ioports; i++) {
+        libxl_ioport_range *io = &d_config->b_info.ioports[i];
+
+        LOG(DEBUG, "dom%d ioports %"PRIx32"-%"PRIx32,
+            domid, io->first, io->first + io->number - 1);
+
+        ret = xc_domain_ioport_permission(CTX->xch, domid,
+                                          io->first, io->number, 1);
+        if ( ret<0 ){
+            LOGE(ERROR,
+                 "failed give dom%d access to ioports
%"PRIx32"-%"PRIx32,
+                 domid, io->first, io->first + io->number - 1);
+            ret = ERROR_FAIL;
+        }
+    }
+
+    for (i = 0; i < d_config->b_info.num_irqs; i++) {
+        uint32_t irq = d_config->b_info.irqs[i];
+
+        LOG(DEBUG, "dom%d irq %"PRIx32, domid, irq);
+
+        ret = xc_domain_irq_permission(CTX->xch, domid, irq, 1);
+        if ( ret<0 ){
+            LOGE(ERROR,
+                 "failed give dom%d access to irq %"PRId32, domid,
irq);
+            ret = ERROR_FAIL;
+        }
+    }
+
     for (i = 0; i < d_config->num_nics; i++) {
         /* We have to init the nic here, because we still haven''t
          * called libxl_device_nic_add at this point, but qemu needs
diff -r ccbee5bcb31b -r ddde6c2c45de tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Fri Aug 31 12:03:55 2012 +0100
+++ b/tools/libxl/libxl_types.idl	Fri Aug 31 16:54:01 2012 +0100
@@ -135,6 +135,11 @@ libxl_vga_interface_type = Enumeration("
 # Complex libxl types
 #
 
+libxl_ioport_range = Struct("ioport_range", [
+    ("first", uint32),
+    ("number", uint32),
+    ])
+
 libxl_vga_interface_info = Struct("vga_interface_info", [
     ("kind",    libxl_vga_interface_type),
     ])
@@ -277,6 +282,9 @@ libxl_domain_build_info = Struct("domain
     #  parameters for all type of scheduler
     ("sched_params",     libxl_domain_sched_params),
 
+    ("ioports",          Array(libxl_ioport_range,
"num_ioports")),
+    ("irqs",             Array(uint32, "num_irqs")),
+    
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",        
string),
                                        ("bios",            
libxl_bios_type),
diff -r ccbee5bcb31b -r ddde6c2c45de tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Aug 31 12:03:55 2012 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Fri Aug 31 16:54:01 2012 +0100
@@ -573,10 +573,12 @@ static void parse_config_data(const char
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids;
+    XLU_ConfigList *ioports, *irqs;
+    int num_ioports, num_irqs;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
     int pci_permissive = 0;
-    int e;
+    int i, e;
 
     libxl_domain_create_info *c_info = &d_config->c_info;
     libxl_domain_build_info *b_info = &d_config->b_info;
@@ -919,6 +921,89 @@ static void parse_config_data(const char
         abort();
     }
 
+    if (!xlu_cfg_get_list(config, "ioports", &ioports,
&num_ioports, 0)) {
+        b_info->num_ioports = num_ioports;
+        b_info->ioports = calloc(num_ioports, sizeof(*b_info->ioports));
+        if (b_info->ioports == NULL) {
+            fprintf(stderr, "unable to allocate memory for
ioports\n");
+            exit(-1);
+        }
+
+        for (i = 0; i < num_ioports; i++) {
+            const char *buf2;
+            char *ep;
+            uint32_t s, e;
+            unsigned long ul;
+
+            buf = xlu_cfg_get_listitem (ioports, i);
+            if (!buf) {
+                fprintf(stderr,
+                        "xl: Unable to get element #%d in ioport
list\n", i);
+                exit(1);
+            }
+            ul = strtoul(buf, &ep, 16);
+            if (ep == buf) {
+                fprintf(stderr, "xl: Invalid argument parsing ioport:
%s\n",
+                        buf);
+                exit(1);
+            }
+            if (ul > UINT32_MAX) {
+                fprintf(stderr, "xl: ioport %lx too big\n", ul);
+                exit(1);
+            }
+            s = e = ul;
+
+            if (*ep == ''-'') {
+                buf2 = ep + 1;
+                ul = strtoul(buf2, &ep, 16);
+                if (ep == buf2 || *ep != ''\0'' || s > e) {
+                    fprintf(stderr,
+                            "xl: Invalid argument parsing ioport:
%s\n", buf);
+                    exit(1);
+                }
+                if (ul > UINT32_MAX) {
+                    fprintf(stderr, "xl: ioport %lx too big\n", ul);
+                    exit(1);
+                }
+                e = ul;
+            } else if ( *ep != ''\0'' )
+                fprintf(stderr,
+                        "xl: Invalid argument parsing ioport: %s\n",
buf);
+            b_info->ioports[i].first = s;
+            b_info->ioports[i].number = e - s + 1;
+        }
+    }
+
+    if (!xlu_cfg_get_list(config, "irqs", &irqs, &num_irqs,
0)) {
+        b_info->num_irqs = num_irqs;
+        b_info->irqs = calloc(num_irqs, sizeof(*b_info->irqs));
+        if (b_info->irqs == NULL) {
+            fprintf(stderr, "unable to allocate memory for
ioports\n");
+            exit(-1);
+        }
+        for (i = 0; i < num_irqs; i++) {
+            char *ep;
+            unsigned long ul;
+            buf = xlu_cfg_get_listitem (irqs, i);
+            if (!buf) {
+                fprintf(stderr,
+                        "xl: Unable to get element %d in irq list\n",
i);
+                exit(1);
+            }
+            ul = strtoul(buf, &ep, 10);
+            if (ep == buf) {
+                fprintf(stderr,
+                        "xl: Invalid argument parsing irq: %s\n",
buf);
+                exit(1);
+            }
+            if (ul > UINT32_MAX) {
+                fprintf(stderr, "xl: irq %lx too big\n", ul);
+                exit(1);
+            }
+            b_info->irqs[i] = ul;
+        }
+    }
+
     if (!xlu_cfg_get_list (config, "disk", &vbds, 0, 0)) {
         d_config->num_disks = 0;
         d_config->disks = NULL;

Ian Campbell writes ("[PATCH V2] libxl/xl: implement support for guest
iooprt and irq permissions"):
> libxl/xl: implement support for guest iooprt and irq permissions.
...> 
> -    int e;
> +    int i, e;
...
> +            ul = strtoul(buf, &ep, 16);
> +            if (ep == buf) {
> +                fprintf(stderr, "xl: Invalid argument parsing ioport:
%s\n",
> +                        buf);
> +                exit(1);
> +            }
> +            if (ul > UINT32_MAX) {
> +                fprintf(stderr, "xl: ioport %lx too big\n", ul);
> +                exit(1);
> +            }
1. If long and int are the same size, this still mishandles
"1100000055\0", passing -1 to libxl.

2. You compare to UINT32_MAX and then assign to a signed integer.
That seems odd.

Ian.

On Fri, 2012-08-31 at 17:01 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH V2] libxl/xl: implement support for guest
iooprt and irq permissions"):
> > libxl/xl: implement support for guest iooprt and irq permissions.
> ...> 
> > -    int e;
> > +    int i, e;
> ...
> > +            ul = strtoul(buf, &ep, 16);
> > +            if (ep == buf) {
> > +                fprintf(stderr, "xl: Invalid argument parsing
ioport: %s\n",
> > +                        buf);
> > +                exit(1);
> > +            }
> > +            if (ul > UINT32_MAX) {
> > +                fprintf(stderr, "xl: ioport %lx too big\n",
ul);
> > +                exit(1);
> > +            }
> 
> 1. If long and int are the same size, this still mishandles
> "1100000055\0", passing -1 to libxl.
> 
> 2. You compare to UINT32_MAX and then assign to a signed integer.
> That seems odd.
Everything here is signed now.

Oh, I see the confusion, I''ve unwittingly shadowed the e
you''ve quoted
with an uint32_t in this scope. I''ll change the variable name since
that
obviously not helpful.

Ian.