Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Marcel Apfelbaum 

On 01/14/2016 04:49 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:

2. The same as with pxb, disable Integrated End points for pxb-pcie.


My vote, without a doubt.


Yea, me too.


On a related note: I wonder whether enough resources will be allocated
to the bridge to actually make it possible to add devices by hotplug
later.



It works the same as with PXB, but now instead of having one internal 
PCI-bridge,
we will have several switches/root ports. Each of them will get the minimum MEM 
required by
PCI bridges, however the IO will be allocated only if at least one legacy device
will be present at boot time. (this is at least what SeaBIOS does, I am going 
to check OVMF actions)

Also related, checking that PCIe native hotplug works for devices behind
pxb-pcie bridges is my next step after I fix the current issue.

Thanks,
Marcel





I am going to look at 1., maybe I is doable in a clean way.


My vote: don't. :)

Thanks
Laszlo


Thanks,
Marcel


[...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Michael S. Tsirkin 
On Thu, Jan 14, 2016 at 05:23:27PM +0200, Marcel Apfelbaum wrote:
> On 01/14/2016 04:49 PM, Michael S. Tsirkin wrote:
> >On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:
> >>>2. The same as with pxb, disable Integrated End points for pxb-pcie.
> >>
> >>My vote, without a doubt.
> >
> >Yea, me too.
> >
> >
> >On a related note: I wonder whether enough resources will be allocated
> >to the bridge to actually make it possible to add devices by hotplug
> >later.
> >
> 
> It works the same as with PXB, but now instead of having one internal 
> PCI-bridge,
> we will have several switches/root ports. Each of them will get the minimum 
> MEM required by
> PCI bridges,

what does this mean? What if you add a bunch of devices
with large memory BARs? They won't fit will they?

> however the IO will be allocated only if at least one legacy device
> will be present at boot time. (this is at least what SeaBIOS does, I am going 
> to check OVMF actions)
> 
> Also related, checking that PCIe native hotplug works for devices behind
> pxb-pcie bridges is my next step after I fix the current issue.
> 
> Thanks,
> Marcel
> 
> >
> >>>
> >>>I am going to look at 1., maybe I is doable in a clean way.
> >>
> >>My vote: don't. :)
> >>
> >>Thanks
> >>Laszlo
> >>
> >>>Thanks,
> >>>Marcel
> >>>
> >>>
> >>>[...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Marcel Apfelbaum 

On 01/14/2016 05:37 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 05:23:27PM +0200, Marcel Apfelbaum wrote:

On 01/14/2016 04:49 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:

2. The same as with pxb, disable Integrated End points for pxb-pcie.


My vote, without a doubt.


Yea, me too.


On a related note: I wonder whether enough resources will be allocated
to the bridge to actually make it possible to add devices by hotplug
later.



It works the same as with PXB, but now instead of having one internal 
PCI-bridge,
we will have several switches/root ports. Each of them will get the minimum MEM 
required by
PCI bridges,


what does this mean? What if you add a bunch of devices
with large memory BARs? They won't fit will they?



Indeed, devices with over 1 MB (I think) BARs can't be hot-plugged.
This is a known design limitation. We can think of a way to handle this,
but the real reason we have multiple root bridges is to be able to
correlate an assigned device with a NUMA node. In this case the device
will be added more likely at boot time.


I think the first step is to have *some* hot-plug support for pxb/pxb-pcie
with the current constraints, once it works we can think
of a way to make it work for devices with large BARs.

Thanks,
Marcel


however the IO will be allocated only if at least one legacy device
will be present at boot time. (this is at least what SeaBIOS does, I am going 
to check OVMF actions)

Also related, checking that PCIe native hotplug works for devices behind
pxb-pcie bridges is my next step after I fix the current issue.

Thanks,
Marcel





I am going to look at 1., maybe I is doable in a clean way.


My vote: don't. :)

Thanks
Laszlo


Thanks,
Marcel


[...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Michael S. Tsirkin 
On Thu, Jan 14, 2016 at 07:20:32PM +0200, Marcel Apfelbaum wrote:
> On 01/14/2016 05:37 PM, Michael S. Tsirkin wrote:
> >On Thu, Jan 14, 2016 at 05:23:27PM +0200, Marcel Apfelbaum wrote:
> >>On 01/14/2016 04:49 PM, Michael S. Tsirkin wrote:
> >>>On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:
> >2. The same as with pxb, disable Integrated End points for pxb-pcie.
> 
> My vote, without a doubt.
> >>>
> >>>Yea, me too.
> >>>
> >>>
> >>>On a related note: I wonder whether enough resources will be allocated
> >>>to the bridge to actually make it possible to add devices by hotplug
> >>>later.
> >>>
> >>
> >>It works the same as with PXB, but now instead of having one internal 
> >>PCI-bridge,
> >>we will have several switches/root ports. Each of them will get the minimum 
> >>MEM required by
> >>PCI bridges,
> >
> >what does this mean? What if you add a bunch of devices
> >with large memory BARs? They won't fit will they?
> >
> 
> Indeed, devices with over 1 MB (I think) BARs can't be hot-plugged.
> This is a known design limitation. We can think of a way to handle this,
> but the real reason we have multiple root bridges is to be able to
> correlate an assigned device with a NUMA node. In this case the device
> will be added more likely at boot time.

Ugh. That's pretty nasty, esp considering live
migration pretty much requires hotplug ATM.

> 
> I think the first step is to have *some* hot-plug support for pxb/pxb-pcie
> with the current constraints, once it works we can think
> of a way to make it work for devices with large BARs.
> 
> Thanks,
> Marcel

Well OK but I suspect changes will require host/guest interface changes.
Time enough before 2.6 but I would hate to release 2.6 with this
limitation in place.

And I'd like to mention a real pci express host won't
have this issue I think as it is normally allocated
a range of memory at boot time.


> >>however the IO will be allocated only if at least one legacy device
> >>will be present at boot time. (this is at least what SeaBIOS does, I am 
> >>going to check OVMF actions)
> >>
> >>Also related, checking that PCIe native hotplug works for devices behind
> >>pxb-pcie bridges is my next step after I fix the current issue.
> >>
> >>Thanks,
> >>Marcel
> >>
> >>>
> >
> >I am going to look at 1., maybe I is doable in a clean way.
> 
> My vote: don't. :)
> 
> Thanks
> Laszlo
> 
> >Thanks,
> >Marcel
> >
> >
> >[...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Marcel Apfelbaum 

On 01/11/2016 08:57 PM, Marcel Apfelbaum wrote:

On 01/11/2016 08:44 PM, Laszlo Ersek wrote:

On 01/11/16 19:01, Marcel Apfelbaum wrote:

On 01/11/2016 07:15 PM, Laszlo Ersek wrote:

On 01/11/16 17:34, Marcel Apfelbaum wrote:

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:

On 01/11/16 13:24, Marcel Apfelbaum wrote:

Two reasons:
- PCI Spec indicates that while the bit is not set
  the memory sizing is not finished.
- pci_bar_address will return PCI_BAR_UNMAPPED
  and a previous value can be accidentally overridden
  if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
1. It disables the Decode (I/O or memory) bit in the Command
register
2. It configures the device BARS
3. Makes some tests on the Command register
4. ...
5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge
resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
inside the pxb.
Now we can enable the SHPC for it too.


I encountered the exact same problem months ago. I posted patches for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

As you can see under the second link above, I made the same analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver / enumerator
that is built into OVMF.)


Wow, I just re-worked this issue again from 0! I wish I have remembered
those threads :(
This was another symptom of the exact problem! And I remembered
something about
SHPC, I should have looked at those mail threads again...



I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

he wrote "[t]his is problematic - disabled BAR values have no meaning
according to the PCI spec".



Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.


The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"



I confess I don't exactly understand what the SHPC of the pci-2-pci
bridge
has to do with sibling devices on the pxb's root bus (SHPC is the
hot-plug controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have devices
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge that
behaves like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)



In comparison, your patch doesn't change pci_bar_address(). Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(), if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance with the
PCI specs, then it's likely a good patch. (And apparently more general
than the SHPC-specific solution we have now.)



Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.



I just don't know if it's a good idea to leave any old mappings active
while the BARs are being reprogrammed (with the command bits clear).



First, because the OS can't use the IO/MEM anyway, secondly the guest
OS/firmware
is the one that disabled the bit... (in order to program resources)


I have something like the following in mind. Do you think it is a valid
(although contrived) use case?

- guest programs some BAR and uses it (as designed / intended)
- guest disables command bit, modifies BAR location
- guest accesses *old* BAR location

What should a guest *expect* in such a case? Is this invalid guest
behavior?


Yes, this is indeed invalid behavior, from the device point of view
it is disabled. Best case scenario - the guest will see 0x,
worst case - garbage.



If it is not invalid, then will QEMU comply with the guest's
expectations if your patch is applied? Pre-patch, the guest would likely
access a "hole" in

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Laszlo Ersek 
On 01/14/16 13:24, Marcel Apfelbaum wrote:
> On 01/11/2016 08:57 PM, Marcel Apfelbaum wrote:
>> On 01/11/2016 08:44 PM, Laszlo Ersek wrote:
>>> On 01/11/16 19:01, Marcel Apfelbaum wrote:
 On 01/11/2016 07:15 PM, Laszlo Ersek wrote:
> On 01/11/16 17:34, Marcel Apfelbaum wrote:
>> On 01/11/2016 06:11 PM, Laszlo Ersek wrote:
>>> On 01/11/16 13:24, Marcel Apfelbaum wrote:
 Two reasons:
 - PCI Spec indicates that while the bit is not set
   the memory sizing is not finished.
 - pci_bar_address will return PCI_BAR_UNMAPPED
   and a previous value can be accidentally overridden
   if the command register is modified (and not the BAR).

 Signed-off-by: Marcel Apfelbaum 
 ---

 Hi,

 I found this when trying to use multiple root complexes with OVMF.

 When trying to attach a device to the pxb-pcie device as Integrated
 Device it did not receive the IO/MEM resources.

 The reason is that OVMF is working like that:
 1. It disables the Decode (I/O or memory) bit in the Command
 register
 2. It configures the device BARS
 3. Makes some tests on the Command register
 4. ...
 5. Enables the Decode (I/O or memory) at some point.

 On step 3 all the BARS are overridden to 0x by QEMU.

 Since QEMU uses the device BARs to compute the new host bridge
 resources
 it now gets garbage.

 Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
 inside the pxb.
 Now we can enable the SHPC for it too.
>>>
>>> I encountered the exact same problem months ago. I posted patches
>>> for
>>> it; you were CC'd. :)
>>>
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
>>>
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210
>>>
>>>
>>> As you can see under the second link above, I made the same
>>> analysis &
>>> observations as you do now. (It took me quite long to track down the
>>> "inexplicable" behavior of edk2's generic PCI bus driver /
>>> enumerator
>>> that is built into OVMF.)
>>
>> Wow, I just re-worked this issue again from 0! I wish I have
>> remembered
>> those threads :(
>> This was another symptom of the exact problem! And I remembered
>> something about
>> SHPC, I should have looked at those mail threads again...
>>
>>>
>>> I proposed to change pci_bar_address() so that it could return, to
>>> distinguished callers, the BAR values "under programming", even
>>> if the
>>> command bits were clear. Then the ACPI generator would utilize this
>>> special exception.
>>>
>>> Michael disagreed; in
>>>
>>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242
>>>
>>>
>>> he wrote "[t]his is problematic - disabled BAR values have no
>>> meaning
>>> according to the PCI spec".
>>>
>>
>> Yes... because it looked like a hook for our case only,
>> the good news is that this patch is based exactly on the fact that
>> the BARs have no meaning if the bit is not set.
>>
>>> The current  solution to the problem (= we disable the SHPC) was
>>> recommended by Michael in that message: "It might be best to add a
>>> property to just disable shpc in the bridge so no devices reside
>>> directly behind the pxb?"
>>>
>>
>> I confess I don't exactly understand what the SHPC of the pci-2-pci
>> bridge
>> has to do with sibling devices on the pxb's root bus (SHPC is the
>> hot-plug controller
>> for the devices behind the pci-2-pci bridge).
>>
>> The second part I do understand, the pxb design was to not have
>> devices
>> directly behind
>> the pxb, so maybe he meant that SHPC is the part of the pci-bridge
>> that
>> behaves like
>> a device in the sense it requires IO/MEM resources.
>>
>> Bottom line, your solution for the PXB was just fine :)
>>
>>
>>> In comparison, your patch doesn't change pci_bar_address().
>>> Instead, it
>>> modifies pci_update_mappings() *not to call* pci_bar_address(),
>>> if the
>>> respective command bits are clear.
>>>
>>> I guess that could have about the same effect.
>>>
>>> If, unlike my patch, yours actually improves QEMU's compliance
>>> with the
>>> PCI specs, then it's likely a good patch. (And apparently more
>>> general
>>> than the SHPC-specific solution we have now.)
>>
>>
>> Exactly! Why should a pci write to the command  register *delete*
>> previously set resources? I am looking at it as a bug.
>>
>> And also updating the mappings while the

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Marcel Apfelbaum 

On 01/14/2016 07:28 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 07:20:32PM +0200, Marcel Apfelbaum wrote:

On 01/14/2016 05:37 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 05:23:27PM +0200, Marcel Apfelbaum wrote:

On 01/14/2016 04:49 PM, Michael S. Tsirkin wrote:

On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:

2. The same as with pxb, disable Integrated End points for pxb-pcie.


My vote, without a doubt.


Yea, me too.


On a related note: I wonder whether enough resources will be allocated
to the bridge to actually make it possible to add devices by hotplug
later.



It works the same as with PXB, but now instead of having one internal 
PCI-bridge,
we will have several switches/root ports. Each of them will get the minimum MEM 
required by
PCI bridges,


what does this mean? What if you add a bunch of devices
with large memory BARs? They won't fit will they?



Indeed, devices with over 1 MB (I think) BARs can't be hot-plugged.
This is a known design limitation. We can think of a way to handle this,
but the real reason we have multiple root bridges is to be able to
correlate an assigned device with a NUMA node. In this case the device
will be added more likely at boot time.


Ugh. That's pretty nasty, esp considering live
migration pretty much requires hotplug ATM.



I think the first step is to have *some* hot-plug support for pxb/pxb-pcie
with the current constraints, once it works we can think
of a way to make it work for devices with large BARs.

