[Xen-devel] [PATCH] refine C++ header checking compiler invocation

[Jan Beulich]

g++ 4.1.x dies with "cc1plus: error: output filename specified twice"
on the currently used construct. That's apparently due to it converting
the manually specified "c++" into "c++-header", and mis-handling that
(which, when using "c++-header" explicitly btw gets mis-handled even
with 4.9.x and also, using "c-header", by the plain C compiler).

Signed-off-by: Jan Beulich 

--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,9 +104,10 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Make
 headers++.chk: $(PUBLIC_HEADERS) Makefile
if $(CXX) -v >/dev/null 2>&1; then \
for i in $(filter %.h,$^); do \
-   $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
-  -include stdint.h -include public/xen.h \
-  -S -o /dev/null $$i || exit 1; \
+   echo '#include "$$i"' \
+   | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
+ -include stdint.h -include public/xen.h -S -o /dev/null - \
+   || exit 1; \
echo $$i; \
done ; \
fi >$@.new



refine C++ header checking compiler invocation

g++ 4.1.x dies with "cc1plus: error: output filename specified twice"
on the currently used construct. That's apparently due to it converting
the manually specified "c++" into "c++-header", and mis-handling that
(which, when using "c++-header" explicitly btw gets mis-handled even
with 4.9.x and also, using "c-header", by the plain C compiler).

Signed-off-by: Jan Beulich 

--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -104,9 +104,10 @@ headers.chk: $(PUBLIC_ANSI_HEADERS) Make
 headers++.chk: $(PUBLIC_HEADERS) Makefile
if $(CXX) -v >/dev/null 2>&1; then \
for i in $(filter %.h,$^); do \
-   $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
-  -include stdint.h -include public/xen.h \
-  -S -o /dev/null $$i || exit 1; \
+   echo '#include "$$i"' \
+   | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__ \
+ -include stdint.h -include public/xen.h -S -o /dev/null - \
+   || exit 1; \
echo $$i; \
done ; \
fi >$@.new
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] adjust assertion in alloc_heap_pages()

[Jan Beulich]

Older gcc warns (and due to -Werror fails) on this ASSERT() now that
"node" is of unsigned type. Make it more useful at once.

Signed-off-by: Jan Beulich 

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -604,7 +604,7 @@ static struct page_info *alloc_heap_page
 }
 first_node = node;
 
-ASSERT(node >= 0);
+ASSERT(node < MAX_NUMNODES);
 ASSERT(zone_lo <= zone_hi);
 ASSERT(zone_hi < NR_ZONES);
 



adjust assertion in alloc_heap_pages()

Older gcc warns (and due to -Werror fails) on this ASSERT() now that
"node" is of unsigned type. Make it more useful at once.

Signed-off-by: Jan Beulich 

--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -604,7 +604,7 @@ static struct page_info *alloc_heap_page
 }
 first_node = node;
 
-ASSERT(node >= 0);
+ASSERT(node < MAX_NUMNODES);
 ASSERT(zone_lo <= zone_hi);
 ASSERT(zone_hi < NR_ZONES);
 
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST Nested PATCH v8 4/7] Changes on test step of Debian hvm guest install

[Robert Hu]

On Tue, 2015-04-21 at 11:28 +0100, Ian Campbell wrote:
> On Mon, 2015-04-13 at 17:19 -0400, longtao.pang wrote:
> > 1. Increase disk size to accommodate to nested test requirement.
> > 2. Since 'Debain-xxx-.iso' image will be stored in rootfs of L1 guest,
> > therefore needs more disk capacity, increase root partition size in
> > preseed generation.
> > 3. In L1 installation context, assign more memory to it; Since it
> > acts as a nested hypervisor anyway.
> > 4. Comment out CDROM entry in sources.list to make HTTP URL entry
> > available for L1 hvm guest.
> > 5. Enable nestedhvm feature in ExtraConfig for nested job.
> > 
> > Signed-off-by: longtao.pang 
> > ---
> > Changes in v8:
> > 1. Update a conventional way to comment out CDROM entry in sources.list.
> > 2. Enable nestedhvm feature only for the nested job.
> > 3. Set nested disk and memroy size to be driven from runvars in
> > 'ts-debian-hvm-install'.
> > ---
> >  ts-debian-hvm-install |9 ++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
> > index cfd5144..13009d0 100755
> > --- a/ts-debian-hvm-install
> > +++ b/ts-debian-hvm-install
> > @@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
> >  
> >  # guest memory size will be set based on host free memory, see below
> >  our $ram_mb;
> > -our $disk_mb= 1;
> > +our $disk_mb= $r{'nested_disk'} ? $r{'nested_disk'} : 1;
> 
> This shouldn't hardcode 'nested' here, but use the guest ident. (nested
> is the wrong name now anyway I think, since it is nestedl1 in this
> iteration, which is why we avoid such hardcoding)
Right, we shall store this runvar with name of ${l1_ident}_disksize in
ts-nested-setup.
> 
> So you should arrange to do this somewhere that $gho is in scope and use
> guest_var($gho, 'disk', 1). $gho is in scope in $prep where $disk_mb
> is used and AFAICT it is only used in that function, so I think you can
> move this to a local variable
yes, it is supposed to be local of prep(). However, look at other ts-*
(ts-debian-install, ts-freebsd-install, etc.), $disk_mb is defined
'our'. They seems to be in alliance, are we going to it specifically in
ts-debian-hvm-install? or in some future day, we change these
ts-*-install in another patch series?

