| Message ID | 1391682273-8457-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 >
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
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
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);
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(-)