[Qemu-devel] [PATCH RFC v1] Cleanups + various fixes due to libxl ABI + more logging on errors.

[Qemu-devel] [PATCH RFC v1] Cleanups + various fixes due to libxl ABI + more logging on errors.

[Konrad Rzeszutek Wilk]

As I am in the process of syncing the 'dev.config' and Xen's internal
cache of the PCI config space I noticed that some oddities
in the code and figured that having them in would be easier
for me (so I don't have to handle 20 odd patches).

These:
 [PATCH RFC 1/6] xen/pt: Update comments with proper function name.
 [PATCH RFC 2/6] xen/pt: Make xen_pt_msi_set_enable static
 [PATCH RFC 3/6] xen/pt: xen_host_pci_config_read returns -errno, not
 [PATCH RFC 4/6] xen: Print and use errno where applicable.

are quite simple.

The two other ones:
 [PATCH RFC 5/6] xen/pt/msi: Add the register value when printing
 [PATCH RFC 6/6] xen: Add backtrace for serious issues.

are more for adding the proper infrastructure to debug in the field
bugs that may come about.

 hw/xen/xen_pt.c             |  9 ++++++---
 hw/xen/xen_pt.h             |  1 -
 hw/xen/xen_pt_config_init.c |  6 +++---
 hw/xen/xen_pt_msi.c         |  2 +-
 include/hw/xen/xen_common.h |  1 +
 xen-hvm.c                   | 26 ++++++++++++++++++++++----
 6 files changed, 33 insertions(+), 12 deletions(-)

Konrad Rzeszutek Wilk (6):
      xen/pt: Update comments with proper function name.
      xen/pt: Make xen_pt_msi_set_enable static
      xen/pt: xen_host_pci_config_read returns -errno, not -1 on failure
      xen: Print and use errno where applicable.
      xen/pt/msi: Add the register value when printing logging and error messages
      xen: Add backtrace for serious issues.

[Qemu-devel] [PATCH RFC 1/6] xen/pt: Update comments with proper function name.

[Konrad Rzeszutek Wilk]

It has changed but the comments still refer to the old names.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index ed5fcae..706e3d9 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -378,7 +378,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
         }
     }
 
-    /* need to shift back before passing them to xen_host_pci_device */
+    /* need to shift back before passing them to xen_host_pci_set_block. */
     val >>= (addr & 3) << 3;
 
     memory_region_transaction_commit();
@@ -406,7 +406,7 @@ out:
                                     (uint8_t *)&val + index, len);
 
         if (rc < 0) {
-            XEN_PT_ERR(d, "pci_write_block failed. return value: %d.\n", rc);
+            XEN_PT_ERR(d, "xen_host_pci_set_block failed. return value: %d.\n", rc);
         }
     }
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC 1/6] xen/pt: Update comments with proper function name.

[Stefano Stabellini]

On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> It has changed but the comments still refer to the old names.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/xen/xen_pt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index ed5fcae..706e3d9 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -378,7 +378,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr,
>          }
>      }
>  
> -    /* need to shift back before passing them to xen_host_pci_device */
> +    /* need to shift back before passing them to xen_host_pci_set_block. */
>      val >>= (addr & 3) << 3;
>  
>      memory_region_transaction_commit();
> @@ -406,7 +406,7 @@ out:
>                                      (uint8_t *)&val + index, len);
>  
>          if (rc < 0) {
> -            XEN_PT_ERR(d, "pci_write_block failed. return value: %d.\n", rc);
> +            XEN_PT_ERR(d, "xen_host_pci_set_block failed. return value: %d.\n", rc);
>          }
>      }
>  }
> -- 
> 2.1.0
> 

[Qemu-devel] [PATCH RFC 2/6] xen/pt: Make xen_pt_msi_set_enable static

[Konrad Rzeszutek Wilk]

As we do not use it outside our code.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.h     | 1 -
 hw/xen/xen_pt_msi.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 393f36c..09358b1 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -289,7 +289,6 @@ static inline uint8_t xen_pt_pci_intx(XenPCIPassthroughState *s)
 }
 
 /* MSI/MSI-X */
-int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool en);
 int xen_pt_msi_setup(XenPCIPassthroughState *s);
 int xen_pt_msi_update(XenPCIPassthroughState *d);
 void xen_pt_msi_disable(XenPCIPassthroughState *s);
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 263e051..5822df5 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -220,7 +220,7 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
  * MSI virtualization functions
  */
 