To use guest_var(), it needs $gho in scope as you said. But in prep() we
can see until prepareguest() returns, we don't have $gho in scope, while
prepareguest() needs $disk_mb as input. So here we get into a loop.
I think we shall in prepareguest(), check if $r{${guest_ident}_disksize}
is defined, then override passed in $mb value. How would you like this?
> 
> Perhaps also consider a more descriptive runvar name too, like disksize?
> 
> >  our $guesthost= "$gn.guest.osstest";
> >  our $gho;
> > @@ -60,7 +60,7 @@ d-i partman-auto/expert_recipe string \\
> >  use_filesystem{ } filesystem{ vfat } \\
> >  mountpoint{ /boot/efi } \\
> >  . \\
> > -5000 50 5000 ext4 \\
> > +1 50 1 ext4 \\
> >  method{ format } format{ } \\
> >  use_filesystem{ } filesystem{ ext4 } \\
> >  mountpoint{ / } \\
> > @@ -75,6 +75,7 @@ d-i preseed/late_command string \\
> >  in-target mkdir -p /boot/efi/EFI/boot; \\
> >  in-target cp /boot/efi/EFI/debian/grubx64.efi 
> > /boot/efi/EFI/boot/bootx64.efi ;\\
> >  in-target mkdir -p /root/.ssh; \\
> > +in-target sed -i 's/^deb *cdrom/#&/g' /etc/apt/sources.list; \\
> >  in-target sh -c "echo -e '$authkeys'> /root/.ssh/authorized_keys";
> >  END
> >  return $preseed_file;
> > @@ -149,9 +150,11 @@ sub prep () {
> >  target_putfilecontents_root_stash($ho, 10, preseed(),
> >$preseed_file_path);
> >  
> > +my $nestedhvm_feature = $gn eq 'nestedl1' ? 'nestedhvm=1' : '';
> 
> This should be based on guest_var($gho, "enable_nestedhvm", undef) not
> $gn eq "nested1".
> 
> Also I think this would be better done as:
>my $extra_config = '';
>$extra_config .= 'nestedhvm=1\n'
>if guest_var($gho, "enable_nestedhvm", 'false') eq 'true';
> 
> Then use $extra_config below. This will make it easier to add more such
> things in the future.
Ack.
> 
> >  more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
> >OnReboot => 'preserve',
> >Bios => $r{bios},
> > +  ExtraConfig => $nestedhvm_feature,
> >PostImageHook => sub {
> >  my $cmds = iso_copy_content_from_image($gho, $newiso);
> >  $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
> > @@ -174,7 +177,7 @@ my $ram_lots = 5000;
> >  if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
> >  $ram_mb = $ram_lots;
> >  } else {
> > -$ram_mb = 768;
> > +$ram_mb = $r{"${gn}_memo

[Xen-devel] [PATCH 2/2] xen/arm: gicv2: Adding support for GICv2m in Dom0

[Suravee Suthikulpanit]

This patch detect and propagate the gic-v2m-frame devicetree sub-node.
This allows Dom0 kernel to setup and intialize GICv2m MSI frame.

Signed-off-by: Suravee Suthikulpanit 
---
 xen/arch/arm/gic-v2.c | 169 ++
 1 file changed, 169 insertions(+)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 80acc62..0c3352e 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -600,6 +600,171 @@ static void gicv2_irq_set_affinity(struct irq_desc *desc, 
const cpumask_t *cpu_m
 spin_unlock(&gicv2.lock);
 }
 
+/*
+ * Set up gic v2m DT sub-node.
+ *
+ * gic0: interrupt-controller@e1101000 {
+ *  compatible = "arm,gic-400", "arm,cortex-a15-gic";
+ *  interrupt-controller;
+ *  #interrupt-cells = <3>;
+ *  #address-cells = <2>;
+ *  #size-cells = <2>;
+ *  reg = <0x0 0xe111 0 0x1000>,
+ *<0x0 0xe112f000 0 0x2000>,
+ *<0x0 0xe114 0 0x1>,
+ *<0x0 0xe116 0 0x1>;
+ *  interrupts = <1 9 0xf04>;
+ *  ranges = <0 0 0 0xe110 0 0x10>;
+ *  v2m0: v2m@e008 {
+ *  compatible = "arm,gic-v2m-frame";
+ *  msi-controller;
+ *  arm,msi-base-spi = <64>;
+ *  arm,msi-num-spis = <256>;
+ *  reg = <0x0 0x0008 0 0x1000>;
+ *  };
+ * };
+ */
+static int gicv2m_make_dt_node(const struct domain *d,
+  const struct dt_device_node *node,
+  void *fdt)
+{
+u32 len, base_spi, num_spis;
+u64 addr, size;
+int res, i;
+const void *prop = NULL;
+struct domain *dom;
+unsigned long mm_start, mm_nr;
+const struct dt_device_node *gic = dt_interrupt_controller;
+const struct dt_device_node *v2m = NULL;
+
+/* v2m is optional */
+v2m = dt_find_compatible_node(NULL, NULL, "arm,gic-v2m-frame");
+if ( !v2m )
+return 0;
+
+/* Sub-node requires the ranges property */
+prop = dt_get_property(gic, "ranges", &len);
+if ( !prop )
+{
+dprintk(XENLOG_ERR, "Can't find ranges property for the gic node\n");
+return -FDT_ERR_XEN(ENOENT);
+}
+
+res = fdt_property(fdt, "ranges", prop, len);
+if ( res )
+return res;
+
+dprintk(XENLOG_DEBUG, "Create v2m node\n");
+
+res = fdt_begin_node(fdt, "v2m");
+if ( res )
+return res;
+
+res = fdt_property(fdt, "compatible", "arm,gic-v2m-frame", 18);
+if ( res )
+goto err_out0;
+
+res = fdt_property(fdt, "msi-controller", NULL, 0);
+if ( res )
+goto err_out0;
+
+if ( v2m->phandle )
+{
+res = fdt_property_cell(fdt, "phandle", v2m->phandle);
+if ( res )
+goto err_out0;
+}
+
+prop = dt_get_property(v2m, "reg", &len);
+if ( !prop )
+{
+dprintk(XENLOG_ERR, "v2m: Can't find reg property.\n");
+res = -FDT_ERR_XEN(ENOENT);
+goto err_out0;
+}
+
+len = dt_cells_to_size(dt_n_addr_cells(node) + dt_n_size_cells(node));
+res = fdt_property(fdt, "reg", prop, len);
+if ( res )
+goto err_out0;
+
+/* Mapping MMIO regions for v2m frame */
+res = dt_device_get_address(v2m, 0, &addr, &size);
+if ( res )
+goto err_out0;
+
+dom = xzalloc_bytes(sizeof(struct domain));
+if ( !dom )
+{
+res = -ENOMEM;
+goto err_out0;
+}
+
+memcpy(dom, d, sizeof(struct domain));
+mm_start = paddr_to_pfn(addr & PAGE_MASK);
+mm_nr = DIV_ROUND_UP(size, PAGE_SIZE);
+res = map_mmio_regions(dom, mm_start, mm_nr, mm_start);
+if ( res )
+{
+dprintk(XENLOG_ERR, "v2m: map_mmio_regions failed.\n");
+goto err_out1;
+}
+
+/* Set up msi-base-spi dt property */
+prop = dt_get_property(v2m, "arm,msi-base-spi", &len);
+if ( !prop )
+{
+dprintk(XENLOG_ERR, "v2m: Can't find msi-base-spi.\n");
+goto err_out2;
+}
+base_spi = be32_to_cpup(prop);
+res = fdt_property(fdt, "arm,msi-base-spi", prop, len);
+if ( res )
+goto err_out2;
+
+/* Set up msi-num-spis dt property */
+prop = dt_get_property(v2m, "arm,msi-num-spis", &len);
+if ( !prop )
+{
+dprintk(XENLOG_ERR, "v2m: Can't find msi-num-spis.\n");
+goto err_out2;
+}
+num_spis = be32_to_cpup(prop);
+res = fdt_property(fdt, "arm,msi-num-spis", prop, len);
+if ( res )
+goto err_out2;
+
+/*
+ * Currently, we assign all SPIs for MSI to dom0
+ */
+for (i = base_spi; i < (base_spi + num_spis); i++)
+{
+res = irq_set_spi_type(i, DT_IRQ_TYPE_EDGE_RISING);
+if ( res )
+{
+dprintk(XENLOG_ERR, "v2m: Failed to set MSI interrupt type.\n");
+goto err_out2;
+}
+
+res = route_irq_to_guest(dom, i, i, "v2m");
+if ( res )
+{
+dprintk(XENLOG

[Xen-devel] [PATCH 1/2] xen/arm: gic: Refactor the code for creating gic node

[Suravee Suthikulpanit]

Since fdt_begin_node() is called by all gicXX_make_dt_node() to create
the interrupt-controller devicetree node, this patch refactors the call
and moves it inside make_gic_node(). This also matches the fdt_end_node()
call at the end of make_gic_node().

This patch also move the call to gic_make_node() wrapper to be after all
other generic properties are setup. This allows creating sub-node inside
gic_make_node() in order to support v2m sub-node.

Signed-off-by: Suravee Suthikulpanit 
---
 xen/arch/arm/domain_build.c | 18 +++---
 xen/arch/arm/gic-hip04.c|  4 
 xen/arch/arm/gic-v2.c   |  4 
 xen/arch/arm/gic-v3.c   |  4 
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 987ee1e..a2cd471 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -808,8 +808,8 @@ static int make_gic_node(const struct domain *d, void *fdt,
 {
 const struct dt_device_node *gic = dt_interrupt_controller;
 int res = 0;
-const void *addrcells;
-u32 addrcells_len;
+const void *addrcells, *sizecells;
+u32 addrcells_len, sizecells_len;
 
 /*
  * Xen currently supports only a single GIC. Discard any secondary
@@ -823,7 +823,7 @@ static int make_gic_node(const struct domain *d, void *fdt,
 
 DPRINT("Create gic node\n");
 
-res = gic_make_node(d, node, fdt);
+res = fdt_begin_node(fdt, "interrupt-controller");
 if ( res )
 return res;
 
@@ -847,6 +847,14 @@ static int make_gic_node(const struct domain *d, void *fdt,
 return res;
 }
 
+sizecells = dt_get_property(gic, "#size-cells", &sizecells_len);
+if ( sizecells )
+{
+res = fdt_property(fdt, "#size-cells", sizecells, sizecells_len);
+if ( res )
+return res;
+}
+
 res = fdt_property_cell(fdt, "#interrupt-cells", 3);
 if ( res )
 return res;
@@ -855,6 +863,10 @@ static int make_gic_node(const struct domain *d, void *fdt,
 if ( res )
 return res;
 
+res = gic_make_node(d, node, fdt);
+if ( res )
+return res;
+
 res = fdt_end_node(fdt);
 
 return res;
diff --git a/xen/arch/arm/gic-hip04.c b/xen/arch/arm/gic-hip04.c
index 223c414..6d527f1 100644
--- a/xen/arch/arm/gic-hip04.c
+++ b/xen/arch/arm/gic-hip04.c
@@ -628,10 +628,6 @@ static int hip04gic_make_dt_node(const struct domain *d,
 compatible = DT_COMPAT_GIC_CORTEX_A15;
 len = strlen((char*) compatible) + 1;
 
-res = fdt_begin_node(fdt, "interrupt-controller");
-if ( res )
-return res;
-
 res = fdt_property(fdt, "compatible", compatible, len);
 if ( res )
 return res;
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 073fec2..80acc62 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -616,10 +616,6 @@ static int gicv2_make_dt_node(const struct domain *d,
 return -FDT_ERR_XEN(ENOENT);
 }
 
-res = fdt_begin_node(fdt, "interrupt-controller");
-if ( res )
-return res;
-
 res = fdt_property(fdt, "compatible", compatible, len);
 if ( res )
 return res;
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
index e9a8eda..db498ed 100644
--- a/xen/arch/arm/gic-v3.c
+++ b/xen/arch/arm/gic-v3.c
@@ -1098,10 +1098,6 @@ static int gicv3_make_dt_node(const struct domain *d,
 return -FDT_ERR_XEN(ENOENT);
 }
 
-res = fdt_begin_node(fdt, "interrupt-controller");
-if ( res )
-return res;
-
 res = fdt_property(fdt, "compatible", compatible, len);
 if ( res )
 return res;
-- 
2.1.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 0/2] Introducing GICv2m Supports

[Suravee Suthikulpanit]

This patch series introduce GICv2m supports in Xen Dom0.

This patch series depend on: 
[PATCH v3 0/5] xen: arm: Parse PCI DT nodes' ranges and interrupt-map
http://lists.xen.org/archives/html/xen-devel/2015-04/msg02200.html

This has been tested on AMD Seattle platform with the following kernel:
git clone https://github.com/ssuthiku/linux.git xen-seattle-revA-pci

Suravee Suthikulpanit (2):
  xen/arm: gic: Refactor the code for creating gic node
  xen/arm: gicv2: Adding support for GICv2m in Dom0

 xen/arch/arm/domain_build.c |  18 -
 xen/arch/arm/gic-hip04.c|   4 -
 xen/arch/arm/gic-v2.c   | 173 +++-
 xen/arch/arm/gic-v3.c   |   4 -
 4 files changed, 184 insertions(+), 15 deletions(-)

-- 
2.1.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] Possible bug/question in xen-hptool?

[Meng Xu]

Hi,

I was looking at using xen-hptool (tool/misc/xen-hptool.c) to make one page
of a guest domain offline.

I created a guest domain on Xen unstable:​
# xen-mfndump dump-p2m 1
I have dom1's mfn of pfn (0x1d):
pfn=0x1d ==> mfn=0x14ee17 (type 0x0)

​Run `lookup-pte` to find the mfn of the pte of mfn (0x14ee17)​:
# xen-mfndump lookup-pte 1 0x14ee17
 --- Lookig for PTEs mapping mfn 0x14ee17 for domain 1 ---
 Guest Width: 8, PT Levels: 4 P2M size: = 262144
  0x14ee17 <-- [0xd948e][29]: 0x114ee17027

​Now I use xen-hptool to make mfn (0x14ee17) offline​:
# xen-hptool mem-offline 0x14ee17
Prepare to offline MEMORY mfn 14ee17
DOM1: No suspend port, try live migration
Failed to suspend guest 1 for mfn 14ee17
​(Comment: I modified the code to bypass the suspension of the dom1. I
should use libxl to suspend dom1 or use the event channel to notify dom1 to
suspend as the original code does. But this is not the question/issue I'm
talking about here right now and I don't think this will affect the
following discussion/conclusion.)​
xc: error: Failure when submitting mmu updates: Internal error
xc: error: clear pte failed: Internal error
Memory mfn 14ee17 offlined successfully , this page is DOM1 page yet failed
to be exchanged. current state is [PG_OFFLINE_PENDING, PG_OFFLINE_OWNED]
(XEN) mm.c:2004:d0v0 Error pfn d948e: rd=83015d446000,
od=83017d8d
​​
, caf=8004, taf=1402
(XEN) mm.c:3544:d0v0 Could not get page for normal update

​I looked into the do_mmu_update() @ xen/arch/x86/mm.c, the reason why this
mmu_update fails is because the owner of the page table of mfn (0x14ee17),
denoted as pt_dom, is domain 0, while the owner of the page of mfn
(0x14ee17) is domain 1 in do_mmu_update().

After digging into it, I found the following code confused/suspicious:

Inside do_mmu_update() @ xen/arch/x86/mm.c,
pt_dom is assigned by the this line:   if ( (pt_dom = foreigndom >> 16 ) !=
0 ) .
However, in flush_mmu_updates() @ tools/libxc/xc_private.c, the foreigndom
is assigned by the following line: hypercall.arg[3] = mmu->subject; where
mmu->subject is the guest domain id of the page table.

The first question is:
Why should we use "foreigndom >> 16" instead of "foreigndom" to get the
pt_dom?
(When a page is marked offline, we can get the domid of the page via
status, using status >> PG_OFFLINE_OWNER_SHIFT. But why should we left
shift 16 bits again in do_mmu_update?)
(I think this explains why pt_owner is treated as 0 because pt_owner was
just using the default value which is the domain of current vcpu that runs
the hypercall.)

pt_owner is retrieved by the following line :
if ( (pt_owner = rcu_lock_domain_by_id(pt_dom - 1)) == NULL )
My second question is:
Why should we use "pt_dom - 1" instead of  "pt_dom" here?

If I set the old foreigndom (1) as (foreigndom << 16 | foreigndom) and pass
the new foreigndom as the last parameter of do_mmu_update(), and change
"pt_dom - 1" to "pt_dom", the xen-hptool will successfully make the mfn
offline. Here is the output after issuing the command:Memory
mfn 0x14ee17 offlined successfully, this page is DOM1 page and being
swapped successfully, current state is [PG_OFFLINE_OFFLINED,
PG_OFFLINE_OWNED]

I'm wondering if this is a bug in do_mmu_update() or  at least some
inconsistence is in the do_mmu_update() code?
Of course, this could also be because I misunderstood something. If so,
could you please let me know what I misunderstood and how I should correct
it?

Thank you very much for your time!

Meng


---
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] tools/libxc: Set HVM_PARAM_CONSOLE_EVTCHN during restore

[Boris Ostrovsky]

When resuming, the guest needs to check whether the port has changed. HVM
guests use this parameter to get the port number.

(We can't always use xenstore where this value is also written: for example
on Linux the console is resumed very early, before the store is up).

Signed-off-by: Boris Ostrovsky 
---
 tools/libxc/xc_domain_restore.c |5 -
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index 2ab9f46..f9dfd2d 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -2328,7 +2328,10 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
uint32_t dom,
 HVM_PARAM_PAE_ENABLED, pae))
  || (frc = xc_hvm_param_set(xch, dom,
 HVM_PARAM_STORE_EVTCHN,
-store_evtchn)) )
+store_evtchn))
+ || (frc = xc_hvm_param_set(xch, dom,
+HVM_PARAM_CONSOLE_EVTCHN,
+console_evtchn)) )
 {
 PERROR("error setting HVM params: %i", frc);
 goto out;
-- 
1.7.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen: Add __GFP_DMA flag when xen_swiotlb_init gets free pages.

[Chen Baozi]

On Tue, Apr 21, 2015 at 12:11:01PM +0100, Stefano Stabellini wrote:
> Chen,
> could you please try the patch below in your repro scenario?
> I have only build tested it.
> 
> ---
> 
> xen: Add __GFP_DMA flag when xen_swiotlb_init gets free pages on ARM
> 
> From: Chen Baozi 
> 
> Make sure that xen_swiotlb_init allocates buffers that are DMA capable
> when at least one memblock is available below 4G. Otherwise we assume
> that all devices on the SoC can cope with >4G addresses.
> 
> No functional changes on x86.
> 
> Signed-off-by: Chen Baozi 
> Signed-off-by: Stefano Stabellini 

Tested-by: Chen Baozi 

Cheers,

Baozi

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm for nested test

[Hu, Robert]

> -Original Message-
> From: Ian Jackson [mailto:ian.jack...@eu.citrix.com]
> Sent: Wednesday, April 22, 2015 8:50 PM
> To: Ian Campbell
> Cc: Pang, LongtaoX; xen-devel@lists.xen.org; wei.l...@citrix.com; Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in TestSupport.pm
> for nested test
> 
> Ian Campbell writes ("Re: [OSSTEST Nested PATCH v8 3/7] Edit some APIs in
> TestSupport.pm for nested test"):
> > It will, I think, need to be integrated with the existing assignment to
> > $ho->{Ip} in select host, so something like:
> >
> > if ( $r{"${ident}_ip"} ) {
> > $ho->{Ip}= $r{"${ident}_ip"};
> > } else {
> > $ho->{Ip}= $ho->{IpStatic};
> > }
> 
> Yes.
Yes, otherwise the code would go 'die' somewhere later.
> 
> > or perhaps:
> >
> > $ho->{Ip} = $r{"${ident}_ip"} ? $r{"${ident}_ip"} : $ho->{IpStatic};
> 
> The shortest way to spell this is to use //, eg:
> 
>   $ho->{Ip} = $r{"${ident}_ip"} // $ho->{IpStatic};
Ah, yes!
> 
> Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: document foreground '-F' option of create command

[Giuseppe Mazzotta]

On 04/22/2015 04:40 PM, Giuseppe Mazzotta wrote:
> On 04/22/2015 04:21 PM, Ian Campbell wrote:
>> On Fri, 2015-04-17 at 17:36 +0200, Giuseppe Mazzotta wrote:
>> Do you fancy also adding -F to docs/man/xl.pod.1?
>>
> Yes, I will follow-up in a few hours with another patch for that.
> 
Please disregard my 2-patches set trying to document -e and -F, sorry
for the noise. I will refrain from submitting doc updates for the other
missing options (there's more, apparently) until I better understand how
they work.

The latest one is the patch I propose for now:
http://lists.xen.org/archives/html/xen-devel/2015-04/msg02606.html

Kind regards,
--
  Giuseppe

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] docs/xl: document -F options of create subcommand

[Giuseppe Mazzotta]

From: Giuseppe 

Document '-F' option. Other options are still missing and not in this patch.

Signed-off-by: Giuseppe Mazzotta 
---
 docs/man/xl.pod.1 | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 16783c8..02bf531 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -115,6 +115,9 @@ Create will return B as the domain is started.  
This B mean the guest OS in the domain has actually booted, or is
 available for input.
 
+If the I<-F> option is specified, create will start the domain and not
+return until its death.
+
 B
 
 =over 4 
@@ -131,6 +134,10 @@ Use the given configuration file.
 
 Leave the domain paused after it is created.
 
+=item B<-F>
+
+Run in foreground until death of the domain.
+
 =item B<-V>, B<--vncviewer>
 
 Attach to domain's VNC server, forking a vncviewer process.
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/2] libxl: reword command-line help text for -e and -F options of create subcommand

[Giuseppe Mazzotta]

Use a better wording about what the -e and -F options do; this is evident by
looking at main_create(), as the only difference is the setting of 'monitor'
variable. The help text for '-e' was possibly a copy-paste artifact.

Signed-off-by: Giuseppe Mazzotta 
---
 tools/libxl/xl_cmdtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 7f4759b..804c3bf 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -30,8 +30,8 @@ struct cmd_spec cmd_table[] = {
   "-n, --dryrunDry run - prints the resulting configuration\n"
   " (deprecated in favour of global -N option).\n"
   "-d  Enable debug messages.\n"
-  "-F  Run in foreground until death of the domain.\n"
-  "-e  Do not wait in the background for the death of 
the domain.\n"
+  "-e  Run in foreground until death of the domain.\n"
+  "-F  Same as -e, with additional guest monitoring 
messages.\n"
   "-V, --vncviewer Connect to the VNC display after the domain is 
created.\n"
   "-A, --vncviewer-autopass\n"
   "Pass VNC password to viewer via stdin."
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 2/2] docs/xl: document -e and -F options of create subcommand

[Giuseppe Mazzotta]

From: Giuseppe 

While documenting '-F' option, I discovered that not only '-e' was missing
(and hereby added with this patch) but others too. I will eventually submit
other patches for the remaining missing options, as this one focuses only on
'-e' and '-F'.

Signed-off-by: Giuseppe Mazzotta 
---
 docs/man/xl.pod.1 | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 16783c8..7bb26a6 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -115,6 +115,9 @@ Create will return B as the domain is started.  
This B mean the guest OS in the domain has actually booted, or is
 available for input.
 
+If the I<-F> or I<-e> option is specified, create will start the domain and
+not return until its death.
+
 B
 
 =over 4 
@@ -131,6 +134,14 @@ Use the given configuration file.
 
 Leave the domain paused after it is created.
 
+=item B<-e>
+
+Run in foreground until death of the domain.
+
+=item B<-F>
+
+Same as I<-e>, with additional guest monitoring messages.
+
 =item B<-V>, B<--vncviewer>
 
 Attach to domain's VNC server, forking a vncviewer process.
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] [media] ivtv: use arch_phys_wc_add() and require PAT disabled

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" 

We are burrying direct access to MTRR code support on
x86 in order to take advantage of PAT. In the future we
also want to make the default behaviour of ioremap_nocache()
to use strong UC, use of mtrr_add() on those systems
would make write-combining void.

In order to help both enable us to later make strong
UC default and in order to phase out direct MTRR access
code port the driver over to arch_phys_wc_add() and
annotate that the device driver requires systems to
boot with PAT disabled, with the nopat kernel parameter.

This is a worthy comprmise given that the hardware is
really rare these days, and perhaps only some lost souls
in some third world country are expected to be using this
feature of the device driver.

Cc: Mauro Carvalho Chehab 
Cc: Andy Lutomirski 
Cc: Andy Walls 
Cc: Suresh Siddha 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Juergen Gross 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: Bjorn Helgaas 
Cc: Antonino Daplas 
Cc: Jean-Christophe Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: Dave Hansen 
Cc: Arnd Bergmann 
Cc: Michael S. Tsirkin 
Cc: Stefan Bader 
Cc: Ville Syrjälä 
Cc: Mel Gorman 
Cc: Vlastimil Babka 
Cc: Borislav Petkov 
Cc: Davidlohr Bueso 
Cc: konrad.w...@oracle.com
Cc: ville.syrj...@linux.intel.com
Cc: david.vra...@citrix.com
Cc: jbeul...@suse.com
Cc: toshi.k...@hp.com
Cc: Roger Pau Monné 
Cc: linux-fb...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: ivtv-de...@ivtvdriver.org
Cc: linux-me...@vger.kernel.org
Cc: xen-de...@lists.xensource.com
Signed-off-by: Luis R. Rodriguez 
---
 drivers/media/pci/ivtv/Kconfig  |  3 +++
 drivers/media/pci/ivtv/ivtvfb.c | 59 +
 2 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/media/pci/ivtv/Kconfig b/drivers/media/pci/ivtv/Kconfig
index dd6ee57e..b2a7f88 100644
--- a/drivers/media/pci/ivtv/Kconfig
+++ b/drivers/media/pci/ivtv/Kconfig
@@ -57,5 +57,8 @@ config VIDEO_FB_IVTV
  This is used in the Hauppauge PVR-350 card. There is a driver
  homepage at .
 
+ If you have this hardware you will need to boot with PAT disabled
+ on your x86 systems, use the nopat kernel parameter.
+
  To compile this driver as a module, choose M here: the
  module will be called ivtvfb.
diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 9ff1230..552408b 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -44,8 +44,8 @@
 #include 
 #include 
 
-#ifdef CONFIG_MTRR
-#include 
+#ifdef CONFIG_X86_64
+#include 
 #endif
 
 #include "ivtv-driver.h"
@@ -155,12 +155,11 @@ struct osd_info {
/* Buffer size */
u32 video_buffer_size;
 
-#ifdef CONFIG_MTRR
/* video_base rounded down as required by hardware MTRRs */
unsigned long fb_start_aligned_physaddr;
/* video_base rounded up as required by hardware MTRRs */
unsigned long fb_end_aligned_physaddr;
-#endif
+   int wc_cookie;
 
/* Store the buffer offset */
int set_osd_coords_x;
@@ -1099,6 +1098,8 @@ static int ivtvfb_init_vidmode(struct ivtv *itv)
 static int ivtvfb_init_io(struct ivtv *itv)
 {
struct osd_info *oi = itv->osd_info;
+   /* Find the largest power of two that maps the whole buffer */
+   int size_shift = 31;
 
mutex_lock(&itv->serialize_lock);
if (ivtv_init_on_first_open(itv)) {
@@ -1120,6 +1121,7 @@ static int ivtvfb_init_io(struct ivtv *itv)
oi->video_buffer_size = 1704960;
 
oi->video_pbase = itv->base_addr + IVTV_DECODER_OFFSET + 
oi->video_rbase;
+   /* XXX: split this for PAT */
oi->video_vbase = itv->dec_mem + oi->video_rbase;
 
if (!oi->video_vbase) {
@@ -1132,29 +1134,16 @@ static int ivtvfb_init_io(struct ivtv *itv)
oi->video_pbase, oi->video_vbase,
oi->video_buffer_size / 1024);
 
-#ifdef CONFIG_MTRR
-   {
-   /* Find the largest power of two that maps the whole buffer */
-   int size_shift = 31;
-
-   while (!(oi->video_buffer_size & (1 << size_shift))) {
-   size_shift--;
-   }
-   size_shift++;
-   oi->fb_start_aligned_physaddr = oi->video_pbase & ~((1 << 
size_shift) - 1);
-   oi->fb_end_aligned_physaddr = oi->video_pbase + 
oi->video_buffer_size;
-   oi->fb_end_aligned_physaddr += (1 << size_shift) - 1;
-   oi->fb_end_aligned_physaddr &= ~((1 << size_shift) - 1);
-   if (mtrr_add(oi->fb_start_aligned_physaddr,
-   oi->fb_end_aligned_physaddr - 
oi->fb_start_aligned_physaddr,
-MTRR_TYPE_WRCOMB, 1) < 0) {
-   IVTVFB_INFO("disabled mttr\n");
-   oi->fb_start_aligned_physaddr = 0;
-   oi->fb_end_aligned_physaddr = 0;
-   }
-   }
-#endi

[Xen-devel] [PATCH] IB/ipath: use arch_phys_wc_add() and require PAT disabled

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" 

We are burrying direct access to MTRR code support on
x86 in order to take advantage of PAT. In the future we
also want to make the default behaviour of ioremap_nocache()
to use strong UC, use of mtrr_add() on those systems
would make write-combining void.

In order to help both enable us to later make strong
UC default and in order to phase out direct MTRR access
code port the driver over to arch_phys_wc_add() and
annotate that the device driver requires systems to
boot with PAT disabled, with the nopat kernel parameter.

This is a worthy compromise given that the ipath device
driver powers the old HTX bus cards that only work in
AMD systems, while the newer IB/qib device driver
powers all PCI-e cards. The ipath device driver is
obsolete, hardware hard to find and because of this
this its a reasonable compromise to make to require
users of ipath to boot with nopat.

Cc: Doug Ledford 
Cc: Hal Rosenstock 
Cc: Sean Hefty 
Cc: Suresh Siddha 
Cc: Rickard Strandqvist 
Cc: Mike Marciniszyn 
Cc: Roland Dreier 
Cc: Andy Lutomirski 
Cc: Ingo Molnar 
Cc: Linus Torvalds 
Cc: Thomas Gleixner 
Cc: Juergen Gross 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: Bjorn Helgaas 
Cc: Antonino Daplas 
Cc: Jean-Christophe Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: Ville Syrjälä 
Cc: Mel Gorman 
Cc: Vlastimil Babka 
Cc: Borislav Petkov 
Cc: Davidlohr Bueso 
Cc: Dave Hansen 
Cc: Arnd Bergmann 
Cc: Michael S. Tsirkin 
Cc: Stefan Bader 
Cc: konrad.w...@oracle.com
Cc: ville.syrj...@linux.intel.com
Cc: david.vra...@citrix.com
Cc: jbeul...@suse.com
Cc: toshi.k...@hp.com
Cc: Roger Pau Monné 
Cc: infinip...@intel.com
Cc: linux-r...@vger.kernel.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: xen-de...@lists.xensource.com
Signed-off-by: Luis R. Rodriguez 
---
 drivers/infiniband/hw/ipath/Kconfig   |  3 ++
 drivers/infiniband/hw/ipath/ipath_driver.c| 18 +++
 drivers/infiniband/hw/ipath/ipath_kernel.h|  4 +--
 drivers/infiniband/hw/ipath/ipath_wc_x86_64.c | 43 ++-
 4 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/drivers/infiniband/hw/ipath/Kconfig 
b/drivers/infiniband/hw/ipath/Kconfig
index 1d9bb11..8fe54ff 100644
--- a/drivers/infiniband/hw/ipath/Kconfig
+++ b/drivers/infiniband/hw/ipath/Kconfig
@@ -9,3 +9,6 @@ config INFINIBAND_IPATH
as IP-over-InfiniBand as well as with userspace applications
(in conjunction with InfiniBand userspace access).
For QLogic PCIe QLE based cards, use the QIB driver instead.
+
+   If you have this hardware you will need to boot with PAT disabled
+   on your x86-64 systems, use the nopat kernel parameter.
diff --git a/drivers/infiniband/hw/ipath/ipath_driver.c 
b/drivers/infiniband/hw/ipath/ipath_driver.c
index bd0caed..3ef592c 100644
--- a/drivers/infiniband/hw/ipath/ipath_driver.c
+++ b/drivers/infiniband/hw/ipath/ipath_driver.c
@@ -42,6 +42,9 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_X86_64
+#include 
+#endif
 
 #include "ipath_kernel.h"
 #include "ipath_verbs.h"
@@ -395,6 +398,14 @@ static int ipath_init_one(struct pci_dev *pdev, const 
struct pci_device_id *ent)
unsigned long long addr;
u32 bar0 = 0, bar1 = 0;
 
+#ifdef CONFIG_X86_64
+   if (WARN(pat_enabled,
+"ipath needs PAT disabled, boot with nopat kernel 
parameter\n")) {
+   ret = EINVAL;
+   goto bail;
+   }
+#endif
+
dd = ipath_alloc_devdata(pdev);
if (IS_ERR(dd)) {
ret = PTR_ERR(dd);
@@ -542,6 +553,7 @@ static int ipath_init_one(struct pci_dev *pdev, const 
struct pci_device_id *ent)
dd->ipath_kregbase = __ioremap(addr, len,
(_PAGE_NO_CACHE|_PAGE_WRITETHRU));
 #else
+   /* XXX: split this properly to enable on PAT */
dd->ipath_kregbase = ioremap_nocache(addr, len);
 #endif
 
@@ -587,12 +599,8 @@ static int ipath_init_one(struct pci_dev *pdev, const 
struct pci_device_id *ent)
 
ret = ipath_enable_wc(dd);
 
-   if (ret) {
-   ipath_dev_err(dd, "Write combining not enabled "
- "(err %d): performance may be poor\n",
- -ret);
+   if (ret)
ret = 0;
-   }
 
ipath_verify_pioperf(dd);
 
diff --git a/drivers/infiniband/hw/ipath/ipath_kernel.h 
b/drivers/infiniband/hw/ipath/ipath_kernel.h
index e08db70..f0f9471 100644
--- a/drivers/infiniband/hw/ipath/ipath_kernel.h
+++ b/drivers/infiniband/hw/ipath/ipath_kernel.h
@@ -463,9 +463,7 @@ struct ipath_devdata {
/* offset in HT config space of slave/primary interface block */
u8 ipath_ht_slave_off;
/* for write combining settings */
-   unsigned long ipath_wc_cookie;
-   unsigned long ipath_wc_base;
-   unsigned long ipath_wc_len;
+   int wc_cookie;
/* ref count for each pkey */
atomic_t ipath_pkeyrefs[4];
/* shadow copy of struct page *'s for exp tid pages */
d

[Xen-devel] [PATCH v5 2/2] IB/qib: use arch_phys_wc_add()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" 

This driver already makes use of ioremap_wc() on PIO buffers,
so convert it to use arch_phys_wc_add().

The qib driver uses a mmap() special case for when PAT is
not used, this behaviour used to be determined with a
module parameter but since we have been asked to just
remove that module parameter this checks for the WC cookie,
if not set we can assume PAT was used. If its set we do
what we used to do for the mmap for when MTRR was enabled.

The removal of the module parameter is OK given that Andy
notes that even if users of module parameter are still around
it will not prevent loading of the module on recent kernels.

Cc: Doug Ledford 
Cc: Toshi Kani 
Cc: Rickard Strandqvist 
Cc: Mike Marciniszyn 
Cc: Roland Dreier 
Cc: Sean Hefty 
Cc: Hal Rosenstock 
Cc: Dennis Dalessandro 
Cc: Andy Lutomirski 
Cc: Suresh Siddha 
Cc: Ingo Molnar 
Cc: Thomas Gleixner 
Cc: Juergen Gross 
Cc: Daniel Vetter 
Cc: Dave Airlie 
Cc: Bjorn Helgaas 
Cc: Antonino Daplas 
Cc: Jean-Christophe Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: Dave Hansen 
Cc: Arnd Bergmann 
Cc: Michael S. Tsirkin 
Cc: Stefan Bader 
Cc: konrad.w...@oracle.com
Cc: ville.syrj...@linux.intel.com
Cc: david.vra...@citrix.com
Cc: jbeul...@suse.com
Cc: Roger Pau Monné 
Cc: infinip...@intel.com
Cc: linux-r...@vger.kernel.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Cc: xen-de...@lists.xensource.com
Signed-off-by: Luis R. Rodriguez 
---

This v5 changes the comment to make it clear when the mmap code
will be true as requested by Doug. This patch still depends on
the v4 1/2 qib patch which enables accounting for the mtrr_add()
call. I won't be resending that one.

 drivers/infiniband/hw/qib/qib.h   |  1 -
 drivers/infiniband/hw/qib/qib_file_ops.c  |  3 ++-
 drivers/infiniband/hw/qib/qib_iba6120.c   |  8 +++---
 drivers/infiniband/hw/qib/qib_iba7220.c   |  8 +++---
 drivers/infiniband/hw/qib/qib_iba7322.c   | 41 +++
 drivers/infiniband/hw/qib/qib_init.c  | 26 ++--
 drivers/infiniband/hw/qib/qib_wc_x86_64.c | 31 +++
 7 files changed, 39 insertions(+), 79 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
index ffd48bf..ba5173e2 100644
--- a/drivers/infiniband/hw/qib/qib.h
+++ b/drivers/infiniband/hw/qib/qib.h
@@ -1136,7 +1136,6 @@ extern struct qib_devdata *qib_lookup(int unit);
 extern u32 qib_cpulist_count;
 extern unsigned long *qib_cpulist;
 
-extern unsigned qib_wc_pat;
 extern unsigned qib_cc_table_size;
 int qib_init(struct qib_devdata *, int);
 int init_chip_wc_pat(struct qib_devdata *dd, u32);
diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c 
b/drivers/infiniband/hw/qib/qib_file_ops.c
index 9ea6c44..7258818 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -835,7 +835,8 @@ static int mmap_piobufs(struct vm_area_struct *vma,
vma->vm_flags &= ~VM_MAYREAD;
vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND;
 
-   if (qib_wc_pat)
+   /* We used PAT if wc_cookie == 0 */
+   if (!dd->wc_cookie)
vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
 
ret = io_remap_pfn_range(vma, vma->vm_start, phys >> PAGE_SHIFT,
diff --git a/drivers/infiniband/hw/qib/qib_iba6120.c 
b/drivers/infiniband/hw/qib/qib_iba6120.c
index 0d2ba59..4b927809 100644
--- a/drivers/infiniband/hw/qib/qib_iba6120.c
+++ b/drivers/infiniband/hw/qib/qib_iba6120.c
@@ -3315,11 +3315,9 @@ static int init_6120_variables(struct qib_devdata *dd)
qib_6120_config_ctxts(dd);
qib_set_ctxtcnt(dd);
 
-   if (qib_wc_pat) {
-   ret = init_chip_wc_pat(dd, 0);
-   if (ret)
-   goto bail;
-   }
+   ret = init_chip_wc_pat(dd, 0);
+   if (ret)
+   goto bail;
set_6120_baseaddrs(dd); /* set chip access pointers now */
 
ret = 0;
diff --git a/drivers/infiniband/hw/qib/qib_iba7220.c 
b/drivers/infiniband/hw/qib/qib_iba7220.c
index 22affda..00b2af2 100644
--- a/drivers/infiniband/hw/qib/qib_iba7220.c
+++ b/drivers/infiniband/hw/qib/qib_iba7220.c
@@ -4126,11 +4126,9 @@ static int qib_init_7220_variables(struct qib_devdata 
*dd)
qib_7220_config_ctxts(dd);
qib_set_ctxtcnt(dd);  /* needed for PAT setup */
 
-   if (qib_wc_pat) {
-   ret = init_chip_wc_pat(dd, 0);
-   if (ret)
-   goto bail;
-   }
+   ret = init_chip_wc_pat(dd, 0);
+   if (ret)
+   goto bail;
set_7220_baseaddrs(dd); /* set chip access pointers now */
 
ret = 0;
diff --git a/drivers/infiniband/hw/qib/qib_iba7322.c 
b/drivers/infiniband/hw/qib/qib_iba7322.c
index ef97b71..f32b462 100644
--- a/drivers/infiniband/hw/qib/qib_iba7322.c
+++ b/drivers/infiniband/hw/qib/qib_iba7322.c
@@ -6429,6 +6429,7 @@ static int qib_init_7322_variables(struct qib_devdata *dd)
unsigned features, pidx

Re: [Xen-devel] [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Luis R. Rodriguez]

On Wed, Apr 22, 2015 at 01:48:27PM -0400, Doug Ledford wrote:
> On Wed, 2015-04-22 at 19:37 +0200, Luis R. Rodriguez wrote:
> > On Wed, Apr 22, 2015 at 12:57:18PM -0400, Doug Ledford wrote:
> > > On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> > > > On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > > > > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > > > > 
> > > > > This:
> > > > > > +   /* MTRR was used if this is non-zero */
> > > > > > +   if (!dd->wc_cookie)
> > > > > > vma->vm_page_prot = 
> > > > > > pgprot_writecombine(vma->vm_page_prot);
> > > > > 
> > > > > And this:
> > > > > > +   dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > > > > +   if (dd->wc_cookie < 0)
> > > > > > +   ret = -EINVAL;
> > > > > 
> > > > > don't agree on what wc_cookie will be on error.
> > > > 
> > > > Can you elaborate? The one below is the one that starts things,
> > > > and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> > > > systems it will return a number > 0 *iff* a valid MTRR was added.
> > > > It will return negative onloy on error then.
> > > > 
> > > > The change above is meant to replace a check put in place to see
> > > > if PAT was enabled. The way we replace this is to ensure that
> > > > arch_phys_wc_add() returned 0.
> > > > 
> > > > If you disagree it'd be great if you can elaborate why.
> > > 
> > > Maybe I'm missing something, but in qib_enable_wc() you store the return
> > > from arch_phys_wc_add into wc_cookie.  That return is negative,
> > 
> > If and only if the system was non-PAT and mtrr_add() failed.
> > 
> > >  so you
> > > return from qib_enable_wc() to qib_init_one(), they see the ret value,
> > > they print out a warning about bad performance, then they clear the
> > > return value and continue with device initialization.
> > > 
> > > In all of this though, wc_cookie is never cleared and so it still has
> > > the error condition in it.  Then, much later at run time, you call
> > > mmap_piobufs() and you check the contents of wc_cookie, and if it's
> > > non-0 (which is still will be), you do the wrong thing, right?
> > 
> > Originally the code had it to run pgprot_writecombine() if PAT was going to 
> > be
> > used. After the code changes we check for !cookie which will be true when
> > cookie is 0 only. In case the cookie was an error, that is if mtrr_add()
> > failed, then this code would not run because (!negative) is false. The goal 
> > was
> > to trigger a run if the cookie was 0, which can only happen if PAT was 
> > enabled.
> 
> OK, the logic works, but as much as anything, it's the comment that's
> misleading.  The code would be clearer with a comment like this:
> 
> /* We used PAT if wc_cookie == 0 */
> if (!dd->wc_cookie) {
> 
> That would be more accurate as well since the original comment didn't
> account for the possible error code in wc_cookie, so it's possible you
> didn't use either PAT or wc if you have that error code.

Fair enough, will send a v5 follow up with the comment enhanced,
but will leave the first patch in this series as-is.

  Luis

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-04-22 at 17:33 +0100, Jan Beulich wrote:
> >>> On 22.04.15 at 17:36,  wrote:
> > On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
> >> >>> On 22.04.15 at 16:01,  wrote:
> >> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> >> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> >> >> top of the current target") caused a regression for "xl mem-set"
> >> >> against Dom0: While prior to creation of the first domain this works,
> >> >> the first domain creation involving ballooning breaks. Due to "enforce"
> >> >> not being set in the domain creation case, and due to Dom0's initial
> >> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> >> >> "memorykb" in the first "xl mem-set" adusting the target upwards
> >> >> subsequent to domain creation and termination may cause an overflow,
> >> >> resulting in Dom0's maximum getting to a very small value. This small
> >> >> maximum will the make the subsequent setting of the PoD target fail.
> >> >> 
> >> >> Signed-off-by: Jan Beulich 
> >> >> ---
> >> >> Note that this only fixes the immediate problem - there appear to be
> >> >> further issues lurking here:
> >> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> >> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> >> > 
> >> > I think that increasing the width of these variables wouldn't break the
> >> > API guarantee which we make, at least not in a practical way, since any
> >> > existing 32-bit arguments passed will just get promoted.
> >> 
> >> No, not even on 64-bit. On 32-bit, two arguments slots are needed
> >> for what so far requires only one. On 64-bit (at least x86-64), the
> >> calling code isn't required to zero-extend a value calculated in a
> >> register (e.g. a result of earlier calculations which had more than
> >> 32 significant bits could be passed unchanged to the called function);
> >> it just so happens that 32-bit arithmetic on registers would always
> >> implicitly zero the upper halves (and iirc that's the same on ARM64).
> > 
> > You seem to be talking about ABI? As I tried to note in my response for
> > libxl we only make guarantees about the API (P not B in the middle).
> 
> Oh, okay. That would mean the libxl side is fine. The change to the
> libxc interface might however still be a problem for qemu (Stefano
> was telling me something to the effect of a compatibility layer in
> upstream qemu to deal with such version differences, but of course
> the implication - without versioned libxc interfaces - would still be
> that existing qemu binaries on top of updated libxc would no
> longer work).

Correct, but we would surely bump the SONAME for a change such as this,
so it wouldn't matter in practice.

Ian.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Doug Ledford]

On Wed, 2015-04-22 at 19:37 +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 22, 2015 at 12:57:18PM -0400, Doug Ledford wrote:
> > On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> > > On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > > > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > > > 
> > > > This:
> > > > > + /* MTRR was used if this is non-zero */
> > > > > + if (!dd->wc_cookie)
> > > > >   vma->vm_page_prot = 
> > > > > pgprot_writecombine(vma->vm_page_prot);
> > > > 
> > > > And this:
> > > > > + dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > > > + if (dd->wc_cookie < 0)
> > > > > + ret = -EINVAL;
> > > > 
> > > > don't agree on what wc_cookie will be on error.
> > > 
> > > Can you elaborate? The one below is the one that starts things,
> > > and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> > > systems it will return a number > 0 *iff* a valid MTRR was added.
> > > It will return negative onloy on error then.
> > > 
> > > The change above is meant to replace a check put in place to see
> > > if PAT was enabled. The way we replace this is to ensure that
> > > arch_phys_wc_add() returned 0.
> > > 
> > > If you disagree it'd be great if you can elaborate why.
> > 
> > Maybe I'm missing something, but in qib_enable_wc() you store the return
> > from arch_phys_wc_add into wc_cookie.  That return is negative,
> 
> If and only if the system was non-PAT and mtrr_add() failed.
> 
> >  so you
> > return from qib_enable_wc() to qib_init_one(), they see the ret value,
> > they print out a warning about bad performance, then they clear the
> > return value and continue with device initialization.
> > 
> > In all of this though, wc_cookie is never cleared and so it still has
> > the error condition in it.  Then, much later at run time, you call
> > mmap_piobufs() and you check the contents of wc_cookie, and if it's
> > non-0 (which is still will be), you do the wrong thing, right?
> 
> Originally the code had it to run pgprot_writecombine() if PAT was going to be
> used. After the code changes we check for !cookie which will be true when
> cookie is 0 only. In case the cookie was an error, that is if mtrr_add()
> failed, then this code would not run because (!negative) is false. The goal 
> was
> to trigger a run if the cookie was 0, which can only happen if PAT was 
> enabled.

OK, the logic works, but as much as anything, it's the comment that's
misleading.  The code would be clearer with a comment like this:

/* We used PAT if wc_cookie == 0 */
if (!dd->wc_cookie) {

That would be more accurate as well since the original comment didn't
account for the possible error code in wc_cookie, so it's possible you
didn't use either PAT or wc if you have that error code.

> Please let me know, I'd like to get this right too.
> 
> > And what
> > about at shutdown when you call qib_disable_wc() and your cookie still
> > has an error code in it as well?
> 
> Well fortunately arch_phys_wc_del(negative) and arch_phys_wc_del(0) will be
> a no-op. Its what helps us remove so much clutter.

OK.

-- 
Doug Ledford 
  GPG KeyID: 0E572FDD




signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Luis R. Rodriguez]

On Wed, Apr 22, 2015 at 12:57:18PM -0400, Doug Ledford wrote:
> On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> > On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > > 
> > > This:
> > > > +   /* MTRR was used if this is non-zero */
> > > > +   if (!dd->wc_cookie)
> > > > vma->vm_page_prot = 
> > > > pgprot_writecombine(vma->vm_page_prot);
> > > 
> > > And this:
> > > > +   dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > > +   if (dd->wc_cookie < 0)
> > > > +   ret = -EINVAL;
> > > 
> > > don't agree on what wc_cookie will be on error.
> > 
> > Can you elaborate? The one below is the one that starts things,
> > and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> > systems it will return a number > 0 *iff* a valid MTRR was added.
> > It will return negative onloy on error then.
> > 
> > The change above is meant to replace a check put in place to see
> > if PAT was enabled. The way we replace this is to ensure that
> > arch_phys_wc_add() returned 0.
> > 
> > If you disagree it'd be great if you can elaborate why.
> 
> Maybe I'm missing something, but in qib_enable_wc() you store the return
> from arch_phys_wc_add into wc_cookie.  That return is negative,

If and only if the system was non-PAT and mtrr_add() failed.

>  so you
> return from qib_enable_wc() to qib_init_one(), they see the ret value,
> they print out a warning about bad performance, then they clear the
> return value and continue with device initialization.
> 
> In all of this though, wc_cookie is never cleared and so it still has
> the error condition in it.  Then, much later at run time, you call
> mmap_piobufs() and you check the contents of wc_cookie, and if it's
> non-0 (which is still will be), you do the wrong thing, right?

Originally the code had it to run pgprot_writecombine() if PAT was going to be
used. After the code changes we check for !cookie which will be true when
cookie is 0 only. In case the cookie was an error, that is if mtrr_add()
failed, then this code would not run because (!negative) is false. The goal was
to trigger a run if the cookie was 0, which can only happen if PAT was enabled.

Please let me know, I'd like to get this right too.

> And what
> about at shutdown when you call qib_disable_wc() and your cookie still
> has an error code in it as well?

Well fortunately arch_phys_wc_del(negative) and arch_phys_wc_del(0) will be
a no-op. Its what helps us remove so much clutter.

 Luis

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Doug Ledford]

On Wed, 2015-04-22 at 17:33 +0200, Luis R. Rodriguez wrote:
> On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> > On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> > 
> > This:
> > > + /* MTRR was used if this is non-zero */
> > > + if (!dd->wc_cookie)
> > >   vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> > 
> > And this:
> > > + dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > > + if (dd->wc_cookie < 0)
> > > + ret = -EINVAL;
> > 
> > don't agree on what wc_cookie will be on error.
> 
> Can you elaborate? The one below is the one that starts things,
> and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
> systems it will return a number > 0 *iff* a valid MTRR was added.
> It will return negative onloy on error then.
> 
> The change above is meant to replace a check put in place to see
> if PAT was enabled. The way we replace this is to ensure that
> arch_phys_wc_add() returned 0.
> 
> If you disagree it'd be great if you can elaborate why.

Maybe I'm missing something, but in qib_enable_wc() you store the return
from arch_phys_wc_add into wc_cookie.  That return is negative, so you
return from qib_enable_wc() to qib_init_one(), they see the ret value,
they print out a warning about bad performance, then they clear the
return value and continue with device initialization.

In all of this though, wc_cookie is never cleared and so it still has
the error condition in it.  Then, much later at run time, you call
mmap_piobufs() and you check the contents of wc_cookie, and if it's
non-0 (which is still will be), you do the wrong thing, right?  And what
about at shutdown when you call qib_disable_wc() and your cookie still
has an error code in it as well?

-- 
Doug Ledford 
  GPG KeyID: 0E572FDD




signature.asc
Description: This is a digitally signed message part
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

>>> On 22.04.15 at 17:36,  wrote:
> On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
>> >>> On 22.04.15 at 16:01,  wrote:
>> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
>> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
>> >> top of the current target") caused a regression for "xl mem-set"
>> >> against Dom0: While prior to creation of the first domain this works,
>> >> the first domain creation involving ballooning breaks. Due to "enforce"
>> >> not being set in the domain creation case, and due to Dom0's initial
>> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
>> >> "memorykb" in the first "xl mem-set" adusting the target upwards
>> >> subsequent to domain creation and termination may cause an overflow,
>> >> resulting in Dom0's maximum getting to a very small value. This small
>> >> maximum will the make the subsequent setting of the PoD target fail.
>> >> 
>> >> Signed-off-by: Jan Beulich 
>> >> ---
>> >> Note that this only fixes the immediate problem - there appear to be
>> >> further issues lurking here:
>> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
>> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
>> > 
>> > I think that increasing the width of these variables wouldn't break the
>> > API guarantee which we make, at least not in a practical way, since any
>> > existing 32-bit arguments passed will just get promoted.
>> 
>> No, not even on 64-bit. On 32-bit, two arguments slots are needed
>> for what so far requires only one. On 64-bit (at least x86-64), the
>> calling code isn't required to zero-extend a value calculated in a
>> register (e.g. a result of earlier calculations which had more than
>> 32 significant bits could be passed unchanged to the called function);
>> it just so happens that 32-bit arithmetic on registers would always
>> implicitly zero the upper halves (and iirc that's the same on ARM64).
> 
> You seem to be talking about ABI? As I tried to note in my response for
> libxl we only make guarantees about the API (P not B in the middle).

Oh, okay. That would mean the libxl side is fine. The change to the
libxc interface might however still be a problem for qemu (Stefano
was telling me something to the effect of a compatibility layer in
upstream qemu to deal with such version differences, but of course
the implication - without versioned libxc interfaces - would still be
that existing qemu binaries on top of updated libxc would no
longer work).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists

[David Vrabel]

From: Malcolm Crossley 

Performance analysis of aggregate network throughput with many VMs
shows that performance is signficantly limited by contention on the
maptrack lock when obtaining/releasing maptrack handles from the free
list.

Instead of a single free list use a per-VCPU list. This avoids any
contention when obtaining a handle.  Handles must be released back to
their original list and since this may occur on a different VCPU there
is some contention on the destination VCPU's free list tail pointer
(but this is much better than a per-domain lock).

A domain's maptrack limit is multiplied by the number of VCPUs.  This
ensures that a worst case domain that only performs grant table
operations via one VCPU will not see a lower map track limit.

Signed-off-by: Malcolm Crossley 
Signed-off-by: David Vrabel 
---
 xen/common/grant_table.c  |  129 -
 xen/include/xen/grant_table.h |8 ++-
 xen/include/xen/sched.h   |5 ++
 3 files changed, 86 insertions(+), 56 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 8f3d125..a237d11 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -118,9 +118,9 @@ struct gnttab_unmap_common {
 ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])
 
 static inline unsigned int
-nr_maptrack_frames(struct grant_table *t)
+nr_vcpu_maptrack_frames(struct vcpu *v)
 {
-return t->maptrack_limit / MAPTRACK_PER_PAGE;
+return v->maptrack_limit / MAPTRACK_PER_PAGE;
 }
 
 #define MAPTRACK_TAIL (~0u)
@@ -249,66 +249,91 @@ static int __get_paged_frame(unsigned long gfn, unsigned 
long *frame, struct pag
 
 static inline int
 __get_maptrack_handle(
-struct grant_table *t)
+struct grant_table *t,
+struct vcpu *v)
 {
-unsigned int h;
-if ( unlikely((h = t->maptrack_head) == MAPTRACK_TAIL) )
+unsigned int head, next;
+
+head = v->maptrack_head;
+if ( unlikely(head == MAPTRACK_TAIL) )
+return -1;
+
+next = maptrack_entry(t, head).ref;
+if ( unlikely(next == MAPTRACK_TAIL) )
 return -1;
-t->maptrack_head = maptrack_entry(t, h).ref;
-return h;
+
+v->maptrack_head = next;
+
+return head;
 }
 
 static inline void
 put_maptrack_handle(
 struct grant_table *t, int handle)
 {
-spin_lock(&t->maptrack_lock);
-maptrack_entry(t, handle).ref = t->maptrack_head;
-t->maptrack_head = handle;
-spin_unlock(&t->maptrack_lock);
+struct domain *d = current->domain;
+struct vcpu *v;
+u32 prev_tail, cur_tail;
+
+/* Set current hande to have TAIL reference */
+maptrack_entry(t, handle).ref = MAPTRACK_TAIL;
+v = d->vcpu[maptrack_entry(t,handle).vcpu];
+cur_tail = v->maptrack_tail;
+do {
+prev_tail = cur_tail;
+cur_tail = cmpxchg((u32 *)&v->maptrack_tail, prev_tail, handle);
+} while ( cur_tail != prev_tail );
+maptrack_entry(t, prev_tail).ref = handle;
 }
 
 static inline int
 get_maptrack_handle(
 struct grant_table *lgt)
 {
+struct vcpu  *v = current;
 int   i;
 grant_handle_thandle;
 struct grant_mapping *new_mt;
 unsigned int  new_mt_limit, nr_frames;
 
-spin_lock(&lgt->maptrack_lock);
-
-while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
-{
-nr_frames = nr_maptrack_frames(lgt);
-if ( nr_frames >= max_maptrack_frames )
-break;
-
-new_mt = alloc_xenheap_page();
-if ( !new_mt )
-break;
+handle = __get_maptrack_handle(lgt, v);
+if ( likely(handle != -1) )
+return handle;
 
-clear_page(new_mt);
+nr_frames = nr_vcpu_maptrack_frames(v);
+if ( nr_frames >= max_maptrack_frames )
+return -1;
 
-new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
+new_mt = alloc_xenheap_page();
+if ( !new_mt )
+return -1;
 
-for ( i = 1; i < MAPTRACK_PER_PAGE; i++ )
-new_mt[i - 1].ref = lgt->maptrack_limit + i;
-new_mt[i - 1].ref = lgt->maptrack_head;
-lgt->maptrack_head = lgt->maptrack_limit;
+clear_page(new_mt);
 
-lgt->maptrack[nr_frames] = new_mt;
-smp_wmb();
-lgt->maptrack_limit  = new_mt_limit;
+new_mt_limit = v->maptrack_limit + MAPTRACK_PER_PAGE;
 
-gdprintk(XENLOG_INFO, "Increased maptrack size to %u frames\n",
- nr_frames + 1);
+for ( i = 1; i < MAPTRACK_PER_PAGE; i++ ){
+new_mt[i - 1].ref = (lgt->maptrack_pages * MAPTRACK_PER_PAGE) + i;
+new_mt[i - 1].vcpu = v->vcpu_id;
+}
+/* Set last entry vcpu and ref */
+new_mt[i - 1].ref = v->maptrack_head;
+new_mt[i - 1].vcpu = v->vcpu_id;
+v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE;
+if (v->maptrack_tail == MAPTRACK_TAIL)
+{
+v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE)
++ MAPTRACK_PER_PAGE - 1;
+new_mt[i - 1].

Re: [Xen-devel] [PATCH v3 4/4] sched: credit2: consider per-vcpu soft affinity

[George Dunlap]

On Thu, Mar 26, 2015 at 9:48 AM, Justin T. Weaver  wrote:
[snip]
> Added a function to determine the number of soft cpus gained (or lost) by a
> given vcpu if it is migrated from a given source run queue to a given
> destination run queue.
>
> Modified algorithm in balance_load and consider...
>  * if the load on lrqd and/or orqd is less than the number of their active
>cpus, balance_load will look for vcpus that would have their soft affinity
>improved by being pushed and/or pulled. Load does not need to be considered
>since a run queue recieveing a pushed or pulled vcpu is not being fully
>utilized. This returns vcpus that may have been migrated away from their
>soft affinity due to load balancing back to their soft affinity.
>  * in consider, vcpus that might be picked for migration because pushing or
>pulling them decreases the load delta are not picked if their current run
>queue's load is less than its active cpu count and if that migration would
>harm their soft affinity. There's no need to push/pull if the load is under
>capacity, and the vcpu would lose access to some or all of its soft cpus.
>  * in consider, if a push/pull/swap migration decreases the load delta by a
>similar amount to another push/pull/swap migration, then use soft cpu gain
>as a tie breaker. This allows load to continue to balance across run 
> queues,
>but favors soft affinity gains if the load deltas are close.

OK, I've done a lot of thinking about this over the last few days, and
here's what I've come up with.

As I said before, there are basically two different activities in this
patch.  The first is to try to honor soft affinity  *given* where the
vcpu is placed; the other is to try to take soft affinity into account
when *placing* a vcpu.

The first bit is basically correct, and I've already given feedback on it.

The second half certainly represents a lot of hard work and I think
it's not a *bad* solution; but overall I'm not satisfied with it and I
think we can do better.

At a basic level, I don't like adding in the "if the load is less than
the number of pcpus then ignore load and only pull based on
softaffinity" shortcut to balance_load.  I think that it's likely that
the two halves of balance_load will have very different ideas about
what "good" looks like, and that as a result vcpus will end up getting
bounced back and forth depending on which particular algorithm ends up
being activated.

Apart from that, the whole thing is rather ad-hoc and makes it much
more complicated to understand what is going on, or predict what would
happen.

My original plan wrt load balancing was to calculate some sort of
"badness" (or perhaps "expected overhead") to the current placement
situation, and try to minimize that.  The idea was that you could then
try to boil all the factors down to a single number to optimize.  For
example, you could add in "badness" for spinning up extra cores or
sockets (or "goodness" for having them entirely empty), and then
automatically balance performance vs power consumption; or, in this
case, to balance the cost of running with remote memory accesses (as
running outside your soft affinity would normally mean) vs the cost of
waiting for the cpu.

Of course, if you're only considering load, then no matter what you
do, minimizing "badness" means minimizing load, so there's no reason
to complicate things, which is why I wrote the load balancing code the
way I did.  But I've been thinking for the last few days and I think I
can probably structure it differently to make it easier to get a
consistent evaluation of the goodness / badness of a given setup.

So what about this:

1. You re-submit this series, with the comments I've given, and the
first half of this patch (i.e., leaving out the changes to choose_cpu
and balance_load)

2. I'll work on re-working the load balancing code based on my ideas;
i.e., calculating "badness" of the load and deciding  based on that.
I'll probably try to do the soft-affinity factoring myself as a proof
of concept.

What do you think?  (Feel free to weigh in here too, Dario.)

---

Also, a few comments on the code (just for feedback):

> @@ -1207,15 +1296,75 @@ typedef struct {
>  /* NB: Modified by consider() */
>  s_time_t load_delta;
>  struct csched2_vcpu * best_push_svc, *best_pull_svc;
> +int soft_affinity_boost;
> +bool_t valid_sa_boost;
>  /* NB: Read by consider() */
>  struct csched2_runqueue_data *lrqd;
>  struct csched2_runqueue_data *orqd;
>  } balance_state_t;
>
> +/*
> + * Return the number of pcpus gained in vc's soft affinity mask that vc can
> + * run on if vc is migrated from run queue src_rqd to run queue dst_rqd.
> + */
> +static int get_soft_affinity_gain(const struct vcpu *vc,
> +  const struct csched2_runqueue_data 
> *src_rqd,
> +  const struct csched2_runqueue_data 
> *dst_rqd)
> +{
> +/*
> + * Locks

[Xen-devel] [PATCHv6 1/5] gnttab: add locking documentation

[David Vrabel]

From: Matt Wilson 

The grant table locking is becomes more fine-grained in subsequent
commits.  Describe how it will work.

Signed-off-by: Matt Wilson 
Signed-off-by: David Vrabel 
---
 docs/misc/grant-tables.txt |   35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/docs/misc/grant-tables.txt b/docs/misc/grant-tables.txt
index 19db4ec..626f40b 100644
--- a/docs/misc/grant-tables.txt
+++ b/docs/misc/grant-tables.txt
@@ -63,6 +63,7 @@ is complete.
   act->domid : remote domain being granted rights
   act->frame : machine frame being granted
   act->pin   : used to hold reference counts
+  act->lock  : spinlock used to serialize access to active entry state
 
  Map tracking
  
@@ -74,7 +75,39 @@ is complete.
  matching map track entry is then removed, as if unmap had been invoked.
  These are not used by the transfer mechanism.
   map->domid : owner of the mapped frame
-  map->ref_and_flags : grant reference, ro/rw, mapped for host or device access
+  map->ref   : grant reference
+  map->flags : ro/rw, mapped for host or device access
+
+
+ Locking
+ ~~~
+ Xen uses several locks to serialize access to the internal grant table state.
+
+  grant_table->lock  : lock used to protect grant table size, version,
+   etc. updates
+  grant_table->maptrack_lock : spinlock used to protect the maptrack state
+  active_grant_entry->lock   : spinlock used to serialize modifications to
+   active entries
+
+ The primary lock for the grant table is a spinlock. All functions
+ that modify or access members of struct grant_table must acquire the
+ lock around critical sections.  As an exception, testing that ref in
+ in range of nr_grant_frames and nr_status_frames or gt_version is
+ initialized, does _not_ require the grant table lock.  This is safe
+ because the grant table only grows and the table version is only set
+ once (to 1 or 2).  In particular, acquiring an active entry (see
+ below) does not require the grant table lock to be held.
+
+ The maptrack state is protected by its own spinlock. Any access (read
+ or write) of struct grant_table members that have a "maptrack_"
+ prefix must be made while holding the maptrack lock. The maptrack
+ state can be rapidly modified under some workloads, and the critical
+ sections are very small, thus we use a spinlock to protect them.
+
+ Active entries are obtained by calling active_entry_acquire(gt, ref).
+ This function returns a pointer to the active entry after locking its
+ spinlock. Once all access to the active entry is complete, release
+ the lock by calling active_entry_release(act).
 
 

 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCHv6 4/5] gnttab: remove unnecessary grant table locks

[David Vrabel]

From: Malcolm Crossley 

The grant table lock is not required to protect reads of the
table version, the active entry array, or the map track array.

This is safe because: a) the grant table version only changes once
from 0 to 1 or 2; b) the active entry array only grows; and c) the map
track array only grows.

Signed-off-by: Malcolm Crossley 
Signed-off-by: David Vrabel 
---
 xen/common/grant_table.c  |   76 +
 xen/include/xen/grant_table.h |8 ++---
 2 files changed, 21 insertions(+), 63 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a5470c8..8f3d125 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -196,8 +196,6 @@ active_entry_acquire(struct grant_table *t, grant_ref_t e)
 {
 struct active_grant_entry *act;
 
-ASSERT(spin_is_locked(&t->lock));
-
 act = &_active_entry(t, e);
 spin_lock(&act->lock);
 
@@ -647,7 +645,6 @@ __gnttab_map_grant_ref(
 }
 
 rgt = rd->grant_table;
-spin_lock(&rgt->lock);
 
 if ( rgt->gt_version == 0 )
 PIN_FAIL(unlock_out, GNTST_general_error,
@@ -827,7 +824,6 @@ __gnttab_map_grant_ref(
 mt->flags = op->flags;
 
 active_entry_release(act);
-spin_unlock(&rgt->lock);
 
 op->dev_bus_addr = (u64)frame << PAGE_SHIFT;
 op->handle   = handle;
@@ -869,7 +865,6 @@ __gnttab_map_grant_ref(
 active_entry_release(act);
 
  unlock_out:
-spin_unlock(&rgt->lock);
 op->status = rc;
 put_maptrack_handle(lgt, handle);
 rcu_unlock_domain(rd);
@@ -919,18 +914,15 @@ __gnttab_unmap_common(
 }
 
 op->map = &maptrack_entry(lgt, op->handle);
-spin_lock(&lgt->lock);
 
 if ( unlikely(!op->map->flags) )
 {
-spin_unlock(&lgt->lock);
 gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle);
 op->status = GNTST_bad_handle;
 return;
 }
 
 dom = op->map->domid;
-spin_unlock(&lgt->lock);
 
 if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) )
 {
@@ -952,7 +944,6 @@ __gnttab_unmap_common(
 
 rgt = rd->grant_table;
 
-spin_lock(&rgt->lock);
 act = active_entry_acquire(rgt, op->map->ref);
 
 op->flags = op->map->flags;
@@ -1023,7 +1014,6 @@ __gnttab_unmap_common(
 
  act_release_out:
 active_entry_release(act);
-spin_unlock(&rgt->lock);
 
 op->status = rc;
 rcu_unlock_domain(rd);
@@ -1055,7 +1045,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common 
*op)
 rcu_lock_domain(rd);
 rgt = rd->grant_table;
 
-spin_lock(&rgt->lock);
 if ( rgt->gt_version == 0 )
 goto unlock_out;
 
@@ -1122,8 +,6 @@ __gnttab_unmap_common_complete(struct gnttab_unmap_common 
*op)
 active_entry_release(act);
 
  unlock_out:
-spin_unlock(&rgt->lock);
-
 if ( put_handle )
 {
 op->map->flags = 0;
@@ -1277,7 +1264,7 @@ gnttab_populate_status_frames(struct domain *d, struct 
grant_table *gt,
 for ( i = nr_status_frames(gt); i < req_status_frames; i++ )
 gnttab_create_status_page(d, gt, i);
 
-gt->nr_status_frames = req_status_frames;
+atomic_set(>->nr_status_frames, req_status_frames);
 
 return 0;
 
@@ -1306,7 +1293,7 @@ gnttab_unpopulate_status_frames(struct domain *d, struct 
grant_table *gt)
 free_xenheap_page(gt->status[i]);
 gt->status[i] = NULL;
 }
-gt->nr_status_frames = 0;
+atomic_set(>->nr_status_frames, 0);
 }
 
 /* 
@@ -1356,7 +1343,7 @@ gnttab_grow_table(struct domain *d, unsigned int 
req_nr_frames)
 /* Share the new shared frames with the recipient domain */
 for ( i = nr_grant_frames(gt); i < req_nr_frames; i++ )
 gnttab_create_shared_page(d, gt, i);
-gt->nr_grant_frames = req_nr_frames;
+atomic_set(>->nr_grant_frames, req_nr_frames);
 
 return 1;
 
@@ -1423,7 +1410,17 @@ gnttab_setup_table(
 }
 
 gt = d->grant_table;
-spin_lock(>->lock);
+
+/* Tracking of mapped foreign frames table */
+if ( (gt->maptrack = xzalloc_array(struct grant_mapping *,
+   max_maptrack_frames * d->max_vcpus)) == 
NULL )
+goto out2;
+for_each_vcpu( d, v )
+{
+v->maptrack_head = MAPTRACK_TAIL;
+v->maptrack_tail = MAPTRACK_TAIL;
+}
+gt->maptrack_pages = 0;
 
 if ( gt->gt_version == 0 )
 gt->gt_version = 1;
@@ -1437,7 +1434,7 @@ gnttab_setup_table(
  "Expand grant table to %u failed. Current: %u Max: %u\n",
  op.nr_frames, nr_grant_frames(gt), max_grant_frames);
 op.status = GNTST_general_error;
-goto out3;
+goto out2;
 }
  
 op.status = GNTST_okay;
@@ -1450,8 +1447,6 @@ gnttab_setup_table(
 op.status = GNTST_bad_virt_addr;
 }
 
- out3:
-spin_unlock(>->lock);
  out2:
 rcu_unlock_domain(d);
  out1:
@@ -1525,8 +1520,6 @@ gnttab_prepare_for_transfer(
 union grant_combo   scombo, prev_scombo, new_scombo;
 int  

[Xen-devel] [PATCHv6 2/5] gnttab: introduce per-active entry locks

[David Vrabel]

From: Matt Wilson 

Instead of protecting the state of a grant table active entry with the
grant table lock, use per active entry locks.

Active entries must be acquired with active_entry_acquire() and
released with active_entry_release() which lock and unlock the entry's
spinlock.

This is the first step to improving grant table scalability and will
allow the heaviliy contented grant table lock to be used less.

Signed-off-by: Matt Wilson 
Signed-off-by: David Vrabel 
---
 xen/common/grant_table.c |  213 --
 1 file changed, 129 insertions(+), 84 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index dfb45f8..3a555f9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -157,10 +157,13 @@ struct active_grant_entry {
in the page.   */
 unsigned  length:16; /* For sub-page grants, the length of the
 grant.*/
+spinlock_tlock;  /* lock to protect access of this entry.
+see docs/misc/grant-tables.txt for
+locking protocol  */
 };
 
 #define ACGNT_PER_PAGE (PAGE_SIZE / sizeof(struct active_grant_entry))
-#define active_entry(t, e) \
+#define _active_entry(t, e) \
 ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])
 
 static inline void gnttab_flush_tlb(const struct domain *d)
@@ -188,6 +191,24 @@ nr_active_grant_frames(struct grant_table *gt)
 return num_act_frames_from_sha_frames(nr_grant_frames(gt));
 }
 
+static inline struct active_grant_entry *
+active_entry_acquire(struct grant_table *t, grant_ref_t e)
+{
+struct active_grant_entry *act;
+
+ASSERT(spin_is_locked(&t->lock));
+
+act = &_active_entry(t, e);
+spin_lock(&act->lock);
+
+return act;
+}
+
+static inline void active_entry_release(struct active_grant_entry *act)
+{
+spin_unlock(&act->lock);
+}
+
 /* Check if the page has been paged out, or needs unsharing. 
If rc == GNTST_okay, *page contains the page struct with a ref taken.
Caller must do put_page(*page).
@@ -228,30 +249,6 @@ static int __get_paged_frame(unsigned long gfn, unsigned 
long *frame, struct pag
 return rc;
 }
 
-static inline void
-double_gt_lock(struct grant_table *lgt, struct grant_table *rgt)
-{
-if ( lgt < rgt )
-{
-spin_lock(&lgt->lock);
-spin_lock(&rgt->lock);
-}
-else
-{
-if ( lgt != rgt )
-spin_lock(&rgt->lock);
-spin_lock(&lgt->lock);
-}
-}
-
-static inline void
-double_gt_unlock(struct grant_table *lgt, struct grant_table *rgt)
-{
-spin_unlock(&lgt->lock);
-if ( lgt != rgt )
-spin_unlock(&rgt->lock);
-}
-
 static inline int
 __get_maptrack_handle(
 struct grant_table *t)
@@ -505,26 +502,23 @@ static int grant_map_exists(const struct domain *ld,
 unsigned long mfn,
 unsigned int *ref_count)
 {
-const struct active_grant_entry *act;
+struct active_grant_entry *act;
 unsigned int ref, max_iter;
 
 ASSERT(spin_is_locked(&rgt->lock));
 
 max_iter = min(*ref_count + (1 << GNTTABOP_CONTINUATION_ARG_SHIFT),
nr_grant_entries(rgt));
-for ( ref = *ref_count; ref < max_iter; ref++ )
+for ( ref = *ref_count; ref < max_iter; active_entry_release(act), ref++ )
 {
-act = &active_entry(rgt, ref);
+act = active_entry_acquire(rgt, ref);
 
-if ( !act->pin )
-continue;
-
-if ( act->domid != ld->domain_id )
-continue;
-
-if ( act->frame != mfn )
+if ( !act->pin ||
+ act->domid != ld->domain_id ||
+ act->frame != mfn )
 continue;
 
+active_entry_release(act);
 return 0;
 }
 
@@ -548,11 +542,14 @@ static void mapcount(
 
 for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
 {
+struct active_grant_entry *act;
+
 map = &maptrack_entry(lgt, handle);
 if ( !(map->flags & (GNTMAP_device_map|GNTMAP_host_map)) ||
  map->domid != rd->domain_id )
 continue;
-if ( active_entry(rd->grant_table, map->ref).frame == mfn )
+act = &_active_entry(rd->grant_table, map->ref);
+if ( act->frame == mfn )
 (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
 }
 }
@@ -576,7 +573,6 @@ __gnttab_map_grant_ref(
 struct page_info *pg = NULL;
 intrc = GNTST_okay;
 u32old_pin;
-u32act_pin;
 unsigned int   cache_flags;
 struct active_grant_entry *act = NULL;
 struct grant_mapping *mt;
@@ -639,7 +635,7 @@ __gnttab_map_grant_ref(
 if ( unlikely(op->ref >= nr_grant_entries(rgt)))
 PIN_FAIL(unlock_out, GNTST_bad_gntref, "Bad ref (%d).\n", op->ref);
 
-act = &active_entry(

[Xen-devel] [PATCHv6 0/6] gnttab: Improve scaleability

[David Vrabel]

The series makes the grant table locking for fine-grained and add
per-VCPU maptrack free lists, which greatly improves scalability.

The series builds on the original series by Matt Wilson and Christoph
Egger from Amazon.

The per-VCPU maptrack free lists makes one of our aggregate intrahost
network throughput benchmarks increases from 15 Gbit/s to 75 Gbit/s,
when compared to just Amazon's original patches.

v6:
  * Remove most uses of the grant table lock.
  * Make the grant table lock a spin lock again (there were only
writers left after the above)
  * Add per-VCPU maptrack free lists.
v5:
  * Addressed locking issue pointed out by Jan Beulich
  * Fixed git rebase merge issue introduced in v4
(acquiring locking twice)
  * Change for ()-loop in grant_map_exists
  * Coding style fixes
v4:
  * Coding style nits from Jan Beulich
  * Fixup read locks pointed out by Jan Beulich
  * renamed double_gt_(un)lock to double_maptrack_(un)lock
per request from Jan Beulich
  * Addressed ASSERT()'s from Jan Beulich
  * Addressed locking issues in unmap_common pointed out
by Jan Beulich
v3:
  * Addressed gnttab_swap_grant_ref() comment from Andrew Cooper
v2:
  * Add arm part per request from Julien Grall

David


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCHv6 3/5] gnttab: split grant table lock into table and maptrack locks

[David Vrabel]

From: Matt Wilson 

The maptrack lock protects the maptrack state only.

Signed-off-by: Matt Wilson 
Signed-off-by: David Vrabel 
---
Subsequent changes make both these locks uncontented.  Is this patch
really necessary? -- dvrabel
---
 xen/common/grant_table.c  |   33 -
 xen/include/xen/grant_table.h |7 ++-
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 3a555f9..a5470c8 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -264,10 +264,10 @@ static inline void
 put_maptrack_handle(
 struct grant_table *t, int handle)
 {
-spin_lock(&t->lock);
+spin_lock(&t->maptrack_lock);
 maptrack_entry(t, handle).ref = t->maptrack_head;
 t->maptrack_head = handle;
-spin_unlock(&t->lock);
+spin_unlock(&t->maptrack_lock);
 }
 
 static inline int
@@ -279,7 +279,7 @@ get_maptrack_handle(
 struct grant_mapping *new_mt;
 unsigned int  new_mt_limit, nr_frames;
 
-spin_lock(&lgt->lock);
+spin_lock(&lgt->maptrack_lock);
 
 while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
 {
@@ -308,12 +308,15 @@ get_maptrack_handle(
  nr_frames + 1);
 }
 
-spin_unlock(&lgt->lock);
+spin_unlock(&lgt->maptrack_lock);
 
 return handle;
 }
 
-/* Number of grant table entries. Caller must hold d's grant table lock. */
+/* 
+ * Number of grant table entries. Caller must hold d's grant table
+ * read lock.
+ */
 static unsigned int nr_grant_entries(struct grant_table *gt)
 {
 ASSERT(gt->gt_version != 0);
@@ -531,6 +534,13 @@ static int grant_map_exists(const struct domain *ld,
 return -EINVAL;
 }
 
+/* 
+ * Count the number of mapped ro or rw entries tracked in the left
+ * grant table given a provided mfn provided by a foreign domain. 
+ *
+ * This function takes the maptrack lock from the left grant table and
+ * must be called with the right grant table's rwlock held.
+ */
 static void mapcount(
 struct grant_table *lgt, struct domain *rd, unsigned long mfn,
 unsigned int *wrc, unsigned int *rdc)
@@ -540,6 +550,16 @@ static void mapcount(
 
 *wrc = *rdc = 0;
 
+/* 
+ * While taking the local maptrack spinlock prevents any mapping
+ * changes, the remote active entries could be changing while we
+ * are counting. The caller has to hold the grant table lock, or
+ * some other mechanism should be used to prevent concurrent
+ * changes during this operation.
+ */
+ASSERT(spin_is_locked(&rd->grant_table->lock));
+spin_lock(&lgt->maptrack_lock);
+
 for ( handle = 0; handle < lgt->maptrack_limit; handle++ )
 {
 struct active_grant_entry *act;
@@ -552,6 +572,8 @@ static void mapcount(
 if ( act->frame == mfn )
 (map->flags & GNTMAP_readonly) ? (*rdc)++ : (*wrc)++;
 }
+
+spin_unlock(&lgt->maptrack_lock);
 }
 
 /*
@@ -2980,6 +3002,7 @@ grant_table_create(
 
 /* Simple stuff. */
 spin_lock_init(&t->lock);
+spin_lock_init(&t->maptrack_lock);
 t->nr_grant_frames = INITIAL_NR_GRANT_FRAMES;
 
 /* Active grant table. */
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 32f5786..8ff1883 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -82,7 +82,12 @@ struct grant_table {
 struct grant_mapping **maptrack;
 unsigned int  maptrack_head;
 unsigned int  maptrack_limit;
-/* Lock protecting updates to active and shared grant tables. */
+/* Lock protecting the maptrack page list, head, and limit */
+spinlock_tmaptrack_lock;
+/* 
+ * Lock protecting updates to grant table state (version, active
+ * entry list, etc.)
+ */
 spinlock_tlock;
 /* The defined versions are 1 and 2.  Set to 0 if we don't know
what version to use yet. */
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] tools/libxc: Fix build of 32bit toolstacks on CentOS 5.x following XSA-125

[Andrew Cooper]

Ping on the patches themselves?

On 13/04/15 17:37, Andrew Cooper wrote:
> On 13/04/15 17:33, Ian Jackson wrote:
>> Andrew Cooper writes ("[Xen-devel] [PATCH] tools/libxc: Fix build of 32bit 
>> toolstacks on CentOS 5.x following XSA-125"):
>>> gcc 4.1 of CentOS 5.x era does not like the typecheck in min() between
>>> uint64_t and unsigned long.
>>>
>>> Signed-off-by: Andrew Cooper 
>>> CC: Konrad Rzeszutek Wilk 
>>> CC: Ian Campbell 
>>> CC: Ian Jackson 
>>> CC: Wei Liu 
>> As and when this is acked and pushed to #staging, we should update the
>> advisory.
>>
>>> This needs backporting to 4.5
>> To be clear, do you know whether this is a trivial cherry pick ?
> It is trivial to cherrypick.
>
> 4.4 is the problematic case which I did the backport for separately, but
> then that is trivial to cherrypick back to 4.2
>
> ~Andrew
>
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 11/32] dma: fix incorrect bh scheduling

[Andrew Cooper]

From: Chunjie Zhu 

The following 2 cases should be avoided:

  1. DMAAIOCB has been released but continue_after_map_failure
 schedules a bh for it.
  2. Multiple bh calls are schduled on the same DMAAIOCB.

Signed-off-by: Chunjie Zhu 
Reviewed-by: Andrew Cooper 
---
 dma-helpers.c |   16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 2a1621b..c9fbbd9 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -50,8 +50,14 @@ typedef struct {
 target_phys_addr_t sg_cur_byte;
 QEMUIOVector iov;
 QEMUBH *bh;
+int in_use;
 } DMAAIOCB;
 
+static void dma_aio_cb_reset(DMAAIOCB *p)
+{
+p->in_use = 0;
+}
+
 static void dma_bdrv_cb(void *opaque, int ret);
 
 static void reschedule_dma(void *opaque)
@@ -60,6 +66,10 @@ static void reschedule_dma(void *opaque)
 
 qemu_bh_delete(dbs->bh);
 dbs->bh = NULL;
+
+if (!dbs->in_use)
+return;
+
 dma_bdrv_cb(opaque, 0);
 }
 
@@ -67,7 +77,8 @@ static void continue_after_map_failure(void *opaque)
 {
 DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
-dbs->bh = qemu_bh_new(reschedule_dma, dbs);
+if (!dbs->bh)
+dbs->bh = qemu_bh_new(reschedule_dma, dbs);
 qemu_bh_schedule(dbs->bh);
 }
 
@@ -97,6 +108,7 @@ void dma_bdrv_cb(void *opaque, int ret)
 dbs->common.cb(dbs->common.opaque, ret);
 qemu_iovec_destroy(&dbs->iov);
 qemu_aio_release(dbs);
+dma_aio_cb_reset(dbs);
 return;
 }
 
@@ -129,6 +141,7 @@ void dma_bdrv_cb(void *opaque, int ret)
 if (!dbs->acb) {
 dma_bdrv_unmap(dbs);
 qemu_iovec_destroy(&dbs->iov);
+dma_aio_cb_reset(dbs);
 return;
 }
 }
@@ -148,6 +161,7 @@ static BlockDriverAIOCB *dma_bdrv_io(
 dbs->sg_cur_byte = 0;
 dbs->is_write = is_write;
 dbs->bh = NULL;
+dbs->in_use = 1;
 qemu_iovec_init(&dbs->iov, sg->nsg);
 dma_bdrv_cb(dbs, 0);
 if (!dbs->acb) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 28/32] net: initialize parameters before use in net_socket_fd_init_dgram()

[Andrew Cooper]

From: Yunlei Ding 

Signed-off-by: Yunlei Ding 
Coverity-IDs: 1005339 1005340
Reviewed-by: Andrew Cooper 
---
 net.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net.c b/net.c
index 33460d8..8aba3c2 100644
--- a/net.c
+++ b/net.c
@@ -1316,9 +1316,11 @@ static NetSocketState 
*net_socket_fd_init_dgram(VLANState *vlan,
 {
 struct sockaddr_in saddr;
 int newfd;
-socklen_t saddr_len;
+socklen_t saddr_len = sizeof(saddr);
 NetSocketState *s;
 
+memset(&saddr, 0, sizeof(saddr));
+
 /* fd passed: multicast: "learn" dgram_dst address from bound address and 
save it
  * Because this may be "shared" socket from a "master" process, datagrams 
would be recv()
  * by ONLY ONE process: we must "clone" this dgram socket --jjo
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 18/32] block-cow: don't close cow_fd twice on error

[Andrew Cooper]

From: Kaifeng Zhu 

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1056200
Reviewed-by: Andrew Cooper 
---
 block-cow.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/block-cow.c b/block-cow.c
index 777d8a5..9ad0a32 100644
--- a/block-cow.c
+++ b/block-cow.c
@@ -224,7 +224,6 @@ static int cow_create(const char *filename, int64_t 
image_sectors,
 
 fd = open(image_filename, O_RDONLY | O_BINARY);
 if (fd < 0) {
-close(cow_fd);
 goto mtime_fail;
 }
 if (fstat(fd, &st) != 0) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
> >>> On 22.04.15 at 16:01,  wrote:
> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> >> top of the current target") caused a regression for "xl mem-set"
> >> against Dom0: While prior to creation of the first domain this works,
> >> the first domain creation involving ballooning breaks. Due to "enforce"
> >> not being set in the domain creation case, and due to Dom0's initial
> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> >> "memorykb" in the first "xl mem-set" adusting the target upwards
> >> subsequent to domain creation and termination may cause an overflow,
> >> resulting in Dom0's maximum getting to a very small value. This small
> >> maximum will the make the subsequent setting of the PoD target fail.
> >> 
> >> Signed-off-by: Jan Beulich 
> >> ---
> >> Note that this only fixes the immediate problem - there appear to be
> >> further issues lurking here:
> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> > 
> > I think that increasing the width of these variables wouldn't break the
> > API guarantee which we make, at least not in a practical way, since any
> > existing 32-bit arguments passed will just get promoted.
> 
> No, not even on 64-bit. On 32-bit, two arguments slots are needed
> for what so far requires only one. On 64-bit (at least x86-64), the
> calling code isn't required to zero-extend a value calculated in a
> register (e.g. a result of earlier calculations which had more than
> 32 significant bits could be passed unchanged to the called function);
> it just so happens that 32-bit arithmetic on registers would always
> implicitly zero the upper halves (and iirc that's the same on ARM64).

You seem to be talking about ABI? As I tried to note in my response for
libxl we only make guarantees about the API (P not B in the middle).

Ian.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 27/32] virtio-blk: correctly link new request in virtio_blk_load()

[Andrew Cooper]

From: Yunlei Ding 

s->rq should be set with req instead of req-next.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055910
Reviewed-by: Andrew Cooper 
---
 hw/virtio-blk.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index fcf893a..f3a81e3 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -288,7 +288,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int 
version_id)
 VirtIOBlockReq *req = virtio_blk_alloc_request(s);
 qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
 req->next = s->rq;
-s->rq = req->next;
+s->rq = req;
 }
 
 return 0;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 4/6] raisin: pass --with-system-seabios with seabios was built

[Ian Campbell]

On Wed, 2015-04-22 at 15:49 +0100, Stefano Stabellini wrote:
> On Wed, 22 Apr 2015, George Dunlap wrote:
> > Re stubdoms, I think that we should let the xen component do it until
> > it's possible to do it out of tree (i.e., no regression in functionality).
> 
> On the other hands current stubdoms are not even tested in osstest, so
> we might as well wait for the next generation to be ready before having
> them in raisin.

They are at least build tested.

And George's point about "no regression in functionality" (i.e. for
users, not test systems) is a strong one.

Unless you were suggesting to leave them to xen.git for now?

Ian.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 13/32] smbios: Don't allocate smbus eeprom buffer

[Andrew Cooper]

smbus_eeprom_device_init() has been disabled since 2007.  The #define turns
the actual function call into a comma expression with no effect.

Removing the leaked allocation also makes Valgrind happier.

Signed-off-by: Andrew Cooper 
Coverity-ID: 1090387
---
 hw/pc.c |   12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 3e33694..7359338 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -79,7 +79,6 @@ static void xen_relocator_hook(target_phys_addr_t 
*prot_addr_upd,
uint16_t protocol,
   const uint8_t header[], int kernel_size,
   target_phys_addr_t real_addr, int real_size);
-#define smbus_eeprom_device_init (void)
 
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
@@ -1135,17 +1134,8 @@ vga_bios_error:
 }
 
 if (pci_enabled && acpi_enabled) {
-uint8_t *eeprom_buf = qemu_mallocz(8 * 256); /* XXX: make this 
persistent */
-i2c_bus *smbus;
-
 /* TODO: Populate SPD eeprom data.  */
-smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100, i8259[9]);
-
-   if (smbus) {
-   for (i = 0; i < 8; i++) {
-   smbus_eeprom_device_init(smbus, 0x50 + i, eeprom_buf + (i * 
256));
-   }
-}
+piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100, i8259[9]);
 }
 
 if (i440fx_state) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 22/32] hw/ide: fix memory leak from qemu_allocate_irqs()

[Andrew Cooper]

From: Kaifeng Zhu 

qemu_allocate_irqs would return an array of irqs, not store the allocated
array pointer, and subsequently leak it.

Signed-off-by: Kaifeng Zhu 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/ide.c |2 +-
 hw/irq.c |   18 +-
 hw/irq.h |4 
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index 83e3c70..f372b7b 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -4769,7 +4769,7 @@ struct pcmcia_card_s *dscm1_init(BlockDriverState 
*bdrv)
 md->card.cis = dscm1_cis;
 md->card.cis_len = sizeof(dscm1_cis);
 
-ide_init2(md->ide, bdrv, 0, qemu_allocate_irqs(md_set_irq, md, 1)[0]);
+ide_init2(md->ide, bdrv, 0, qemu_allocate_irq(md_set_irq, md));
 md->ide->is_cf = 1;
 md->ide->mdata_size = METADATA_SIZE;
 md->ide->mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
diff --git a/hw/irq.c b/hw/irq.c
index 7703f62..c7c4864 100644
--- a/hw/irq.c
+++ b/hw/irq.c
@@ -38,6 +38,22 @@ void qemu_set_irq(qemu_irq irq, int level)
 irq->handler(irq->opaque, irq->n, level);
 }
 
+qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque)
+{
+struct IRQState *irq;
+
+irq = (struct IRQState *)qemu_mallocz(sizeof(struct IRQState));
+irq->handler = handler;
+irq->opaque = opaque;
+irq->n = 0;
+return irq;
+}
+
+void qemu_free_irq(qemu_irq irq)
+{
+qemu_free(irq);
+}
+
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
 {
 qemu_irq *s;
@@ -73,5 +89,5 @@ qemu_irq qemu_irq_invert(qemu_irq irq)
 {
 /* The default state for IRQs is low, so raise the output now.  */
 qemu_irq_raise(irq);
-return qemu_allocate_irqs(qemu_notirq, irq, 1)[0];
+return qemu_allocate_irq(qemu_notirq, irq);
 }
diff --git a/hw/irq.h b/hw/irq.h
index 5daae44..da34ae3 100644
--- a/hw/irq.h
+++ b/hw/irq.h
@@ -25,6 +25,10 @@ static inline void qemu_irq_pulse(qemu_irq irq)
 qemu_set_irq(irq, 0);
 }
 
+/* Returns one IRQ.  */
+qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque);
+void qemu_free_irq(qemu_irq irq);
+
 /* Returns an array of N IRQs.  */
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
 void qemu_free_irqs(qemu_irq *s);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] raisin: Some git-checkout improvements

[Ian Campbell]

On Wed, 2015-04-22 at 15:43 +0100, George Dunlap wrote:
> > Also wouldn't it be possible to achieve the same goal with the GIT
> > environmental variable?
> 
> A brief scan of the git man page, combined with a brief survey of
> Google, didn't turn up anything...

It's not an env variable, but in ~/.gitconfig you can use insteadOf to
prepend such a thing to all URLs, I think.

I thought I had one in my .gitconifg, but I don't, so I don't have a
testing example handy, sorry.

Ian.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 17/32] readline: fix memory corruption when adding history

[Andrew Cooper]

From: Kaifeng Zhu 

idx can be down to 0, so TERM_MAX_CMDS-idx+1 could be TERM_MAX_CMDS+1, which
exceeds the size of term_history.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055739
Reviewed-by: Andrew Cooper 
---
 readline.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/readline.c b/readline.c
index 8572841..4b68726 100644
--- a/readline.c
+++ b/readline.c
@@ -267,7 +267,7 @@ static void term_hist_add(const char *cmdline)
new_entry = hist_entry;
/* Put this entry at the end of history */
memmove(&term_history[idx], &term_history[idx + 1],
-   (TERM_MAX_CMDS - idx + 1) * sizeof(char *));
+   (TERM_MAX_CMDS - (idx + 1)) * sizeof(char *));
term_history[TERM_MAX_CMDS - 1] = NULL;
for (; idx < TERM_MAX_CMDS; idx++) {
if (term_history[idx] == NULL)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v9 02/15] libxc/progress: Extend the progress interface

[Ian Campbell]

On Mon, 2015-04-20 at 14:15 +0100, Andrew Cooper wrote:
> On 15/04/15 11:55, Ian Campbell wrote:
> > On Fri, 2015-04-10 at 18:15 +0100, Andrew Cooper wrote:
> >> Not everything which needs reporting as progress comes with a range.  
> >> Extend
> >> the interface to allow reporting of a single statement.
> >>
> >> The programming interface now looks like:
> >>   xc_set_progress_prefix()
> >> set the prefix string to be used
> >>   xc_report_progress_single()
> >> report a single action
> >>   xc_report_progress_step()
> >> report $X of $Y
> >>
> >> The new programming interface is implemented in a compatible way with the
> >> existing caller interface (by reporting a single action as "0 of 0").
> > I suppose the underlying motivation here is that there a difference
> > between this new xc_report_progress_step and calling xc_report/v? IOW
> > some difference between the semantics of the logger's ->vmessage and
> > ->progress hooks. What is it though?
> 
> They arrive via separate callbacks to the callee.
> 
> xc_report_progress & friends come through the domain build logger
> (parameter 2 of xc_interface_open()) while other logging tends to come
> through the regular logger (parameter 1 of xc_interface_open()).
> 
> Technically speaking, xc_report/v does allow the caller to choose which
> logger is used, but also requires the caller to provide all the fine
> detail, which detracts from the readability of the callsite.
> 
> This new progress API attempts to resolve this by providing a high level
> way of putting a single message through the progress logger.

Makes sense, thanks.

> > I suspected the distinction was in the automatic inclusion of
> > xch->currently_progress_reporting into the messages, but you appear to
> > make that non-mandatory below.
> >
> > Speaking of which, I think it should be mandatory now to call
> > xc_set_progress_prefix as it was to call progress_start before, and that
> > both of your new functions should assert. Those who think they want to
> > use xc_report_progress_single without calling xc_set_progress_prefix
> > should be using xc_report() instead.
> 
> Technically speaking it is safe to use a NULL prefix; the vsprintf won't
> fall over.

Having log message prefixed by "(null)" or whatever would be a bit
rubbish. I think lets keep the current semantics (by asserting things
have been set) and if there is a strong case to allow no prefix relax
things in a subsequent patch which makes everywhere DTRT.

> I can assert() that these invariants are held, but it is not critical to
> the functionality.

Yes, please add the asserts for now.

Ian.



___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 4/6] raisin: pass --with-system-seabios with seabios was built

[Stefano Stabellini]

On Wed, 22 Apr 2015, George Dunlap wrote:
> On Wed, Apr 22, 2015 at 3:49 PM, Stefano Stabellini
>  wrote:
> >> Re stubdoms, I think that we should let the xen component do it until
> >> it's possible to do it out of tree (i.e., no regression in functionality).
> >
> > On the other hands current stubdoms are not even tested in osstest, so
> > we might as well wait for the next generation to be ready before having
> > them in raisin.
> 
> I don't really get the logic here.  But it is true that we shipped 4.5
> with qemu stubdomains broken and hardly anyone noticed for what, a
> month?

Yes, this is what I was referring to.

The logic is that they are not tested, they are not enabled by default,
and in their current form they get in the way of getting rid of all git
cloning in xen-unstable. I would rather disable them until we get them
properly in raisin.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 2/2] IB/qib: use arch_phys_wc_add()

[Luis R. Rodriguez]

On Wed, Apr 22, 2015 at 09:54:38AM -0400, Doug Ledford wrote:
> On Tue, 2015-04-21 at 14:50 -0700, Luis R. Rodriguez wrote:
> 
> This:
> > +   /* MTRR was used if this is non-zero */
> > +   if (!dd->wc_cookie)
> > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> 
> And this:
> > +   dd->wc_cookie = arch_phys_wc_add(pioaddr, piolen);
> > +   if (dd->wc_cookie < 0)
> > +   ret = -EINVAL;
> 
> don't agree on what wc_cookie will be on error.

Can you elaborate? The one below is the one that starts things,
and arch_phys_wc_add() will return 0 on PAT systems. For non-PAT
systems it will return a number > 0 *iff* a valid MTRR was added.
It will return negative onloy on error then.

The change above is meant to replace a check put in place to see
if PAT was enabled. The way we replace this is to ensure that
arch_phys_wc_add() returned 0.

If you disagree it'd be great if you can elaborate why.

  Luis

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 4/6] raisin: pass --with-system-seabios with seabios was built

[George Dunlap]

On Wed, Apr 22, 2015 at 3:49 PM, Stefano Stabellini
 wrote:
>> Re stubdoms, I think that we should let the xen component do it until
>> it's possible to do it out of tree (i.e., no regression in functionality).
>
> On the other hands current stubdoms are not even tested in osstest, so
> we might as well wait for the next generation to be ready before having
> them in raisin.

I don't really get the logic here.  But it is true that we shipped 4.5
with qemu stubdomains broken and hardly anyone noticed for what, a
month?

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC PATCH COLO v5 11/29] adjust the indentation

[Ian Campbell]

On Wed, 2015-04-01 at 14:41 +0800, Yang Hongyang wrote:
> From: Wen Congyang 

I think this is just tidying up after the previous automatic renaming,
if that is the case please can you say so.

> 
> Signed-off-by: Wen Congyang 
> ---
>  tools/libxl/libxl_checkpoint_device.c | 23 ---
>  tools/libxl/libxl_internal.h  | 21 -
>  tools/libxl/libxl_remus.c | 12 
>  3 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxl/libxl_checkpoint_device.c 
> b/tools/libxl/libxl_checkpoint_device.c
> index 109cd23..0cfabc3 100644
> --- a/tools/libxl/libxl_checkpoint_device.c
> +++ b/tools/libxl/libxl_checkpoint_device.c
> @@ -73,9 +73,9 @@ static void devices_teardown_cb(libxl__egc *egc,
>  /* checkpoint device setup and teardown */
>  
>  static libxl__checkpoint_device* checkpoint_device_init(libxl__egc *egc,
> -  
> libxl__checkpoint_devices_state *cds,
> -  libxl__device_kind kind,
> -  void *libxl_dev)
> +libxl__checkpoint_devices_state *cds,
> +libxl__device_kind kind,
> +void *libxl_dev)
>  {
>  libxl__checkpoint_device *dev = NULL;
>  
> @@ -89,9 +89,10 @@ static libxl__checkpoint_device* 
> checkpoint_device_init(libxl__egc *egc,
>  }
>  
>  static void checkpoint_devices_setup(libxl__egc *egc,
> -libxl__checkpoint_devices_state *cds);
> + libxl__checkpoint_devices_state *cds);
>  
> -void libxl__checkpoint_devices_setup(libxl__egc *egc, 
> libxl__checkpoint_devices_state *cds)
> +void libxl__checkpoint_devices_setup(libxl__egc *egc,
> + libxl__checkpoint_devices_state *cds)
>  {
>  int i, rc;
>  
> @@ -137,7 +138,7 @@ out:
>  }
>  
>  static void checkpoint_devices_setup(libxl__egc *egc,
> -libxl__checkpoint_devices_state *cds)
> + libxl__checkpoint_devices_state *cds)
>  {
>  int i, rc;
>  
> @@ -223,7 +224,7 @@ static void all_devices_setup_cb(libxl__egc *egc,
>  }
>  
>  void libxl__checkpoint_devices_teardown(libxl__egc *egc,
> -   libxl__checkpoint_devices_state *cds)
> +libxl__checkpoint_devices_state *cds)
>  {
>  int i;
>  libxl__checkpoint_device *dev;
> @@ -285,12 +286,12 @@ static void devices_checkpoint_cb(libxl__egc *egc,
>  
>  /* API implementations */
>  
> -#define define_checkpoint_api(api)\
> -void libxl__checkpoint_devices_##api(libxl__egc *egc,
> \
> -libxl__checkpoint_devices_state *cds)
> \
> +#define define_checkpoint_api(api)  \
> +void libxl__checkpoint_devices_##api(libxl__egc *egc,   \
> +libxl__checkpoint_devices_state *cds)   \
>  {   \
>  int i;  \
> -libxl__checkpoint_device *dev;   
> \
> +libxl__checkpoint_device *dev;  \
>  \
>  STATE_AO_GC(cds->ao);   \
>  \
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 7e7c3b3..4b8590c 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2652,7 +2652,8 @@ typedef struct libxl__save_helper_state {
>   * Each device type needs to implement the interfaces specified in
>   * the libxl__checkpoint_device_instance_ops if it wishes to support Remus.
>   *
> - * The high-level control flow through the checkpoint device layer is shown 
> below:
> + * The high-level control flow through the checkpoint device layer is shown
> + * below:
>   *
>   * xl remus
>   *  |->  libxl_domain_remus_start
> @@ -2713,7 +2714,8 @@ int 
> init_subkind_drbd_disk(libxl__checkpoint_devices_state *cds);
>  void cleanup_subkind_drbd_disk(libxl__checkpoint_devices_state *cds);
>  
>  typedef void libxl__checkpoint_callback(libxl__egc *,
> -   libxl__checkpoint_devices_state *, int 
> rc);
> +libxl__checkpoint_devices_state *,
> +int rc);
>  
>  /*
>   * State associated with a checkpoint invocation, including parameters
> @@ -2721,7 +2723,7 @@ typedef void libxl__checkpoint_callback(libxl__egc *,
>   * save/restore machinery.
>   */
>  struct libxl__checkpoint_devices_state {
> -/*---

[Xen-devel] [PATCH 27/32] virtio-blk: correctly link new request in virtio_blk_load()

[Andrew Cooper]

From: Yunlei Ding 

s->rq should be set with req instead of req-next.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055910
Reviewed-by: Andrew Cooper 
---
 hw/virtio-blk.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index fcf893a..f3a81e3 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -288,7 +288,7 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int 
version_id)
 VirtIOBlockReq *req = virtio_blk_alloc_request(s);
 qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem));
 req->next = s->rq;
-s->rq = req->next;
+s->rq = req;
 }
 
 return 0;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 21/32] qemu-char: fix memory leak in qemu_char_open_pty()

[Andrew Cooper]

From: Kaifeng Zhu 

The momery pointed by s and chr could be leaked if openpty return a value
less then 0.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055926 1055927
Reviewed-by: Andrew Cooper 
---
 qemu-char.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 324ed16..f62a6af 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -932,6 +932,8 @@ static CharDriverState *qemu_chr_open_pty(void)
 s = qemu_mallocz(sizeof(PtyCharDriver));
 
 if (openpty(&s->fd, &slave_fd, pty_name, NULL, NULL) < 0) {
+qemu_free(s);
+qemu_free(chr);
 return NULL;
 }
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 13/32] smbios: Don't allocate smbus eeprom buffer

[Andrew Cooper]

smbus_eeprom_device_init() has been disabled since 2007.  The #define turns
the actual function call into a comma expression with no effect.

Removing the leaked allocation also makes Valgrind happier.

Signed-off-by: Andrew Cooper 
Coverity-ID: 1090387
---
 hw/pc.c |   12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 3e33694..7359338 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -79,7 +79,6 @@ static void xen_relocator_hook(target_phys_addr_t 
*prot_addr_upd,
uint16_t protocol,
   const uint8_t header[], int kernel_size,
   target_phys_addr_t real_addr, int real_size);
-#define smbus_eeprom_device_init (void)
 
 static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
 {
@@ -1135,17 +1134,8 @@ vga_bios_error:
 }
 
 if (pci_enabled && acpi_enabled) {
-uint8_t *eeprom_buf = qemu_mallocz(8 * 256); /* XXX: make this 
persistent */
-i2c_bus *smbus;
-
 /* TODO: Populate SPD eeprom data.  */
-smbus = piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100, i8259[9]);
-
-   if (smbus) {
-   for (i = 0; i < 8; i++) {
-   smbus_eeprom_device_init(smbus, 0x50 + i, eeprom_buf + (i * 
256));
-   }
-}
+piix4_pm_init(pci_bus, piix3_devfn + 3, 0xb100, i8259[9]);
 }
 
 if (i440fx_state) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 30/32] block-nbd: close sock in nbd_open() error path

[Andrew Cooper]

From: Yunlei Ding 

Close sock handle before return.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055914
Reviewed-by: Andrew Cooper 
---
 block-nbd.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/block-nbd.c b/block-nbd.c
index dc63183..e2c90eb 100644
--- a/block-nbd.c
+++ b/block-nbd.c
@@ -88,7 +88,10 @@ static int nbd_open(BlockDriverState *bs, const char* 
filename, int flags)
 
 ret = nbd_receive_negotiate(sock, &size, &blocksize);
 if (ret == -1)
+{
+close(sock);
 return -errno;
+}
 
 s->sock = sock;
 s->size = size;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 24/32] net: Fix memory/handle leaks in net_socket_listen_init()

