[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH RFC 1 2/8] xen/pt: Sync up the dev.config and data values.



On Mon, 29 Jun 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. There is also some reg->size accounting taken
> into consideration - which could be removed.
> 
> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  hw/xen/xen_pt_config_init.c | 45 
> ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index e34f9f8..91c3a14 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,47 @@ 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);
> +
> +        if (xen_host_pci_get_long(&s->real_device, offset, &val))
> +            val = pci_get_long(s->dev.config + offset); /* Pfff... */

I don't understand this, a more helpful comment would be helpful


> +        /* 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;

This can potentially change reg_entry->data too with the guest view of
the register, right?
It is worth mentioning in the commit message, as it is one of the goals
of the series.

>      }
>      /* list add register entry */
>      QLIST_INSERT_HEAD(&reg_grp->reg_tbl_list, reg_entry, entries);
> -- 
> 2.1.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.