diff mbox series

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

Message ID 857e327a240f2175fe5105f0ebdfe1357fef32c7.1583781494.git.balaton@eik.bme.hu
State New
Headers show
Series Implement "non 100% native mode" in via-ide | expand

Commit Message

BALATON Zoltan March 9, 2020, 7:18 p.m. UTC 
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(-)

Comments

Michael S. Tsirkin March 9, 2020, 8:39 p.m. UTC | #1 
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
> 
>
BALATON Zoltan March 9, 2020, 8:54 p.m. UTC | #2 
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
Michael S. Tsirkin March 10, 2020, 3:36 a.m. UTC | #3 
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
Mark Cave-Ayland March 10, 2020, 6:15 p.m. UTC | #4 
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.
BALATON Zoltan March 10, 2020, 6:20 p.m. UTC | #5 
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 mbox series

Patch

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) {