[Andrew Cooper]

From: Kaifeng Zhu 

fd and s could be leaked in case bind/listen failed.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055923 1055924
Reviewed-by: Andrew Cooper 
---
 net.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/net.c b/net.c
index f3887a7..33460d8 100644
--- a/net.c
+++ b/net.c
@@ -1460,6 +1460,7 @@ static int net_socket_listen_init(VLANState *vlan,
 fd = socket(PF_INET, SOCK_STREAM, 0);
 if (fd < 0) {
 perror("socket");
+qemu_free(s);
 return -1;
 }
 socket_set_nonblock(fd);
@@ -1471,11 +1472,15 @@ static int net_socket_listen_init(VLANState *vlan,
 ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
 if (ret < 0) {
 perror("bind");
+closesocket(fd);
+qemu_free(s);
 return -1;
 }
 ret = listen(fd, 0);
 if (ret < 0) {
 perror("listen");
+closesocket(fd);
+qemu_free(s);
 return -1;
 }
 s->vlan = vlan;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 19/32] console: Avoid overrunning the dmask arrays

[Andrew Cooper]

From: Kaifeng Zhu 

The valide range of font_data should be [0, 0xFF].

Signed-off-by: Kaifeng Zhu 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 console.c |9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/console.c b/console.c
index 9984d6f..d4f1ad0 100644
--- a/console.c
+++ b/console.c
@@ -421,7 +421,8 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 {
 uint8_t *d;
 const uint8_t *font_ptr;
-unsigned int font_data, linesize, xorcol, bpp;
+uint8_t font_data;
+unsigned int linesize, xorcol, bpp;
 int i;
 unsigned int fgcol, bgcol;
 
@@ -450,7 +451,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 font_data = *font_ptr++;
 if (t_attrib->uline
 && ((i == FONT_HEIGHT - 2) || (i == FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (dmask16[(font_data >> 4)] & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (dmask16[(font_data >> 0) & 0xf] & xorcol) ^ 
bgcol;
@@ -463,7 +464,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 font_data = *font_ptr++;
 if (t_attrib->uline
 && ((i == FONT_HEIGHT - 2) || (i == FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (dmask4[(font_data >> 6)] & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (dmask4[(font_data >> 4) & 3] & xorcol) ^ 
bgcol;
@@ -476,7 +477,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 for(i = 0; i < FONT_HEIGHT; i++) {
 font_data = *font_ptr++;
 if (t_attrib->uline && ((i == FONT_HEIGHT - 2) || (i == 
FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (-((font_data >> 7)) & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (-((font_data >> 6) & 1) & xorcol) ^ bgcol;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 23/32] net: don't leak an fd after an error

[Andrew Cooper]

From: Kaifeng Zhu 

fd will be leaked if launch_script failed.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055925
Reviewed-by: Andrew Cooper 
---
 net.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net.c b/net.c
index 720027c..f3887a7 100644
--- a/net.c
+++ b/net.c
@@ -1049,8 +1049,10 @@ static int net_tap_init(VLANState *vlan, const char 
*model,
 if (!setup_script || !strcmp(setup_script, "no"))
 setup_script = "";
 if (setup_script[0] != '\0') {
-   if (launch_script(setup_script, ifname, script_arg, fd))
+   if (launch_script(setup_script, ifname, script_arg, fd)) {
+   close(fd);
return -1;
+   }
 }
 s = net_tap_fd_init(vlan, model, name, fd);
 if (!s)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 22/32] hw/ide: fix memory leak from qemu_allocate_irqs()

[Andrew Cooper]

From: Kaifeng Zhu 

qemu_allocate_irqs would return an array of irqs, not store the allocated
array pointer, and subsequently leak it.

Signed-off-by: Kaifeng Zhu 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/ide.c |2 +-
 hw/irq.c |   18 +-
 hw/irq.h |4 
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index 83e3c70..f372b7b 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -4769,7 +4769,7 @@ struct pcmcia_card_s *dscm1_init(BlockDriverState 
*bdrv)
 md->card.cis = dscm1_cis;
 md->card.cis_len = sizeof(dscm1_cis);
 
-ide_init2(md->ide, bdrv, 0, qemu_allocate_irqs(md_set_irq, md, 1)[0]);
+ide_init2(md->ide, bdrv, 0, qemu_allocate_irq(md_set_irq, md));
 md->ide->is_cf = 1;
 md->ide->mdata_size = METADATA_SIZE;
 md->ide->mdata_storage = (uint8_t *) qemu_mallocz(METADATA_SIZE);
diff --git a/hw/irq.c b/hw/irq.c
index 7703f62..c7c4864 100644
--- a/hw/irq.c
+++ b/hw/irq.c
@@ -38,6 +38,22 @@ void qemu_set_irq(qemu_irq irq, int level)
 irq->handler(irq->opaque, irq->n, level);
 }
 
+qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque)
+{
+struct IRQState *irq;
+
+irq = (struct IRQState *)qemu_mallocz(sizeof(struct IRQState));
+irq->handler = handler;
+irq->opaque = opaque;
+irq->n = 0;
+return irq;
+}
+
+void qemu_free_irq(qemu_irq irq)
+{
+qemu_free(irq);
+}
+
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n)
 {
 qemu_irq *s;
@@ -73,5 +89,5 @@ qemu_irq qemu_irq_invert(qemu_irq irq)
 {
 /* The default state for IRQs is low, so raise the output now.  */
 qemu_irq_raise(irq);
-return qemu_allocate_irqs(qemu_notirq, irq, 1)[0];
+return qemu_allocate_irq(qemu_notirq, irq);
 }
diff --git a/hw/irq.h b/hw/irq.h
index 5daae44..da34ae3 100644
--- a/hw/irq.h
+++ b/hw/irq.h
@@ -25,6 +25,10 @@ static inline void qemu_irq_pulse(qemu_irq irq)
 qemu_set_irq(irq, 0);
 }
 
+/* Returns one IRQ.  */
+qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque);
+void qemu_free_irq(qemu_irq irq);
+
 /* Returns an array of N IRQs.  */
 qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void *opaque, int n);
 void qemu_free_irqs(qemu_irq *s);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 26/32] block-vvfat: fix memory leak in check_directory_consistency()

[Andrew Cooper]

From: Kaifeng Zhu 

Memory pointed by cluster leaks in error handling code.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055917
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block-vvfat.c b/block-vvfat.c
index ec3363c..6cd57a2 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -1769,7 +1769,7 @@ static int check_directory_consistency(BDRVVVFATState *s,
 
if (s->used_clusters[cluster_num] & USED_ANY) {
fprintf(stderr, "cluster %d used more than once\n", 
(int)cluster_num);
-   return 0;
+   goto fail;
}
s->used_clusters[cluster_num] = USED_DIRECTORY;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 32/32] block-vvfat: fix resource leaks in read_directory()

[Andrew Cooper]

From: Yunlei Ding 

Signed-off-by: Yunlei Ding 
Coverity-IDs: 1055920 1055921
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/block-vvfat.c b/block-vvfat.c
index 6cd57a2..ff5c8bf 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -760,6 +760,7 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 if (st.st_size > 0x7fff) {
fprintf(stderr, "File %s is larger than 2GB\n", buffer);
free(buffer);
+closedir(dir);
return -2;
 }
direntry->size=cpu_to_le32(S_ISDIR(st.st_mode)?0:st.st_size);
@@ -787,6 +788,8 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
s->current_mapping->read_only =
(st.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) == 0;
}
+else
+qemu_free(buffer);
 }
 closedir(dir);
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 31/32] block-raw-posix: Fix memory leak in posix_aio_init()

[Andrew Cooper]

From: Yunlei Ding 

Free allocated memory s before return.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055915
Reviewed-by: Andrew Cooper 
---
 block-raw-posix.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 795cd5b..8a1baa8 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -602,6 +602,7 @@ static int posix_aio_init(void)
 s->first_aio = NULL;
 if (pipe(fds) == -1) {
 fprintf(stderr, "failed to create pipe\n");
+qemu_free(s);
 return -errno;
 }
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 12/32] cmdline: Parse -pciemulation before trying to use it

[Andrew Cooper]

From: Kaifeng Zhu 

Signed-off-by: Kaifeng Zhu 
Reviewed-by: Andrew Cooper 
---
 vl.c |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index d21c3aa..67d9d86 100644
--- a/vl.c
+++ b/vl.c
@@ -5952,6 +5952,15 @@ int main(int argc, char **argv, char **envp)
 }
 }
 
+#ifdef CONFIG_PASSTHROUGH
+for (i = 0; i < nb_pci_emulation; i++) {
+if (pci_emulation_add(pci_emulation_config_text[i]) < 0) {
+fprintf(stderr, "Warning: could not add PCI device %s\n",
+pci_emulation_config_text[i]);
+}
+}
+#endif
+
 machine->init(ram_size, vga_ram_size, boot_devices,
   kernel_filename, kernel_cmdline, initrd_filename, cpu_model,
  direct_pci);
@@ -6068,15 +6077,6 @@ int main(int argc, char **argv, char **envp)
 }
 }
 
