QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Peter Maydell]

I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
where an attempt to write to an address within the PCI IO window
where the guest hasn't mapped a BAR causes a CPU exception rather than
(what I believe is) the PCI-required behaviour of writes-ignored, reads
return -1.

What in the QEMU PCI code is responsible for giving the PCI-spec
behaviour for accesses to the PCI IO and memory windows where there
is no BAR? I was expecting the generic PCI code to map a background
memory region over the whole window to do this, but it looks like it
doesn't...

thanks
-- PMM

Re: QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Philippe Mathieu-Daudé]

On 3/19/21 1:35 PM, Peter Maydell wrote:
> I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
> PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
> where an attempt to write to an address within the PCI IO window
> where the guest hasn't mapped a BAR causes a CPU exception rather than
> (what I believe is) the PCI-required behaviour of writes-ignored, reads
> return -1.
> 
> What in the QEMU PCI code is responsible for giving the PCI-spec
> behaviour for accesses to the PCI IO and memory windows where there
> is no BAR? I was expecting the generic PCI code to map a background
> memory region over the whole window to do this, but it looks like it
> doesn't...

Indeed, this is something I noticed in my generic AddressSpace
cleanup series.

Currently it is (incorrectly?) done locally by each pci-host
implementation, some register the regions behaving as RAZ/WI,
some don't.

Re: QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Laszlo Ersek]

On 03/19/21 15:14, Philippe Mathieu-Daudé wrote:
> On 3/19/21 1:35 PM, Peter Maydell wrote:
>> I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
>> PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
>> where an attempt to write to an address within the PCI IO window
>> where the guest hasn't mapped a BAR causes a CPU exception rather than
>> (what I believe is) the PCI-required behaviour of writes-ignored, reads
>> return -1.
>>
>> What in the QEMU PCI code is responsible for giving the PCI-spec
>> behaviour for accesses to the PCI IO and memory windows where there
>> is no BAR? I was expecting the generic PCI code to map a background
>> memory region over the whole window to do this, but it looks like it
>> doesn't...
> 
> Indeed, this is something I noticed in my generic AddressSpace
> cleanup series.
> 
> Currently it is (incorrectly?) done locally by each pci-host
> implementation, some register the regions behaving as RAZ/WI,
> some don't.
> 

Related discussions from the past:

(1)

[Qemu-devel] [PATCH] exec: fix regression by making system-memory region
https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg00142.html

(Commits:

a53ae8e934cd ('hw/pci: partially handle pci master abort', 2013-10-14)
818f86b88394 ('exec: limit system memory size', 2013-11-04)
0fbf50b6ec12 ('Revert "hw/pci: partially handle pci master abort"',
2013-11-10)
ef9e455d645b ('Revert "exec: limit system memory size"', 2013-11-10)
)

(2)

[Qemu-devel] [PATCH] memory region: check the old.mmio.read status
https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01391.html

The last two messages in that thread appear particularly relevant:

- https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01524.html
- https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01525.html

Assuming I'm not completely confused, my take-away is that it does
belong in each pci-host device model (or maybe into an extracted, parent
device model). Namely, Peter said in the last message linked above that
whether an unmapped access would cause a CPU fault was the function of
two things:

- whether the CPU supported bus faults architecturally,
- whether the bus in an SoC or board reported unmapped accesses to the
CPU as bus faults.

For the case where the first question was answered with "yes" but the
second was answered with "no", "adding a suitable io accessor to the
relevant container MR" was suggested -- and that's what the pci-host
device models do, at the moment, IIUC.

*Where* this unassigned IO MR should be implemented is unclear:

- The 2nd variable above is board/SoC dependent, so does the unassigned
IO "catch" belong in board code?

- Or else, because the PCI spec is board-independent, does it belong
with some common pci-host code?

Personally, I'd add it to the pci-gpex controller; that would fix the
symptom, satisfy the generic requirement (a function of two variables,
see above), and come with no risk of regressions, for other boards / PCI
host controllers.

Thanks
Laszlo

Re: QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Michael S. Tsirkin]

On Fri, Mar 19, 2021 at 12:35:31PM +0000, Peter Maydell wrote:
> I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
> PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
> where an attempt to write to an address within the PCI IO window
> where the guest hasn't mapped a BAR causes a CPU exception rather than
> (what I believe is) the PCI-required behaviour of writes-ignored, reads
> return -1.
> 
> What in the QEMU PCI code is responsible for giving the PCI-spec
> behaviour for accesses to the PCI IO and memory windows where there
> is no BAR? I was expecting the generic PCI code to map a background
> memory region over the whole window to do this, but it looks like it
> doesn't...
> 
> thanks
> -- PMM

As far as I know, at the PCI level what happens is Master Abort
on PCI/PCI-X and Unsupported Request on Express.
PCI spec says:
	The host bus bridge, in PC compatible systems, must return all 1's on a read transaction and
	discard data on a write transaction when terminated with Master-Abort.

We thus implement this per host e.g. on pc compatible systems by
calling pc_pci_as_mapping_init.

A note aside:

PCI Express spec has this weird text:

Read Data Values with UR Completion Status
	Some system configuration software depends on reading a data value of all 1’s when a Configuration
	Read Request is terminated as an Unsupported Request, particularly when probing to determine the
	existence of a device in the system. A Root Complex intended for use with software that depends
	on a read-data value of all 1’s must synthesize this value when UR Completion Status is returned for
	a Configuration Read Request.

the title says "Read Data" while the text says "Configuration Read".
Can't say whether that is intentional.

-- 
MST

Re: QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Peter Maydell]

On Sat, 20 Mar 2021 at 18:59, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 19, 2021 at 12:35:31PM +0000, Peter Maydell wrote:
> > I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
> > PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
> > where an attempt to write to an address within the PCI IO window
> > where the guest hasn't mapped a BAR causes a CPU exception rather than
> > (what I believe is) the PCI-required behaviour of writes-ignored, reads
> > return -1.
> >
> > What in the QEMU PCI code is responsible for giving the PCI-spec
> > behaviour for accesses to the PCI IO and memory windows where there
> > is no BAR? I was expecting the generic PCI code to map a background
> > memory region over the whole window to do this, but it looks like it
> > doesn't...

> As far as I know, at the PCI level what happens is Master Abort
> on PCI/PCI-X and Unsupported Request on Express.
> PCI spec says:
>         The host bus bridge, in PC compatible systems, must return all 1's on a read transaction and
>         discard data on a write transaction when terminated with Master-Abort.
>
> We thus implement this per host e.g. on pc compatible systems by
> calling pc_pci_as_mapping_init.

Isn't pc_pci_as_mapping_init() "put the PCI space into the system
address space", rather than "define the default behaviour for
accesses in PCI space" ? IIRC x86 has -1/discard for everywhere,
though, so maybe you get that without having to do anything special.

Q: if PCI device A does a bus-mastering DMA read to a PCI address
where no other device has been mapped, does the spec require
it to (a) get back a "transaction failed" response or (b) get
back read-data of -1 ? It sounds like the answer based on what
you write above is (a), device A gets a Master Abort.
(Put another way, is the -1/discard behaviour general to PCI
transactions or is it strictly something that happens at the host
bridge where the host bridge turns host CPU transactions into PCI
transactions ?)

If this is host-bridge specific then I guess our current
implementation of "leave it up to the host bridge code" makes
sense, but it also seems like a recipe for all our host bridges
forgetting this corner case, in the absence of support from
the common code for making it easy/the default...

Anyway, I think that for hw/pci-host/gpex.c we would need
to change the current
    memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
    [...]
    sysbus_init_mmio(sbd, &s->io_mmio);
    [...]
    pci->bus = pci_register_root_bus(dev, "pcie.0", gpex_set_irq,
                                     pci_swizzle_map_irq_fn, s, &s->io_mmio,
                                     &s->io_ioport, 0, 4, TYPE_PCIE_BUS);

to also create a container MR with a background set of io read/write
functions to give the -1/discard behaviour, map s->io_mmio into
that container, and return the container as the sysbus MMIO region.
(and same again for the IO window).

thanks
-- PMM

Re: QEMU PCI subsystem: what code is responsible for making accesses to non-mapped addresses read as -1?

[Michael S. Tsirkin]

On Sat, Mar 20, 2021 at 08:40:13PM +0000, Peter Maydell wrote:
> On Sat, 20 Mar 2021 at 18:59, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Mar 19, 2021 at 12:35:31PM +0000, Peter Maydell wrote:
> > > I'm looking at a bug reported against the QEMU arm virt board's pci-gpex
> > > PCI controller: https://bugs.launchpad.net/qemu/+bug/1918917
> > > where an attempt to write to an address within the PCI IO window
> > > where the guest hasn't mapped a BAR causes a CPU exception rather than
> > > (what I believe is) the PCI-required behaviour of writes-ignored, reads
> > > return -1.
> > >
> > > What in the QEMU PCI code is responsible for giving the PCI-spec
> > > behaviour for accesses to the PCI IO and memory windows where there
> > > is no BAR? I was expecting the generic PCI code to map a background
> > > memory region over the whole window to do this, but it looks like it
> > > doesn't...
> 
> > As far as I know, at the PCI level what happens is Master Abort
> > on PCI/PCI-X and Unsupported Request on Express.
> > PCI spec says:
> >         The host bus bridge, in PC compatible systems, must return all 1's on a read transaction and
> >         discard data on a write transaction when terminated with Master-Abort.
> >
> > We thus implement this per host e.g. on pc compatible systems by
> > calling pc_pci_as_mapping_init.
> 
> Isn't pc_pci_as_mapping_init() "put the PCI space into the system
> address space", rather than "define the default behaviour for
> accesses in PCI space" ? IIRC x86 has -1/discard for everywhere,
> though, so maybe you get that without having to do anything special.

Hmm ... I guess it's too late in the night here, and I am confused ..
will try to recall tomorrow.

> Q: if PCI device A does a bus-mastering DMA read to a PCI address
> where no other device has been mapped, does the spec require
> it to (a) get back a "transaction failed" response or (b) get
> back read-data of -1 ? It sounds like the answer based on what
> you write above is (a), device A gets a Master Abort.
> (Put another way, is the -1/discard behaviour general to PCI
> transactions or is it strictly something that happens at the host
> bridge where the host bridge turns host CPU transactions into PCI
> transactions ?)

IIUC it's the host that is doing the conversion.

> If this is host-bridge specific then I guess our current
> implementation of "leave it up to the host bridge code" makes
> sense, but it also seems like a recipe for all our host bridges
> forgetting this corner case, in the absence of support from
> the common code for making it easy/the default...

Right at some level ... I'm not against just doing this in the pci core.

> Anyway, I think that for hw/pci-host/gpex.c we would need
> to change the current
>     memory_region_init(&s->io_mmio, OBJECT(s), "gpex_mmio", UINT64_MAX);
>     [...]
>     sysbus_init_mmio(sbd, &s->io_mmio);
>     [...]
>     pci->bus = pci_register_root_bus(dev, "pcie.0", gpex_set_irq,
>                                      pci_swizzle_map_irq_fn, s, &s->io_mmio,
>                                      &s->io_ioport, 0, 4, TYPE_PCIE_BUS);
> 
> to also create a container MR with a background set of io read/write
> functions to give the -1/discard behaviour, map s->io_mmio into
> that container, and return the container as the sysbus MMIO region.
> (and same again for the IO window).
> 
> thanks
> -- PMM

Hmm. But see
commit 0fbf50b6ec126600dca115adb1563c657cc27695
Author: Marcel Apfelbaum <marcel.a@redhat.com>
Date:   Sun Nov 10 11:49:57 2013 +0200

    Revert "hw/pci: partially handle pci master abort"
    
    This reverts commit a53ae8e934cd54686875b5bcfc2f434244ee55d6.
    
    The patch being reverted introduced a low-priority memory region
    covering all 64 bit pci address space.  This exposed the following bugs
    elsewhere in the code:
     1. Some memory regions have INT64_MAX size, where the
        intent was all 64 bit address space.
        This results in a sub-page region, should be UINT64_MAX.
     2. page table rendering in exec.c ignores physical address bits
        above TARGET_PHYS_ADDR_SPACE_BITS.
        Access outside this range (e.g. from device DMA, or gdb stub)
        ends up with a wrong region.  Registering a region outside this
        range leads to page table corruption.
     3. Some regions overlap PCI hole and have same priority.
        This only works as long as no device uses the overlapping address.
    
    It doesn't look like we can resolve all issues in time for 1.7.
    Let's fix the bugs first and apply afterwards for 1.8.
    
    Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I wonder whether we fixed all of these by now ...  I think we basically
fixed 3 and some greps could not find any more instances of 1.  Not sure
about 2 though. Should we just limit the region to
TARGET_PHYS_ADDR_SPACE_BITS?

-- 
MST