Thanks,
Marcel


Well OK but I suspect changes will require host/guest interface changes.
Time enough before 2.6 but I would hate to release 2.6 with this
limitation in place.


Understood, the amount of work depends on the design:
1. How much memory/IO should we put aside for each root bridge?
 -  we can let the default as is today, and add optional parameters to pxb 
devices.
2. Pass this to guest firmware?
 - Better not. We let the firmware to config the resources as today, and
   when we build the ACPI tables we just "crop" some extra ranges for
   each pxb based on user input.


Does it sound acceptable?

Thanks,
Marcel






And I'd like to mention a real pci express host won't
have this issue I think as it is normally allocated
a range of memory at boot time.



however the IO will be allocated only if at least one legacy device
will be present at boot time. (this is at least what SeaBIOS does, I am going 
to check OVMF actions)

Also related, checking that PCIe native hotplug works for devices behind
pxb-pcie bridges is my next step after I fix the current issue.

Thanks,
Marcel





I am going to look at 1., maybe I is doable in a clean way.


My vote: don't. :)

Thanks
Laszlo


Thanks,
Marcel


[...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Michael S. Tsirkin 
On Thu, Jan 14, 2016 at 03:30:41PM +0100, Laszlo Ersek wrote:
> > 2. The same as with pxb, disable Integrated End points for pxb-pcie.
> 
> My vote, without a doubt.

Yea, me too.


On a related note: I wonder whether enough resources will be allocated
to the bridge to actually make it possible to add devices by hotplug
later.


> > 
> > I am going to look at 1., maybe I is doable in a clean way.
> 
> My vote: don't. :)
> 
> Thanks
> Laszlo
> 
> > Thanks,
> > Marcel
> > 
> > 
> > [...]

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-14 Thread Marcel Apfelbaum 

On 01/14/2016 04:30 PM, Laszlo Ersek wrote:

On 01/14/16 13:24, Marcel Apfelbaum wrote:

On 01/11/2016 08:57 PM, Marcel Apfelbaum wrote:

On 01/11/2016 08:44 PM, Laszlo Ersek wrote:

On 01/11/16 19:01, Marcel Apfelbaum wrote:

On 01/11/2016 07:15 PM, Laszlo Ersek wrote:

On 01/11/16 17:34, Marcel Apfelbaum wrote:

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:

On 01/11/16 13:24, Marcel Apfelbaum wrote:

Two reasons:
 - PCI Spec indicates that while the bit is not set
   the memory sizing is not finished.
 - pci_bar_address will return PCI_BAR_UNMAPPED
   and a previous value can be accidentally overridden
   if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
 1. It disables the Decode (I/O or memory) bit in the Command
register
 2. It configures the device BARS
 3. Makes some tests on the Command register
 4. ...
 5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge
resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
inside the pxb.
Now we can enable the SHPC for it too.


I encountered the exact same problem months ago. I posted patches
for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210


As you can see under the second link above, I made the same
analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver /
enumerator
that is built into OVMF.)


