Re: [PATCH 2/4] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 4350 bytes --]

On Thu, 30 Oct 2025, Mark Cave-Ayland wrote:
> On 29/10/2025 13:25, BALATON Zoltan wrote:
>> On Wed, 29 Oct 2025, BALATON Zoltan wrote:
>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>> On 2025/10/29 6:28, BALATON Zoltan wrote:
>>>>> On Wed, 29 Oct 2025, Akihiko Odaki wrote:
>>>>>> On 2025/10/28 21:59, BALATON Zoltan wrote:
>>>>>>> On Tue, 28 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>> On 27/10/25 20:47, BALATON Zoltan wrote:
>>>>>>>>> On Mon, 27 Oct 2025, Philippe Mathieu-Daudé wrote:
>>>>>>>>>> On 25/10/25 01:31, BALATON Zoltan wrote:
>>>>>>>>>>> These memory windows are a result of the address decoding in the
>>>>>>>>>>> Articia S north bridge so better model it there and not in board 
>>>>>>>>>>> code.
>>>>>>>>>>> 
>>>>>>>>>>> Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>>>>>>>> ---
>>>>>>>>>>>   hw/pci-host/articia.c | 15 ++++++++++++++-
>>>>>>>>>>>   hw/ppc/amigaone.c     | 28 +++++-----------------------
>>>>>>>>>>>   hw/ppc/pegasos2.c     | 13 -------------
>>>>>>>>>>>   3 files changed, 19 insertions(+), 37 deletions(-)
>>>>>>>>>> 
>> [...]
>>> It looks like we won't be able to come to an agreement before the freeze 
>>> and I don't have time now to change this patch but don't want to miss the 
>>> release with this series that finishes pegasos renaming because of this. 
>>> So for this patch I'd say since this is already how it is now and it does 
>>> not make it worse and this object is not user creatable anyway so cannot 
>>> leak please take it as it is and we'll do a clean up later after we finish 
>>> discussion.
>> 
>> As for all of these files I'm the maintainer let me make an executive 
>> decision here to keep this patch without Philippe's reviewed-by for now to 
>> be able to move on with this series before the freeze. Fixing the 
>> theoretical leak can be done on top and since that's a fix it can be done 
>> during soft freeze that would give us more time. So Harsh please go ahead 
>> and merge this series too if there are no other concerns. I'll then address 
>> this later together with other similar issues elsewhere.
>
> I think the issue here is that several people have now raised concerns as to 
> the way you are trying to use MemoryRegions (both here and in the raven 
> series): simply put, if reviewers didn't see any problems with this approach, 
> your series would already have review tags.

I got that and did not mean to ignore those comments forever.

> If you want to suggest a different approach to managing MemoryRegions, then I 
> would suggest you send a proposal to the mailing list so it can be reviewed 
> by the QOM and memory people (along with updated code style documentation so

That's what I plan to do but got no time right now.

> we can all use it). But with freeze coming up in a few days this is certainly 
> out of scope for 10.2.

That's why I proposed to take this patch for now as it only moves existing 
code to another place so it does not introduce a new problem that's not 
already there. And the problem is theoretical as it does not cause a leak 
in this case and nobody raised concerns so far and it was only noticed by 
this patch making it clearer removing duplicated code. So I still think 
this patch has merit, it helped to find a bug and would make code simpler 
without making it worse so it could be taken for that. But I'm OK with 
dropping it too, I'll include it again in the series with the other 
proposed changes to fix the problem.

> For now I would suggest the easiest way to get review tags is to stick with 
> the existing inline MemoryRegion approach for 10.2 to aid getting your 
> patches merged: it's simply not fair to put time pressure on Harsh to merge 
> patches in this way.

I'm not putting time pressure on Harsh, the coming freeze does and I don't 
have time right now to change patches only later but I don't want to miss 
a release again. I've already submitted these pegasos1 emulation series 
for the previous release but due to missing maintainer it was missed then. 
I think it's now resolved by dropping this patch and taking the rest of 
the series which is fine with me. Thanks everybody for the reviews and 
help. I'll prepare patches to resolve concerns when I'll have time again.

Regards,
BALATON Zoltan