More scenario checks by junjiemao1 · Pull Request #14 · junjiemao1/acrn-hypervisor

junjiemao1 · 2022-01-25T01:53:05Z

No description provided.

Today the XML validation logic is embedded in scenario_cfg_gen.py which is highly entangled with the Python-internal representation of configurations. Such representation is used by the current configurator, but will soon be obsolete when the new configurator is introduced. In order to avoid unnecessary work on this internal representation when we refine the schema of scenario XML files, this patch separates the validation logic into a new script which can either be used from the command line or imported in other Python-based applications. At build time this script will be used instead to validate the XML files given by users. This change makes it easier to refine the current configuration items for better developer experience. Migration of existing checks in scenario_cfg_gen.py to XML schema will be done by a following series. v1 -> v2: * Remove "all rights reserved" from the license header * Upgrade the severity of the message indicating lack of xmlschema as error according to our latest definitions of log severities, as validation violations could indicate build time or boot time failures. Tracked-On: projectacrn#6690 Signed-off-by: Junjie Mao <junjie.mao@intel.com>

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

dbkinder · 2022-02-06T23:58:30Z

misc/config_tools/schema/checks/cpu_assignment.xsd

+
+  <xs:assert test="every $vm in /acrn-config/vm satisfies
+                   count(distinct-values($vm//cpu_affinity/pcpu_id)) = count($vm//cpu_affinity/pcpu_id)">
+    <xs:annotation acrn:severity="warning">


Can acrn:severity values be defined somewhere in a schema? Are there any schema validation checks on these values? Does the assert allow a severity indication? What happens if someone misspells a severity, say as acrn:severity="waring"? Is there a default severity?

It is possible to have an XML schema for an XML schema file. I can draft one to ensure correct annotation to the assertions.

I just pushed another commit which adds XML schema files that checks the checker. The script misc/config_tools/schema/checks/check_the_checkers.sh will apply that schema to all the XSD files under misc/config_tools/schema/checks using xmllint.

Of course you need xmllint (which is part of libxml2) for this. I'm not using xmlschema-validate (which is provided by the xmlschema Python module) because that tool does not allow redefinition of XSD elements such as xs:element.

misc/config_tools/schema/checks/cpu_assignment.xsd

misc/config_tools/schema/checks/interrupts.xsd

misc/config_tools/schema/checks/passthrough_devices.xsd

misc/config_tools/schema/checks/vm_types.xsd

Co-authored-by: David Kinder <david.b.kinder@intel.com>

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

amyreye · 2022-02-08T00:07:23Z

misc/config_tools/schema/checks/passthrough_devices.xsd

+  <xs:assert test="every $vm in /acrn-config/vm satisfies
+                   starts-with($vm//vm_type, 'PRE') or not($vm//mmio_resources/p2sb = 'y')">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>VM {$vm/@id} is not a pre-launched VM and cannot have a P2SB (Primary-to-Sideband bridge) passed through to it.  Remove the mmio_resources.p2sb value for this VM.</xs:documentation>


Does this message appear in the context of the ACRN Configurator UI? If so, consider writing the action in that context. For example, instead of "Remove the mmio_resources.p2sb value for this VM" say "Clear this option" (assuming it's a checkbox in the UI). Same comment for all messages.

These documentation can appear in three places:

As error messages in the ACRN configurator.

As build-time errors in terminal.

In web pages on https://projectacrn.github.io/.

Only in the first case will an error be related to one (or more) widget.

I can't tell which messages will be in the UI, but let's consider whether an additional UI message field is needed for those messages.

Let's revisit this once the UI has integrated the validation rules and shown validation errors. Based on what we have at that time, we can see if any message about additional actions is needed.

Sounds good, thanks

misc/config_tools/schema/checks/passthrough_devices.xsd

dbkinder

A few more questions...

dbkinder · 2022-02-09T17:23:04Z

misc/config_tools/schema/checks/check_the_checkers.sh

@@ -0,0 +1,6 @@
+#!/bin/bash


Should put a copyright and SPDX header comment in scripts too.

dbkinder · 2022-02-09T17:23:56Z

misc/config_tools/schema/checks/cpu_assignment.xsd

@@ -0,0 +1,20 @@
+<?xml version="1.0"?>


Do you think we should also put a copyright and SPDX header comment in all our xsd files?

Added to all XSD files for data checks. For the schema XSDs I'll update them the next time I update them (which will happen in a few days).

misc/config_tools/schema/checks/cpu_assignment.xsd

dbkinder · 2022-02-09T19:42:46Z

misc/config_tools/schema/checks/passthrough_devices.xsd

+  <xs:assert test="every $vm in /acrn-config/vm satisfies
+                   starts-with($vm//vm_type, 'PRE') or not($vm//mmio_resources/p2sb = 'y')">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>VM {$vm/@id} is not a pre-launched VM and cannot have a P2SB (Primary-to-Sideband bridge) passed through to it.  Remove the mmio_resources.p2sb value for this VM.</xs:documentation>


Looking at this again, shouldn't this show the name of the VM and not its ID number? All VMs have a name, right?

Will replace with VM names.

dbkinder · 2022-02-09T19:43:20Z

misc/config_tools/schema/checks/passthrough_devices.xsd

+  <xs:assert test="every $vm in /acrn-config/vm satisfies
+                   not($vm//mmio_resources/TPM2 = 'y') or not($vm//mmio_resources/p2sb = 'y')">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>VM {$vm/@id} is assigned both a TPM2 and P2SB (Primary-to-Sideband bridge), which is not a supported configuration. Remove one of these choices.</xs:documentation>


Same comment here (show the name not the ID number)?

dbkinder · 2022-02-09T19:46:21Z

misc/config_tools/schema/checks/vm_types.xsd

+
+  <xs:assert test="count(/acrn-config/vm[vm_type = 'SERVICE_VM']) &lt; 2">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>There can be at most one service VM, but more than one was configured. Verify there is only one VM with  vm_type set to SERVICE_VM. </xs:documentation>


How does one get more than one Service VM Can someone change the vm_type to SERVICE_VM for any VM in the configurator, and this is the assert that will catch that error (eventually)?

Users cannot hit this when using the configurator.

Meanwhile, these checks are also applied at an early stage of building, so that errors in manually-edited XMLs can be caught early. VM type errors could exist in this case.

Good you're thinking of errors that could be introduced by editing the xml file. Have you considered the configurator adding a test to detect manual edits (hash or checksum kind of test)?

dbkinder · 2022-02-09T19:49:33Z

misc/config_tools/schema/checks/vm_types.xsd

+                   every $svm in /acrn-config/vm[vm_type = 'SERVICE_VM'] satisfies
+                   $pre_vm/@id &lt; $svm/@id">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>This pre-launched VM (ID: {$pre_vm/@id}) must have a VM ID value less than the service VM (ID: {$svm/@id}). Adjust the VM ID values to fix this.</xs:documentation>


How does this situation happen? How does the developer change a VM ID in the configurator? I don't think we have a way in the UI do do this? For example, you can't drag and drop a VM tab to move them around, right?

See the response above.

dbkinder · 2022-02-09T19:49:54Z

misc/config_tools/schema/checks/vm_types.xsd

+                   every $svm in /acrn-config/vm[vm_type = 'SERVICE_VM'] satisfies
+                   $post_vm/@id &gt; $svm/@id">
+    <xs:annotation acrn:severity="error">
+      <xs:documentation>This post-launched VM (ID: {$post_vm/@id}) must have a VM ID value greater than the service VM (ID: {$svm/@id}). Adjust the VM ID values to fix this.</xs:documentation>


Same comment as above here.

See the response above.

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

junjiemao1 added 3 commits January 20, 2022 21:04

schema: find and report counter examples on validation error

12e36ac

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

config_tools: implement existing scenario checks in XSD

e0fdf2c

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

dbkinder requested changes Feb 7, 2022

View reviewed changes

junjiemao1 and others added 2 commits February 7, 2022 08:46

Apply suggestions from David

4bd0cce

Co-authored-by: David Kinder <david.b.kinder@intel.com>

schema: add schema of checkers

2ae8de7

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

amyreye reviewed Feb 8, 2022

View reviewed changes

dbkinder requested changes Feb 9, 2022

View reviewed changes

junjiemao1 added 2 commits February 11, 2022 17:09

Use VM names in error descriptions.

b5d8fde

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

add SPDX header

a3be5d2

Signed-off-by: Junjie Mao <junjie.mao@intel.com>

Conversation

junjiemao1 commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants