On Thu, 6 Feb 2014 14:22:54 +0200
"Michael S. Tsirkin" <m...@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 <imamm...@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
