Re: Kernel requires x86-64 CPU, after modifying arch_shared_info struct

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jun 29, 2020 at 09:56:43AM +0000, Jan Ruh wrote:
> > Did you try backing up your changes and seeing if the same happens?
> 
> If backing up my changes everything works as expected.
> 
> > Did you rebuild both the Xen hypervisor and the tools?
> 
> Yes, I rebuild both Xen hypervisor and the tools.
> 
> > Pasting your xl config file would be helpful in order to help debug.
> 
> As requested my xl config:

xl parser will just ignore the ';', you can remove them.

>     type="hvm"; extra="console=hvc0 earlyprintk=xen";
>     kernel="/usr/lib/kernel/vmlinuz-domu";
>     ramdisk="/usr/lib/kernel/initrd.img-domu";
>     root="/dev/xvda2 ro";
>     memory=1024;
>     autoballoon="off";

autoballoon is not a xl.cfg option.

>     xen_platform_pci=1;
>     pae=1; acpi=1; apic=1;

All those are already enabled by default, no need to specify them
here.

>     vcpus=1;
>     name="vm1";
>     disk=["file:domu.img,hda,w"];
>     vif=["bridge=xenbr0"];
>     vfb=["type=vnc,keymap=de"];
>     vnclisten="192.168.2.4:0";
>     boot="c";'
> 
> > Posting your changes might also help spot something wonky.
> 
> diff --git a/xen/include/public/arch-x86/xen.h 
> b/xen/include/public/arch-x86/xen.h
> index 629cb2ba40..61c46504a5 100644
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -265,6 +265,14 @@ struct arch_shared_info {
>      /* There's no room for this field in the generic structure. */
>      uint32_t wc_sec_hi;
>  #endif
> +
> +    uint32_t st_version;
> +    uint64_t time_sec;
> +    uint64_t time_nsec;
> +    uint64_t cycle_last;
> +    uint32_t mult;
> +    uint32_t shift;
> +
>  };
>  typedef struct arch_shared_info arch_shared_info_t;
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index c39fbe50a0..2782cb5127 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1254,15 +1254,15 @@ void update_domain_synctime(struct domain *d)

This doesn't seem to exist in current Xen code, so I guess there are
further changes applied here?

>  {
>      uint32_t *st_version;
> 
> +    st_version = &shared_info(d, arch.st_version);
>      *st_version = version_update_begin(*st_version);
>      smp_wmb();
> 
> +    shared_info(d, arch.mult)        = global_time.mult;
> +    shared_info(d, arch.shift)       = global_time.shift;
> +    shared_info(d, arch.cycle_last)  = global_time.cycle_last;
> +    shared_info(d, arch.time_sec)    = global_time.time_sec;
> +    shared_info(d, arch.time_nsec)   = global_time.time_nsec;
> 
>      smp_wmb();
>      *st_version = version_update_end(*st_version);
> 
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 72e7d33708..4b9ad0261b 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -503,22 +503,22 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>      }
>      case XEN_SYSCTL_adjust_gtime:
>      {
> +        struct adjust_gtime *adjust_gtime = (struct adjust_gtime*) 
> &op->u.adjust_gtime;
> 
> +        ret = do_adj_gtime(adjust_gtime);

Same with do_adj_gtime, I cannot find it in the current code, hence
I'm afraid it's impossible to tell what it's actually doing.

> +
>          copyback = 1;
> 
>          break;
> 
> diff --git a/tools/include/xen-foreign/reference.size 
> b/tools/include/xen-foreign/reference.size
> index bb2ada32fb..9afd11e5fa 100644
> --- a/tools/include/xen-foreign/reference.size
> +++ b/tools/include/xen-foreign/reference.size
> @@ -9,6 +9,6 @@ vcpu_guest_context        |     344     344    2800    5168
>  arch_vcpu_info            |       0       0      24      16
>  vcpu_time_info            |      32      32      32      32
>  vcpu_info                 |      48      48      64      64
> -arch_shared_info          |       0       0      28      48
> +arch_shared_info          |       0       0      64      88

Aren't you missing a line below that contains the shared_info size,
and that also need to be updated (since arch_shared_info is contained
in shared_info)?

> 
> 
> global_time is a static struct in time.c, no existing Xen code is changed, 
> just functions added that are being called from the sysctl adjust_gtime.
> 
> Further tests show that in /tools/libxc/xc_dom_binloader.c: 
> xc_dom_parse_bin_kernel xc sets the dom->guest_type to "xen-3.0-x86_32" 
> instead of "xen-3.0-x86_64". I also cannot see though how it can be connected 
> to my change to arch_shared_info.

Hm, I think guest type should be hvm-3.0-x86_32, as xen-* are PV guest
types, and you are trying to boot a HVM guest.

Anyway I'm not familiar with HVM direct kernel boot, so this might
have no effect here.

Are you sure the type is set to "xen-3.0-x86_64" prior to your
changes?

Maybe it would be worth to also paste the output of 'xl -vvv create
...'.

> Sorry for the banner, I always forget that the mail client adds that thing, I 
> hope it doesn't do it again.

I'm afraid it's still appended to this email, see below.

Roger.

> CONFIDENTIALITY: The contents of this e-mail are confidential and
> intended only for the above addressee(s). If you are not the
> intended recipient, or the person responsible for delivering it to
> the intended recipient, copying or delivering it to anyone else or
> using it in any unauthorized manner is prohibited and may be
> unlawful. If you receive this e-mail by mistake, please notify the
> sender and the systems administrator at straymail@xxxxxxxxxx
> immediately.