Wow, I just re-worked this issue again from 0! I wish I have
remembered
those threads :(
This was another symptom of the exact problem! And I remembered
something about
SHPC, I should have looked at those mail threads again...



I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even
if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242


he wrote "[t]his is problematic - disabled BAR values have no
meaning
according to the PCI spec".



Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.


The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"



I confess I don't exactly understand what the SHPC of the pci-2-pci
bridge
has to do with sibling devices on the pxb's root bus (SHPC is the
hot-plug controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have
devices
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge
that
behaves like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)



In comparison, your patch doesn't change pci_bar_address().
Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(),
if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance
with the
PCI specs, then it's likely a good patch. (And apparently more
general
than the SHPC-specific solution we have now.)



Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.



I just don't know if it's a good idea to leave any old mappings
active
while the BARs are being reprogrammed (with the command bits clear).



First, because the OS can't use the IO/MEM anyway, secondly the guest
OS/firmware
is the one that disabled the bit... (in order to program resources)


I have something like the following in mind. Do you think it is a
valid
(although contrived) use case?

- guest programs some BAR and uses it (as designed / intended)
- guest disables command bit, modifies BAR location
- guest accesses *old* BAR location

What should a guest *expect* in such a case? Is this invalid guest
behavior?


Yes, this is indeed invalid behavior, from the device point of view
it is disabled. Best case scenario - the guest will see 0x,
worst case - garbage.



If it is not invalid, then will QEMU comply with

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Igor Mammedov 
On Mon, 11 Jan 2016 14:24:23 +0200
Marcel Apfelbaum  wrote:

> Two reasons:
>  - PCI Spec indicates that while the bit is not set
>the memory sizing is not finished.
>  - pci_bar_address will return PCI_BAR_UNMAPPED
>and a previous value can be accidentally overridden
>if the command register is modified (and not the BAR).
> 
> Signed-off-by: Marcel Apfelbaum 
> ---
> 
> Hi,
> 
> I found this when trying to use multiple root complexes with OVMF.
> 
> When trying to attach a device to the pxb-pcie device as Integrated
> Device it did not receive the IO/MEM resources.
> 
> The reason is that OVMF is working like that:
>  1. It disables the Decode (I/O or memory) bit in the Command register
>  2. It configures the device BARS
>  3. Makes some tests on the Command register
>  4. ...
>  5. Enables the Decode (I/O or memory) at some point.
> 
> On step 3 all the BARS are overridden to 0x by QEMU.
> 
> Since QEMU uses the device BARs to compute the new host bridge resources
> it now gets garbage.
> 
> Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the 
> pxb.
> Now we can enable the SHPC for it too.

What about migration case?
Shouldn't mappings be updated to match source even if bit isn't set?

