Re: [Qemu-devel] [PATCH RFC v2 2/2] hw/pci: handle unassigned pci addresses

[Peter Maydell]

On 15 September 2013 08:14, Michael S. Tsirkin <address@hidden> wrote:
> On Tue, Sep 10, 2013 at 02:12:56PM +0100, Peter Maydell wrote:
>> On 10 September 2013 14:02, Michael S. Tsirkin <address@hidden> wrote:
>> > On Tue, Sep 10, 2013 at 01:50:47PM +0100, Peter Maydell wrote:
>> >> On 10 September 2013 13:39, Michael S. Tsirkin <address@hidden> wrote:
>> >> > On Mon, Sep 09, 2013 at 02:16:41PM +0100, Peter Maydell wrote:
>> >> >>     memory_region_init_alias(&pci_dev->bus_master_enable_region,
>> >> >>                              OBJECT(pci_dev), "bus master",
>> >> >>                              dma_as->root, 0,
>> >> >>                              memory_region_size(dma_as->root));
>> >> >>
>> >> >> If instead of using this alias directly as the
>> >> >> bus_master_enable region you instead:
>> >> >>  * create a container region
>> >> >>  * create a 'background' region at negative priority
>> >> >>    (ie one per device, and you can make the 'opaque' pointer
>> >> >>    point to the device, not the bus)
>> >> >>  * put the alias and the background region into the container
>> >> >>  * use the container as the bus_master_enable region
>> >> >
>> >> > Interesting. There's one thing I don't understand here:
>> >> > as far as I can see bus_master_enable_region covers the
>> >> > whole 64 bit memory address space.
>> >> >
>> >> > It looks like it will always override the background
>> >> > region in the same container. What did I miss?
>> >>
>> >> That should be itself a container,
>> >> so assuming it doesn't
>> >> itself have any kind of background region the "holes"
>> >> inside it will still be present when we put it in
>> >> our new container. (Basically putting a container,
>> >> or an alias to one, inside a region is just saying
>> >> "put everything in that container inside this region
>> >> at the appropriate place").
>> >
>> > Confused.  "That" and "it" here refers to what exactly?
>>
>> Well, I was a bit confused by your talking about
>> the properties of "bus_master_enable_region" when my
>> suggestion is effectively that we change what that is.
>> So let's start again:
>>  * create a container region
>> This is 64 bits wide, but totally empty
>>  * create a 'background' region at negative priority
>> 64 bits wide
>>  * put the alias and the background region into the container
>> The alias is 64 bits wide too, but it is an alias of
>> dma_as->root, which is a container with no background
>> region.
>>  * use the container as the bus_master_enable region
>>  -- all you see in this container is the background region
>> and anyhing that was in dma_as->root.
>
> This confused me even more.
> dma root covers whole 64 bit doesn't it?

It has a size of 64 bits but it doesn't necessarily have
actual memory subregions with I/O operations covering the
whole contiguous range from 0 to 2^64-1 (that would only
happen if the things responding to DMA cover the whole
64 bit address, which corresponds to the situation in hardware
where something will respond to the bus master transaction
for any address. The hardware still has the "if nothing
responds do this" capability, it just never gets used.)

The case where the DMA root is an IOMMU could be
a problem though, because address_space_translate()
hardcodes io_mem_unassigned as the fallback if the
IOMMU says "no, can't write here".

> The doc says:
> "This is done with memory_region_add_subregion_overlap(), which
> allows the region to overlap any other region in the same container, and
> specifies a priority that allows the core to decide which of two regions
> at
> the same address are visible (highest wins)."
>
> So if I create an alias that also covers whole 64 bit
> and background in the same
> container, background with a negative priority,
> won't alias always win?

The alias will win for the addresses it handles. But if
the alias is a container with "holes" then it doesn't handle
the "holes" and the lower priority background region will
get them.

-- PMM