Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks based on memory region offset

[Alexander Graf]

On 11.09.14 12:14, address@hidden wrote:
>> -----Original Message-----
>> From: Alexander Graf [mailto:address@hidden
>> Sent: Wednesday, September 10, 2014 4:56 PM
>> To: Purcareata Bogdan-B43198; Wood Scott-B07421
>> Cc: Caraman Mihai Claudiu-B02008; address@hidden; address@hidden
>> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region callbacks
>> based on memory region offset
>>
>>
>>
>> On 10.09.14 13:40, address@hidden wrote:
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Friday, September 05, 2014 6:47 PM
>>>> To: Purcareata Bogdan-B43198
>>>> Cc: address@hidden; Caraman Mihai Claudiu-B02008; qemu-
>> address@hidden
>>>> Subject: Re: [Qemu-ppc] [PATCH 2/2] PPC: openpic_kvm: Filter region
>> callbacks
>>>> based on memory region offset
>>>>
>>>> On Wed, 2014-09-03 at 14:36 -0400, Bogdan Purcareata wrote:
>>>>> This is done due to the fact that the kvm-openpic region_{add,del}
>> callbacks
>>>>> can be invoked for sections generated from other memory regions as well.
>>>> These
>>>>> callbacks should handle only requests for the kvm-openpic memory region.
>>>>>
>>>>> The patch fixes a bug on target-ppc occuring when the "e500-pci-bar0"
>> memory
>>>>> region is added. This memory region registers an alias to the "e500-ccsr"
>>>> memory
>>>>> region, which further contains the "kvm-openpic" subregion. Due to this
>>>> alias,
>>>>> the kvm_openpic_region_add is called once more, with an offset within the
>>>>> "e500-pci-bar" memory region. This generates the remapping of the
>>>>> in-kernel MPIC at a wrong offset.
>>>>
>>>> OK, so the problem is that we really do have the MPIC mapped in two
>>>> locations (and the kernel API only lets us map one of them).  It would
>>>> be nice if the MemoryRegionSection struct indicated the alias being
>>>> represented rather than needing to recalculate the non-aliased address.
>>>
>>> Not sure I fully understand the terminology of the alias being represented.
>> In fact, out of the two mentioned in the previous mail, "e500-pci-bar0" and
>> "e500-ccsr", I don't know which one is the "alias".
>>>
>>> If it's the former, thus "e500-pci-bar0" is an alias for "e500-ccsr", this
>> information could be propagated down to the MemoryRegionSection. However,
>> according to [1], "aliases may point to any type of region, including other
>> aliases". So if the MemoryRegionSection has been built by going through a
>> chain of aliases, all this information must be included in the structure.
>>>
>>> If it's the latter, thus "e500-ccsr" is an alias for "e500-pci-bar0", we can
>> get to it in the current model as well. For our specific case, the
>> MemoryRegionSection points to the "kvm-openpic" MemoryRegion, which in turn
>> points to "e500-ccsr" as its parent (container).
>>>
>>> What do you think?
>>
>> I don't think it matters whether a mapping is an alias or not. What your
>> patch really does is it only allows for a single mapping to happen. The
>> first one wins.
>>
>> I think that's reasonable.
>>
>> However, there's no need to extend the memory API with anything here.
>> The only reason you need the additional call is because you need to
>> figure out where you think you were mapped. But since we set up the map
>> ourselves in an ioctl, we already know where we are mapped.
>>
>> How about this patch?
> 
> Yes, this patch fixes the issue and follows a simpler approach.
> 
> Would you like to submit it or should I send a v2 with your changes?

I've sent it as an official patch. Thanks a lot again for digging down
into this!


Alex