Re: [PATCH v1 10/12] KVM: arm64: Rework logic to en/decode VTCR_EL2.{SL0, SL2} fields

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 20, 2022 at 09:01:19AM +0000, Ryan Roberts wrote:
> On 20/12/2022 00:06, Oliver Upton wrote:
> > Hi Ryan,
> > 
> > On Tue, Dec 06, 2022 at 01:59:28PM +0000, Ryan Roberts wrote:
> >> In order to support 5 level translation, FEAT_LPA2 introduces the 1-bit
> >> SL2 field within VTCR_EL2 to extend the existing 2-bit SL0 field. The
> >> SL2[0]:SL0[1:0] encodings have no simple algorithmic relationship to the
> >> start levels they represent (that I can find, at least), so replace the
> >> existing macros with functions that do lookups to encode and decode the
> >> values. These new functions no longer make hardcoded assumptions about
> >> the maximum level and instead rely on KVM_PGTABLE_FIRST_LEVEL and
> >> KVM_PGTABLE_LAST_LEVEL.
> >>
> >> This is preparatory work for enabling 52-bit IPA for 4KB and 16KB pages
> >> with FEAT_LPA2.
> >>
> >> No functional change intended.
> >>
> >> Signed-off-by: Ryan Roberts <ryan.roberts@xxxxxxx>
> > 
> > Why do we need to support 5-level paging at stage-2?
> > 
> > A configuration of start_level = 0, T0SZ = 12 with 4K paging would
> > result in 16 concatenated tables at level 0, avoiding the level -1
> > lookup altogether.
> 
> Yes, agreed. And that's exactly what the code does. So we could remove this
> patch from the series and everything would continue to function correctly. But I
> was trying to make things more consistent and maintainable (this now works in
> terms of KVM_PGTABLE_FIRST_LEVEL and KVM_PGTABLE_LAST_LEVEL for example).

My largest concern was the plumbing that was added for setting a start
level of -1 that is effectively dead code. I worry about it because it
can be confusing for newcomers and can be an open invitation to mess
things up later down the line.

> So happy to remove this and replace with a comment describing the limitations if
> that's your preference?

Marc, feel free to put me in line here if I'm not thinking about this
right, but adding support for an unused feature is likely less
maintainable. So, I'd prefer we drop the patch.

--
Thanks,
Oliver
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm



[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux