Message ID | 857e327a240f2175fe5105f0ebdfe1357fef32c7.1583781494.git.balaton@eik.bme.hu |
---|---|
State | New |
Headers | show |
Series | Implement "non 100% native mode" in via-ide | expand |
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 <mark.cave-ayland@ilande.co.uk> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > 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. > 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. > -- > 2.21.1 > >
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 <mark.cave-ayland@ilande.co.uk> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> 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
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 <mark.cave-ayland@ilande.co.uk> > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > --- > > > 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
On 09/03/2020 20:54, 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 <mark.cave-ayland@ilande.co.uk> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> 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? Ooops yes it should pci_byte_test_and_clear_mask(). Clearly I got a bit too excited from copying the existing examples in pci_do_device_reset(). ATB, Mark.
On Tue, 10 Mar 2020, Mark Cave-Ayland wrote: > On 09/03/2020 20:54, 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 <mark.cave-ayland@ilande.co.uk> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> --- >>>> 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? > > Ooops yes it should pci_byte_test_and_clear_mask(). Clearly I got a bit too excited > from copying the existing examples in pci_do_device_reset(). OK will test it and send updated version. Regards, BALATON Zoltan
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)); 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) {
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 <mark.cave-ayland@ilande.co.uk> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/pci/pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)