-#ifdef CONFIG_PASSTHROUGH
-for (i = 0; i < nb_pci_emulation; i++) {
-if (pci_emulation_add(pci_emulation_config_text[i]) < 0) {
-fprintf(stderr, "Warning: could not add PCI device %s\n",
-pci_emulation_config_text[i]);
-}
-}
-#endif
-
 for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
 const char *devname = virtio_consoles[i];
 if (virtcon_hds[i] && devname) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 16/32] hw/msmouse.c: Fix deref_after_free and double free

[Andrew Cooper]

From: Yunlei Ding 

msmouse_chr_close is only pointed by chr->chr_close in qemu_chr_close
function. After calling chr->chr_close, chr will be freed. So we don't
need to free it again here.

Signed-off-by: Yunlei Ding 
(defect not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/msmouse.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/msmouse.c b/hw/msmouse.c
index 69356a5..2d2703b 100644
--- a/hw/msmouse.c
+++ b/hw/msmouse.c
@@ -61,7 +61,6 @@ static int msmouse_chr_write (struct CharDriverState *s, 
const uint8_t *buf, int
 
 static void msmouse_chr_close (struct CharDriverState *chr)
 {
-qemu_free (chr);
 }
 
 CharDriverState *qemu_chr_open_msmouse(void)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 15/32] signal: Don't use uninitalised sival_ptr

[Andrew Cooper]

In 64bit builds, setting sival_int to 0 doesn't clear the upper half of the
sival_ptr pointer.  Valgrind does not like this.

Signed-off-by: Andrew Cooper 
---
 vl.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 67d9d86..c864e7d 100644
--- a/vl.c
+++ b/vl.c
@@ -1568,7 +1568,7 @@ static void rtc_stop_timer(struct qemu_alarm_timer *t)
 
 static int dynticks_start_timer(struct qemu_alarm_timer *t)
 {
-struct sigevent ev;
+struct sigevent ev = { { 0 } };
 timer_t host_timer;
 struct sigaction act;
 
@@ -1578,7 +1578,6 @@ static int dynticks_start_timer(struct qemu_alarm_timer 
*t)
 
 sigaction(SIGALRM, &act, NULL);
 
-ev.sigev_value.sival_int = 0;
 ev.sigev_notify = SIGEV_SIGNAL;
 ev.sigev_signo = SIGALRM;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 23/32] net: don't leak an fd after an error

[Andrew Cooper]

From: Kaifeng Zhu 

fd will be leaked if launch_script failed.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055925
Reviewed-by: Andrew Cooper 
---
 net.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net.c b/net.c
index 720027c..f3887a7 100644
--- a/net.c
+++ b/net.c
@@ -1049,8 +1049,10 @@ static int net_tap_init(VLANState *vlan, const char 
*model,
 if (!setup_script || !strcmp(setup_script, "no"))
 setup_script = "";
 if (setup_script[0] != '\0') {
-   if (launch_script(setup_script, ifname, script_arg, fd))
+   if (launch_script(setup_script, ifname, script_arg, fd)) {
+   close(fd);
return -1;
+   }
 }
 s = net_tap_fd_init(vlan, model, name, fd);
 if (!s)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 32/32] block-vvfat: fix resource leaks in read_directory()

[Andrew Cooper]

From: Yunlei Ding 

Signed-off-by: Yunlei Ding 
Coverity-IDs: 1055920 1055921
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/block-vvfat.c b/block-vvfat.c
index 6cd57a2..ff5c8bf 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -760,6 +760,7 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
 if (st.st_size > 0x7fff) {
fprintf(stderr, "File %s is larger than 2GB\n", buffer);
free(buffer);
+closedir(dir);
return -2;
 }
direntry->size=cpu_to_le32(S_ISDIR(st.st_mode)?0:st.st_size);
@@ -787,6 +788,8 @@ static int read_directory(BDRVVVFATState* s, int 
mapping_index)
s->current_mapping->read_only =
(st.st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) == 0;
}
+else
+qemu_free(buffer);
 }
 closedir(dir);
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 15/32] signal: Don't use uninitalised sival_ptr