> 
> Thanks,
> Marcel
> 
>  hw/pci/pci.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 168b9cc..f9127dc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d)
>  PCIIORegion *r;
>  int i;
>  pcibus_t new_addr;
> +uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
>  
>  for(i = 0; i < PCI_NUM_REGIONS; i++) {
>  r = >io_regions[i];
> @@ -1156,6 +1157,22 @@ static void pci_update_mappings(PCIDevice *d)
>  if (!r->size)
>  continue;
>  
> +/*
> + * Do not update the mappings until the command register's
> + * Decode (I/O or memory) bit is not set. Two reasons:
> + * - PCI Spec indicates that while the bit is not set
> + *   the memory sizing is not finished.
> + * - pci_bar_address will return PCI_BAR_UNMAPPED
> + *   and a previous value can be accidentally overridden
> + *   if the command register is modified (and not the BAR).
> + * */
> +if (((r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> + !(cmd & PCI_COMMAND_IO)) ||
> +((r->type != PCI_BASE_ADDRESS_SPACE_IO) &&
> + !(cmd & PCI_COMMAND_MEMORY))) {
> +continue;
> +}
> +
>  new_addr = pci_bar_address(d, i, r->type, r->size);
>  
>  /* This bar isn't changed */

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Laszlo Ersek 
On 01/11/16 17:34, Marcel Apfelbaum wrote:
> On 01/11/2016 06:11 PM, Laszlo Ersek wrote:
>> On 01/11/16 13:24, Marcel Apfelbaum wrote:
>>> Two reasons:
>>>   - PCI Spec indicates that while the bit is not set
>>> the memory sizing is not finished.
>>>   - pci_bar_address will return PCI_BAR_UNMAPPED
>>> and a previous value can be accidentally overridden
>>> if the command register is modified (and not the BAR).
>>>
>>> Signed-off-by: Marcel Apfelbaum 
>>> ---
>>>
>>> Hi,
>>>
>>> I found this when trying to use multiple root complexes with OVMF.
>>>
>>> When trying to attach a device to the pxb-pcie device as Integrated
>>> Device it did not receive the IO/MEM resources.
>>>
>>> The reason is that OVMF is working like that:
>>>   1. It disables the Decode (I/O or memory) bit in the Command register
>>>   2. It configures the device BARS
>>>   3. Makes some tests on the Command register
>>>   4. ...
>>>   5. Enables the Decode (I/O or memory) at some point.
>>>
>>> On step 3 all the BARS are overridden to 0x by QEMU.
>>>
>>> Since QEMU uses the device BARs to compute the new host bridge resources
>>> it now gets garbage.
>>>
>>> Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
>>> inside the pxb.
>>> Now we can enable the SHPC for it too.
>>
>> I encountered the exact same problem months ago. I posted patches for
>> it; you were CC'd. :)
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210
>>
>> As you can see under the second link above, I made the same analysis &
>> observations as you do now. (It took me quite long to track down the
>> "inexplicable" behavior of edk2's generic PCI bus driver / enumerator
>> that is built into OVMF.)
> 
> Wow, I just re-worked this issue again from 0! I wish I have remembered
> those threads :(
> This was another symptom of the exact problem! And I remembered
> something about
> SHPC, I should have looked at those mail threads again...
> 
>>
>> I proposed to change pci_bar_address() so that it could return, to
>> distinguished callers, the BAR values "under programming", even if the
>> command bits were clear. Then the ACPI generator would utilize this
>> special exception.
>>
>> Michael disagreed; in
>>
>> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242
>>
>> he wrote "[t]his is problematic - disabled BAR values have no meaning
>> according to the PCI spec".
>>
> 
> Yes... because it looked like a hook for our case only,
> the good news is that this patch is based exactly on the fact that
> the BARs have no meaning if the bit is not set.
> 
>> The current  solution to the problem (= we disable the SHPC) was
>> recommended by Michael in that message: "It might be best to add a
>> property to just disable shpc in the bridge so no devices reside
>> directly behind the pxb?"
>>
> 
> I confess I don't exactly understand what the SHPC of the pci-2-pci bridge
> has to do with sibling devices on the pxb's root bus (SHPC is the
> hot-plug controller
> for the devices behind the pci-2-pci bridge).
> 
> The second part I do understand, the pxb design was to not have devices
> directly behind
> the pxb, so maybe he meant that SHPC is the part of the pci-bridge that
> behaves like
> a device in the sense it requires IO/MEM resources.
> 
> Bottom line, your solution for the PXB was just fine :)
> 
> 
>> In comparison, your patch doesn't change pci_bar_address(). Instead, it
>> modifies pci_update_mappings() *not to call* pci_bar_address(), if the
>> respective command bits are clear.
>>
>> I guess that could have about the same effect.
>>
>> If, unlike my patch, yours actually improves QEMU's compliance with the
>> PCI specs, then it's likely a good patch. (And apparently more general
>> than the SHPC-specific solution we have now.)
> 
> 
> Exactly! Why should a pci write to the command  register *delete*
> previously set resources? I am looking at it as a bug.
> 
> And also updating the mappings while the Decoding bit is not enables
> is at least not necessary.
> 
>>
>> I just don't know if it's a good idea to leave any old mappings active
>> while the BARs are being reprogrammed (with the command bits clear).
>>
> 
> First, because the OS can't use the IO/MEM anyway, secondly the guest
> OS/firmware
> is the one that disabled the bit... (in order to program resources)

I have something like the following in mind. Do you think it is a valid
(although contrived) use case?

- guest programs some BAR and uses it (as designed / intended)
- guest disables command bit, modifies BAR location
- guest accesses *old* BAR location

What should a guest *expect* in such a case? Is this invalid guest behavior?

If it is not invalid, then will QEMU comply with the guest's
expectations if your patch is applied? Pre-patch, the guest would likely
access a "hole" in the host bridge MMIO aperture, whereas with your
patch (I

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Marcel Apfelbaum 

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:

On 01/11/16 13:24, Marcel Apfelbaum wrote:

Two reasons:
  - PCI Spec indicates that while the bit is not set
the memory sizing is not finished.
  - pci_bar_address will return PCI_BAR_UNMAPPED
and a previous value can be accidentally overridden
if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
  1. It disables the Decode (I/O or memory) bit in the Command register
  2. It configures the device BARS
  3. Makes some tests on the Command register
  4. ...
  5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the 
pxb.
Now we can enable the SHPC for it too.


I encountered the exact same problem months ago. I posted patches for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

As you can see under the second link above, I made the same analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver / enumerator
that is built into OVMF.)


Wow, I just re-worked this issue again from 0! I wish I have remembered those 
threads :(
This was another symptom of the exact problem! And I remembered something about
SHPC, I should have looked at those mail threads again...



I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

he wrote "[t]his is problematic - disabled BAR values have no meaning
according to the PCI spec".



Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.


The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"



I confess I don't exactly understand what the SHPC of the pci-2-pci bridge
has to do with sibling devices on the pxb's root bus (SHPC is the hot-plug 
controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have devices 
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge that behaves 
like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)



In comparison, your patch doesn't change pci_bar_address(). Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(), if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance with the
PCI specs, then it's likely a good patch. (And apparently more general
than the SHPC-specific solution we have now.)



Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.



I just don't know if it's a good idea to leave any old mappings active
while the BARs are being reprogrammed (with the command bits clear).



First, because the OS can't use the IO/MEM anyway, secondly the guest 
OS/firmware
is the one that disabled the bit... (in order to program resources)


