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

[Marcel Apfelbaum <marcel@redhat.com>]

On 11/10/2017 9:35, Mark Cave-Ayland wrote:
> On 01/10/17 22:44, Mark Cave-Ayland wrote:
> 
>> On 28/09/17 08:56, Laszlo Ersek wrote:
>>
>>> On 09/28/17 09:31, Mark Cave-Ayland wrote:
>>>> On 25/09/17 09:11, Dr. David Alan Gilbert wrote:
>>>>
>>>>> * Marcel Apfelbaum (marcel@redhat.com) 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 <mark.cave-ayland@ilande.co.uk>
>>>>>>>>> ---
>>>>>>>>>    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?
>>>
>>> None from my side. (I didn't "object" to begin with :) , I was just
>>> curious about any possible migration impact.)
>>
>> Okay great! I guess I mis-read your query as being a NACK for the patch.
> 
> Hi Marcel,
> 
> Did you manage to get any further with your testing? It appears that
> freeze is coming soon and my remaining changes for the sun4u machine are
> currently blocked on this patch :/
> 

Not yet, but I see your patch is already merged.
I'll let you know if there will be any problems.

Thanks,
Marcel

> 
> ATB,
> 
> Mark.
>