[PATCH v1 01/10] xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config

[PATCH v1 01/10] xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config

[Konrad Rzeszutek Wilk]

During init time we treat the dev.config area as a cache
of the host view. However during execution time we treat it
as guest view (by the generic PCI API). We need to sync Xen's
code to the generic PCI API view. This is the first step
by replacing all of the code that uses dev.config or
pci_get_[byte|word] to get host value to actually use the
xen_host_pci_get_[byte|word] functions.

Interestingly in 'xen_pt_ptr_reg_init' we also needed to swap
reg_field from uint32_t to uint8_t - since the access is only
for one byte not four bytes. We can split this as a seperate
patch however we would have to use a cast to thwart compiler
warnings in the meantime.

We also truncated 'flags' to 'flag' to make the code fit within
the 80 characters.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c             | 24 +++++++++++---
 hw/xen/xen_pt_config_init.c | 77 +++++++++++++++++++++++++++++++--------------
 2 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index ea1ceda..7575311 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -689,7 +689,7 @@ static int xen_pt_initfn(PCIDevice *d)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
     int rc = 0;
-    uint8_t machine_irq = 0;
+    uint8_t machine_irq = 0, scratch;
     uint16_t cmd = 0;
     int pirq = XEN_PT_UNASSIGNED_PIRQ;
 
@@ -735,7 +735,12 @@ static int xen_pt_initfn(PCIDevice *d)
     }
 
     /* Bind interrupt */
-    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
+    rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
+    if (rc) {
+        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
+        scratch = 0;
+    }
+    if (!scratch) {
         XEN_PT_LOG(d, "no pin interrupt\n");
         goto out;
     }
@@ -785,8 +790,19 @@ static int xen_pt_initfn(PCIDevice *d)
 
 out:
     if (cmd) {
-        xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
-                              pci_get_word(d->config + PCI_COMMAND) | cmd);
+        uint16_t val;
+
+        rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
+        if (rc) {
+            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+        } else {
+            val |= cmd;
+            rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
+            if (rc) {
+                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
+                           val, rc);
+            }
+        }
     }
 
     memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 21d4938..e34f9f8 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -800,15 +800,21 @@ static XenPTRegInfo xen_pt_emu_reg_vendor[] = {
 static inline uint8_t get_capability_version(XenPCIPassthroughState *s,
                                              uint32_t offset)
 {
-    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
-    return flags & PCI_EXP_FLAGS_VERS;
+    uint8_t flag;
+    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
+        return 0;
+    }
+    return flag & PCI_EXP_FLAGS_VERS;
 }
 
 static inline uint8_t get_device_type(XenPCIPassthroughState *s,
                                       uint32_t offset)
 {
-    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
-    return (flags & PCI_EXP_FLAGS_TYPE) >> 4;
+    uint8_t flag;
+    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
+        return 0;
+    }
+    return (flag & PCI_EXP_FLAGS_TYPE) >> 4;
 }
 
 /* initialize Link Control register */
@@ -857,8 +863,14 @@ static int xen_pt_linkctrl2_reg_init(XenPCIPassthroughState *s,
         reg_field = XEN_PT_INVALID_REG;
     } else {
         /* set Supported Link Speed */
-        uint8_t lnkcap = pci_get_byte(s->dev.config + real_offset - reg->offset
-                                      + PCI_EXP_LNKCAP);
+        uint8_t lnkcap;
+        int rc;
+        rc = xen_host_pci_get_byte(&s->real_device,
+                                   real_offset - reg->offset + PCI_EXP_LNKCAP,
+                                   &lnkcap);
+        if (rc) {
+            return rc;
+        }
         reg_field |= PCI_EXP_LNKCAP_SLS & lnkcap;
     }
 
