| Message ID | 1441905361-31967-21-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On 10/09/2015 19:15, Stefano Stabellini wrote: > + > + switch (reg->size) { > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); A bit ugly, and it relies on the host being little endian. > + break; > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); Same here. > + break; > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > + break; > + default: assert(1); This should be assert(0) or, better, abort(). Paolo
CC Konrad On Mon, 14 Sep 2015, Paolo Bonzini wrote: > On 10/09/2015 19:15, Stefano Stabellini wrote: > > + > > + switch (reg->size) { > > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); > > A bit ugly, and it relies on the host being little endian. > > > + break; > > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); > > Same here. cpu_to_le32? But in practice, Xen being little endian only, I doubt that xen_pt_config_init.c would actually work on be. > > + break; > > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > > + break; > > + default: assert(1); > > This should be assert(0) or, better, abort().
On Tue, Sep 15, 2015 at 11:07:02AM +0100, Stefano Stabellini wrote: > CC Konrad > > On Mon, 14 Sep 2015, Paolo Bonzini wrote: > > On 10/09/2015 19:15, Stefano Stabellini wrote: > > > + > > > + switch (reg->size) { > > > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); > > > > A bit ugly, and it relies on the host being little endian. > > > > > + break; > > > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); > > > > Same here. > > cpu_to_le32? > > But in practice, Xen being little endian only, I doubt that xen_pt_config_init.c > would actually work on be. > > > > > + break; > > > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > > > + break; > > > + default: assert(1); > > > > This should be assert(0) or, better, abort(). OK. Stefano, do you want me to: 1). Rebase the patches on top of your tag? 2). Send an follow up patch to change this to abort()? (and wherever else I used assert(..)? 3). Wait till Paolo is done going through the patchset and then revisit 1) or 2)?
On Tue, 15 Sep 2015, Konrad Rzeszutek Wilk wrote: > On Tue, Sep 15, 2015 at 11:07:02AM +0100, Stefano Stabellini wrote: > > CC Konrad > > > > On Mon, 14 Sep 2015, Paolo Bonzini wrote: > > > On 10/09/2015 19:15, Stefano Stabellini wrote: > > > > + > > > > + switch (reg->size) { > > > > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); > > > > > > A bit ugly, and it relies on the host being little endian. > > > > > > > + break; > > > > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); > > > > > > Same here. > > > > cpu_to_le32? > > > > But in practice, Xen being little endian only, I doubt that xen_pt_config_init.c > > would actually work on be. > > > > > > > > + break; > > > > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > > > > + break; > > > > + default: assert(1); > > > > > > This should be assert(0) or, better, abort(). > > OK. Stefano, do you want me to: > > 1). Rebase the patches on top of your tag? Patches are already upstream, so no worries about rebasing. > 2). Send an follow up patch to change this to abort()? (and wherever else I used > assert(..)? Yes, that would be good. > 3). Wait till Paolo is done going through the patchset and then revisit 1) or 2)? I don't know if Paolo has any more comments. Regarding his previous comment on little-endian, as I wrote I am not sure if sprinkling around some cpu_to_le32 would actually make hw/xen/xen_pt_config_init.c much better. I'll leave that to you.
On 15/09/2015 15:28, Stefano Stabellini wrote: > >> > 2). Send an follow up patch to change this to abort()? (and wherever else I used >> > assert(..)? > Yes, that would be good. > > >> > 3). Wait till Paolo is done going through the patchset and then revisit 1) or 2)? > I don't know if Paolo has any more comments. No, the few comments I had were only prompted by Coverity. Paolo
On Tue, Sep 15, 2015 at 02:28:51PM +0100, Stefano Stabellini wrote: > On Tue, 15 Sep 2015, Konrad Rzeszutek Wilk wrote: > > On Tue, Sep 15, 2015 at 11:07:02AM +0100, Stefano Stabellini wrote: > > > CC Konrad > > > > > > On Mon, 14 Sep 2015, Paolo Bonzini wrote: > > > > On 10/09/2015 19:15, Stefano Stabellini wrote: > > > > > + > > > > > + switch (reg->size) { > > > > > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); > > > > > > > > A bit ugly, and it relies on the host being little endian. > > > > > > > > > + break; > > > > > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); > > > > > > > > Same here. > > > > > > cpu_to_le32? > > > > > > But in practice, Xen being little endian only, I doubt that xen_pt_config_init.c > > > would actually work on be. > > > > > > > > > > > + break; > > > > > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > > > > > + break; > > > > > + default: assert(1); > > > > > > > > This should be assert(0) or, better, abort(). > > > > OK. Stefano, do you want me to: > > > > 1). Rebase the patches on top of your tag? > > Patches are already upstream, so no worries about rebasing. > > > > 2). Send an follow up patch to change this to abort()? (and wherever else I used > > assert(..)? > > Yes, that would be good. > > > > 3). Wait till Paolo is done going through the patchset and then revisit 1) or 2)? > > I don't know if Paolo has any more comments. > > Regarding his previous comment on little-endian, as I wrote I am not > sure if sprinkling around some cpu_to_le32 would actually make > hw/xen/xen_pt_config_init.c much better. I'll leave that to you. I got some other outstanding things I need to get done so will be as most lazy as possible in regards to this and just send you a patch :-)
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c index a75baea..b1ad743 100644 --- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -1893,6 +1893,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); @@ -1905,8 +1909,61 @@ 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); + + 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); + 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);