[Andrew Cooper]

In 64bit builds, setting sival_int to 0 doesn't clear the upper half of the
sival_ptr pointer.  Valgrind does not like this.

Signed-off-by: Andrew Cooper 
---
 vl.c |3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 67d9d86..c864e7d 100644
--- a/vl.c
+++ b/vl.c
@@ -1568,7 +1568,7 @@ static void rtc_stop_timer(struct qemu_alarm_timer *t)
 
 static int dynticks_start_timer(struct qemu_alarm_timer *t)
 {
-struct sigevent ev;
+struct sigevent ev = { { 0 } };
 timer_t host_timer;
 struct sigaction act;
 
@@ -1578,7 +1578,6 @@ static int dynticks_start_timer(struct qemu_alarm_timer 
*t)
 
 sigaction(SIGALRM, &act, NULL);
 
-ev.sigev_value.sival_int = 0;
 ev.sigev_notify = SIGEV_SIGNAL;
 ev.sigev_signo = SIGALRM;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 29/32] ide: don't leak irq array in pci_cmd646_ide_init()

[Andrew Cooper]

From: Yunlei Ding 

Call qemu_allocate_irq() twice instead of qemu_allocate_irqs to
allocate memory.

Signed-off-by: Yunlei Ding 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/ide.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index f372b7b..e7b9f24 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -3652,7 +3652,6 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState 
**hd_table,
 PCIIDEState *d;
 uint8_t *pci_conf;
 int i;
-qemu_irq *irq;
 
 d = (PCIIDEState *)pci_register_device(bus, "CMD646 IDE",
sizeof(PCIIDEState),
@@ -3694,9 +3693,8 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState 
**hd_table,
 for(i = 0; i < 4; i++)
 d->ide_if[i].pci_dev = (PCIDevice *)d;
 
-irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], irq[0]);
-ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], irq[1]);
+ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], 
qemu_allocate_irq(cmd646_set_irq, d));
+ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], 
qemu_allocate_irq(cmd646_set_irq, d));
 
 register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 qemu_register_reset(cmd646_reset, d);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 28/32] net: initialize parameters before use in net_socket_fd_init_dgram()

[Andrew Cooper]

From: Yunlei Ding 

Signed-off-by: Yunlei Ding 
Coverity-IDs: 1005339 1005340
Reviewed-by: Andrew Cooper 
---
 net.c |4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net.c b/net.c
index 33460d8..8aba3c2 100644
--- a/net.c
+++ b/net.c
@@ -1316,9 +1316,11 @@ static NetSocketState 
*net_socket_fd_init_dgram(VLANState *vlan,
 {
 struct sockaddr_in saddr;
 int newfd;
-socklen_t saddr_len;
+socklen_t saddr_len = sizeof(saddr);
 NetSocketState *s;
 
+memset(&saddr, 0, sizeof(saddr));
+
 /* fd passed: multicast: "learn" dgram_dst address from bound address and 
save it
  * Because this may be "shared" socket from a "master" process, datagrams 
would be recv()
  * by ONLY ONE process: we must "clone" this dgram socket --jjo
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 18/32] block-cow: don't close cow_fd twice on error

[Andrew Cooper]

From: Kaifeng Zhu 

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1056200
Reviewed-by: Andrew Cooper 
---
 block-cow.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/block-cow.c b/block-cow.c
index 777d8a5..9ad0a32 100644
--- a/block-cow.c
+++ b/block-cow.c
@@ -224,7 +224,6 @@ static int cow_create(const char *filename, int64_t 
image_sectors,
 
 fd = open(image_filename, O_RDONLY | O_BINARY);
 if (fd < 0) {
-close(cow_fd);
 goto mtime_fail;
 }
 if (fstat(fd, &st) != 0) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 14/32] pic: Don't allocate irq buffers

[Andrew Cooper]

i8259_init() doesn't inspect its argument at all, causing the allocation to
be leaked and never used.

Signed-off-by: Andrew Cooper 
---
 hw/pc.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 7359338..09b4af4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -801,7 +801,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 PCIBus *pci_bus;
 int piix3_devfn = -1;
 CPUState *env;
-qemu_irq *cpu_irq;
 qemu_irq *i8259;
 int index;
 BlockDriverState *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -970,8 +969,7 @@ vga_bios_error:
 
 bochs_bios_init();
 
-cpu_irq = qemu_allocate_irqs(pic_irq_request, NULL, 1);
-i8259 = i8259_init(cpu_irq[0]);
+i8259 = i8259_init(NULL);
 ferr_irq = i8259[13];
 
 if (pci_enabled) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 31/32] block-raw-posix: Fix memory leak in posix_aio_init()

[Andrew Cooper]

From: Yunlei Ding 

Free allocated memory s before return.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055915
Reviewed-by: Andrew Cooper 
---
 block-raw-posix.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 795cd5b..8a1baa8 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -602,6 +602,7 @@ static int posix_aio_init(void)
 s->first_aio = NULL;
 if (pipe(fds) == -1) {
 fprintf(stderr, "failed to create pipe\n");
+qemu_free(s);
 return -errno;
 }
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 19/32] console: Avoid overrunning the dmask arrays

[Andrew Cooper]

From: Kaifeng Zhu 

The valide range of font_data should be [0, 0xFF].

Signed-off-by: Kaifeng Zhu 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 console.c |9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/console.c b/console.c
index 9984d6f..d4f1ad0 100644
--- a/console.c
+++ b/console.c
@@ -421,7 +421,8 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 {
 uint8_t *d;
 const uint8_t *font_ptr;
-unsigned int font_data, linesize, xorcol, bpp;
+uint8_t font_data;
+unsigned int linesize, xorcol, bpp;
 int i;
 unsigned int fgcol, bgcol;
 
@@ -450,7 +451,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 font_data = *font_ptr++;
 if (t_attrib->uline
 && ((i == FONT_HEIGHT - 2) || (i == FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (dmask16[(font_data >> 4)] & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (dmask16[(font_data >> 0) & 0xf] & xorcol) ^ 
bgcol;
@@ -463,7 +464,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 font_data = *font_ptr++;
 if (t_attrib->uline
 && ((i == FONT_HEIGHT - 2) || (i == FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (dmask4[(font_data >> 6)] & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (dmask4[(font_data >> 4) & 3] & xorcol) ^ 
bgcol;
@@ -476,7 +477,7 @@ static void vga_putcharxy(DisplayState *ds, int x, int y, 
int ch,
 for(i = 0; i < FONT_HEIGHT; i++) {
 font_data = *font_ptr++;
 if (t_attrib->uline && ((i == FONT_HEIGHT - 2) || (i == 
FONT_HEIGHT - 3))) {
-font_data = 0x;
+font_data = 0xFF;
 }
 ((uint32_t *)d)[0] = (-((font_data >> 7)) & xorcol) ^ bgcol;
 ((uint32_t *)d)[1] = (-((font_data >> 6) & 1) & xorcol) ^ bgcol;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 25/32] block-vvfat: fix memory/handle leaks in commit_one_file()

[Andrew Cooper]

From: Kaifeng Zhu 

Some handles and memory in commit_one_file are going to be leaked if
certain function calls failed.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055918 1055919
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |   17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block-vvfat.c b/block-vvfat.c
index 345d7be..ec3363c 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -2229,11 +2229,15 @@ static int commit_one_file(BDRVVVFATState* s,
 if (fd < 0) {
fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
strerror(errno), errno);
+   qemu_free(cluster);
return fd;
 }
 if (offset > 0)
-   if (lseek(fd, offset, SEEK_SET) != offset)
+   if (lseek(fd, offset, SEEK_SET) != offset) {
+   close(fd);
+   qemu_free(cluster);
return -3;
+   }
 
 while (offset < size) {
uint32_t c1;
@@ -2249,11 +2253,17 @@ static int commit_one_file(BDRVVVFATState* s,
ret = vvfat_read(s->bs, cluster2sector(s, c),
(uint8_t*)cluster, (rest_size + 0x1ff) / 0x200);
 
-   if (ret < 0)
+   if (ret < 0) {
+   close(fd);
+   qemu_free(cluster);
return ret;
+   }
 
-   if (qemu_write_ok(fd, cluster, rest_size) < 0)
+   if (qemu_write_ok(fd, cluster, rest_size) < 0) {
+   close(fd);
+   qemu_free(cluster);
return -2;
+   }
 
offset += rest_size;
c = c1;
@@ -2261,6 +2271,7 @@ static int commit_one_file(BDRVVVFATState* s,
 
 ftruncate(fd, size);
 close(fd);
+qemu_free(cluster);
 
 return commit_mappings(s, first_cluster, dir_index);
 }
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 21/32] qemu-char: fix memory leak in qemu_char_open_pty()

[Andrew Cooper]

From: Kaifeng Zhu 

The momery pointed by s and chr could be leaked if openpty return a value
less then 0.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055926 1055927
Reviewed-by: Andrew Cooper 
---
 qemu-char.c |2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-char.c b/qemu-char.c
index 324ed16..f62a6af 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -932,6 +932,8 @@ static CharDriverState *qemu_chr_open_pty(void)
 s = qemu_mallocz(sizeof(PtyCharDriver));
 
 if (openpty(&s->fd, &slave_fd, pty_name, NULL, NULL) < 0) {
+qemu_free(s);
+qemu_free(chr);
 return NULL;
 }
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 11/32] dma: fix incorrect bh scheduling

[Andrew Cooper]

From: Chunjie Zhu 

The following 2 cases should be avoided:

  1. DMAAIOCB has been released but continue_after_map_failure
 schedules a bh for it.
  2. Multiple bh calls are schduled on the same DMAAIOCB.

Signed-off-by: Chunjie Zhu 
Reviewed-by: Andrew Cooper 
---
 dma-helpers.c |   16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 2a1621b..c9fbbd9 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -50,8 +50,14 @@ typedef struct {
 target_phys_addr_t sg_cur_byte;
 QEMUIOVector iov;
 QEMUBH *bh;
+int in_use;
 } DMAAIOCB;
 
+static void dma_aio_cb_reset(DMAAIOCB *p)
+{
+p->in_use = 0;
+}
+
 static void dma_bdrv_cb(void *opaque, int ret);
 
 static void reschedule_dma(void *opaque)
@@ -60,6 +66,10 @@ static void reschedule_dma(void *opaque)
 
 qemu_bh_delete(dbs->bh);
 dbs->bh = NULL;
+
+if (!dbs->in_use)
+return;
+
 dma_bdrv_cb(opaque, 0);
 }
 
@@ -67,7 +77,8 @@ static void continue_after_map_failure(void *opaque)
 {
 DMAAIOCB *dbs = (DMAAIOCB *)opaque;
 
-dbs->bh = qemu_bh_new(reschedule_dma, dbs);
+if (!dbs->bh)
+dbs->bh = qemu_bh_new(reschedule_dma, dbs);
 qemu_bh_schedule(dbs->bh);
 }
 
@@ -97,6 +108,7 @@ void dma_bdrv_cb(void *opaque, int ret)
 dbs->common.cb(dbs->common.opaque, ret);
 qemu_iovec_destroy(&dbs->iov);
 qemu_aio_release(dbs);
+dma_aio_cb_reset(dbs);
 return;
 }
 
@@ -129,6 +141,7 @@ void dma_bdrv_cb(void *opaque, int ret)
 if (!dbs->acb) {
 dma_bdrv_unmap(dbs);
 qemu_iovec_destroy(&dbs->iov);
+dma_aio_cb_reset(dbs);
 return;
 }
 }
@@ -148,6 +161,7 @@ static BlockDriverAIOCB *dma_bdrv_io(
 dbs->sg_cur_byte = 0;
 dbs->is_write = is_write;
 dbs->bh = NULL;
+dbs->in_use = 1;
 qemu_iovec_init(&dbs->iov, sg->nsg);
 dma_bdrv_cb(dbs, 0);
 if (!dbs->acb) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 29/32] ide: don't leak irq array in pci_cmd646_ide_init()

[Andrew Cooper]

From: Yunlei Ding 

Call qemu_allocate_irq() twice instead of qemu_allocate_irqs to
allocate memory.

Signed-off-by: Yunlei Ding 
(defects not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/ide.c |6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index f372b7b..e7b9f24 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -3652,7 +3652,6 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState 
**hd_table,
 PCIIDEState *d;
 uint8_t *pci_conf;
 int i;
-qemu_irq *irq;
 
 d = (PCIIDEState *)pci_register_device(bus, "CMD646 IDE",
sizeof(PCIIDEState),
@@ -3694,9 +3693,8 @@ void pci_cmd646_ide_init(PCIBus *bus, BlockDriverState 
**hd_table,
 for(i = 0; i < 4; i++)
 d->ide_if[i].pci_dev = (PCIDevice *)d;
 
-irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], irq[0]);
-ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], irq[1]);
+ide_init2(&d->ide_if[0], hd_table[0], hd_table[1], 
qemu_allocate_irq(cmd646_set_irq, d));
+ide_init2(&d->ide_if[2], hd_table[2], hd_table[3], 
qemu_allocate_irq(cmd646_set_irq, d));
 
 register_savevm("ide", 0, 3, pci_ide_save, pci_ide_load, d);
 qemu_register_reset(cmd646_reset, d);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 20/32] hw/device-hotplug: fix test of drive_add() return

[Andrew Cooper]

From: Kaifeng Zhu 

drive_opt_idx could be -1 in case error occurs inside drive_add, so the error
check should be "if (drive_opt_idx < 0)" instead of original
"if (!drive_opt_idx)".

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055574
Reviewed-by: Andrew Cooper 
---
 hw/device-hotplug.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 3bdc048..97d3529 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -34,7 +34,7 @@ int add_init_drive(const char *opts)
 int ret = -1;
 
 drive_opt_idx = drive_add(NULL, "%s", opts);
-if (!drive_opt_idx)
+if (drive_opt_idx < 0)
 return ret;
 
 drive_idx = drive_init(&drives_opt[drive_opt_idx], 0, current_machine);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 26/32] block-vvfat: fix memory leak in check_directory_consistency()

[Andrew Cooper]

From: Kaifeng Zhu 

Memory pointed by cluster leaks in error handling code.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055917
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block-vvfat.c b/block-vvfat.c
index ec3363c..6cd57a2 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -1769,7 +1769,7 @@ static int check_directory_consistency(BDRVVVFATState *s,
 
if (s->used_clusters[cluster_num] & USED_ANY) {
fprintf(stderr, "cluster %d used more than once\n", 
(int)cluster_num);
-   return 0;
+   goto fail;
}
s->used_clusters[cluster_num] = USED_DIRECTORY;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 17/32] readline: fix memory corruption when adding history

[Andrew Cooper]

From: Kaifeng Zhu 

idx can be down to 0, so TERM_MAX_CMDS-idx+1 could be TERM_MAX_CMDS+1, which
exceeds the size of term_history.

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055739
Reviewed-by: Andrew Cooper 
---
 readline.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/readline.c b/readline.c
index 8572841..4b68726 100644
--- a/readline.c
+++ b/readline.c
@@ -267,7 +267,7 @@ static void term_hist_add(const char *cmdline)
new_entry = hist_entry;
/* Put this entry at the end of history */
memmove(&term_history[idx], &term_history[idx + 1],
-   (TERM_MAX_CMDS - idx + 1) * sizeof(char *));
+   (TERM_MAX_CMDS - (idx + 1)) * sizeof(char *));
term_history[TERM_MAX_CMDS - 1] = NULL;
for (; idx < TERM_MAX_CMDS; idx++) {
if (term_history[idx] == NULL)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 14/32] pic: Don't allocate irq buffers

[Andrew Cooper]

i8259_init() doesn't inspect its argument at all, causing the allocation to
be leaked and never used.

Signed-off-by: Andrew Cooper 
---
 hw/pc.c |4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index 7359338..09b4af4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -801,7 +801,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
 PCIBus *pci_bus;
 int piix3_devfn = -1;
 CPUState *env;
-qemu_irq *cpu_irq;
 qemu_irq *i8259;
 int index;
 BlockDriverState *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
@@ -970,8 +969,7 @@ vga_bios_error:
 
 bochs_bios_init();
 
-cpu_irq = qemu_allocate_irqs(pic_irq_request, NULL, 1);
-i8259 = i8259_init(cpu_irq[0]);
+i8259 = i8259_init(NULL);
 ferr_irq = i8259[13];
 
 if (pci_enabled) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 16/32] hw/msmouse.c: Fix deref_after_free and double free