@@ -1039,13 +1051,15 @@ static int xen_pt_msgctrl_reg_init(XenPCIPassthroughState *s,
                                    XenPTRegInfo *reg, uint32_t real_offset,
                                    uint32_t *data)
 {
-    PCIDevice *d = &s->dev;
     XenPTMSI *msi = s->msi;
-    uint16_t reg_field = 0;
+    uint16_t reg_field;
+    int rc;
 
     /* use I/O device register's value as initial value */
-    reg_field = pci_get_word(d->config + real_offset);
-
+    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
+    if (rc) {
+        return rc;
+    }
     if (reg_field & PCI_MSI_FLAGS_ENABLE) {
         XEN_PT_LOG(&s->dev, "MSI already enabled, disabling it first\n");
         xen_host_pci_set_word(&s->real_device, real_offset,
@@ -1411,12 +1425,14 @@ static int xen_pt_msixctrl_reg_init(XenPCIPassthroughState *s,
                                     XenPTRegInfo *reg, uint32_t real_offset,
                                     uint32_t *data)
 {
-    PCIDevice *d = &s->dev;
-    uint16_t reg_field = 0;
+    uint16_t reg_field;
+    int rc;
 
     /* use I/O device register's value as initial value */
-    reg_field = pci_get_word(d->config + real_offset);
-
+    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
+    if (rc) {
+        return rc;
+    }
     if (reg_field & PCI_MSIX_FLAGS_ENABLE) {
         XEN_PT_LOG(&s->dev, "MSIX already enabled, disabling it first\n");
         xen_host_pci_set_word(&s->real_device, real_offset,
@@ -1511,8 +1527,7 @@ static int xen_pt_vendor_size_init(XenPCIPassthroughState *s,
                                    const XenPTRegGroupInfo *grp_reg,
                                    uint32_t base_offset, uint8_t *size)
 {
-    *size = pci_get_byte(s->dev.config + base_offset + 0x02);
-    return 0;
+    return xen_host_pci_get_byte(&s->real_device, base_offset + 0x02, size);
 }
 /* get PCI Express Capability Structure register group size */
 static int xen_pt_pcie_size_init(XenPCIPassthroughState *s,
@@ -1591,12 +1606,15 @@ static int xen_pt_msi_size_init(XenPCIPassthroughState *s,
                                 const XenPTRegGroupInfo *grp_reg,
                                 uint32_t base_offset, uint8_t *size)
 {
-    PCIDevice *d = &s->dev;
     uint16_t msg_ctrl = 0;
     uint8_t msi_size = 0xa;
+    int rc;
 
-    msg_ctrl = pci_get_word(d->config + (base_offset + PCI_MSI_FLAGS));
-
+    rc = xen_host_pci_get_word(&s->real_device, base_offset + PCI_MSI_FLAGS,
+                               &msg_ctrl);
+    if (rc) {
+        return rc;
+    }
     /* check if 64-bit address is capable of per-vector masking */
     if (msg_ctrl & PCI_MSI_FLAGS_64BIT) {
         msi_size += 4;
@@ -1739,11 +1757,14 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
                                XenPTRegInfo *reg, uint32_t real_offset,
                                uint32_t *data)
 {
-    int i;
-    uint8_t *config = s->dev.config;
-    uint32_t reg_field = pci_get_byte(config + real_offset);
+    int i, rc;
+    uint8_t reg_field;
     uint8_t cap_id = 0;
 
+    rc = xen_host_pci_get_byte(&s->real_device, real_offset, &reg_field);
+    if (rc) {
+        return rc;
+    }
     /* find capability offset */
     while (reg_field) {
         for (i = 0; xen_pt_emu_reg_grps[i].grp_size != 0; i++) {
@@ -1752,7 +1773,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
                 continue;
             }
 
-            cap_id = pci_get_byte(config + reg_field + PCI_CAP_LIST_ID);
+            rc = xen_host_pci_get_byte(&s->real_device,
+                                       reg_field + PCI_CAP_LIST_ID, &cap_id);
+            if (rc) {
+                return rc;
+            }
             if (xen_pt_emu_reg_grps[i].grp_id == cap_id) {
                 if (xen_pt_emu_reg_grps[i].grp_type == XEN_PT_GRP_TYPE_EMU) {
                     goto out;
@@ -1763,7 +1788,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
         }
 
         /* next capability */
-        reg_field = pci_get_byte(config + reg_field + PCI_CAP_LIST_NEXT);
+        rc = xen_host_pci_get_byte(&s->real_device,
+                                   reg_field + PCI_CAP_LIST_NEXT, &reg_field);
+        if (rc) {
+            return rc;
+        }
     }
 
 out:
-- 
2.1.0

[PATCH v1 02/10] xen/pt: Sync up the dev.config and data values.

[Konrad Rzeszutek Wilk]

For a passthrough device we maintain a state of emulated
registers value contained within d->config. We also consult
the host registers (and apply ro and write masks) whenever
the guest access the registers. This is done in xen_pt_pci_write_config
and xen_pt_pci_read_config.

Also in this picture we call pci_default_write_config which
updates the d->config and if the d->config[PCI_COMMAND] register
has PCI_COMMAND_MEMORY (or PCI_COMMAND_IO) acts on those changes.

On startup the d->config[PCI_COMMAND] are the host values, not
what the guest initial values should be, which is exactly what
we do _not_ want to do for 64-bit BARs when the guest just wants
to read the size of the BAR. Huh you say?

To get the size of 64-bit memory space BARs,  the guest has
to calculate ((BAR[x] & 0xFFFFFFF0) + ((BAR[x+1] & 0xFFFFFFFF) << 32))
which means it has to do two writes of ~0 to BARx and BARx+1.

prior to this patch and with XSA120-addendum patch (Linux kernel)
the PCI_COMMAND register is copied from the host it can have
PCI_COMMAND_MEMORY bit set which means that QEMU will try to
update the hypervisor's P2M with BARx+1 value to ~0 (0xffffffff)
(to sync the guest state to host) instead of just having
xen_pt_pci_write_config and xen_pt_bar_reg_write apply the
proper masks and return the size to the guest.

To thwart this, this patch syncs up the host values with the
guest values taking into account the emu_mask (bit set means
we emulate, PCI_COMMAND_MEMORY and PCI_COMMAND_IO are set).
That is we copy the host values - masking out any bits which
we will emulate. Then merge it with the initial emulation register
values. Lastly this value is then copied both in
dev.config _and_ XenPTReg->data field.

There is also reg->size accounting taken into consideration
that ends up being used in two patches:
 xen/pt: Check if reg->init function sets the 'data' past the reg->size
 xen/pt: Use xen_host_pci_get_[byte,word,long] instead of xen_host_pci_get_long

This fixes errors such as these:

(XEN) memory_map:add: dom2 gfn=fffe0 mfn=fbce0 nr=20
(DEBUG) 189 pci dev 04:0 BAR16 wrote ~0.
(DEBUG) 200 pci dev 04:0 BAR16 read 0x0fffe0004.
(XEN) memory_map:remove: dom2 gfn=fffe0 mfn=fbce0 nr=20
(DEBUG) 204 pci dev 04:0 BAR16 wrote 0x0fffe0004.
(DEBUG) 217 pci dev 04:0 BAR16 read upper 0x000000000.
(XEN) memory_map:add: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
(XEN) p2m.c:883:d0v0 p2m_set_entry failed! mfn=ffffffffffffffff rc:-22
(XEN) memory_map:fail: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20 ret:-22
(XEN) memory_map:remove: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
(XEN) p2m.c:920:d0v0 gfn_to_mfn failed! gfn=ffffffff00000 type:4
(XEN) p2m.c:920:d0v0 gfn_to_mfn failed! gfn=ffffffff00001 type:4
..
(XEN) memory_map: error -22 removing dom2 access to [fbce0,fbcff]
(DEBUG) 222 pci dev 04:0 BAR16 read upper 0x0ffffffff.
(XEN) memory_map:remove: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
(XEN) memory_map: error -22 removing dom2 access to [fbce0,fbcff]

[The DEBUG is to illustate what the hvmloader was doing]

Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt_config_init.c | 48 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index e34f9f8..3938afd 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1856,6 +1856,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
     reg_entry->reg = reg;
 
     if (reg->init) {
+        uint32_t host_mask, size_mask;
+        unsigned int offset;
+        uint32_t val;
+
         /* initialize emulate register */
         rc = reg->init(s, reg_entry->reg,
                        reg_grp->base_offset + reg->offset, &data);
@@ -1868,8 +1872,50 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
             g_free(reg_entry);
             return 0;
         }
+        /* Sync up the data to dev.config */
+        offset = reg_grp->base_offset + reg->offset;
+        size_mask = 0xFFFFFFFF >> ((4 - reg->size) << 3);
+
+        rc = xen_host_pci_get_long(&s->real_device, offset, &val);
+        if (rc) {
+            /* Serious issues when we cannot read the host values! */
+            g_free(reg_entry);
+            return rc;
+        }
+        /* Set bits in emu_mask are the ones we emulate. The dev.config shall
+         * contain the emulated view of the guest - therefore we flip the mask
+         * to mask out the host values (which dev.config initially has) . */
+        host_mask = size_mask & ~reg->emu_mask;
+
+        if ((data & host_mask) != (val & host_mask)) {
+            uint32_t new_val;
+
+            /* Mask out host (including past size). */
+            new_val = val & host_mask;
+            /* Merge emulated ones (excluding the non-emulated ones). */
+            new_val |= data & host_mask;
+            /* Leave intact host and emulated values past the size - even though
+             * we do not care as we write per reg->size granularity, but for the
+             * logging below lets have the proper value. */
+            new_val |= ((val | data)) & ~size_mask;
+            XEN_PT_LOG(&s->dev,"Offset 0x%04x mismatch! Emulated=0x%04x, host=0x%04x, syncing to 0x%04x.\n",
+                       offset, data, val, new_val);
+            val = new_val;
+        } else
+            val = data;
+
+        /* This could be just pci_set_long as we don't modify the bits
+         * past reg->size, but in case this routine is run in parallel
+         * we do not want to over-write other registers. */
+        switch (reg->size) {
+        case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
+        case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
+        case 4: pci_set_long(s->dev.config + offset, val); break;
+        default: assert(1);
+        }
         /* set register value */
-        reg_entry->data = data;
+        reg_entry->data = val;
+
     }
     /* list add register entry */
     QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
-- 
2.1.0

Re: [PATCH v1 02/10] xen/pt: Sync up the dev.config and data values.

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> For a passthrough device we maintain a state of emulated
> registers value contained within d->config. We also consult
> the host registers (and apply ro and write masks) whenever
> the guest access the registers. This is done in xen_pt_pci_write_config
> and xen_pt_pci_read_config.
> 
> Also in this picture we call pci_default_write_config which
> updates the d->config and if the d->config[PCI_COMMAND] register
> has PCI_COMMAND_MEMORY (or PCI_COMMAND_IO) acts on those changes.
> 
> On startup the d->config[PCI_COMMAND] are the host values, not
> what the guest initial values should be, which is exactly what
> we do _not_ want to do for 64-bit BARs when the guest just wants
> to read the size of the BAR. Huh you say?
> 
> To get the size of 64-bit memory space BARs,  the guest has
> to calculate ((BAR[x] & 0xFFFFFFF0) + ((BAR[x+1] & 0xFFFFFFFF) << 32))
> which means it has to do two writes of ~0 to BARx and BARx+1.
> 
> prior to this patch and with XSA120-addendum patch (Linux kernel)
> the PCI_COMMAND register is copied from the host it can have
> PCI_COMMAND_MEMORY bit set which means that QEMU will try to
> update the hypervisor's P2M with BARx+1 value to ~0 (0xffffffff)
> (to sync the guest state to host) instead of just having
> xen_pt_pci_write_config and xen_pt_bar_reg_write apply the
> proper masks and return the size to the guest.
> 
> To thwart this, this patch syncs up the host values with the
> guest values taking into account the emu_mask (bit set means
> we emulate, PCI_COMMAND_MEMORY and PCI_COMMAND_IO are set).
> That is we copy the host values - masking out any bits which
> we will emulate. Then merge it with the initial emulation register
> values. Lastly this value is then copied both in
> dev.config _and_ XenPTReg->data field.
> 
> There is also reg->size accounting taken into consideration
> that ends up being used in two patches:
>  xen/pt: Check if reg->init function sets the 'data' past the reg->size
>  xen/pt: Use xen_host_pci_get_[byte,word,long] instead of xen_host_pci_get_long
> 
> This fixes errors such as these:
> 
> (XEN) memory_map:add: dom2 gfn=fffe0 mfn=fbce0 nr=20
> (DEBUG) 189 pci dev 04:0 BAR16 wrote ~0.
> (DEBUG) 200 pci dev 04:0 BAR16 read 0x0fffe0004.
> (XEN) memory_map:remove: dom2 gfn=fffe0 mfn=fbce0 nr=20
> (DEBUG) 204 pci dev 04:0 BAR16 wrote 0x0fffe0004.
> (DEBUG) 217 pci dev 04:0 BAR16 read upper 0x000000000.
> (XEN) memory_map:add: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
> (XEN) p2m.c:883:d0v0 p2m_set_entry failed! mfn=ffffffffffffffff rc:-22
> (XEN) memory_map:fail: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20 ret:-22
> (XEN) memory_map:remove: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
> (XEN) p2m.c:920:d0v0 gfn_to_mfn failed! gfn=ffffffff00000 type:4
> (XEN) p2m.c:920:d0v0 gfn_to_mfn failed! gfn=ffffffff00001 type:4
> ..
> (XEN) memory_map: error -22 removing dom2 access to [fbce0,fbcff]
> (DEBUG) 222 pci dev 04:0 BAR16 read upper 0x0ffffffff.
> (XEN) memory_map:remove: dom2 gfn=ffffffff00000 mfn=fbce0 nr=20
> (XEN) memory_map: error -22 removing dom2 access to [fbce0,fbcff]
> 
> [The DEBUG is to illustate what the hvmloader was doing]
> 
> Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
> 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 | 48 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index e34f9f8..3938afd 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1856,6 +1856,10 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>      reg_entry->reg = reg;
>  
>      if (reg->init) {
> +        uint32_t host_mask, size_mask;
> +        unsigned int offset;
> +        uint32_t val;
> +
>          /* initialize emulate register */
>          rc = reg->init(s, reg_entry->reg,
>                         reg_grp->base_offset + reg->offset, &data);
> @@ -1868,8 +1872,50 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>              g_free(reg_entry);
>              return 0;
>          }
> +        /* Sync up the data to dev.config */
> +        offset = reg_grp->base_offset + reg->offset;
> +        size_mask = 0xFFFFFFFF >> ((4 - reg->size) << 3);
> +
> +        rc = xen_host_pci_get_long(&s->real_device, offset, &val);
> +        if (rc) {
> +            /* Serious issues when we cannot read the host values! */
> +            g_free(reg_entry);
> +            return rc;
> +        }
> +        /* Set bits in emu_mask are the ones we emulate. The dev.config shall
> +         * contain the emulated view of the guest - therefore we flip the mask
> +         * to mask out the host values (which dev.config initially has) . */
> +        host_mask = size_mask & ~reg->emu_mask;
> +
> +        if ((data & host_mask) != (val & host_mask)) {
> +            uint32_t new_val;
> +
> +            /* Mask out host (including past size). */
> +            new_val = val & host_mask;
> +            /* Merge emulated ones (excluding the non-emulated ones). */
> +            new_val |= data & host_mask;
> +            /* Leave intact host and emulated values past the size - even though
> +             * we do not care as we write per reg->size granularity, but for the
> +             * logging below lets have the proper value. */
> +            new_val |= ((val | data)) & ~size_mask;
> +            XEN_PT_LOG(&s->dev,"Offset 0x%04x mismatch! Emulated=0x%04x, host=0x%04x, syncing to 0x%04x.\n",
> +                       offset, data, val, new_val);
> +            val = new_val;
> +        } else
> +            val = data;
> +
> +        /* This could be just pci_set_long as we don't modify the bits
> +         * past reg->size, but in case this routine is run in parallel
> +         * we do not want to over-write other registers. */
> +        switch (reg->size) {
> +        case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
> +        case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
> +        case 4: pci_set_long(s->dev.config + offset, val); break;
> +        default: assert(1);
> +        }
>          /* set register value */
> -        reg_entry->data = data;
> +        reg_entry->data = val;
> +
>      }
>      /* list add register entry */
>      QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
> -- 
> 2.1.0
> 

[PATCH v1 03/10] xen/pt: Check if reg->init function sets the 'data' past the reg->size

[Konrad Rzeszutek Wilk]

It should never happen, but in case it does (an developer adds
a new register and the 'init_val' expands past the register
size) we want to report. The code will only write up to
reg->size so there is no runtime danger of the register spilling
across other ones - however to catch this sort of thing
we still return an error.

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

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 3938afd..09309ba 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1904,9 +1904,15 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         } else
             val = data;
 
+        if (val & ~size_mask) {
+            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
+                       offset, val, reg->size);
+            g_free(reg_entry);
+            return -ENXIO;
+        }
         /* This could be just pci_set_long as we don't modify the bits
-         * past reg->size, but in case this routine is run in parallel
-         * we do not want to over-write other registers. */
+         * past reg->size, but in case this routine is run in parallel or the
+         * init value is larger, we do not want to over-write registers. */
         switch (reg->size) {
         case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
         case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
-- 
2.1.0

Re: [PATCH v1 03/10] xen/pt: Check if reg->init function sets the 'data' past the reg->size

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> It should never happen, but in case it does (an developer adds
> a new register and the 'init_val' expands past the register
> size) we want to report. The code will only write up to
> reg->size so there is no runtime danger of the register spilling
> across other ones - however to catch this sort of thing
> we still return an error.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  hw/xen/xen_pt_config_init.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 3938afd..09309ba 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1904,9 +1904,15 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>          } else
>              val = data;
>  
> +        if (val & ~size_mask) {
> +            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
> +                       offset, val, reg->size);
> +            g_free(reg_entry);
> +            return -ENXIO;
> +        }

If we worry about changes to init_val, wouldn't it be better to add
QEMU_BUILD_BUG_ON(data & ~size_mask)?


>          /* This could be just pci_set_long as we don't modify the bits
> -         * past reg->size, but in case this routine is run in parallel
> -         * we do not want to over-write other registers. */
> +         * past reg->size, but in case this routine is run in parallel or the
> +         * init value is larger, we do not want to over-write registers. */
>          switch (reg->size) {
>          case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
>          case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
> -- 
> 2.1.0
> 

Re: [PATCH v1 03/10] xen/pt: Check if reg->init function sets the 'data' past the reg->size

[Konrad Rzeszutek Wilk]

On Fri, Jul 17, 2015 at 05:03:44PM +0100, Stefano Stabellini wrote:
> On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> > It should never happen, but in case it does (an developer adds
> > a new register and the 'init_val' expands past the register
> > size) we want to report. The code will only write up to
> > reg->size so there is no runtime danger of the register spilling
> > across other ones - however to catch this sort of thing
> > we still return an error.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  hw/xen/xen_pt_config_init.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> > index 3938afd..09309ba 100644
> > --- a/hw/xen/xen_pt_config_init.c
> > +++ b/hw/xen/xen_pt_config_init.c
> > @@ -1904,9 +1904,15 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> >          } else
> >              val = data;
> >  
> > +        if (val & ~size_mask) {
> > +            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
> > +                       offset, val, reg->size);
> > +            g_free(reg_entry);
> > +            return -ENXIO;
> > +        }
> 
> If we worry about changes to init_val, wouldn't it be better to add
> QEMU_BUILD_BUG_ON(data & ~size_mask)?

Duh. Yes :-)
> 
> 
> >          /* This could be just pci_set_long as we don't modify the bits
> > -         * past reg->size, but in case this routine is run in parallel
> > -         * we do not want to over-write other registers. */
> > +         * past reg->size, but in case this routine is run in parallel or the
> > +         * init value is larger, we do not want to over-write registers. */
> >          switch (reg->size) {
> >          case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
> >          case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
> > -- 
> > 2.1.0
> > 

Re: [PATCH v1 03/10] xen/pt: Check if reg->init function sets the 'data' past the reg->size

[Konrad Rzeszutek Wilk]

On Fri, Jul 17, 2015 at 05:03:44PM +0100, Stefano Stabellini wrote:
> On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> > It should never happen, but in case it does (an developer adds
> > a new register and the 'init_val' expands past the register
> > size) we want to report. The code will only write up to
> > reg->size so there is no runtime danger of the register spilling
> > across other ones - however to catch this sort of thing
> > we still return an error.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  hw/xen/xen_pt_config_init.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> > index 3938afd..09309ba 100644
> > --- a/hw/xen/xen_pt_config_init.c
> > +++ b/hw/xen/xen_pt_config_init.c
> > @@ -1904,9 +1904,15 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
> >          } else
> >              val = data;
> >  
> > +        if (val & ~size_mask) {
> > +            XEN_PT_ERR(&s->dev,"Offset 0x%04x:0x%04x expands past register size(%d)!\n",
> > +                       offset, val, reg->size);
> > +            g_free(reg_entry);
> > +            return -ENXIO;
> > +        }
> 
> If we worry about changes to init_val, wouldn't it be better to add
> QEMU_BUILD_BUG_ON(data & ~size_mask)?

I couldnt' figure out how to make that work nicely.

The QEMU_BUILD_BUG_ON look to be build time - not run-time.

Which means that doing:
	for (i = 0; i < grp_entries; i++)
	{
		entries = grp_entries[i]...
		for (j = 0; j < entries; j++)
			QEMU_BUILD_BUG_ON(entries[j].init_val & ~size_mask)
	}

is not something I can image the compiler working with?

> 
> 
> >          /* This could be just pci_set_long as we don't modify the bits
> > -         * past reg->size, but in case this routine is run in parallel
> > -         * we do not want to over-write other registers. */
> > +         * past reg->size, but in case this routine is run in parallel or the
> > +         * init value is larger, we do not want to over-write registers. */
> >          switch (reg->size) {
> >          case 1: pci_set_byte(s->dev.config + offset, (uint8_t)val); break;
> >          case 2: pci_set_word(s->dev.config + offset, (uint16_t)val); break;
> > -- 
> > 2.1.0
> > 

[PATCH v1 04/10] xen/pt: Use xen_host_pci_get_[byte, word, long] instead of xen_host_pci_get_long

[Konrad Rzeszutek Wilk]

Otherwise we get:

xen_pt_config_reg_init: Offset 0x0004 mismatch! Emulated=0x0000, host=0x2300017, syncing to 0x2300014.
xen_pt_config_reg_init: Error: Offset 0x0004:0x2300014 expands past register size(2)!

which is not surprising. We read the value as an 32-bit (from host),
then operate it as a 16-bit - and the remainder is left unchanged.

We end up writting the value as 16-bit (so 0014) to dev.config
(as we use proper xen_set_host_[byte,word,long] so we don't spill
to other registers) but in XenPTReg->data it is as 32-bit (0x2300014)!

It is harmless as the read/write functions end up using an size mask
and never modify the bits past 16-bit (reg->size is 2).

This patch fixes the warnings by reading the value using the
proper size.

Note that the check for size is still left in-case the developer
sets bits past the reg->size in the ->init routines.

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

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 09309ba..e597993 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1876,7 +1876,12 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         offset = reg_grp->base_offset + reg->offset;
         size_mask = 0xFFFFFFFF >> ((4 - reg->size) << 3);
 
-        rc = xen_host_pci_get_long(&s->real_device, offset, &val);
+        switch (reg->size) {
+        case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val);break;
+        case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val);break;
+        case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val);break;
+        default: assert(1);
+        }
         if (rc) {
             /* Serious issues when we cannot read the host values! */
             g_free(reg_entry);
-- 
2.1.0

Re: [PATCH v1 04/10] xen/pt: Use xen_host_pci_get_[byte, word, long] instead of xen_host_pci_get_long

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> Otherwise we get:
> 
> xen_pt_config_reg_init: Offset 0x0004 mismatch! Emulated=0x0000, host=0x2300017, syncing to 0x2300014.
> xen_pt_config_reg_init: Error: Offset 0x0004:0x2300014 expands past register size(2)!
> 
> which is not surprising. We read the value as an 32-bit (from host),
> then operate it as a 16-bit - and the remainder is left unchanged.
> 
> We end up writting the value as 16-bit (so 0014) to dev.config
             ^ writing


> (as we use proper xen_set_host_[byte,word,long] so we don't spill
> to other registers) but in XenPTReg->data it is as 32-bit (0x2300014)!
> 
> It is harmless as the read/write functions end up using an size mask
> and never modify the bits past 16-bit (reg->size is 2).
> 
> This patch fixes the warnings by reading the value using the
> proper size.
> 
> Note that the check for size is still left in-case the developer
> sets bits past the reg->size in the ->init routines.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  hw/xen/xen_pt_config_init.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 09309ba..e597993 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1876,7 +1876,12 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>          offset = reg_grp->base_offset + reg->offset;
>          size_mask = 0xFFFFFFFF >> ((4 - reg->size) << 3);
>  
> -        rc = xen_host_pci_get_long(&s->real_device, offset, &val);
> +        switch (reg->size) {
> +        case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val);break;
> +        case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val);break;
> +        case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val);break;
> +        default: assert(1);
> +        }
>          if (rc) {
>              /* Serious issues when we cannot read the host values! */
>              g_free(reg_entry);

Please merge this patch with patch #2/10.
It is best to place the break statements on new lines, to follow QEMU's code style.

[PATCH v1 05/10] xen/pt: Remove XenPTReg->data field.

[Konrad Rzeszutek Wilk]

We do not want to have two entries to cache the guest configuration
registers: XenPTReg->data and dev.config. Instead we want to use
only the dev.config.

To do without much complications we rip out the ->data field
and replace it with an pointer to the dev.config. This way we
have the type-checking (uint8_t, uint16_t, etc) and as well
and pre-computed location.

Alternatively we could compute the offset in dev.config by
using the XenPTRRegInfo and XenPTRegGroup every time but
this way we have the pre-computed values.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.h             |  6 +++-
 hw/xen/xen_pt_config_init.c | 73 +++++++++++++++++++++++++++------------------
 2 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 09358b1..586d055 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -134,7 +134,11 @@ struct XenPTRegInfo {
 struct XenPTReg {
     QLIST_ENTRY(XenPTReg) entries;
     XenPTRegInfo *reg;
-    uint32_t data; /* emulated value */
+    union {
+        uint8_t *byte;
+        uint16_t *word;
+        uint32_t *dbword;
+    } ptr; /* pointer to dev.config. */
 };
 
 typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index e597993..0a710a2 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -128,10 +128,11 @@ static int xen_pt_byte_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint8_t valid_emu_mask = 0;
+    uint8_t *data = cfg_entry->ptr.byte;
 
     /* emulate byte register */
     valid_emu_mask = reg->emu_mask & valid_mask;
-    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
+    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
 
     return 0;
 }
@@ -140,10 +141,11 @@ static int xen_pt_word_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t valid_emu_mask = 0;
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* emulate word register */
     valid_emu_mask = reg->emu_mask & valid_mask;
-    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
+    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
 
     return 0;
 }
@@ -152,10 +154,11 @@ static int xen_pt_long_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t valid_emu_mask = 0;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     /* emulate long register */
     valid_emu_mask = reg->emu_mask & valid_mask;
-    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
+    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
 
     return 0;
 }
@@ -169,10 +172,11 @@ static int xen_pt_byte_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     XenPTRegInfo *reg = cfg_entry->reg;
     uint8_t writable_mask = 0;
     uint8_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint8_t *data = cfg_entry->ptr.byte;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -186,10 +190,11 @@ static int xen_pt_word_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -203,10 +208,11 @@ static int xen_pt_long_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
     uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -255,7 +261,7 @@ static int xen_pt_status_reg_init(XenPCIPassthroughState *s,
         reg_entry = xen_pt_find_reg(reg_grp_entry, PCI_CAPABILITY_LIST);
         if (reg_entry) {
             /* check Capabilities Pointer register */
-            if (reg_entry->data) {
+            if (*reg_entry->ptr.word) {
                 reg_field |= PCI_STATUS_CAP_LIST;
             } else {
                 reg_field &= ~PCI_STATUS_CAP_LIST;
@@ -301,10 +307,11 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* modify emulate register */
     writable_mask = ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     if (*val & PCI_COMMAND_INTX_DISABLE) {
@@ -447,7 +454,7 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 
     /* emulate BAR */
     valid_emu_mask = bar_emu_mask & valid_mask;
-    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
+    *value = XEN_PT_MERGE_VALUE(*value, *cfg_entry->ptr.dbword, ~valid_emu_mask);
 
     return 0;
 }
@@ -464,6 +471,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
     uint32_t bar_ro_mask = 0;
     uint32_t r_size = 0;
     int index = 0;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     index = xen_pt_bar_offset_to_index(reg->offset);
     if (index < 0 || index >= PCI_NUM_REGIONS) {
@@ -500,7 +508,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
 
     /* modify emulate register */
     writable_mask = bar_emu_mask & ~bar_ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* check whether we need to update the virtual region address or not */
     switch (s->bases[index].bar_flag) {
@@ -533,6 +541,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
     uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
     pcibus_t r_size = 0;
     uint32_t bar_ro_mask = 0;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     r_size = d->io_regions[PCI_ROM_SLOT].size;
     base = &s->bases[PCI_ROM_SLOT];
@@ -544,7 +553,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
 
     /* modify emulate register */
     writable_mask = ~bar_ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -983,10 +992,11 @@ static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
     XenPTRegInfo *reg = cfg_entry->reg;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
@@ -1081,6 +1091,7 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
     XenPTMSI *msi = s->msi;
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* Currently no support for multi-vector */
     if (*val & PCI_MSI_FLAGS_QSIZE) {
@@ -1089,8 +1100,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
-    msi->flags |= cfg_entry->data & ~PCI_MSI_FLAGS_ENABLE;
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
+    msi->flags |= *data & ~PCI_MSI_FLAGS_ENABLE;
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -1204,18 +1215,19 @@ static int xen_pt_msgaddr32_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
-    uint32_t old_addr = cfg_entry->data;
+    uint32_t old_addr = *cfg_entry->ptr.dbword;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
-    s->msi->addr_lo = cfg_entry->data;
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
+    s->msi->addr_lo = *data;
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
-    if (cfg_entry->data != old_addr) {
+    if (*data != old_addr) {
         if (s->msi->mapped) {
             xen_pt_msi_update(s);
         }
@@ -1230,7 +1242,8 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
 {
     XenPTRegInfo *reg = cfg_entry->reg;
     uint32_t writable_mask = 0;
-    uint32_t old_addr = cfg_entry->data;
+    uint32_t old_addr = *cfg_entry->ptr.dbword;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     /* check whether the type is 64 bit or not */
     if (!(s->msi->flags & PCI_MSI_FLAGS_64BIT)) {
@@ -1241,15 +1254,15 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
     /* update the msi_info too */
-    s->msi->addr_hi = cfg_entry->data;
+    s->msi->addr_hi = *data;
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
-    if (cfg_entry->data != old_addr) {
+    if (*data != old_addr) {
         if (s->msi->mapped) {
             xen_pt_msi_update(s);
         }
@@ -1268,8 +1281,9 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
     XenPTRegInfo *reg = cfg_entry->reg;
     XenPTMSI *msi = s->msi;
     uint16_t writable_mask = 0;
-    uint16_t old_data = cfg_entry->data;
+    uint16_t old_data = *cfg_entry->ptr.dbword;
     uint32_t offset = reg->offset;
+    uint32_t *data = cfg_entry->ptr.dbword;
 
     /* check the offset whether matches the type or not */
     if (!xen_pt_msi_check_type(offset, msi->flags, DATA)) {
@@ -1280,15 +1294,15 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
     /* update the msi_info too */
-    msi->data = cfg_entry->data;
+    msi->data = *data;
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
 
     /* update MSI */
-    if (cfg_entry->data != old_data) {
+    if (*data != old_data) {
         if (msi->mapped) {
             xen_pt_msi_update(s);
         }
@@ -1452,10 +1466,11 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
     uint16_t writable_mask = 0;
     uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
     int debug_msix_enabled_old;
+    uint16_t *data = cfg_entry->ptr.word;
 
     /* modify emulate register */
     writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
-    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
+    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
 
     /* create value for writing to I/O device register */
     *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
@@ -1924,8 +1939,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
         case 4: pci_set_long(s->dev.config + offset, val); break;
         default: assert(1);
         }
-        /* set register value */
-        reg_entry->data = val;
+        /* set register value pointer to the data. */
+        reg_entry->ptr.byte = s->dev.config + offset;
 
     }
     /* list add register entry */
-- 
2.1.0

Re: [PATCH v1 05/10] xen/pt: Remove XenPTReg->data field.

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> We do not want to have two entries to cache the guest configuration
> registers: XenPTReg->data and dev.config. Instead we want to use
> only the dev.config.
> 
> To do without much complications we rip out the ->data field
> and replace it with an pointer to the dev.config. This way we
> have the type-checking (uint8_t, uint16_t, etc) and as well
> and pre-computed location.
> 
> Alternatively we could compute the offset in dev.config by
> using the XenPTRRegInfo and XenPTRegGroup every time but
> this way we have the pre-computed values.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  hw/xen/xen_pt.h             |  6 +++-
>  hw/xen/xen_pt_config_init.c | 73 +++++++++++++++++++++++++++------------------
>  2 files changed, 49 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 09358b1..586d055 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -134,7 +134,11 @@ struct XenPTRegInfo {
>  struct XenPTReg {
>      QLIST_ENTRY(XenPTReg) entries;
>      XenPTRegInfo *reg;
> -    uint32_t data; /* emulated value */
> +    union {
> +        uint8_t *byte;
> +        uint16_t *word;
> +        uint32_t *dbword;
> +    } ptr; /* pointer to dev.config. */

Nice (nasty?) trick. I would probably have just introduced a single
pointer, but this might turn out to be better as it avoids the risk of
merging bits that should be ignored.

However it should be half-word (uint16_t) and word (uint32_t).


>  };
>  
>  typedef const struct XenPTRegGroupInfo XenPTRegGroupInfo;
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index e597993..0a710a2 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -128,10 +128,11 @@ static int xen_pt_byte_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint8_t valid_emu_mask = 0;
> +    uint8_t *data = cfg_entry->ptr.byte;
>  
>      /* emulate byte register */
>      valid_emu_mask = reg->emu_mask & valid_mask;
> -    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
> +    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
>  
>      return 0;
>  }
> @@ -140,10 +141,11 @@ static int xen_pt_word_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint16_t valid_emu_mask = 0;
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* emulate word register */
>      valid_emu_mask = reg->emu_mask & valid_mask;
> -    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
> +    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
>  
>      return 0;
>  }
> @@ -152,10 +154,11 @@ static int xen_pt_long_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint32_t valid_emu_mask = 0;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      /* emulate long register */
>      valid_emu_mask = reg->emu_mask & valid_mask;
> -    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
> +    *value = XEN_PT_MERGE_VALUE(*value, *data, ~valid_emu_mask);
>  
>      return 0;
>  }
> @@ -169,10 +172,11 @@ static int xen_pt_byte_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint8_t writable_mask = 0;
>      uint8_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint8_t *data = cfg_entry->ptr.byte;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -186,10 +190,11 @@ static int xen_pt_word_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint16_t writable_mask = 0;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -203,10 +208,11 @@ static int xen_pt_long_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint32_t writable_mask = 0;
>      uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -255,7 +261,7 @@ static int xen_pt_status_reg_init(XenPCIPassthroughState *s,
>          reg_entry = xen_pt_find_reg(reg_grp_entry, PCI_CAPABILITY_LIST);
>          if (reg_entry) {
>              /* check Capabilities Pointer register */
> -            if (reg_entry->data) {
> +            if (*reg_entry->ptr.word) {
>                  reg_field |= PCI_STATUS_CAP_LIST;
>              } else {
>                  reg_field &= ~PCI_STATUS_CAP_LIST;
> @@ -301,10 +307,11 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint16_t writable_mask = 0;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* modify emulate register */
>      writable_mask = ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      if (*val & PCI_COMMAND_INTX_DISABLE) {
> @@ -447,7 +454,7 @@ static int xen_pt_bar_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  
>      /* emulate BAR */
>      valid_emu_mask = bar_emu_mask & valid_mask;
> -    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
> +    *value = XEN_PT_MERGE_VALUE(*value, *cfg_entry->ptr.dbword, ~valid_emu_mask);
>  
>      return 0;
>  }
> @@ -464,6 +471,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>      uint32_t bar_ro_mask = 0;
>      uint32_t r_size = 0;
>      int index = 0;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      index = xen_pt_bar_offset_to_index(reg->offset);
>      if (index < 0 || index >= PCI_NUM_REGIONS) {
> @@ -500,7 +508,7 @@ static int xen_pt_bar_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
>  
>      /* modify emulate register */
>      writable_mask = bar_emu_mask & ~bar_ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* check whether we need to update the virtual region address or not */
>      switch (s->bases[index].bar_flag) {
> @@ -533,6 +541,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
>      uint32_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
>      pcibus_t r_size = 0;
>      uint32_t bar_ro_mask = 0;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      r_size = d->io_regions[PCI_ROM_SLOT].size;
>      base = &s->bases[PCI_ROM_SLOT];
> @@ -544,7 +553,7 @@ static int xen_pt_exp_rom_bar_reg_write(XenPCIPassthroughState *s,
>  
>      /* modify emulate register */
>      writable_mask = ~bar_ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -983,10 +992,11 @@ static int xen_pt_pmcsr_reg_write(XenPCIPassthroughState *s,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint16_t writable_mask = 0;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value & ~PCI_PM_CTRL_PME_STATUS,
> @@ -1081,6 +1091,7 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
>      XenPTMSI *msi = s->msi;
>      uint16_t writable_mask = 0;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* Currently no support for multi-vector */
>      if (*val & PCI_MSI_FLAGS_QSIZE) {
> @@ -1089,8 +1100,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> -    msi->flags |= cfg_entry->data & ~PCI_MSI_FLAGS_ENABLE;
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
> +    msi->flags |= *data & ~PCI_MSI_FLAGS_ENABLE;
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -1204,18 +1215,19 @@ static int xen_pt_msgaddr32_reg_write(XenPCIPassthroughState *s,
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint32_t writable_mask = 0;
> -    uint32_t old_addr = cfg_entry->data;
> +    uint32_t old_addr = *cfg_entry->ptr.dbword;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> -    s->msi->addr_lo = cfg_entry->data;
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
> +    s->msi->addr_lo = *data;
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
>  
>      /* update MSI */
> -    if (cfg_entry->data != old_addr) {
> +    if (*data != old_addr) {
>          if (s->msi->mapped) {
>              xen_pt_msi_update(s);
>          }
> @@ -1230,7 +1242,8 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
>  {
>      XenPTRegInfo *reg = cfg_entry->reg;
>      uint32_t writable_mask = 0;
> -    uint32_t old_addr = cfg_entry->data;
> +    uint32_t old_addr = *cfg_entry->ptr.dbword;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      /* check whether the type is 64 bit or not */
>      if (!(s->msi->flags & PCI_MSI_FLAGS_64BIT)) {
> @@ -1241,15 +1254,15 @@ static int xen_pt_msgaddr64_reg_write(XenPCIPassthroughState *s,
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>      /* update the msi_info too */
> -    s->msi->addr_hi = cfg_entry->data;
> +    s->msi->addr_hi = *data;
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
>  
>      /* update MSI */
> -    if (cfg_entry->data != old_addr) {
> +    if (*data != old_addr) {
>          if (s->msi->mapped) {
>              xen_pt_msi_update(s);
>          }
> @@ -1268,8 +1281,9 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
>      XenPTRegInfo *reg = cfg_entry->reg;
>      XenPTMSI *msi = s->msi;
>      uint16_t writable_mask = 0;
> -    uint16_t old_data = cfg_entry->data;
> +    uint16_t old_data = *cfg_entry->ptr.dbword;
>      uint32_t offset = reg->offset;
> +    uint32_t *data = cfg_entry->ptr.dbword;
>  
>      /* check the offset whether matches the type or not */
>      if (!xen_pt_msi_check_type(offset, msi->flags, DATA)) {
> @@ -1280,15 +1294,15 @@ static int xen_pt_msgdata_reg_write(XenPCIPassthroughState *s,
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>      /* update the msi_info too */
> -    msi->data = cfg_entry->data;
> +    msi->data = *data;
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, 0);
>  
>      /* update MSI */
> -    if (cfg_entry->data != old_data) {
> +    if (*data != old_data) {
>          if (msi->mapped) {
>              xen_pt_msi_update(s);
>          }
> @@ -1452,10 +1466,11 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
>      uint16_t writable_mask = 0;
>      uint16_t throughable_mask = get_throughable_mask(s, reg, valid_mask);
>      int debug_msix_enabled_old;
> +    uint16_t *data = cfg_entry->ptr.word;
>  
>      /* modify emulate register */
>      writable_mask = reg->emu_mask & ~reg->ro_mask & valid_mask;
> -    cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> +    *data = XEN_PT_MERGE_VALUE(*val, *data, writable_mask);
>  
>      /* create value for writing to I/O device register */
>      *val = XEN_PT_MERGE_VALUE(*val, dev_value, throughable_mask);
> @@ -1924,8 +1939,8 @@ static int xen_pt_config_reg_init(XenPCIPassthroughState *s,
>          case 4: pci_set_long(s->dev.config + offset, val); break;
>          default: assert(1);
>          }
> -        /* set register value */
> -        reg_entry->data = val;
> +        /* set register value pointer to the data. */
> +        reg_entry->ptr.byte = s->dev.config + offset;
>  
>      }
>      /* list add register entry */
> -- 
> 2.1.0
> 

Re: [PATCH v1 05/10] xen/pt: Remove XenPTReg->data field.

[Konrad Rzeszutek Wilk]

On Fri, Jul 17, 2015 at 05:30:39PM +0100, Stefano Stabellini wrote:
> On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> > We do not want to have two entries to cache the guest configuration
> > registers: XenPTReg->data and dev.config. Instead we want to use
> > only the dev.config.
> > 
> > To do without much complications we rip out the ->data field
> > and replace it with an pointer to the dev.config. This way we
> > have the type-checking (uint8_t, uint16_t, etc) and as well
> > and pre-computed location.
> > 
> > Alternatively we could compute the offset in dev.config by
> > using the XenPTRRegInfo and XenPTRegGroup every time but
> > this way we have the pre-computed values.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  hw/xen/xen_pt.h             |  6 +++-
> >  hw/xen/xen_pt_config_init.c | 73 +++++++++++++++++++++++++++------------------
> >  2 files changed, 49 insertions(+), 30 deletions(-)
> > 
> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > index 09358b1..586d055 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -134,7 +134,11 @@ struct XenPTRegInfo {
> >  struct XenPTReg {
> >      QLIST_ENTRY(XenPTReg) entries;
> >      XenPTRegInfo *reg;
> > -    uint32_t data; /* emulated value */
> > +    union {
> > +        uint8_t *byte;
> > +        uint16_t *word;
> > +        uint32_t *dbword;
> > +    } ptr; /* pointer to dev.config. */
> 
> Nice (nasty?) trick. I would probably have just introduced a single
> pointer, but this might turn out to be better as it avoids the risk of
> merging bits that should be ignored.

Right, and also adds nice type-casting checks.
> 
> However it should be half-word (uint16_t) and word (uint32_t).

OK.

[PATCH v1 06/10] xen/pt: Log xen_host_pci_get in two init functions

[Konrad Rzeszutek Wilk]

To help with troubleshooting in the field.

Acked-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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 0a710a2..a2af415 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1791,6 +1791,8 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
             rc = xen_host_pci_get_byte(&s->real_device,
                                        reg_field + PCI_CAP_LIST_ID, &cap_id);
             if (rc) {
+                XEN_PT_ERR(&s->dev, "Failed to read capability @0x%x (rc:%d)\n",
+                           reg_field + PCI_CAP_LIST_ID, rc);
                 return rc;
             }
             if (xen_pt_emu_reg_grps[i].grp_id == cap_id) {
@@ -1984,6 +1986,9 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
                                                   reg_grp_offset,
                                                   &reg_grp_entry->size);
             if (rc < 0) {
+                XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld, type=0x%x, rc:%d\n",
+                           i, ARRAY_SIZE(xen_pt_emu_reg_grps),
+                           xen_pt_emu_reg_grps[i].grp_type, rc);
                 xen_pt_config_delete(s);
                 return rc;
             }
@@ -1998,6 +2003,10 @@ int xen_pt_config_init(XenPCIPassthroughState *s)
                     /* initialize capability register */
                     rc = xen_pt_config_reg_init(s, reg_grp_entry, regs);
                     if (rc < 0) {
+                        XEN_PT_LOG(&s->dev, "Failed to initialize %d/%ld reg 0x%x in grp_type=0x%x (%d/%ld), rc=%d\n",
+                                   j, ARRAY_SIZE(xen_pt_emu_reg_grps[i].emu_regs),
+                                   regs->offset, xen_pt_emu_reg_grps[i].grp_type,
+                                   i, ARRAY_SIZE(xen_pt_emu_reg_grps), rc);
                         xen_pt_config_delete(s);
                         return rc;
                     }
-- 
2.1.0

[PATCH v1 07/10] xen/pt: Log xen_host_pci_get/set errors in MSI code.

[Konrad Rzeszutek Wilk]

We seem to only use these functions when de-activating the
MSI - so just log errors.

Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt_msi.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 5822df5..e3d7194 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -75,19 +75,29 @@ static int msi_msix_enable(XenPCIPassthroughState *s,
                            bool enable)
 {
     uint16_t val = 0;
+    int rc;
 
     if (!address) {
         return -1;
     }
 
-    xen_host_pci_get_word(&s->real_device, address, &val);
+    rc = xen_host_pci_get_word(&s->real_device, address, &val);
+    if (rc) {
+        XEN_PT_ERR(&s->dev, "Failed to read MSI/MSI-X register (0x%x), rc:%d\n",
+                   address, rc);
+        return rc;
+    }
     if (enable) {
         val |= flag;
     } else {
         val &= ~flag;
     }
-    xen_host_pci_set_word(&s->real_device, address, val);
-    return 0;
+    rc = xen_host_pci_set_word(&s->real_device, address, val);
+    if (rc) {
+        XEN_PT_ERR(&s->dev, "Failed to write MSI/MSI-X register (0x%x), rc:%d\n",
+                   address, rc);
+    }
+    return rc;
 }
 
 static int msi_msix_setup(XenPCIPassthroughState *s,
@@ -276,7 +286,7 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s)
         return;
     }
 
-    xen_pt_msi_set_enable(s, false);
+    (void)xen_pt_msi_set_enable(s, false);
 
     msi_msix_disable(s, msi_addr64(msi), msi->data, msi->pirq, false,
                      msi->initialized);
-- 
2.1.0

[PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent

[Konrad Rzeszutek Wilk]

To deal with xen_host_pci_[set|get]_ functions returning error values
and clearing ourselves in the init function we should make the
.exit (xen_pt_unregister_device) function be idempotent in case
the generic code starts calling .exit (or for fun does it before
calling .init!).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen-host-pci-device.c |  5 +++++
 hw/xen/xen-host-pci-device.h |  1 +
 hw/xen/xen_pt.c              | 22 ++++++++++++++++------
 hw/xen/xen_pt.h              |  2 ++
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
index 743b37b..5b20570 100644
--- a/hw/xen/xen-host-pci-device.c
+++ b/hw/xen/xen-host-pci-device.c
@@ -387,6 +387,11 @@ error:
     return rc;
 }
 
+bool xen_host_pci_device_closed(XenHostPCIDevice *d)
+{
+    return d->config_fd == -1;
+}
+
 void xen_host_pci_device_put(XenHostPCIDevice *d)
 {
     if (d->config_fd >= 0) {
diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
index c2486f0..16f4805 100644
--- a/hw/xen/xen-host-pci-device.h
+++ b/hw/xen/xen-host-pci-device.h
@@ -38,6 +38,7 @@ typedef struct XenHostPCIDevice {
 int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
                             uint8_t bus, uint8_t dev, uint8_t func);
 void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
+bool xen_host_pci_device_closed(XenHostPCIDevice *d);
 
 int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p);
 int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p);
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 7575311..a094f7c 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -807,6 +807,7 @@ out:
 
     memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
     memory_listener_register(&s->io_listener, &address_space_io);
+    s->listener_set = true;
     XEN_PT_LOG(d,
                "Real physical device %02x:%02x.%d registered successfully!\n",
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
@@ -818,10 +819,13 @@ static void xen_pt_unregister_device(PCIDevice *d)
 {
     XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
     uint8_t machine_irq = s->machine_irq;
-    uint8_t intx = xen_pt_pci_intx(s);
+    uint8_t intx;
     int rc;
 
-    if (machine_irq) {
+     /* Note that if xen_host_pci_device_put had closed config_fd, then
+      * intx value becomes 0xff. */
+    intx = xen_pt_pci_intx(s);
+    if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) {
         rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq,
                                      PT_IRQ_TYPE_PCI,
                                      pci_bus_num(d->bus),
@@ -836,6 +840,7 @@ static void xen_pt_unregister_device(PCIDevice *d)
         }
     }
 
+    /* N.B. xen_pt_config_delete takes care of freeing them. */
     if (s->msi) {
         xen_pt_msi_disable(s);
     }
@@ -855,15 +860,20 @@ static void xen_pt_unregister_device(PCIDevice *d)
                            machine_irq, errno);
             }
         }
+        s->machine_irq = 0;
     }
 
     /* delete all emulated config registers */
     xen_pt_config_delete(s);
 
-    memory_listener_unregister(&s->memory_listener);
-    memory_listener_unregister(&s->io_listener);
-
-    xen_host_pci_device_put(&s->real_device);
+    if (s->listener_set) {
+        memory_listener_unregister(&s->memory_listener);
+        memory_listener_unregister(&s->io_listener);
+        s->listener_set = false;
+    }
+    if (!xen_host_pci_device_closed(&s->real_device)) {
+        xen_host_pci_device_put(&s->real_device);
+    }
 }
 
 static Property xen_pci_passthrough_properties[] = {
diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
index 586d055..2b58521 100644
--- a/hw/xen/xen_pt.h
+++ b/hw/xen/xen_pt.h
@@ -221,6 +221,7 @@ struct XenPCIPassthroughState {
 
     MemoryListener memory_listener;
     MemoryListener io_listener;
+    bool listener_set;
 };
 
 int xen_pt_config_init(XenPCIPassthroughState *s);
@@ -286,6 +287,7 @@ static inline uint8_t xen_pt_pci_intx(XenPCIPassthroughState *s)
                    " value=%i, acceptable range is 1 - 4\n", r_val);
         r_val = 0;
     } else {
+        /* Note that if s.real_device.config_fd is closed we make 0xff. */
         r_val -= 1;
     }
 
-- 
2.1.0

[PATCH v1 09/10] xen/pt: Move bulk of xen_pt_unregister_device in its own routine.

[Konrad Rzeszutek Wilk]

This way we can call it if we fail during init.

This code movement introduces no changes.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c | 119 +++++++++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 57 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index a094f7c..2dc4277 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -683,6 +683,67 @@ static const MemoryListener xen_pt_io_listener = {
     .priority = 10,
 };
 
+/* destroy. */
+static void xen_pt_destroy(PCIDevice *d) {
+
+    XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
+    uint8_t machine_irq = s->machine_irq;
+    uint8_t intx;
+    int rc;
+
+     /* Note that if xen_host_pci_device_put had closed config_fd, then
+      * intx value becomes 0xff. */
+    intx = xen_pt_pci_intx(s);
+    if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) {
+        rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq,
+                                     PT_IRQ_TYPE_PCI,
+                                     pci_bus_num(d->bus),
+                                     PCI_SLOT(s->dev.devfn),
+                                     intx,
+                                     0 /* isa_irq */);
+        if (rc < 0) {
+            XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
+                       " (machine irq: %i, err: %d)"
+                       " But bravely continuing on..\n",
+                       'a' + intx, machine_irq, errno);
+        }
+    }
+
+    /* N.B. xen_pt_config_delete takes care of freeing them. */
+    if (s->msi) {
+        xen_pt_msi_disable(s);
+    }
+    if (s->msix) {
+        xen_pt_msix_disable(s);
+    }
+
+    if (machine_irq) {
+        xen_pt_mapped_machine_irq[machine_irq]--;
+
+        if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
+            rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
+
+            if (rc < 0) {
+                XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
+                           " But bravely continuing on..\n",
+                           machine_irq, errno);
+            }
+        }
+        s->machine_irq = 0;
+    }
+
+    /* delete all emulated config registers */
+    xen_pt_config_delete(s);
+
+    if (s->listener_set) {
+        memory_listener_unregister(&s->memory_listener);
+        memory_listener_unregister(&s->io_listener);
+        s->listener_set = false;
+    }
+    if (!xen_host_pci_device_closed(&s->real_device)) {
+        xen_host_pci_device_put(&s->real_device);
+    }
+}
 /* init */
 
 static int xen_pt_initfn(PCIDevice *d)
@@ -817,63 +878,7 @@ out:
 
 static void xen_pt_unregister_device(PCIDevice *d)
 {
-    XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
-    uint8_t machine_irq = s->machine_irq;
-    uint8_t intx;
-    int rc;
-
-     /* Note that if xen_host_pci_device_put had closed config_fd, then
-      * intx value becomes 0xff. */
-    intx = xen_pt_pci_intx(s);
-    if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) {
-        rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq,
-                                     PT_IRQ_TYPE_PCI,
-                                     pci_bus_num(d->bus),
-                                     PCI_SLOT(s->dev.devfn),
-                                     intx,
-                                     0 /* isa_irq */);
-        if (rc < 0) {
-            XEN_PT_ERR(d, "unbinding of interrupt INT%c failed."
-                       " (machine irq: %i, err: %d)"
-                       " But bravely continuing on..\n",
-                       'a' + intx, machine_irq, errno);
-        }
-    }
-
-    /* N.B. xen_pt_config_delete takes care of freeing them. */
-    if (s->msi) {
-        xen_pt_msi_disable(s);
-    }
-    if (s->msix) {
-        xen_pt_msix_disable(s);
-    }
-
-    if (machine_irq) {
-        xen_pt_mapped_machine_irq[machine_irq]--;
-
-        if (xen_pt_mapped_machine_irq[machine_irq] == 0) {
-            rc = xc_physdev_unmap_pirq(xen_xc, xen_domid, machine_irq);
-
-            if (rc < 0) {
-                XEN_PT_ERR(d, "unmapping of interrupt %i failed. (err: %d)"
-                           " But bravely continuing on..\n",
-                           machine_irq, errno);
-            }
-        }
-        s->machine_irq = 0;
-    }
-
-    /* delete all emulated config registers */
-    xen_pt_config_delete(s);
-
-    if (s->listener_set) {
-        memory_listener_unregister(&s->memory_listener);
-        memory_listener_unregister(&s->io_listener);
-        s->listener_set = false;
-    }
-    if (!xen_host_pci_device_closed(&s->real_device)) {
-        xen_host_pci_device_put(&s->real_device);
-    }
+    xen_pt_destroy(d);
 }
 
 static Property xen_pci_passthrough_properties[] = {
-- 
2.1.0

[PATCH v1 10/10] xen/pt: Check for return values for xen_host_pci_[get|set] in init

[Konrad Rzeszutek Wilk]

and if we have failures we call xen_pt_destroy introduced in
'xen/pt: Move bulk of xen_pt_unregister_device in its own routine.'
and free all of the allocated structures.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 2dc4277..62f5751 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -776,10 +776,11 @@ 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) < 0) {
-        xen_host_pci_device_put(&s->real_device);
-        return -1;
+    rc = xen_host_pci_get_block(&s->real_device, 0, d->config,
+                                PCI_CONFIG_SPACE_SIZE);
+    if (rc < 0) {
+        XEN_PT_ERR(d,"Could not read PCI_CONFIG space! (rc:%d)\n", rc);
+        goto err_out;
     }
 
     s->memory_listener = xen_pt_memory_listener;
@@ -789,17 +790,17 @@ static int xen_pt_initfn(PCIDevice *d)
     xen_pt_register_regions(s, &cmd);
 
     /* reinitialize each config register to be emulated */
-    if (xen_pt_config_init(s)) {
+    rc = xen_pt_config_init(s);
+    if (rc) {
         XEN_PT_ERR(d, "PCI Config space initialisation failed.\n");
-        xen_host_pci_device_put(&s->real_device);
-        return -1;
+        goto err_out;
     }
 
     /* Bind interrupt */
     rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
     if (rc) {
         XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
-        scratch = 0;
+        goto err_out;
     }
     if (!scratch) {
         XEN_PT_LOG(d, "no pin interrupt\n");
@@ -856,12 +857,14 @@ out:
         rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
         if (rc) {
             XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
+            goto err_out;
         } else {
             val |= cmd;
             rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
             if (rc) {
                 XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
                            val, rc);
+                goto err_out;
             }
         }
     }
@@ -874,6 +877,11 @@ out:
                s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
 
     return 0;
+
+err_out:
+    xen_pt_destroy(d);
+    assert(rc);
+    return rc;
 }
 
 static void xen_pt_unregister_device(PCIDevice *d)
-- 
2.1.0

[PATCH] Follow-on to Remove XenPTReg->data and use dev.config for guest configuration values.

[Konrad Rzeszutek Wilk]

Hey,

This is a follow up patch - as we have now removed the XenPTReg->data and
are using dev.config for registers, and each ->init routine sets the
proper registers (and reads from the host SysFS) - there is no need
to slurp up at the start all of the configuration registers - as we
end up overwritting with emulated registers.


 hw/xen/xen_pt.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)


Konrad Rzeszutek Wilk (1):
      xen/pt: Don't slurp wholesale the PCI configuration registers

[PATCH] xen/pt: Don't slurp wholesale the PCI configuration registers

[Konrad Rzeszutek Wilk]

Instead we have the emulation registers ->init functions which
consult the host values to see what the initial value should be
and they are responsible for populating the dev.config.

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

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index 05828e0..cd69cb4 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -780,12 +780,7 @@ static int xen_pt_initfn(PCIDevice *d)
     }
 
     /* Initialize virtualized PCI configuration (Extended 256 Bytes) */
-    rc = xen_host_pci_get_block(&s->real_device, 0, d->config,
-                                PCI_CONFIG_SPACE_SIZE);
-    if (rc < 0) {
-        XEN_PT_ERR(d,"Could not read PCI_CONFIG space! (rc:%d)\n", rc);
-        goto err_out;
-    }
+    memset(d->config, 0, PCI_CONFIG_SPACE_SIZE);
 
     s->memory_listener = xen_pt_memory_listener;
     s->io_listener = xen_pt_io_listener;
-- 
1.8.4.2

Re: [PATCH] xen/pt: Don't slurp wholesale the PCI configuration registers

[Stefano Stabellini]

On Wed, 8 Jul 2015, Konrad Rzeszutek Wilk wrote:
> Instead we have the emulation registers ->init functions which
> consult the host values to see what the initial value should be
> and they are responsible for populating the dev.config.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

>  hw/xen/xen_pt.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 05828e0..cd69cb4 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -780,12 +780,7 @@ static int xen_pt_initfn(PCIDevice *d)
>      }
>  
>      /* Initialize virtualized PCI configuration (Extended 256 Bytes) */
> -    rc = xen_host_pci_get_block(&s->real_device, 0, d->config,
> -                                PCI_CONFIG_SPACE_SIZE);
> -    if (rc < 0) {
> -        XEN_PT_ERR(d,"Could not read PCI_CONFIG space! (rc:%d)\n", rc);
> -        goto err_out;
> -    }
> +    memset(d->config, 0, PCI_CONFIG_SPACE_SIZE);
>  
>      s->memory_listener = xen_pt_memory_listener;
>      s->io_listener = xen_pt_io_listener;

Re: [PATCH v1 01/10] xen/pt: Use xen_host_pci_get_[byte|word] instead of dev.config

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> During init time we treat the dev.config area as a cache
> of the host view. However during execution time we treat it
> as guest view (by the generic PCI API). We need to sync Xen's
> code to the generic PCI API view. This is the first step
> by replacing all of the code that uses dev.config or
> pci_get_[byte|word] to get host value to actually use the
> xen_host_pci_get_[byte|word] functions.
> 
> Interestingly in 'xen_pt_ptr_reg_init' we also needed to swap
> reg_field from uint32_t to uint8_t - since the access is only
> for one byte not four bytes. We can split this as a seperate
> patch however we would have to use a cast to thwart compiler
> warnings in the meantime.
> 
> We also truncated 'flags' to 'flag' to make the code fit within
> the 80 characters.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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


>  hw/xen/xen_pt.c             | 24 +++++++++++---
>  hw/xen/xen_pt_config_init.c | 77 +++++++++++++++++++++++++++++++--------------
>  2 files changed, 73 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index ea1ceda..7575311 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -689,7 +689,7 @@ static int xen_pt_initfn(PCIDevice *d)
>  {
>      XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
>      int rc = 0;
> -    uint8_t machine_irq = 0;
> +    uint8_t machine_irq = 0, scratch;
>      uint16_t cmd = 0;
>      int pirq = XEN_PT_UNASSIGNED_PIRQ;
>  
> @@ -735,7 +735,12 @@ static int xen_pt_initfn(PCIDevice *d)
>      }
>  
>      /* Bind interrupt */
> -    if (!s->dev.config[PCI_INTERRUPT_PIN]) {
> +    rc = xen_host_pci_get_byte(&s->real_device, PCI_INTERRUPT_PIN, &scratch);
> +    if (rc) {
> +        XEN_PT_ERR(d, "Failed to read PCI_INTERRUPT_PIN! (rc:%d)\n", rc);
> +        scratch = 0;
> +    }
> +    if (!scratch) {
>          XEN_PT_LOG(d, "no pin interrupt\n");
>          goto out;
>      }
> @@ -785,8 +790,19 @@ static int xen_pt_initfn(PCIDevice *d)
>  
>  out:
>      if (cmd) {
> -        xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
> -                              pci_get_word(d->config + PCI_COMMAND) | cmd);
> +        uint16_t val;
> +
> +        rc = xen_host_pci_get_word(&s->real_device, PCI_COMMAND, &val);
> +        if (rc) {
> +            XEN_PT_ERR(d, "Failed to read PCI_COMMAND! (rc: %d)\n", rc);
> +        } else {
> +            val |= cmd;
> +            rc = xen_host_pci_set_word(&s->real_device, PCI_COMMAND, val);
> +            if (rc) {
> +                XEN_PT_ERR(d, "Failed to write PCI_COMMAND val=0x%x!(rc: %d)\n",
> +                           val, rc);
> +            }
> +        }
>      }
>  
>      memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 21d4938..e34f9f8 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -800,15 +800,21 @@ static XenPTRegInfo xen_pt_emu_reg_vendor[] = {
>  static inline uint8_t get_capability_version(XenPCIPassthroughState *s,
>                                               uint32_t offset)
>  {
> -    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
> -    return flags & PCI_EXP_FLAGS_VERS;
> +    uint8_t flag;
> +    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
> +        return 0;
> +    }
> +    return flag & PCI_EXP_FLAGS_VERS;
>  }
>  
>  static inline uint8_t get_device_type(XenPCIPassthroughState *s,
>                                        uint32_t offset)
>  {
> -    uint8_t flags = pci_get_byte(s->dev.config + offset + PCI_EXP_FLAGS);
> -    return (flags & PCI_EXP_FLAGS_TYPE) >> 4;
> +    uint8_t flag;
> +    if (xen_host_pci_get_byte(&s->real_device, offset + PCI_EXP_FLAGS, &flag)) {
> +        return 0;
> +    }
> +    return (flag & PCI_EXP_FLAGS_TYPE) >> 4;
>  }
>  
>  /* initialize Link Control register */
> @@ -857,8 +863,14 @@ static int xen_pt_linkctrl2_reg_init(XenPCIPassthroughState *s,
>          reg_field = XEN_PT_INVALID_REG;
>      } else {
>          /* set Supported Link Speed */
> -        uint8_t lnkcap = pci_get_byte(s->dev.config + real_offset - reg->offset
> -                                      + PCI_EXP_LNKCAP);
> +        uint8_t lnkcap;
> +        int rc;
> +        rc = xen_host_pci_get_byte(&s->real_device,
> +                                   real_offset - reg->offset + PCI_EXP_LNKCAP,
> +                                   &lnkcap);
> +        if (rc) {
> +            return rc;
> +        }
>          reg_field |= PCI_EXP_LNKCAP_SLS & lnkcap;
>      }
>  
> @@ -1039,13 +1051,15 @@ static int xen_pt_msgctrl_reg_init(XenPCIPassthroughState *s,
>                                     XenPTRegInfo *reg, uint32_t real_offset,
>                                     uint32_t *data)
>  {
> -    PCIDevice *d = &s->dev;
>      XenPTMSI *msi = s->msi;
> -    uint16_t reg_field = 0;
> +    uint16_t reg_field;
> +    int rc;
>  
>      /* use I/O device register's value as initial value */
> -    reg_field = pci_get_word(d->config + real_offset);
> -
> +    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      if (reg_field & PCI_MSI_FLAGS_ENABLE) {
>          XEN_PT_LOG(&s->dev, "MSI already enabled, disabling it first\n");
>          xen_host_pci_set_word(&s->real_device, real_offset,
> @@ -1411,12 +1425,14 @@ static int xen_pt_msixctrl_reg_init(XenPCIPassthroughState *s,
>                                      XenPTRegInfo *reg, uint32_t real_offset,
>                                      uint32_t *data)
>  {
> -    PCIDevice *d = &s->dev;
> -    uint16_t reg_field = 0;
> +    uint16_t reg_field;
> +    int rc;
>  
>      /* use I/O device register's value as initial value */
> -    reg_field = pci_get_word(d->config + real_offset);
> -
> +    rc = xen_host_pci_get_word(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      if (reg_field & PCI_MSIX_FLAGS_ENABLE) {
>          XEN_PT_LOG(&s->dev, "MSIX already enabled, disabling it first\n");
>          xen_host_pci_set_word(&s->real_device, real_offset,
> @@ -1511,8 +1527,7 @@ static int xen_pt_vendor_size_init(XenPCIPassthroughState *s,
>                                     const XenPTRegGroupInfo *grp_reg,
>                                     uint32_t base_offset, uint8_t *size)
>  {
> -    *size = pci_get_byte(s->dev.config + base_offset + 0x02);
> -    return 0;
> +    return xen_host_pci_get_byte(&s->real_device, base_offset + 0x02, size);
>  }
>  /* get PCI Express Capability Structure register group size */
>  static int xen_pt_pcie_size_init(XenPCIPassthroughState *s,
> @@ -1591,12 +1606,15 @@ static int xen_pt_msi_size_init(XenPCIPassthroughState *s,
>                                  const XenPTRegGroupInfo *grp_reg,
>                                  uint32_t base_offset, uint8_t *size)
>  {
> -    PCIDevice *d = &s->dev;
>      uint16_t msg_ctrl = 0;
>      uint8_t msi_size = 0xa;
> +    int rc;
>  
> -    msg_ctrl = pci_get_word(d->config + (base_offset + PCI_MSI_FLAGS));
> -
> +    rc = xen_host_pci_get_word(&s->real_device, base_offset + PCI_MSI_FLAGS,
> +                               &msg_ctrl);
> +    if (rc) {
> +        return rc;
> +    }
>      /* check if 64-bit address is capable of per-vector masking */
>      if (msg_ctrl & PCI_MSI_FLAGS_64BIT) {
>          msi_size += 4;
> @@ -1739,11 +1757,14 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>                                 XenPTRegInfo *reg, uint32_t real_offset,
>                                 uint32_t *data)
>  {
> -    int i;
> -    uint8_t *config = s->dev.config;
> -    uint32_t reg_field = pci_get_byte(config + real_offset);
> +    int i, rc;
> +    uint8_t reg_field;
>      uint8_t cap_id = 0;
>  
> +    rc = xen_host_pci_get_byte(&s->real_device, real_offset, &reg_field);
> +    if (rc) {
> +        return rc;
> +    }
>      /* find capability offset */
>      while (reg_field) {
>          for (i = 0; xen_pt_emu_reg_grps[i].grp_size != 0; i++) {
> @@ -1752,7 +1773,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>                  continue;
>              }
>  
> -            cap_id = pci_get_byte(config + reg_field + PCI_CAP_LIST_ID);
> +            rc = xen_host_pci_get_byte(&s->real_device,
> +                                       reg_field + PCI_CAP_LIST_ID, &cap_id);
> +            if (rc) {
> +                return rc;
> +            }
>              if (xen_pt_emu_reg_grps[i].grp_id == cap_id) {
>                  if (xen_pt_emu_reg_grps[i].grp_type == XEN_PT_GRP_TYPE_EMU) {
>                      goto out;
> @@ -1763,7 +1788,11 @@ static int xen_pt_ptr_reg_init(XenPCIPassthroughState *s,
>          }
>  
>          /* next capability */
> -        reg_field = pci_get_byte(config + reg_field + PCI_CAP_LIST_NEXT);
> +        rc = xen_host_pci_get_byte(&s->real_device,
> +                                   reg_field + PCI_CAP_LIST_NEXT, &reg_field);
> +        if (rc) {
> +            return rc;
> +        }
>      }
>  
>  out:
> -- 
> 2.1.0
> 

Re: [PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent

[Stefano Stabellini]

On Thu, 2 Jul 2015, Konrad Rzeszutek Wilk wrote:
> To deal with xen_host_pci_[set|get]_ functions returning error values
> and clearing ourselves in the init function we should make the
> .exit (xen_pt_unregister_device) function be idempotent in case
> the generic code starts calling .exit (or for fun does it before
> calling .init!).
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  hw/xen/xen-host-pci-device.c |  5 +++++
>  hw/xen/xen-host-pci-device.h |  1 +
>  hw/xen/xen_pt.c              | 22 ++++++++++++++++------
>  hw/xen/xen_pt.h              |  2 ++
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
> index 743b37b..5b20570 100644
> --- a/hw/xen/xen-host-pci-device.c
> +++ b/hw/xen/xen-host-pci-device.c
> @@ -387,6 +387,11 @@ error:
>      return rc;
>  }
>  
> +bool xen_host_pci_device_closed(XenHostPCIDevice *d)
> +{
> +    return d->config_fd == -1;
> +}
> +
>  void xen_host_pci_device_put(XenHostPCIDevice *d)
>  {
>      if (d->config_fd >= 0) {
> diff --git a/hw/xen/xen-host-pci-device.h b/hw/xen/xen-host-pci-device.h
> index c2486f0..16f4805 100644
> --- a/hw/xen/xen-host-pci-device.h
> +++ b/hw/xen/xen-host-pci-device.h
> @@ -38,6 +38,7 @@ typedef struct XenHostPCIDevice {
>  int xen_host_pci_device_get(XenHostPCIDevice *d, uint16_t domain,
>                              uint8_t bus, uint8_t dev, uint8_t func);
>  void xen_host_pci_device_put(XenHostPCIDevice *pci_dev);
> +bool xen_host_pci_device_closed(XenHostPCIDevice *d);
>  
>  int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p);
>  int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p);
> diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> index 7575311..a094f7c 100644
> --- a/hw/xen/xen_pt.c
> +++ b/hw/xen/xen_pt.c
> @@ -807,6 +807,7 @@ out:
>  
>      memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
>      memory_listener_register(&s->io_listener, &address_space_io);
> +    s->listener_set = true;
>      XEN_PT_LOG(d,
>                 "Real physical device %02x:%02x.%d registered successfully!\n",
>                 s->hostaddr.bus, s->hostaddr.slot, s->hostaddr.function);
> @@ -818,10 +819,13 @@ static void xen_pt_unregister_device(PCIDevice *d)
>  {
>      XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
>      uint8_t machine_irq = s->machine_irq;
> -    uint8_t intx = xen_pt_pci_intx(s);
> +    uint8_t intx;
>      int rc;
>  
> -    if (machine_irq) {
> +     /* Note that if xen_host_pci_device_put had closed config_fd, then
> +      * intx value becomes 0xff. */
> +    intx = xen_pt_pci_intx(s);

Wouldn't it make sense to move this assignment inside the if below?

Aside from this small item:

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


> +    if (machine_irq && !xen_host_pci_device_closed(&s->real_device)) {
>
>          rc = xc_domain_unbind_pt_irq(xen_xc, xen_domid, machine_irq,
>                                       PT_IRQ_TYPE_PCI,
>                                       pci_bus_num(d->bus),
> @@ -836,6 +840,7 @@ static void xen_pt_unregister_device(PCIDevice *d)
>          }
>      }
>  
> +    /* N.B. xen_pt_config_delete takes care of freeing them. */
>      if (s->msi) {
>          xen_pt_msi_disable(s);
>      }
> @@ -855,15 +860,20 @@ static void xen_pt_unregister_device(PCIDevice *d)
>                             machine_irq, errno);
>              }
>          }
> +        s->machine_irq = 0;
>      }
>  
>      /* delete all emulated config registers */
>      xen_pt_config_delete(s);
>  
> -    memory_listener_unregister(&s->memory_listener);
> -    memory_listener_unregister(&s->io_listener);
> -
> -    xen_host_pci_device_put(&s->real_device);
> +    if (s->listener_set) {
> +        memory_listener_unregister(&s->memory_listener);
> +        memory_listener_unregister(&s->io_listener);
> +        s->listener_set = false;
> +    }
> +    if (!xen_host_pci_device_closed(&s->real_device)) {
> +        xen_host_pci_device_put(&s->real_device);
> +    }
>  }
>  
>  static Property xen_pci_passthrough_properties[] = {
> diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> index 586d055..2b58521 100644
> --- a/hw/xen/xen_pt.h
> +++ b/hw/xen/xen_pt.h
> @@ -221,6 +221,7 @@ struct XenPCIPassthroughState {
>  
>      MemoryListener memory_listener;
>      MemoryListener io_listener;
> +    bool listener_set;
>  };
>  
>  int xen_pt_config_init(XenPCIPassthroughState *s);
> @@ -286,6 +287,7 @@ static inline uint8_t xen_pt_pci_intx(XenPCIPassthroughState *s)
>                     " value=%i, acceptable range is 1 - 4\n", r_val);
>          r_val = 0;
>      } else {
> +        /* Note that if s.real_device.config_fd is closed we make 0xff. */
>          r_val -= 1;
>      }
>  
> -- 
> 2.1.0
> 

Re: [PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent

[Konrad Rzeszutek Wilk]

> > @@ -818,10 +819,13 @@ static void xen_pt_unregister_device(PCIDevice *d)
> >  {
> >      XenPCIPassthroughState *s = XEN_PT_DEVICE(d);
> >      uint8_t machine_irq = s->machine_irq;
> > -    uint8_t intx = xen_pt_pci_intx(s);
> > +    uint8_t intx;
> >      int rc;
> >  
> > -    if (machine_irq) {
> > +     /* Note that if xen_host_pci_device_put had closed config_fd, then
> > +      * intx value becomes 0xff. */
> > +    intx = xen_pt_pci_intx(s);
> 
> Wouldn't it make sense to move this assignment inside the if below?

Totally. Plus it also allows me to remove the comment above.
> 
> Aside from this small item:
> 
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Thank you.