* [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
@ 2010-12-21 23:20 Andreas Färber
2010-12-22 2:50 ` Isaku Yamahata
0 siblings, 1 reply; 14+ messages in thread
From: Andreas Färber @ 2010-12-21 23:20 UTC (permalink / raw)
To: qemu-devel; +Cc: Andreas Färber, Hervé Poussineau, Michael S. Tsirkin
From: Hervé Poussineau <hpoussin@reactos.org>
v1:
* Rebased.
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
Hello Michael,
Could you please take a look at this? I'm out of my field here.
The intention of the first part appears to be to save (val & ~mask),
whereas the inline helper would've returned (val & mask).
The second part makes existing code conditional on that value.
Regards,
Andreas
hw/pci.c | 22 ++++++++++++++--------
1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/hw/pci.c b/hw/pci.c
index ef00d20..4db4b1f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
static void pci_device_reset(PCIDevice *dev)
{
int r;
+ uint16_t cmd;
/* TODO: call the below unconditionally once all pci devices
* are qdevified */
if (dev->qdev.info) {
@@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
dev->irq_state = 0;
pci_update_irq_status(dev);
/* Clear all writeable bits */
- pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
- pci_get_word(dev->wmask + PCI_COMMAND) |
- pci_get_word(dev->w1cmask + PCI_COMMAND));
+ cmd = pci_get_word(dev->config + PCI_COMMAND);
+ cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
+ pci_get_word(dev->w1cmask + PCI_COMMAND));
+ pci_set_word(dev->config + PCI_COMMAND, cmd);
pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
pci_get_word(dev->wmask + PCI_STATUS) |
pci_get_word(dev->w1cmask + PCI_STATUS));
@@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
continue;
}
- if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
- region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
- pci_set_quad(dev->config + pci_bar(dev, r), region->type);
- } else {
- pci_set_long(dev->config + pci_bar(dev, r), region->type);
+ if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & PCI_COMMAND_MEMORY)) ||
+ (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & PCI_COMMAND_IO))) {
+ /* Reset region address, as it it is not read-only */
+ if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
+ region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
+ pci_set_quad(dev->config + pci_bar(dev, r), region->type);
+ } else {
+ pci_set_long(dev->config + pci_bar(dev, r), region->type);
+ }
}
}
pci_update_mappings(dev);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2010-12-21 23:20 [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space Andreas Färber
@ 2010-12-22 2:50 ` Isaku Yamahata
2010-12-22 6:24 ` Michael S. Tsirkin
2010-12-22 6:30 ` Hervé Poussineau
0 siblings, 2 replies; 14+ messages in thread
From: Isaku Yamahata @ 2010-12-22 2:50 UTC (permalink / raw)
To: Andreas Färber; +Cc: Hervé Poussineau, qemu-devel, Michael S. Tsirkin
On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> From: Hervé Poussineau <hpoussin@reactos.org>
>
> v1:
> * Rebased.
>
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>
> Hello Michael,
>
> Could you please take a look at this? I'm out of my field here.
>
> The intention of the first part appears to be to save (val & ~mask),
> whereas the inline helper would've returned (val & mask).
Such behavior is intended.
The returned value is just discarded in this case.
test-and-clear means
clear the bits
return if those cleared bits were really set.
> The second part makes existing code conditional on that value.
What issue are you addressing?
Although the spec doesn't says about the default value of BAR registers
after reset, the current code assumes that almost all the pci devices clear
those registers.
Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
You, however, seem to want to keep BARs over resets.
thanks,
>
> Regards,
> Andreas
>
> hw/pci.c | 22 ++++++++++++++--------
> 1 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index ef00d20..4db4b1f 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
> static void pci_device_reset(PCIDevice *dev)
> {
> int r;
> + uint16_t cmd;
> /* TODO: call the below unconditionally once all pci devices
> * are qdevified */
> if (dev->qdev.info) {
> @@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
> dev->irq_state = 0;
> pci_update_irq_status(dev);
> /* Clear all writeable bits */
> - pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> - pci_get_word(dev->wmask + PCI_COMMAND) |
> - pci_get_word(dev->w1cmask + PCI_COMMAND));
> + cmd = pci_get_word(dev->config + PCI_COMMAND);
> + cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
> + pci_get_word(dev->w1cmask + PCI_COMMAND));
> + pci_set_word(dev->config + PCI_COMMAND, cmd);
> pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> pci_get_word(dev->wmask + PCI_STATUS) |
> pci_get_word(dev->w1cmask + PCI_STATUS));
> @@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
> continue;
> }
>
> - if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> - region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> - pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> - } else {
> - pci_set_long(dev->config + pci_bar(dev, r), region->type);
> + if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & PCI_COMMAND_MEMORY)) ||
> + (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & PCI_COMMAND_IO))) {
> + /* Reset region address, as it it is not read-only */
> + if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> + region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> + pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> + } else {
> + pci_set_long(dev->config + pci_bar(dev, r), region->type);
> + }
> }
> }
> pci_update_mappings(dev);
> --
> 1.7.3.4
>
>
--
yamahata
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2010-12-22 2:50 ` Isaku Yamahata
@ 2010-12-22 6:24 ` Michael S. Tsirkin
2010-12-22 6:30 ` Hervé Poussineau
1 sibling, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2010-12-22 6:24 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: Andreas Färber, Hervé Poussineau, qemu-devel
On Wed, Dec 22, 2010 at 11:50:14AM +0900, Isaku Yamahata wrote:
> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> > From: Hervé Poussineau <hpoussin@reactos.org>
> >
> > v1:
> > * Rebased.
> >
> > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> > ---
> >
> > Hello Michael,
> >
> > Could you please take a look at this? I'm out of my field here.
> >
> > The intention of the first part appears to be to save (val & ~mask),
> > whereas the inline helper would've returned (val & mask).
>
> Such behavior is intended.
> The returned value is just discarded in this case.
> test-and-clear means
> clear the bits
> return if those cleared bits were really set.
>
>
> > The second part makes existing code conditional on that value.
>
> What issue are you addressing?
Yes I'd like to know that too:
> Although the spec doesn't says about the default value of BAR registers
> after reset, the current code assumes that almost all the pci devices clear
> those registers.
> Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
> You, however, seem to want to keep BARs over resets.
>
> thanks,
>
>
> >
> > Regards,
> > Andreas
> >
> > hw/pci.c | 22 ++++++++++++++--------
> > 1 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/pci.c b/hw/pci.c
> > index ef00d20..4db4b1f 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
> > static void pci_device_reset(PCIDevice *dev)
> > {
> > int r;
> > + uint16_t cmd;
> > /* TODO: call the below unconditionally once all pci devices
> > * are qdevified */
> > if (dev->qdev.info) {
> > @@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
> > dev->irq_state = 0;
> > pci_update_irq_status(dev);
> > /* Clear all writeable bits */
> > - pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> > - pci_get_word(dev->wmask + PCI_COMMAND) |
> > - pci_get_word(dev->w1cmask + PCI_COMMAND));
> > + cmd = pci_get_word(dev->config + PCI_COMMAND);
> > + cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
> > + pci_get_word(dev->w1cmask + PCI_COMMAND));
> > + pci_set_word(dev->config + PCI_COMMAND, cmd);
> > pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> > pci_get_word(dev->wmask + PCI_STATUS) |
> > pci_get_word(dev->w1cmask + PCI_STATUS));
> > @@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
> > continue;
> > }
> >
> > - if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > - region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > - pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> > - } else {
> > - pci_set_long(dev->config + pci_bar(dev, r), region->type);
> > + if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & PCI_COMMAND_MEMORY)) ||
> > + (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & PCI_COMMAND_IO))) {
You want to make memory/io bits in the command register read-only
and hardwired to 1? This seems out of spec, no?
> > + /* Reset region address, as it it is not read-only */
> > + if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > + region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> > + pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> > + } else {
> > + pci_set_long(dev->config + pci_bar(dev, r), region->type);
> > + }
> > }
> > }
> > pci_update_mappings(dev);
> > --
> > 1.7.3.4
> >
> >
>
> --
> yamahata
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2010-12-22 2:50 ` Isaku Yamahata
2010-12-22 6:24 ` Michael S. Tsirkin
@ 2010-12-22 6:30 ` Hervé Poussineau
2010-12-22 6:49 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Hervé Poussineau @ 2010-12-22 6:30 UTC (permalink / raw)
To: Isaku Yamahata
Cc: Andreas Färber, Hervé Poussineau, qemu-devel,
Michael S. Tsirkin
Isaku Yamahata a écrit :
> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>
>> From: Hervé Poussineau <hpoussin@reactos.org>
>>
>> v1:
>> * Rebased.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>>
>> Hello Michael,
>>
>> Could you please take a look at this? I'm out of my field here.
>>
>> The intention of the first part appears to be to save (val & ~mask),
>> whereas the inline helper would've returned (val & mask).
>>
>
> Such behavior is intended.
> The returned value is just discarded in this case.
> test-and-clear means
> clear the bits
> return if those cleared bits were really set.
>
>
>
>> The second part makes existing code conditional on that value.
>>
>
> What issue are you addressing?
> Although the spec doesn't says about the default value of BAR registers
> after reset, the current code assumes that almost all the pci devices clear
> those registers.
> Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
> You, however, seem to want to keep BARs over resets.
>
> thanks,
>
>
>
As you have seen, the intend here is to be able to keep BARs over resets.
It is required for some really specific devices, like a PCI to ISA
bridge, where MMIO is always at the same address.
In that case, the device keeps PCI_COMMAND_MEMORY and/or PCI_COMMAND_IO
flags as read-only.
Hervé
>>
>> Regards,
>> Andreas
>>
>> hw/pci.c | 22 ++++++++++++++--------
>> 1 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index ef00d20..4db4b1f 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
>> static void pci_device_reset(PCIDevice *dev)
>> {
>> int r;
>> + uint16_t cmd;
>> /* TODO: call the below unconditionally once all pci devices
>> * are qdevified */
>> if (dev->qdev.info) {
>> @@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
>> dev->irq_state = 0;
>> pci_update_irq_status(dev);
>> /* Clear all writeable bits */
>> - pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
>> - pci_get_word(dev->wmask + PCI_COMMAND) |
>> - pci_get_word(dev->w1cmask + PCI_COMMAND));
>> + cmd = pci_get_word(dev->config + PCI_COMMAND);
>> + cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
>> + pci_get_word(dev->w1cmask + PCI_COMMAND));
>> + pci_set_word(dev->config + PCI_COMMAND, cmd);
>> pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
>> pci_get_word(dev->wmask + PCI_STATUS) |
>> pci_get_word(dev->w1cmask + PCI_STATUS));
>> @@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
>> continue;
>> }
>>
>> - if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
>> - region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> - pci_set_quad(dev->config + pci_bar(dev, r), region->type);
>> - } else {
>> - pci_set_long(dev->config + pci_bar(dev, r), region->type);
>> + if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & PCI_COMMAND_MEMORY)) ||
>> + (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & PCI_COMMAND_IO))) {
>> + /* Reset region address, as it it is not read-only */
>> + if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
>> + region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> + pci_set_quad(dev->config + pci_bar(dev, r), region->type);
>> + } else {
>> + pci_set_long(dev->config + pci_bar(dev, r), region->type);
>> + }
>> }
>> }
>> pci_update_mappings(dev);
>> --
>> 1.7.3.4
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2010-12-22 6:30 ` Hervé Poussineau
@ 2010-12-22 6:49 ` Michael S. Tsirkin
2011-06-12 11:36 ` Andreas Färber
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2010-12-22 6:49 UTC (permalink / raw)
To: Hervé Poussineau; +Cc: Isaku Yamahata, Andreas Färber, qemu-devel
On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> Isaku Yamahata a écrit :
> >On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>From: Hervé Poussineau <hpoussin@reactos.org>
> >>
> >>v1:
> >>* Rebased.
> >>
> >>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>---
> >> Hello Michael,
> >> Could you please take a look at this? I'm out of my field here.
> >> The intention of the first part appears to be to save (val & ~mask),
> >> whereas the inline helper would've returned (val & mask).
> >
> >Such behavior is intended.
> >The returned value is just discarded in this case.
> >test-and-clear means
> > clear the bits
> > return if those cleared bits were really set.
> >
What about this first chunk? Is it necessary.
> >> The second part makes existing code conditional on that value.
> >
> >What issue are you addressing?
> >Although the spec doesn't says about the default value of BAR registers
> >after reset, the current code assumes that almost all the pci devices clear
> >those registers.
> >Anyway after cold/warm reset firmware sets up BARs, so it doesn't matter.
> >You, however, seem to want to keep BARs over resets.
> >
> >thanks,
> >
> >
> As you have seen, the intend here is to be able to keep BARs over resets.
> It is required for some really specific devices, like a PCI to ISA
> bridge, where MMIO is always at the same address.
> In that case, the device keeps PCI_COMMAND_MEMORY and/or
> PCI_COMMAND_IO flags as read-only.
>
> Hervé
Aha. Are the BARs still writeable? If not maybe that's the right thing
to check? If yes maybe the device simply should have a reset
handler to rewrite them?
Also I would like the above text (maybe a bit shortened) to replace the comment:
/* Reset region address, as it it is not read-only */
the general idea is to document the reason the code
is what it is, not restate what it does.
Something similar should also go into the commit message I think.
> >> Regards,
> >> Andreas
> >> hw/pci.c | 22 ++++++++++++++--------
> >> 1 files changed, 14 insertions(+), 8 deletions(-)
> >>
> >>diff --git a/hw/pci.c b/hw/pci.c
> >>index ef00d20..4db4b1f 100644
> >>--- a/hw/pci.c
> >>+++ b/hw/pci.c
> >>@@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice *dev)
> >> static void pci_device_reset(PCIDevice *dev)
> >> {
> >> int r;
> >>+ uint16_t cmd;
> >> /* TODO: call the below unconditionally once all pci devices
> >> * are qdevified */
> >> if (dev->qdev.info) {
> >>@@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
> >> dev->irq_state = 0;
> >> pci_update_irq_status(dev);
> >> /* Clear all writeable bits */
> >>- pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
> >>- pci_get_word(dev->wmask + PCI_COMMAND) |
> >>- pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+ cmd = pci_get_word(dev->config + PCI_COMMAND);
> >>+ cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
> >>+ pci_get_word(dev->w1cmask + PCI_COMMAND));
> >>+ pci_set_word(dev->config + PCI_COMMAND, cmd);
> >> pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
> >> pci_get_word(dev->wmask + PCI_STATUS) |
> >> pci_get_word(dev->w1cmask + PCI_STATUS));
> >>@@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
> >> continue;
> >> }
> >>- if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>- region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>- pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>- } else {
> >>- pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+ if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd & PCI_COMMAND_MEMORY)) ||
> >>+ (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !(cmd & PCI_COMMAND_IO))) {
These lines are too long.
> >>+ /* Reset region address, as it it is not read-only */
Checking wmask would be more straight-forward?
> >>+ if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> >>+ region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
> >>+ pci_set_quad(dev->config + pci_bar(dev, r), region->type);
> >>+ } else {
> >>+ pci_set_long(dev->config + pci_bar(dev, r), region->type);
> >>+ }
> >> }
> >> }
> >> pci_update_mappings(dev);
> >>--
> >>1.7.3.4
> >>
> >>
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2010-12-22 6:49 ` Michael S. Tsirkin
@ 2011-06-12 11:36 ` Andreas Färber
2011-06-12 12:40 ` Hervé Poussineau
0 siblings, 1 reply; 14+ messages in thread
From: Andreas Färber @ 2011-06-12 11:36 UTC (permalink / raw)
To: Hervé Poussineau
Cc: Isaku Yamahata, qemu-devel Developers, Michael S. Tsirkin
Hervé,
Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
> On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
>> Isaku Yamahata a écrit :
>>> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>
>>>> v1:
>>>> * Rebased.
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>> ---
>>>> Hello Michael,
>>>> Could you please take a look at this? I'm out of my field here.
>>>> The intention of the first part appears to be to save (val &
>>>> ~mask),
>>>> whereas the inline helper would've returned (val & mask).
>>>
>>> Such behavior is intended.
>>> The returned value is just discarded in this case.
>>> test-and-clear means
>>> clear the bits
>>> return if those cleared bits were really set.
>>>
>
> What about this first chunk? Is it necessary.
>
>>>> The second part makes existing code conditional on that value.
>>>
>>> What issue are you addressing?
>>> Although the spec doesn't says about the default value of BAR
>>> registers
>>> after reset, the current code assumes that almost all the pci
>>> devices clear
>>> those registers.
>>> Anyway after cold/warm reset firmware sets up BARs, so it doesn't
>>> matter.
>>> You, however, seem to want to keep BARs over resets.
>>>
>>> thanks,
>>>
>>>
>> As you have seen, the intend here is to be able to keep BARs over
>> resets.
>> It is required for some really specific devices, like a PCI to ISA
>> bridge, where MMIO is always at the same address.
>> In that case, the device keeps PCI_COMMAND_MEMORY and/or
>> PCI_COMMAND_IO flags as read-only.
>>
>> Hervé
>
> Aha. Are the BARs still writeable? If not maybe that's the right
> thing
> to check? If yes maybe the device simply should have a reset
> handler to rewrite them?
I haven't noticed a follow-up patch of yours.
Since I don't know what to do here, I'll have to take this out of the
PReP queue for now.
Without this patch, I get to at least the second bootloader icon, the
PCI graphics still work. What particular symptoms did you observe wrt
the i82378 that we can reproduce?
Thanks,
Andreas
> Also I would like the above text (maybe a bit shortened) to replace
> the comment:
> /* Reset region address, as it it is not read-only */
> the general idea is to document the reason the code
> is what it is, not restate what it does.
>
> Something similar should also go into the commit message I think.
>
>
>>>> Regards,
>>>> Andreas
>>>> hw/pci.c | 22 ++++++++++++++--------
>>>> 1 files changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/hw/pci.c b/hw/pci.c
>>>> index ef00d20..4db4b1f 100644
>>>> --- a/hw/pci.c
>>>> +++ b/hw/pci.c
>>>> @@ -140,6 +140,7 @@ static void pci_update_irq_status(PCIDevice
>>>> *dev)
>>>> static void pci_device_reset(PCIDevice *dev)
>>>> {
>>>> int r;
>>>> + uint16_t cmd;
>>>> /* TODO: call the below unconditionally once all pci devices
>>>> * are qdevified */
>>>> if (dev->qdev.info) {
>>>> @@ -149,9 +150,10 @@ static void pci_device_reset(PCIDevice *dev)
>>>> dev->irq_state = 0;
>>>> pci_update_irq_status(dev);
>>>> /* Clear all writeable bits */
>>>> - pci_word_test_and_clear_mask(dev->config + PCI_COMMAND,
>>>> - pci_get_word(dev->wmask +
>>>> PCI_COMMAND) |
>>>> - pci_get_word(dev->w1cmask +
>>>> PCI_COMMAND));
>>>> + cmd = pci_get_word(dev->config + PCI_COMMAND);
>>>> + cmd &= ~(pci_get_word(dev->wmask + PCI_COMMAND) |
>>>> + pci_get_word(dev->w1cmask + PCI_COMMAND));
>>>> + pci_set_word(dev->config + PCI_COMMAND, cmd);
>>>> pci_word_test_and_clear_mask(dev->config + PCI_STATUS,
>>>> pci_get_word(dev->wmask +
>>>> PCI_STATUS) |
>>>> pci_get_word(dev->w1cmask +
>>>> PCI_STATUS));
>>>> @@ -163,11 +165,15 @@ static void pci_device_reset(PCIDevice *dev)
>>>> continue;
>>>> }
>>>> - if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
>>>> - region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>>> - pci_set_quad(dev->config + pci_bar(dev, r), region-
>>>> >type);
>>>> - } else {
>>>> - pci_set_long(dev->config + pci_bar(dev, r), region-
>>>> >type);
>>>> + if ((region->type == PCI_BASE_ADDRESS_SPACE_IO && !(cmd
>>>> & PCI_COMMAND_MEMORY)) ||
>>>> + (region->type == PCI_BASE_ADDRESS_SPACE_MEMORY && !
>>>> (cmd & PCI_COMMAND_IO))) {
>
> These lines are too long.
>
>>>> + /* Reset region address, as it it is not read-only */
>
> Checking wmask would be more straight-forward?
>
>
>>>> + if (!(region->type & PCI_BASE_ADDRESS_SPACE_IO) &&
>>>> + region->type & PCI_BASE_ADDRESS_MEM_TYPE_64) {
>>>> + pci_set_quad(dev->config + pci_bar(dev, r),
>>>> region->type);
>>>> + } else {
>>>> + pci_set_long(dev->config + pci_bar(dev, r),
>>>> region->type);
>>>> + }
>>>> }
>>>> }
>>>> pci_update_mappings(dev);
>>>> --
>>>> 1.7.3.4
>>>>
>>>>
>>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 11:36 ` Andreas Färber
@ 2011-06-12 12:40 ` Hervé Poussineau
2011-06-12 14:20 ` Andreas Färber
2011-06-12 19:33 ` Michael S. Tsirkin
0 siblings, 2 replies; 14+ messages in thread
From: Hervé Poussineau @ 2011-06-12 12:40 UTC (permalink / raw)
To: Andreas Färber
Cc: Isaku Yamahata, Hervé Poussineau, qemu-devel Developers,
Michael S. Tsirkin
Andreas,
Andreas Färber a écrit :
>
> Hervé,
>
> Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
>
>> On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
>>> Isaku Yamahata a écrit :
>>>> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>
>>>>> v1:
>>>>> * Rebased.
>>>>>
>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>> ---
>>>>> Hello Michael,
>>>>> Could you please take a look at this? I'm out of my field here.
>>>>> The intention of the first part appears to be to save (val & ~mask),
>>>>> whereas the inline helper would've returned (val & mask).
>>>>
>>>> Such behavior is intended.
>>>> The returned value is just discarded in this case.
>>>> test-and-clear means
>>>> clear the bits
>>>> return if those cleared bits were really set.
>>>>
>>
>> What about this first chunk? Is it necessary.
>>
>>>>> The second part makes existing code conditional on that value.
>>>>
>>>> What issue are you addressing?
>>>> Although the spec doesn't says about the default value of BAR
>>>> registers
>>>> after reset, the current code assumes that almost all the pci
>>>> devices clear
>>>> those registers.
>>>> Anyway after cold/warm reset firmware sets up BARs, so it doesn't
>>>> matter.
>>>> You, however, seem to want to keep BARs over resets.
>>>>
>>>> thanks,
>>>>
>>>>
>>> As you have seen, the intend here is to be able to keep BARs over
>>> resets.
>>> It is required for some really specific devices, like a PCI to ISA
>>> bridge, where MMIO is always at the same address.
>>> In that case, the device keeps PCI_COMMAND_MEMORY and/or
>>> PCI_COMMAND_IO flags as read-only.
>>>
>>> Hervé
>>
>> Aha. Are the BARs still writeable? If not maybe that's the right thing
>> to check? If yes maybe the device simply should have a reset
>> handler to rewrite them?
>
> I haven't noticed a follow-up patch of yours.
>
> Since I don't know what to do here, I'll have to take this out of the
> PReP queue for now.
> Without this patch, I get to at least the second bootloader icon, the
> PCI graphics still work. What particular symptoms did you observe wrt
> the i82378 that we can reproduce?
>
> Thanks,
> Andreas
>
Try do do info qtree with and without this patch, and check the i82378
device
With this patch, I have:
bar 0: mem at 0x80000000 [0x8000ffff]
bar 1: mem at 0xc0000000 [0xc0ffffff]
Without it, I have:
bar 0: mem at 0xffffffffffffffff [0xfffe]
bar 1: mem at 0xffffffffffffffff [0xfffffe]
I think that firmware doesn't initialize BARs for this device. However,
I don't know what can be the consequences of not doing it.
Regards,
Hervé
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 12:40 ` Hervé Poussineau
@ 2011-06-12 14:20 ` Andreas Färber
2011-06-12 18:28 ` Hervé Poussineau
2011-06-12 19:53 ` Michael S. Tsirkin
2011-06-12 19:33 ` Michael S. Tsirkin
1 sibling, 2 replies; 14+ messages in thread
From: Andreas Färber @ 2011-06-12 14:20 UTC (permalink / raw)
To: Hervé Poussineau
Cc: Isaku Yamahata, qemu-devel Developers, Michael S. Tsirkin
Am 12.06.2011 um 14:40 schrieb Hervé Poussineau:
> Andreas Färber a écrit :
>> Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
>>
>>> On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
>>>> Isaku Yamahata a écrit :
>>>>> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>
>>>>>> v1:
>>>>>> * Rebased.
>>>>>>
>>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>>> ---
>>>>>> Hello Michael,
>>>>>> Could you please take a look at this? I'm out of my field here.
>>>>>> The intention of the first part appears to be to save (val &
>>>>>> ~mask),
>>>>>> whereas the inline helper would've returned (val & mask).
>>>>>
>>>>> Such behavior is intended.
>>>>> The returned value is just discarded in this case.
>>>>> test-and-clear means
>>>>> clear the bits
>>>>> return if those cleared bits were really set.
>>>>>
>>>
>>> What about this first chunk? Is it necessary.
>>>
>>>>>> The second part makes existing code conditional on that value.
>>>>>
>>>>> What issue are you addressing?
>>>>> Although the spec doesn't says about the default value of BAR
>>>>> registers
>>>>> after reset, the current code assumes that almost all the pci
>>>>> devices clear
>>>>> those registers.
>>>>> Anyway after cold/warm reset firmware sets up BARs, so it
>>>>> doesn't matter.
>>>>> You, however, seem to want to keep BARs over resets.
>>>>>
>>>> As you have seen, the intend here is to be able to keep BARs over
>>>> resets.
>>>> It is required for some really specific devices, like a PCI to ISA
>>>> bridge, where MMIO is always at the same address.
>>>> In that case, the device keeps PCI_COMMAND_MEMORY and/or
>>>> PCI_COMMAND_IO flags as read-only.
>>>
>>> Aha. Are the BARs still writeable? If not maybe that's the right
>>> thing
>>> to check? If yes maybe the device simply should have a reset
>>> handler to rewrite them?
>>
>> I haven't noticed a follow-up patch of yours.
>>
>> Since I don't know what to do here, I'll have to take this out of
>> the PReP queue for now.
>> Without this patch, I get to at least the second bootloader icon,
>> the PCI graphics still work. What particular symptoms did you
>> observe wrt the i82378 that we can reproduce?
>
> Try do do info qtree with and without this patch, and check the
> i82378 device
> With this patch, I have:
> bar 0: mem at 0x80000000 [0x8000ffff]
> bar 1: mem at 0xc0000000 [0xc0ffffff]
>
> Without it, I have:
> bar 0: mem at 0xffffffffffffffff [0xfffe]
> bar 1: mem at 0xffffffffffffffff [0xfffffe]
That I can confirm.
> I think that firmware doesn't initialize BARs for this device.
> However, I don't know what can be the consequences of not doing it.
Re-testing this after a rebase, I don't get any graphics at all
without this patch. Must've done something wrong earlier...
So, how to tell if the BARs are "still writable"? (mst's question)
Andreas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 14:20 ` Andreas Färber
@ 2011-06-12 18:28 ` Hervé Poussineau
2011-06-12 19:35 ` Michael S. Tsirkin
2011-06-12 19:53 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Hervé Poussineau @ 2011-06-12 18:28 UTC (permalink / raw)
To: Andreas Färber
Cc: Isaku Yamahata, Hervé Poussineau, qemu-devel Developers,
Michael S. Tsirkin
Andreas Färber a écrit :
>
> Am 12.06.2011 um 14:40 schrieb Hervé Poussineau:
>
>> Andreas Färber a écrit :
>>> Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
>>>
>>>> On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
>>>>> Isaku Yamahata a écrit :
>>>>>> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>>>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>>
>>>>>>> v1:
>>>>>>> * Rebased.
>>>>>>>
>>>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>>>> ---
>>>>>>> Hello Michael,
>>>>>>> Could you please take a look at this? I'm out of my field here.
>>>>>>> The intention of the first part appears to be to save (val &
>>>>>>> ~mask),
>>>>>>> whereas the inline helper would've returned (val & mask).
>>>>>>
>>>>>> Such behavior is intended.
>>>>>> The returned value is just discarded in this case.
>>>>>> test-and-clear means
>>>>>> clear the bits
>>>>>> return if those cleared bits were really set.
>>>>>>
>>>>
>>>> What about this first chunk? Is it necessary.
>>>>
>>>>>>> The second part makes existing code conditional on that value.
>>>>>>
>>>>>> What issue are you addressing?
>>>>>> Although the spec doesn't says about the default value of BAR
>>>>>> registers
>>>>>> after reset, the current code assumes that almost all the pci
>>>>>> devices clear
>>>>>> those registers.
>>>>>> Anyway after cold/warm reset firmware sets up BARs, so it doesn't
>>>>>> matter.
>>>>>> You, however, seem to want to keep BARs over resets.
>>>>>>
>>>>> As you have seen, the intend here is to be able to keep BARs over
>>>>> resets.
>>>>> It is required for some really specific devices, like a PCI to ISA
>>>>> bridge, where MMIO is always at the same address.
>>>>> In that case, the device keeps PCI_COMMAND_MEMORY and/or
>>>>> PCI_COMMAND_IO flags as read-only.
>>>>
>>>> Aha. Are the BARs still writeable? If not maybe that's the right
>>>> thing
>>>> to check? If yes maybe the device simply should have a reset
>>>> handler to rewrite them?
>>>
>>> I haven't noticed a follow-up patch of yours.
>>>
>>> Since I don't know what to do here, I'll have to take this out of
>>> the PReP queue for now.
>>> Without this patch, I get to at least the second bootloader icon,
>>> the PCI graphics still work. What particular symptoms did you
>>> observe wrt the i82378 that we can reproduce?
>>
>> Try do do info qtree with and without this patch, and check the
>> i82378 device
>> With this patch, I have:
>> bar 0: mem at 0x80000000 [0x8000ffff]
>> bar 1: mem at 0xc0000000 [0xc0ffffff]
>>
>> Without it, I have:
>> bar 0: mem at 0xffffffffffffffff [0xfffe]
>> bar 1: mem at 0xffffffffffffffff [0xfffffe]
>
> That I can confirm.
>
>> I think that firmware doesn't initialize BARs for this device.
>> However, I don't know what can be the consequences of not doing it.
>
> Re-testing this after a rebase, I don't get any graphics at all
> without this patch. Must've done something wrong earlier...
>
> So, how to tell if the BARs are "still writable"? (mst's question)
>
I don't know if the BARs are hardwired to some value, or if they have
default values.
I've no real information on this aspect, so you might take a look at the
datasheet:
http://www.datasheetcatalog.org/datasheets2/10/1064845_1.pdf
Regarding PCI command register, it is explicitly said page 33 that "Bus
Master Enable", "Memory Space Enable" and "I/O Space Enable" are
hardwired to 1.
Regards,
Hervé
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 12:40 ` Hervé Poussineau
2011-06-12 14:20 ` Andreas Färber
@ 2011-06-12 19:33 ` Michael S. Tsirkin
2011-06-12 19:50 ` Hervé Poussineau
1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2011-06-12 19:33 UTC (permalink / raw)
To: Hervé Poussineau
Cc: Isaku Yamahata, Andreas Färber, qemu-devel Developers
On Sun, Jun 12, 2011 at 02:40:35PM +0200, Hervé Poussineau wrote:
> Andreas,
>
> Andreas Färber a écrit :
> >
> >Hervé,
> >
> >Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
> >
> >>On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> >>>Isaku Yamahata a écrit :
> >>>>On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>>>>From: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>
> >>>>>v1:
> >>>>>* Rebased.
> >>>>>
> >>>>>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>>>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>>>>---
> >>>>>Hello Michael,
> >>>>>Could you please take a look at this? I'm out of my field here.
> >>>>>The intention of the first part appears to be to save (val & ~mask),
> >>>>>whereas the inline helper would've returned (val & mask).
> >>>>
> >>>>Such behavior is intended.
> >>>>The returned value is just discarded in this case.
> >>>>test-and-clear means
> >>>> clear the bits
> >>>> return if those cleared bits were really set.
> >>>>
> >>
> >>What about this first chunk? Is it necessary.
> >>
> >>>>>The second part makes existing code conditional on that value.
> >>>>
> >>>>What issue are you addressing?
> >>>>Although the spec doesn't says about the default value of
> >>>>BAR registers
> >>>>after reset, the current code assumes that almost all the
> >>>>pci devices clear
> >>>>those registers.
> >>>>Anyway after cold/warm reset firmware sets up BARs, so it
> >>>>doesn't matter.
> >>>>You, however, seem to want to keep BARs over resets.
> >>>>
> >>>>thanks,
> >>>>
> >>>>
> >>>As you have seen, the intend here is to be able to keep BARs
> >>>over resets.
> >>>It is required for some really specific devices, like a PCI to ISA
> >>>bridge, where MMIO is always at the same address.
> >>>In that case, the device keeps PCI_COMMAND_MEMORY and/or
> >>>PCI_COMMAND_IO flags as read-only.
> >>>
> >>>Hervé
> >>
> >>Aha. Are the BARs still writeable? If not maybe that's the right thing
> >>to check? If yes maybe the device simply should have a reset
> >>handler to rewrite them?
> >
> >I haven't noticed a follow-up patch of yours.
> >
> >Since I don't know what to do here, I'll have to take this out of
> >the PReP queue for now.
> >Without this patch, I get to at least the second bootloader icon,
> >the PCI graphics still work. What particular symptoms did you
> >observe wrt the i82378 that we can reproduce?
> >
> >Thanks,
> >Andreas
> >
>
> Try do do info qtree with and without this patch, and check the
> i82378 device
> With this patch, I have:
> bar 0: mem at 0x80000000 [0x8000ffff]
> bar 1: mem at 0xc0000000 [0xc0ffffff]
>
> Without it, I have:
> bar 0: mem at 0xffffffffffffffff [0xfffe]
> bar 1: mem at 0xffffffffffffffff [0xfffffe]
>
> I think that firmware doesn't initialize BARs for this device.
Interesting. So what set the BARs to these values (0x80000000
and 0xc0000000)?
> However, I don't know what can be the consequences of not doing it.
>
> Regards,
>
> Hervé
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 18:28 ` Hervé Poussineau
@ 2011-06-12 19:35 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2011-06-12 19:35 UTC (permalink / raw)
To: Hervé Poussineau
Cc: Isaku Yamahata, Andreas Färber, qemu-devel Developers
On Sun, Jun 12, 2011 at 08:28:09PM +0200, Hervé Poussineau wrote:
> Andreas Färber a écrit :
> >
> >Am 12.06.2011 um 14:40 schrieb Hervé Poussineau:
> >
> >>Andreas Färber a écrit :
> >>>Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
> >>>
> >>>>On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> >>>>>Isaku Yamahata a écrit :
> >>>>>>On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>>>>>>From: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>>
> >>>>>>>v1:
> >>>>>>>* Rebased.
> >>>>>>>
> >>>>>>>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>>>>>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>>>>>>---
> >>>>>>>Hello Michael,
> >>>>>>>Could you please take a look at this? I'm out of my field here.
> >>>>>>>The intention of the first part appears to be to save
> >>>>>>>(val & ~mask),
> >>>>>>>whereas the inline helper would've returned (val & mask).
> >>>>>>
> >>>>>>Such behavior is intended.
> >>>>>>The returned value is just discarded in this case.
> >>>>>>test-and-clear means
> >>>>>> clear the bits
> >>>>>> return if those cleared bits were really set.
> >>>>>>
> >>>>
> >>>>What about this first chunk? Is it necessary.
> >>>>
> >>>>>>>The second part makes existing code conditional on that value.
> >>>>>>
> >>>>>>What issue are you addressing?
> >>>>>>Although the spec doesn't says about the default value
> >>>>>>of BAR registers
> >>>>>>after reset, the current code assumes that almost all
> >>>>>>the pci devices clear
> >>>>>>those registers.
> >>>>>>Anyway after cold/warm reset firmware sets up BARs, so
> >>>>>>it doesn't matter.
> >>>>>>You, however, seem to want to keep BARs over resets.
> >>>>>>
> >>>>>As you have seen, the intend here is to be able to keep
> >>>>>BARs over resets.
> >>>>>It is required for some really specific devices, like a PCI to ISA
> >>>>>bridge, where MMIO is always at the same address.
> >>>>>In that case, the device keeps PCI_COMMAND_MEMORY and/or
> >>>>>PCI_COMMAND_IO flags as read-only.
> >>>>
> >>>>Aha. Are the BARs still writeable? If not maybe that's the
> >>>>right thing
> >>>>to check? If yes maybe the device simply should have a reset
> >>>>handler to rewrite them?
> >>>
> >>>I haven't noticed a follow-up patch of yours.
> >>>
> >>>Since I don't know what to do here, I'll have to take this out
> >>>of the PReP queue for now.
> >>>Without this patch, I get to at least the second bootloader
> >>>icon, the PCI graphics still work. What particular symptoms
> >>>did you observe wrt the i82378 that we can reproduce?
> >>
> >>Try do do info qtree with and without this patch, and check the
> >>i82378 device
> >>With this patch, I have:
> >> bar 0: mem at 0x80000000 [0x8000ffff]
> >> bar 1: mem at 0xc0000000 [0xc0ffffff]
> >>
> >>Without it, I have:
> >> bar 0: mem at 0xffffffffffffffff [0xfffe]
> >> bar 1: mem at 0xffffffffffffffff [0xfffffe]
> >
> >That I can confirm.
> >
> >>I think that firmware doesn't initialize BARs for this device.
> >>However, I don't know what can be the consequences of not doing
> >>it.
> >
> >Re-testing this after a rebase, I don't get any graphics at all
> >without this patch. Must've done something wrong earlier...
> >
> >So, how to tell if the BARs are "still writable"? (mst's question)
> >
> I don't know if the BARs are hardwired to some value, or if they
> have default values.
> I've no real information on this aspect, so you might take a look at
> the datasheet:
> http://www.datasheetcatalog.org/datasheets2/10/1064845_1.pdf
This doesn't say anything about BARs.
> Regarding PCI command register, it is explicitly said page 33 that
> "Bus Master Enable", "Memory Space Enable" and "I/O Space Enable"
> are hardwired to 1.
>
> Regards,
>
> Hervé
Right. It would seem that the only way this can be safe
is if the device has no BARs (only legacy ranges)
or if the BARs are initializaed to a safe value
somehow.
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 19:33 ` Michael S. Tsirkin
@ 2011-06-12 19:50 ` Hervé Poussineau
2011-06-12 19:59 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Hervé Poussineau @ 2011-06-12 19:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Isaku Yamahata, Andreas Färber, Hervé Poussineau,
qemu-devel Developers
Michael S. Tsirkin a écrit :
> On Sun, Jun 12, 2011 at 02:40:35PM +0200, Hervé Poussineau wrote:
>
>> Andreas,
>>
>> Andreas Färber a écrit :
>>
>>> Hervé,
>>>
>>> Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
>>>
>>>
>>>> On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
>>>>
>>>>> Isaku Yamahata a écrit :
>>>>>
>>>>>> On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
>>>>>>
>>>>>>> From: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>>
>>>>>>> v1:
>>>>>>> * Rebased.
>>>>>>>
>>>>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>>>> ---
>>>>>>> Hello Michael,
>>>>>>> Could you please take a look at this? I'm out of my field here.
>>>>>>> The intention of the first part appears to be to save (val & ~mask),
>>>>>>> whereas the inline helper would've returned (val & mask).
>>>>>>>
>>>>>> Such behavior is intended.
>>>>>> The returned value is just discarded in this case.
>>>>>> test-and-clear means
>>>>>> clear the bits
>>>>>> return if those cleared bits were really set.
>>>>>>
>>>>>>
>>>> What about this first chunk? Is it necessary.
>>>>
>>>>
>>>>>>> The second part makes existing code conditional on that value.
>>>>>>>
>>>>>> What issue are you addressing?
>>>>>> Although the spec doesn't says about the default value of
>>>>>> BAR registers
>>>>>> after reset, the current code assumes that almost all the
>>>>>> pci devices clear
>>>>>> those registers.
>>>>>> Anyway after cold/warm reset firmware sets up BARs, so it
>>>>>> doesn't matter.
>>>>>> You, however, seem to want to keep BARs over resets.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>>
>>>>>>
>>>>> As you have seen, the intend here is to be able to keep BARs
>>>>> over resets.
>>>>> It is required for some really specific devices, like a PCI to ISA
>>>>> bridge, where MMIO is always at the same address.
>>>>> In that case, the device keeps PCI_COMMAND_MEMORY and/or
>>>>> PCI_COMMAND_IO flags as read-only.
>>>>>
>>>>> Hervé
>>>>>
>>>> Aha. Are the BARs still writeable? If not maybe that's the right thing
>>>> to check? If yes maybe the device simply should have a reset
>>>> handler to rewrite them?
>>>>
>>> I haven't noticed a follow-up patch of yours.
>>>
>>> Since I don't know what to do here, I'll have to take this out of
>>> the PReP queue for now.
>>> Without this patch, I get to at least the second bootloader icon,
>>> the PCI graphics still work. What particular symptoms did you
>>> observe wrt the i82378 that we can reproduce?
>>>
>>> Thanks,
>>> Andreas
>>>
>>>
>> Try do do info qtree with and without this patch, and check the
>> i82378 device
>> With this patch, I have:
>> bar 0: mem at 0x80000000 [0x8000ffff]
>> bar 1: mem at 0xc0000000 [0xc0ffffff]
>>
>> Without it, I have:
>> bar 0: mem at 0xffffffffffffffff [0xfffe]
>> bar 1: mem at 0xffffffffffffffff [0xfffffe]
>>
>> I think that firmware doesn't initialize BARs for this device.
>>
>
> Interesting. So what set the BARs to these values (0x80000000
> and 0xc0000000)
Currently, those default values are stored in qdev properties of device,
and device fills in BARs addresses in its init function
+static int pci_i82378_init(PCIDevice *pci_dev)
...
+ /* Make addresses read only */
+ pci_set_word(pci_dev->wmask + PCI_COMMAND,
+ PCI_COMMAND_SPECIAL);
+ pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 0 * 4, pci->isa_io_base);
+ pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 1 * 4, pci->isa_mem_base);
Regards
Hervé
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 14:20 ` Andreas Färber
2011-06-12 18:28 ` Hervé Poussineau
@ 2011-06-12 19:53 ` Michael S. Tsirkin
1 sibling, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2011-06-12 19:53 UTC (permalink / raw)
To: Andreas Färber
Cc: Isaku Yamahata, Hervé Poussineau, qemu-devel Developers
On Sun, Jun 12, 2011 at 04:20:10PM +0200, Andreas Färber wrote:
> Am 12.06.2011 um 14:40 schrieb Hervé Poussineau:
>
> >Andreas Färber a écrit :
> >>Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
> >>
> >>>On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> >>>>Isaku Yamahata a écrit :
> >>>>>On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>>>>>From: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>
> >>>>>>v1:
> >>>>>>* Rebased.
> >>>>>>
> >>>>>>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>>>>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>>>>>---
> >>>>>>Hello Michael,
> >>>>>>Could you please take a look at this? I'm out of my field here.
> >>>>>>The intention of the first part appears to be to save
> >>>>>>(val & ~mask),
> >>>>>>whereas the inline helper would've returned (val & mask).
> >>>>>
> >>>>>Such behavior is intended.
> >>>>>The returned value is just discarded in this case.
> >>>>>test-and-clear means
> >>>>> clear the bits
> >>>>> return if those cleared bits were really set.
> >>>>>
> >>>
> >>>What about this first chunk? Is it necessary.
> >>>
> >>>>>>The second part makes existing code conditional on that value.
> >>>>>
> >>>>>What issue are you addressing?
> >>>>>Although the spec doesn't says about the default value of
> >>>>>BAR registers
> >>>>>after reset, the current code assumes that almost all the
> >>>>>pci devices clear
> >>>>>those registers.
> >>>>>Anyway after cold/warm reset firmware sets up BARs, so it
> >>>>>doesn't matter.
> >>>>>You, however, seem to want to keep BARs over resets.
> >>>>>
> >>>>As you have seen, the intend here is to be able to keep BARs
> >>>>over resets.
> >>>>It is required for some really specific devices, like a PCI to ISA
> >>>>bridge, where MMIO is always at the same address.
> >>>>In that case, the device keeps PCI_COMMAND_MEMORY and/or
> >>>>PCI_COMMAND_IO flags as read-only.
> >>>
> >>>Aha. Are the BARs still writeable? If not maybe that's the
> >>>right thing
> >>>to check? If yes maybe the device simply should have a reset
> >>>handler to rewrite them?
> >>
> >>I haven't noticed a follow-up patch of yours.
> >>
> >>Since I don't know what to do here, I'll have to take this out
> >>of the PReP queue for now.
> >>Without this patch, I get to at least the second bootloader
> >>icon, the PCI graphics still work. What particular symptoms did
> >>you observe wrt the i82378 that we can reproduce?
> >
> >Try do do info qtree with and without this patch, and check the
> >i82378 device
> >With this patch, I have:
> > bar 0: mem at 0x80000000 [0x8000ffff]
> > bar 1: mem at 0xc0000000 [0xc0ffffff]
> >
> >Without it, I have:
> > bar 0: mem at 0xffffffffffffffff [0xfffe]
> > bar 1: mem at 0xffffffffffffffff [0xfffffe]
>
> That I can confirm.
>
> >I think that firmware doesn't initialize BARs for this device.
> >However, I don't know what can be the consequences of not doing
> >it.
>
> Re-testing this after a rebase, I don't get any graphics at all
> without this patch. Must've done something wrong earlier...
>
> So, how to tell if the BARs are "still writable"? (mst's question)
>
> Andreas
I'm just asking what exactly happens with the patch?
- After first boot, firmware writes something
into BARs, then triggers a reset and then
avoids writing there?
- After first boot, firmware writes something
into BARs, but does not enable memory/io/bus master?
- Something else?
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space
2011-06-12 19:50 ` Hervé Poussineau
@ 2011-06-12 19:59 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2011-06-12 19:59 UTC (permalink / raw)
To: Hervé Poussineau
Cc: Isaku Yamahata, Andreas Färber, qemu-devel Developers
On Sun, Jun 12, 2011 at 09:50:45PM +0200, Hervé Poussineau wrote:
> Michael S. Tsirkin a écrit :
> >On Sun, Jun 12, 2011 at 02:40:35PM +0200, Hervé Poussineau wrote:
> >>Andreas,
> >>
> >>Andreas Färber a écrit :
> >>>Hervé,
> >>>
> >>>Am 22.12.2010 um 07:49 schrieb Michael S. Tsirkin:
> >>>
> >>>>On Wed, Dec 22, 2010 at 07:30:33AM +0100, Hervé Poussineau wrote:
> >>>>>Isaku Yamahata a écrit :
> >>>>>>On Wed, Dec 22, 2010 at 12:20:23AM +0100, Andreas Färber wrote:
> >>>>>>>From: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>>
> >>>>>>>v1:
> >>>>>>>* Rebased.
> >>>>>>>
> >>>>>>>Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> >>>>>>>Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>>>>>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>>>>>>---
> >>>>>>>Hello Michael,
> >>>>>>>Could you please take a look at this? I'm out of my field here.
> >>>>>>>The intention of the first part appears to be to save (val & ~mask),
> >>>>>>>whereas the inline helper would've returned (val & mask).
> >>>>>>Such behavior is intended.
> >>>>>>The returned value is just discarded in this case.
> >>>>>>test-and-clear means
> >>>>>> clear the bits
> >>>>>> return if those cleared bits were really set.
> >>>>>>
> >>>>What about this first chunk? Is it necessary.
> >>>>
> >>>>>>>The second part makes existing code conditional on that value.
> >>>>>>What issue are you addressing?
> >>>>>>Although the spec doesn't says about the default value of
> >>>>>>BAR registers
> >>>>>>after reset, the current code assumes that almost all the
> >>>>>>pci devices clear
> >>>>>>those registers.
> >>>>>>Anyway after cold/warm reset firmware sets up BARs, so it
> >>>>>>doesn't matter.
> >>>>>>You, however, seem to want to keep BARs over resets.
> >>>>>>
> >>>>>>thanks,
> >>>>>>
> >>>>>>
> >>>>>As you have seen, the intend here is to be able to keep BARs
> >>>>>over resets.
> >>>>>It is required for some really specific devices, like a PCI to ISA
> >>>>>bridge, where MMIO is always at the same address.
> >>>>>In that case, the device keeps PCI_COMMAND_MEMORY and/or
> >>>>>PCI_COMMAND_IO flags as read-only.
> >>>>>
> >>>>>Hervé
> >>>>Aha. Are the BARs still writeable? If not maybe that's the right thing
> >>>>to check? If yes maybe the device simply should have a reset
> >>>>handler to rewrite them?
> >>>I haven't noticed a follow-up patch of yours.
> >>>
> >>>Since I don't know what to do here, I'll have to take this out of
> >>>the PReP queue for now.
> >>>Without this patch, I get to at least the second bootloader icon,
> >>>the PCI graphics still work. What particular symptoms did you
> >>>observe wrt the i82378 that we can reproduce?
> >>>
> >>>Thanks,
> >>>Andreas
> >>>
> >>Try do do info qtree with and without this patch, and check the
> >>i82378 device
> >>With this patch, I have:
> >> bar 0: mem at 0x80000000 [0x8000ffff]
> >> bar 1: mem at 0xc0000000 [0xc0ffffff]
> >>
> >>Without it, I have:
> >> bar 0: mem at 0xffffffffffffffff [0xfffe]
> >> bar 1: mem at 0xffffffffffffffff [0xfffffe]
> >>
> >>I think that firmware doesn't initialize BARs for this device.
> >
> >Interesting. So what set the BARs to these values (0x80000000
> >and 0xc0000000)
> Currently, those default values are stored in qdev properties of device,
> and device fills in BARs addresses in its init function
>
> +static int pci_i82378_init(PCIDevice *pci_dev)
> ...
> + /* Make addresses read only */
> + pci_set_word(pci_dev->wmask + PCI_COMMAND,
> + PCI_COMMAND_SPECIAL);
> + pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 0 * 4, pci->isa_io_base);
> + pci_set_long(pci_conf + PCI_BASE_ADDRESS_0 + 1 * 4, pci->isa_mem_base);
>
> Regards
>
> Hervé
>
I see. So the right thing to do wrt BARs is probably
to restore these values on system reset as well?
It might be a good idea to replace 'Make addresses read only'
with 'only special cycle enable bit is writeable'.
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-06-12 19:59 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-21 23:20 [Qemu-devel] [PATCH, RFC] pci: allow PCI devices to fix address space Andreas Färber
2010-12-22 2:50 ` Isaku Yamahata
2010-12-22 6:24 ` Michael S. Tsirkin
2010-12-22 6:30 ` Hervé Poussineau
2010-12-22 6:49 ` Michael S. Tsirkin
2011-06-12 11:36 ` Andreas Färber
2011-06-12 12:40 ` Hervé Poussineau
2011-06-12 14:20 ` Andreas Färber
2011-06-12 18:28 ` Hervé Poussineau
2011-06-12 19:35 ` Michael S. Tsirkin
2011-06-12 19:53 ` Michael S. Tsirkin
2011-06-12 19:33 ` Michael S. Tsirkin
2011-06-12 19:50 ` Hervé Poussineau
2011-06-12 19:59 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).