| Message ID | 1444683308-30543-1-git-send-email-agordeev@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 12, 2015 at 10:03:03PM +0100, Peter Maydell wrote: > On 12 October 2015 at 21:55, Alexander Gordeev <agordeev@redhat.com> wrote: > > Currently PCI IO address 0 is not allowed even though > > the IO space starts from 0. As result, PCI IO is not > > possible to use at all. > > I don't see any reason for us not to allow 0 IO addresses, > but I'm not sure how your your conclusion follows. It > should be entirely possible to map PCI IO to some other > address than zero in the IO window, which is what I would > have expected the guest to do. You are right - my changelog is incorrect. The rest of IO space should be alright. However, as 0 IO address is exposed via "ranges" to the guest, it must be usable - isn't it? So it either should be allowed or the range should be different. Thanks! > thanks > -- PMM
On 13 October 2015 at 13:48, Alexander Gordeev <agordeev@redhat.com> wrote: > On Tue, Oct 13, 2015 at 09:16:34AM +0100, Peter Maydell wrote: >> In any case, setting pci_allow_0_address is the right thing, >> so we can just change the commit message in this patch. > > I will post v2 with an updated changelog then. > >> Incidentally, why is this a property on the machine >> and not on the PCI controller device? > > I am CC-ing Laurent Vivier who introduced the flag. > > But IMO it *is* a machine property, not PCI controller's > one, unless I am missing something. I think "does this PCI controller handle BARs with zero addresses, or does it treat them as if the BAR was unmapped" is definitely a controller property... you might have in theory a machine with two PCI controllers, one of which could deal with zero-addresses and one of which could not. thanks -- PMM
On 13/10/2015 14:33, Peter Maydell wrote: > On 13 October 2015 at 13:48, Alexander Gordeev <agordeev@redhat.com> wrote: >> On Tue, Oct 13, 2015 at 09:16:34AM +0100, Peter Maydell wrote: >>> In any case, setting pci_allow_0_address is the right thing, >>> so we can just change the commit message in this patch. >> >> I will post v2 with an updated changelog then. >> >>> Incidentally, why is this a property on the machine >>> and not on the PCI controller device? >> >> I am CC-ing Laurent Vivier who introduced the flag. >> >> But IMO it *is* a machine property, not PCI controller's >> one, unless I am missing something. > > I think "does this PCI controller handle BARs with > zero addresses, or does it treat them as if the BAR > was unmapped" is definitely a controller property... > you might have in theory a machine with two PCI > controllers, one of which could deal with zero-addresses > and one of which could not. MST asked for a global flag: https://lists.gnu.org/archive/html/qemu-ppc/2015-07/msg00364.html But perhaps the machine is not the good place for this global flag ? CC: Michael for that. > thanks > -- PMM >
On Tue, Oct 13, 2015 at 09:16:34AM +0100, Peter Maydell wrote: > On 13 October 2015 at 07:31, Alexander Gordeev <agordeev@redhat.com> wrote: > > On Mon, Oct 12, 2015 at 10:03:03PM +0100, Peter Maydell wrote: > >> On 12 October 2015 at 21:55, Alexander Gordeev <agordeev@redhat.com> wrote: > >> > Currently PCI IO address 0 is not allowed even though > >> > the IO space starts from 0. As result, PCI IO is not > >> > possible to use at all. > >> > >> I don't see any reason for us not to allow 0 IO addresses, > >> but I'm not sure how your your conclusion follows. It > >> should be entirely possible to map PCI IO to some other > >> address than zero in the IO window, which is what I would > >> have expected the guest to do. > > > > You are right - my changelog is incorrect. The rest of IO > > space should be alright. > > > > However, as 0 IO address is exposed via "ranges" to the guest, > > it must be usable - isn't it? So it either should be allowed > > or the range should be different. > > Depends on your point of view. If you take the Linus Torvalds > view that 0 is always an invalid PCI address, then you'll > never try to use it anyway. > > In any case, setting pci_allow_0_address is the right thing, > so we can just change the commit message in this patch. I will post v2 with an updated changelog then. > Incidentally, why is this a property on the machine > and not on the PCI controller device? I am CC-ing Laurent Vivier who introduced the flag. But IMO it *is* a machine property, not PCI controller's one, unless I am missing something. Thanks! > thanks > -- PMM
On Tue, Oct 13, 2015 at 02:47:33PM +0200, Laurent Vivier wrote: > > > On 13/10/2015 14:33, Peter Maydell wrote: > > On 13 October 2015 at 13:48, Alexander Gordeev <agordeev@redhat.com> wrote: > >> On Tue, Oct 13, 2015 at 09:16:34AM +0100, Peter Maydell wrote: > >>> In any case, setting pci_allow_0_address is the right thing, > >>> so we can just change the commit message in this patch. > >> > >> I will post v2 with an updated changelog then. > >> > >>> Incidentally, why is this a property on the machine > >>> and not on the PCI controller device? > >> > >> I am CC-ing Laurent Vivier who introduced the flag. > >> > >> But IMO it *is* a machine property, not PCI controller's > >> one, unless I am missing something. > > > > I think "does this PCI controller handle BARs with > > zero addresses, or does it treat them as if the BAR > > was unmapped" is definitely a controller property... > > you might have in theory a machine with two PCI > > controllers, one of which could deal with zero-addresses > > and one of which could not. > > MST asked for a global flag: > > https://lists.gnu.org/archive/html/qemu-ppc/2015-07/msg00364.html > > But perhaps the machine is not the good place for this global flag ? > > CC: Michael for that. Whether controllers treat zero specially should be expressed using priorities in memory APIs. This is just about buggy machine types that do not specify priority for overlapping regions correctly. As a temporary hacky work-around, a global seems sufficient. > > thanks > > -- PMM > >
On 13 October 2015 at 14:04, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Oct 13, 2015 at 02:47:33PM +0200, Laurent Vivier wrote: >> MST asked for a global flag: >> >> https://lists.gnu.org/archive/html/qemu-ppc/2015-07/msg00364.html >> >> But perhaps the machine is not the good place for this global flag ? >> >> CC: Michael for that. > > Whether controllers treat zero specially should be expressed > using priorities in memory APIs. Mmm, I guess it's a bug in how the machine model wires up the controller rather than in the controller model itself. > This is just about buggy machine types that do > not specify priority for overlapping regions correctly. > > As a temporary hacky work-around, a global seems sufficient. Well, if we're going to go around and fix the machines which don't get things right, I guess. It's a shame the default for the global is "this machine is broken", because now every new machine will default unnecessarily to broken, and there's no way to grep the source tree for machines which need fixing. -- PMM
On Tue, Oct 13, 2015 at 02:12:34PM +0100, Peter Maydell wrote: > On 13 October 2015 at 14:04, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Oct 13, 2015 at 02:47:33PM +0200, Laurent Vivier wrote: > >> MST asked for a global flag: > >> > >> https://lists.gnu.org/archive/html/qemu-ppc/2015-07/msg00364.html > >> > >> But perhaps the machine is not the good place for this global flag ? > >> > >> CC: Michael for that. > > > > Whether controllers treat zero specially should be expressed > > using priorities in memory APIs. > > Mmm, I guess it's a bug in how the machine model > wires up the controller rather than in the controller > model itself. > > > This is just about buggy machine types that do > > not specify priority for overlapping regions correctly. > > > > As a temporary hacky work-around, a global seems sufficient. > > Well, if we're going to go around and fix the machines which > don't get things right, I guess. It's a shame the default for > the global is "this machine is broken", because now every > new machine will default unnecessarily to broken, and there's > no way to grep the source tree for machines which need fixing. > > -- PMM It'd be easy enough to revert the logic if someone's willing to start on this. I'm reluctant to make this patchset depend on changing all machines, but if you think I'm wrong, pls let me know.
On 13 October 2015 at 14:19, Michael S. Tsirkin <mst@redhat.com> wrote: > On Tue, Oct 13, 2015 at 02:12:34PM +0100, Peter Maydell wrote: >> Well, if we're going to go around and fix the machines which >> don't get things right, I guess. It's a shame the default for >> the global is "this machine is broken", because now every >> new machine will default unnecessarily to broken, and there's >> no way to grep the source tree for machines which need fixing. > It'd be easy enough to revert the logic if someone's willing to start on > this. I'm reluctant to make this patchset depend on changing all > machines, but if you think I'm wrong, pls let me know. Most machines don't have a PCI controller, luckily. I'll have a look at how many files it would touch... -- PMM
On 13 October 2015 at 14:25, Peter Maydell <peter.maydell@linaro.org> wrote: > On 13 October 2015 at 14:19, Michael S. Tsirkin <mst@redhat.com> wrote: >> On Tue, Oct 13, 2015 at 02:12:34PM +0100, Peter Maydell wrote: >>> Well, if we're going to go around and fix the machines which >>> don't get things right, I guess. It's a shame the default for >>> the global is "this machine is broken", because now every >>> new machine will default unnecessarily to broken, and there's >>> no way to grep the source tree for machines which need fixing. > >> It'd be easy enough to revert the logic if someone's willing to start on >> this. I'm reluctant to make this patchset depend on changing all >> machines, but if you think I'm wrong, pls let me know. > > Most machines don't have a PCI controller, luckily. > I'll have a look at how many files it would touch... So, first up I'm happy that the gpex and versatile pci controllers don't have this problem (the way they set up the mmio and io windows means there won't be overlaps). That leaves the following pci controllers, which I've listed with the source files which define machines that use them: hw/pci-host/pbm hw/sparc64/sun4u.c hw/pci-host/bonito hw/mips/mips_fulong2e.c hw/pci-host/grackle hw/pc/mac_oldworld.c hw/pci-host/piix [the x86 systems] hw/pci-host/ppce500 hw/ppc/e500.c hw/pci-host/prep hw/ppc/prep.c hw/pci-host/q35 [x86 systems] hw/pci-host/uninorth hw/ppc/mac_newworld.c hw/alpha/typhoon used in hw/alpha/dp264.c hw/mips/gt64xxx_pci hw/mips/mips_malta.c hw/ppc/ppc4xx_pci hw/ppc/ppc440_bamboo.c hw/ppc/spapr_pci hw/ppc/spapr.c [already marked as '0 addrs ok'] hw/s390/s390-pci-bus hw/s390/s390-virtio-ccw.c hw/sh4/sh_pci hw/sh4/rd2.c So we'd need to touch perhaps fifteen files in total. I don't insist we do that rather than applying this particular patch, but I don't think it's a huge effort. thanks -- PMM
On 13/10/2015 15:55, Peter Maydell wrote: > On 13 October 2015 at 14:25, Peter Maydell <peter.maydell@linaro.org> wrote: >> On 13 October 2015 at 14:19, Michael S. Tsirkin <mst@redhat.com> wrote: >>> On Tue, Oct 13, 2015 at 02:12:34PM +0100, Peter Maydell wrote: >>>> Well, if we're going to go around and fix the machines which >>>> don't get things right, I guess. It's a shame the default for >>>> the global is "this machine is broken", because now every >>>> new machine will default unnecessarily to broken, and there's >>>> no way to grep the source tree for machines which need fixing. >> >>> It'd be easy enough to revert the logic if someone's willing to start on >>> this. I'm reluctant to make this patchset depend on changing all >>> machines, but if you think I'm wrong, pls let me know. >> >> Most machines don't have a PCI controller, luckily. >> I'll have a look at how many files it would touch... > > So, first up I'm happy that the gpex and versatile > pci controllers don't have this problem (the way they > set up the mmio and io windows means there won't be > overlaps). That leaves the following pci controllers, > which I've listed with the source files which define machines > that use them: > > hw/pci-host/pbm > hw/sparc64/sun4u.c > hw/pci-host/bonito > hw/mips/mips_fulong2e.c > hw/pci-host/grackle > hw/pc/mac_oldworld.c > hw/pci-host/piix > [the x86 systems] > hw/pci-host/ppce500 > hw/ppc/e500.c > hw/pci-host/prep > hw/ppc/prep.c > hw/pci-host/q35 > [x86 systems] > hw/pci-host/uninorth > hw/ppc/mac_newworld.c > hw/alpha/typhoon > used in hw/alpha/dp264.c > hw/mips/gt64xxx_pci > hw/mips/mips_malta.c > hw/ppc/ppc4xx_pci > hw/ppc/ppc440_bamboo.c > hw/ppc/spapr_pci > hw/ppc/spapr.c [already marked as '0 addrs ok'] > hw/s390/s390-pci-bus > hw/s390/s390-virtio-ccw.c > hw/sh4/sh_pci > hw/sh4/rd2.c > > So we'd need to touch perhaps fifteen files in total. > > I don't insist we do that rather than applying this particular > patch, but I don't think it's a huge effort. I'm going to remove the zero address checking and try to start a qemu for each of them to see which ones are broken. I remember from a previous discussion on my initial patch that x86 should be ok. > thanks > -- PMM > Laurent
On Fri, Oct 16, 2015 at 10:52:00AM +0200, Laurent Vivier wrote: > > > On 13/10/2015 15:55, Peter Maydell wrote: > > On 13 October 2015 at 14:25, Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 13 October 2015 at 14:19, Michael S. Tsirkin <mst@redhat.com> wrote: > >>> On Tue, Oct 13, 2015 at 02:12:34PM +0100, Peter Maydell wrote: > >>>> Well, if we're going to go around and fix the machines which > >>>> don't get things right, I guess. It's a shame the default for > >>>> the global is "this machine is broken", because now every > >>>> new machine will default unnecessarily to broken, and there's > >>>> no way to grep the source tree for machines which need fixing. > >> > >>> It'd be easy enough to revert the logic if someone's willing to start on > >>> this. I'm reluctant to make this patchset depend on changing all > >>> machines, but if you think I'm wrong, pls let me know. > >> > >> Most machines don't have a PCI controller, luckily. > >> I'll have a look at how many files it would touch... > > > > So, first up I'm happy that the gpex and versatile > > pci controllers don't have this problem (the way they > > set up the mmio and io windows means there won't be > > overlaps). That leaves the following pci controllers, > > which I've listed with the source files which define machines > > that use them: > > > > hw/pci-host/pbm > > hw/sparc64/sun4u.c > > hw/pci-host/bonito > > hw/mips/mips_fulong2e.c > > hw/pci-host/grackle > > hw/pc/mac_oldworld.c > > hw/pci-host/piix > > [the x86 systems] > > hw/pci-host/ppce500 > > hw/ppc/e500.c > > hw/pci-host/prep > > hw/ppc/prep.c > > hw/pci-host/q35 > > [x86 systems] > > hw/pci-host/uninorth > > hw/ppc/mac_newworld.c > > hw/alpha/typhoon > > used in hw/alpha/dp264.c > > hw/mips/gt64xxx_pci > > hw/mips/mips_malta.c > > hw/ppc/ppc4xx_pci > > hw/ppc/ppc440_bamboo.c > > hw/ppc/spapr_pci > > hw/ppc/spapr.c [already marked as '0 addrs ok'] > > hw/s390/s390-pci-bus > > hw/s390/s390-virtio-ccw.c > > hw/sh4/sh_pci > > hw/sh4/rd2.c > > > > So we'd need to touch perhaps fifteen files in total. > > > > I don't insist we do that rather than applying this particular > > patch, but I don't think it's a huge effort. > > I'm going to remove the zero address checking and try to start a qemu > for each of them to see which ones are broken. Can't hurt, but I'm not sure that will be enough, as overlaps might depend on what does the guest do. It's a question of looking at how are mmio and io windows for pci set up. > I remember from a previous discussion on my initial patch that x86 > should be ok. Yes because on x86 PCI has priority -1 - lower than all other system devices. That was handled by this patch: commit 83d08f2673504a299194dcac1657a13754b5932a Author: Michael S. Tsirkin <mst@redhat.com> Date: Tue Oct 29 13:57:34 2013 +0100 pc: map PCI address space as catchall region for not mapped addresses > > thanks > > -- PMM > > > Laurent
On 16 October 2015 at 09:52, Laurent Vivier <lvivier@redhat.com> wrote: > I'm going to remove the zero address checking and try to start a qemu > for each of them to see which ones are broken. You need to also make sure there's a PCI card in there and that the guest maps it with a BAR at address zero (testing both MMIO BARs and IO BARs), and then exercise the guest sufficiently to check that behaviour of whatever it might be overlapping is still OK. You might find it easier to do by code inspection in some cases (boards which implement PCI as a simple "memory window into the PCI space" rather than with x86-style "anything in the background not covered by another device is PCI space" should be ok). thanks -- PMM
On 16/10/2015 11:32, Peter Maydell wrote: > On 16 October 2015 at 09:52, Laurent Vivier <lvivier@redhat.com> wrote: >> I'm going to remove the zero address checking and try to start a qemu >> for each of them to see which ones are broken. > > You need to also make sure there's a PCI card in there > and that the guest maps it with a BAR at address zero > (testing both MMIO BARs and IO BARs), and then exercise > the guest sufficiently to check that behaviour of whatever > it might be overlapping is still OK. I think it will be hard to trigger this case. :( For instance, on pseries, the card is put at address 0 only when it is hotplugged, and on reboot it is moved to another address. > You might find it easier to do by code inspection in > some cases (boards which implement PCI as a simple > "memory window into the PCI space" rather than with > x86-style "anything in the background not covered by > another device is PCI space" should be ok). OK, I will... Laurent
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d25d6cf..3620489 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1157,6 +1157,7 @@ static void virt_class_init(ObjectClass *oc, void *data) mc->has_dynamic_sysbus = true; mc->block_default_type = IF_VIRTIO; mc->no_cdrom = 1; + mc->pci_allow_0_address = true; } static const TypeInfo machvirt_info = {
Currently PCI IO address 0 is not allowed even though the IO space starts from 0. As result, PCI IO is not possible to use at all. CC: Peter Maydell <peter.maydell@linaro.org> CC: Andrew Jones <drjones@redhat.com> Signed-off-by: Alexander Gordeev <agordeev@redhat.com> --- hw/arm/virt.c | 1 + 1 file changed, 1 insertion(+)