Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses

From: Peter Maydell <peter.maydell@linaro.org>
To: Tom Hanson <thomas.hanson@linaro.org>
Cc: QEMU Developers <qemu-devel@nongnu.org>,
	Grant Likely <grant.likely@hpe.com>
Subject: Re: [Qemu-devel] [PATCH 3/3] target-arm: Comments to mark location of pending work for 56 bit addresses
Date: Fri, 30 Sep 2016 16:24:38 -0700	[thread overview]
Message-ID: <CAFEAcA_dqm6VgUvoTez8Mcu_Tcv9V+Tng46kw7gSsR+ohfW6QA@mail.gmail.com> (raw)
In-Reply-To: <57EEEB2D.7010100@linaro.org>

On 30 September 2016 at 15:46, Tom Hanson <thomas.hanson@linaro.org> wrote:
> On 09/29/2016 07:27 PM, Peter Maydell wrote:
> ...
>>> This work was not done at this time since the changes could not be tested
>>> with current CPU models.  Comments have been added to flag the locations
>>> where this will need to be fixed once a model is available.
>>
>> This is *not* why we haven't done this work. We haven't done it
>> because the maximum virtual address size permitted by the
>> architecture is less than 56 bits, and so this is a "can't happen"
>> situation.
>
> But, in an earlier discussion which we had about the desire to use QEMU
> to test potential new ARM-based architectures with large address spaces
> I suggested that these changes be made now.  You said that the changes
> shouldn't be made because:
>     where there is no supported guest CPU that could use
>     that code, the code shouldn't be there because it's untested
>     and untestable
> Isn't that the same thing I said above?

That's a general statement of principle about what I think we
should or shouldn't write code for in QEMU. In this particular case,
it's true, but the reason it's true isn't just that we don't
currently have any 56 bit-VA CPUs implemented, but because such
a CPU is not permitted by the architecture. That's a stronger
statement and I think it's worth making.

>>> 3 comments added in same file to identify cases in a switch.
>>
>> This should be a separate patch, because it is unrelated to the
>> tagged address stuff.
>
> As part of that same conversation you suggested adding these
> comments rather than making the changes:
>     If we can assert, or failing that have a comment in the place
>     that would be modified anyway for 56 bit addresses then that
>     ought to catch the future case I think.

Yes, I still think this. What does it have to do with adding
"SVC", "HVC", etc comments to the switch cases? Those have
nothing to do with tagged addresses or 56 bit VAs, and should
not be in this patch (though I don't object to them inherently).

thanks
-- PMM