Re: [Qemu-devel] [PATCH v3 23/23] pci: honor PCI_COMMAND_MASTER

[Avi Kivity]

On 10/11/2012 10:49 AM, liu ping fan wrote:
> On Thu, Oct 11, 2012 at 4:34 PM, Avi Kivity <address@hidden> wrote:
>> On 10/11/2012 05:38 AM, liu ping fan wrote:
>>> On Wed, Oct 10, 2012 at 12:32 AM, Avi Kivity <address@hidden> wrote:
>>>> Currently we ignore PCI_COMMAND_MASTER completely: DMA succeeds even when
>>>> the bit is clear.
>>>>
>>>> Honor PCI_COMMAND_MASTER by inserting a memory region into the device's
>>>> bus master address space, and tying its enable status to 
>>>> PCI_COMMAND_MASTER.
>>>>
>>>> Tested using
>>>>
>>>>   setpci -s 03 COMMAND=3
>>>>
>>>> while a ping was running on a NIC in slot 3.  The kernel (Linux) detected
>>>> the stall and recovered after the command
>>>>
>>>>   setpci -s 03 COMMAND=7
>>>>
>>>> was issued.
>>>>
>>>> Signed-off-by: Avi Kivity <address@hidden>
>>>> ---
>>>>  hw/pci.c | 13 +++++++++++--
>>>>  hw/pci.h |  1 +
>>>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci.c b/hw/pci.c
>>>> index 8e8e030..7adf61b 100644
>>>> --- a/hw/pci.c
>>>> +++ b/hw/pci.c
>>>> @@ -782,7 +782,11 @@ static PCIDevice *do_pci_register_device(PCIDevice 
>>>> *pci_dev, PCIBus *bus,
>>>>          /* FIXME: Make dma_context_fn use MemoryRegions instead, so this 
>>>> path is
>>>>           * taken unconditionally */
>>>>          /* FIXME: inherit memory region from bus creator */
>>>> -        address_space_init(&pci_dev->bus_master_as, get_system_memory());
>>>> +        memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus 
>>>> master",
>>>> +                                 get_system_memory(), 0,
>>>> +                                 memory_region_size(get_system_memory()));
>>>
>>> Could we achieve that  hiding special mr from some address space?  I
>>> think one approach is to limit the iommu's lookup table, but that is
>>> the guest's willing.  The other one is use dummy-mr to overlap some
>>> piece of region of system_memory.
>>> This method will require changing the render sequence of alias mr.
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 2f68d67..cf67c66 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -499,6 +499,11 @@ static void render_memory_region(FlatView *view,
>>>
>>>      clip = addrrange_intersection(tmp, clip);
>>>
>>> +    /* Render subregions in priority order. */
>>> +    QTAILQ_FOREACH(subregion, &mr->subregions, subregions_link) {
>>> +        render_memory_region(view, subregion, base, clip, readonly);
>>> +    }
>>> +
>>>      if (mr->alias) {
>>>          int128_subfrom(&base, int128_make64(mr->alias->addr));
>>>          int128_subfrom(&base, int128_make64(mr->alias_offset));
>>> @@ -506,11 +511,6 @@ static void render_memory_region(FlatView *view,
>>>          return;
>>>      }
>>>
>>> -    /* Render subregions in priority order. */
>>> -    QTAILQ_FOREACH(subregion, &mr->subregions, subregions_link) {
>>> -        render_memory_region(view, subregion, base, clip, readonly);
>>> -    }
>>> -
>>>
>>
>> I don't really follow.  We don't have any memory region that is both a
>> container and an alias, so this change has no effect.  Can you describe
>> what you intend?
>>
> Suppose iommuA takes over only a subsection of system_memory, and
> forbidden to touch other space. How can we achieve this model after
> adopting this series?

I will soon post patches.  Basically an iommu is just another memory
region, and it can be anywhere in the memory hierarchy except it cannot
be a leaf (like an alias).


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