Re: [Qemu-devel] [PATCH] memory_region_present: return false if address is not found in child MemoryRegion

[Igor Mammedov <imammedo@redhat.com>]

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
>