| Message ID | 20221114192011.1539233-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] hw/xen/xen_pt: Call default handler only if no custom one is set | expand |
On Mon, Nov 14, 2022 at 08:20:10PM +0100, Marek Marczykowski-Górecki wrote: > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > index 0ec7e52183..269bd26109 100644 > --- a/hw/xen/xen_pt.c > +++ b/hw/xen/xen_pt.c > @@ -255,6 +255,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > uint32_t find_addr = addr; > XenPTRegInfo *reg = NULL; > bool wp_flag = false; > + uint32_t emul_mask = 0, write_val; > > if (xen_pt_pci_config_access_check(d, addr, len)) { > return; > @@ -310,7 +311,6 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > } > > memory_region_transaction_begin(); > - pci_default_write_config(d, addr, val, len); > > /* adjust the read and write value to appropriate CFC-CFF window */ > read_val <<= (addr & 3) << 3; > @@ -370,6 +370,8 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > return; > } > > + emul_mask |= ( (1 << (reg->size * 8) ) - 1 ) << ((find_addr & 3) * 8); > + > /* calculate next address to find */ > emul_len -= reg->size; > if (emul_len > 0) { > @@ -396,6 +398,24 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > /* need to shift back before passing them to xen_host_pci_set_block. */ > val >>= (addr & 3) << 3; > > + /* store emulated registers that didn't have specific hooks */ > + write_val = val; > + for (index = 0; emul_mask; index += emul_len) { `index` isn't used, was it meant to be use for something? > + emul_len = 0; > + while (emul_mask & 0xff) { > + emul_len++; This seems to count the number of byte that have a hook (xen_pt_find_reg() found a `reg_entry`). This loop should count instead the number of bytes for which no `reg_entry` have been found, right? Shouldn't the loop count when a byte in emul_mask is unset? > + emul_mask >>= 8; > + } > + if (emul_len) { > + uint32_t mask = ((1 << (emul_len * 8)) - 1); > + pci_default_write_config(d, addr, write_val & mask, emul_len); `addr` isn't updated in the loop, aren't we going to write bytes to the wrong place? If for example "emul_mask == 0x00ff00ff" ? > + write_val >>= emul_len * 8; > + } else { > + emul_mask >>= 8; > + write_val >>= 8; > + } > + } Thanks,
On Tue, Nov 22, 2022 at 05:12:59PM +0000, Anthony PERARD wrote: > On Mon, Nov 14, 2022 at 08:20:10PM +0100, Marek Marczykowski-Górecki wrote: > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c > > index 0ec7e52183..269bd26109 100644 > > --- a/hw/xen/xen_pt.c > > +++ b/hw/xen/xen_pt.c > > @@ -255,6 +255,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > > uint32_t find_addr = addr; > > XenPTRegInfo *reg = NULL; > > bool wp_flag = false; > > + uint32_t emul_mask = 0, write_val; > > > > if (xen_pt_pci_config_access_check(d, addr, len)) { > > return; > > @@ -310,7 +311,6 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > > } > > > > memory_region_transaction_begin(); > > - pci_default_write_config(d, addr, val, len); > > > > /* adjust the read and write value to appropriate CFC-CFF window */ > > read_val <<= (addr & 3) << 3; > > @@ -370,6 +370,8 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > > return; > > } > > > > + emul_mask |= ( (1 << (reg->size * 8) ) - 1 ) << ((find_addr & 3) * 8); > > + > > /* calculate next address to find */ > > emul_len -= reg->size; > > if (emul_len > 0) { > > @@ -396,6 +398,24 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, > > /* need to shift back before passing them to xen_host_pci_set_block. */ > > val >>= (addr & 3) << 3; > > > > + /* store emulated registers that didn't have specific hooks */ > > + write_val = val; > > + for (index = 0; emul_mask; index += emul_len) { > > `index` isn't used, was it meant to be use for something? Yes, it should be used as addr + index below. > > + emul_len = 0; > > + while (emul_mask & 0xff) { > > + emul_len++; > > This seems to count the number of byte that have a hook > (xen_pt_find_reg() found a `reg_entry`). > This loop should count instead the number of bytes for which no > `reg_entry` have been found, right? Shouldn't the loop count when a byte > in emul_mask is unset? No, see the patch description - only declared registers should be saved. The patch title is misleading, I'll clarify it. > > + emul_mask >>= 8; > > + } > > + if (emul_len) { > > + uint32_t mask = ((1 << (emul_len * 8)) - 1); > > + pci_default_write_config(d, addr, write_val & mask, emul_len); > > `addr` isn't updated in the loop, aren't we going to write bytes to the > wrong place? If for example "emul_mask == 0x00ff00ff" ? Indeed, it should be addr + index. > > + write_val >>= emul_len * 8; > > + } else { > > + emul_mask >>= 8; > > + write_val >>= 8; > > + } > > + } > > Thanks, > > -- > Anthony PERARD
diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c index 0ec7e52183..269bd26109 100644 --- a/hw/xen/xen_pt.c +++ b/hw/xen/xen_pt.c @@ -255,6 +255,7 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, uint32_t find_addr = addr; XenPTRegInfo *reg = NULL; bool wp_flag = false; + uint32_t emul_mask = 0, write_val; if (xen_pt_pci_config_access_check(d, addr, len)) { return; @@ -310,7 +311,6 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, } memory_region_transaction_begin(); - pci_default_write_config(d, addr, val, len); /* adjust the read and write value to appropriate CFC-CFF window */ read_val <<= (addr & 3) << 3; @@ -370,6 +370,8 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, return; } + emul_mask |= ( (1 << (reg->size * 8) ) - 1 ) << ((find_addr & 3) * 8); + /* calculate next address to find */ emul_len -= reg->size; if (emul_len > 0) { @@ -396,6 +398,24 @@ static void xen_pt_pci_write_config(PCIDevice *d, uint32_t addr, /* need to shift back before passing them to xen_host_pci_set_block. */ val >>= (addr & 3) << 3; + /* store emulated registers that didn't have specific hooks */ + write_val = val; + for (index = 0; emul_mask; index += emul_len) { + emul_len = 0; + while (emul_mask & 0xff) { + emul_len++; + emul_mask >>= 8; + } + if (emul_len) { + uint32_t mask = ((1 << (emul_len * 8)) - 1); + pci_default_write_config(d, addr, write_val & mask, emul_len); + write_val >>= emul_len * 8; + } else { + emul_mask >>= 8; + write_val >>= 8; + } + } + memory_region_transaction_commit(); out:
Call pci_default_write_config() in xen_pt_pci_write_config() only for registers that do not have custom handler, and do that only after resolving them. This is important for two reasons: 1. XenPTRegInfo has ro_mask which needs to be enforced - Xen-specific hooks do that on their own (especially xen_pt_*_reg_write()). 2. Not setting value early allows the hooks to see the old value too. If it would be only about the first point, setting PCIDevice.wmask would probably be sufficient, but given the second point, restructure those writes. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- hw/xen/xen_pt.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)