-int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool enable)
+static int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool enable)
 {
     XEN_PT_LOG(&s->dev, "%s MSI.\n", enable ? "enabling" : "disabling");
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC 2/6] xen/pt: Make xen_pt_msi_set_enable static

[Stefano Stabellini]

On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> As we do not use it outside our code.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/xen/xen_pt.h     | 1 -
>  hw/xen/xen_pt_msi.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 393f36c..09358b1 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -289,7 +289,6 @@ static inline uint8_t xen_pt_pci_intx(XenPCIPassthroughState *s)
>  }
>  
>  /* MSI/MSI-X */
> -int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool en);
>  int xen_pt_msi_setup(XenPCIPassthroughState *s);
>  int xen_pt_msi_update(XenPCIPassthroughState *d);
>  void xen_pt_msi_disable(XenPCIPassthroughState *s);
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 263e051..5822df5 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -220,7 +220,7 @@ static int msi_msix_disable(XenPCIPassthroughState *s,
>   * MSI virtualization functions
>   */
>  
> -int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool enable)
> +static int xen_pt_msi_set_enable(XenPCIPassthroughState *s, bool enable)
>  {
>      XEN_PT_LOG(&s->dev, "%s MSI.\n", enable ? "enabling" : "disabling");
>  
> -- 
> 2.1.0
> 

[Qemu-devel] [PATCH RFC 3/6] xen/pt: xen_host_pci_config_read returns -errno, not -1 on failure

[Konrad Rzeszutek Wilk]

However the init routines assume that on errors the return
code is -1 (as the libxc API is) - while those xen_host_* routines follow
another paradigm - negative errno on return, 0 on success.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 706e3d9..ea1ceda 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -716,7 +716,7 @@ static int xen_pt_initfn(PCIDevice *d)
 
     /* Initialize virtualized PCI configuration (Extended 256 Bytes) */
     if (xen_host_pci_get_block(&s->real_device, 0, d->config,
-                               PCI_CONFIG_SPACE_SIZE) == -1) {
+                               PCI_CONFIG_SPACE_SIZE) < 0) {
         xen_host_pci_device_put(&s->real_device);
         return -1;
     }
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC 3/6] xen/pt: xen_host_pci_config_read returns -errno, not -1 on failure

[Stefano Stabellini]

On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> However the init routines assume that on errors the return
> code is -1 (as the libxc API is) - while those xen_host_* routines follow
> another paradigm - negative errno on return, 0 on success.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  hw/xen/xen_pt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 706e3d9..ea1ceda 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -716,7 +716,7 @@ static int xen_pt_initfn(PCIDevice *d)
>  
>      /* Initialize virtualized PCI configuration (Extended 256 Bytes) */
>      if (xen_host_pci_get_block(&s->real_device, 0, d->config,
> -                               PCI_CONFIG_SPACE_SIZE) == -1) {
> +                               PCI_CONFIG_SPACE_SIZE) < 0) {
>          xen_host_pci_device_put(&s->real_device);
>          return -1;
>      }
> -- 
> 2.1.0
> 

[Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable.

[Konrad Rzeszutek Wilk]

In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592
"libxc: Fix do_memory_op to return negative value on errors"
made the libxc API less odd-ball: On errors, return value is
-1 and error code is in errno. On success the return value
is either 0 or an positive value.

Since we could be running with an old toolstack in which the
Exx value is in rc or the newer, we print both and return
the -EXX depending on rc == -1 condition.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen-hvm.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen-hvm.c b/xen-hvm.c
index 0408462..a92bc14 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -345,11 +345,12 @@ go_physmap:
         unsigned long idx = pfn + i;
         xen_pfn_t gpfn = start_gpfn + i;
 
+        /* In Xen 4.6 rc is -1 and errno contains the error value. */
         rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
         if (rc) {
             DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
-            return -rc;
+                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
+            return rc == -1 ? -errno : -rc;
         }
     }
 
@@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state,
         xen_pfn_t idx = start_addr + i;
         xen_pfn_t gpfn = phys_offset + i;
 
+        /* In Xen 4.6 rc is -1 and errno contains the error value. */
         rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
         if (rc) {
             fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
-            return -rc;
+                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
+            return rc == -1 ? -errno : -rc;
         }
     }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable.

[Stefano Stabellini]

On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592
> "libxc: Fix do_memory_op to return negative value on errors"
> made the libxc API less odd-ball: On errors, return value is
> -1 and error code is in errno. On success the return value
> is either 0 or an positive value.
> 
> Since we could be running with an old toolstack in which the
> Exx value is in rc or the newer, we print both and return
> the -EXX depending on rc == -1 condition.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen-hvm.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 0408462..a92bc14 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -345,11 +345,12 @@ go_physmap:
>          unsigned long idx = pfn + i;
>          xen_pfn_t gpfn = start_gpfn + i;
>  
> +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
>          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
>          if (rc) {
>              DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> -            return -rc;
> +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> +            return rc == -1 ? -errno : -rc;

Printing both rc and errno is the right thing to do, but I am not sure
changing return value depending on the libxc version is a good idea.
Maybe we should be consistent and always return rc?


>          }
>      }
>  
> @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state,
>          xen_pfn_t idx = start_addr + i;
>          xen_pfn_t gpfn = phys_offset + i;
>  
> +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
>          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
>          if (rc) {
>              fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> -            return -rc;
> +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> +            return rc == -1 ? -errno : -rc;
>          }
>      }
>  
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable.

[Konrad Rzeszutek Wilk]

On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote:
> On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592
> > "libxc: Fix do_memory_op to return negative value on errors"
> > made the libxc API less odd-ball: On errors, return value is
> > -1 and error code is in errno. On success the return value
> > is either 0 or an positive value.
> > 
> > Since we could be running with an old toolstack in which the
> > Exx value is in rc or the newer, we print both and return
> > the -EXX depending on rc == -1 condition.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  xen-hvm.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 0408462..a92bc14 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -345,11 +345,12 @@ go_physmap:
> >          unsigned long idx = pfn + i;
> >          xen_pfn_t gpfn = start_gpfn + i;
> >  
> > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> >          if (rc) {
> >              DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > -            return -rc;
> > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > +            return rc == -1 ? -errno : -rc;
> 
> Printing both rc and errno is the right thing to do, but I am not sure
> changing return value depending on the libxc version is a good idea.
> Maybe we should be consistent and always return rc?

In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL).
With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while
the errno would have EINVAL.

To be consistent and have this function return an proper -Exx value we
need that check to use errno in case rc == -1.

I am uncomfortable with returning positive values as errors, which reminds me -
I need to update the commit to mention the return 1 issue.
> 
> 
> >          }
> >      }
> >  
> > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state,
> >          xen_pfn_t idx = start_addr + i;
> >          xen_pfn_t gpfn = phys_offset + i;
> >  
> > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> >          if (rc) {
> >              fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > -            return -rc;
> > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > +            return rc == -1 ? -errno : -rc;
> >          }
> >      }
> >  
> > -- 
> > 2.1.0
> > 

Re: [Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable.

[Stefano Stabellini]

On Wed, 1 Jul 2015, Konrad Rzeszutek Wilk wrote:
> On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote:
> > On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592
> > > "libxc: Fix do_memory_op to return negative value on errors"
> > > made the libxc API less odd-ball: On errors, return value is
> > > -1 and error code is in errno. On success the return value
> > > is either 0 or an positive value.
> > > 
> > > Since we could be running with an old toolstack in which the
> > > Exx value is in rc or the newer, we print both and return
> > > the -EXX depending on rc == -1 condition.
> > > 
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > ---
> > >  xen-hvm.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > index 0408462..a92bc14 100644
> > > --- a/xen-hvm.c
> > > +++ b/xen-hvm.c
> > > @@ -345,11 +345,12 @@ go_physmap:
> > >          unsigned long idx = pfn + i;
> > >          xen_pfn_t gpfn = start_gpfn + i;
> > >  
> > > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> > >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> > >          if (rc) {
> > >              DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > > -            return -rc;
> > > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > > +            return rc == -1 ? -errno : -rc;
> > 
> > Printing both rc and errno is the right thing to do, but I am not sure
> > changing return value depending on the libxc version is a good idea.
> > Maybe we should be consistent and always return rc?
> 
> In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL).
> With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while
> the errno would have EINVAL.
> 
> To be consistent and have this function return an proper -Exx value we
> need that check to use errno in case rc == -1.

Maybe the best thing to do is to introduce a versioned
xen_xc_domain_add_to_physmap to include/hw/xen/xen_common.h


> I am uncomfortable with returning positive values as errors, which reminds me -
> I need to update the commit to mention the return 1 issue.

Agreed


> > 
> > 
> > >          }
> > >      }
> > >  
> > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state,
> > >          xen_pfn_t idx = start_addr + i;
> > >          xen_pfn_t gpfn = phys_offset + i;
> > >  
> > > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> > >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> > >          if (rc) {
> > >              fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > > -            return -rc;
> > > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > > +            return rc == -1 ? -errno : -rc;
> > >          }
> > >      }
> > >  
> > > -- 
> > > 2.1.0
> > > 
> 

Re: [Qemu-devel] [PATCH RFC 4/6] xen: Print and use errno where applicable.

[Konrad Rzeszutek Wilk]

On Thu, Jul 02, 2015 at 12:00:29PM +0100, Stefano Stabellini wrote:
> On Wed, 1 Jul 2015, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jul 01, 2015 at 02:01:07PM +0100, Stefano Stabellini wrote:
> > > On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > > > In Xen 4.6 commit cd2f100f0f61b3f333d52d1737dd73f02daee592
> > > > "libxc: Fix do_memory_op to return negative value on errors"
> > > > made the libxc API less odd-ball: On errors, return value is
> > > > -1 and error code is in errno. On success the return value
> > > > is either 0 or an positive value.
> > > > 
> > > > Since we could be running with an old toolstack in which the
> > > > Exx value is in rc or the newer, we print both and return
> > > > the -EXX depending on rc == -1 condition.
> > > > 
> > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > > ---
> > > >  xen-hvm.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > > index 0408462..a92bc14 100644
> > > > --- a/xen-hvm.c
> > > > +++ b/xen-hvm.c
> > > > @@ -345,11 +345,12 @@ go_physmap:
> > > >          unsigned long idx = pfn + i;
> > > >          xen_pfn_t gpfn = start_gpfn + i;
> > > >  
> > > > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> > > >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> > > >          if (rc) {
> > > >              DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > > > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > > > -            return -rc;
> > > > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > > > +            return rc == -1 ? -errno : -rc;
> > > 
> > > Printing both rc and errno is the right thing to do, but I am not sure
> > > changing return value depending on the libxc version is a good idea.
> > > Maybe we should be consistent and always return rc?
> > 
> > In Xen 4.5 and earlier this function would return -EINVAL (say rc=EINVAL).
> > With Xen 4.6 it would always return 1 on errors (rc is -1, and with --1 we get 1), while
> > the errno would have EINVAL.
> > 
> > To be consistent and have this function return an proper -Exx value we
> > need that check to use errno in case rc == -1.
> 
> Maybe the best thing to do is to introduce a versioned
> xen_xc_domain_add_to_physmap to include/hw/xen/xen_common.h

Aah, hadn't seen that before. Yes will do that.
> 
> 
> > I am uncomfortable with returning positive values as errors, which reminds me -
> > I need to update the commit to mention the return 1 issue.
> 
> Agreed
> 
> 
> > > 
> > > 
> > > >          }
> > > >      }
> > > >  
> > > > @@ -422,11 +423,12 @@ static int xen_remove_from_physmap(XenIOState *state,
> > > >          xen_pfn_t idx = start_addr + i;
> > > >          xen_pfn_t gpfn = phys_offset + i;
> > > >  
> > > > +        /* In Xen 4.6 rc is -1 and errno contains the error value. */
> > > >          rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> > > >          if (rc) {
> > > >              fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> > > > -                    PRI_xen_pfn" failed: %d\n", idx, gpfn, rc);
> > > > -            return -rc;
> > > > +                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> > > > +            return rc == -1 ? -errno : -rc;
> > > >          }
> > > >      }
> > > >  
> > > > -- 
> > > > 2.1.0
> > > > 
> > 

[Qemu-devel] [PATCH RFC 5/6] xen/pt/msi: Add the register value when printing logging and error messages

[Konrad Rzeszutek Wilk]

We would like to know what the MSI register value is to help
in troubleshooting in the field. As such modify the logging
logic to include such details in xen_pt_msgctrl_reg_write.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt_config_init.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index dd37be3..462e1b9 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1086,7 +1086,7 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
         /* setup MSI pirq for the first time */
         if (!msi->initialized) {
             /* Init physical one */
-            XEN_PT_LOG(&s->dev, "setup MSI\n");
+            XEN_PT_LOG(&s->dev, "setup MSI (register: %x).\n", *val);
             if (xen_pt_msi_setup(s)) {
                 /* We do not broadcast the error to the framework code, so
                  * that MSI errors are contained in MSI emulation code and
@@ -1094,12 +1094,12 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
                  * Guest MSI would be actually not working.
                  */
                 *val &= ~PCI_MSI_FLAGS_ENABLE;
-                XEN_PT_WARN(&s->dev, "Can not map MSI.\n");
+                XEN_PT_WARN(&s->dev, "Can not map MSI (register: %x)!\n", *val);
                 return 0;
             }
             if (xen_pt_msi_update(s)) {
                 *val &= ~PCI_MSI_FLAGS_ENABLE;
-                XEN_PT_WARN(&s->dev, "Can not bind MSI\n");
+                XEN_PT_WARN(&s->dev, "Can not bind MSI (register: %x)!\n", *val);
                 return 0;
             }
             msi->initialized = true;
-- 
2.1.0

[Qemu-devel] [PATCH RFC 6/6] xen: Add backtrace for serious issues.

[Konrad Rzeszutek Wilk]

When debugging issues that caused the emulator to kill itself
or skipping certain operations (unable to write to host
registers) an stack trace will most definitly aid in debugging
the problem.

As such this patch uses the most basic backtrace to print out
details.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c             |  3 +++
 include/hw/xen/xen_common.h |  1 +
 xen-hvm.c                   | 16 ++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index ea1ceda..1d256b9 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -407,6 +407,7 @@ out:
 
         if (rc < 0) {
             XEN_PT_ERR(d, "xen_host_pci_set_block failed. return value: %d.\n", rc);
+            xen_dump_stack();
         }
     }
 }
