RE: [PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX

[address@hidden]

> I think this will be more clear once I get the patch posted (which I haven't 
> started
> writing yet). I'll try to get it posted by tomorrow evening though, since I 
> have
> vacation on Friday.

While Andrew is working on the patch in a hurry, 
I'm sorry, I'll be on vacation for a while starting Friday too,
so my reply will be delayed.

Best regards.


> -----Original Message-----
> From: Andrew Jones <drjones@redhat.com>
> Sent: Wednesday, August 18, 2021 5:58 PM
> To: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>;
> peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu A64FX
> 
> On Wed, Aug 18, 2021 at 08:29:15AM +0000, ishii.shuuichir@fujitsu.com wrote:
> >
> > We appreciate everyone's comments.
> > Before making the V5 patch, please let me check the patch contents.
> >
> > > This looks reasonable to me, but you also need the 'sve' property
> > > that states sve in supported at all.
> > > > > So maybe we should just go ahead and add all sve* properties,
> >
> > In response to the above comment,
> > We understood that the sve property will be added to the v4 patch.
> >
> > i.e.
> > (QEMU) query-cpu-model-expansion type=full model={"name":"a64fx"}
> > {"return": {"model": {"name": "a64fx", "props": {"sve128": false,
> > "sve256": true, "sve": true, "sve512": true, "aarch64": true, "pmu":
> > true}}}}
> >
> > > > > but
> > > > > then make sure the default vq map is correct.
> >
> > Furthermore, We understood that I need to add the above process as well, is
> that correct?
> >
> > > That's a good idea. I'll send a patch with your suggested-by.
> >
> > If that's correct,
> > In the current v4 patch, in the aarch64_a64fx_initfn function, the
> > a64fx_cpu_set_sve function is executed to set the SVE property, and
> > the arm_cpu_sve_finalize function is not called.
> >
> > In which function is it appropriate to execute the modulo max_vq
> > function (or equivalent process)?
> >
> > If We are not understanding you correctly, We would appreciate your
> > comments.
> 
> Richard's suggestion is to generalize the "supported" bitmap concept, which is
> currently only used for KVM, in order to also use it for TCG cpu models. The 
> 'max'
> cpu type will have the trivial all-set supported bitmap, but the a64fx will 
> have a
> specific one. I plan to do this "supported" bitmap generalization and apply 
> it to the
> TCG max cpu type. You'll need to rebase this series on those patches and 
> provide
> the a64fx supported bitmap.
> 
> I think this will be more clear once I get the patch posted (which I haven't 
> started
> writing yet). I'll try to get it posted by tomorrow evening though, since I 
> have
> vacation on Friday.
> 
> Thanks,
> drew
> 
> 
> >
> > Best regards.
> >
> > > -----Original Message-----
> > > From: Andrew Jones <drjones@redhat.com>
> > > Sent: Wednesday, August 18, 2021 1:28 AM
> > > To: Richard Henderson <richard.henderson@linaro.org>
> > > Cc: Ishii, Shuuichirou/石井 周一郎 <ishii.shuuichir@fujitsu.com>;
> > > peter.maydell@linaro.org; qemu-arm@nongnu.org; qemu-devel@nongnu.org
> > > Subject: Re: [PATCH v4 1/3] target-arm: Add support for Fujitsu
> > > A64FX
> > >
> > > On Tue, Aug 17, 2021 at 05:53:34AM -1000, Richard Henderson wrote:
> > > > On 8/17/21 5:36 AM, Andrew Jones wrote:
> > > > > On Tue, Aug 17, 2021 at 05:23:17AM -1000, Richard Henderson wrote:
> > > > > > On 8/17/21 1:56 AM, Andrew Jones wrote:
> > > > > > > I guess it's fine. You could easily create a new
> > > > > > > cpu_arm_set_sve_vq() which would forbid changing the
> > > > > > > properties if you wanted to, but then we need to answer
> > > > > > > Peter's question in order to see if there's a precedent for that 
> > > > > > > type of
> property.
> > > > > >
> > > > > > I don't see the point in read-only properties.  If the user
> > > > > > wants to set non-standard values on the command-line, let
> > > > > > them.  What is most important is getting the correct default from 
> > > > > > '-cpu
> a64fx'.
> > > > > >
> > > > >
> > > > > So maybe we should just go ahead and add all sve* properties,
> > > > > but then make sure the default vq map is correct.
> > > >
> > > > I think that's the right answer.
> > > >
> > > > Presently we have a kvm_supported variable that's initialized by
> > > > kvm_arm_sve_get_vls().  I think we want to rename that variable
> > > > and provide a version of that function for tcg. Probably we should
> > > > have done that before, with a trivial function for -cpu max to set all 
> > > > bits.
> > > >
> > > > Then eliminate most of the other kvm_enabled() checks in
> > > > arm_cpu_sve_finalize.  I think the only one we keep is the last,
> > > > where we verify that the final sve_vq_map matches kvm_enabled
> > > > exactly, modulo
> > > max_vq.
> > > >
> > > > This should minimize the differences in behaviour between tcg and kvm.
> > >
> > > That's a good idea. I'll send a patch with your suggested-by.
> > >
> > > Thanks,
> > > drew
> >