In other words, what guarantees that this change will not regress
anything? (I'm not doubting -- I'm asking; I honestly don't know.)

So I guess I'll defer to Michael on this one.


Michael, do you agree with the above?



In any case, I fully agree with your analysis of OVMF's behavior.


Thanks! I looked for this bug in OVMF for some time now :)
Marcel



Thanks!
Laszlo


Thanks,
Marcel

  hw/pci/pci.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..f9127dc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d)
  PCIIORegion *r;
  int i;
  pcibus_t new_addr;
+uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);

  for(i = 0; i < PCI_NUM_REGIONS; i++) {
  r = >io_regions[i];
@@ -1156,6 +1157,22 @@ static void

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Laszlo Ersek 
On 01/11/16 13:24, Marcel Apfelbaum wrote:
> Two reasons:
>  - PCI Spec indicates that while the bit is not set
>the memory sizing is not finished.
>  - pci_bar_address will return PCI_BAR_UNMAPPED
>and a previous value can be accidentally overridden
>if the command register is modified (and not the BAR).
> 
> Signed-off-by: Marcel Apfelbaum 
> ---
> 
> Hi,
> 
> I found this when trying to use multiple root complexes with OVMF.
> 
> When trying to attach a device to the pxb-pcie device as Integrated
> Device it did not receive the IO/MEM resources.
> 
> The reason is that OVMF is working like that:
>  1. It disables the Decode (I/O or memory) bit in the Command register
>  2. It configures the device BARS
>  3. Makes some tests on the Command register
>  4. ...
>  5. Enables the Decode (I/O or memory) at some point.
> 
> On step 3 all the BARS are overridden to 0x by QEMU.
> 
> Since QEMU uses the device BARs to compute the new host bridge resources
> it now gets garbage.
> 
> Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the 
> pxb.
> Now we can enable the SHPC for it too.

I encountered the exact same problem months ago. I posted patches for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

As you can see under the second link above, I made the same analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver / enumerator
that is built into OVMF.)

I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

he wrote "[t]his is problematic - disabled BAR values have no meaning
according to the PCI spec".

The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"

In comparison, your patch doesn't change pci_bar_address(). Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(), if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance with the
PCI specs, then it's likely a good patch. (And apparently more general
than the SHPC-specific solution we have now.)

I just don't know if it's a good idea to leave any old mappings active
while the BARs are being reprogrammed (with the command bits clear).

In other words, what guarantees that this change will not regress
anything? (I'm not doubting -- I'm asking; I honestly don't know.)

So I guess I'll defer to Michael on this one.

In any case, I fully agree with your analysis of OVMF's behavior.

Thanks!
Laszlo

> Thanks,
> Marcel
> 
>  hw/pci/pci.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 168b9cc..f9127dc 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d)
>  PCIIORegion *r;
>  int i;
>  pcibus_t new_addr;
> +uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
>  
>  for(i = 0; i < PCI_NUM_REGIONS; i++) {
>  r = >io_regions[i];
> @@ -1156,6 +1157,22 @@ static void pci_update_mappings(PCIDevice *d)
>  if (!r->size)
>  continue;
>  
> +/*
> + * Do not update the mappings until the command register's
> + * Decode (I/O or memory) bit is not set. Two reasons:
> + * - PCI Spec indicates that while the bit is not set
> + *   the memory sizing is not finished.
> + * - pci_bar_address will return PCI_BAR_UNMAPPED
> + *   and a previous value can be accidentally overridden
> + *   if the command register is modified (and not the BAR).
> + * */
> +if (((r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> + !(cmd & PCI_COMMAND_IO)) ||
> +((r->type != PCI_BASE_ADDRESS_SPACE_IO) &&
> + !(cmd & PCI_COMMAND_MEMORY))) {
> +continue;
> +}
> +
>  new_addr = pci_bar_address(d, i, r->type, r->size);
>  
>  /* This bar isn't changed */
>

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Marcel Apfelbaum 

On 01/11/2016 04:07 PM, Igor Mammedov wrote:

On Mon, 11 Jan 2016 14:24:23 +0200
Marcel Apfelbaum  wrote:


Two reasons:
  - PCI Spec indicates that while the bit is not set
the memory sizing is not finished.
  - pci_bar_address will return PCI_BAR_UNMAPPED
and a previous value can be accidentally overridden
if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
  1. It disables the Decode (I/O or memory) bit in the Command register
  2. It configures the device BARS
  3. Makes some tests on the Command register
  4. ...
  5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the 
pxb.
Now we can enable the SHPC for it too.




Hi Igor,
Thanks for the review.


What about migration case?
Shouldn't mappings be updated to match source even if bit isn't set?


I don't know much about migration, but I think it would be just OK
because the regions itself are migrated (I think...) and the migration
target can't use the IO or MEM (until the bit will be set), so the mappings 
don't matter.
Once set, the mappings would be updated on the target.

Another thing, in the source the mappings are not updated either until the bit 
is is set.

Thanks,
Marcel






Thanks,
Marcel

  hw/pci/pci.c | 17 +
  1 file changed, 17 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 168b9cc..f9127dc 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d)
  PCIIORegion *r;
  int i;
  pcibus_t new_addr;
+uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);

  for(i = 0; i < PCI_NUM_REGIONS; i++) {
  r = >io_regions[i];
@@ -1156,6 +1157,22 @@ static void pci_update_mappings(PCIDevice *d)
  if (!r->size)
  continue;

+/*
+ * Do not update the mappings until the command register's
+ * Decode (I/O or memory) bit is not set. Two reasons:
+ * - PCI Spec indicates that while the bit is not set
+ *   the memory sizing is not finished.
+ * - pci_bar_address will return PCI_BAR_UNMAPPED
+ *   and a previous value can be accidentally overridden
+ *   if the command register is modified (and not the BAR).
+ * */
+if (((r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
+ !(cmd & PCI_COMMAND_IO)) ||
+((r->type != PCI_BASE_ADDRESS_SPACE_IO) &&
+ !(cmd & PCI_COMMAND_MEMORY))) {
+continue;
+}
+
  new_addr = pci_bar_address(d, i, r->type, r->size);

  /* This bar isn't changed */

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Marcel Apfelbaum 

On 01/11/2016 08:44 PM, Laszlo Ersek wrote:

On 01/11/16 19:01, Marcel Apfelbaum wrote:

On 01/11/2016 07:15 PM, Laszlo Ersek wrote:

On 01/11/16 17:34, Marcel Apfelbaum wrote:

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:

On 01/11/16 13:24, Marcel Apfelbaum wrote:

Two reasons:
- PCI Spec indicates that while the bit is not set
  the memory sizing is not finished.
- pci_bar_address will return PCI_BAR_UNMAPPED
  and a previous value can be accidentally overridden
  if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
1. It disables the Decode (I/O or memory) bit in the Command
register
2. It configures the device BARS
3. Makes some tests on the Command register
4. ...
5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge
resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
inside the pxb.
Now we can enable the SHPC for it too.


I encountered the exact same problem months ago. I posted patches for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

As you can see under the second link above, I made the same analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver / enumerator
that is built into OVMF.)


Wow, I just re-worked this issue again from 0! I wish I have remembered
those threads :(
This was another symptom of the exact problem! And I remembered
something about
SHPC, I should have looked at those mail threads again...



I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

he wrote "[t]his is problematic - disabled BAR values have no meaning
according to the PCI spec".



Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.


The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"



I confess I don't exactly understand what the SHPC of the pci-2-pci
bridge
has to do with sibling devices on the pxb's root bus (SHPC is the
hot-plug controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have devices
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge that
behaves like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)



In comparison, your patch doesn't change pci_bar_address(). Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(), if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance with the
PCI specs, then it's likely a good patch. (And apparently more general
than the SHPC-specific solution we have now.)



Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.



