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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature