Re: [PATCH v3 2/3] pci: Honour wmask when resetting PCI_INTERRUPT_LINE

[Michael S. Tsirkin]

On Mon, Mar 09, 2020 at 09:54:27PM +0100, BALATON Zoltan wrote:
> On Mon, 9 Mar 2020, Michael S. Tsirkin wrote:
> > On Mon, Mar 09, 2020 at 08:18:13PM +0100, BALATON Zoltan wrote:
> > > The pci_do_device_reset() function (called from pci_device_reset)
> > > clears the PCI_INTERRUPT_LINE config reg of devices on the bus but did
> > > this without taking wmask into account. We'll have a device model now
> > > that needs to set a constant value for this reg and this patch allows
> > > to do that without additional workaround in device emulation to
> > > reverse the effect of this PCI bus reset function.
> > > 
> > > Suggested-by: Mark Cave-Ayland <address@hidden>
> > > Signed-off-by: BALATON Zoltan <address@hidden>
> > > ---
> > >  hw/pci/pci.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index e1ed6677e1..d07e4ed9de 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -302,8 +302,10 @@ static void pci_do_device_reset(PCIDevice *dev)
> > >      pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> > >                                   pci_get_word(dev->wmask + PCI_STATUS) |
> > >                                   pci_get_word(dev->w1cmask + 
> > > PCI_STATUS));
> > > +    pci_word_test_and_clear_mask(dev->config + PCI_INTERRUPT_LINE,
> > > +                              pci_get_word(dev->wmask + 
> > > PCI_INTERRUPT_LINE) |
> > > +                              pci_get_word(dev->w1cmask + 
> > > PCI_INTERRUPT_LINE));
> > 
> > PCI spec says:
> > 
> > Interrupt Line
> > The Interrupt Line register is an eight-bit register used to communicate 
> > interrupt line routing
> > information.
> > 
> > I don't see how it makes sense to access it as a word.
> 
> Patch actually comes from Mark, I don't know. Should we change it to
> pci_byte_test_and_clear_mask or what's the appropriate way here?

Superficially that makes sense. I don't know if that does what
you want to do.


> > 
> > >      dev->config[PCI_CACHE_LINE_SIZE] = 0x0;
> > > -    dev->config[PCI_INTERRUPT_LINE] = 0x0;
> > >      for (r = 0; r < PCI_NUM_REGIONS; ++r) {
> > >          PCIIORegion *region = &dev->io_regions[r];
> > >          if (!region->size) {
> > 
> > Please add comments here explaining that some devices
> > make PCI_INTERRUPT_LINE read-only.
> 
> Something like the commit message would be appropriate?

Code comments are more appropriate when we want to describe why code is
the way it is. commit message is there to describe the change, answering
questions like: why we aren't happy with the old code? why is the new
code is better? etc ...

> Regards,
> BALATON Zoltan