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

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

[Konrad Rzeszutek Wilk]

Hey!

since RFC [https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07326.html]
 - Added Acks
 - Followed 'xen: Print and use errno where applicable.' suggestion by Stefano
   and added a wrapper.

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).


Stefano, patches 1-3 are unchanged since the RFC posting (well, they have the
Ack tag added). Patch:

 [PATCH v1 4/6] xen: use errno instead of rc for xc_domain_add_to_physmap

uses the new wrapper you suggested.

The last two patches are new and I missed them the first time I posted:
 [PATCH v1 5/6] xen/pt/msi: Add the register value when printing
 [PATCH v1 6/6] xen/pt: Use XEN_PT_LOG properly to guard against

In short, please review #4,#5, and #6.


 hw/xen/xen_pt.c             |  6 +++---
 hw/xen/xen_pt.h             |  1 -
 hw/xen/xen_pt_config_init.c |  8 ++++----
 hw/xen/xen_pt_msi.c         |  2 +-
 include/hw/xen/xen_common.h | 22 ++++++++++++++++++++++
 xen-hvm.c                   |  4 ++--
 6 files changed, 32 insertions(+), 11 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: use errno instead of rc for xc_domain_add_to_physmap
      xen/pt/msi: Add the register value when printing logging and error messages
      xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.

[PATCH v1 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.

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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

[PATCH v1 2/6] xen/pt: Make xen_pt_msi_set_enable static

[Konrad Rzeszutek Wilk]

As we do not use it outside our code.

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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

[PATCH v1 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.

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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

[PATCH v1 4/6] xen: use errno instead of rc for xc_domain_add_to_physmap

[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 add an wrapper around
the xc_domain_add_to_physmap (called xen_xc_domain_add_to_physmap)
which will always return the EXX.

Suggested-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 include/hw/xen/xen_common.h | 22 ++++++++++++++++++++++
 xen-hvm.c                   |  4 ++--
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 38f29fb..fc66c3c 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -407,4 +407,26 @@ static inline int xen_set_ioreq_server_state(XenXC xc, domid_t dom,
 
 #endif
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 460
+static inline int xen_xc_domain_add_to_physmap(XenXC xch, uint32_t domid,
+                                               unsigned int space,
+                                               unsigned long idx,
+                                               xen_pfn_t gpfn)
+{
+    return xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
+}
+#else
+static inline int xen_xc_domain_add_to_physmap(XenXC xch, uint32_t domid,
+                                               unsigned int space,
+                                               unsigned long idx,
+                                               xen_pfn_t gpfn)
+{
+    /* In Xen 4.6 rc is -1 and errno contains the error value. */
+    int rc = xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
+    if (rc == -1)
+        return errno;
+    return rc;
+}
+#endif
+
 #endif /* QEMU_HW_XEN_COMMON_H */
diff --git a/xen-hvm.c b/xen-hvm.c
index 0408462..fae2d22 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -345,7 +345,7 @@ go_physmap:
         unsigned long idx = pfn + i;
         xen_pfn_t gpfn = start_gpfn + i;
 
-        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
+        rc = xen_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);
@@ -422,7 +422,7 @@ static int xen_remove_from_physmap(XenIOState *state,
         xen_pfn_t idx = start_addr + i;
         xen_pfn_t gpfn = phys_offset + i;
 
-        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
+        rc = xen_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);
-- 
2.1.0

Re: [PATCH v1 4/6] xen: use errno instead of rc for xc_domain_add_to_physmap

[Stefano Stabellini]

On Thu, 2 Jul 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 add an wrapper around
> the xc_domain_add_to_physmap (called xen_xc_domain_add_to_physmap)
> which will always return the EXX.
> 
> Suggested-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  include/hw/xen/xen_common.h | 22 ++++++++++++++++++++++
>  xen-hvm.c                   |  4 ++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 38f29fb..fc66c3c 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -407,4 +407,26 @@ static inline int xen_set_ioreq_server_state(XenXC xc, domid_t dom,
>  
>  #endif
>  
> +#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 460

This patch is fine as is and you can add my

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

however there is no code to detect 460 at the moment, see
configure:1844. We need one more test case in the configure script to
detect 4.6.


> +static inline int xen_xc_domain_add_to_physmap(XenXC xch, uint32_t domid,
> +                                               unsigned int space,
> +                                               unsigned long idx,
> +                                               xen_pfn_t gpfn)
> +{
> +    return xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
> +}
> +#else
> +static inline int xen_xc_domain_add_to_physmap(XenXC xch, uint32_t domid,
> +                                               unsigned int space,
> +                                               unsigned long idx,
> +                                               xen_pfn_t gpfn)
> +{
> +    /* In Xen 4.6 rc is -1 and errno contains the error value. */
> +    int rc = xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
> +    if (rc == -1)
> +        return errno;
> +    return rc;
> +}
> +#endif
> +
>  #endif /* QEMU_HW_XEN_COMMON_H */
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 0408462..fae2d22 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -345,7 +345,7 @@ go_physmap:
>          unsigned long idx = pfn + i;
>          xen_pfn_t gpfn = start_gpfn + i;
>  
> -        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> +        rc = xen_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);
> @@ -422,7 +422,7 @@ static int xen_remove_from_physmap(XenIOState *state,
>          xen_pfn_t idx = start_addr + i;
>          xen_pfn_t gpfn = phys_offset + i;
>  
> -        rc = xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
> +        rc = xen_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);
> -- 
> 2.1.0
> 

[PATCH v1 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

[PATCH v1 6/6] xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.

[Konrad Rzeszutek Wilk]

If XEN_PT_LOGGING_ENABLED is enabled the XEN_PT_LOG macros start
using the first argument. Which means if within the function there
is only one user of the argument ('d') and XEN_PT_LOGGING_ENABLED
is not set, we get compiler warnings. This is not the case now
but with the "xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config"
we will hit - so this sync up the function to the rest of them.

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

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 462e1b9..21d4938 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1418,7 +1418,7 @@ static int xen_pt_msixctrl_reg_init(XenPCIPassthroughState *s,
     reg_field = pci_get_word(d->config + real_offset);
 
     if (reg_field & PCI_MSIX_FLAGS_ENABLE) {
-        XEN_PT_LOG(d, "MSIX already enabled, disabling it first\n");
+        XEN_PT_LOG(&s->dev, "MSIX already enabled, disabling it first\n");
         xen_host_pci_set_word(&s->real_device, real_offset,
                               reg_field & ~PCI_MSIX_FLAGS_ENABLE);
     }
-- 
2.1.0

Re: [PATCH v1 6/6] xen/pt: Use XEN_PT_LOG properly to guard against compiler warnings.

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> If XEN_PT_LOGGING_ENABLED is enabled the XEN_PT_LOG macros start
> using the first argument. Which means if within the function there
> is only one user of the argument ('d') and XEN_PT_LOGGING_ENABLED
> is not set, we get compiler warnings. This is not the case now
> but with the "xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config"
> we will hit - so this sync up the function to the rest of them.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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


>  hw/xen/xen_pt_config_init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 462e1b9..21d4938 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1418,7 +1418,7 @@ static int xen_pt_msixctrl_reg_init(XenPCIPassthroughState *s,
>      reg_field = pci_get_word(d->config + real_offset);
>  
>      if (reg_field & PCI_MSIX_FLAGS_ENABLE) {
> -        XEN_PT_LOG(d, "MSIX already enabled, disabling it first\n");
> +        XEN_PT_LOG(&s->dev, "MSIX already enabled, disabling it first\n");
>          xen_host_pci_set_word(&s->real_device, real_offset,
>                                reg_field & ~PCI_MSIX_FLAGS_ENABLE);
>      }
> -- 
> 2.1.0
> 

Re: [PATCH v1 5/6] xen/pt/msi: Add the register value when printing logging and error messages

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> 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>

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.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
>