On Wed, Jul 21, 2021 at 12:42:52PM +0530, Praveen Kumar wrote:

On 21-07-2021 09:40, Michael Kelley wrote:

quoted

From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 9:29 AM

quoted

On Tue, Jul 20, 2021 at 04:20:44PM +0000, Michael Kelley wrote:

quoted

From: Wei Liu <wei.liu@kernel.org> Sent: Tuesday, July 20, 2021 6:35 AM

quoted

On Tue, Jul 20, 2021 at 06:55:56PM +0530, Praveen Kumar wrote:
[...]

quoted

quoted

quoted

+	if (hv_root_partition &&
+	    ms_hyperv.features & HV_MSR_APIC_ACCESS_AVAILABLE) {

Is HV_MSR_APIC_ACCESS_AVAILABLE a root only flag? Shouldn't non-root
kernel check this too?

Yes, you are right. Will update this in v2. thanks.

Please split adding this check to its own patch.

Ideally one patch only does one thing.

Wei.

I was just looking around in the Hyper-V TLFS, and I didn't see
anywhere that the ability to set up a VP Assist page is dependent
on HV_MSR_APIC_ACCESS_AVAILABLE.  Or did I just miss it?

The feature bit Praveen used is wrong and should be fixed.

Per internal discussion this is gated by the AccessIntrCtrlRegs bit.

Wei.

The AccessIntrCtrlRegs bit *is* HV_MSR_APIC_ACCESS_AVAILABLE.
Both are defined as bit 4 of the Partition Privilege flags.  :-)   I don't
know why the names don't line up.   Even so, it's not clear to me that
AccessIntrCtrlRegs has any bearing on the VP Assist page.  I see this
description of AccessIntrCtrlRegs:

Yup, what I understood as well, this is the one required one for Partition Privilege Flags (4th bit), however, cannot comment on the naming convention.

     5 /* Virtual APIC assist and VP assist page registers available */
     4 #define HV_MSR_APIC_ACCESS_AVAILABLE            BIT(4)

Urgh, okay. It is my fault for not reading the code closely. Sorry for
the confusion.

quoted

The partition has access to the synthetic MSRs associated with the
APIC (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR).
If this flag is cleared, accesses to these MSRs results in a #GP fault if
the MSR intercept is not installed.

As per what I also understood from the TLFS doc,that we let partition
access the MSR and do a fault.  However, the point is, does it make
sense to allocate page for vp assist and perform action which is meant
to fail when the flag is cleared ?

Like Michael said, there are some other things that are not tied to that
particular bit. We should get more clarity on what gates what.  Perhaps
that privilege bit only controls access to the EOI assist bit and the
other things in the VP assist page are gated by other privilege bits.
This basically means we should setup the page when there is at least one
thing in that page can be used.

This is mostly an orthogonal issue from the one we want to fix. In
the interest of making progress we can drop the new check for now and
just add a root specific path for setting up and tearing down the VP
assist pages.

How does that sound?

Wei.

quoted

But maybe you have additional info that applies to the root
partition that is not in the TLFS.

As per what discussed internally and I understood, the root partition
shares the vp assist page provided by hypervisor and its read only for
Root kernel.

quoted

Michael

Regards,

~Praveen.