On Sat, Apr 17, 2021 at 12:30:23PM +0200, Philippe Mathieu-Daudé wrote:
> Commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region")
> abused an AddressSpace API weakness which allows set non-zero base
> address to AddressSpace root region. We will fix that in the next
> commit. First add an assertion to ensure no regression is introduced.
> As raven_io_address() is called by both MemoryRegionOps handlers, it
> is a good place for such assert call.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/pci-host/raven.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index 730f31a8931..36652122424 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -141,6 +141,17 @@ static const MemoryRegionOps raven_intack_ops = {
>  static inline hwaddr raven_io_address(PREPPCIState *s,
>                                        hwaddr addr)
>  {
> +    /*
> +     * We shouldn't access AddressSpace internals. However this assert
> +     * is temporarily introduced to prove a subtle inconsistency from
> +     * commit 1ae1dc5ba24 ("raven: Set a correct PCI I/O memory region"):
> +     * AddressSpace root region must be zero-based, but the Raven use is not.
> +     *
> +     * Assert the root region is based on physical address 0x80000000
> +     * until the issue is fixed.
> +     */
> +    assert(s->pci_io_as.root->addr == PCI_IO_BASE_ADDR);
> +
>      if (s->contiguous_map == 0) {
>          /* 64 KB contiguous space for IOs */
>          addr &= 0xFFFF;

-- 
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