[Andrew Cooper]

From: Yunlei Ding 

msmouse_chr_close is only pointed by chr->chr_close in qemu_chr_close
function. After calling chr->chr_close, chr will be freed. So we don't
need to free it again here.

Signed-off-by: Yunlei Ding 
(defect not identified by Coverity Scan)
Reviewed-by: Andrew Cooper 
---
 hw/msmouse.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/msmouse.c b/hw/msmouse.c
index 69356a5..2d2703b 100644
--- a/hw/msmouse.c
+++ b/hw/msmouse.c
@@ -61,7 +61,6 @@ static int msmouse_chr_write (struct CharDriverState *s, 
const uint8_t *buf, int
 
 static void msmouse_chr_close (struct CharDriverState *chr)
 {
-qemu_free (chr);
 }
 
 CharDriverState *qemu_chr_open_msmouse(void)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 12/32] cmdline: Parse -pciemulation before trying to use it

[Andrew Cooper]

From: Kaifeng Zhu 

Signed-off-by: Kaifeng Zhu 
Reviewed-by: Andrew Cooper 
---
 vl.c |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/vl.c b/vl.c
index d21c3aa..67d9d86 100644
--- a/vl.c
+++ b/vl.c
@@ -5952,6 +5952,15 @@ int main(int argc, char **argv, char **envp)
 }
 }
 
+#ifdef CONFIG_PASSTHROUGH
+for (i = 0; i < nb_pci_emulation; i++) {
+if (pci_emulation_add(pci_emulation_config_text[i]) < 0) {
+fprintf(stderr, "Warning: could not add PCI device %s\n",
+pci_emulation_config_text[i]);
+}
+}
+#endif
+
 machine->init(ram_size, vga_ram_size, boot_devices,
   kernel_filename, kernel_cmdline, initrd_filename, cpu_model,
  direct_pci);
@@ -6068,15 +6077,6 @@ int main(int argc, char **argv, char **envp)
 }
 }
 
-#ifdef CONFIG_PASSTHROUGH
-for (i = 0; i < nb_pci_emulation; i++) {
-if (pci_emulation_add(pci_emulation_config_text[i]) < 0) {
-fprintf(stderr, "Warning: could not add PCI device %s\n",
-pci_emulation_config_text[i]);
-}
-}
-#endif
-
 for(i = 0; i < MAX_VIRTIO_CONSOLES; i++) {
 const char *devname = virtio_consoles[i];
 if (virtcon_hds[i] && devname) {
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 20/32] hw/device-hotplug: fix test of drive_add() return

[Andrew Cooper]

From: Kaifeng Zhu 

drive_opt_idx could be -1 in case error occurs inside drive_add, so the error
check should be "if (drive_opt_idx < 0)" instead of original
"if (!drive_opt_idx)".

Signed-off-by: Kaifeng Zhu 
Coverity-ID: 1055574
Reviewed-by: Andrew Cooper 
---
 hw/device-hotplug.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c
index 3bdc048..97d3529 100644
--- a/hw/device-hotplug.c
+++ b/hw/device-hotplug.c
@@ -34,7 +34,7 @@ int add_init_drive(const char *opts)
 int ret = -1;
 
 drive_opt_idx = drive_add(NULL, "%s", opts);
-if (!drive_opt_idx)
+if (drive_opt_idx < 0)
 return ret;
 
 drive_idx = drive_init(&drives_opt[drive_opt_idx], 0, current_machine);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 30/32] block-nbd: close sock in nbd_open() error path

[Andrew Cooper]

From: Yunlei Ding 

Close sock handle before return.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1055914
Reviewed-by: Andrew Cooper 
---
 block-nbd.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/block-nbd.c b/block-nbd.c
index dc63183..e2c90eb 100644
--- a/block-nbd.c
+++ b/block-nbd.c
@@ -88,7 +88,10 @@ static int nbd_open(BlockDriverState *bs, const char* 
filename, int flags)
 
 ret = nbd_receive_negotiate(sock, &size, &blocksize);
 if (ret == -1)
+{
+close(sock);
 return -errno;
+}
 
 s->sock = sock;
 s->size = size;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 24/32] net: Fix memory/handle leaks in net_socket_listen_init()

[Andrew Cooper]

From: Kaifeng Zhu 

fd and s could be leaked in case bind/listen failed.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055923 1055924
Reviewed-by: Andrew Cooper 
---
 net.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/net.c b/net.c
index f3887a7..33460d8 100644
--- a/net.c
+++ b/net.c
@@ -1460,6 +1460,7 @@ static int net_socket_listen_init(VLANState *vlan,
 fd = socket(PF_INET, SOCK_STREAM, 0);
 if (fd < 0) {
 perror("socket");
+qemu_free(s);
 return -1;
 }
 socket_set_nonblock(fd);
@@ -1471,11 +1472,15 @@ static int net_socket_listen_init(VLANState *vlan,
 ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
 if (ret < 0) {
 perror("bind");
+closesocket(fd);
+qemu_free(s);
 return -1;
 }
 ret = listen(fd, 0);
 if (ret < 0) {
 perror("listen");
+closesocket(fd);
+qemu_free(s);
 return -1;
 }
 s->vlan = vlan;
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 25/32] block-vvfat: fix memory/handle leaks in commit_one_file()

[Andrew Cooper]

From: Kaifeng Zhu 

Some handles and memory in commit_one_file are going to be leaked if
certain function calls failed.

Signed-off-by: Kaifeng Zhu 
Coverity-IDs: 1055918 1055919
Reviewed-by: Andrew Cooper 
---
 block-vvfat.c |   17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block-vvfat.c b/block-vvfat.c
index 345d7be..ec3363c 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -2229,11 +2229,15 @@ static int commit_one_file(BDRVVVFATState* s,
 if (fd < 0) {
fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
strerror(errno), errno);
+   qemu_free(cluster);
return fd;
 }
 if (offset > 0)
-   if (lseek(fd, offset, SEEK_SET) != offset)
+   if (lseek(fd, offset, SEEK_SET) != offset) {
+   close(fd);
+   qemu_free(cluster);
return -3;
+   }
 
 while (offset < size) {
uint32_t c1;
@@ -2249,11 +2253,17 @@ static int commit_one_file(BDRVVVFATState* s,
ret = vvfat_read(s->bs, cluster2sector(s, c),
(uint8_t*)cluster, (rest_size + 0x1ff) / 0x200);
 
-   if (ret < 0)
+   if (ret < 0) {
+   close(fd);
+   qemu_free(cluster);
return ret;
+   }
 
-   if (qemu_write_ok(fd, cluster, rest_size) < 0)
+   if (qemu_write_ok(fd, cluster, rest_size) < 0) {
+   close(fd);
+   qemu_free(cluster);
return -2;
+   }
 
offset += rest_size;
c = c1;
@@ -2261,6 +2271,7 @@ static int commit_one_file(BDRVVVFATState* s,
 
 ftruncate(fd, size);
 close(fd);
+qemu_free(cluster);
 
 return commit_mappings(s, first_cluster, dir_index);
 }
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 05/32] usb-linux.c: fix buffer overflow

[Andrew Cooper]

From: Jim Paris 

In usb-linux.c:usb_host_handle_control, we pass a 1024-byte buffer and
length to the kernel.  However, the length was provided by the caller
of dev->handle_packet, and is not checked, so the kernel might provide
too much data and overflow our buffer.

For example, hw/usb-uhci.c could set the length to 2047.
hw/usb-ohci.c looks like it might go up to 4096 or 8192.

This causes a qemu crash, as reported here:
  http://www.mail-archive.com/kvm@vger.kernel.org/msg18447.html

This patch increases the usb-linux.c buffer size to 2048 to fix the
specific device reported, and adds a check to avoid the overflow in
any case.

Signed-off-by: Jim Paris 
Signed-off-by: Anthony Liguori 
---
 usb-linux.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 5dfed8c..51bac8a 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -117,7 +117,7 @@ struct ctrl_struct {
 uint16_t offset;
 uint8_t  state;
 struct   usb_ctrlrequest req;
-uint8_t  buffer[1024];
+uint8_t  buffer[2048];
 };
 
 typedef struct USBHostDevice {
@@ -554,6 +554,7 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 struct usbdevfs_urb *urb;
 AsyncURB *aurb;
 int ret, value, index;
+int buffer_len;
 
 /* 
  * Process certain standard device requests.
@@ -582,6 +583,13 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 
 /* The rest are asynchronous */
 
+buffer_len = 8 + s->ctrl.len;
+if (buffer_len > sizeof(s->ctrl.buffer)) {
+   fprintf(stderr, "husb: ctrl buffer too small (%d > %zu)\n",
+   buffer_len, sizeof(s->ctrl.buffer));
+   return USB_RET_STALL;
+}
+
 aurb = async_alloc();
 aurb->hdev   = s;
 aurb->packet = p;
@@ -598,7 +606,7 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 urb->endpoint = p->devep;
 
 urb->buffer= &s->ctrl.req;
-urb->buffer_length = 8 + s->ctrl.len;
+urb->buffer_length = buffer_len;
 
 urb->usercontext = s;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 02/32] cirrus_vga: default all I/O port reads to 0xff

[Andrew Cooper]

Some error paths in vga_ioport_read() would return undefined values.
Always default the result to 0xff.

Signed-off-by: Andrew Cooper 
---
 hw/cirrus_vga.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index a26b051..11ce212 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2674,7 +2674,7 @@ static void cirrus_update_memory_access(CirrusVGAState *s)
 static uint32_t vga_ioport_read(void *opaque, uint32_t addr)
 {
 CirrusVGAState *s = opaque;
-int val, index;
+int val = 0xff, index;
 
 /* check port range access depending on color/monochrome mode */
 if ((addr >= 0x3b0 && addr <= 0x3bf && (s->msr & MSR_COLOR_EMULATION))
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 04/32] block-vvfat: fix fat_chksum() buffer overrun warning

[Andrew Cooper]

Newer GCC versions raise an undefined behaviour warning in
fat_chksum() because it overruns the name buffer.  However, this is
intentional behaviour because the extension array immediately follows.

Refactor this function to avoid the warning and make it clear it's
checksumming both parts.

Signed-off-by: Andrew Cooper 
Coverity-ID: 1055738
---
 block-vvfat.c |   15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/block-vvfat.c b/block-vvfat.c
index 9eb676b..345d7be 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -504,14 +504,21 @@ static void set_begin_of_direntry(direntry_t* direntry, 
uint32_t begin)
 
 /* fat functions */
 
+static inline void fat_chksum_part(const char *name, size_t len, uint8_t 
*chksum)
+{
+size_t i;
+
+for(i = 0; i < len; i++)
+   *chksum = (((*chksum&0xfe) >> 1) | ((*chksum & 0x01) ? 0x80 : 0))
+   + (unsigned char)name[i];
+}
+
 static inline uint8_t fat_chksum(const direntry_t* entry)
 {
 uint8_t chksum=0;
-int i;
 
-for(i=0;i<11;i++)
-   chksum=(((chksum&0xfe)>>1)|((chksum&0x01)?0x80:0))
-   +(unsigned char)entry->name[i];
+fat_chksum_part(entry->name, ARRAY_SIZE(entry->name), &chksum);
+fat_chksum_part(entry->extension, ARRAY_SIZE(entry->extension), &chksum);
 
 return chksum;
 }
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] raisin: Some git-checkout improvements

[Stefano Stabellini]

On Wed, 22 Apr 2015, George Dunlap wrote:
> On 04/22/2015 03:54 PM, Stefano Stabellini wrote:
> > On Wed, 22 Apr 2015, George Dunlap wrote:
> >> On 04/22/2015 03:11 PM, Stefano Stabellini wrote:
> >>> On Tue, 21 Apr 2015, George Dunlap wrote:
>  1. Switch local variables to lower-case and declare them local.
> >>>
> >>> This is good.
> >>>
> >>>
>  2. Cloning git trees from remote repos is often a very long operation.
>  Allow the user to specify a faster git cache as a prefix.
> 
>  3. At the moment you can either check out a specific changeset or
>  "master", but you can't check out a different branch, because git
>  doesn't always look in origin/ for the branch.  If the initial git
>  checkout $tag fails, try checking out origin/$tag before giving up.
> >>>
> >>> I am in two minds about this, because it is still possible for the user
> >>> to simply:
> >>>
> >>> LIBVIRT_REVISION="origin/blah"
> >>
> >> Then we should have all the revisions point to "origin/master" for
> >> consistency.
> > 
> > That is true. I think it is probably simpler that way.
> > 
> > 
> >> In any case, it requires the user to know the default remote repository
> >> name.
> > 
> > Right, but that is always origin, isn't it? In fact you would hardcode
> > it in the script with your change below:
> > 
> > +   $GIT checkout -b dummy $tag \
> > +   || $GIT checkout -b dummy origin/$tag
> 
> Sure, but that's an internal implementation detail.  We just cloned it,
> so we should know what the remote branch name is.  We don't necessarily
> want the user to rely on that, in case we want to change the
> implementation somehow.
> 
> But if you don't mind changing the default to "origin/master", I won't
> argue too much.  I think the chances of us changing away from "origin"
> are *very* slim. :-)

OK, good


> >> A brief scan of the git man page, combined with a brief survey of
> >> Google, didn't turn up anything...
> > 
> > All right. You might just want to add few more words in the comment on
> > defconfig, or maybe a pointer to an url that explains what a git proxy
> > is.
> 
> I would have done it this time if I'd known much about the git proxy.
> Unfortunately it doesn't look like a proper project at the moment; it
> seems to be mainly available from a debian package called
> "chiark-scripts" that IanJ maintains.
> 
> We shouldn't rely on a private program not publicly available; so either
> we should try to make git-cache-proxy a public project, or I should find
> another suitable proxy to point people to.  (Or just give up on it for now.)

I agree

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Stefano Stabellini]

On Wed, 22 Apr 2015, Jan Beulich wrote:
> >>> On 22.04.15 at 15:57,  wrote:
> > From the description of the problem above, we have two issues:
> > 
> > 1) we don't detect that maxmem is already UINT_MAX*4, so we shouldn't try
> >to increase it
> > 
> > 2) unsigned int / uint64_t mismatch
> > 
> > 
> > 1) is pretty easy and might just come down to one more if statement in
> > libxl_set_memory_target. Something like:
> > 
> > 
> > #define MAXMEM_MAX_KB ((uint64_t)UINT32_MAX * 4)
> > if ((new_target_memkb - current_target_memkb) > 0 && 
> > (ptr.max_memkb - new_target_memkb + current_target_memkb) < 
> > ptr.max_memkb)
> > {
> > /* avoid overflow */
> >rc = xc_domain_setmaxmem(ctx->xch, domid, MAXMEM_MAX_KB);
> > } else {
> >rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
> > }
> 
> Which would build into the tool stack an assumption that the
> hypervisor representation is a 32-bit value worth of pages. Not
> really desirable imo. If anything we'd need to do actual overflow
> checking (and saturation, similar to what the hypervisor patch
> does) here too.

Sure, as long as Xen is able to handle maxmem arguments > UINT32_MAX *
4, that it doesn't look like is the case at the moment.



> > 2) is difficult as we have unsigned int in many many places.  Not only
> > we need to fix the libxc interface, but as far as I can tell we also
> > need to fix at least libxl_set_memory_target,
> > libxl__fill_dom0_memory_info, libxl__get_memory_target, all the callers.
> > This is the minimum set of changes I think are required:
> 
> I'll leave that to the tool stack maintainers to decide upon. All I
> really need immediately is that "xl mem-set" works again.

What I am saying is that, as you probably know, this patch is not enough
to get it right.

I have now seen your hypervisor side patch (it would have been better
to have them in a series so that it would have been obvious that they
need each other).

Even after your hypervisor patch and this small libxl change, xl mem-set
might work in your scenario, but libxl still doesn't have the right
behaviour: libxl is unintentionally causing overflows.
xc_domain_setmaxmem should rightfully return errors in those cases, and
libxl would abort the operation. This is not what we want, right?

I think that at the very least you need to add overflow checking in
libxl_set_memory_target.


The other changes I suggested come from:

memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;

memorykb and ptr.max_memkb might both be uint64_t with your change, but
current_target_memkb and new_target_memkb are not. There could still be
overflow there. However I do understand that they actually refer to the
memory target, not maxmem, so they could be fixed separately. If you
don't want to handle that in your series, that's fine by me.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 03/32] lm832x: don't overrun file buffer on save/restore

[Andrew Cooper]

Saving and restoring an lm832x record would overrun the pwm.file array
since pwm.file is uint16_t elements and sizeof(pwm.file) twice as many
elements.

To ensure compatibility, padding bytes are added to the record.

Signed-off-by: Andrew Cooper 
Coverity-IDs: 1055728 1055729
---
 hw/lm832x.c |   11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/lm832x.c b/hw/lm832x.c
index dd94310..a212866 100644
--- a/hw/lm832x.c
+++ b/hw/lm832x.c
@@ -439,8 +439,11 @@ static void lm_kbd_save(QEMUFile *f, void *opaque)
 qemu_put_byte(f, s->kbd.len);
 qemu_put_buffer(f, s->kbd.fifo, sizeof(s->kbd.fifo));
 
-for (i = 0; i < sizeof(s->pwm.file); i ++)
+for (i = 0; i < ARRAY_SIZE(s->pwm.file); i ++)
 qemu_put_be16s(f, &s->pwm.file[i]);
+/* Padding for compatibility with older records. */
+for ( ; i < sizeof(s->pwm.file); i++)
+qemu_put_be16s(f, 0);
 qemu_put_8s(f, &s->pwm.faddr);
 qemu_put_buffer(f, s->pwm.addr, sizeof(s->pwm.addr));
 qemu_put_timer(f, s->pwm.tm[0]);
@@ -451,6 +454,7 @@ static void lm_kbd_save(QEMUFile *f, void *opaque)
 static int lm_kbd_load(QEMUFile *f, void *opaque, int version_id)
 {
 struct lm_kbd_s *s = (struct lm_kbd_s *) opaque;
+uint16_t pad;
 int i;
 
 i2c_slave_load(f, &s->i2c);
@@ -475,8 +479,11 @@ static int lm_kbd_load(QEMUFile *f, void *opaque, int 
version_id)
 s->kbd.len = qemu_get_byte(f);
 qemu_get_buffer(f, s->kbd.fifo, sizeof(s->kbd.fifo));
 
-for (i = 0; i < sizeof(s->pwm.file); i ++)
+for (i = 0; i < ARRAY_SIZE(s->pwm.file); i ++)
 qemu_get_be16s(f, &s->pwm.file[i]);
+/* Skip padding. */
+for ( ; i < sizeof(s->pwm.file); i++)
+qemu_get_be16(f);
 qemu_get_8s(f, &s->pwm.faddr);
 qemu_get_buffer(f, s->pwm.addr, sizeof(s->pwm.addr));
 qemu_get_timer(f, s->pwm.tm[0]);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 07/32] CVE-2014-7815: vnc: sanitize bits_per_pixel from the client

[Andrew Cooper]

Backport of qemu-upstream:
 * e6908bfe8e07f2b452e78e677da1b45b1c0f6829

Signed-off-by: Andrew Cooper 
---
 vnc.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/vnc.c b/vnc.c
index 7629dfa..7006a34 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1616,6 +1616,16 @@ static void set_pixel_format(VncState *vs,
 return;
 }
 
+switch (bits_per_pixel) {
+case 8:
+case 16:
+case 32:
+break;
+default:
+vnc_client_error(vs);
+return;
+}
+
 vs->clientds = vs->serverds;
 vs->clientds.pf.rmax = red_max;
 count_bits(vs->clientds.pf.rbits, red_max);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 02/32] cirrus_vga: default all I/O port reads to 0xff

[Andrew Cooper]

Some error paths in vga_ioport_read() would return undefined values.
Always default the result to 0xff.

Signed-off-by: Andrew Cooper 
---
 hw/cirrus_vga.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index a26b051..11ce212 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2674,7 +2674,7 @@ static void cirrus_update_memory_access(CirrusVGAState *s)
 static uint32_t vga_ioport_read(void *opaque, uint32_t addr)
 {
 CirrusVGAState *s = opaque;
-int val, index;
+int val = 0xff, index;
 
 /* check port range access depending on color/monochrome mode */
 if ((addr >= 0x3b0 && addr <= 0x3bf && (s->msr & MSR_COLOR_EMULATION))
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC PATCH COLO v5 08/29] tools/libxl: Introduce bitops macros

[Ian Campbell]

On Wed, 2015-04-01 at 14:41 +0800, Yang Hongyang wrote:
> From: Wen Congyang 
> 
> This is the same set used by libxc.

What is this for?

libxl already exposes a fairly complete libxl_bitmap type and helpers
for use in its own interfaces and by its users.

For libxl's internal purposes (i.e. to interact with libxc) I don't see
why it can't just use xc_bitops.h.

Ian.


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 10/32] ide: cancel dma operations on command abort or error

[Andrew Cooper]

From: Chunjie Zhu 

Otherwise, a guest can cause Qemu to reuse an active aio structure.

Signed-off-by: Chunjie Zhu 
Reviewed-by: Andrew Cooper 
---
 hw/ide.c |7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ide.c b/hw/ide.c
index 791666b..83e3c70 100644
--- a/hw/ide.c
+++ b/hw/ide.c
@@ -919,8 +919,10 @@ static void ide_set_signature(IDEState *s)
 }
 }
 
