Re: [PATCH] v2 (Re: cs:21768 causes guest spend more time on boot up)

[Keir Fraser <keir.fraser@eu.citrix.com>]

I suggest to try zapping the entire shared-info page when hvmloader
finishes. There is nothing in there that is useful to keep across hvmloader
and guest OS; zapping will ensure that other flags with rising-edge
semantics such as per-vcpu evtchn selector words get reset; and doing
anything more than zeroing is pointless since e.g., the evtchn_mask array
offset and size is dependent on whether the guest OS is 32-bit or 64-bit. If
hvmloader were to set the mask to all 1s and then boot a 64-bit guest, the
rearranged shared_info would actually mean that hvmloader has set 1s in part
of the 64-bit extended evtchn_pending array!

Just a thought...

 -- Keir

On 22/07/2010 10:01, "Zhang, Jianwu" <jianwu.zhang@intel.com> wrote:

> Hi Tim
> When we set the VCPUS parameter more than 2 in guest configure file, We meet
> this issue again . We try it on cs21837, and the guest os is rhel5u3.
> 
> Thanks 
> Jianwu
> 
> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Tuesday, July 20, 2010 7:17 PM
> To: Tim Deegan
> Cc: Zhang, Yang Z; xen-devel@lists.xensource.com; Zhang, Jianwu; Xu, Jiajun
> Subject: Re: [PATCH] v2 (Re: cs:21768 causes guest spend more time on boot up)
> 
> Ah, this is because you poll the ring and never actually use let alone clear
> the event channel rx port? This looks like a good fix, thanks.
> 
>  -- Keir
> 
> On 20/07/2010 11:38, "Tim Deegan" <Tim.Deegan@eu.citrix.com> wrote:
> 
>> Here's another patch that also unsticks CentOS 5.5 boot for me, and
>> seems safer and saner (even if it turns out that the bug is somewhere
>> else and I'm just perturbing the inputs to some more complex system...)
>> 
>> Cheers,
>> 
>> Tim.
>> 
>> hvmloader: clear the xenbus event-channel when we're done with it.
>> Otherwise a later xenbus client that naively waits for the rising edge
>> could get stuck.
>> 
>> Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>
>> 
>> diff -r c4a83e3cc6b4 tools/firmware/hvmloader/util.c
>> --- a/tools/firmware/hvmloader/util.c Thu Jul 15 18:18:16 2010 +0100
>> +++ b/tools/firmware/hvmloader/util.c Tue Jul 20 11:34:06 2010 +0100
>> @@ -587,10 +587,28 @@
>>      return table;
>>  }
>>  
>> +struct shared_info *get_shared_info(void)
>> +{
>> +    static struct shared_info *shared_info = NULL;
>> +    struct xen_add_to_physmap xatp;
>> +
>> +    if ( shared_info == NULL )
>> +    {
>> +        /* Map shared-info page. */
>> +        xatp.domid = DOMID_SELF;
>> +        xatp.space = XENMAPSPACE_shared_info;
>> +        xatp.idx   = 0;
>> +        xatp.gpfn  = 0xfffff;
>> +        if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>> +            BUG();
>> +        shared_info = (struct shared_info *) (xatp.gpfn << 12);
>> +    }
>> +    return shared_info;
>> +}
>> +
>>  uint16_t get_cpu_mhz(void)
>>  {
>> -    struct xen_add_to_physmap xatp;
>> -    struct shared_info *shared_info = (struct shared_info *)0xfffff000;
>> +    struct shared_info *shared_info = get_shared_info();
>>      struct vcpu_time_info *info = &shared_info->vcpu_info[0].time;
>>      uint64_t cpu_khz;
>>      uint32_t tsc_to_nsec_mul, version;
>> @@ -599,14 +617,6 @@
>>      static uint16_t cpu_mhz;
>>      if ( cpu_mhz != 0 )
>>          return cpu_mhz;
>> -
>> -    /* Map shared-info page. */
>> -    xatp.domid = DOMID_SELF;
>> -    xatp.space = XENMAPSPACE_shared_info;
>> -    xatp.idx   = 0;
>> -    xatp.gpfn  = (unsigned long)shared_info >> 12;
>> -    if ( hypercall_memory_op(XENMEM_add_to_physmap, &xatp) != 0 )
>> -        BUG();
>>  
>>      /* Get a consistent snapshot of scale factor (multiplier and shift). */
>>      do {
>> diff -r c4a83e3cc6b4 tools/firmware/hvmloader/util.h
>> --- a/tools/firmware/hvmloader/util.h Thu Jul 15 18:18:16 2010 +0100
>> +++ b/tools/firmware/hvmloader/util.h Tue Jul 20 11:34:06 2010 +0100
>> @@ -68,6 +68,9 @@
>>  #define pci_writeb(devfn, reg, val) (pci_write(devfn, reg, 1, (uint8_t)
>> val))
>>  #define pci_writew(devfn, reg, val) (pci_write(devfn, reg, 2,
>> (uint16_t)val))
>>  #define pci_writel(devfn, reg, val) (pci_write(devfn, reg, 4,
>> (uint32_t)val))
>> +
>> +/* Get a pointer to the shared-info page */
>> +struct shared_info *get_shared_info(void);
>>  
>>  /* Get CPU speed in MHz. */
>>  uint16_t get_cpu_mhz(void);
>> diff -r c4a83e3cc6b4 tools/firmware/hvmloader/xenbus.c
>> --- a/tools/firmware/hvmloader/xenbus.c Thu Jul 15 18:18:16 2010 +0100
>> +++ b/tools/firmware/hvmloader/xenbus.c Tue Jul 20 11:34:06 2010 +0100
>> @@ -53,14 +53,20 @@
>>             (unsigned long) rings, (unsigned long) event);
>>  }
>>  
>> -/* Reset the xenbus connection so the next kernel can start again.
>> - * We zero out the whole ring -- the backend can handle this, and it's
>> - * not going to surprise any frontends since it's equivalent to never
>> - * having used the rings. */
>> +/* Reset the xenbus connection so the next kernel can start again. */
>>  void xenbus_shutdown(void)
>>  {
>>      ASSERT(rings != NULL);
>> +
>> +    /* We zero out the whole ring -- the backend can handle this, and it's
>> +     * not going to surprise any frontends since it's equivalent to never
>> +     * having used the rings. */
>>      memset(rings, 0, sizeof *rings);
>> +
>> +    /* Clear the xenbus event-channel too */
>> +    get_shared_info()->evtchn_pending[event / sizeof (unsigned long)]
>> +        &= ~(1UL << ((event % sizeof (unsigned long))));
>> +
>>      rings = NULL;
>>  }
>>  
> 
>