I just don't know if it's a good idea to leave any old mappings active
while the BARs are being reprogrammed (with the command bits clear).



First, because the OS can't use the IO/MEM anyway, secondly the guest
OS/firmware
is the one that disabled the bit... (in order to program resources)


I have something like the following in mind. Do you think it is a valid
(although contrived) use case?

- guest programs some BAR and uses it (as designed / intended)
- guest disables command bit, modifies BAR location
- guest accesses *old* BAR location

What should a guest *expect* in such a case? Is this invalid guest
behavior?


Yes, this is indeed invalid behavior, from the device point of view
it is disabled. Best case scenario - the guest will see 0x,
worst case - garbage.



If it is not invalid, then will QEMU comply with the guest's
expectations if your patch is applied? Pre-patch, the guest would likely
access a "hole" in the host bridge MMIO aperture, whereas with your

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Marcel Apfelbaum 

On 01/11/2016 07:15 PM, Laszlo Ersek wrote:

On 01/11/16 17:34, Marcel Apfelbaum wrote:

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:

On 01/11/16 13:24, Marcel Apfelbaum wrote:

Two reasons:
   - PCI Spec indicates that while the bit is not set
 the memory sizing is not finished.
   - pci_bar_address will return PCI_BAR_UNMAPPED
 and a previous value can be accidentally overridden
 if the command register is modified (and not the BAR).

Signed-off-by: Marcel Apfelbaum 
---

Hi,

I found this when trying to use multiple root complexes with OVMF.

When trying to attach a device to the pxb-pcie device as Integrated
Device it did not receive the IO/MEM resources.

The reason is that OVMF is working like that:
   1. It disables the Decode (I/O or memory) bit in the Command register
   2. It configures the device BARS
   3. Makes some tests on the Command register
   4. ...
   5. Enables the Decode (I/O or memory) at some point.

On step 3 all the BARS are overridden to 0x by QEMU.

Since QEMU uses the device BARs to compute the new host bridge resources
it now gets garbage.

Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
inside the pxb.
Now we can enable the SHPC for it too.


I encountered the exact same problem months ago. I posted patches for
it; you were CC'd. :)

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

As you can see under the second link above, I made the same analysis &
observations as you do now. (It took me quite long to track down the
"inexplicable" behavior of edk2's generic PCI bus driver / enumerator
that is built into OVMF.)


Wow, I just re-worked this issue again from 0! I wish I have remembered
those threads :(
This was another symptom of the exact problem! And I remembered
something about
SHPC, I should have looked at those mail threads again...



I proposed to change pci_bar_address() so that it could return, to
distinguished callers, the BAR values "under programming", even if the
command bits were clear. Then the ACPI generator would utilize this
special exception.

Michael disagreed; in

http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

he wrote "[t]his is problematic - disabled BAR values have no meaning
according to the PCI spec".



Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.


The current  solution to the problem (= we disable the SHPC) was
recommended by Michael in that message: "It might be best to add a
property to just disable shpc in the bridge so no devices reside
directly behind the pxb?"



I confess I don't exactly understand what the SHPC of the pci-2-pci bridge
has to do with sibling devices on the pxb's root bus (SHPC is the
hot-plug controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have devices
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge that
behaves like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)



In comparison, your patch doesn't change pci_bar_address(). Instead, it
modifies pci_update_mappings() *not to call* pci_bar_address(), if the
respective command bits are clear.

I guess that could have about the same effect.

