Re: [PATCH 1/2] hw/xen/xen_pt: Call default handler only if no custom one is set

[Anthony PERARD]

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,

-- 
Anthony PERARD