Re: [Qemu-devel] [PATCH] pci: allow 32-bit PCI IO accesses to pass through the PCI bridge

[Mark Cave-Ayland]

On 28/09/17 10:19, Marcel Apfelbaum wrote:

> On 28/09/2017 10:31, Mark Cave-Ayland wrote:
>> On 25/09/17 09:11, Dr. David Alan Gilbert wrote:
>>
>>> * Marcel Apfelbaum (address@hidden) wrote:
>>>> On 23/09/2017 11:23, Mark Cave-Ayland wrote:
>>>>> On 22/09/17 23:18, Laszlo Ersek wrote:
>>>>>
>>>>>> On 09/22/17 14:18, Mark Cave-Ayland wrote:
>>>>>>> Whilst the underlying PCI bridge implementation supports 32-bit
>>>>>>> PCI IO
>>>>>>> accesses, unfortunately they are truncated at the legacy 64K limit.
>>>>>>>
>>>>>>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>>>>>>> ---
>>>>>>>    hw/pci/pci_bridge.c |    3 ++-
>>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
>>>>>>> index 17feae5..a47d257 100644
>>>>>>> --- a/hw/pci/pci_bridge.c
>>>>>>> +++ b/hw/pci/pci_bridge.c
>>>>>>> @@ -379,7 +379,8 @@ void pci_bridge_initfn(PCIDevice *dev, const
>>>>>>> char *typename)
>>>>>>>        sec_bus->address_space_mem = &br->address_space_mem;
>>>>>>>        memory_region_init(&br->address_space_mem, OBJECT(br),
>>>>>>> "pci_bridge_pci", UINT64_MAX);
>>>>>>>        sec_bus->address_space_io = &br->address_space_io;
>>>>>>> -    memory_region_init(&br->address_space_io, OBJECT(br),
>>>>>>> "pci_bridge_io", 65536);
>>>>>>> +    memory_region_init(&br->address_space_io, OBJECT(br),
>>>>>>> "pci_bridge_io",
>>>>>>> +                       UINT32_MAX);
>>>>>>>        br->windows = pci_bridge_region_init(br);
>>>>>>>        QLIST_INIT(&sec_bus->child);
>>>>>>>        QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
>>>>>>>
>>>>>>
>>>>
>>>> Hi Mark,
>>>>
>>>>>> Based on the commit message, I assume this change is
>>>>>> guest-visible. If
>>>>>> so, should it be made dependent on a compat property, so that it
>>>>>> doesn't
>>>>>> cause problems with migration?
>>>>>
>>>>> In order to enable 32-bit IO accesses the PCI bridge needs to set
>>>>> bit 0
>>>>> in the IO_LIMIT and IO_BASE registers - this bit is read-only to
>>>>> guests,
>>>>> so unless a PCI bridge has this bit set then it's impossible for this
>>>>> change to be guest visible.
>>>>>
>>>>> I did a grep for PCI_IO_RANGE_TYPE_32 and didn't see any existing
>>>>> users
>>>>> (other than an upcoming patchset from me!), so this combined with the
>>>>> fact that without this patch the feature is broken makes me think
>>>>> that I
>>>>> am the first user and so existing guests won't have a problem.
>>>>>
>>>>
>>>> (adding Dave for his expertise)
>>>>
>>>> Do you know how the migration code will behave if it will have
>>>> a 65k address space on source and MAX UINT on destination?
>>>> (and the other way around for rolling back)
>>>
>>> Hmm not sure; we don't migrate regions explicitly; just RAMBlocks
>>> and devices that back them.  If the change is visible in the IO
>>> addresses allocated to the PCI devices or in the config space then
>>> it might fail.
>>
>> For reference here is the link to the sun4u patch I posted yesterday
>> that requires this fix if anyone else would like to test:
>> https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg07355.html.
>>
>> Other than that are there any further objections to this patch?
>>
> 
> Since we don't support 32-bit IO accesses (yet) in x86
> and the address space is not visible to guest, it should be OK.
> I'll run a simple migration test to be sure, I'll let you
> know if something goes wrong.

Fantastic! In that case I'll assume that no news is good news :)  Do you
know approximately how long it will be before the patch gets merged? I'm
just trying to get a feel for when I can send the pull request for the
sun4u changes which I'd like to do soon if possible given that it's such
a large change.


ATB,

Mark.