Xen devel - Aug 2013 - [PATCH 0/2] Fix SMBios table regressions in HVM guests

The series "HVM firmware passthrough" series in Jan 2013 from Ross
Philipson
cause two regressions for HVM guests which sadly found their way into the Xen
4.3 release.

The first regression causes an incorrect count of tables to be placed in the
main header, and can be seen by running dmidecode in any applicable HVM domain.

The second regression found its way into the public ABI, making it harder to
fix.  However, I believe (and have argued) that the correct fix is to change
the ABI.  I would appreciate thoughts/comments.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Ross Philipson <ross.philipson@citrix.com>

-- 
1.7.10.4

Fixes regression indirectly introduced by c/s 4d23036e709627

That changeset added some smbios tables which were option based on the
toolstack providing appropriate xenstore keys.  The do_struct() macro would
unconditionally increment nr_structs, even if a table was not actually
written.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 tools/firmware/hvmloader/smbios.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/firmware/hvmloader/smbios.c
b/tools/firmware/hvmloader/smbios.c
index 3d5dc51..9f292cc 100644
--- a/tools/firmware/hvmloader/smbios.c
+++ b/tools/firmware/hvmloader/smbios.c
@@ -192,7 +192,8 @@ write_smbios_tables(void *ep, void *start,
 
 #define do_struct(fn) do {                      \
     q = (fn);                                   \
-    (*nr_structs)++;                            \
+    if ( q != p )                               \
+        (*nr_structs)++;                        \
     if ( (q - p) > *max_struct_size )           \
         *max_struct_size = q - p;               \
     p = q;                                      \
-- 
1.7.10.4

The old code for OEM SMBios strings was:

        char path[20] = "bios-strings/oem-XX";
        path[(sizeof path) - 3] = ''0'' + ((i < 10) ? i : i /
10);
        path[(sizeof path) - 2] = (i < 10) ? ''\0'' :
''0'' + (i % 10);

Where oem-1 thru 9 specifically had no leading 0.

However, the definition of HVM_XS_OEM_STRINGS specifically requires leading
0s.

This regression was introduced by the combination of c/s 4d23036e709627 and
e64c3f71ceb662

I realise that this patch causes a change to the public headers.  However I
feel it is justified as:

* All toolstacks used to have to embed the magic string (and almost certainly
  still do)
* If by some miriacle a new toolstack has started using the new define will
  continue to work.
* The only intree consumer of the define is hvmloader itself.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/include/public/hvm/hvm_xs_strings.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/include/public/hvm/hvm_xs_strings.h
b/xen/include/public/hvm/hvm_xs_strings.h
index 4de5881..8aec935 100644
--- a/xen/include/public/hvm/hvm_xs_strings.h
+++ b/xen/include/public/hvm/hvm_xs_strings.h
@@ -75,6 +75,6 @@
 /* 1 to 99 OEM strings can be set in xenstore using values of the form
  * below. These strings will be loaded into the SMBIOS type 11 structure.
  */
-#define HVM_XS_OEM_STRINGS             "bios-strings/oem-%02d"
+#define HVM_XS_OEM_STRINGS             "bios-strings/oem-%d"
 
 #endif /* __XEN_PUBLIC_HVM_HVM_XS_STRINGS_H__ */
-- 
1.7.10.4

On 29/08/2013 20:20, "Andrew Cooper" <andrew.cooper3@citrix.com>
wrote:
> The series "HVM firmware passthrough" series in Jan 2013 from
Ross Philipson
> cause two regressions for HVM guests which sadly found their way into the
Xen
> 4.3 release.
> 
> The first regression causes an incorrect count of tables to be placed in
the
> main header, and can be seen by running dmidecode in any applicable HVM
> domain.
> 
> The second regression found its way into the public ABI, making it harder
to
> fix.  However, I believe (and have argued) that the correct fix is to
change
> the ABI.  I would appreciate thoughts/comments.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Ross Philipson <ross.philipson@citrix.com>
Acked-by: Keir Fraser <keir@xen.org>