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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


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

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