[Qemu-devel] sub-page-sized mmio regions and address passed to read/write fns

[Qemu-devel] sub-page-sized mmio regions and address passed to read/write fns

[Peter Maydell]

Hi; I was working on a refactoring of the ARM 11MPCore/A9MP private
peripherals and encountered something odd. Rather than having a single
large mmio region, I tried splitting into several regions, like this:

    memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
    memory_region_init_io(&s->scu_iomem, &a9_scu_ops, s, "a9mp-scu", 0x100);
    memory_region_init_io(&s->gic_cpu_iomem, &a9_gic_cpu_ops, s,
                          "a9mp-gic-cpu", 0x100);
    memory_region_init_io(&s->ptimer_iomem, &a9_ptimer_ops, s,
                          "a9mp-ptimer", 0x100);
    memory_region_add_subregion(&s->container, 0, &s->scu_iomem);
    memory_region_add_subregion(&s->container, 0x100, &s->gic_cpu_iomem);
    memory_region_add_subregion(&s->container, 0x600, &s->ptimer_iomem);
    memory_region_add_subregion(&s->container, 0x1000, &s->gic.iomem);
    sysbus_init_mmio_region(dev, &s->container);

However what I found is that the addresses passed to the read/write
functions aren't what I would expect. For instance if the board
maps the container at address 0x1e000000, then a read from 0x1e000100
goes to the functions given by a9_gic_cpu_ops, as it should. However,
the offset parameter that the read function is passed is not 0x0
(offset from the start of the a9mp-gic-cpu region) but 0x100 (offset
from the start of the page, I think).

Is this expected behaviour? I certainly wasn't expecting it...

I looked through the code that's getting called for reads, and
it looks to me like exec.c:subpage_readlen() is causing this.
We look up the subpage_t based on the address within the page,
but we don't then adjust the address we pass to io_mem_read
(except by region_offset, which I take from the comment at the
top of cpu_register_physical_memory_log() to be for something
else.)

-- PMM

Re: [Qemu-devel] sub-page-sized mmio regions and address passed to read/write fns

[Avi Kivity]

On 12/02/2011 04:49 PM, Peter Maydell wrote:
> Hi; I was working on a refactoring of the ARM 11MPCore/A9MP private
> peripherals and encountered something odd. Rather than having a single
> large mmio region, I tried splitting into several regions, like this:
>
>     memory_region_init(&s->container, "a9mp-priv-container", 0x2000);
>     memory_region_init_io(&s->scu_iomem, &a9_scu_ops, s, "a9mp-scu", 0x100);
>     memory_region_init_io(&s->gic_cpu_iomem, &a9_gic_cpu_ops, s,
>                           "a9mp-gic-cpu", 0x100);
>     memory_region_init_io(&s->ptimer_iomem, &a9_ptimer_ops, s,
>                           "a9mp-ptimer", 0x100);
>     memory_region_add_subregion(&s->container, 0, &s->scu_iomem);
>     memory_region_add_subregion(&s->container, 0x100, &s->gic_cpu_iomem);
>     memory_region_add_subregion(&s->container, 0x600, &s->ptimer_iomem);
>     memory_region_add_subregion(&s->container, 0x1000, &s->gic.iomem);
>     sysbus_init_mmio_region(dev, &s->container);

Good practice IMO, will become more important when we introduce a
Register class.

> However what I found is that the addresses passed to the read/write
> functions aren't what I would expect. For instance if the board
> maps the container at address 0x1e000000, then a read from 0x1e000100
> goes to the functions given by a9_gic_cpu_ops, as it should. However,
> the offset parameter that the read function is passed is not 0x0
> (offset from the start of the a9mp-gic-cpu region) but 0x100 (offset
> from the start of the page, I think).
>
> Is this expected behaviour? I certainly wasn't expecting it...

A while ago this was the behaviour across the board.  Then 8da3ff1809747
changed addresses to be relative, but apparently missed the subpage case.

> I looked through the code that's getting called for reads, and
> it looks to me like exec.c:subpage_readlen() is causing this.
> We look up the subpage_t based on the address within the page,
> but we don't then adjust the address we pass to io_mem_read
> (except by region_offset, which I take from the comment at the
> top of cpu_register_physical_memory_log() to be for something
> else.)
>

I think you can use subpage_t's region_offset array for this (adding
into it, of course, so the original value remains).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] sub-page-sized mmio regions and address passed to read/write fns

[Peter Maydell]

On 4 December 2011 12:17, Avi Kivity <avi@redhat.com> wrote:
> On 12/02/2011 04:49 PM, Peter Maydell wrote:
>> However what I found is that the addresses passed to the read/write
>> functions aren't what I would expect. For instance if the board
>> maps the container at address 0x1e000000, then a read from 0x1e000100
>> goes to the functions given by a9_gic_cpu_ops, as it should. However,
>> the offset parameter that the read function is passed is not 0x0
>> (offset from the start of the a9mp-gic-cpu region) but 0x100 (offset
>> from the start of the page, I think).
>>
>> Is this expected behaviour? I certainly wasn't expecting it...
>
> A while ago this was the behaviour across the board.  Then 8da3ff1809747
> changed addresses to be relative, but apparently missed the subpage case.

Having looked a bit more closely at the code I think this is what
the comment at the top of cpu_register_physical_memory_log() is
referring to:

# Both start_addr and region_offset are rounded down to a page boundary
# before calculating this offset.  This should not be a problem unless
# the low bits of start_addr and region_offset differ.

In the case of a subregion at a non-page-aligned-address the
start_addr is not page aligned, but the region_offset is zero,
in the usual case, so we have differing low bits.

>> I looked through the code that's getting called for reads, and
>> it looks to me like exec.c:subpage_readlen() is causing this.
>> We look up the subpage_t based on the address within the page,
>> but we don't then adjust the address we pass to io_mem_read
>> (except by region_offset, which I take from the comment at the
>> top of cpu_register_physical_memory_log() to be for something
>> else.)

> I think you can use subpage_t's region_offset array for this (adding
> into it, of course, so the original value remains).

Yes. I think the correction has to be calculated and applied in
cpu_register_physical_memory_log() -- for a region which starts
at a non-page-aligned address and extends over more than a page
the correcting offset needs to be applied for the whole region,
not just the first partial page.

-- PMM

Re: [Qemu-devel] sub-page-sized mmio regions and address passed to read/write fns

[Avi Kivity]

On 12/04/2011 11:15 PM, Peter Maydell wrote:
> On 4 December 2011 12:17, Avi Kivity <avi@redhat.com> wrote:
> > On 12/02/2011 04:49 PM, Peter Maydell wrote:
> >> However what I found is that the addresses passed to the read/write
> >> functions aren't what I would expect. For instance if the board
> >> maps the container at address 0x1e000000, then a read from 0x1e000100
> >> goes to the functions given by a9_gic_cpu_ops, as it should. However,
> >> the offset parameter that the read function is passed is not 0x0
> >> (offset from the start of the a9mp-gic-cpu region) but 0x100 (offset
> >> from the start of the page, I think).
> >>
> >> Is this expected behaviour? I certainly wasn't expecting it...
> >
> > A while ago this was the behaviour across the board.  Then 8da3ff1809747
> > changed addresses to be relative, but apparently missed the subpage case.
>
> Having looked a bit more closely at the code I think this is what
> the comment at the top of cpu_register_physical_memory_log() is
> referring to:
>
> # Both start_addr and region_offset are rounded down to a page boundary
> # before calculating this offset.  This should not be a problem unless
> # the low bits of start_addr and region_offset differ.
>
> In the case of a subregion at a non-page-aligned-address the
> start_addr is not page aligned, but the region_offset is zero,
> in the usual case, so we have differing low bits.

Not an issue in the subpage code.  As long as you extract the mmio index
before adding region_offset, you're fine (as the mmio index resides in
the low order bits).

> >> I looked through the code that's getting called for reads, and
> >> it looks to me like exec.c:subpage_readlen() is causing this.
> >> We look up the subpage_t based on the address within the page,
> >> but we don't then adjust the address we pass to io_mem_read
> >> (except by region_offset, which I take from the comment at the
> >> top of cpu_register_physical_memory_log() to be for something
> >> else.)
>
> > I think you can use subpage_t's region_offset array for this (adding
> > into it, of course, so the original value remains).
>
> Yes. I think the correction has to be calculated and applied in
> cpu_register_physical_memory_log() -- for a region which starts
> at a non-page-aligned address and extends over more than a page
> the correcting offset needs to be applied for the whole region,
> not just the first partial page.

In that case we have to use subpages for full pages.  But better to just
assert() that this never happens for now.

-- 
error compiling committee.c: too many arguments to function