[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 02/10] xen/pt: Sync up the dev.config and dat
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [PATCH v1 02/10] xen/pt: Sync up the dev.config and data values. |
Date: |
Fri, 17 Jul 2015 16:54:16 +0100 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
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 <address@hidden>
> Signed-off-by: Konrad Rzeszutek Wilk <address@hidden>
Reviewed-by: Stefano Stabellini <address@hidden>
> 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(®_grp->reg_tbl_list, reg_entry, entries);
> --
> 2.1.0
>
- [Qemu-devel] [PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent, (continued)
- [Qemu-devel] [PATCH v1 08/10] xen/pt: Make xen_pt_unregister_device idempotent, Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH v1 03/10] xen/pt: Check if reg->init function sets the 'data' past the reg->size, Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH v1 06/10] xen/pt: Log xen_host_pci_get in two init functions, Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH v1 05/10] xen/pt: Remove XenPTReg->data field., Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH v1 02/10] xen/pt: Sync up the dev.config and data values., Konrad Rzeszutek Wilk, 2015/07/02
- Re: [Qemu-devel] [PATCH v1 02/10] xen/pt: Sync up the dev.config and data values.,
Stefano Stabellini <=
- [Qemu-devel] [PATCH v1 09/10] xen/pt: Move bulk of xen_pt_unregister_device in its own routine., Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH v1 07/10] xen/pt: Log xen_host_pci_get/set errors in MSI code., Konrad Rzeszutek Wilk, 2015/07/02
- [Qemu-devel] [PATCH] Follow-on to Remove XenPTReg->data and use dev.config for guest configuration values., Konrad Rzeszutek Wilk, 2015/07/08