On Sat, 2023-01-07 at 00:35 +0000, Sean Christopherson wrote:
> On Thu, Dec 29, 2022, Robert Hoo wrote:
> > On Wed, 2022-12-28 at 11:37 +0800, Binbin Wu wrote:
> > > On 12/9/2022 12:45 PM, Robert Hoo wrote:
> > > > kvm_vcpu_arch::cr4_guest_owned_bits and
> > > > kvm_vcpu_arch::cr4_guest_rsvd_bits
> > > > looks confusing. Rename latter to cr4_host_rsvd_bits, because
> > > > it in
> > > > fact decribes the effective host reserved cr4 bits from the
> > > > vcpu's
> > > > perspective.
> > >
> > > IMO, the current name cr4_guest_rsvd_bits is OK becuase it shows
> > > that these
> > > bits are reserved bits from the pointview of guest.
> >
> > Actually, it's cr4_guest_owned_bits that from the perspective of
> > guest.
>
> No, cr4_guest_owned_bits is KVM's view of things.
That's all right. Perhaps my expression wasn't very accurate. Perhaps I
would have said "cr4_guest_owned_bits stands on guest's points, as it
reads, guest owns these (set) bits". Whereas, "cr4_guest_rsvd_bits"
doesn't literally as the word reads, its set bits doesn't mean "guest
reserved these bits" but the opposite, those set bits are reserved by
host:
set_cr4_guest_host_mask()
{
...
vcpu->arch.cr4_guest_owned_bits = KVM_POSSIBLE_CR4_GUEST_BITS &
~vcpu->arch.cr4_guest_rsvd_bits;
// cr4_guest_owned_bit = (~host owned bits) &
KVM_POSSIBLE_CR4_GUEST_BITS (the filter)
// cr4_guest_owned_bit and cr4_guest_rsvd_bits are generally opposite
relationship
...
vmcs_writel(CR4_GUEST_HOST_MASK, ~vcpu->arch.cr4_guest_owned_bits);
//the opposite of guest owned bits are effectively written to
CR4_GUEST_HOST_MASK
}
These code are the implementation of SDM 25.6.6 "Guest/Host Masks and
Read Shadows for CR0 and CR4"
"In general, bits set to 1 in a guest/host mask correspond to bits
“owned” by the host."
> It tracks which bits have
> effectively been passed through to the guest and so need to be read
> out of the
> VMCS after running the vCPU.
Yes, as above, ~cr4_guest_owned_bits is effective final guest/host CR4
mask that's written to VMCS.CR4_GUEST_HOST_MASK.
>
> > cr4_guest_owned_bits and cr4_guest_rsvd_bits together looks quite
> > confusing.
>
> I disagree, KVM (and the SDM and the APM) uses "reserved" or "rsvd"
> all over the
> place to indicate reserved bits/values/fields.
I wouldn't object the word "reserved". I was objecting that
"cr4_guest_rsvd_bits" contains guest reserved; it actually contains
"host reserved". ;)
>
> > > > * cr4_reserved_bits --> cr4_kvm_reserved_bits, which describes
>
> Hard no. They aren't just KVM reserved, many of those bits are
> reserved by
> hardware, which is 100% dependent on the host.
That's right. KVM stands on top of HW, then Host, doesn't it? ;)
My interpretation is that, also the theme of this patch, those
xxx_cr4_reserved consts/variables are actually layered relationships.
>
> > > > CR4_HOST_RESERVED_BITS + !kvm_cap_has() = kvm level cr4
> > > > reserved
> > > > bits.
> > > >
> > > > * __cr4_reserved_bits() --> __cr4_calc_reserved_bits(), which
> > > > to
> > > > calc
> > > > effective cr4 reserved bits for kvm or vm level, by
> > > > corresponding
> > > > x_cpu_has() input.
> > > >
> > > > Thus, by these renames, the hierarchical relations of those
> > > > reserved CR4
> > > > bits is more clear.
>
> Sorry, but I don't like any of the changes in this patch. At best,
> some of the
> changes are a wash (neither better nor worse), and in that case the
> churn, however
> minor isn't worth it.
That's all right. Regretful I cannot convince you. This patch just
demonstrates my interpretation when I come confused by the code, then
dig into and SDM reading. Anyway it's not LAM necessity, I'll abandon
this patch in next version.