@@ -421,6 +422,7 @@ static uint64_t xen_pt_bar_read(void *o, hwaddr addr,
      * misconfiguration of the IOMMU. */
     XEN_PT_ERR(d, "Should not read BAR through QEMU. @0x"TARGET_FMT_plx"\n",
                addr);
+    xen_dump_stack();
     return 0;
 }
 static void xen_pt_bar_write(void *o, hwaddr addr, uint64_t val,
@@ -430,6 +432,7 @@ static void xen_pt_bar_write(void *o, hwaddr addr, uint64_t val,
     /* Same comment as xen_pt_bar_read function */
     XEN_PT_ERR(d, "Should not write BAR through QEMU. @0x"TARGET_FMT_plx"\n",
                addr);
+    xen_dump_stack();
 }
 
 static const MemoryRegionOps ops = {
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 38f29fb..3983cfb 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -165,6 +165,7 @@ void destroy_hvm_domain(bool reboot);
 
 /* shutdown/destroy current domain because of an error */
 void xen_shutdown_fatal_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+void xen_dump_stack(void);
 
 #ifdef HVM_PARAM_VMPORT_REGS_PFN
 static inline int xen_get_vmport_regs_pfn(XenXC xc, domid_t dom,
diff --git a/xen-hvm.c b/xen-hvm.c
index a92bc14..8bf4a57 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -10,6 +10,7 @@
 
 #include <sys/mman.h>
 
+#include <execinfo.h>
 #include "hw/pci/pci.h"
 #include "hw/i386/pc.h"
 #include "hw/xen/xen_common.h"
@@ -1328,6 +1329,20 @@ void xen_register_framebuffer(MemoryRegion *mr)
     framebuffer = mr;
 }
 
+void xen_dump_stack(void)
+{
+    int nptrs;
+#define SIZE 1024
+    void *buffer[SIZE];
+
+    nptrs = backtrace(buffer, SIZE);
+    if (!nptrs)
+        return;
+
+    backtrace_symbols_fd(buffer, nptrs, STDERR_FILENO);
+#undef SIZE
+}
+
 void xen_shutdown_fatal_error(const char *fmt, ...)
 {
     va_list ap;
@@ -1335,6 +1350,7 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
     va_start(ap, fmt);
     vfprintf(stderr, fmt, ap);
     va_end(ap);
+    xen_dump_stack();
     fprintf(stderr, "Will destroy the domain.\n");
     /* destroy the domain */
     qemu_system_shutdown_request();
-- 
2.1.0

Re: [Qemu-devel] [PATCH RFC 6/6] xen: Add backtrace for serious issues.

[Stefano Stabellini]

On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> When debugging issues that caused the emulator to kill itself
> or skipping certain operations (unable to write to host
> registers) an stack trace will most definitly aid in debugging
> the problem.
> 
> As such this patch uses the most basic backtrace to print out
> details.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I think it could be useful, but it cannot be done as a xen-hvm.c thing.
It should be somewhere generic, maybe under util? Stefan, any
suggestions?


>  hw/xen/xen_pt.c             |  3 +++
>  include/hw/xen/xen_common.h |  1 +
>  xen-hvm.c                   | 16 ++++++++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index ea1ceda..1d256b9 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -407,6 +407,7 @@ out:
>  
>          if (rc < 0) {
>              XEN_PT_ERR(d, "xen_host_pci_set_block failed. return value: %d.\n", rc);
> +            xen_dump_stack();
>          }
>      }
>  }
> @@ -421,6 +422,7 @@ static uint64_t xen_pt_bar_read(void *o, hwaddr addr,
>       * misconfiguration of the IOMMU. */
>      XEN_PT_ERR(d, "Should not read BAR through QEMU. @0x"TARGET_FMT_plx"\n",
>                 addr);
> +    xen_dump_stack();
>      return 0;
>  }
>  static void xen_pt_bar_write(void *o, hwaddr addr, uint64_t val,
> @@ -430,6 +432,7 @@ static void xen_pt_bar_write(void *o, hwaddr addr, uint64_t val,
>      /* Same comment as xen_pt_bar_read function */
>      XEN_PT_ERR(d, "Should not write BAR through QEMU. @0x"TARGET_FMT_plx"\n",
>                 addr);
> +    xen_dump_stack();
>  }
>  
>  static const MemoryRegionOps ops = {
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 38f29fb..3983cfb 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -165,6 +165,7 @@ void destroy_hvm_domain(bool reboot);
>  
>  /* shutdown/destroy current domain because of an error */
>  void xen_shutdown_fatal_error(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
> +void xen_dump_stack(void);
>  
>  #ifdef HVM_PARAM_VMPORT_REGS_PFN
>  static inline int xen_get_vmport_regs_pfn(XenXC xc, domid_t dom,
> diff --git a/xen-hvm.c b/xen-hvm.c
> index a92bc14..8bf4a57 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -10,6 +10,7 @@
>  
>  #include <sys/mman.h>
>  
> +#include <execinfo.h>
>  #include "hw/pci/pci.h"
>  #include "hw/i386/pc.h"
>  #include "hw/xen/xen_common.h"
> @@ -1328,6 +1329,20 @@ void xen_register_framebuffer(MemoryRegion *mr)
>      framebuffer = mr;
>  }
>  
> +void xen_dump_stack(void)
> +{
> +    int nptrs;
> +#define SIZE 1024
> +    void *buffer[SIZE];
> +
> +    nptrs = backtrace(buffer, SIZE);
> +    if (!nptrs)
> +        return;
> +
> +    backtrace_symbols_fd(buffer, nptrs, STDERR_FILENO);
> +#undef SIZE
> +}
> +
>  void xen_shutdown_fatal_error(const char *fmt, ...)
>  {
>      va_list ap;
> @@ -1335,6 +1350,7 @@ void xen_shutdown_fatal_error(const char *fmt, ...)
>      va_start(ap, fmt);
>      vfprintf(stderr, fmt, ap);
>      va_end(ap);
> +    xen_dump_stack();
>      fprintf(stderr, "Will destroy the domain.\n");
>      /* destroy the domain */
>      qemu_system_shutdown_request();
> -- 
> 2.1.0
> 

Re: [Qemu-devel] [PATCH RFC 6/6] xen: Add backtrace for serious issues.

[Stefan Hajnoczi]

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

On Wed, Jul 01, 2015 at 02:06:30PM +0100, Stefano Stabellini wrote:
> On Mon, 29 Jun 2015, Konrad Rzeszutek Wilk wrote:
> > When debugging issues that caused the emulator to kill itself
> > or skipping certain operations (unable to write to host
> > registers) an stack trace will most definitly aid in debugging
> > the problem.
> > 
> > As such this patch uses the most basic backtrace to print out
> > details.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I think it could be useful, but it cannot be done as a xen-hvm.c thing.
> It should be somewhere generic, maybe under util? Stefan, any
> suggestions?

Yes, it seems like a util/ thing.  backtrace() and
backtrace_symbols_fd() are glibc-specific so it must not break the build
on other platforms.

I think the reason we've surivived without backtraces so far is because
fatal errors are typically handled with abort(3).  It causes a core dump
so you have the full process state, including backtraces.

I'm fine with adding a backtrace function though since it's more
lightweight and allows for graceful shutdown or error recovery.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]