[Qemu-devel] Re: [PATCH v9 2/8] pci: fix accesses to pci status register

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Tue, Nov 16, 2010 at 12:52:31PM +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 16, 2010 at 05:26:06PM +0900, Isaku Yamahata wrote:
> > pci status register is 16 bit, not 8 bit.
> > So use helper function to manipulate status register.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> At least the subject is wrong: the relevant bit is in the low byte. So
> the code is correct as written on BE machines.

The code is correct, but also inconsistent and prone to errors.
Except the commit message, are you objecting the patch contents itself?
How about the following commit log?

pci: make accesses to pci status register consistent

pci status register is 16 bits, not 8 bits.
It is inconsistent to access 16 bits register as uint8_t.
So use pci config space functions to manipulate status register
for consistency.


> 
> > ---
> >  hw/pci.c |   21 +++++++++++++--------
> >  1 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index 2fc8ab1..52fe655 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -127,9 +127,11 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
> >  static void pci_update_irq_status(PCIDevice *dev)
> >  {
> >      if (dev->irq_state) {
> > -        dev->config[PCI_STATUS] |= PCI_STATUS_INTERRUPT;
> > +        pci_word_test_and_set_mask(dev->config + PCI_STATUS,
> > +                                   PCI_STATUS_INTERRUPT);
> >      } else {
> > -        dev->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> > +        pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> > +                                     PCI_STATUS_INTERRUPT);
> >      }
> >  }
> >  
> > @@ -404,7 +406,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> >       * in irq_state which we are saving.
> >       * This makes us compatible with old devices
> >       * which never set or clear this bit. */
> > -    s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> > +    pci_word_test_and_clear_mask(s->config + PCI_STATUS, PCI_STATUS_INTERRUPT);
> >      vmstate_save_state(f, pci_get_vmstate(s), s);
> >      /* Restore the interrupt status bit. */
> >      pci_update_irq_status(s);
> > @@ -530,7 +532,7 @@ static void pci_init_cmask(PCIDevice *dev)
> >  {
> >      pci_set_word(dev->cmask + PCI_VENDOR_ID, 0xffff);
> >      pci_set_word(dev->cmask + PCI_DEVICE_ID, 0xffff);
> > -    dev->cmask[PCI_STATUS] = PCI_STATUS_CAP_LIST;
> > +    pci_set_word(dev->cmask + PCI_STATUS, PCI_STATUS_CAP_LIST);
> >      dev->cmask[PCI_REVISION_ID] = 0xff;
> >      dev->cmask[PCI_CLASS_PROG] = 0xff;
> >      pci_set_word(dev->cmask + PCI_CLASS_DEVICE, 0xffff);
> > @@ -1697,8 +1699,9 @@ static uint8_t pci_find_capability_list(PCIDevice *pdev, uint8_t cap_id,
> >  {
> >      uint8_t next, prev;
> >  
> > -    if (!(pdev->config[PCI_STATUS] & PCI_STATUS_CAP_LIST))
> > +    if (!(pci_get_word(pdev->config + PCI_STATUS) & PCI_STATUS_CAP_LIST)) {
> >          return 0;
> > +    }
> >  
> >      for (prev = PCI_CAPABILITY_LIST; (next = pdev->config[prev]);
> >           prev = next + PCI_CAP_LIST_NEXT)
> > @@ -1804,7 +1807,7 @@ int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
> >      config[PCI_CAP_LIST_ID] = cap_id;
> >      config[PCI_CAP_LIST_NEXT] = pdev->config[PCI_CAPABILITY_LIST];
> >      pdev->config[PCI_CAPABILITY_LIST] = offset;
> > -    pdev->config[PCI_STATUS] |= PCI_STATUS_CAP_LIST;
> > +    pci_word_test_and_set_mask(pdev->config + PCI_STATUS, PCI_STATUS_CAP_LIST);
> >      memset(pdev->used + offset, 0xFF, size);
> >      /* Make capability read-only by default */
> >      memset(pdev->wmask + offset, 0, size);
> > @@ -1827,8 +1830,10 @@ void pci_del_capability(PCIDevice *pdev, uint8_t cap_id, uint8_t size)
> >      memset(pdev->cmask + offset, 0, size);
> >      memset(pdev->used + offset, 0, size);
> >  
> > -    if (!pdev->config[PCI_CAPABILITY_LIST])
> > -        pdev->config[PCI_STATUS] &= ~PCI_STATUS_CAP_LIST;
> > +    if (!pdev->config[PCI_CAPABILITY_LIST]) {
> > +        pci_word_test_and_clear_mask(pdev->config + PCI_STATUS,
> > +                                     PCI_STATUS_CAP_LIST);
> > +    }
> >  }
> >  
> >  /* Reserve space for capability at a known offset (to call after load). */
> > -- 
> > 1.7.1.1
> 

-- 
yamahata