Re: [PATCH 4/5] pci-assign: Convert need_emulate_cmd into a bitmask

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 13, 2010 at 04:56:29PM -0700, Alex Williamson wrote:
> On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> > From: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > Define a mask of PCI command register bits that need to be emulated,
> > i.e. read back from their shadow state. We will need this for
> > selectively emulating the INTx mask bit.
> > 
> > Note: No initialization of emulate_cmd_mask to zero needed, the device
> > state is already zero-initialized.
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  hw/device-assignment.c |   18 ++++++++++--------
> >  hw/device-assignment.h |    2 +-
> >  2 files changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > index ef045f4..26d3bd7 100644
> > --- a/hw/device-assignment.c
> > +++ b/hw/device-assignment.c
> > @@ -525,14 +525,17 @@ again:
> >      DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
> >            (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
> >  
> > -    if (pci_dev->need_emulate_cmd &&
> > +    if (pci_dev->emulate_cmd_mask &&
> >          ranges_overlap(address, len, PCI_COMMAND, 2)) {
> >          if (address == PCI_COMMAND) {
> > -            val &= 0xffff0000;
> > -            val |= pci_default_read_config(d, PCI_COMMAND, 2);
> > +            val &= ~pci_dev->emulate_cmd_mask;
> > +            val |= pci_default_read_config(d, PCI_COMMAND, 2) &
> > +                        pci_dev->emulate_cmd_mask;
> >          } else {
> >              /* high-byte access */
> > -            val = pci_default_read_config(d, PCI_COMMAND+1, 1);
> > +            val &= ~(pci_dev->emulate_cmd_mask >> 8);
> > +            val |= pci_default_read_config(d, PCI_COMMAND+1, 1) &
> > +                        (pci_dev->emulate_cmd_mask >> 8);
> >          }
> >      }
> 
> We should definitely be using merge_bits here, this is the sort of thing
> I had in mind for it:
> 
> val = merge_bits(val, pci_default_read_config(d, address, len), PCI_COMMAND, pci_dev->emulate_cmd_mask);


Not a comment on this patch at all, rather on
assigned-devices support code generally:

	I think code will become cleaner if we add a mask array mapping the
	config space, along the lines of wmask and friends in pci.c

	I think this will make it possible to mostly get rid of
	merge_bits and tricky range check hacks.


> > @@ -800,10 +803,9 @@ again:
> >  
> >      /* dealing with virtual function device */
> >      snprintf(name, sizeof(name), "%sphysfn/", dir);
> > -    if (!stat(name, &statbuf))
> > -	    pci_dev->need_emulate_cmd = 1;
> > -    else
> > -	    pci_dev->need_emulate_cmd = 0;
> > +    if (!stat(name, &statbuf)) {
> > +        pci_dev->emulate_cmd_mask = 0xffff;
> > +    }
> >  
> >      dev->region_number = r;
> >      return 0;
> > diff --git a/hw/device-assignment.h b/hw/device-assignment.h
> > index c94a730..9ead022 100644
> > --- a/hw/device-assignment.h
> > +++ b/hw/device-assignment.h
> > @@ -109,7 +109,7 @@ typedef struct AssignedDevice {
> >      void *msix_table_page;
> >      target_phys_addr_t msix_table_addr;
> >      int mmio_index;
> > -    int need_emulate_cmd;
> > +    uint32_t emulate_cmd_mask;
> >      char *configfd_name;
> >      QLIST_ENTRY(AssignedDevice) next;
> >  } AssignedDevice;
> 
>