Re: [PATCH 3/5] hw/pci-host/prep: Remove unuseful memory region mapping

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3359 bytes --]

On Fri, Mar 12, 2021 at 03:38:21PM -0500, Peter Xu wrote:
> On Fri, Mar 12, 2021 at 07:28:49PM +0100, Philippe Mathieu-Daudé wrote:
> > The pci_io_non_contiguous region is mapped on top of pci_io
> > with higher priority, but simply dispatch into this region
> > address space. Simplify by directly registering the former
> > region in place, and adapt the address space dispatch offsets.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/pci-host/prep.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/pci-host/prep.c b/hw/pci-host/prep.c
> > index 0a9162fba97..00a28c2d18c 100644
> > --- a/hw/pci-host/prep.c
> > +++ b/hw/pci-host/prep.c
> > @@ -159,8 +159,7 @@ static uint64_t raven_io_read(void *opaque, hwaddr addr,
> >      uint8_t buf[4];
> >  
> >      addr = raven_io_address(s, addr);
> > -    address_space_read(&s->pci_io_as, addr + 0x80000000,
> > -                       MEMTXATTRS_UNSPECIFIED, buf, size);
> > +    address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
> >  
> >      if (size == 1) {
> >          return buf[0];
> > @@ -191,8 +190,7 @@ static void raven_io_write(void *opaque, hwaddr addr,
> >          g_assert_not_reached();
> >      }
> >  
> > -    address_space_write(&s->pci_io_as, addr + 0x80000000,
> > -                        MEMTXATTRS_UNSPECIFIED, buf, size);
> > +    address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED, buf, size);
> 
> This changes access to s->pci_io_as, but below didn't change s->pci_io_as
> layout at all (below is about address_space_mem).  Is this intended?
> 
> >  }
> >  
> >  static const MemoryRegionOps raven_io_ops = {
> > @@ -294,9 +292,8 @@ static void raven_pcihost_initfn(Object *obj)
> >      address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
> >  
> >      /* CPU address space */
> > -    memory_region_add_subregion(address_space_mem, 0x80000000, &s->pci_io);
> > -    memory_region_add_subregion_overlap(address_space_mem, 0x80000000,
> > -                                        &s->pci_io_non_contiguous, 1);
> > +    memory_region_add_subregion(address_space_mem, 0x80000000,
> > +                                &s->pci_io_non_contiguous);
> 
> I don't know any of this code at all... but it seems the two memory regions are
> not identical in size:
> 
>     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
>                           "pci-io-non-contiguous", 0x00800000);
> 
> Then it seems the memory access dispatching to (0x00800000, 0x3f800000) would
> change too, from s->pci_io to nothing.  Raise this up too since I don't know
> either whether it's intended..

Right, it seems like this removes the mapping of s->pci_io entirely.

> 
> >      memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> >      pci_root_bus_new_inplace(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
> >                               &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]