XenPvhDetected() can be used to figure out if OVMF has started via the
Xen PVH entry point.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/XenPlatformPei/Platform.h |  5 +++++
 OvmfPkg/XenPlatformPei/Xen.c      | 13 +++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
index 4a80057bdc..db9a62572f 100644
--- a/OvmfPkg/XenPlatformPei/Platform.h
+++ b/OvmfPkg/XenPlatformPei/Platform.h
@@ -99,6 +99,11 @@ XenHvmloaderDetected (
   VOID
   );
 
+BOOLEAN
+XenPvhDetected (
+  VOID
+  );
+
 VOID
 AmdSevInitialize (
   VOID
diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
index 29b42b746c..71fe5de446 100644
--- a/OvmfPkg/XenPlatformPei/Xen.c
+++ b/OvmfPkg/XenPlatformPei/Xen.c
@@ -214,6 +214,19 @@ XenHvmloaderDetected (
   return (mXenHvmloaderInfo != NULL);
 }
 
+BOOLEAN
+XenPvhDetected (
+  VOID
+  )
+{
+  //
+  // This function should only be used after XenConnect
+  //
+  ASSERT (mXenInfo.VersionMajor != 0);
+
+  return mXenHvmloaderInfo == NULL;
+}
+
 VOID
 XenPublishRamRegions (
   VOID
-- 
Anthony PERARD


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#44550): https://edk2.groups.io/g/devel/message/44550
Mute This Topic: https://groups.io/mt/32644077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Mon, Jul 29, 2019 at 04:39:29PM +0100, Anthony PERARD wrote:
> XenPvhDetected() can be used to figure out if OVMF has started via the
> Xen PVH entry point.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1689
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks, I've got a comment, but it can be fixed afterwards if required.

> ---
>  OvmfPkg/XenPlatformPei/Platform.h |  5 +++++
>  OvmfPkg/XenPlatformPei/Xen.c      | 13 +++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/OvmfPkg/XenPlatformPei/Platform.h b/OvmfPkg/XenPlatformPei/Platform.h
> index 4a80057bdc..db9a62572f 100644
> --- a/OvmfPkg/XenPlatformPei/Platform.h
> +++ b/OvmfPkg/XenPlatformPei/Platform.h
> @@ -99,6 +99,11 @@ XenHvmloaderDetected (
>    VOID
>    );
>  
> +BOOLEAN
> +XenPvhDetected (
> +  VOID
> +  );
> +
>  VOID
>  AmdSevInitialize (
>    VOID
> diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> index 29b42b746c..71fe5de446 100644
> --- a/OvmfPkg/XenPlatformPei/Xen.c
> +++ b/OvmfPkg/XenPlatformPei/Xen.c
> @@ -214,6 +214,19 @@ XenHvmloaderDetected (
>    return (mXenHvmloaderInfo != NULL);
>  }
>  
> +BOOLEAN
> +XenPvhDetected (
> +  VOID
> +  )
> +{
> +  //
> +  // This function should only be used after XenConnect
> +  //
> +  ASSERT (mXenInfo.VersionMajor != 0);

That's IMO dangerous. Using the version as an indication that
XenConnect has run seems like a bad idea, since returning a major
version of 0 is a valid number to return. Can't you check against
something else that doesn't depends on hypervisor provided data? (ie:
like some allocations or such that happen in XenConnect)

A paranoid could provider could even return major == 0 and minor == 0
in order to attempt to hide the Xen version used, since guests are not
supposed to infer anything from the Xen version, available hypervisor
features are reported by other means.

Thanks, Roger.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45010): https://edk2.groups.io/g/devel/message/45010
Mute This Topic: https://groups.io/mt/32644077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, Aug 07, 2019 at 05:03:46PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:29PM +0100, Anthony PERARD wrote:
> > +BOOLEAN
> > +XenPvhDetected (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // This function should only be used after XenConnect
> > +  //
> > +  ASSERT (mXenInfo.VersionMajor != 0);
> 
> That's IMO dangerous. Using the version as an indication that
> XenConnect has run seems like a bad idea, since returning a major
> version of 0 is a valid number to return. Can't you check against
> something else that doesn't depends on hypervisor provided data? (ie:
> like some allocations or such that happen in XenConnect)
> 
> A paranoid could provider could even return major == 0 and minor == 0
> in order to attempt to hide the Xen version used, since guests are not
> supposed to infer anything from the Xen version, available hypervisor
> features are reported by other means.

I'm sure a paranoid provider wouldn't use a debug build of OVMF :-). So
that assert doesn't matter. There's nothing dangerous in a `nop'! :-D

But I could use mXenInfo.HyperPages instead.

Thanks,

-- 
Anthony PERARD

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45156): https://edk2.groups.io/g/devel/message/45156
Mute This Topic: https://groups.io/mt/32644077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, Aug 08, 2019 at 11:38:13AM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 05:03:46PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:29PM +0100, Anthony PERARD wrote:
> > > +BOOLEAN
> > > +XenPvhDetected (
> > > +  VOID
> > > +  )
> > > +{
> > > +  //
> > > +  // This function should only be used after XenConnect
> > > +  //
> > > +  ASSERT (mXenInfo.VersionMajor != 0);
> > 
> > That's IMO dangerous. Using the version as an indication that
> > XenConnect has run seems like a bad idea, since returning a major
> > version of 0 is a valid number to return. Can't you check against
> > something else that doesn't depends on hypervisor provided data? (ie:
> > like some allocations or such that happen in XenConnect)
> > 
> > A paranoid could provider could even return major == 0 and minor == 0
> > in order to attempt to hide the Xen version used, since guests are not
> > supposed to infer anything from the Xen version, available hypervisor
> > features are reported by other means.
> 
> I'm sure a paranoid provider wouldn't use a debug build of OVMF :-). So
> that assert doesn't matter. There's nothing dangerous in a `nop'! :-D
> 
> But I could use mXenInfo.HyperPages instead.

It's just a nit, and TBH it's quite unlikely for anyone to report a
major version of 0, it's just that if you have something else to
assert for initialization it might be safer.

Thanks, Roger.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45159): https://edk2.groups.io/g/devel/message/45159
Mute This Topic: https://groups.io/mt/32644077/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-