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

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

[Konrad Rzeszutek Wilk]

Hey!

Since v2 (http://lists.xen.org/archives/html/xen-devel/2015-07/msg00431.html)
 - Added Reviewed-by.

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, patch 

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

was the only one that changed - where I implemented the configure script
as you suggested. All the rest are the same.

Please review or git pull the following tag:

git://xenbits.xen.org/people/konradwilk/qemu.git v2-cleanups-fixes-due-to-libxlABI


 configure                   | 26 ++++++++++++++++++++++++++
 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 ++--
 7 files changed, 58 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.

[Qemu-devel] [PATCH v2 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

[Qemu-devel] [PATCH v2 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

[Qemu-devel] [PATCH v2 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

[Qemu-devel] [PATCH v2 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.

Xen 4.6 did not change the libxc functions mentioned (same parameters)
so we piggyback on the fact that Xen 4.6 has a new function:
commit 504ed2053362381ac01b98db9313454488b7db40 "tools/libxc: Expose
new hypercall xc_reserved_device_memory_map" and check for that.

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

diff --git a/configure b/configure
index d854936..f162025 100755
--- a/configure
+++ b/configure
@@ -1889,6 +1889,32 @@ int main(void) {
   xc_gnttab_open(NULL, 0);
   xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
   xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
+  xc_reserved_device_memory_map(xc, 0, 0, 0, 0, NULL, 0);
+  return 0;
+}
+EOF
+      compile_prog "" "$xen_libs"
+    then
+    xen_ctrl_version=460
+    xen=yes
+
+  elif
+      cat > $TMPC <<EOF &&
+#include <xenctrl.h>
+#include <xenstore.h>
+#include <stdint.h>
+#include <xen/hvm/hvm_info_table.h>
+#if !defined(HVM_MAX_VCPUS)
+# error HVM_MAX_VCPUS not defined
+#endif
+int main(void) {
+  xc_interface *xc;
+  xs_daemon_open();
+  xc = xc_interface_open(0, 0, 0);
+  xc_hvm_set_mem_type(0, 0, HVMMEM_ram_ro, 0, 0);
+  xc_gnttab_open(NULL, 0);
+  xc_domain_add_to_physmap(0, 0, XENMAPSPACE_gmfn, 0, 0);
+  xc_hvm_inject_msi(xc, 0, 0xf0000000, 0x00000000);
   xc_hvm_create_ioreq_server(xc, 0, 0, NULL);
   return 0;
 }
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index ed5fd3e..3cd5875 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

[Qemu-devel] [PATCH v2 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.

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

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
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: [Qemu-devel] [PATCH v2] Cleanups + various fixes due to libxl ABI + more logging on errors.

[Stefano Stabellini]

Applied

On Tue, 8 Sep 2015, Konrad Rzeszutek Wilk wrote:
> Hey!
> 
> Since v2 (http://lists.xen.org/archives/html/xen-devel/2015-07/msg00431.html)
>  - Added Reviewed-by.
> 
> 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, patch 
> 
>  [PATCH v2 4/6] xen: use errno instead of rc for
> 
> was the only one that changed - where I implemented the configure script
> as you suggested. All the rest are the same.
> 
> Please review or git pull the following tag:
> 
> git://xenbits.xen.org/people/konradwilk/qemu.git v2-cleanups-fixes-due-to-libxlABI
> 
> 
>  configure                   | 26 ++++++++++++++++++++++++++
>  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 ++--
>  7 files changed, 58 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.
>