* [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion @ 2014-02-06 10:24 Igor Mammedov 2014-02-06 12:22 ` Michael S. Tsirkin 2014-02-16 16:40 ` Michael S. Tsirkin 0 siblings, 2 replies; 7+ messages in thread From: Igor Mammedov @ 2014-02-06 10:24 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, mst, jan.kiszka, anthony, pbonzini, rth Windows XP shows COM2 port as non functional in "Device Manager" although no COM2 port backing device is present in QEMU. That is caused by the fact that QEMU reports to OSPM that device is present by setting 5th bit in PII4XPM.pci_conf[0x67] register when COM2 doesn't exist. It happens due to memory_region_present(io_as, 0x2f8) returning false positive since 0x2f8 address eventually translates into catchall io_as address space. Fix memory_region_present(parent, addr) by returning true only if addr maps into a MemoryRegion within parent (excluding parent itself), to match its doc comment. While at it fix copy/paste error in memory_region_present() doc comment. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- include/exec/memory.h | 6 +++--- memory.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/exec/memory.h b/include/exec/memory.h index 296d6ab..a5eb4c8 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, hwaddr offset); /** - * memory_region_present: translate an address/size relative to a - * MemoryRegion into a #MemoryRegionSection. + * memory_region_present: checks if an address relative to a @parent + * translates into #MemoryRegion within @parent * * Answer whether a #MemoryRegion within @parent covers the address * @addr. * - * @parent: a MemoryRegion within which @addr is a relative address + * @parent: a #MemoryRegion within which @addr is a relative address * @addr: the area within @parent to be searched */ bool memory_region_present(MemoryRegion *parent, hwaddr addr); diff --git a/memory.c b/memory.c index 59ecc28..3f1df23 100644 --- a/memory.c +++ b/memory.c @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) bool memory_region_present(MemoryRegion *parent, hwaddr addr) { MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; - if (!mr) { + if (!mr || (mr == parent)) { return false; } memory_region_unref(mr); -- 1.7.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 10:24 [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion Igor Mammedov @ 2014-02-06 12:22 ` Michael S. Tsirkin 2014-02-06 12:32 ` Igor Mammedov 2014-02-16 16:40 ` Michael S. Tsirkin 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2014-02-06 12:22 UTC (permalink / raw) To: Igor Mammedov Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, pbonzini, rth On Thu, Feb 06, 2014 at 11:24:33AM +0100, Igor Mammedov wrote: > Windows XP shows COM2 port as non functional in > "Device Manager" although no COM2 port backing device > is present in QEMU. > > That is caused by the fact that QEMU reports to > OSPM that device is present by setting 5th bit in > PII4XPM.pci_conf[0x67] register when COM2 doesn't > exist. > > It happens due to memory_region_present(io_as, 0x2f8) > returning false positive since 0x2f8 address eventually > translates into catchall io_as address space. > > Fix memory_region_present(parent, addr) by returning > true only if addr maps into a MemoryRegion within > parent (excluding parent itself), to match its > doc comment. > > While at it fix copy/paste error in > memory_region_present() doc comment. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Hmm, I wonder whether this still should return true if parent is not a pure container. Also what if MR isn't under parent at all? Maybe the right thing to do is return mr->ops == &unassigned_mem_ops ? > --- > include/exec/memory.h | 6 +++--- > memory.c | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 296d6ab..a5eb4c8 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, > hwaddr offset); > > /** > - * memory_region_present: translate an address/size relative to a > - * MemoryRegion into a #MemoryRegionSection. > + * memory_region_present: checks if an address relative to a @parent > + * translates into #MemoryRegion within @parent > * > * Answer whether a #MemoryRegion within @parent covers the address > * @addr. > * > - * @parent: a MemoryRegion within which @addr is a relative address > + * @parent: a #MemoryRegion within which @addr is a relative address > * @addr: the area within @parent to be searched > */ > bool memory_region_present(MemoryRegion *parent, hwaddr addr); > diff --git a/memory.c b/memory.c > index 59ecc28..3f1df23 100644 > --- a/memory.c > +++ b/memory.c > @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) > bool memory_region_present(MemoryRegion *parent, hwaddr addr) > { > MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; > - if (!mr) { > + if (!mr || (mr == parent)) { > return false; > } > memory_region_unref(mr); > -- > 1.7.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 12:22 ` Michael S. Tsirkin @ 2014-02-06 12:32 ` Igor Mammedov 2014-02-06 13:21 ` Michael S. Tsirkin 2014-02-07 10:00 ` Paolo Bonzini 0 siblings, 2 replies; 7+ messages in thread From: Igor Mammedov @ 2014-02-06 12:32 UTC (permalink / raw) To: Michael S. Tsirkin Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, pbonzini, rth On Thu, 6 Feb 2014 14:22:54 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Feb 06, 2014 at 11:24:33AM +0100, Igor Mammedov wrote: > > Windows XP shows COM2 port as non functional in > > "Device Manager" although no COM2 port backing device > > is present in QEMU. > > > > That is caused by the fact that QEMU reports to > > OSPM that device is present by setting 5th bit in > > PII4XPM.pci_conf[0x67] register when COM2 doesn't > > exist. > > > > It happens due to memory_region_present(io_as, 0x2f8) > > returning false positive since 0x2f8 address eventually > > translates into catchall io_as address space. > > > > Fix memory_region_present(parent, addr) by returning > > true only if addr maps into a MemoryRegion within > > parent (excluding parent itself), to match its > > doc comment. > > > > While at it fix copy/paste error in > > memory_region_present() doc comment. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Hmm, I wonder whether this still should return true > if parent is not a pure container. io_as in not a pure container, that's why memory_region_find() finds it at all. This patch only fixes function to behave as it was originally documented. I guess we need to wait on Paolo's opinion, as an author if original code. > Also what if MR isn't under parent at all? that function was never meant to handle this case. > > Maybe the right thing to do is > return mr->ops == &unassigned_mem_ops ? it could be but it looks more like a hack. i.e this exit conditions could pile up over time. > > > > --- > > include/exec/memory.h | 6 +++--- > > memory.c | 2 +- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 296d6ab..a5eb4c8 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, > > hwaddr offset); > > > > /** > > - * memory_region_present: translate an address/size relative to a > > - * MemoryRegion into a #MemoryRegionSection. > > + * memory_region_present: checks if an address relative to a @parent > > + * translates into #MemoryRegion within @parent > > * > > * Answer whether a #MemoryRegion within @parent covers the address > > * @addr. > > * > > - * @parent: a MemoryRegion within which @addr is a relative address > > + * @parent: a #MemoryRegion within which @addr is a relative address > > * @addr: the area within @parent to be searched > > */ > > bool memory_region_present(MemoryRegion *parent, hwaddr addr); > > diff --git a/memory.c b/memory.c > > index 59ecc28..3f1df23 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) > > bool memory_region_present(MemoryRegion *parent, hwaddr addr) > > { > > MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; > > - if (!mr) { > > + if (!mr || (mr == parent)) { > > return false; > > } > > memory_region_unref(mr); > > -- > > 1.7.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 12:32 ` Igor Mammedov @ 2014-02-06 13:21 ` Michael S. Tsirkin 2014-02-06 13:34 ` Igor Mammedov 2014-02-07 10:00 ` Paolo Bonzini 1 sibling, 1 reply; 7+ messages in thread From: Michael S. Tsirkin @ 2014-02-06 13:21 UTC (permalink / raw) To: Igor Mammedov Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, pbonzini, rth On Thu, Feb 06, 2014 at 01:32:25PM +0100, Igor Mammedov wrote: > On Thu, 6 Feb 2014 14:22:54 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Feb 06, 2014 at 11:24:33AM +0100, Igor Mammedov wrote: > > > Windows XP shows COM2 port as non functional in > > > "Device Manager" although no COM2 port backing device > > > is present in QEMU. > > > > > > That is caused by the fact that QEMU reports to > > > OSPM that device is present by setting 5th bit in > > > PII4XPM.pci_conf[0x67] register when COM2 doesn't > > > exist. > > > > > > It happens due to memory_region_present(io_as, 0x2f8) > > > returning false positive since 0x2f8 address eventually > > > translates into catchall io_as address space. > > > > > > Fix memory_region_present(parent, addr) by returning > > > true only if addr maps into a MemoryRegion within > > > parent (excluding parent itself), to match its > > > doc comment. > > > > > > While at it fix copy/paste error in > > > memory_region_present() doc comment. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > Hmm, I wonder whether this still should return true > > if parent is not a pure container. > io_as in not a pure container, that's why > memory_region_find() finds it at all. Ah. So this regression is really due to 3bb28b7208b349e7a1b326e3c6ef9efac1d462bf? I think we really need better handling for unassigned regions, for memory we need it to implement master abort properly. > This patch only fixes function to behave as it was > originally documented. > > I guess we need to wait on Paolo's opinion, as an author > if original code. Well - consider what this function came to replace: - pci_conf[0x67] = (memory_region_find(io_as, 0x3f8, 1).mr ? 0x08 : 0) | - (memory_region_find(io_as, 0x2f8, 1).mr ? 0x90 : 0); + pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) | + (memory_region_present(io_as, 0x2f8) ? 0x90 : 0); It's just that this function no longer works for io addresses. > > Also what if MR isn't under parent at all? > that function was never meant to handle this case. > > > > > Maybe the right thing to do is > > return mr->ops == &unassigned_mem_ops ? > it could be but it looks more like a hack. > i.e this exit conditions could pile up over time. Sigh. It's all a hack, we need to handle errors better. For now I'm ok with your patch. > > > > > > > --- > > > include/exec/memory.h | 6 +++--- > > > memory.c | 2 +- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > index 296d6ab..a5eb4c8 100644 > > > --- a/include/exec/memory.h > > > +++ b/include/exec/memory.h > > > @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, > > > hwaddr offset); > > > > > > /** > > > - * memory_region_present: translate an address/size relative to a > > > - * MemoryRegion into a #MemoryRegionSection. > > > + * memory_region_present: checks if an address relative to a @parent > > > + * translates into #MemoryRegion within @parent > > > * > > > * Answer whether a #MemoryRegion within @parent covers the address > > > * @addr. > > > * > > > - * @parent: a MemoryRegion within which @addr is a relative address > > > + * @parent: a #MemoryRegion within which @addr is a relative address > > > * @addr: the area within @parent to be searched > > > */ > > > bool memory_region_present(MemoryRegion *parent, hwaddr addr); > > > diff --git a/memory.c b/memory.c > > > index 59ecc28..3f1df23 100644 > > > --- a/memory.c > > > +++ b/memory.c > > > @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) > > > bool memory_region_present(MemoryRegion *parent, hwaddr addr) > > > { > > > MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; > > > - if (!mr) { > > > + if (!mr || (mr == parent)) { > > > return false; > > > } > > > memory_region_unref(mr); > > > -- > > > 1.7.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 13:21 ` Michael S. Tsirkin @ 2014-02-06 13:34 ` Igor Mammedov 0 siblings, 0 replies; 7+ messages in thread From: Igor Mammedov @ 2014-02-06 13:34 UTC (permalink / raw) To: Michael S. Tsirkin Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, pbonzini, rth On Thu, 6 Feb 2014 15:21:56 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Feb 06, 2014 at 01:32:25PM +0100, Igor Mammedov wrote: > > On Thu, 6 Feb 2014 14:22:54 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Feb 06, 2014 at 11:24:33AM +0100, Igor Mammedov wrote: > > > > Windows XP shows COM2 port as non functional in > > > > "Device Manager" although no COM2 port backing device > > > > is present in QEMU. > > > > > > > > That is caused by the fact that QEMU reports to > > > > OSPM that device is present by setting 5th bit in > > > > PII4XPM.pci_conf[0x67] register when COM2 doesn't > > > > exist. > > > > > > > > It happens due to memory_region_present(io_as, 0x2f8) > > > > returning false positive since 0x2f8 address eventually > > > > translates into catchall io_as address space. > > > > > > > > Fix memory_region_present(parent, addr) by returning > > > > true only if addr maps into a MemoryRegion within > > > > parent (excluding parent itself), to match its > > > > doc comment. > > > > > > > > While at it fix copy/paste error in > > > > memory_region_present() doc comment. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > Hmm, I wonder whether this still should return true > > > if parent is not a pure container. > > io_as in not a pure container, that's why > > memory_region_find() finds it at all. > > Ah. So this regression is really due to 3bb28b7208b349e7a1b326e3c6ef9efac1d462bf? yep, it was working before this patch. > I think we really need better handling for unassigned regions, > for memory we need it to implement master abort properly. > > > This patch only fixes function to behave as it was > > originally documented. > > > > I guess we need to wait on Paolo's opinion, as an author > > if original code. > > Well - consider what this function came to replace: > - pci_conf[0x67] = (memory_region_find(io_as, 0x3f8, 1).mr ? 0x08 : 0) | > - (memory_region_find(io_as, 0x2f8, 1).mr ? 0x90 : 0); > + pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) | > + (memory_region_present(io_as, 0x2f8) ? 0x90 : 0); > > It's just that this function no longer works for io addresses. yep, I guess it would be so for PCI address space (i.e. master abort patches) is AS will terminate unhanded accesses. > > > > Also what if MR isn't under parent at all? > > that function was never meant to handle this case. > > > > > > > > Maybe the right thing to do is > > > return mr->ops == &unassigned_mem_ops ? > > it could be but it looks more like a hack. > > i.e this exit conditions could pile up over time. > > Sigh. It's all a hack, we need to handle errors better. > For now I'm ok with your patch. > > > > > > > > > > > --- > > > > include/exec/memory.h | 6 +++--- > > > > memory.c | 2 +- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > > > index 296d6ab..a5eb4c8 100644 > > > > --- a/include/exec/memory.h > > > > +++ b/include/exec/memory.h > > > > @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, > > > > hwaddr offset); > > > > > > > > /** > > > > - * memory_region_present: translate an address/size relative to a > > > > - * MemoryRegion into a #MemoryRegionSection. > > > > + * memory_region_present: checks if an address relative to a @parent > > > > + * translates into #MemoryRegion within @parent > > > > * > > > > * Answer whether a #MemoryRegion within @parent covers the address > > > > * @addr. > > > > * > > > > - * @parent: a MemoryRegion within which @addr is a relative address > > > > + * @parent: a #MemoryRegion within which @addr is a relative address > > > > * @addr: the area within @parent to be searched > > > > */ > > > > bool memory_region_present(MemoryRegion *parent, hwaddr addr); > > > > diff --git a/memory.c b/memory.c > > > > index 59ecc28..3f1df23 100644 > > > > --- a/memory.c > > > > +++ b/memory.c > > > > @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) > > > > bool memory_region_present(MemoryRegion *parent, hwaddr addr) > > > > { > > > > MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; > > > > - if (!mr) { > > > > + if (!mr || (mr == parent)) { > > > > return false; > > > > } > > > > memory_region_unref(mr); > > > > -- > > > > 1.7.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 12:32 ` Igor Mammedov 2014-02-06 13:21 ` Michael S. Tsirkin @ 2014-02-07 10:00 ` Paolo Bonzini 1 sibling, 0 replies; 7+ messages in thread From: Paolo Bonzini @ 2014-02-07 10:00 UTC (permalink / raw) To: Igor Mammedov, Michael S. Tsirkin Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, rth Il 06/02/2014 13:32, Igor Mammedov ha scritto: > io_as in not a pure container, that's why > memory_region_find() finds it at all. > > This patch only fixes function to behave as it was > originally documented. The idea was that parent would be a pure container :) but your patch makes sense and fixes the problem. Paolo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion 2014-02-06 10:24 [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion Igor Mammedov 2014-02-06 12:22 ` Michael S. Tsirkin @ 2014-02-16 16:40 ` Michael S. Tsirkin 1 sibling, 0 replies; 7+ messages in thread From: Michael S. Tsirkin @ 2014-02-16 16:40 UTC (permalink / raw) To: Igor Mammedov Cc: peter.maydell, jan.kiszka, qemu-devel, anthony, pbonzini, rth On Thu, Feb 06, 2014 at 11:24:33AM +0100, Igor Mammedov wrote: > Windows XP shows COM2 port as non functional in > "Device Manager" although no COM2 port backing device > is present in QEMU. > > That is caused by the fact that QEMU reports to > OSPM that device is present by setting 5th bit in > PII4XPM.pci_conf[0x67] register when COM2 doesn't > exist. > > It happens due to memory_region_present(io_as, 0x2f8) > returning false positive since 0x2f8 address eventually > translates into catchall io_as address space. > > Fix memory_region_present(parent, addr) by returning > true only if addr maps into a MemoryRegion within > parent (excluding parent itself), to match its > doc comment. > > While at it fix copy/paste error in > memory_region_present() doc comment. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Applied, thanks! > --- > include/exec/memory.h | 6 +++--- > memory.c | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 296d6ab..a5eb4c8 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -838,13 +838,13 @@ void memory_region_set_alias_offset(MemoryRegion *mr, > hwaddr offset); > > /** > - * memory_region_present: translate an address/size relative to a > - * MemoryRegion into a #MemoryRegionSection. > + * memory_region_present: checks if an address relative to a @parent > + * translates into #MemoryRegion within @parent > * > * Answer whether a #MemoryRegion within @parent covers the address > * @addr. > * > - * @parent: a MemoryRegion within which @addr is a relative address > + * @parent: a #MemoryRegion within which @addr is a relative address > * @addr: the area within @parent to be searched > */ > bool memory_region_present(MemoryRegion *parent, hwaddr addr); > diff --git a/memory.c b/memory.c > index 59ecc28..3f1df23 100644 > --- a/memory.c > +++ b/memory.c > @@ -1562,7 +1562,7 @@ static FlatRange *flatview_lookup(FlatView *view, AddrRange addr) > bool memory_region_present(MemoryRegion *parent, hwaddr addr) > { > MemoryRegion *mr = memory_region_find(parent, addr, 1).mr; > - if (!mr) { > + if (!mr || (mr == parent)) { > return false; > } > memory_region_unref(mr); > -- > 1.7.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-02-16 16:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-06 10:24 [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion Igor Mammedov 2014-02-06 12:22 ` Michael S. Tsirkin 2014-02-06 12:32 ` Igor Mammedov 2014-02-06 13:21 ` Michael S. Tsirkin 2014-02-06 13:34 ` Igor Mammedov 2014-02-07 10:00 ` Paolo Bonzini 2014-02-16 16:40 ` 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).