+static void ide_dma_cancel(BMDMAState *bm);
 static inline void ide_abort_command(IDEState *s)
 {
+if (s->bmdma) ide_dma_cancel(s->bmdma);
 s->status = READY_STAT | ERR_STAT;
 s->error = ABRT_ERR;
 }
@@ -1098,6 +1100,7 @@ static void dma_buf_commit(IDEState *s, int is_write)
 
 static void ide_dma_error(IDEState *s)
 {
+if (s->bmdma) ide_dma_cancel(s->bmdma);
 ide_transfer_stop(s);
 s->error = ABRT_ERR;
 s->status = READY_STAT | ERR_STAT;
@@ -1230,7 +1233,7 @@ static void ide_read_dma_cb(void *opaque, int ret)
return;
 }
 
-if (!s->bs) return; /* ouch! (see ide_flush_cb) */
+if (!s || !s->bs) return; /* ouch! (see ide_dma_error & ide_flush_cb) */
 
 n = s->io_buffer_size >> 9;
 sector_num = ide_get_sector(s);
@@ -1371,7 +1374,7 @@ static void ide_write_dma_cb(void *opaque, int ret)
 return;
 }
 
-if (!s->bs) return; /* ouch! (see ide_flush_cb) */
+if (!s || !s->bs) return; /* ouch! (see ide_dma_error & ide_flush_cb) */
 
 n = s->io_buffer_size >> 9;
 sector_num = ide_get_sector(s);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 09/32] cirrus_vga: fix division by 0 for color expansion rop

[Andrew Cooper]

From: Aurelien Jarno 

Commit d85d0d3883f5a567fa2969a0396e42e0a662b3fa introduces a regression
with Windows ME that leads to a division by 0 and a crash.

It uses the color expansion rop with the source pitch set to 0. This is
something allowed, as the manual explicitely says "When the source of
color-expand data is display memory, the source pitch is ignored.".

This patch fixes this regression by computing sx, sy and others
variables only if they are going to be used later, that is for a plain
copy ROP. It basically consists in moving code.

Signed-off-by: Aurelien Jarno 
---
 hw/cirrus_vga.c |   70 +++
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index d3bf4cf..e6c3893 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -754,46 +754,46 @@ static int 
cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
 
 static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 {
-int sx, sy;
-int dx, dy;
-int width, height;
-int depth;
+int sx = 0, sy = 0;
+int dx = 0, dy = 0;
+int depth = 0;
 int notify = 0;
 
-depth = s->get_bpp((VGAState *)s) / 8;
-s->get_resolution((VGAState *)s, &width, &height);
-
-/* extra x, y */
-sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
-sy = (src / ABS(s->cirrus_blt_srcpitch));
-dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
-dy = (dst / ABS(s->cirrus_blt_dstpitch));
-
-/* normalize width */
-w /= depth;
-
-/* if we're doing a backward copy, we have to adjust
-   our x/y to be the upper left corner (instead of the lower
-   right corner) */
-if (s->cirrus_blt_dstpitch < 0) {
-   sx -= (s->cirrus_blt_width / depth) - 1;
-   dx -= (s->cirrus_blt_width / depth) - 1;
-   sy -= s->cirrus_blt_height - 1;
-   dy -= s->cirrus_blt_height - 1;
-}
+/* make sure to only copy if it's a plain copy ROP */
+if (*s->cirrus_rop == cirrus_bitblt_rop_fwd_src ||
+*s->cirrus_rop == cirrus_bitblt_rop_bkwd_src) {
+int width, height;
+
+depth = s->get_bpp((VGAState *)s) / 8;
+s->get_resolution((VGAState *)s, &width, &height);
+
+/* extra x, y */
+sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
+sy = (src / ABS(s->cirrus_blt_srcpitch));
+dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
+dy = (dst / ABS(s->cirrus_blt_dstpitch));
+
+/* normalize width */
+w /= depth;
+
+/* if we're doing a backward copy, we have to adjust
+   our x/y to be the upper left corner (instead of the lower
+   right corner) */
+if (s->cirrus_blt_dstpitch < 0) {
+sx -= (s->cirrus_blt_width / depth) - 1;
+dx -= (s->cirrus_blt_width / depth) - 1;
+sy -= s->cirrus_blt_height - 1;
+dy -= s->cirrus_blt_height - 1;
+}
 
-/* are we in the visible portion of memory? */
-if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
-   (sx + w) <= width && (sy + h) <= height &&
-   (dx + w) <= width && (dy + h) <= height) {
-   notify = 1;
+/* are we in the visible portion of memory? */
+if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
+(sx + w) <= width && (sy + h) <= height &&
+(dx + w) <= width && (dy + h) <= height) {
+notify = 1;
+}
 }
 
-/* make to sure only copy if it's a plain copy ROP */
-if (*s->cirrus_rop != cirrus_bitblt_rop_fwd_src &&
-   *s->cirrus_rop != cirrus_bitblt_rop_bkwd_src)
-   notify = 0;
-
 /* we have to flush all pending changes so that the copy
is generated at the appropriate moment in time */
 if (notify)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 01/32] virtio-blk: initialise unused blkcfg.size_max field

[Andrew Cooper]

From: Yunlei Ding 

Newer GCC warns about memcpy()ing uninitialised data.

Signed-off-by: Yunlei Ding 
Coverity-ID: 1056088
Reviewed-by: Andrew Cooper 
---
 hw/virtio-blk.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 520ad1b..fcf893a 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -252,6 +252,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, 
uint8_t *config)
 stw_raw(&blkcfg.cylinders, cylinders);
 blkcfg.heads = heads;
 blkcfg.sectors = secs;
+blkcfg.size_max = 0;
 memcpy(config, &blkcfg, sizeof(blkcfg));
 }
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 07/32] CVE-2014-7815: vnc: sanitize bits_per_pixel from the client

[Andrew Cooper]

Backport of qemu-upstream:
 * e6908bfe8e07f2b452e78e677da1b45b1c0f6829

Signed-off-by: Andrew Cooper 
---
 vnc.c |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/vnc.c b/vnc.c
index 7629dfa..7006a34 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1616,6 +1616,16 @@ static void set_pixel_format(VncState *vs,
 return;
 }
 
+switch (bits_per_pixel) {
+case 8:
+case 16:
+case 32:
+break;
+default:
+vnc_client_error(vs);
+return;
+}
+
 vs->clientds = vs->serverds;
 vs->clientds.pf.rmax = red_max;
 count_bits(vs->clientds.pf.rbits, red_max);
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 05/32] usb-linux.c: fix buffer overflow

[Andrew Cooper]

From: Jim Paris 

In usb-linux.c:usb_host_handle_control, we pass a 1024-byte buffer and
length to the kernel.  However, the length was provided by the caller
of dev->handle_packet, and is not checked, so the kernel might provide
too much data and overflow our buffer.

For example, hw/usb-uhci.c could set the length to 2047.
hw/usb-ohci.c looks like it might go up to 4096 or 8192.

This causes a qemu crash, as reported here:
  http://www.mail-archive.com/kvm@vger.kernel.org/msg18447.html

This patch increases the usb-linux.c buffer size to 2048 to fix the
specific device reported, and adds a check to avoid the overflow in
any case.

Signed-off-by: Jim Paris 
Signed-off-by: Anthony Liguori 
---
 usb-linux.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/usb-linux.c b/usb-linux.c
index 5dfed8c..51bac8a 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -117,7 +117,7 @@ struct ctrl_struct {
 uint16_t offset;
 uint8_t  state;
 struct   usb_ctrlrequest req;
-uint8_t  buffer[1024];
+uint8_t  buffer[2048];
 };
 
 typedef struct USBHostDevice {
@@ -554,6 +554,7 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 struct usbdevfs_urb *urb;
 AsyncURB *aurb;
 int ret, value, index;
+int buffer_len;
 
 /* 
  * Process certain standard device requests.
@@ -582,6 +583,13 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 
 /* The rest are asynchronous */
 
+buffer_len = 8 + s->ctrl.len;
+if (buffer_len > sizeof(s->ctrl.buffer)) {
+   fprintf(stderr, "husb: ctrl buffer too small (%d > %zu)\n",
+   buffer_len, sizeof(s->ctrl.buffer));
+   return USB_RET_STALL;
+}
+
 aurb = async_alloc();
 aurb->hdev   = s;
 aurb->packet = p;
@@ -598,7 +606,7 @@ static int usb_host_handle_control(USBHostDevice *s, 
USBPacket *p)
 urb->endpoint = p->devep;
 
 urb->buffer= &s->ctrl.req;
-urb->buffer_length = 8 + s->ctrl.len;
+urb->buffer_length = buffer_len;
 
 urb->usercontext = s;
 
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 08/32] CVE-2014-3615: vbe: rework sanity checks

[Andrew Cooper]

Backport of qemu-upstream:
 * c1b886c45dc70f247300f549dce9833f3fa2def5

Signed-off-by: Andrew Cooper 
---
 hw/vga.c |  154 ++
 1 file changed, 95 insertions(+), 59 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index d0c12aa..e8b1ce0 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -521,6 +521,93 @@ static void vga_ioport_write(void *opaque, uint32_t addr, 
uint32_t val)
 }
 
 #ifdef CONFIG_BOCHS_VBE
+/*
+ * Sanity check vbe register writes.
+ *
+ * As we don't have a way to signal errors to the guest in the bochs
+ * dispi interface we'll go adjust the registers to the closest valid
+ * value.
+ */
+static void vbe_fixup_regs(VGAState *s)
+{
+uint16_t *r = s->vbe_regs;
+uint32_t bits, linelength, maxy, offset;
+
+if (!(r[VBE_DISPI_INDEX_ENABLE] & VBE_DISPI_ENABLED)) {
+/* vbe is turned off -- nothing to do */
+return;
+}
+
+/* check depth */
+switch (r[VBE_DISPI_INDEX_BPP]) {
+case 4:
+case 8:
+case 16:
+case 24:
+case 32:
+bits = r[VBE_DISPI_INDEX_BPP];
+break;
+case 15:
+bits = 16;
+break;
+default:
+bits = r[VBE_DISPI_INDEX_BPP] = 8;
+break;
+}
+
+/* check width */
+r[VBE_DISPI_INDEX_XRES] &= ~7u;
+if (r[VBE_DISPI_INDEX_XRES] == 0) {
+r[VBE_DISPI_INDEX_XRES] = 8;
+}
+if (r[VBE_DISPI_INDEX_XRES] > VBE_DISPI_MAX_XRES) {
+r[VBE_DISPI_INDEX_XRES] = VBE_DISPI_MAX_XRES;
+}
+r[VBE_DISPI_INDEX_VIRT_WIDTH] &= ~7u;
+if (r[VBE_DISPI_INDEX_VIRT_WIDTH] > VBE_DISPI_MAX_XRES) {
+r[VBE_DISPI_INDEX_VIRT_WIDTH] = VBE_DISPI_MAX_XRES;
+}
+if (r[VBE_DISPI_INDEX_VIRT_WIDTH] < r[VBE_DISPI_INDEX_XRES]) {
+r[VBE_DISPI_INDEX_VIRT_WIDTH] = r[VBE_DISPI_INDEX_XRES];
+}
+
+/* check height */
+linelength = r[VBE_DISPI_INDEX_VIRT_WIDTH] * bits / 8;
+maxy = s->vram_size / linelength;
+if (r[VBE_DISPI_INDEX_YRES] == 0) {
+r[VBE_DISPI_INDEX_YRES] = 1;
+}
+if (r[VBE_DISPI_INDEX_YRES] > VBE_DISPI_MAX_YRES) {
+r[VBE_DISPI_INDEX_YRES] = VBE_DISPI_MAX_YRES;
+}
+if (r[VBE_DISPI_INDEX_YRES] > maxy) {
+r[VBE_DISPI_INDEX_YRES] = maxy;
+}
+
+/* check offset */
+if (r[VBE_DISPI_INDEX_X_OFFSET] > VBE_DISPI_MAX_XRES) {
+r[VBE_DISPI_INDEX_X_OFFSET] = VBE_DISPI_MAX_XRES;
+}
+if (r[VBE_DISPI_INDEX_Y_OFFSET] > VBE_DISPI_MAX_YRES) {
+r[VBE_DISPI_INDEX_Y_OFFSET] = VBE_DISPI_MAX_YRES;
+}
+offset = r[VBE_DISPI_INDEX_X_OFFSET] * bits / 8;
+offset += r[VBE_DISPI_INDEX_Y_OFFSET] * linelength;
+if (offset + r[VBE_DISPI_INDEX_YRES] * linelength > s->vram_size) {
+r[VBE_DISPI_INDEX_Y_OFFSET] = 0;
+offset = r[VBE_DISPI_INDEX_X_OFFSET] * bits / 8;
+if (offset + r[VBE_DISPI_INDEX_YRES] * linelength > s->vram_size) {
+r[VBE_DISPI_INDEX_X_OFFSET] = 0;
+offset = 0;
+}
+}
+
+/* update vga state */
+r[VBE_DISPI_INDEX_VIRT_HEIGHT] = maxy;
+s->vbe_line_offset = linelength;
+s->vbe_start_addr  = offset / 4;
+}
+
 static uint32_t vbe_ioport_read_index(void *opaque, uint32_t addr)
 {
 VGAState *s = opaque;
@@ -588,22 +675,13 @@ static void vbe_ioport_write_data(void *opaque, uint32_t 
addr, uint32_t val)
 }
 break;
 case VBE_DISPI_INDEX_XRES:
-if ((val <= VBE_DISPI_MAX_XRES) && ((val & 7) == 0)) {
-s->vbe_regs[s->vbe_index] = val;
-}
-break;
 case VBE_DISPI_INDEX_YRES:
-if (val <= VBE_DISPI_MAX_YRES) {
-s->vbe_regs[s->vbe_index] = val;
-}
-break;
 case VBE_DISPI_INDEX_BPP:
-if (val == 0)
-val = 8;
-if (val == 4 || val == 8 || val == 15 ||
-val == 16 || val == 24 || val == 32) {
-s->vbe_regs[s->vbe_index] = val;
-}
+case VBE_DISPI_INDEX_VIRT_WIDTH:
+case VBE_DISPI_INDEX_X_OFFSET:
+case VBE_DISPI_INDEX_Y_OFFSET:
+s->vbe_regs[s->vbe_index] = val;
+vbe_fixup_regs(s);
 break;
 case VBE_DISPI_INDEX_BANK:
 if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
@@ -623,19 +701,11 @@ static void vbe_ioport_write_data(void *opaque, uint32_t 
addr, uint32_t val)
  set_vram_mapping(s, s->lfb_addr, s->lfb_end);
 }
 
-s->vbe_regs[VBE_DISPI_INDEX_VIRT_WIDTH] =
-s->vbe_regs[VBE_DISPI_INDEX_XRES];
-s->vbe_regs[VBE_DISPI_INDEX_VIRT_HEIGHT] =
-s->vbe_regs[VBE_DISPI_INDEX_YRES];
+s->vbe_regs[VBE_DISPI_INDEX_VIRT_WIDTH] = 0;
 s->vbe_regs[VBE_DISPI_INDEX_X_OFFSET] = 0;
 s->vbe_regs[VBE_DISPI_INDEX_Y_OFFSET] = 0;
-
-if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4)
-s->vbe_line_offset = s->

[Xen-devel] [PATCH 06/32] CVE-2014-8106: cirrus: fix blit region check

[Andrew Cooper]

Backport of qemu-upstream:
 * bf25983345ca44aec3dd92c57142be45452bd38a
 * d3532a0db02296e687711b8cdc7791924efccea0

Signed-off-by: Andrew Cooper 
---
 hw/cirrus_vga.c |   66 +--
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 11ce212..d3bf4cf 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -34,6 +34,8 @@
 #include "qemu-xen.h"
 #include "qemu-log.h"
 
+#include 
+
 /*
  * TODO:
  *- destination write mask support not complete (bits 5..7)
@@ -223,20 +225,6 @@
 
 #define ABS(a) ((signed)(a) > 0 ? a : -a)
 
-#define BLTUNSAFE(s) \
-( \
-( /* check dst is within bounds */ \
-(s)->cirrus_blt_height * ABS((s)->cirrus_blt_dstpitch) \
-+ ((s)->cirrus_blt_dstaddr & (s)->cirrus_addr_mask) > \
-(s)->vram_size \
-) || \
-( /* check src is within bounds */ \
-(s)->cirrus_blt_height * ABS((s)->cirrus_blt_srcpitch) \
-+ ((s)->cirrus_blt_srcaddr & (s)->cirrus_addr_mask) > \
-(s)->vram_size \
-) \
-)
-
 struct CirrusVGAState;
 typedef void (*cirrus_bitblt_rop_t) (struct CirrusVGAState *s,
  uint8_t * dst, const uint8_t * src,
@@ -315,6 +303,50 @@ static void cirrus_vga_mem_writew(void *opaque, 
target_phys_addr_t addr, uint32_
  *
  ***/
 
+static bool blit_region_is_unsafe(struct CirrusVGAState *s,
+  int32_t pitch, int32_t addr)
+{
+if (pitch < 0) {
+int64_t min = addr
++ ((int64_t)s->cirrus_blt_height-1) * pitch;
+int32_t max = addr
++ s->cirrus_blt_width;
+if (min < 0 || max >= s->vram_size) {
+return true;
+}
+} else {
+int64_t max = addr
++ ((int64_t)s->cirrus_blt_height-1) * pitch
++ s->cirrus_blt_width;
+if (max >= s->vram_size) {
+return true;
+}
+}
+return false;
+}
+
+static bool blit_is_unsafe(struct CirrusVGAState *s)
+{
+/* should be the case, see cirrus_bitblt_start */
+assert(s->cirrus_blt_width > 0);
+assert(s->cirrus_blt_height > 0);
+
+if (s->cirrus_blt_width > CIRRUS_BLTBUFSIZE) {
+return true;
+}
+
+if (blit_region_is_unsafe(s, s->cirrus_blt_dstpitch,
+  s->cirrus_blt_dstaddr & s->cirrus_addr_mask)) {
+return true;
+}
+if (blit_region_is_unsafe(s, s->cirrus_blt_srcpitch,
+  s->cirrus_blt_srcaddr & s->cirrus_addr_mask)) {
+return true;
+}
+
+return false;
+}
+
 static void cirrus_bitblt_rop_nop(CirrusVGAState *s,
   uint8_t *dst,const uint8_t *src,
   int dstpitch,int srcpitch,
@@ -676,7 +708,7 @@ static int cirrus_bitblt_common_patterncopy(CirrusVGAState 
* s,
 
 dst = s->vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask);
 
-if (BLTUNSAFE(s))
+if (blit_is_unsafe(s))
 return 0;
 
 (*s->cirrus_rop) (s, dst, src,
@@ -694,7 +726,7 @@ static int cirrus_bitblt_solidfill(CirrusVGAState *s, int 
blt_rop)
 {
 cirrus_fill_t rop_func;
 
-if (BLTUNSAFE(s))
+if (blit_is_unsafe(s))
 return 0;
 rop_func = cirrus_fill[rop_to_index[blt_rop]][s->cirrus_blt_pixelwidth - 
1];
 rop_func(s, s->vram_ptr + (s->cirrus_blt_dstaddr & s->cirrus_addr_mask),
@@ -790,7 +822,7 @@ static void cirrus_do_copy(CirrusVGAState *s, int dst, int 
src, int w, int h)
 
 static int cirrus_bitblt_videotovideo_copy(CirrusVGAState * s)
 {
-if (BLTUNSAFE(s))
+if (blit_is_unsafe(s))
 return 0;
 
 cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->start_addr,
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 09/32] cirrus_vga: fix division by 0 for color expansion rop

[Andrew Cooper]

From: Aurelien Jarno 

Commit d85d0d3883f5a567fa2969a0396e42e0a662b3fa introduces a regression
with Windows ME that leads to a division by 0 and a crash.

It uses the color expansion rop with the source pitch set to 0. This is
something allowed, as the manual explicitely says "When the source of
color-expand data is display memory, the source pitch is ignored.".

This patch fixes this regression by computing sx, sy and others
variables only if they are going to be used later, that is for a plain
copy ROP. It basically consists in moving code.

Signed-off-by: Aurelien Jarno 
---
 hw/cirrus_vga.c |   70 +++
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index d3bf4cf..e6c3893 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -754,46 +754,46 @@ static int 
cirrus_bitblt_videotovideo_patterncopy(CirrusVGAState * s)
 
 static void cirrus_do_copy(CirrusVGAState *s, int dst, int src, int w, int h)
 {
-int sx, sy;
-int dx, dy;
-int width, height;
-int depth;
+int sx = 0, sy = 0;
+int dx = 0, dy = 0;
+int depth = 0;
 int notify = 0;
 
-depth = s->get_bpp((VGAState *)s) / 8;
-s->get_resolution((VGAState *)s, &width, &height);
-
-/* extra x, y */
-sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
-sy = (src / ABS(s->cirrus_blt_srcpitch));
-dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
-dy = (dst / ABS(s->cirrus_blt_dstpitch));
-
-/* normalize width */
-w /= depth;
-
-/* if we're doing a backward copy, we have to adjust
-   our x/y to be the upper left corner (instead of the lower
-   right corner) */
-if (s->cirrus_blt_dstpitch < 0) {
-   sx -= (s->cirrus_blt_width / depth) - 1;
-   dx -= (s->cirrus_blt_width / depth) - 1;
-   sy -= s->cirrus_blt_height - 1;
-   dy -= s->cirrus_blt_height - 1;
-}
+/* make sure to only copy if it's a plain copy ROP */
+if (*s->cirrus_rop == cirrus_bitblt_rop_fwd_src ||
+*s->cirrus_rop == cirrus_bitblt_rop_bkwd_src) {
+int width, height;
+
+depth = s->get_bpp((VGAState *)s) / 8;
+s->get_resolution((VGAState *)s, &width, &height);
+
+/* extra x, y */
+sx = (src % ABS(s->cirrus_blt_srcpitch)) / depth;
+sy = (src / ABS(s->cirrus_blt_srcpitch));
+dx = (dst % ABS(s->cirrus_blt_dstpitch)) / depth;
+dy = (dst / ABS(s->cirrus_blt_dstpitch));
+
+/* normalize width */
+w /= depth;
+
+/* if we're doing a backward copy, we have to adjust
+   our x/y to be the upper left corner (instead of the lower
+   right corner) */
+if (s->cirrus_blt_dstpitch < 0) {
+sx -= (s->cirrus_blt_width / depth) - 1;
+dx -= (s->cirrus_blt_width / depth) - 1;
+sy -= s->cirrus_blt_height - 1;
+dy -= s->cirrus_blt_height - 1;
+}
 
-/* are we in the visible portion of memory? */
-if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
-   (sx + w) <= width && (sy + h) <= height &&
-   (dx + w) <= width && (dy + h) <= height) {
-   notify = 1;
+/* are we in the visible portion of memory? */
+if (sx >= 0 && sy >= 0 && dx >= 0 && dy >= 0 &&
+(sx + w) <= width && (sy + h) <= height &&
+(dx + w) <= width && (dy + h) <= height) {
+notify = 1;
+}
 }
 
-/* make to sure only copy if it's a plain copy ROP */
-if (*s->cirrus_rop != cirrus_bitblt_rop_fwd_src &&
-   *s->cirrus_rop != cirrus_bitblt_rop_bkwd_src)
-   notify = 0;
-
 /* we have to flush all pending changes so that the copy
is generated at the appropriate moment in time */
 if (notify)
-- 
1.7.10.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel