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

[BALATON Zoltan]

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?

> >      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?

Regards,
BALATON Zoltan