If, unlike my patch, yours actually improves QEMU's compliance with the
PCI specs, then it's likely a good patch. (And apparently more general
than the SHPC-specific solution we have now.)



Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.



I just don't know if it's a good idea to leave any old mappings active
while the BARs are being reprogrammed (with the command bits clear).



First, because the OS can't use the IO/MEM anyway, secondly the guest
OS/firmware
is the one that disabled the bit... (in order to program resources)


I have something like the following in mind. Do you think it is a valid
(although contrived) use case?

- guest programs some BAR and uses it (as designed / intended)
- guest disables command bit, modifies BAR location
- guest accesses *old* BAR location

What should a guest *expect* in such a case? Is this invalid guest behavior?


Yes, this is indeed invalid behavior, from the device point of view
it is disabled. Best case scenario - the guest will see 0x,
worst case - garbage.



If it is not invalid, then will QEMU comply with the guest's
expectations if your patch is applied? Pre-patch, the guest would likely
access a "hole" in the host bridge MMIO aperture, whereas with your
patch (I guess?) it still might access the device through the old (still
active) BAR?



Since the

Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

2016-01-11 Thread Laszlo Ersek 
On 01/11/16 19:01, Marcel Apfelbaum wrote:
> On 01/11/2016 07:15 PM, Laszlo Ersek wrote:
>> On 01/11/16 17:34, Marcel Apfelbaum wrote:
>>> On 01/11/2016 06:11 PM, Laszlo Ersek wrote:
 On 01/11/16 13:24, Marcel Apfelbaum wrote:
> Two reasons:
>- PCI Spec indicates that while the bit is not set
>  the memory sizing is not finished.
>- pci_bar_address will return PCI_BAR_UNMAPPED
>  and a previous value can be accidentally overridden
>  if the command register is modified (and not the BAR).
>
> Signed-off-by: Marcel Apfelbaum 
> ---
>
> Hi,
>
> I found this when trying to use multiple root complexes with OVMF.
>
> When trying to attach a device to the pxb-pcie device as Integrated
> Device it did not receive the IO/MEM resources.
>
> The reason is that OVMF is working like that:
>1. It disables the Decode (I/O or memory) bit in the Command
> register
>2. It configures the device BARS
>3. Makes some tests on the Command register
>4. ...
>5. Enables the Decode (I/O or memory) at some point.
>
> On step 3 all the BARS are overridden to 0x by QEMU.
>
> Since QEMU uses the device BARs to compute the new host bridge
> resources
> it now gets garbage.
>
> Laszlo, this also solves the SHPC problem for the pci-2-pci bridge
> inside the pxb.
> Now we can enable the SHPC for it too.

 I encountered the exact same problem months ago. I posted patches for
 it; you were CC'd. :)

 http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
 http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210

 As you can see under the second link above, I made the same analysis &
 observations as you do now. (It took me quite long to track down the
 "inexplicable" behavior of edk2's generic PCI bus driver / enumerator
 that is built into OVMF.)
>>>
>>> Wow, I just re-worked this issue again from 0! I wish I have remembered
>>> those threads :(
>>> This was another symptom of the exact problem! And I remembered
>>> something about
>>> SHPC, I should have looked at those mail threads again...
>>>

 I proposed to change pci_bar_address() so that it could return, to
 distinguished callers, the BAR values "under programming", even if the
 command bits were clear. Then the ACPI generator would utilize this
 special exception.

 Michael disagreed; in

 http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242

 he wrote "[t]his is problematic - disabled BAR values have no meaning
 according to the PCI spec".

>>>
>>> Yes... because it looked like a hook for our case only,
>>> the good news is that this patch is based exactly on the fact that
>>> the BARs have no meaning if the bit is not set.
>>>
 The current  solution to the problem (= we disable the SHPC) was
 recommended by Michael in that message: "It might be best to add a
 property to just disable shpc in the bridge so no devices reside
 directly behind the pxb?"

>>>
>>> I confess I don't exactly understand what the SHPC of the pci-2-pci
>>> bridge
>>> has to do with sibling devices on the pxb's root bus (SHPC is the
>>> hot-plug controller
>>> for the devices behind the pci-2-pci bridge).
>>>
>>> The second part I do understand, the pxb design was to not have devices
>>> directly behind
>>> the pxb, so maybe he meant that SHPC is the part of the pci-bridge that
>>> behaves like
>>> a device in the sense it requires IO/MEM resources.
>>>
>>> Bottom line, your solution for the PXB was just fine :)
>>>
>>>
 In comparison, your patch doesn't change pci_bar_address(). Instead, it
 modifies pci_update_mappings() *not to call* pci_bar_address(), if the
 respective command bits are clear.

 I guess that could have about the same effect.

 If, unlike my patch, yours actually improves QEMU's compliance with the
 PCI specs, then it's likely a good patch. (And apparently more general
 than the SHPC-specific solution we have now.)
>>>
>>>
>>> Exactly! Why should a pci write to the command  register *delete*
>>> previously set resources? I am looking at it as a bug.
>>>
>>> And also updating the mappings while the Decoding bit is not enables
>>> is at least not necessary.
>>>

 I just don't know if it's a good idea to leave any old mappings active
 while the BARs are being reprogrammed (with the command bits clear).

>>>
>>> First, because the OS can't use the IO/MEM anyway, secondly the guest
>>> OS/firmware
>>> is the one that disabled the bit... (in order to program resources)
>>
>> I have something like the following in mind. Do you think it is a valid
>> (although contrived) use case?
>>
>> - guest programs some BAR and uses it (as designed / intended)
>> - guest disables command bit,