Re: [Xen-devel] [PATCH V6] vm_event: Allow subscribing to write events for specific MSR-s

[Razvan Cojocaru]

On 04/29/16 05:44, Tian, Kevin wrote:
>> From: Razvan Cojocaru [mailto:rcojoc...@bitdefender.com]
>> Sent: Wednesday, April 27, 2016 3:48 PM
>> +
>> +static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
>> +{
>> +ASSERT(d->arch.monitor_msr_bitmap && msr);
>> +
>> +switch ( *msr )
>> +{
>> +case 0 ... 0x1fff:
>> +BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->low) * 8 <
>> 0x1fff);
>> +return >arch.monitor_msr_bitmap->low;
>> +
>> +case 0x4000 ... 0x40001fff:
>> +BUILD_BUG_ON(
>> +ARRAY_SIZE(d->arch.monitor_msr_bitmap->hypervisor) * 8 < 
>> 0x1fff);
>> +*msr &= 0x1fff;
>> +return >arch.monitor_msr_bitmap->hypervisor;
> 
> I think we shouldn't change '*msr' directly here. Since later, 
> 
>> +
>> +case 0xc000 ... 0xc0001fff:
>> +BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->high) * 8 <
>> 0x1fff);
>> +*msr &= 0x1fff;
>> +return >arch.monitor_msr_bitmap->high;
>> +
>> +default:
>> +return NULL;
>> +}
>> +}
>> +
>> +static int monitor_enable_msr(struct domain *d, u32 msr)
>> +{
>> +u32 *bitmap;
>> +
>> +if ( !d->arch.monitor_msr_bitmap )
>> +return -ENXIO;
>> +
>> +bitmap = monitor_bitmap_for_msr(d, );
>> +
>> +if ( !bitmap )
>> +return -EINVAL;
>> +
>> +__set_bit(msr, bitmap);
>> +
>> +hvm_enable_msr_interception(d, msr);
> 
> Here the original msr value is expected.

Thanks for catching that. You're right. I didn't catch that while
testing because the only interesting MSR-s (for introspection) which
could also be disabled (and thus needed the explicit interception
enabling) were below 0x1fff, and so no problems occured (although of
course the patch was also enabling interception for a few other MSR-s
below 0x1fff which shouldn't have been intercepted).

I'll leave monitor_bitmap_for_msr() as it is, but will cache the old
value there and pass that to hvm_enable_msr_interception().


Thanks,
Razvan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] how to set up a #VE

[Big Strong]

>
> You can always just add a new page to the domain to be used for #VE.

It's there a method to directly assign physical pages to guest from dom0?
Using xc_map_foreign_address just like libvmi?

2016-04-28 23:07 GMT+08:00 Tamas K Lengyel :

>
>
> On Thu, Apr 28, 2016 at 8:36 AM, Big Strong  wrote:
>
>> I want to set up an EPT page so as to trigger the #VE for testing
>> purpose. However, some problems are met.
>>
>> As the Intel Manual said, there are many conditions to trigger a #VE:
>>
>> a)  If an access to a guest-physical address causes an EPT
>> violation, bit 63 (0) of exactly one of the EPT paging-structure entries
>> used to translate that address is used to determine *whether the EPT
>> violation is convertible*: either an entry that is not present (if the
>> guest-physical address does not translate to a physical address) or an
>> entry that maps a page (if it does).
>>
>> b)  A convertible EPT violation instead causes a virtualization
>> exception if the following all hold:
>>
>> • CR0.PE = 1;
>>
>> • the logical processor is not in the process of delivering an event
>> through the IDT; and
>>
>> • the 32 bits at offset 4 in the virtualization-exception information
>> area are all 0.
>> In xc_altp2m.c, there is a function xc_altp2m_set_vcpu_enable_notify
>> which is used to set up the #VE information area. However, as the arguments
>> gfn is a physical address (of the guest?), how can I safely assign an
>> unused physical memory space to store #VE info?
>>
>
> You can always just add a new page to the domain to be used for #VE.
>
>
>>
>> Besides, there is no xenctrl interface for setting the suprress_ve bit
>> (63) of the EPT PTE, which is needed to trigger #VE. Even though I can set
>> that with ept_set_entry function, this is an internal function of Xen and
>> unavailble to dom0.
>>
>
> It's undocumented enough (and it took me a bit to find as well) but if you
> use xc_altp2m_set_mem_access and have used xc_altp2m_set_vcpu_enable_notify
> before, then those EPT PTE entries will be converted to #VE automatically.
>
> Tamas
>
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] linux-next: manual merge of the xen-tip tree with the tip tree

[Stephen Rothwell]

Hi all,

Today's linux-next merge of the xen-tip tree got a conflict in:

  drivers/firmware/efi/arm-runtime.c

between commit:

  14c43be60166 ("efi/arm*: Drop writable mapping of the UEFI System table")

from the tip tree and commit:

  21c8dfaa2327 ("Xen: EFI: Parse DT parameters for Xen specific UEFI")

from the xen-tip tree.

I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc drivers/firmware/efi/arm-runtime.c
index 17ccf0a8787a,ac609b9f0b99..
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@@ -109,24 -90,41 +110,30 @@@ static int __init arm_enable_runtime_se
  
pr_info("Remapping and enabling EFI services.\n");
  
 -  mapsize = memmap.map_end - memmap.map;
 -  memmap.map = (__force void *)ioremap_cache(memmap.phys_map,
 - mapsize);
 -  if (!memmap.map) {
 -  pr_err("Failed to remap EFI memory map\n");
 -  return -ENOMEM;
 -  }
 -  memmap.map_end = memmap.map + mapsize;
 -  efi.memmap = 
 +  mapsize = efi.memmap.map_end - efi.memmap.map;
  
 -  efi.systab = (__force void *)ioremap_cache(efi_system_table,
 - sizeof(efi_system_table_t));
 -  if (!efi.systab) {
 -  pr_err("Failed to remap EFI System Table\n");
 +  efi.memmap.map = memremap(efi.memmap.phys_map, mapsize, MEMREMAP_WB);
 +  if (!efi.memmap.map) {
 +  pr_err("Failed to remap EFI memory map\n");
return -ENOMEM;
}
 -  set_bit(EFI_SYSTEM_TABLES, );
 +  efi.memmap.map_end = efi.memmap.map + mapsize;
  
-   if (!efi_virtmap_init()) {
-   pr_err("UEFI virtual mapping missing or invalid -- runtime 
services will not be available\n");
-   return -ENOMEM;
+   if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) {
+   /* Set up runtime services function pointers for Xen Dom0 */
+   xen_efi_runtime_setup();
+   } else {
+   if (!efi_virtmap_init()) {
 -  pr_err("No UEFI virtual mapping was installed -- 
runtime services will not be available\n");
++  pr_err("UEFI virtual mapping missing or invalid -- 
runtime services will not be available\n");
+   return -ENOMEM;
+   }
+ 
+   /* Set up runtime services function pointers */
+   efi_native_runtime_setup();
}
  
-   /* Set up runtime services function pointers */
-   efi_native_runtime_setup();
set_bit(EFI_RUNTIME_SERVICES, );
  
 -  efi.runtime_version = efi.systab->hdr.revision;
 -
return 0;
  }
  early_initcall(arm_enable_runtime_services);

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/hvm: Add debug exception vm_events

[Tamas K Lengyel]

On Thu, Apr 28, 2016 at 8:29 PM, Tian, Kevin  wrote:

> > From: Tamas K Lengyel [mailto:ta...@tklengyel.com]
> > Sent: Thursday, April 28, 2016 4:33 AM
> >
> > Since in-guest debug exceptions are now unconditionally trapped to Xen,
> adding
> > a hook for vm_event subscribers to tap into this new always-on guest
> event. We
> > rename along the way hvm_event_breakpoint_type to hvm_event_type to
> better
>
> I don't quite understand above renaming. How do you define 'event' here?
> There are other functions like hvm_event_msr, hvm_event_cr, etc. The
> new name hvm_event_type looks need to cover all those paths too?
>

Thanks, already changed the name for v2 to be hvm_monitor_debug (with a
precursor patch renaming hvm/event to hvm/monitor).

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V6] vm_event: Allow subscribing to write events for specific MSR-s

[Tian, Kevin]

> From: Razvan Cojocaru [mailto:rcojoc...@bitdefender.com]
> Sent: Wednesday, April 27, 2016 3:48 PM
> +
> +static void *monitor_bitmap_for_msr(struct domain *d, u32 *msr)
> +{
> +ASSERT(d->arch.monitor_msr_bitmap && msr);
> +
> +switch ( *msr )
> +{
> +case 0 ... 0x1fff:
> +BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->low) * 8 <
> 0x1fff);
> +return >arch.monitor_msr_bitmap->low;
> +
> +case 0x4000 ... 0x40001fff:
> +BUILD_BUG_ON(
> +ARRAY_SIZE(d->arch.monitor_msr_bitmap->hypervisor) * 8 < 0x1fff);
> +*msr &= 0x1fff;
> +return >arch.monitor_msr_bitmap->hypervisor;

I think we shouldn't change '*msr' directly here. Since later, 

> +
> +case 0xc000 ... 0xc0001fff:
> +BUILD_BUG_ON(ARRAY_SIZE(d->arch.monitor_msr_bitmap->high) * 8 <
> 0x1fff);
> +*msr &= 0x1fff;
> +return >arch.monitor_msr_bitmap->high;
> +
> +default:
> +return NULL;
> +}
> +}
> +
> +static int monitor_enable_msr(struct domain *d, u32 msr)
> +{
> +u32 *bitmap;
> +
> +if ( !d->arch.monitor_msr_bitmap )
> +return -ENXIO;
> +
> +bitmap = monitor_bitmap_for_msr(d, );
> +
> +if ( !bitmap )
> +return -EINVAL;
> +
> +__set_bit(msr, bitmap);
> +
> +hvm_enable_msr_interception(d, msr);

Here the original msr value is expected.

Thanks
Kevin

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Xu, Quan]

 On April 29, 2016 10:21 AM, Tian, Kevin  wrote:
> > From: Xu, Quan
> > Sent: Friday, April 29, 2016 12:09 AM
> > On April 28, 2016 11:12 PM, Jan Beulich  wrote:
> > > >>> On 28.04.16 at 17:03,  wrote:
> > > > On April 28, 2016 10:36 PM, Jan Beulich  wrote:
> > > >> >>> On 28.04.16 at 16:14,  wrote:
> > > >> > On April 25, 2016 7:53 PM, Jan Beulich 
> wrote:
> > > >> >> >>> On 18.04.16 at 16:00,  wrote:
> > > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > >> >
> > > >> >
> > > >> >> > -static void iommu_flush_all(void)
> > > >> >> > +static int iommu_flush_all(void)
> > > >> >>
> > > >> >> __must_check
> > > >> >>
> > > >> >
> > > >> > The iommu_flush_all() is also called in
> > > >> > intel_iommu_hwdom_init() and vtd_crash_shutdown().
> > > >> > As we were on the same page, we can ignore the error code
> > > >> > propagation for these two call trees.
> > > >>
> > > >> I don't know what you're referring to here with "we were on the
> > > >> same
> > > page".
> > > > I
> > > >> don't think I've ever agreed (in the context of this series) to
> > > >> ignore any
> > > > error
> > > >> returns.
> > > >>
> > > >
> > > >
> > > > Look at the below link.
> > > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg032
> > > > 34.h
> > > > tml
> > >
> > > Which still talks about (conditionally) logging messages, not ignoring of
> errors.
> > >
> >
> > Sorry, check it again.
> > in intel_iommu_hwdom_init() and  vtd_crash_shutdown(), I still need to
> > log messages as:
> >
> > if ( iommu_flush_all() )
> >  printk()
> >
> > ?
> >
> > Thanks for your patience.
> > Quan
> 
> Yes, but please make sure you only print once to be useful.
>

 
Thanks.
Now I check the status in caller to make the print include caller which is 
failed, instead print in iommu_flush_all().
i.e., 
vtd_crash_shutdown()
{
..
if ( iommu_flush_all() )
printk(XENLOG_WARNING VTDPREFIX
   " vtd_crash_shutdown: IOMMU flush all failed.\n");
..
}

I am afraid I still don't get the point. To be honest, in such a fix,
The print is not so useful to me ( Correct me, I will continue to enhance it).

Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/hvm: Add debug exception vm_events

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:ta...@tklengyel.com]
> Sent: Thursday, April 28, 2016 4:33 AM
> 
> Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
> a hook for vm_event subscribers to tap into this new always-on guest event. We
> rename along the way hvm_event_breakpoint_type to hvm_event_type to better

I don't quite understand above renaming. How do you define 'event' here?
There are other functions like hvm_event_msr, hvm_event_cr, etc. The
new name hvm_event_type looks need to cover all those paths too?

> match the various events that can be passed with it. We also introduce the
> necessary monitor_op domctl's to enable subscribing to the events.
> 
> Signed-off-by: Tamas K Lengyel 

Thanks
Kevin

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Tian, Kevin]

> From: Xu, Quan
> Sent: Friday, April 29, 2016 12:09 AM
> 
> On April 28, 2016 11:12 PM, Jan Beulich  wrote:
> > >>> On 28.04.16 at 17:03,  wrote:
> > > On April 28, 2016 10:36 PM, Jan Beulich  wrote:
> > >> >>> On 28.04.16 at 16:14,  wrote:
> > >> > On April 25, 2016 7:53 PM, Jan Beulich  wrote:
> > >> >> >>> On 18.04.16 at 16:00,  wrote:
> > >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > >> >
> > >> >
> > >> >> > -static void iommu_flush_all(void)
> > >> >> > +static int iommu_flush_all(void)
> > >> >>
> > >> >> __must_check
> > >> >>
> > >> >
> > >> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()
> > >> > and vtd_crash_shutdown().
> > >> > As we were on the same page, we can ignore the error code
> > >> > propagation for these two call trees.
> > >>
> > >> I don't know what you're referring to here with "we were on the same
> > page".
> > > I
> > >> don't think I've ever agreed (in the context of this series) to
> > >> ignore any
> > > error
> > >> returns.
> > >>
> > >
> > >
> > > Look at the below link.
> > > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.h
> > > tml
> >
> > Which still talks about (conditionally) logging messages, not ignoring of 
> > errors.
> >
> 
> Sorry, check it again.
> in intel_iommu_hwdom_init() and  vtd_crash_shutdown(),
> I still need to log messages as:
> 
> if ( iommu_flush_all() )
>  printk()
> 
> ?
> 
> Thanks for your patience.
> Quan

Yes, but please make sure you only print once to be useful.

Thanks
Kevin
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Konrad Rzeszutek Wilk]

On Thu, Apr 28, 2016 at 09:52:14PM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Apr 29, 2016 at 12:23:38AM +0100, Andrew Cooper wrote:
> > On 28/04/2016 19:16, Konrad Rzeszutek Wilk wrote:
> > > diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> > > index 1b67d39..48088ce 100644
> > > --- a/xen/common/xsplice.c
> > > +++ b/xen/common/xsplice.c
> > > @@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
> > >  data->rc = rc;
> > >  }
> > >  
> > > +static bool_t is_work_scheduled(struct payload *data)
> > 
> > const struct payload *data
> 
> Yes!
> > 
> > Otherwise, Reviewed-by: Andrew Cooper 
> 
> And of course 5 hours later I realized there is a more straightforward
> for this. It follows the same idea but it piggyback on data->rc
> being set by 'schedule_work' to -EAGAIN once work is scheduled:

Err, 'schedule_work' does not set data->rc to -EAGAIN. It happens
within xsplice_do_action:

data->rc = -EAGAIN; 
rc = schedule_work(data, action->cmd, action->timeout);  

(for either replace, revert, or apply).

> 
> It could even be rolled in "xsplice: Implement support for
> applying/reverting/replacing patches."
> 
> 
> >From 83053e3f984b67dfae74cb64e75b8871b9d236ca Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk 
> Date: Thu, 28 Apr 2016 21:22:49 -0400
> Subject: [PATCH] xsplice: Check data->rc for -EAGAIN to guard against races.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Currently it is possible to:
> 
> 1)  xc_xsplice_apply()
>  \-> xsplice_action
> spin_lock(payload_lock)
>  \- schedule_work()
>  data->rc=-EAGAIN
> spin_unlock(payload_lock);
> 
> 2)  xc_xsplice_unload()
>  \-> xsplice_action
> spin_lock(payload_lock)
>  free_payload(data);
> spin_unlock(payload_lock);
> 
> .. all CPUs are quiesced.
> 
> 3) check_for_xsplice_work()
>  \-> apply_payload
> \-> arch_xsplice_apply_jmp
> BOOM
>  data->rc =0
> 
> The reason is that state is in 'CHECKED' which changes to 'APPLIED'
> once check_for_xsplice_work finishes (and it updates data->rc to zero).
> 
> But we have a race between 1) -> 3) where one can manipulate the payload
> (as the state is in 'CHECKED' from which you can apply/revert and unload).
> 
> This patch adds a simple check on data->rc to see if it is in -EAGAIN
> which means that schedule_work has been called for this payload.
> 
> If the payload aborts in check_for_xsplice_work (timed out, etc),
> the data->rc will be -EBUSY -so one can still unload the payload or
> retry the operation.
> 
> Signed-off-by: Konrad Rzeszutek Wilk 
> Reported-by: Roger Pau Monné 
> 
> ---
> CC: Andrew Cooper 
> CC: George Dunlap 
> CC: Ian Jackson 
> CC: Jan Beulich 
> CC: Konrad Rzeszutek Wilk 
> CC: Stefano Stabellini 
> CC: Tim Deegan 
> CC: Wei Liu 
> CC: Ross Lagerwall 
> ---
> ---
>  xen/common/xsplice.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index 1b67d39..0bc7e0f 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -1363,6 +1363,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
> *action)
>  return PTR_ERR(data);
>  }
>  
> +if ( data->rc == -EAGAIN ) /* schedule_work has been called. */
> +goto out;
> +
>  switch ( action->cmd )
>  {
>  case XSPLICE_ACTION_UNLOAD:
> @@ -1423,6 +1426,7 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
> *action)
>  break;
>  }
>  
> + out:
>  spin_unlock(_lock);
>  
>  return rc;
> -- 
> 2.4.0
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [ovmf test] 93114: regressions - FAIL

[osstest service owner]

flight 93114 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93114/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 65543
 test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail REGR. vs. 65543

version targeted for testing:
 ovmf 59844e126614fc8275aab083fafa5818cde0901c
baseline version:
 ovmf 5ac96e3a28dd26eabee421919f67fa7c443a47f1

Last test of basis65543  2015-12-08 08:45:15 Z  142 days
Failing since 65593  2015-12-08 23:44:51 Z  142 days  187 attempts
Testing same since93090  2016-04-28 13:43:45 Z0 days2 attempts


People who touched revisions under test:
  "Samer El-Haj-Mahmoud" 
  "Wu, Hao A" 
  "Yao, Jiewen" 
  Abdul Lateef Attar 
  Alcantara, Paulo 
  Anbazhagan Baraneedharan 
  Andrew Fish 
  Ard Biesheuvel 
  Arthur Crippa Burigo 
  Cecil Sheng 
  Chao Zhang 
  Chao Zhang
  Charles Duffy 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Daocheng Bu 
  Daryl McDaniel 
  David Woodhouse 
  Derek Lin 
  edk2 dev 
  edk2-devel 
  Eric Dong 
  Eric Dong 
  erictian
  Eugene Cohen 
  Evan Lloyd 
  Feng Tian 
  Fu Siyuan 
  Gabriel Somlo 
  Gary Ching-Pang Lin 
  Gary Lin 
  Ghazi Belaam 
  Hao Wu 
  Haojian Zhuang 
  Hegde Nagaraj P 
  Hegde, Nagaraj P 
  Hess Chen 
  Heyi Guo 
  Hot Tian 
  Jaben Carsey 
  James Bottomley 
  Jeff Fan 
  Jeremy Linton 
  Jiaxin Wu 
  jiewen yao 
  Jim Dailey 
  jim_dai...@dell.com 
  jljusten
  Jordan Justen 
  Juliano Ciocari 
  Justen, Jordan L 
  Karyne Mayer 
  Ken Lu 
  Kun Gui 
  Larry Hauch 
  Laszlo Ersek 
  Leahy, Leroy P
  Leahy, Leroy P 
  Lee Leahy 
  Leekha Shaveta 
  Leendert van Doorn 
  Leif Lindholm 
  Leif Lindholm 
  Leo Duran 
  Leon Li 
  Liming Gao 
  lzeng14
  Mark 
  Mark Doran 
  Mark Rutland 
  Marvin H?user 
  Marvin Haeuser 
  Marvin Häuser 
  marvin.haeu...@outlook.com 
  Michael D Kinney 
  Michael Kinney 
  Michael LeMay 
  Michael Thomas 
  Michał Zegan 
  Nagaraj Hegde 
  Ni, Ruiyu 
  Ni, Ruiyu 
  niruiyu
  Olivier Martin 
  Paolo Bonzini 
  Paulo Alcantara 
  Paulo Alcantara Cavalcanti 
  Peter Kirmeier 
  Qin Long 
  Qing Huang 
  Qiu Shumin 
  Qiu, Shumin 
  Rodrigo Dias Correa 
  Ruiyu Ni 
  Ryan Harkin 
  Samer El-Haj-Mahmoud 
  Samer El-Haj-Mahmoud 
  Sami Mujawar 
  Shivamurthy Shastri 
  Shumin Qiu 
  Star Zeng 
  Supreeth Venkatesh 
  Tapan Shah 
  Thomas Palmer 
  Tian Feng 
  Tian, Feng 
  Vladislav Vovchenko 
  Volker Rümelin 
  Yao Jiewen 
  Yao, 

Re: [Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Konrad Rzeszutek Wilk]

On Fri, Apr 29, 2016 at 12:23:38AM +0100, Andrew Cooper wrote:
> On 28/04/2016 19:16, Konrad Rzeszutek Wilk wrote:
> > diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> > index 1b67d39..48088ce 100644
> > --- a/xen/common/xsplice.c
> > +++ b/xen/common/xsplice.c
> > @@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
> >  data->rc = rc;
> >  }
> >  
> > +static bool_t is_work_scheduled(struct payload *data)
> 
> const struct payload *data

Yes!
> 
> Otherwise, Reviewed-by: Andrew Cooper 

And of course 5 hours later I realized there is a more straightforward
for this. It follows the same idea but it piggyback on data->rc
being set by 'schedule_work' to -EAGAIN once work is scheduled:

It could even be rolled in "xsplice: Implement support for
applying/reverting/replacing patches."


From 83053e3f984b67dfae74cb64e75b8871b9d236ca Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk 
Date: Thu, 28 Apr 2016 21:22:49 -0400
Subject: [PATCH] xsplice: Check data->rc for -EAGAIN to guard against races.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently it is possible to:

1)  xc_xsplice_apply()
 \-> xsplice_action
spin_lock(payload_lock)
 \- schedule_work()
 data->rc=-EAGAIN
spin_unlock(payload_lock);

2)  xc_xsplice_unload()
 \-> xsplice_action
spin_lock(payload_lock)
 free_payload(data);
spin_unlock(payload_lock);

.. all CPUs are quiesced.

3) check_for_xsplice_work()
 \-> apply_payload
\-> arch_xsplice_apply_jmp
BOOM
 data->rc =0

The reason is that state is in 'CHECKED' which changes to 'APPLIED'
once check_for_xsplice_work finishes (and it updates data->rc to zero).

But we have a race between 1) -> 3) where one can manipulate the payload
(as the state is in 'CHECKED' from which you can apply/revert and unload).

This patch adds a simple check on data->rc to see if it is in -EAGAIN
which means that schedule_work has been called for this payload.

If the payload aborts in check_for_xsplice_work (timed out, etc),
the data->rc will be -EBUSY -so one can still unload the payload or
retry the operation.

Signed-off-by: Konrad Rzeszutek Wilk 
Reported-by: Roger Pau Monné 

---
CC: Andrew Cooper 
CC: George Dunlap 
CC: Ian Jackson 
CC: Jan Beulich 
CC: Konrad Rzeszutek Wilk 
CC: Stefano Stabellini 
CC: Tim Deegan 
CC: Wei Liu 
CC: Ross Lagerwall 
---
---
 xen/common/xsplice.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 1b67d39..0bc7e0f 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -1363,6 +1363,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
 return PTR_ERR(data);
 }
 
+if ( data->rc == -EAGAIN ) /* schedule_work has been called. */
+goto out;
+
 switch ( action->cmd )
 {
 case XSPLICE_ACTION_UNLOAD:
@@ -1423,6 +1426,7 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
 break;
 }
 
+ out:
 spin_unlock(_lock);
 
 return rc;
-- 
2.4.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH V7 1/3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

[Shuai Ruan]

On Mon, Apr 25, 2016 at 12:51:44AM -0600, Jan Beulich wrote:
> >>> On 31.03.16 at 10:57,  wrote:
> > +#define XRSTOR(pfx) \
> > +if ( v->arch.xcr0_accum & XSTATE_XSAVES_ONLY ) \
> > +{ \
> > +if ( unlikely(!(ptr->xsave_hdr.xcomp_bv & \
> > +XSTATE_COMPACTION_ENABLED)) ) \
> > +ptr->xsave_hdr.xcomp_bv |= ptr->xsave_hdr.xstate_bv | \
> > +   XSTATE_COMPACTION_ENABLED; \
> 
> From v5 to v6 this changed from just = to |=, without any
> explanation, and without me really noticing - why? Weren't
> the other changes done specifically to guarantee xcomp_bv
> to be zero up to this point? In which case I'd prefer to make
> this obvious/explicit, by using = and perhaps an ASSERT()
> here. (I have a patch ready, but I'd like to understand if
> there was a reason for this change that I don't see.)
> 
> Jan
Using "=" is better. xcomp_bv can be guarantee to be zero to this
point.

Thanks

> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/xstate: don't clobber or leak state when using XSAVES

[Shuai Ruan]

On Mon, Apr 25, 2016 at 01:07:54AM -0600, Jan Beulich wrote:
> Commit 4d27280572 ("x86/xsaves: fix overwriting between non-lazy/lazy
> xsaves") switched to always saving full state when using compacted
> format (which is the only one XSAVES allows). It didn't, however, also
> adjust the restore side: In order to save full state, we also need to
> make sure we always load full state, or else the subject vCPU's state
> would get clobbered by that of the vCPU which happened to last have in
> use the respective component(s).
> 
> Signed-off-by: Jan Beulich 
This looks good to me.

Thanks 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Andrew Cooper]

On 28/04/2016 19:16, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index 1b67d39..48088ce 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
>  data->rc = rc;
>  }
>  
> +static bool_t is_work_scheduled(struct payload *data)

const struct payload *data

Otherwise, Reviewed-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] libxl: don't add cache mode for empty drives

[Jim Fehlig]

Roger Pau Monné wrote:
> On Thu, Apr 28, 2016 at 09:27:30AM +0100, George Dunlap wrote:
>> On Wed, Apr 27, 2016 at 5:22 PM, Jim Fehlig  wrote:
>>> On 04/27/2016 01:38 AM, Roger Pau Monné wrote:
 On Tue, Apr 26, 2016 at 10:35:31PM -0600, Jim Fehlig wrote:
> qemu commit 91a097e7 forbids specifying the cache mode for empty
> drives. Attempting to create a domain with an empty qdisk cdrom
> results in
>
> qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
>cache=writeback,id=ide-832: Must specify either driver or file
>
> Change libxl to only emit cache mode when a cdrom target is specified.
 What happens then when a cdrom is inserted? I cannot seem to find the code
 in libxl_cdrom_insert that sets the cache mode.
>>> I cannot find it either. I suppose it would need to be setup via xenstore,
>>> similar to other options like feature_discard. But looking at
>>> $qemu-src/hw/block/xen_disk.c, it seems the XenBlkDev struct has no field to
>>> specify cache mode. Would qemu's xen_disk need to be extended to support 
>>> cache
>>> mode, followed by a libxl patch to set the cache mode in xenstore?
>>>
  Is the default one used
 then?
>>> Yes, the default cache mode (which is already writeback AIUI) would be used 
>>> if
>>> not explicitly specified. Which brings up the option of removing
>>> 'cache=writeback' for cdroms altogether. Any opinion on that option?
>> What's the effective difference between caching modes for read-only
>> media anyway?
> 
> That's right, cdroms should always be read-only in which case the cache mode 
> doesn't matter. But I'm not sure if this is enforced in libxl.

xl-disk-configuration.txt states the default 'access=' value for cdrom devices
is readonly. It doesn't mention readonly is enforced for cdrom devices, but
xlu_disk_parse() in libxlutil unconditionally sets the disk's readwrite field to
0 for cdroms.

> IMHO, we should make sure ro is enforced with cdrom devices and then we can 
> use the default cache mode.

I'll send a V2 based on the above findings.

Regards,
Jim

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH V2] libxl: don't add cache mode for qdisk cdrom drives

[Jim Fehlig]

qemu commit 91a097e7 forbids specifying cache mode for empty
drives. Attempting to create a domain with an empty qdisk cdrom
drive results in

qemu-system-x86_64: -drive if=ide,index=1,readonly=on,media=cdrom,
   cache=writeback,id=ide-832: Must specify either driver or file

libxl only allows an empty 'target=' for cdroms. By default, cdroms
are readonly (see the 'access' parameter in xl-disk-configuration.txt)
and forced to readonly by any tools (e.g. xl) using libxlutil's
xlu_disk_parse() function. With cdroms always marked readonly,
explicitly specifying the cache mode for cdrom drives can be dropped.
The drive's 'readonly=on' option can also be set unconditionally.

Signed-off-by: Jim Fehlig 
---

V2:
Drop explicitly setting cache mode since cdrom devices are
readonly.
Unconditionally add 'readonly=on' drive option for cdroms.

 tools/libxl/libxl_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index fd12844..959e292 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1368,8 +1368,8 @@ static int libxl__build_device_model_args_new(libxl__gc 
*gc,
 
 if (disks[i].is_cdrom) {
 drive = libxl__sprintf(gc,
- 
"if=ide,index=%d,readonly=%s,media=cdrom,cache=writeback,id=ide-%i",
- disk, disks[i].readwrite ? "off" : "on", dev_number);
+ "if=ide,index=%d,readonly=on,media=cdrom,id=ide-%i",
+ disk, dev_number);
 
 if (target_path)
 drive = libxl__sprintf(gc, "%s,file=%s,format=%s",
-- 
1.8.0.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Wei Liu]

On Thu, Apr 28, 2016 at 06:29:03PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type 
> argument to xc_psr_*"):
> > On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> > > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> > > libxl_psr_cbm_type, so do the conversion.
> > > 
> > > Signed-off-by: Roger Pau Monné 
> > 
> > Acked-by: Wei Liu 
> 
> Nacked-by: Ian Jackson 
> 
> > > -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
>  cbm)) {
> > > +if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ
> e)type,
> > > +   socketid, cbm)) {
> 
> I'm very much against introducing casts which are not absolutely
> necessary.  Casts are a big hammer which can suppress important
> warnings (such as conversions between integers and pointers).
> 
> This anomaly with the same enum defined in two places with two names
> is pretty poor.  But if we are to perpetuate it, as perhaps we must,
> then rather than casting at each conversion point, we should introduce
> an inline function which contains the cast.  That way each call site
> remains more typesafe.
> 

The two enums aren't going away any time soon.

Does the following diff meet your requirement?

---8<---
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 40f2d5f..7a34c04 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -293,12 +293,18 @@ out:
 return rc;
 }
 
+static inline xc_psr_cat_type libxl_psr_cbm_type_to_libxc_psr_cat_type(
+libxl_psr_cbm_type type)
+{
+BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
+return (xc_psr_cat_type)type;
+}
+
 int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
   libxl_psr_cbm_type type, libxl_bitmap *target_map,
   uint64_t cbm)
 {
 GC_INIT(ctx);
-BUILD_BUG_ON(sizeof(libxl_psr_cbm_type) != sizeof(xc_psr_cat_type));
 int rc;
 int socketid, nr_sockets;
 
@@ -309,9 +315,13 @@ int libxl_psr_cat_set_cbm(libxl_ctx *ctx, uint32_t domid,
 }
 
 libxl_for_each_set_bit(socketid, *target_map) {
+xc_psr_cat_type xc_type;
+
 if (socketid >= nr_sockets)
 break;
-if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+
+xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
+if (xc_psr_cat_set_domain_data(ctx->xch, domid, xc_type,
socketid, cbm)) {
 libxl__psr_cat_log_err_msg(gc, errno);
 rc = ERROR_FAIL;
@@ -329,8 +339,9 @@ int libxl_psr_cat_get_cbm(libxl_ctx *ctx, uint32_t domid,
 {
 GC_INIT(ctx);
 int rc = 0;
+xc_psr_cat_type xc_type = libxl_psr_cbm_type_to_libxc_psr_cat_type(type);
 
-if (xc_psr_cat_get_domain_data(ctx->xch, domid, (xc_psr_cat_type)type,
+if (xc_psr_cat_get_domain_data(ctx->xch, domid, xc_type,
target, cbm_r)) {
 libxl__psr_cat_log_err_msg(gc, errno);
 rc = ERROR_FAIL;

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Wei Liu]

On Thu, Apr 28, 2016 at 02:16:52PM -0400, Konrad Rzeszutek Wilk wrote:
> Currently it is possible to:
> 
> 1)  xc_xsplice_apply()
>  \-> xsplice_action
>   spin_lock(payload_lock)
>  \- schedule_work()
> spin_unlock(payload_lock);
> 
> 2)  xc_xsplice_unload()
>  \-> xsplice_action
>   spin_lock(payload_lock)
>  free_payload(data);
> spin_unlock(payload_lock);
> 
> .. all CPUs are quisced.

"quiesced"

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [xen-4.3-testing test] 93098: trouble: blocked/broken/fail/pass

[osstest service owner]

flight 93098 xen-4.3-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93098/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64   3 host-install(3) broken REGR. vs. 87893
 build-i386-pvops  3 host-install(3) broken REGR. vs. 87893
 build-amd64-pvops 3 host-install(3) broken REGR. vs. 87893
 build-i3863 host-install(3) broken REGR. vs. 87893

Tests which did not succeed, but are not blocking:
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-rumpuserxen-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pygrub   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   blocked  n/a
 build-amd64-rumpuserxen   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pair 1 build-check(1)   blocked  n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pv   1 build-check(1)   blocked  n/a
 build-i386-rumpuserxen1 build-check(1)   blocked  n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-xend-qemut-winxpsp3  1 build-check(1)  blocked n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-qemut-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-pv1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-qemut-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemut-winxpsp3  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-winxpsp3  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl-qcow2 1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)blocked n/a
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-amd64-amd64-rumpuserxen-amd64  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl-qemut-debianhvm-amd64  1 build-check(1)blocked n/a
 test-armhf-armhf-xl   6 xen-boot fail   never pass
 test-armhf-armhf-libvirt  6 xen-boot fail   never pass
 test-armhf-armhf-xl-vhd   6 xen-boot fail   never pass
 test-armhf-armhf-xl-cubietruck  6 xen-boot fail never pass
 test-armhf-armhf-libvirt-raw  6 xen-boot fail   never pass
 test-armhf-armhf-xl-credit2   6 xen-boot fail   never pass
 test-armhf-armhf-libvirt-qcow2  6 xen-boot fail never pass
 test-armhf-armhf-xl-multivcpu  6 xen-boot fail  never pass
 test-armhf-armhf-xl-arndale   6 xen-boot fail   never pass

version targeted for testing:
 xen  c04846eeb3d96cf670dc5894b66f3f6e61c2531d
baseline version:
 xen  8fa31952e2d08ef63897c43b5e8b33475ebf5d93

Last test of basis87893  2016-03-29 13:49:52 Z   30 days
Testing same since92180  2016-04-20 17:49:21 Z8 days   21 attempts

[Xen-devel] [libvirt test] 92799: regressions - FAIL

[osstest service owner]

flight 92799 libvirt real [real]
http://logs.test-lab.xenproject.org/osstest/logs/92799/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-libvirt  14 guest-saverestore fail REGR. vs. 91479
 test-amd64-amd64-libvirt-xsm 14 guest-saverestore fail REGR. vs. 91479
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore fail 
REGR. vs. 91479
 test-amd64-amd64-libvirt-pair 21 guest-migrate/src_host/dst_host fail REGR. 
vs. 91479
 test-amd64-i386-libvirt-pair 21 guest-migrate/src_host/dst_host fail REGR. vs. 
91479
 test-amd64-amd64-libvirt-vhd 13 guest-saverestore fail REGR. vs. 91479
 test-amd64-i386-libvirt-xsm  14 guest-saverestore fail REGR. vs. 91479
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 guest-saverestore fail 
REGR. vs. 91479
 test-amd64-amd64-libvirt 14 guest-saverestore fail REGR. vs. 91479

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt  12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-xsm 14 guest-saverestorefail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-armhf-armhf-libvirt-raw 13 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt-raw 11 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 14 guest-saverestorefail   never pass
 test-armhf-armhf-libvirt 12 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 11 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  12 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-qcow2 11 migrate-support-checkfail never pass
 test-armhf-armhf-libvirt-qcow2 13 guest-saverestorefail never pass

version targeted for testing:
 libvirt  662bf30c0f8605f960105a9ac1fb8edc80d8971b
baseline version:
 libvirt  8b62c65d24bdb20121d3147b4f4dc98bac4f024b

Last test of basis91479  2016-04-15 05:33:51 Z   13 days
Failing since 91597  2016-04-16 04:20:46 Z   12 days   11 attempts
Testing same since92799  2016-04-26 04:33:21 Z2 days1 attempts


People who touched revisions under test:
  Andrea Bolognani 
  Cole Robinson 
  Dmitry Andreev 
  Erik Skultety 
  Jason J. Herne 
  Jim Fehlig 
  Jiri Denemark 
  John Ferlan 
  Ján Tomko 
  Laine Stump 
  Martin Kletzander 
  Maxim Nestratov 
  Michal Privoznik 
  Mikhail Feoktistov 
  Nikolay Shirokovskiy 
  Nitesh Konkar 
  Nitesh Konkar 
  Olga Krishtal 
  Peter Krempa 
  Richard Laager 
  Richard W.M. Jones 
  Roman Bogorodskiy 
  Simon Arlott 

jobs:
 build-amd64-xsm  pass
 build-armhf-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-armhf  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-armhf-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-armhf-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm   fail
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmfail
 test-amd64-amd64-libvirt-xsm fail
 test-armhf-armhf-libvirt-xsm fail
 test-amd64-i386-libvirt-xsm  fail
 test-amd64-amd64-libvirt fail
 test-armhf-armhf-libvirt

Re: [Xen-devel] [Xen-users] How to install Xen on Fedora 23?

[Konrad Rzeszutek Wilk]

On Wed, Apr 27, 2016 at 05:50:56AM +, Jason Long wrote:
> Can it work on Workstation version too? Or just server version of fedora 
> needed?

I do it under workstation.


> 
> 
> 
> On Wednesday, April 27, 2016 9:53 AM, Jason Long  wrote:
> Not matter.
> I did it too :
> 
> # grub2-mkconfig -o /boot/grub2/grub.cfg
> Generating grub configuration file ...
> Found linux image: /boot/vmlinuz-4.4.7-300.fc23.i686
> Found initrd image: /boot/initramfs-4.4.7-300.fc23.i686.img
> Found linux image: /boot/vmlinuz-4.2.3-300.fc23.i686
> Found initrd image: /boot/initramfs-4.2.3-300.fc23.i686.img
> Found linux image: /boot/vmlinuz-0-rescue-204e260309794cee901bd6e0b46ace3a
> Found initrd image: 
> /boot/initramfs-0-rescue-204e260309794cee901bd6e0b46ace3a.img
> done
> 
> 
> Problem not solved :(

Weird.

Oh wait, i686?!? I don't think the Xen RPM is even present for that?


> 
> 
> 
> 
> On Wednesday, April 27, 2016 9:06 AM, Konrad Rzeszutek Wilk 
>  wrote:
> On Tue, Apr 26, 2016 at 03:00:21PM +, Jason Long wrote:
> > Hello.
> > I installed Fedora 23 x64 on my PC and id below command for installing Xen :
> > 
> > # cd /etc/yum.repos.d/
> > # wget http://fedorapeople.org/groups/virt/...t-preview.repo
> 
> Why? No need for that.
> > # yum update
> > # yum -y install xen xen-hypervisor xen-libs xen-runtime
> > # systemctl enable xend.service
> > # systemctl enable xendomains.service
> > 
> > After it I run below command :
> 
> You are missing 'grub2-mkconfig -o /boot/grub2/grub.cfg'
> 
> 
> > 
> > # grep ^menuentry /boot/grub2/grub.cfg | cut -d "'" -f2
> 
> > Fedora (4.4.7-300.fc23.i686) 23 (Workstation Edition)
> > Fedora (4.2.3-300.fc23.i686) 23 (Workstation Edition)
> > Fedora (0-rescue-204e260309794cee901bd6e0b46ace3a) 23 (Workstation Edition)
> > 
> > But, I can't see any "Fedora, with Xen hypervisor" !!! and when I run "xl 
> > info" it show me below error :
> > 
> > # xl info
> > xc: error: Could not obtain handle on privileged command interface (2 = No 
> > such file or directory): Internal error
> > libxl: error: libxl.c:114:libxl_ctx_alloc: cannot open libxc handle: No 
> > such file or directory
> > cannot init xl context
> > 
> > How can I solve it?
> > 
> > ___
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> ___
> Xen-users mailing list
> xen-us...@lists.xen.org
> http://lists.xen.org/xen-users 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] 2016 Xen hackathon notes - xenstored

[Luis R. Rodriguez]

At the 2016 Xen Hackathon I raised the topic of the default xenstored
used. Here are my notes with some new additions and supported
documentation. It would seem we're moving to oxenstored as the default
on Linux distributions and FreeBSD now, if you have issues or concerns
with this please let us know.

Notes:
=

Although we have had oxenstored be the the default *iff* you have
ocaml dev libs installled when compiling Xen from source both Linux
distributions (Debian, SUSE*, Gentoo) and FreeBSD were still using the
C xenstored as the default. This begged the question of why not make
the switch given Citrix has already been using oxensotred in
production for years with these known gains [0]:

  * 1/5th the size in terms of line of code in comparison to the C xenstored
  * better performance increasing support for the number of guests, it
supports 3 times number of guests for an upper limit of 160 guests

At the 2014 summit Anil's presented work on a Xenstore 2.0 which
hinted also towards the future ability to provide git-like
capabilities for the xenstore, all still written in Ocaml [1]. There
are others who have worked on a C++ replacement lixs (Lightweight
XenStore) as well [2], such work revealed oxenstored had the CPU
pegged after just a few dozen guests. Such work hinted that the
oxenstored that should be considered for more serious work was the
Mirage OS xenstore [3], but that the C++ lixs was also performing
better than the current oxenstored.

Although there are questions about the future of the Xenestore we know
oxenstored performs better than cxenstored and since we are building
and using it by default already it begged the question why haven't
distributions made the switch to use it by default. Since Mirage OS
work seems promising we agreed to just set oxenstored as the default
in distributions and in the future hope that Mirage's work or other
contending efforts make it upstream to consider them as alternatives.

Given Mirage Xenstore would still require ocaml, it would be a good
stepping stone now to just use oxenstored by default more widely on
Linux distributions and FreeBSD. A concern was raised about expertise
over Ocaml, however it would seem that we will have no option but to
rely on the community / folks supporting upstream oxenstored for this.

[0] http://www.do-not-panic.com/2014/04/summary-of-gains-of-xen-oxenstored.html
[1] http://decks.openmirage.org/xendevsummit14#/
[2] 
http://events.linuxfoundation.org/sites/events/files/slides/xendevsummit14_0.pdf
[3] https://github.com/mirage/ocaml-xenstore

  Luis

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC] Minor change to governance document at http://www.xenproject.org/developers/governance.html

[Konrad Rzeszutek Wilk]

On Tue, Apr 26, 2016 at 12:48:41PM +0100, Lars Kurth wrote:
> Hi all,
> 
> following the recent process to elect new maintainers and committers, 
> I would like to suggest the following minor change to our governance 
> document. I believe the process we ran recently worked well, so we 
> should change the governance accordingly. I don't think we need to 
> change the governance to remind the community at least annually of 
> the process, but we should use the appointments@ alias, such that 
> community members can nominate new committers in private.
> 
> Here is the relevant snippet from 
> http://www.xenproject.org/developers/governance.html
> ---
> Committer Elections
> 
> Developers who have earned the trust of committers in their project 
> (including the project lead) can through election be promoted to 
> 
> can 
> [the (including the project lead) makes no sense]
> 
> Committer. A two stage mechanism is used
> 
> * Nomination: A committers should nominate a community member publicly 
>   
>   Community members should nominate candidates by posting 
>   a proposal to appointments at xenproject dot org
>  
> 
> explaining the candidate's contributions to the project and thus why 
> they should be elected 
> 
> as a maintainer on the project's public mailing list. 
> 
> 
> to become a Committer of the project.
> [Typo: this should have been Committer in the first place]
> 
> The nomination should include a project, cite evidence such as patches  
>^
>should
> [the "include a project" makes no sense]
>  
> and other contributions where the case is not obvious. 
> 
>Existing 
> Committers will review all proposals, check whether the nominee 
> would be willing to accept the nomination and publish suitable 
> nominations on the project's public mailing list for wider 
> community input.
> 
> ---
> 
> In the nomination e-mail, we can then use what we used in 
> https://www.mail-archive.com/xen-devel@lists.xen.org/msg63365.html
> ---
> == Providing Feedback ==
> We are inviting community members to provide comments regarding the 
> nomination of . As feedback may be personal and may make 
> nominees uncomfortable, we are asking you to provide feedback by 
> sending a private mail to appointments at xenproject dot org *before* 
>  (please do *not* CC xen-devel@). 
> 
> If you want to congratulate the nominee, it is of course OK to do 
> this in public. Please use your judgement and common sense.
> ---
> 
> This is in line with what we have done earlier this month, and seems
> to be a better approach than requiring discussions about nominations
> to be public from the beginning. Public discussions about a 
> candidates track record may necessarily become personal and thus may 
> prevent good candidates from being nominated.
> 
> Any views?

Looks good!

> 
> Best Regards
> Lars
> 
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] xsplice: Don't perform multiple operations on same payload once work is scheduled.

[Konrad Rzeszutek Wilk]

Currently it is possible to:

1)  xc_xsplice_apply()
 \-> xsplice_action
spin_lock(payload_lock)
 \- schedule_work()
spin_unlock(payload_lock);

2)  xc_xsplice_unload()
 \-> xsplice_action
spin_lock(payload_lock)
 free_payload(data);
spin_unlock(payload_lock);

.. all CPUs are quisced.

3) check_for_xsplice_work()
 \-> apply_payload
\-> arch_xsplice_apply_jmp
BOOM

The reason is that state is in 'CHECKED' which changes to 'APPLIED'
once check_for_xsplice_work finishes. So we have a race between 1) -> 3)
where one can manipulate the payload.

To guard against this we add a check in xsplice_action to not allow
any actions if schedule_work has been called for this specific payload.

The function 'is_work_scheduled' checks xsplice_work which is safe as:
 - The ->do_work changes to 1 under the payload_lock (which we also hold).
 - The ->do_work changes to 0 when all CPUs are quisced and IRQs have
   been disabled.

Signed-off-by: Konrad Rzeszutek Wilk 
Reported-by: Roger Pau Monné 

---
Cc: Roger Pau Monné 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Ian Jackson 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Cc: Wei Liu 
Cc: Ross Lagerwall 
---
---
 xen/common/xsplice.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 1b67d39..48088ce 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -1099,6 +1099,13 @@ static void xsplice_do_action(void)
 data->rc = rc;
 }
 
+static bool_t is_work_scheduled(struct payload *data)
+{
+ASSERT(spin_is_locked(_lock));
+
+return xsplice_work.do_work && xsplice_work.data == data;
+}
+
 static int schedule_work(struct payload *data, uint32_t cmd, uint32_t timeout)
 {
 ASSERT(spin_is_locked(_lock));
@@ -1363,6 +1370,12 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
 return PTR_ERR(data);
 }
 
+if ( is_work_scheduled(data) )
+{
+rc = -EBUSY;
+goto out;
+}
+
 switch ( action->cmd )
 {
 case XSPLICE_ACTION_UNLOAD:
@@ -1423,6 +1436,7 @@ static int xsplice_action(xen_sysctl_xsplice_action_t 
*action)
 break;
 }
 
+ out:
 spin_unlock(_lock);
 
 return rc;
-- 
2.5.0


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 for-4.7 07/16] libxc: fix usage of uninitialized variable

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v3 for-4.7 07/16] libxc: fix usage of uninitialized 
variable"):
> Ian, this is a backport candidate.

Noted, thanks.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] tools: detect appropriate debug optimization level

[Wei Liu]

On Tue, Apr 26, 2016 at 09:38:45AM -0500, Doug Goldstein wrote:
> When building debug use -Og as the optimization level if its available,
> otherwise retain the use of -O0. -Og has been added by GCC to enable all
> optimizations that to not affect debugging while retaining full
> debugability.
> 
> Signed-off-by: Doug Goldstein 
> ---
> change since v2:
> - switch back to cc-option-add to not call cc-option on every invocation
> change since v1:
> - switch to cc-option to only specify -O0 if -Og isn't supported
> ---
>  tools/Rules.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/Rules.mk b/tools/Rules.mk
> index 9ef0b47..1b79a6e 100644
> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -138,6 +138,7 @@ SHLIB_libxenvchan  = $(SHDEPS_libxenvchan) 
> -Wl,-rpath-link=$(XEN_LIBVCHAN)
>  ifeq ($(debug),y)
>  # Disable optimizations and enable debugging information for macros
>  CFLAGS += -O0 -g3
> +$(call cc-option-add,CFLAGS,CC,-Og)
>  # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=.
>  PY_CFLAGS += $(PY_NOOPT_CFLAGS)
>  endif
> -- 
> 2.7.3

Heh, this patch seems to has a (good) side effect. I will look into
fixing that tomorrow.

make[10]: Entering directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/tcgbios'
gcc   -O1 -fno-omit-frame-pointer -m32 -march=i686 -g -fno-strict-aliasing 
-std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O0 -g3 -Og 
-D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF 
.tcgbios.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE 
-fno-optimize-sibling-calls -mno-tls-direct-seg-refs  -Werror 
-fno-stack-protector -fno-exceptions -fno-builtin -msoft-float 
-I/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/tcgbios/../../../../../tools/include
 -I.. -I../..  -c -o tcgbios.o tcgbios.c 
gcc   -O1 -fno-omit-frame-pointer -m32 -march=i686 -g -fno-strict-aliasing 
-std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O0 -g3 -Og 
-D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF 
.tpm_drivers.o.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE 
-fno-optimize-sibling-calls -mno-tls-direct-seg-refs  -Werror 
-fno-stack-protector -fno-exceptions -fno-builtin -msoft-float 
-I/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/tcgbios/../../../../../tools/include
 -I.. -I../..  -c -o tpm_drivers.o tpm_drivers.c 
tcgbios.c: In function ‘tcpa_extend_acpi_log’:
tcgbios.c:362:3: error: ‘size’ may be used uninitialized in this function 
[-Werror=maybe-uninitialized]
   memcpy((char *)lasa_last, (char *)entry_ptr, size);
   ^
tcgbios.c: In function ‘HashLogEvent32’:
tcgbios.c:1142:22: error: ‘entry’ may be used uninitialized in this function 
[-Werror=maybe-uninitialized]
hleo->eventnumber = entry;
  ^
tcgbios.c:1131:10: error: ‘logdataptr’ may be used uninitialized in this 
function [-Werror=maybe-uninitialized]
entry = tcpa_extend_acpi_log(logdataptr);
  ^
cc1: all warnings being treated as errors
/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/tcgbios/../../../../../tools/Rules.mk:192:
 recipe for target 'tcgbios.o' failed
make[10]: *** [tcgbios.o] Error 1
make[10]: Leaving directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/tcgbios'
/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/../../../../tools/Rules.mk:216:
 recipe for target 'subdir-all-tcgbios' failed
make[9]: *** [subdir-all-tcgbios] Error 2
make[9]: Leaving directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit'
/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit/../../../../tools/Rules.mk:211:
 recipe for target 'subdirs-all' failed
make[8]: *** [subdirs-all] Error 2
make[8]: Leaving directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios/32bit'
/local/work/COMMITTER/xen.git/tools/firmware/rombios/../../../tools/Rules.mk:216:
 recipe for target 'subdir-all-32bit' failed
make[7]: *** [subdir-all-32bit] Error 2
make[7]: Leaving directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios'
/local/work/COMMITTER/xen.git/tools/firmware/rombios/../../../tools/Rules.mk:211:
 recipe for target 'subdirs-all' failed
make[6]: *** [subdirs-all] Error 2
make[6]: Leaving directory 
'/local/work/COMMITTER/xen.git/tools/firmware/rombios'
/local/work/COMMITTER/xen.git/tools/firmware/../../tools/Rules.mk:216: recipe 
for target 'subdir-all-rombios' failed
make[5]: *** [subdir-all-rombios] Error 2
make[5]: Leaving directory '/local/work/COMMITTER/xen.git/tools/firmware'
/local/work/COMMITTER/xen.git/tools/firmware/../../tools/Rules.mk:211: recipe 
for target 'subdirs-all' failed
make[4]: *** [subdirs-all] Error 2
make[4]: Leaving directory '/local/work/COMMITTER/xen.git/tools/firmware'
Makefile:33: recipe for target 'all' failed
make[3]: *** [all] Error 2
make[3]: Leaving 

Re: [Xen-devel] [xen-4.3-testing test] 92882: trouble: blocked/broken/fail/pass

[Ian Jackson]

osstest service owner writes ("[xen-4.3-testing test] 92882: trouble: 
blocked/broken/fail/pass"):
> flight 92882 xen-4.3-testing real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/92882/
> 
> Failures and problems with tests :-(
> 
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  build-i386-pvops  3 host-install(3) broken REGR. vs. 87893
>  build-amd64   3 host-install(3) broken REGR. vs. 87893
>  build-amd64-pvops 3 host-install(3) broken REGR. vs. 87893
>  build-i3863 host-install(3) broken REGR. vs. 87893

Whatever is wrong here is 1. definitely not a transient failure of any
kind 2. does not appear to be a bug in the code under test 3. rather
too much debugging for to do now.

I am going to disable the Xen 4.3 branch tests for now.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.7 12/14] libxl: fix passing the type argument to xc_psr_*

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2 for-4.7 12/14] libxl: fix passing the type 
argument to xc_psr_*"):
> On Tue, Apr 26, 2016 at 04:52:21PM +0200, Roger Pau Monne wrote:
> > The xc_psr_* functions expect the type to be xc_psr_cat_type instead of
> > libxl_psr_cbm_type, so do the conversion.
> > 
> > Signed-off-by: Roger Pau Monné 
> 
> Acked-by: Wei Liu 

Nacked-by: Ian Jackson 

> > -if (xc_psr_cat_set_domain_data(ctx->xch, domid, type, socketid,
 cbm)) {
> > +if (xc_psr_cat_set_domain_data(ctx->xch, domid, (xc_psr_cat_typ
e)type,
> > +   socketid, cbm)) {

I'm very much against introducing casts which are not absolutely
necessary.  Casts are a big hammer which can suppress important
warnings (such as conversions between integers and pointers).

This anomaly with the same enum defined in two places with two names
is pretty poor.  But if we are to perpetuate it, as perhaps we must,
then rather than casting at each conversion point, we should introduce
an inline function which contains the cast.  That way each call site
remains more typesafe.

Thanks,
Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Wei Liu]

On Thu, Apr 28, 2016 at 06:26:09PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the 
> printf-like attributes to a couple of functions"):
> > Sigh. I can't say I like turning that into a macro though. On the other
> > hand there doesn't seem to be an elegant way of solving that.
> 
> Well, other than suppressing the warning somehow.
> 

I don't think we want to suppress the warning. It's useful.

> > Roger, please at least make it look like a macro. Say, name it
> > DEVICE_MODEL_XS_PATH or something.
> 
> I'm not sure I agree.  I think if we can make it a function-like
> macro, then it is fine to give it a function-like name.  (Note that
> many libc functions might be macros; getc is traditionally a #define.)
> 
> Obviously it can't be function-like in that you must always pass it a
> literal, but violations of that constraint will be detected by the
> compiler.

I've pushed the series from Roger to staging because it fixes clang
compilation. I don't have strong opinion on what the name of macro
should look like so feel free to send a patch to change that.

Wei.

> 
> Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the printf-like attributes to a couple of functions

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH v2 for-4.7 10/14] libxl: add the 
printf-like attributes to a couple of functions"):
> Sigh. I can't say I like turning that into a macro though. On the other
> hand there doesn't seem to be an elegant way of solving that.

Well, other than suppressing the warning somehow.

> Roger, please at least make it look like a macro. Say, name it
> DEVICE_MODEL_XS_PATH or something.

I'm not sure I agree.  I think if we can make it a function-like
macro, then it is fine to give it a function-like name.  (Note that
many libc functions might be macros; getc is traditionally a #define.)

Obviously it can't be function-like in that you must always pass it a
literal, but violations of that constraint will be detected by the
compiler.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] tools: detect appropriate debug optimization level

[Wei Liu]

On Thu, Apr 28, 2016 at 06:19:19PM +0100, Ian Jackson wrote:
> Doug Goldstein writes ("[PATCH v3] tools: detect appropriate debug 
> optimization level"):
> > When building debug use -Og as the optimization level if its available,
> > otherwise retain the use of -O0. -Og has been added by GCC to enable all
> > optimizations that to not affect debugging while retaining full
> > debugability.
> > 
> > Signed-off-by: Doug Goldstein 
> 
> Acked-by: Ian Jackson 

Queued.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] tools: detect appropriate debug optimization level

[Ian Jackson]

Doug Goldstein writes ("[PATCH v3] tools: detect appropriate debug optimization 
level"):
> When building debug use -Og as the optimization level if its available,
> otherwise retain the use of -O0. -Og has been added by GCC to enable all
> optimizations that to not affect debugging while retaining full
> debugability.
> 
> Signed-off-by: Doug Goldstein 

Acked-by: Ian Jackson 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH] xen/arm64: ensure that the correct SP is used for exceptions

[Kyle Temkin]

From: "Kyle J. Temkin" 

The ARMv8 architecture has a SPSel ("stack pointer selection") machine
register that allows us to determine which exception level's stack
pointer is loaded when an exception occurs. As we don't want to
use the non-priveleged SP_EL0 stack pointer -- or even assume that SP_EL0
points to a valid address in the hypervisor context--  we'll need to ensure
that our EL2 code sets the SPSel to SP_ELn mode, so exceptions that trap
to EL2 use the EL2 stack pointer.

This corrects an issue that can manifest as a hang-on-IRQ on some
arm64 cores if the firmware/bootloader has previously initialized SPSel
to 0; in which case Xen's exceptions will incorrectly use an invalid SP_EL0,
and will endlessly spin on the synchronous abort handler.

Signed-off-by: Kyle Temkin 
---
 xen/arch/arm/arm64/head.S | 5 +
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 946e2c9..d5831f2 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -361,6 +361,11 @@ skip_bss:
 ldr   x0, =(HSCTLR_BASE)
 msr   SCTLR_EL2, x0
 
+/* Ensure that any exceptions encountered at EL2
+ * are handled using the EL2 stack pointer, rather
+ * than SP_EL0. */
+msr spsel, #1
+
 /* Rebuild the boot pagetable's first-level entries. The structure
  * is described in mm.c.
  *
-- 
2.7.2


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN

[Ian Jackson]

Wei Liu writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to 
XEN"):
> So what is the conclusion of this discussion so far? I admit I'm a bit
> lost here.

Undoubtedly:

* That "xm ..." generates the reported error is definitely a bug.

* "xm" exists only in versions of Xen now no longer supported
  upstream.

* Applying the proposed patch to libxc in upstream supported versions
  of Xen will not fix "xm" for users like Zhenzhong Duan.


There are two possible views about the nature of the bug:

1. It is xm's fault for passing an invalid cpumap.

2. It is python xc's fault for failing to tolerate a cpumap with
   bits set which do not correspond to actual cpus.

In the case 1., the xm python code needs to be changed.  But there is
nothing for upstream to do because none of our supported trees contain
this code any more.

In the case 2., the in-tree python xc code should be changed.


I am somewhat reluctant to go down the path implied by case 2, because
we don't know what collateral damage there may be.  OTOH the proposed
patch is one which tolerates a wider range of inputs, which is fairly
safe.

I think that the behaviour of libxl (which has to provide a C API,
rather than a python one) is a poor guide to the best behaviour for
python xc.


So I still don't have a clear view about whether the patch should be
applied.  (I think it clearly shouldn't be backported.)

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [ovmf test] 93090: regressions - FAIL

[osstest service owner]

flight 93090 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93090/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 65543
 test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail REGR. vs. 65543

version targeted for testing:
 ovmf 59844e126614fc8275aab083fafa5818cde0901c
baseline version:
 ovmf 5ac96e3a28dd26eabee421919f67fa7c443a47f1

Last test of basis65543  2015-12-08 08:45:15 Z  142 days
Failing since 65593  2015-12-08 23:44:51 Z  141 days  186 attempts
Testing same since93090  2016-04-28 13:43:45 Z0 days1 attempts


People who touched revisions under test:
  "Samer El-Haj-Mahmoud" 
  "Wu, Hao A" 
  "Yao, Jiewen" 
  Abdul Lateef Attar 
  Alcantara, Paulo 
  Anbazhagan Baraneedharan 
  Andrew Fish 
  Ard Biesheuvel 
  Arthur Crippa Burigo 
  Cecil Sheng 
  Chao Zhang 
  Chao Zhang
  Charles Duffy 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Daocheng Bu 
  Daryl McDaniel 
  David Woodhouse 
  Derek Lin 
  edk2 dev 
  edk2-devel 
  Eric Dong 
  Eric Dong 
  erictian
  Eugene Cohen 
  Evan Lloyd 
  Feng Tian 
  Fu Siyuan 
  Gabriel Somlo 
  Gary Ching-Pang Lin 
  Gary Lin 
  Ghazi Belaam 
  Hao Wu 
  Haojian Zhuang 
  Hegde Nagaraj P 
  Hegde, Nagaraj P 
  Hess Chen 
  Heyi Guo 
  Hot Tian 
  Jaben Carsey 
  James Bottomley 
  Jeff Fan 
  Jeremy Linton 
  Jiaxin Wu 
  jiewen yao 
  Jim Dailey 
  jim_dai...@dell.com 
  jljusten
  Jordan Justen 
  Juliano Ciocari 
  Justen, Jordan L 
  Karyne Mayer 
  Ken Lu 
  Kun Gui 
  Larry Hauch 
  Laszlo Ersek 
  Leahy, Leroy P
  Leahy, Leroy P 
  Lee Leahy 
  Leekha Shaveta 
  Leendert van Doorn 
  Leif Lindholm 
  Leif Lindholm 
  Leo Duran 
  Leon Li 
  Liming Gao 
  lzeng14
  Mark 
  Mark Doran 
  Mark Rutland 
  Marvin H?user 
  Marvin Haeuser 
  Marvin Häuser 
  marvin.haeu...@outlook.com 
  Michael D Kinney 
  Michael Kinney 
  Michael LeMay 
  Michael Thomas 
  Michał Zegan 
  Nagaraj Hegde 
  Ni, Ruiyu 
  Ni, Ruiyu 
  niruiyu
  Olivier Martin 
  Paolo Bonzini 
  Paulo Alcantara 
  Paulo Alcantara Cavalcanti 
  Peter Kirmeier 
  Qin Long 
  Qing Huang 
  Qiu Shumin 
  Qiu, Shumin 
  Rodrigo Dias Correa 
  Ruiyu Ni 
  Ryan Harkin 
  Samer El-Haj-Mahmoud 
  Samer El-Haj-Mahmoud 
  Sami Mujawar 
  Shivamurthy Shastri 
  Shumin Qiu 
  Star Zeng 
  Supreeth Venkatesh 
  Tapan Shah 
  Thomas Palmer 
  Tian Feng 
  Tian, Feng 
  Vladislav Vovchenko 
  Volker Rümelin 
  Yao Jiewen 
  Yao, 

Re: [Xen-devel] [PATCH v2] x86/vMSI-X: also snoop REP MOVS

[Wei Liu]

On Thu, Apr 28, 2016 at 08:52:53AM -0600, Jan Beulich wrote:
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
> 
> Signed-off-by: Jan Beulich 

Release-acked-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/vMSI-X: also snoop REP MOVS

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 15:53
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant; Wei Liu
> Subject: [PATCH v2] x86/vMSI-X: also snoop REP MOVS
> 
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Paul Durrant 

> ---
> v2: Comment conditional being added to msixtbl_range().
> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
>  ASSERT(r->type == IOREQ_TYPE_COPY);
>  if ( r->dir == IOREQ_WRITE )
>  {
> +unsigned int size = r->size;
> +
>  if ( !r->data_is_ptr )
>  {
> -unsigned int size = r->size;
>  uint64_t data = r->data;
> 
>  if ( size == 8 )
> @@ -366,7 +367,29 @@ static int msixtbl_range(struct vcpu *v,
>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>   !(data & PCI_MSIX_VECTOR_BITMASK) )
> +{
>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> +}
> +}
> +else if ( (size == 4 || size == 8) &&
> +  /* Only support forward REP MOVS for now. */
> +  !r->df &&
> +  /*
> +   * Only fully support accesses to a single table entry for
> +   * now (if multiple ones get written to in one go, only the
> +   * final one gets dealt with).
> +   */
> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
> +{
> +BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> + (PCI_MSIX_ENTRY_SIZE - 1));
> +
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> +addr + size * r->count - 4;
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> +r->data + size * r->count - 4;
>  }
>  }
> 
> @@ -471,6 +494,7 @@ out:
>  for_each_vcpu ( d, v )
>  {
>  if ( (v->pause_flags & VPF_blocked_in_xen) &&
> + !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
>   v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
>   (gtable + msi_desc->msi_attrib.entry_nr *
> PCI_MSIX_ENTRY_SIZE +
> @@ -561,9 +585,29 @@ void msixtbl_pt_cleanup(struct domain *d
>  void msix_write_completion(struct vcpu *v)
>  {
>  unsigned long ctrl_address = v-
> >arch.hvm_vcpu.hvm_io.msix_unmask_address;
> +unsigned long snoop_addr = v-
> >arch.hvm_vcpu.hvm_io.msix_snoop_address;
> 
>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
> 
> +if ( !ctrl_address && snoop_addr &&
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
> +{
> +const struct msi_desc *desc;
> +uint32_t data;
> +
> +rcu_read_lock(_rcu_lock);
> +desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
> +snoop_addr);
> +rcu_read_unlock(_rcu_lock);
> +
> +if ( desc &&
> + hvm_copy_from_guest_phys(,
> +  v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
> +  sizeof(data)) == HVMCOPY_okay &&
> + !(data & PCI_MSIX_VECTOR_BITMASK) )
> +ctrl_address = snoop_addr;
> +}
> +
>  if ( !ctrl_address )
>  return;
> 
> --- a/xen/include/asm-x86/hvm/vcpu.h
> +++ b/xen/include/asm-x86/hvm/vcpu.h
> @@ -86,6 +86,7 @@ struct hvm_vcpu_io {
> 
>  unsigned long msix_unmask_address;
>  unsigned long msix_snoop_address;
> +unsigned long msix_snoop_gpa;
> 
>  const struct g2m_ioport *g2m_ioport;
>  };
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Questions about the new usb hotplug code in libxl and about adding hotplug (with qmp) usbredir tcp channels

[Martin Cerveny]

On Thu, 28 Apr 2016, Fabio Fantoni wrote:


Il 27/04/2016 20:03, Martin Cerveny ha scritto:



On Wed, 27 Apr 2016, Fabio Fantoni wrote:


Il 27/04/2016 13:26, George Dunlap ha scritto:

On 27/04/16 12:02, Fabio Fantoni wrote:

Hi, I took a look at the new pvusb hotplug code in libxl to try to add
also hotplug (with qmp) usbredir tcp channels.
Adding usbredir tcp channels at domU start requires for example adding
qemu parameters like these: "-chardev
socket,id=charredir4,host=192.168.1.35,port=4 -device
usb-redir,chardev=charredir4,id=redir4".
It is possible to hotplug it with qmp using "chardev-add" and
"device_add" commands.
Looking at old George Dunlap's patches I tested years ago
(http://xenbits.xen.org/gitweb/?p=people/gdunlap/xen.git;a=commitdiff;h=f7a77843e3fcf070c72115be8ed349a3bfe34e60) 
I can understand what they do and I can add similar qmp functions for

usbredir tcp too.
But now I see that bigger and different usb hotplug code was added, I
looked at these patches:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bf7628f087b212052a0e9f024044b2790c33f820 

http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=043910384cb9ea2c781a7dceac238e110a559c10 


and the full current code in xen's staging branch but I didn't find qmp
commands for the qemu usb passthrough, I suppose it is missing or
incomplete (though strange), am I wrong?
If that is correct, pvusb drivers are needed for both host and domU to
have usb passthrough working but in new windows pv drivers, the pvusb
one is missing, so without the "qemu emulated" usb passthrough it
doesn't work at all in similar cases, right?

That's right -- the PVUSB code *only* does PVUSB passthrough. The
interface was designed, however, to be able to add emulated USB on top.


How do you think I should proceed to implement hotplug usbredir tcp
channels in libxl?

So I'm not familiar with usbredir tcp channels.  I'm guessing that it
allows you to transmit the USB commands "over the wire", so that you
could connect (say) a keyboard or printer on your local computer to the
qemu process running in a remote dom0, and have the USB device presented
as an emulated device to the guest?


Yes.
In qemu upstream there are 2 methods of usbredir use, one is "dynamic" 
using spice. I made a quick and easy implementation in libxl some time ago 
at domU's create:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f5414ee57a17500e650ea11766474b11da940da2 
this also requires an emulated usb<1|2|3> controller, which I added with 
this:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bba6747189fb9c4cb51f3c99977d224615106c59 
It works fine with both windows and linux domUs and I already use it in 
production but it works ONLY using spice.




Not right. The usbredir is _separated_ protocol from spice 
(http://www.spice-space.org/page/UsbRedir).
I know, I spoked about what is actually implemented in libxl and below in my 
old mail there was already details about the other method.
What I asked are advices on how is good implement the other method (upstream 
qemu's usbredir tcp channels) in libxl (about the other code in addition to 
the qmp commands functions itself that I'm writing now in libxl_qmp.c), 2 
days ago I only did some fast "manual external" tests with Sid and W7 domUs 
to be sure is it working hotplug (with qmp) before start to implement it in 
libxl.

Sorry for my bad english if it is not understandable what I write.
I am using this protocol successfully with raspberry-pi "remote usb 
server":

- local usb  enumerator and filter
- remote qmp sender to qemu () (chardev-add/device_add) to
  program "tcp listen" port on qemu
  eg. "remotely" do full automation/filtering (hotplug) - and usbredir 
server () connected to prepared port qemu


(this is part of my VDI solution 
https://gridforums.nvidia.com/default/topic/752/grid-vgpu-benchmarks/vdi-click-to-photon-with-raspberry-pi/)


I will be very pleased if paravirtualized-usb channel will be created
with usb redir protocol in mind. There is problems in qemu "emulation" 
speed - when usb is remotely attached qemu takes about 20% cpu (probably 
problem of active "pooling" usb devices DomU/Dom0) without any traffic on 
tcp channel.


Have you tried to use an usb3 emulated controller? with the usb3 if I 
remember good this problem is mitigate.
If you add usb controller at domU's create with usb=1 (in domU's xl cfg) it 
add the qemu default than with the old chipset (the only currently used by 
xen) is usb1 controller, with the q35 (not supported by xen) the default is 
usb2 controller instead. Can you use usbversion=3 (added in xen 4.4) for add 
usb3 controller at domU's xl create or add nec-usb-xhci device (hotplug with 
"manual" qmp command).


Thanks for hint. I tried and I see "nec-usb-xhci" (info qtree) all devices 
pluged (info qtree, info chardev, info usb) and visible in os (lsusb in 
DomU linux) but _USB1_ devices does not work (keyboards/mouses) - tcp 
channel connected and tcpdump 

Re: [Xen-devel] [OSSTEST PATCH] crontab: Drop linux-mingo-tip-master linux-next linux-linus

[Ian Jackson]

Juergen Gross writes ("Re: [OSSTEST PATCH] crontab: Drop linux-mingo-tip-master 
linux-next linux-linus"):
> On 22/04/16 19:08, Roger Pau Monne wrote:
> > This looks fine to me, but I think I needs to be Acked by the Linux 
> > maintainers. FWIW:
> 
> Acked-by: Juergen Gross 

Thanks.  I have installed this new crontab.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] MAINTAINERS: Clarify the meaning of nested maintainership

[George Dunlap]

On 28/04/16 16:49, Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 28, 2016 at 11:28:20AM +0100, Wei Liu wrote:
>> On Wed, Apr 27, 2016 at 05:06:27PM +0100, George Dunlap wrote:
>>> Clarify the meaning of nested maintainership.
>>>
>>> Signed-off-by: George Dunlap 
>>> ---
>>> We had a discussion about the meaning of nested maintainership at the
>>> recent Xen Hackathon.  The notes of that meeting can be found on this
>>> list [1].  No decision is official until discussed on this list, so
>>> consider this patch the official proposal for this change, and object
>>> or ask for clarification accordingly.
>>>
>>> Compared to v1, there is one change that is worth pointing out: The
>>> claim that THE REST consists of all committers.  This is the case at
>>> the moment, but this change would codify that this is an invariant we
>>> intend to keep going forward.
>>>
>>> The advantage of this is that the dispute resolution mentioned in this
>>> patch for maintainers who can't agree lines up directly with the
>>> fall-back for broader community issues upon which we can't reach
>>> consensus.
>>>
>>> [1] marc.info/?i=
>>>
>>> Changes in v2:
>>> - fixed spelling of "maintainer"
>>> - fixed path of multi.c
>>> - clarified that the resolution by REST would be by *majority* vote
>>> - Asserted that The REST consists of all committers
>>>
>>> CC: Ian Jackson 
>>> CC: Jan Beulich 
>>> CC: Keir Fraser 
>>> CC: Tim Deegan 
>>> CC: Wei Liu 
>>> CC: Konrad Wilk 
>>> CC: Andrew Cooper 
>>> CC: Lars Kurth 
>>
>>
>> Acked-by: Wei Liu 
> 
> Acked-by: Konrad Rzeszutek Wilk 

Thanks Konrad.

I'll check this in tomorrow morning with whatever Acks it has at that
point unless someone objects before then.

 -George


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/HVM: fix forwarding of internally cached requests

[Xu, Quan]

On April 28, 2016 11:13 PM, Jan Beulich  wrote:
> >>> On 25.04.16 at 10:40,  wrote:
> > With other patches also in place, still not work.
> > Jianzhong  has been left and Quan will take over the task.
> 
> May I ask for another try, with current tip of staging plus
> http://lists.xenproject.org/archives/html/xen-devel/2016-
> 04/msg03661.html
> ?


Sure, I will try it on tomorrow afternoon or next Tuesday.
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Xu, Quan]

On April 28, 2016 11:12 PM, Jan Beulich  wrote:
> >>> On 28.04.16 at 17:03,  wrote:
> > On April 28, 2016 10:36 PM, Jan Beulich  wrote:
> >> >>> On 28.04.16 at 16:14,  wrote:
> >> > On April 25, 2016 7:53 PM, Jan Beulich  wrote:
> >> >> >>> On 18.04.16 at 16:00,  wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >
> >> >
> >> >> > -static void iommu_flush_all(void)
> >> >> > +static int iommu_flush_all(void)
> >> >>
> >> >> __must_check
> >> >>
> >> >
> >> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()
> >> > and vtd_crash_shutdown().
> >> > As we were on the same page, we can ignore the error code
> >> > propagation for these two call trees.
> >>
> >> I don't know what you're referring to here with "we were on the same
> page".
> > I
> >> don't think I've ever agreed (in the context of this series) to
> >> ignore any
> > error
> >> returns.
> >>
> >
> >
> > Look at the below link.
> > http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.h
> > tml
> 
> Which still talks about (conditionally) logging messages, not ignoring of 
> errors.
> 

Sorry, check it again.
in intel_iommu_hwdom_init() and  vtd_crash_shutdown(), 
I still need to log messages as:

if ( iommu_flush_all() )
 printk()

?

Thanks for your patience.
Quan






___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] MAINTAINERS: Clarify the meaning of nested maintainership

[Konrad Rzeszutek Wilk]

On Thu, Apr 28, 2016 at 11:28:20AM +0100, Wei Liu wrote:
> On Wed, Apr 27, 2016 at 05:06:27PM +0100, George Dunlap wrote:
> > Clarify the meaning of nested maintainership.
> > 
> > Signed-off-by: George Dunlap 
> > ---
> > We had a discussion about the meaning of nested maintainership at the
> > recent Xen Hackathon.  The notes of that meeting can be found on this
> > list [1].  No decision is official until discussed on this list, so
> > consider this patch the official proposal for this change, and object
> > or ask for clarification accordingly.
> > 
> > Compared to v1, there is one change that is worth pointing out: The
> > claim that THE REST consists of all committers.  This is the case at
> > the moment, but this change would codify that this is an invariant we
> > intend to keep going forward.
> > 
> > The advantage of this is that the dispute resolution mentioned in this
> > patch for maintainers who can't agree lines up directly with the
> > fall-back for broader community issues upon which we can't reach
> > consensus.
> > 
> > [1] marc.info/?i=
> > 
> > Changes in v2:
> > - fixed spelling of "maintainer"
> > - fixed path of multi.c
> > - clarified that the resolution by REST would be by *majority* vote
> > - Asserted that The REST consists of all committers
> > 
> > CC: Ian Jackson 
> > CC: Jan Beulich 
> > CC: Keir Fraser 
> > CC: Tim Deegan 
> > CC: Wei Liu 
> > CC: Konrad Wilk 
> > CC: Andrew Cooper 
> > CC: Lars Kurth 
> 
> 
> Acked-by: Wei Liu 

Acked-by: Konrad Rzeszutek Wilk 
> 
> > ---
> >  MAINTAINERS | 34 ++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5af7a0c..9057f96 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -94,6 +94,40 @@ Descriptions of section entries:
> >   printk, pr_info or pr_err
> >One regex pattern per line.  Multiple K: lines acceptable.
> >  
> > +
> > +The meaning of nesting:
> > +
> > +Many maintainership areas are "nested": for example, there are entries
> > +for xen/arch/x86 as well as xen/arch/x86/mm, and even
> > +xen/arch/x86/mm/shadow; and there is a section at the end called "THE
> > +REST" which lists all committers.  The meaning of nesting is that:
> > +
> > +1. Under normal circumstances, the Ack of the most specific maintainer
> > +is both necessary and sufficient to get a change to a given file
> > +committed.  So a change to xen/arch/x86/mm/shadow/multi.c requires the
> > +the Ack of the xen/arch/x86/mm/shadow maintainer for that part of the
> > +patch, but would not require the Ack of the xen/arch/x86 maintainer or
> > +the xen/arch/x86/mm maintainer.
> > +
> > +(A patch of course needs acks from the maintainers of each file that
> > +it changes; so a patch which changes xen/arch/x86/traps.c,
> > +xen/arch/x86/mm/p2m.c, and xen/arch/x86/mm/shadow/multi.c would
> > +require an Ack from each of the three sets of maintainers.)
> > +
> > +2. In unusual circumstances, a more general maintainer's Ack can stand
> > +in for or even overrule a specific maintainer's Ack.  Unusual
> > +circumstances might include:
> > + - The patch is fixing a high-priority issue causing immediate pain,
> > + and the more specific maintainer is not available.
> > + - The more specific maintainer has not responded either to the
> > + original patch, nor to "pings", within a reasonable amount of time.
> > + - The more general maintainer wants to overrule the more specific
> > + maintainer on some issue. (This should be exceptional.)
> > + - In the case of a disagreement between maintainers, THE REST can
> > + settle the matter by majority vote.  (This should be very exceptional
> > + indeed.)
> > +
> > +
> >  Maintainers List (try to look for most precise areas first)
> >  
> > ---
> > -- 
> > 2.1.4
> > 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [xen-unstable-smoke test] 93093: tolerable all pass - PUSHED

[osstest service owner]

flight 93093 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93093/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  1d5b507e69d67d2a6eee54286e44b5b7cc525e6c
baseline version:
 xen  c106e5b039c712fc7b6f3ce3c82d367de09b8d98

Last test of basis93020  2016-04-27 22:01:53 Z0 days
Testing same since93093  2016-04-28 14:01:33 Z0 days1 attempts


People who touched revisions under test:
  Jan Beulich 
  Roger Pau Monné 
  Stefano Stabellini 
  Yu Zhang 

jobs:
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-amd64-amd64-xl-qemuu-debianhvm-i386 pass
 test-amd64-amd64-libvirt pass



sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/logs/logs
images: /home/logs/images

Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs

Explanation of these reports, and of osstest in general, is at
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master
http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master

Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary


Pushing revision :

+ branch=xen-unstable-smoke
+ revision=1d5b507e69d67d2a6eee54286e44b5b7cc525e6c
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x '!=' x/home/osstest/repos/lock ']'
++ OSSTEST_REPOS_LOCK_LOCKED=/home/osstest/repos/lock
++ exec with-lock-ex -w /home/osstest/repos/lock ./ap-push xen-unstable-smoke 
1d5b507e69d67d2a6eee54286e44b5b7cc525e6c
+ branch=xen-unstable-smoke
+ revision=1d5b507e69d67d2a6eee54286e44b5b7cc525e6c
+ . ./cri-lock-repos
++ . ./cri-common
+++ . ./cri-getconfig
+++ umask 002
+++ getrepos
 getconfig Repos
 perl -e '
use Osstest;
readglobalconfig();
print $c{"Repos"} or die $!;
'
+++ local repos=/home/osstest/repos
+++ '[' -z /home/osstest/repos ']'
+++ '[' '!' -d /home/osstest/repos ']'
+++ echo /home/osstest/repos
++ repos=/home/osstest/repos
++ repos_lock=/home/osstest/repos/lock
++ '[' x/home/osstest/repos/lock '!=' x/home/osstest/repos/lock ']'
+ . ./cri-common
++ . ./cri-getconfig
++ umask 002
+ select_xenbranch
+ case "$branch" in
+ tree=xen
+ xenbranch=xen-unstable-smoke
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-smoke
+ prevxenbranch=xen-4.6-testing
+ '[' x1d5b507e69d67d2a6eee54286e44b5b7cc525e6c = x ']'
+ : tested/2.6.39.x
+ . ./ap-common
++ : osst...@xenbits.xen.org
+++ getconfig OsstestUpstream
+++ perl -e '
use Osstest;
readglobalconfig();
print $c{"OsstestUpstream"} or die $!;
'
++ :
++ : git://xenbits.xen.org/xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/xen.git
++ : git://xenbits.xen.org/qemu-xen-traditional.git
++ : git://git.kernel.org
++ : git://git.kernel.org/pub/scm/linux/kernel/git
++ : git
++ : git://xenbits.xen.org/libvirt.git
++ : osst...@xenbits.xen.org:/home/xen/git/libvirt.git
++ : git://xenbits.xen.org/libvirt.git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : git
++ : git://xenbits.xen.org/rumpuser-xen.git
++ : osst...@xenbits.xen.org:/home/xen/git/rumpuser-xen.git
+++ besteffort_repo https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ cached_repo https://github.com/rumpkernel/rumpkernel-netbsd-src 
'[fetch=try]'
+++ local repo=https://github.com/rumpkernel/rumpkernel-netbsd-src
+++ local 'options=[fetch=try]'
 getconfig GitCacheProxy
 perl -e '
use Osstest;
readglobalconfig();
print 

Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command

[Wei Liu]

On Wed, Mar 16, 2016 at 05:22:27AM -0600, Jan Beulich wrote:
> >>> On 15.03.16 at 16:38,  wrote:
> > Jan Beulich writes ("Re: [PATCH v2 3/3] xl: new "loglvl" command"):
> >> Yes and no. If all of the sudden the hypervisor didn't have an "error"
> >> log level anymore, what would you do? Mapping "error" to "warning"
> >> wouldn't be right. Nor would mapping it to anything else. Correct
> >> behavior in that case would simply be failure, and it wouldn't seem
> >> too relevant to me at what layer that failure would get signaled.
> > 
> > I think you are looking at this the wrong way.
> 
> Quite possible, and all of what you write makes sense. Yet that
> wasn't my intention here. I specifically put the string <-> number
> mapping in xl, so it could be that (and only that, outside the
> hypervisor itself) which gets changed if the hypervisor log levels
> ever change. The tool could use version information or some
> other detection mechanism to provide backwards compatibility
> (and be independent of the precise hypervisor version it got
> built in parallel with, if that's desired). And hence I specifically
> made the interfaces dumb - raw numbers, with no meaning
> assigned to their values.
> 
> And then, with what you describe I assume the current hypervisor
> side implementation wouldn't be suitable anymore anyway, as the
> translation between the interface exposed log levels and the
> internally used ones would need to happen in the sysctl handler.
> 
> To me, all of this looks increasingly like over-engineering for a
> very simple debugging aid (which is all the new command was
> meant for). If you and Wei can settle on some alternative
> implementation, I'm fine to accept that, but I don't think I'm
> going to spend much more time on fiddling with any of the 3
> patches. It's going to be sad though if even the serial console
> based log level adjustment won't make it into 4.7, despite it
> having got posted months ago (with this v2 just extending on
> it).
> 

If this is just a debugging aid and not intending to be consumed by high
level toolstack, maybe we can make a dedicated helper program? We
already have a bunch of those. Should the need really arises we can
then consider making it proper stable API / ABI.

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/hvm: Add debug exception vm_events

[Tamas K Lengyel]

On Wed, Apr 27, 2016 at 11:57 PM, Razvan Cojocaru  wrote:

> On 04/27/16 23:33, Tamas K Lengyel wrote:
> > Since in-guest debug exceptions are now unconditionally trapped to Xen,
> adding
> > a hook for vm_event subscribers to tap into this new always-on guest
> event. We
> > rename along the way hvm_event_breakpoint_type to hvm_event_type to
> better
> > match the various events that can be passed with it. We also introduce
> the
> > necessary monitor_op domctl's to enable subscribing to the events.
>
> The monitor / event code looks fine, but I'm not sure about
> hvm_event_type / hvm_monitor_event(). Presumably even a CR3 write event
> coming from a HVM guest is an event, and HVM-related, hence a hvm_event
> - i.e. the generic nature of the name is a bit confusing to me. Maybe
> not so to other reviewers?
>

Yea, I'm not too happy with the naming in general for hvm/event, it all
should be renamed to be clear that it's part of the monitor subsystem. I
think I'll turn this into a 2-patch series, where I rename the existing
stuff from hvm/event to hvm/monitor then change the name of this function
from hvm_monitor_breakpoint to hvm_monitor_debug.

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Jan Beulich]

>>> On 28.04.16 at 17:03,  wrote:
> On April 28, 2016 10:36 PM, Jan Beulich  wrote:
>> >>> On 28.04.16 at 16:14,  wrote:
>> > On April 25, 2016 7:53 PM, Jan Beulich  wrote:
>> >> >>> On 18.04.16 at 16:00,  wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >
>> >
>> >> > -static void iommu_flush_all(void)
>> >> > +static int iommu_flush_all(void)
>> >>
>> >> __must_check
>> >>
>> >
>> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()  and
>> > vtd_crash_shutdown().
>> > As we were on the same page, we can ignore the error code propagation
>> > for these two call trees.
>> 
>> I don't know what you're referring to here with "we were on the same page". 
> I
>> don't think I've ever agreed (in the context of this series) to ignore any 
> error
>> returns.
>> 
> 
> 
> Look at the below link. 
> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.html 

Which still talks about (conditionally) logging messages, not ignoring
of errors.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2] x86/HVM: fix forwarding of internally cached requests

[Jan Beulich]

>>> On 25.04.16 at 10:40,  wrote:
> With other patches also in place, still not work. 
> Jianzhong  has been left and Quan will take over the task.

May I ask for another try, with current tip of staging plus
http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg03661.html
?

Thanks, Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] Fix cpumap setting before passing to XEN

[Wei Liu]

On Tue, Apr 26, 2016 at 11:54:32AM +0800, Zhenzhong Duan wrote:
> On 2016/4/25 21:26, Ian Jackson wrote:
> >Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] [PATCH] Fix cpumap setting 
> >before passing to XEN"):
> >>On Wed, Apr 20, 2016 at 03:33:13PM +0100, Wei Liu wrote:
> >>>In principle I think having python binding and xl/libxl behave more or less
> >>>the same is the right direction. I'm a bit nervous about the change of
> >>>behaviour on the other hand.
> >>>
> >>>Let's wait for a few more days to see if other people have any comment on
> >>>this.
> >>Reviewed-by: Konrad Rzeszutek Wilk  ?
> >Does this bug report mean that `xm vcpu-pin ... all' has never
> >worked properly ?  Can that really be the case ?
> Xen 4.3 doesn't work, Xen 3.4 works.
> I have no Xen 4.4 around to test that, but checked code, it will not.
> Then I found below commit involved.
> 
> commit 41abbadef60e5fccdfd688579dd458f7f7887cf5
> Author: Ian Jackson 
> Date:   Wed May 29 15:48:11 2013 +0100
> 
> libxc: limit cpu values when setting vcpu affinity
> 
> When support for pinning more than 64 cpus was added, check for cpu
> out-of-range values was removed. This can lead to subsequent
> out-of-bounds cpumap array accesses in case the cpu number is higher
> than the actual count.
> 
> This patch returns the check.
> 
> This is CVE-2013-2072 / XSA-56
> 
> Signed-off-by: Petr Matousek 
> >
> >Also, xm exists in Xen 4.4 and earlier, only.  Xen 4.4 is no longer
> >supported upstream, so we would not apply this patch to Xen 4.4.  So
> >whatever we do, this is not going to fix any bug in `xm vcpu-pin' in
> >4.4.
> The only impact is upper layer or the user need to pass a correct cpumap
> param not beyond the real cpu map to avoid the error.
> But I am not clear if python binding is still used or will be removed just
> as Xend.

I don't think we have plan to remove it any time soon. On the other hand
because no in-tree component uses it so we don't know whether it works
in practice or not.

> >
> >This doesn't necessarily mean that I object to changing the behaviour
> >of the python xc module in still-supported Xen releases.  But I'm not
> >sure the reasoning behind the behaviour of the libxl bitmap functions
> >applies to the Python interface.
> >
> >Zhenzhong Duan, are you using an out-of-tree copy of xm and xend ?
> I am using xen-4.3.0-55.el6.47.33 which is Xen 4.3 variant
> 

So what is the conclusion of this discussion so far? I admit I'm a bit
lost here.

Wei.

> thanks
> zduan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Xu, Quan]

On April 28, 2016 10:36 PM, Jan Beulich  wrote:
> >>> On 28.04.16 at 16:14,  wrote:
> > On April 25, 2016 7:53 PM, Jan Beulich  wrote:
> >> >>> On 18.04.16 at 16:00,  wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >
> >
> >> > -static void iommu_flush_all(void)
> >> > +static int iommu_flush_all(void)
> >>
> >> __must_check
> >>
> >
> > The iommu_flush_all() is also called in intel_iommu_hwdom_init()  and
> > vtd_crash_shutdown().
> > As we were on the same page, we can ignore the error code propagation
> > for these two call trees.
> 
> I don't know what you're referring to here with "we were on the same page". I
> don't think I've ever agreed (in the context of this series) to ignore any 
> error
> returns.
> 


Look at the below link. 
http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03234.html

I hope I understand this correctly.

Quan

> Jan
> 
> > I wonder whether we really need this '__must_check' or not,
> > furthermore, print-out message Is pointless (at least, to me).
> >
> > Jan, could I ignore this '__must_check '?
> >
> > Quan
> 
> 
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] how to set up a #VE

[Tamas K Lengyel]

On Thu, Apr 28, 2016 at 8:36 AM, Big Strong  wrote:

> I want to set up an EPT page so as to trigger the #VE for testing purpose.
> However, some problems are met.
>
> As the Intel Manual said, there are many conditions to trigger a #VE:
>
> a)  If an access to a guest-physical address causes an EPT violation, bit
> 63 (0) of exactly one of the EPT paging-structure entries used to
> translate that address is used to determine *whether the EPT violation is
> convertible*: either an entry that is not present (if the guest-physical
> address does not translate to a physical address) or an entry that maps a
> page (if it does).
>
> b)  A convertible EPT violation instead causes a virtualization
> exception if the following all hold:
>
> • CR0.PE = 1;
>
> • the logical processor is not in the process of delivering an event
> through the IDT; and
>
> • the 32 bits at offset 4 in the virtualization-exception information
> area are all 0.
> In xc_altp2m.c, there is a function xc_altp2m_set_vcpu_enable_notify
> which is used to set up the #VE information area. However, as the arguments
> gfn is a physical address (of the guest?), how can I safely assign an
> unused physical memory space to store #VE info?
>

You can always just add a new page to the domain to be used for #VE.


>
> Besides, there is no xenctrl interface for setting the suprress_ve bit
> (63) of the EPT PTE, which is needed to trigger #VE. Even though I can set
> that with ept_set_entry function, this is an internal function of Xen and
> unavailble to dom0.
>

It's undocumented enough (and it took me a bit to find as well) but if you
use xc_altp2m_set_mem_access and have used xc_altp2m_set_vcpu_enable_notify
before, then those EPT PTE entries will be converted to #VE automatically.

Tamas
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v2] x86/vMSI-X: also snoop REP MOVS

[Jan Beulich]

... as at least certain versions of Windows use such to update the
MSI-X table. However, to not overly complicate the logic for now
- only EFLAGS.DF=0 is being handled,
- only updates not crossing MSI-X table entry boundaries are handled.

Signed-off-by: Jan Beulich 
---
v2: Comment conditional being added to msixtbl_range().

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
 ASSERT(r->type == IOREQ_TYPE_COPY);
 if ( r->dir == IOREQ_WRITE )
 {
+unsigned int size = r->size;
+
 if ( !r->data_is_ptr )
 {
-unsigned int size = r->size;
 uint64_t data = r->data;
 
 if ( size == 8 )
@@ -366,7 +367,29 @@ static int msixtbl_range(struct vcpu *v,
  ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
   PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
  !(data & PCI_MSIX_VECTOR_BITMASK) )
+{
 v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
+}
+}
+else if ( (size == 4 || size == 8) &&
+  /* Only support forward REP MOVS for now. */
+  !r->df &&
+  /*
+   * Only fully support accesses to a single table entry for
+   * now (if multiple ones get written to in one go, only the
+   * final one gets dealt with).
+   */
+  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
+  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
+{
+BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
+ (PCI_MSIX_ENTRY_SIZE - 1));
+
+v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
+addr + size * r->count - 4;
+v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
+r->data + size * r->count - 4;
 }
 }
 
@@ -471,6 +494,7 @@ out:
 for_each_vcpu ( d, v )
 {
 if ( (v->pause_flags & VPF_blocked_in_xen) &&
+ !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
  v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
  (gtable + msi_desc->msi_attrib.entry_nr *
PCI_MSIX_ENTRY_SIZE +
@@ -561,9 +585,29 @@ void msixtbl_pt_cleanup(struct domain *d
 void msix_write_completion(struct vcpu *v)
 {
 unsigned long ctrl_address = v->arch.hvm_vcpu.hvm_io.msix_unmask_address;
+unsigned long snoop_addr = v->arch.hvm_vcpu.hvm_io.msix_snoop_address;
 
 v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
 
+if ( !ctrl_address && snoop_addr &&
+ v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
+{
+const struct msi_desc *desc;
+uint32_t data;
+
+rcu_read_lock(_rcu_lock);
+desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
+snoop_addr);
+rcu_read_unlock(_rcu_lock);
+
+if ( desc &&
+ hvm_copy_from_guest_phys(,
+  v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
+  sizeof(data)) == HVMCOPY_okay &&
+ !(data & PCI_MSIX_VECTOR_BITMASK) )
+ctrl_address = snoop_addr;
+}
+
 if ( !ctrl_address )
 return;
 
--- a/xen/include/asm-x86/hvm/vcpu.h
+++ b/xen/include/asm-x86/hvm/vcpu.h
@@ -86,6 +86,7 @@ struct hvm_vcpu_io {
 
 unsigned long msix_unmask_address;
 unsigned long msix_snoop_address;
+unsigned long msix_snoop_gpa;
 
 const struct g2m_ioport *g2m_ioport;
 };



x86/vMSI-X: also snoop REP MOVS

... as at least certain versions of Windows use such to update the
MSI-X table. However, to not overly complicate the logic for now
- only EFLAGS.DF=0 is being handled,
- only updates not crossing MSI-X table entry boundaries are handled.

Signed-off-by: Jan Beulich 
---
v2: Comment conditional being added to msixtbl_range().

--- a/xen/arch/x86/hvm/vmsi.c
+++ b/xen/arch/x86/hvm/vmsi.c
@@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
 ASSERT(r->type == IOREQ_TYPE_COPY);
 if ( r->dir == IOREQ_WRITE )
 {
+unsigned int size = r->size;
+
 if ( !r->data_is_ptr )
 {
-unsigned int size = r->size;
 uint64_t data = r->data;
 
 if ( size == 8 )
@@ -366,7 +367,29 @@ static int msixtbl_range(struct vcpu *v,
  ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
   PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
  !(data & PCI_MSIX_VECTOR_BITMASK) )
+{
 v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
+v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
+}
+}
+else if ( (size == 4 || size == 8) &&
+  /* Only support 

Re: [Xen-devel] [PATCH v2] altp2m: Allow the hostp2m to be shared

[Tamas K Lengyel]

On Thu, Apr 28, 2016 at 4:36 AM, George Dunlap 
wrote:

> On 27/04/16 16:48, Tamas K Lengyel wrote:
> > Don't propagate altp2m changes from ept_set_entry for memshare as
> memshare
> > already has the lock. We call altp2m propagate changes once memshare
> > successfully finishes. Allow the hostp2m entries to be of type
> > p2m_ram_shared when applying mem_access. Also, do not trigger PoD for
> hostp2m
> > when setting altp2m mem_access to be in-line with non-altp2m mem_access
> path.
> >
> > Signed-off-by: Tamas K Lengyel 
> > ---
> > Cc: George Dunlap 
> > Cc: Keir Fraser 
> > Cc: Jan Beulich 
> > Cc: Andrew Cooper 
> > Cc: Jun Nakajima 
> > Cc: Kevin Tian 
> >
> > v2: Cosmetic fixes and addressing PoD in the commit message
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 10 +-
> >  xen/arch/x86/mm/p2m-ept.c |  2 +-
> >  xen/arch/x86/mm/p2m.c |  5 ++---
> >  3 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c
> b/xen/arch/x86/mm/mem_sharing.c
> > index a522423..6693661 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -35,6 +35,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >
> >  #include "mm-locks.h"
> > @@ -926,11 +927,12 @@ int mem_sharing_share_pages(struct domain *sd,
> unsigned long sgfn, shr_handle_t
> >  int ret = -EINVAL;
> >  mfn_t smfn, cmfn;
> >  p2m_type_t smfn_type, cmfn_type;
> > +p2m_access_t cmfn_a;
> >  struct two_gfns tg;
> >  struct rmap_iterator ri;
> >
> >  get_two_gfns(sd, sgfn, _type, NULL, ,
> > - cd, cgfn, _type, NULL, ,
> > + cd, cgfn, _type, _a, ,
> >   0, );
> >
> >  /* This tricky business is to avoid two callers deadlocking if
> > @@ -1026,6 +1028,12 @@ int mem_sharing_share_pages(struct domain *sd,
> unsigned long sgfn, shr_handle_t
> >  /* We managed to free a domain page. */
> >  atomic_dec(_shared_mfns);
> >  atomic_inc(_saved_mfns);
> > +
> > +/* Save the change to the altp2m tables as well if active */
> > +if ( altp2m_active(cd) )
> > +p2m_altp2m_propagate_change(cd, _gfn(cgfn), smfn, PAGE_ORDER_4K,
> > +p2m_ram_shared, cmfn_a);
> > +
> >  ret = 0;
> >
> >  err_out:
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index 1ed5b47..ff84242 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -847,7 +847,7 @@ out:
> >  if ( is_epte_present(_entry) )
> >  ept_free_entry(p2m, _entry, target);
> >
> > -if ( rc == 0 && p2m_is_hostp2m(p2m) )
> > +if ( rc == 0 && p2m_is_hostp2m(p2m) && p2mt != p2m_ram_shared )
> >  p2m_altp2m_propagate_change(d, _gfn(gfn), mfn, order, p2mt,
> p2ma);
>
> Sorry, just double-checking here: This is the right thing to do because
> the only code that is *setting* a page p2m_ram_shared should be the
> sharing code, which (after this patch) should be propagating the change
> itself?
>

The problem is with the mm-lock really. The mem_sharing paths already apply
mem_sharing_page_lock when changing the type to p2m_ram_shared, which
interferes with altp2m_list_lock. It would be fine to propagate the changes
to the altp2m tables otherwise if we could make the locks not interfere,
but that would require more changes then this patch.


>
> I'm just conscious that this introduces an invariant that would be easy
> to accidentally break.
>
> At the moment the only place that calls set_entry() with p2m_ram_shared
> is in p2m.c:set_shared_p2m_entry(), which is called from
> mem_sharing.c:__mem_sharing_unshare_page() and
> mem_sharing.c:mem_sharing_share_pages().  You've handled the second case
> by adding the if() clause at the end of the operation; and it looks like
> the first case is  handled because the shared page will be overwritten
> by the p2m_change_type_one().
>

Right, I don't really see much point in setting the pages shared (again)
right before unsharing overwrites it so there is no altp2m propagation
there.. Then p2m_change_type_one is outside the mem_sharing_page_lock so
there wasn't a need for an extra altp2m check.


>
> I think at very least adding a comment here saying something like the
> following would be helpful:
>
> "Memsharing is responsible for updating altp2ms after the sharing
> operation is complete"
>
> And perhaps a note before the p2m_change_type_one() in
> __mem_sharing_unshare_page() saying something like:
>
> "This will take care of updating the altp2m mappings."
>
> What do you think?
>

That sounds all good to me. The mem_sharing code is undocumented enough
that IMHO adding more comments would help with making it more maintainable
going forward =)

Thanks,
Tamas

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Jan Beulich]

>>> On 28.04.16 at 16:14,  wrote:
> On April 25, 2016 7:53 PM, Jan Beulich  wrote:
>> >>> On 18.04.16 at 16:00,  wrote:
>> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> 
> 
>> > -static void iommu_flush_all(void)
>> > +static int iommu_flush_all(void)
>> 
>> __must_check
>> 
> 
> The iommu_flush_all() is also called in intel_iommu_hwdom_init()  and 
> vtd_crash_shutdown().
> As we were on the same page, we can ignore the error code propagation for 
> these two call trees.

I don't know what you're referring to here with "we were on the same
page". I don't think I've ever agreed (in the context of this series) to
ignore any error returns.

Jan

> I wonder whether we really need this '__must_check' or not, furthermore, 
> print-out message
> Is pointless (at least, to me).
> 
> Jan, could I ignore this '__must_check '?
> 
> Quan




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] how to set up a #VE

[Big Strong]

I want to set up an EPT page so as to trigger the #VE for testing purpose.
However, some problems are met.

As the Intel Manual said, there are many conditions to trigger a #VE:

a)  If an access to a guest-physical address causes an EPT violation, bit
63 (0) of exactly one of the EPT paging-structure entries used to translate
that address is used to determine *whether the EPT violation is convertible*:
either an entry that is not present (if the guest-physical address does not
translate to a physical address) or an entry that maps a page (if it does).

b)  A convertible EPT violation instead causes a virtualization
exception if the following all hold:

• CR0.PE = 1;

• the logical processor is not in the process of delivering an event
through the IDT; and

• the 32 bits at offset 4 in the virtualization-exception information area
are all 0.
In xc_altp2m.c , there is a function
xc_altp2m_set_vcpu_enable_notify which is used to set up the #VE
information area. However, as the arguments gfn is a physical address (of
the guest?), how can I safely assign an unused physical memory space to
store #VE info?

Besides, there is no xenctrl interface for setting the suprress_ve bit (63)
of the EPT PTE, which is needed to trigger #VE. Even though I can set that
with ept_set_entry function, this is an internal function of Xen and
unavailble to dom0.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [RFC PATCH v6 00/28] libxl: Deprivilege qemu

[Ian Jackson]

Ian Jackson writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege qemu"):
> Stefano Stabellini writes ("Re: [RFC PATCH v6 00/28] libxl: Deprivilege 
> qemu"):
> > I take that this series is going to miss 4.7 at this stage, right?
> 
> I'm afraid so.  We concluded that a crucial piece - arranging for the
> necessary access controls on privcmd - was not going to be in place
> for 4.7.
> 
> Without that, this series does not offer much additional security.  It
> is a shame that the code will rot a bit in the meantime as a result of
> it missing 4.7.

FYI, the Citrix XenServer team is interested in work in this area.
I've been having some conversations with them, to see how we can best
work together.

Watch this space.  I'm hopeful that this work (or something
substantially similar) will be ready for Xen 4.8.

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [xen-4.3-testing test] 93074: trouble: blocked/broken/fail/pass

[osstest service owner]

flight 93074 xen-4.3-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93074/

Failures and problems with tests :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 build-amd64   3 host-install(3) broken REGR. vs. 87893
 build-i386-pvops  3 host-install(3) broken REGR. vs. 87893
 build-amd64-pvops 3 host-install(3) broken REGR. vs. 87893
 build-i3863 host-install(3) broken REGR. vs. 87893

Tests which did not succeed, but are not blocking:
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-rumpuserxen-i386  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pygrub   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   blocked  n/a
 build-amd64-rumpuserxen   1 build-check(1)   blocked  n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pair 1 build-check(1)   blocked  n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pv   1 build-check(1)   blocked  n/a
 build-i386-rumpuserxen1 build-check(1)   blocked  n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-xend-qemut-winxpsp3  1 build-check(1)  blocked n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-qemut-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-pv1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-xl-qemut-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-qemut-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemut-winxpsp3  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemuu-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-winxpsp3  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl-qcow2 1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-qemut-win7-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 test-amd64-amd64-xl-qemuu-ovmf-amd64  1 build-check(1) blocked n/a
 test-amd64-amd64-xl-qemuu-debianhvm-amd64  1 build-check(1)blocked n/a
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-amd64-amd64-rumpuserxen-amd64  1 build-check(1)   blocked n/a
 test-amd64-amd64-xl-qemut-debianhvm-amd64  1 build-check(1)blocked n/a
 test-armhf-armhf-xl   6 xen-boot fail   never pass
 test-armhf-armhf-libvirt  6 xen-boot fail   never pass
 test-armhf-armhf-xl-vhd   6 xen-boot fail   never pass
 test-armhf-armhf-xl-cubietruck  6 xen-boot fail never pass
 test-armhf-armhf-libvirt-raw  6 xen-boot fail   never pass
 test-armhf-armhf-xl-credit2   6 xen-boot fail   never pass
 test-armhf-armhf-libvirt-qcow2  6 xen-boot fail never pass
 test-armhf-armhf-xl-multivcpu  6 xen-boot fail  never pass
 test-armhf-armhf-xl-arndale   6 xen-boot fail   never pass

version targeted for testing:
 xen  c04846eeb3d96cf670dc5894b66f3f6e61c2531d
baseline version:
 xen  8fa31952e2d08ef63897c43b5e8b33475ebf5d93

Last test of basis87893  2016-03-29 13:49:52 Z   30 days
Testing same since92180  2016-04-20 17:49:21 Z7 days   20 attempts

Re: [Xen-devel] Questions about the new usb hotplug code in libxl and about adding hotplug (with qmp) usbredir tcp channels

[Fabio Fantoni]

Il 27/04/2016 20:03, Martin Cerveny ha scritto:



On Wed, 27 Apr 2016, Fabio Fantoni wrote:


Il 27/04/2016 13:26, George Dunlap ha scritto:

On 27/04/16 12:02, Fabio Fantoni wrote:

Hi, I took a look at the new pvusb hotplug code in libxl to try to add
also hotplug (with qmp) usbredir tcp channels.
Adding usbredir tcp channels at domU start requires for example adding
qemu parameters like these: "-chardev
socket,id=charredir4,host=192.168.1.35,port=4 -device
usb-redir,chardev=charredir4,id=redir4".
It is possible to hotplug it with qmp using "chardev-add" and
"device_add" commands.
Looking at old George Dunlap's patches I tested years ago
(http://xenbits.xen.org/gitweb/?p=people/gdunlap/xen.git;a=commitdiff;h=f7a77843e3fcf070c72115be8ed349a3bfe34e60) 


I can understand what they do and I can add similar qmp functions for
usbredir tcp too.
But now I see that bigger and different usb hotplug code was added, I
looked at these patches:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bf7628f087b212052a0e9f024044b2790c33f820 



http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=043910384cb9ea2c781a7dceac238e110a559c10 



and the full current code in xen's staging branch but I didn't find 
qmp

commands for the qemu usb passthrough, I suppose it is missing or
incomplete (though strange), am I wrong?
If that is correct, pvusb drivers are needed for both host and domU to
have usb passthrough working but in new windows pv drivers, the pvusb
one is missing, so without the "qemu emulated" usb passthrough it
doesn't work at all in similar cases, right?

That's right -- the PVUSB code *only* does PVUSB passthrough. The
interface was designed, however, to be able to add emulated USB on top.


How do you think I should proceed to implement hotplug usbredir tcp
channels in libxl?

So I'm not familiar with usbredir tcp channels.  I'm guessing that it
allows you to transmit the USB commands "over the wire", so that you
could connect (say) a keyboard or printer on your local computer to the
qemu process running in a remote dom0, and have the USB device 
presented

as an emulated device to the guest?


Yes.
In qemu upstream there are 2 methods of usbredir use, one is 
"dynamic" using spice. I made a quick and easy implementation in 
libxl some time ago at domU's create:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=f5414ee57a17500e650ea11766474b11da940da2 

this also requires an emulated usb<1|2|3> controller, which I added 
with this:
http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=bba6747189fb9c4cb51f3c99977d224615106c59 

It works fine with both windows and linux domUs and I already use it 
in production but it works ONLY using spice.




Not right. The usbredir is _separated_ protocol from spice 
(http://www.spice-space.org/page/UsbRedir).
I know, I spoked about what is actually implemented in libxl and below 
in my old mail there was already details about the other method.
What I asked are advices on how is good implement the other method 
(upstream qemu's usbredir tcp channels) in libxl (about the other code 
in addition to the qmp commands functions itself that I'm writing now in 
libxl_qmp.c), 2 days ago I only did some fast "manual external" tests 
with Sid and W7 domUs to be sure is it working hotplug (with qmp) before 
start to implement it in libxl.

Sorry for my bad english if it is not understandable what I write.
I am using this protocol successfully with raspberry-pi "remote usb 
server":

- local usb  enumerator and filter
- remote qmp sender to qemu () (chardev-add/device_add) to
  program "tcp listen" port on qemu
  eg. "remotely" do full automation/filtering (hotplug) - and usbredir 
server () connected to prepared port qemu


(this is part of my VDI solution 
https://gridforums.nvidia.com/default/topic/752/grid-vgpu-benchmarks/vdi-click-to-photon-with-raspberry-pi/)


I will be very pleased if paravirtualized-usb channel will be created
with usb redir protocol in mind. There is problems in qemu "emulation" 
speed - when usb is remotely attached qemu takes about 20% cpu 
(probably problem of active "pooling" usb devices DomU/Dom0) without 
any traffic on tcp channel.


Have you tried to use an usb3 emulated controller? with the usb3 if I 
remember good this problem is mitigate.
If you add usb controller at domU's create with usb=1 (in domU's xl cfg) 
it add the qemu default than with the old chipset (the only currently 
used by xen) is usb1 controller, with the q35 (not supported by xen) the 
default is usb2 controller instead. Can you use usbversion=3 (added in 
xen 4.4) for add usb3 controller at domU's xl create or add nec-usb-xhci 
device (hotplug with "manual" qmp command).





Thanks, Martin Cerveny


I want to add the other method to make passthrough of client usb 
devices possible without spice too.
Adding it at domU's create is quick and easy, I could do something 
like the first patch posted above which use the usb controller 
provided by the other patch 

Re: [Xen-devel] MiniOS on ARM64

[Julien Grall]

On 28/04/16 15:06, Wei Liu wrote:

Hi Julien


Hi Wei,


Can you also CC minios-devel@ in the future for relevant discussions?


Sorry I forgot about this ML. I will do it for the next discussion.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v2 09/11] IOMMU: propagate IOMMU Device-TLB flush error up to IOMMU suspending

[Xu, Quan]

On April 25, 2016 7:53 PM, Jan Beulich  wrote:
> >>> On 18.04.16 at 16:00,  wrote:
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c


> > -static void iommu_flush_all(void)
> > +static int iommu_flush_all(void)
> 
> __must_check
> 

The iommu_flush_all() is also called in intel_iommu_hwdom_init()  and 
vtd_crash_shutdown().
As we were on the same page, we can ignore the error code propagation for these 
two call trees.

I wonder whether we really need this '__must_check' or not, furthermore, 
print-out message
Is pointless (at least, to me).

Jan, could I ignore this '__must_check '?

Quan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] MiniOS on ARM64

[Wei Liu]

Hi Julien

Can you also CC minios-devel@ in the future for relevant discussions?

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] MiniOS on ARM64

[Chen Baozi]

Hi Julien,

Sorry for the late reply.

I have ported the basic framework to make it work. It
can be built and boot, but still in the early stage (lots
of functions are there but empty). The code on my github
(https://github.com/baozich/mini-os) is the latest version
(though there have been a long time since I modified it ;-)).
I think that could be a good start for Anastassios to work on.

Here is the command I use to build:

CONFIG_TEST=y CONFIG_START_NETWORK=n CONFIG_BLKFRONT=n CONFIG_NETFRONT=n \
CONFIG_FBFRONT=n CONFIG_KBDFRONT=n CONFIG_CONSFRONT=n CONFIG_XC=n \
MINIOS_TARGET_ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make

And the  XL config:

name = "mini-os"
vcpus = 1
memory = 128
gic_version = "v2"
kernel = "/path/to/mini-os.img"

Julien Grall  writes:

> Hi Chen,
>
> IIRC, you mentioned during the last Linaro Connect that you are working 
> on an ARM64 port of MiniOS.
>
> Anastassios, in CC, is interested get MiniOS running on ARM64 as well.
>
> Do you know what is missing to get MiniOS booting? I have found a tree 
> on your github for the port [1], is it the latest version?
>
> Cheers,

-- 
陈鲍孜 (Chen Baozi)

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Odroid XU3 support

[Suriyan Ramasami]

Hello Julien,
Thank you for the advice. I do have a follow up question.

On Thu, Apr 28, 2016 at 2:50 AM, Julien Grall  wrote:

> Hello,
>
> On 27/04/16 23:53, Suriyan Ramasami wrote:
>
> How can I check which core is currently active?
>> Judging by this link on big.LITTLE architecture:
>> http://forum.odroid.com/viewtopic.php?f=65=2580
>>
>> result of: cat /proc/cpuinfo | grep "CPU part" is
>> CPU part: 0xc07
>>
>> which stands for A7.
>>
>> If you do this in dom0, it will show all of them to be 0xc07. They are
>> vCPUs after all.
>>
>
> Which is not a good idea. This means that Linux is not able to detect
> potential errata for the underlying cores (in this case the cortex-A15).
> Also some userspace may do some runtime optimization based on the kind of
> CPUs available in the guest.
>
> Xen is not ready for big.LITTLE, so I would highly recommend you to
> disable either all the Cortex-A15 or Cortex-A7.
>
>
Ian did recommend that if they were in their own cpu pools it would avoid
mixing them in a guest. I was researching that angle. What is your take on
that?

If Linux is not recognizing it, that is a dom0/domU issue, is it not?

Nonetheless, to start with, to add support, I think there would be less
resistance if the boot cluster (a7) cpus are enabled and the other cluster
disabled (a15)

For your information, I am planning to send a patch to park any CPUs whose
> MIDR is not matching the boot CPU one.
>
> Regards,
>
> --
> Julien Grall
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [ovmf test] 93056: regressions - FAIL

[osstest service owner]

flight 93056 ovmf real [real]
http://logs.test-lab.xenproject.org/osstest/logs/93056/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-amd64-xl-qemuu-ovmf-amd64 9 debian-hvm-install fail REGR. vs. 65543
 test-amd64-i386-xl-qemuu-ovmf-amd64  9 debian-hvm-install fail REGR. vs. 65543

version targeted for testing:
 ovmf 467d5f6b30bcd2bb73bfaafc31118944d95ec28e
baseline version:
 ovmf 5ac96e3a28dd26eabee421919f67fa7c443a47f1

Last test of basis65543  2015-12-08 08:45:15 Z  142 days
Failing since 65593  2015-12-08 23:44:51 Z  141 days  185 attempts
Testing same since93056  2016-04-28 06:08:43 Z0 days1 attempts


People who touched revisions under test:
  "Samer El-Haj-Mahmoud" 
  "Wu, Hao A" 
  "Yao, Jiewen" 
  Abdul Lateef Attar 
  Alcantara, Paulo 
  Anbazhagan Baraneedharan 
  Andrew Fish 
  Ard Biesheuvel 
  Arthur Crippa Burigo 
  Cecil Sheng 
  Chao Zhang 
  Chao Zhang
  Charles Duffy 
  Cinnamon Shia 
  Cohen, Eugene 
  Dandan Bi 
  Daocheng Bu 
  Daryl McDaniel 
  David Woodhouse 
  Derek Lin 
  edk2 dev 
  edk2-devel 
  Eric Dong 
  Eric Dong 
  erictian
  Eugene Cohen 
  Evan Lloyd 
  Feng Tian 
  Fu Siyuan 
  Gabriel Somlo 
  Gary Ching-Pang Lin 
  Gary Lin 
  Ghazi Belaam 
  Hao Wu 
  Haojian Zhuang 
  Hegde Nagaraj P 
  Hegde, Nagaraj P 
  Hess Chen 
  Heyi Guo 
  Hot Tian 
  Jaben Carsey 
  James Bottomley 
  Jeff Fan 
  Jeremy Linton 
  Jiaxin Wu 
  jiewen yao 
  Jim Dailey 
  jim_dai...@dell.com 
  jljusten
  Jordan Justen 
  Juliano Ciocari 
  Justen, Jordan L 
  Karyne Mayer 
  Ken Lu 
  Kun Gui 
  Larry Hauch 
  Laszlo Ersek 
  Leahy, Leroy P
  Leahy, Leroy P 
  Lee Leahy 
  Leekha Shaveta 
  Leendert van Doorn 
  Leif Lindholm 
  Leif Lindholm 
  Leo Duran 
  Leon Li 
  Liming Gao 
  lzeng14
  Mark 
  Mark Doran 
  Mark Rutland 
  Marvin H?user 
  Marvin Haeuser 
  Marvin Häuser 
  marvin.haeu...@outlook.com 
  Michael D Kinney 
  Michael Kinney 
  Michael LeMay 
  Michael Thomas 
  Michał Zegan 
  Nagaraj Hegde 
  Ni, Ruiyu 
  Ni, Ruiyu 
  niruiyu
  Olivier Martin 
  Paolo Bonzini 
  Paulo Alcantara 
  Paulo Alcantara Cavalcanti 
  Peter Kirmeier 
  Qin Long 
  Qing Huang 
  Qiu Shumin 
  Qiu, Shumin 
  Rodrigo Dias Correa 
  Ruiyu Ni 
  Ryan Harkin 
  Samer El-Haj-Mahmoud 
  Samer El-Haj-Mahmoud 
  Sami Mujawar 
  Shivamurthy Shastri 
  Shumin Qiu 
  Star Zeng 
  Supreeth Venkatesh 
  Tapan Shah 
  Thomas Palmer 
  Tian Feng 
  Tian, Feng 
  Vladislav Vovchenko 
  Volker Rümelin 
  Yao Jiewen 
  Yao, 

Re: [Xen-devel] Odroid XU3 support

[Ivan Pavić2]

> Xen is not ready for big.LITTLE, so I would highly recommend you to
> disable either all the Cortex-A15 or Cortex-A7.

> For your information, I am planning to send a patch to park any CPUs
> whose MIDR is not matching the boot CPU one.

> Julien Grall

Ok, 

I decided to use A15s... How can I  disable A7s in software and use
only A15s... Additionally how can I be sure that I'm using A15 cluster?

This maybe more ODROID related:

Suriyan Ramasami wrote:

> The Odroid XU3/XU4 afaict has a hardware pin which dictates if a A7
> or an A15 core should boot up. It is currently set to boot up from the A7s.

This may sound silly but where is that PIN on XU3 board.? Can 
this be modfied(is it accessible)? If not, how can I do it in software... 
Where to change order? Where to disable A7s? Probably in uBoot? 

Regards, Ivan Pavic.
  
 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] SMMU, Unhandled context fault

[Julien Grall]

Hello,

On 28/04/16 13:56, Peng Fan wrote:

On Thu, Apr 28, 2016 at 11:27:22AM +0100, Julien Grall wrote:



On 28/04/16 07:39, Peng Fan wrote:

Hi Julien,


Hello Peng,


On Thu, Apr 28, 2016 at 10:37:54AM +0800, Peng Fan wrote:

Hi Julien,
On Wed, Apr 27, 2016 at 10:58:28AM +0100, Julien Grall wrote:

Hello Peng,

On 27/04/2016 03:02, Peng Fan wrote:

On Tue, Apr 26, 2016 at 04:30:03PM +0200, Edgar E. Iglesias wrote:

On Tue, Apr 26, 2016 at 09:56:33PM +0800, Peng Fan wrote:

You mean the PNU bit(Privileged Not Unprivileged) is 1?
I did not met Unhandled context fault each time.
Actually during my serveral boot test, I only met two times.




I meant the NSSTATE and NSATTR bits in FSYNR are set to zero. I get the
impression that the TrustZone state for the SD controller may be


oh. The NSATTR bit is 0. I did not find NSSTATE in my Issue D SMMU spec.
If without xen, only one linux boots up, sd controller can access memory using
DMA without issue.


IIRC, by default Linux baremetal does not protect the devices with the SMMU.

I would recommend you to check whether the SMMUs are in-used and configured
to generate a fault (disable_bypass = 1).


Ok. I'll set S2CRn to generate fault in xen smmu driver to see whether SMMUs 
in-used or not


I meant in Linux.


My bad. Do you mean enabling SMMU driver in Linux with KVM support?


Yes.

[...]


Is there any big difference between XEN SMMU driver and linux SMMU driver?
I know that XEN only support Stage 2. But the initliaization flow is almost the 
same.


The SMMU driver for Xen is a port from Linux 3.19-rc0. Since then the 
Linux driver has been reworked and it might be possible that we have 
missed some bug fix.


Aside that, for Xen, the page tables are always shared between the SMMU 
and the processor.


Regards,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 8:39 PM, Jan Beulich wrote:

On 28.04.16 at 14:12,  wrote:

I'm still confused why do we need this, especially at such critical
moment. IIUC HVMMEM type is used to get/set mem type, why would someone
define a HVMMEM type but not use it here?


Who knows. And as said - the patch can go in as is, I just inquired
because I like to avoid future code churn whenever possible, i.e.
when a certain way of coding makes it less likely for the code
needing touching again compared to some other variant, I'd
generally like that to be used (as long as it's not meaningfully worse
in other respects).



Thanks Jan.
So my understanding is that this patch does not need any change any
more.

As to your concern, I still do not have any better thought.
And this hole is a problem because of the old mistake I have made in
previous version. Could we be careful in the future review to avoid
another hole(besides the HVMMEM_unused one which is unavoidable with
HVMMEM_ioreq_server), and if this can not be avoided, we try to find a
more graceful solution by then?  :)


Jan




Yu

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] SMMU, Unhandled context fault

[Peng Fan]

On Thu, Apr 28, 2016 at 11:27:22AM +0100, Julien Grall wrote:
>
>
>On 28/04/16 07:39, Peng Fan wrote:
>>Hi Julien,
>
>Hello Peng,
>
>>On Thu, Apr 28, 2016 at 10:37:54AM +0800, Peng Fan wrote:
>>>Hi Julien,
>>>On Wed, Apr 27, 2016 at 10:58:28AM +0100, Julien Grall wrote:
Hello Peng,

On 27/04/2016 03:02, Peng Fan wrote:
>On Tue, Apr 26, 2016 at 04:30:03PM +0200, Edgar E. Iglesias wrote:
>>On Tue, Apr 26, 2016 at 09:56:33PM +0800, Peng Fan wrote:
>>>You mean the PNU bit(Privileged Not Unprivileged) is 1?
>>>I did not met Unhandled context fault each time.
>>>Actually during my serveral boot test, I only met two times.
>>
>>
>>
>>I meant the NSSTATE and NSATTR bits in FSYNR are set to zero. I get the
>>impression that the TrustZone state for the SD controller may be
>
>oh. The NSATTR bit is 0. I did not find NSSTATE in my Issue D SMMU spec.
>If without xen, only one linux boots up, sd controller can access memory 
>using
>DMA without issue.

IIRC, by default Linux baremetal does not protect the devices with the SMMU.

I would recommend you to check whether the SMMUs are in-used and configured
to generate a fault (disable_bypass = 1).
>>>
>>>Ok. I'll set S2CRn to generate fault in xen smmu driver to see whether SMMUs 
>>>in-used or not
>
>I meant in Linux.

My bad. Do you mean enabling SMMU driver in Linux with KVM support?

>
>>>
>>>I found a patch for errata of mmu-500,
>>>https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/commit/?h=iommu/devel=7f0cc5124f5ec66b5b26878ac85137adc6537413
>>>Do you know this?
>>>
>>>I suspect the unstable issue on my platform seems related to this errata,
>>>but i do not have details about this errata.
>
>Have you tried to port the patch to Xen and see if it helps?

I tried. But the value of the register does not change. I am checking with our
IC team.

>
>>
>>I do the following change:
>>  //s2cr = S2CR_TYPE_TRANS | S2CR_PRIVCFG_UNPRIV |
>>   s2cr = S2CR_TYPE_FAULT | S2CR_PRIVCFG_UNPRIV |
>>  (smmu_domain->cfg.cbndx << S2CR_CBNDX_SHIFT);
>>
>>Now XEN SMMU driver reports that:
>>(XEN) smmu: /iommu@5c80: Unexpected global fault, this could be serious
>>(XEN) smmu: /iommu@5c80:GFSR 0x0001, GFSYNR0 0x0004, GFSYNR1 
>>0x0011, GFSYNR2 0x
>>
>>So I think SMMU is working.
>
>Well, it does not help to know whether the SMMU has been correctly
>initialized by Xen.

Is there any big difference between XEN SMMU driver and linux SMMU driver?
I know that XEN only support Stage 2. But the initliaization flow is almost the 
same.

Thanks,
Peng.

>
>Regards,
>
>-- 
>Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen/time: fix gtime_to_gtsc for vtsc=1 PV guests

[Andrew Cooper]

On 25/04/16 12:18, Stefano Stabellini wrote:
> From: Jan Beulich 
>
> For vtsc=1 PV guests, rdtsc is trapped and calculated from get_s_time()
> using gtime_to_gtsc. Similarly the tsc_timestamp, part of struct
> vcpu_time_info, is calculated from stime_local_stamp using
> gtime_to_gtsc.
>
> However gtime_to_gtsc can return 0, if time < vtsc_offset, which can
> actually happen when gtime_to_gtsc is called passing stime_local_stamp
> (the caller function is __update_vcpu_system_time).
>
> In that case the pvclock protocol doesn't work properly and the guest is
> unable to calculate the system time correctly. As a consequence when the
> guest tries to set a timer event (for example calling the
> VCPUOP_set_singleshot_timer hypercall), the event will be in the past
> causing Linux to hang.
>
> The purpose of the pvclock protocol is to allow the guest to calculate
> the system_time in nanosec correctly. The guest calculates as follow:
>
>   from_vtsc_scale(rdtsc - vcpu_time_info.tsc_timestamp) + 
> vcpu_time_info.system_time
>
> Given that with vtsc=1:
>   rdtsc = to_vtsc_scale(NOW() - vtsc_offset)
>   vcpu_time_info.tsc_timestamp = to_vtsc_scale(vcpu_time_info.system_time - 
> vtsc_offset)
>
> The expression evaluates to NOW(), which is what we want.  However when
> stime_local_stamp < vtsc_offset, vcpu_time_info.tsc_timestamp is
> actually 0. As a consequence the calculated overall system_time is not
> correct.
>
> This patch fixes the issue by letting gtime_to_gtsc return a negative
> integer in the form of a wrapped around unsigned integer, thus when the
> guest subtracts vcpu_time_info.tsc_timestamp from rdtsc will calculate
> the right value.
>
> Signed-off-by: Jan Beulich 
> Signed-off-by: Stefano Stabellini 

Reviewed-by: Andrew Cooper 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 13:31
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel
> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> 
> >>> On 28.04.16 at 13:58,  wrote:
> >>  -Original Message-
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 28 April 2016 12:44
> >> To: Paul Durrant
> >> Cc: Andrew Cooper; xen-devel
> >> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> >>
> >> >>> On 28.04.16 at 13:17,  wrote:
> >> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> >> Sent: 28 April 2016 10:50
> >> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
> >> >>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> >> >>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> >> >>   !(data & PCI_MSIX_VECTOR_BITMASK) )
> >> >> +{
> >> >>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> >> >> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> >> >> +}
> >> >> +}
> >> >> +else if ( (size == 4 || size == 8) && !r->df &&
> >> >> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> >> >> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE 
> >> >> - 1)) )
> >> >
> >> > That's quite an if statement. Any chance of making it more
> decipherable? I
> >>
> >> I don't see how I could be doing this.
> >>
> >> > also think it's worth putting the restrictions you outline in the commit 
> >> > in
> > a
> >> > comment here so that it's clear that the code is not trying to handle all
> >> > corner cases.
> >>
> >> Sure. Question is whether by mixing code and comments things
> >> would get better readable (to at least somewhat address your
> >> request above), or whether that instead would make things
> >> worse. Thoughts?
> >
> > I think mentioning why you're only tackling the !r->df case would be worth
> > commenting on and if the && !r->df were on a separate line then the
> comment could
> > go inline.
> 
> That's what I did.
> 
> > Also, do you really need to check r->count (seems like a count of 0
> > should have been picked up before the code got here)
> 
> I've tried to fine where r->count == 0 would be dealt with, but
> could spot the location (other than relying on x86_emulate.c
> never passing such down), so since I want to subtract 1 from it
> (or really 4 from its product with "size") I wanted to be on the
> safe side. If you prefer, I could replace this by a respective
> ASSERT()...
> 
> > and then TBH I'm not
> > even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1))
> is even
> > checking so how about an illustratively named macro?
> 
> How about this (without macro)?
> 
> else if ( (size == 4 || size == 8) &&
>   /* Only support forward REP MOVS for now. */
>   !r->df &&
>   /*
>* Only fully support accesses to a single table entry for
>* now (if multiple ones get written to in one go, only the
>* final one gets dealt with).
>*/
>   r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>   !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
> {
> 

That looks a lot better to me :-)

  Paul

> Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 06:34:48AM -0600, Jan Beulich wrote:
> >>> On 28.04.16 at 14:06,  wrote:
> > On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
> >> On 28/04/16 12:59, Wei Liu wrote:
> >> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> >> >> Thanks Jan. And I admire your rigorous thought. :)
> >> >>
> >> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> >> >> On 28.04.16 at 12:42,  wrote:
> >>  On 28/04/16 11:22, Jan Beulich wrote:
> >>  On 28.04.16 at 10:29,  wrote:
> >> >> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> >>  XEN_GUEST_HANDLE_PARAM(void) arg)
> >> >> [HVMMEM_ram_rw]  = p2m_ram_rw,
> >> >> [HVMMEM_ram_ro]  = p2m_ram_ro,
> >> >> [HVMMEM_mmio_dm] = p2m_mmio_dm,
> >> >> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> >> >> +[HVMMEM_unused] = p2m_invalid
> >> > Why don't you simply delete the old line, without replacement?
> >> >> Well, I did not delete the old line, because in my coming patch(the
> >> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> >> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> >> >> against HVMMEN_unused later in this routine appear in that patch.
> >> >>
> >>  That might have been slightly cleaner; but we're going to have to put 
> >>  it
> >>  back as soon as the development window opens anyway, so I don't really
> >>  see the point of going through the effort of respinning the patch 
> >>  again.
> >> 
> >>  Would you be willing to ack this version anyway?
> >> >>> I have no problem doing so (and in fact I have it on my to by
> >> >>> committed list already), it is just looked slightly confusing (and
> >> >>> I had already typed half a reply that this isn't what was discussed
> >> >>> until I properly looked at the next hunk), and hence I wanted to
> >> >>> understand the motivation. And btw., I'm not convinced it would
> >> >>> need to be put there anyway later: I don't view the used
> >> >>> mechanism as a good (read: extensible) one to deal with what
> >> >>> would be holes in the array above. Indeed we can't leave them
> >> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
> >> >>> should better find a way to initialize _all_ unused slots without
> >> >>> requiring an initializer for each of them. Sadly the desire to allow
> >> >>> compilation with clang prohibits the most natural solution:
> >> >>>
> >> >>>static const p2m_type_t memtype[] = {
> >> >>>[0 ...  - 1] = p2m_invalid,
> >> >> Not sure if this will compile? Can have a try. :)
> >> >>
> >> > To answer your question this can compile with gcc but not probably not
> >> > with clang. This syntax is gcc extension.
> >> >
> >> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html 
> >> 
> >> That syntax works in Clang, but will subsequent entries in the list will
> >> suffer a -Werror,-Winitializer-overrides and fail to compile.
> >> 
> > 
> > This can easily be fixed :-)
> > 
> >  [ 0 ...  ] = p2m_inavlid;
> >  [  ...   ] = p2m_invalid;
> > 
> > But I'm not sure whether you guys think this is pretty or ugly.
> 
> What if multiple holes show up in the future? The goal really is to
> deal with all holes in one line, once and for all.
> 

It's up to you to decide what to do. I don't have further suggestions
really.

Wei.

> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:12,  wrote:
> I'm still confused why do we need this, especially at such critical
> moment. IIUC HVMMEM type is used to get/set mem type, why would someone
> define a HVMMEM type but not use it here?

Who knows. And as said - the patch can go in as is, I just inquired
because I like to avoid future code churn whenever possible, i.e.
when a certain way of coding makes it less likely for the code
needing touching again compared to some other variant, I'd
generally like that to be used (as long as it's not meaningfully worse
in other respects).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:06,  wrote:
> On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
>> On 28/04/16 12:59, Wei Liu wrote:
>> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
>> >> Thanks Jan. And I admire your rigorous thought. :)
>> >>
>> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>> >> On 28.04.16 at 12:42,  wrote:
>>  On 28/04/16 11:22, Jan Beulich wrote:
>>  On 28.04.16 at 10:29,  wrote:
>> >> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>>  XEN_GUEST_HANDLE_PARAM(void) arg)
>> >> [HVMMEM_ram_rw]  = p2m_ram_rw,
>> >> [HVMMEM_ram_ro]  = p2m_ram_ro,
>> >> [HVMMEM_mmio_dm] = p2m_mmio_dm,
>> >> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>> >> +[HVMMEM_unused] = p2m_invalid
>> > Why don't you simply delete the old line, without replacement?
>> >> Well, I did not delete the old line, because in my coming patch(the
>> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
>> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
>> >> against HVMMEN_unused later in this routine appear in that patch.
>> >>
>>  That might have been slightly cleaner; but we're going to have to put it
>>  back as soon as the development window opens anyway, so I don't really
>>  see the point of going through the effort of respinning the patch again.
>> 
>>  Would you be willing to ack this version anyway?
>> >>> I have no problem doing so (and in fact I have it on my to by
>> >>> committed list already), it is just looked slightly confusing (and
>> >>> I had already typed half a reply that this isn't what was discussed
>> >>> until I properly looked at the next hunk), and hence I wanted to
>> >>> understand the motivation. And btw., I'm not convinced it would
>> >>> need to be put there anyway later: I don't view the used
>> >>> mechanism as a good (read: extensible) one to deal with what
>> >>> would be holes in the array above. Indeed we can't leave them
>> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
>> >>> should better find a way to initialize _all_ unused slots without
>> >>> requiring an initializer for each of them. Sadly the desire to allow
>> >>> compilation with clang prohibits the most natural solution:
>> >>>
>> >>>static const p2m_type_t memtype[] = {
>> >>>[0 ...  - 1] = p2m_invalid,
>> >> Not sure if this will compile? Can have a try. :)
>> >>
>> > To answer your question this can compile with gcc but not probably not
>> > with clang. This syntax is gcc extension.
>> >
>> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html 
>> 
>> That syntax works in Clang, but will subsequent entries in the list will
>> suffer a -Werror,-Winitializer-overrides and fail to compile.
>> 
> 
> This can easily be fixed :-)
> 
>  [ 0 ...  ] = p2m_inavlid;
>  [  ...   ] = p2m_invalid;
> 
> But I'm not sure whether you guys think this is pretty or ugly.

What if multiple holes show up in the future? The goal really is to
deal with all holes in one line, once and for all.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

[Jan Beulich]

>>> On 28.04.16 at 13:58,  wrote:
>>  -Original Message-
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 28 April 2016 12:44
>> To: Paul Durrant
>> Cc: Andrew Cooper; xen-devel
>> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
>> 
>> >>> On 28.04.16 at 13:17,  wrote:
>> >> From: Jan Beulich [mailto:jbeul...@suse.com]
>> >> Sent: 28 April 2016 10:50
>> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>> >>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>> >>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>> >>   !(data & PCI_MSIX_VECTOR_BITMASK) )
>> >> +{
>> >>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
>> >> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
>> >> +}
>> >> +}
>> >> +else if ( (size == 4 || size == 8) && !r->df &&
>> >> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>> >> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 
>> >> 1)) )
>> >
>> > That's quite an if statement. Any chance of making it more decipherable? I
>> 
>> I don't see how I could be doing this.
>> 
>> > also think it's worth putting the restrictions you outline in the commit 
>> > in 
> a
>> > comment here so that it's clear that the code is not trying to handle all
>> > corner cases.
>> 
>> Sure. Question is whether by mixing code and comments things
>> would get better readable (to at least somewhat address your
>> request above), or whether that instead would make things
>> worse. Thoughts?
> 
> I think mentioning why you're only tackling the !r->df case would be worth 
> commenting on and if the && !r->df were on a separate line then the comment 
> could 
> go inline.

That's what I did.

> Also, do you really need to check r->count (seems like a count of 0 
> should have been picked up before the code got here)

I've tried to fine where r->count == 0 would be dealt with, but
could spot the location (other than relying on x86_emulate.c
never passing such down), so since I want to subtract 1 from it
(or really 4 from its product with "size") I wanted to be on the
safe side. If you prefer, I could replace this by a respective
ASSERT()...

> and then TBH I'm not 
> even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) is 
> even 
> checking so how about an illustratively named macro?

How about this (without macro)?

else if ( (size == 4 || size == 8) &&
  /* Only support forward REP MOVS for now. */
  !r->df &&
  /*
   * Only fully support accesses to a single table entry for
   * now (if multiple ones get written to in one go, only the
   * final one gets dealt with).
   */
  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )
{

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 14:00,  wrote:

> 
> On 4/28/2016 7:52 PM, Jan Beulich wrote:
> On 28.04.16 at 13:40,  wrote:
>>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>>> On 28.04.16 at 12:42,  wrote:
> That might have been slightly cleaner; but we're going to have to put it
> back as soon as the development window opens anyway, so I don't really
> see the point of going through the effort of respinning the patch again.
>
> Would you be willing to ack this version anyway?

 I have no problem doing so (and in fact I have it on my to by
 committed list already), it is just looked slightly confusing (and
 I had already typed half a reply that this isn't what was discussed
 until I properly looked at the next hunk), and hence I wanted to
 understand the motivation. And btw., I'm not convinced it would
 need to be put there anyway later: I don't view the used
 mechanism as a good (read: extensible) one to deal with what
 would be holes in the array above. Indeed we can't leave them
 uninitialized (as that would mean p2m_ram_rw), but I think we
 should better find a way to initialize _all_ unused slots without
 requiring an initializer for each of them. Sadly the desire to allow
 compilation with clang prohibits the most natural solution:

 static const p2m_type_t memtype[] = {
 [0 ...  - 1] = p2m_invalid,
>>>
>>> Not sure if this will compile? Can have a try. :)
>>
>> gcc will like it, but as said clang won't (afair).
>>
 [HVMMEM_ram_rw]  = p2m_ram_rw,
 [HVMMEM_ram_ro]  = p2m_ram_ro,
 [HVMMEM_mmio_dm] = p2m_mmio_dm,
 };

 Maybe we could do (altering the second hunk of this patch)

 @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
   ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
  goto setmemtype_fail;

 -if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
 +BUILD_BUG_ON(p2m_ram_rw);
 +BUILD_BUG_ON(HVMMEM_ram_rw);
 +if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
 + (a.hvmmem_type && !memtype[a.hvmmem_type]) )
>>>
>>> I guess by !memtype[a.hvmmem_type] you are trying to check if it's
>>> p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
>>> should be checked like memtype[a.hvmmem_type] < 0 and initialize the
>>> holes with -1.
>>
>> No. As said, I want to avoid explicit initializers for unused slots,
>> and hence it has to be zero that gets checked against.
>>
> 
> But "!memtype[a.hvmmem_type]" is indicating the p2m type is p2m_ram_rw,
> which should be allowed here...

Hence the additional check for a.hvmmem_type to not be zero
(that's the only thing mapping to p2m_ram_rw).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 8:06 PM, Wei Liu wrote:

On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:

On 28/04/16 12:59, Wei Liu wrote:

On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:

Thanks Jan. And I admire your rigorous thought. :)

On 4/28/2016 6:57 PM, Jan Beulich wrote:

On 28.04.16 at 12:42,  wrote:

On 28/04/16 11:22, Jan Beulich wrote:

On 28.04.16 at 10:29,  wrote:

@@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,

XEN_GUEST_HANDLE_PARAM(void) arg)

[HVMMEM_ram_rw]  = p2m_ram_rw,
[HVMMEM_ram_ro]  = p2m_ram_ro,
[HVMMEM_mmio_dm] = p2m_mmio_dm,
-[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+[HVMMEM_unused] = p2m_invalid

Why don't you simply delete the old line, without replacement?

Well, I did not delete the old line, because in my coming patch(the
p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
against HVMMEN_unused later in this routine appear in that patch.


That might have been slightly cleaner; but we're going to have to put it
back as soon as the development window opens anyway, so I don't really
see the point of going through the effort of respinning the patch again.

Would you be willing to ack this version anyway?

I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:

   static const p2m_type_t memtype[] = {
   [0 ...  - 1] = p2m_invalid,

Not sure if this will compile? Can have a try. :)


To answer your question this can compile with gcc but not probably not
with clang. This syntax is gcc extension.

See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html


That syntax works in Clang, but will subsequent entries in the list will
suffer a -Werror,-Winitializer-overrides and fail to compile.



This can easily be fixed :-)

 [ 0 ...  ] = p2m_inavlid;
 [  ...   ] = p2m_invalid;

But I'm not sure whether you guys think this is pretty or ugly.



Thanks for your information, Wei. :)
But  and  ...  seems to be holes in this array.

I'm still confused why do we need this, especially at such critical
moment. IIUC HVMMEM type is used to get/set mem type, why would someone
define a HVMMEM type but not use it here?

I know HVMMEM_mmio_write_dm is unused now, but I do not think this
should be a common case in the future.

Frankly, I had thought to remove the HVMMEM_unused in the set_mem_type
code, I choose not to do so, because I do not wanna  the check of
a.hvmmem_type against HVMMEN_unused to pop in my next patch, and I do
not think keeping this will harm any functionality. :)



Wei.



Yu

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 01:00:57PM +0100, Andrew Cooper wrote:
> On 28/04/16 12:59, Wei Liu wrote:
> > On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> >> Thanks Jan. And I admire your rigorous thought. :)
> >>
> >> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> >> On 28.04.16 at 12:42,  wrote:
>  On 28/04/16 11:22, Jan Beulich wrote:
>  On 28.04.16 at 10:29,  wrote:
> >> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
>  XEN_GUEST_HANDLE_PARAM(void) arg)
> >> [HVMMEM_ram_rw]  = p2m_ram_rw,
> >> [HVMMEM_ram_ro]  = p2m_ram_ro,
> >> [HVMMEM_mmio_dm] = p2m_mmio_dm,
> >> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> >> +[HVMMEM_unused] = p2m_invalid
> > Why don't you simply delete the old line, without replacement?
> >> Well, I did not delete the old line, because in my coming patch(the
> >> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> >> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> >> against HVMMEN_unused later in this routine appear in that patch.
> >>
>  That might have been slightly cleaner; but we're going to have to put it
>  back as soon as the development window opens anyway, so I don't really
>  see the point of going through the effort of respinning the patch again.
> 
>  Would you be willing to ack this version anyway?
> >>> I have no problem doing so (and in fact I have it on my to by
> >>> committed list already), it is just looked slightly confusing (and
> >>> I had already typed half a reply that this isn't what was discussed
> >>> until I properly looked at the next hunk), and hence I wanted to
> >>> understand the motivation. And btw., I'm not convinced it would
> >>> need to be put there anyway later: I don't view the used
> >>> mechanism as a good (read: extensible) one to deal with what
> >>> would be holes in the array above. Indeed we can't leave them
> >>> uninitialized (as that would mean p2m_ram_rw), but I think we
> >>> should better find a way to initialize _all_ unused slots without
> >>> requiring an initializer for each of them. Sadly the desire to allow
> >>> compilation with clang prohibits the most natural solution:
> >>>
> >>>static const p2m_type_t memtype[] = {
> >>>[0 ...  - 1] = p2m_invalid,
> >> Not sure if this will compile? Can have a try. :)
> >>
> > To answer your question this can compile with gcc but not probably not
> > with clang. This syntax is gcc extension.
> >
> > See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
> 
> That syntax works in Clang, but will subsequent entries in the list will
> suffer a -Werror,-Winitializer-overrides and fail to compile.
> 

This can easily be fixed :-)

 [ 0 ...  ] = p2m_inavlid;
 [  ...   ] = p2m_invalid;

But I'm not sure whether you guys think this is pretty or ugly.


Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Andrew Cooper]

On 28/04/16 12:59, Wei Liu wrote:
> On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
>> Thanks Jan. And I admire your rigorous thought. :)
>>
>> On 4/28/2016 6:57 PM, Jan Beulich wrote:
>> On 28.04.16 at 12:42,  wrote:
 On 28/04/16 11:22, Jan Beulich wrote:
 On 28.04.16 at 10:29,  wrote:
>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
 XEN_GUEST_HANDLE_PARAM(void) arg)
>> [HVMMEM_ram_rw]  = p2m_ram_rw,
>> [HVMMEM_ram_ro]  = p2m_ram_ro,
>> [HVMMEM_mmio_dm] = p2m_mmio_dm,
>> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>> +[HVMMEM_unused] = p2m_invalid
> Why don't you simply delete the old line, without replacement?
>> Well, I did not delete the old line, because in my coming patch(the
>> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
>> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
>> against HVMMEN_unused later in this routine appear in that patch.
>>
 That might have been slightly cleaner; but we're going to have to put it
 back as soon as the development window opens anyway, so I don't really
 see the point of going through the effort of respinning the patch again.

 Would you be willing to ack this version anyway?
>>> I have no problem doing so (and in fact I have it on my to by
>>> committed list already), it is just looked slightly confusing (and
>>> I had already typed half a reply that this isn't what was discussed
>>> until I properly looked at the next hunk), and hence I wanted to
>>> understand the motivation. And btw., I'm not convinced it would
>>> need to be put there anyway later: I don't view the used
>>> mechanism as a good (read: extensible) one to deal with what
>>> would be holes in the array above. Indeed we can't leave them
>>> uninitialized (as that would mean p2m_ram_rw), but I think we
>>> should better find a way to initialize _all_ unused slots without
>>> requiring an initializer for each of them. Sadly the desire to allow
>>> compilation with clang prohibits the most natural solution:
>>>
>>>static const p2m_type_t memtype[] = {
>>>[0 ...  - 1] = p2m_invalid,
>> Not sure if this will compile? Can have a try. :)
>>
> To answer your question this can compile with gcc but not probably not
> with clang. This syntax is gcc extension.
>
> See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

That syntax works in Clang, but will subsequent entries in the list will
suffer a -Werror,-Winitializer-overrides and fail to compile.

I already had to fix two examples of this to get clang support working
in the past.

(It is a real shame that p2m_invalid doesn't have the value 0)

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

On 4/28/2016 7:52 PM, Jan Beulich wrote:

On 28.04.16 at 13:40,  wrote:

On 4/28/2016 6:57 PM, Jan Beulich wrote:

On 28.04.16 at 12:42,  wrote:

That might have been slightly cleaner; but we're going to have to put it
back as soon as the development window opens anyway, so I don't really
see the point of going through the effort of respinning the patch again.

Would you be willing to ack this version anyway?


I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:

static const p2m_type_t memtype[] = {
[0 ...  - 1] = p2m_invalid,


Not sure if this will compile? Can have a try. :)


gcc will like it, but as said clang won't (afair).


[HVMMEM_ram_rw]  = p2m_ram_rw,
[HVMMEM_ram_ro]  = p2m_ram_ro,
[HVMMEM_mmio_dm] = p2m_mmio_dm,
};

Maybe we could do (altering the second hunk of this patch)

@@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op,

XEN_GUEST_HANDLE_PARAM(void) arg)

  ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
 goto setmemtype_fail;

-if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+BUILD_BUG_ON(p2m_ram_rw);
+BUILD_BUG_ON(HVMMEM_ram_rw);
+if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+ (a.hvmmem_type && !memtype[a.hvmmem_type]) )


I guess by !memtype[a.hvmmem_type] you are trying to check if it's
p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
should be checked like memtype[a.hvmmem_type] < 0 and initialize the
holes with -1.


No. As said, I want to avoid explicit initializers for unused slots,
and hence it has to be zero that gets checked against.



But "!memtype[a.hvmmem_type]" is indicating the p2m type is p2m_ram_rw,
which should be allowed here...


But I still wonder is this really necessary? Because we only have one
hole in this array in the forseeable future.


How do you know?



I had thought I'm the only one proposing to add another HVMMEM type.
Maybe I shall not have this speculation.


Jan




Yu

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 12:44
> To: Paul Durrant
> Cc: Andrew Cooper; xen-devel
> Subject: RE: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> 
> >>> On 28.04.16 at 13:17,  wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 28 April 2016 10:50
> >> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
> >>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> >>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> >>   !(data & PCI_MSIX_VECTOR_BITMASK) )
> >> +{
> >>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> >> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> >> +}
> >> +}
> >> +else if ( (size == 4 || size == 8) && !r->df &&
> >> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> >> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 
> >> 1)) )
> >
> > That's quite an if statement. Any chance of making it more decipherable? I
> 
> I don't see how I could be doing this.
> 
> > also think it's worth putting the restrictions you outline in the commit in 
> > a
> > comment here so that it's clear that the code is not trying to handle all
> > corner cases.
> 
> Sure. Question is whether by mixing code and comments things
> would get better readable (to at least somewhat address your
> request above), or whether that instead would make things
> worse. Thoughts?

I think mentioning why you're only tackling the !r->df case would be worth 
commenting on and if the && !r->df were on a separate line then the comment 
could go inline. Also, do you really need to check r->count (seems like a count 
of 0 should have been picked up before the code got here) and then TBH I'm not 
even sure what !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) is 
even checking so how about an illustratively named macro?

  Paul

> 
> >> +{
> >> +BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> >> + (PCI_MSIX_ENTRY_SIZE - 1));
> >> +
> >> +v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> >> +addr + size * r->count - 4;
> >> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> >> +r->data + size * r->count - 4;
> >
> > Does there need to be any explicit type promotion here since r->data is
> > uint64_t?
> 
> No, because both size and r->count did already get bounds
> checked to very narrow ranges.
> 
> Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Wei Liu]

On Thu, Apr 28, 2016 at 07:40:45PM +0800, Yu, Zhang wrote:
> Thanks Jan. And I admire your rigorous thought. :)
> 
> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> On 28.04.16 at 12:42,  wrote:
> >>On 28/04/16 11:22, Jan Beulich wrote:
> >>On 28.04.16 at 10:29,  wrote:
> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,
> >>XEN_GUEST_HANDLE_PARAM(void) arg)
>  [HVMMEM_ram_rw]  = p2m_ram_rw,
>  [HVMMEM_ram_ro]  = p2m_ram_ro,
>  [HVMMEM_mmio_dm] = p2m_mmio_dm,
> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
> +[HVMMEM_unused] = p2m_invalid
> >>>
> >>>Why don't you simply delete the old line, without replacement?
> >>
> 
> Well, I did not delete the old line, because in my coming patch(the
> p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
> which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
> against HVMMEN_unused later in this routine appear in that patch.
> 
> >>That might have been slightly cleaner; but we're going to have to put it
> >>back as soon as the development window opens anyway, so I don't really
> >>see the point of going through the effort of respinning the patch again.
> >>
> >>Would you be willing to ack this version anyway?
> >
> >I have no problem doing so (and in fact I have it on my to by
> >committed list already), it is just looked slightly confusing (and
> >I had already typed half a reply that this isn't what was discussed
> >until I properly looked at the next hunk), and hence I wanted to
> >understand the motivation. And btw., I'm not convinced it would
> >need to be put there anyway later: I don't view the used
> >mechanism as a good (read: extensible) one to deal with what
> >would be holes in the array above. Indeed we can't leave them
> >uninitialized (as that would mean p2m_ram_rw), but I think we
> >should better find a way to initialize _all_ unused slots without
> >requiring an initializer for each of them. Sadly the desire to allow
> >compilation with clang prohibits the most natural solution:
> >
> >static const p2m_type_t memtype[] = {
> >[0 ...  - 1] = p2m_invalid,
> 
> Not sure if this will compile? Can have a try. :)
> 

To answer your question this can compile with gcc but not probably not
with clang. This syntax is gcc extension.

See: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 13:40,  wrote:
> On 4/28/2016 6:57 PM, Jan Beulich wrote:
> On 28.04.16 at 12:42,  wrote:
>>> That might have been slightly cleaner; but we're going to have to put it
>>> back as soon as the development window opens anyway, so I don't really
>>> see the point of going through the effort of respinning the patch again.
>>>
>>> Would you be willing to ack this version anyway?
>>
>> I have no problem doing so (and in fact I have it on my to by
>> committed list already), it is just looked slightly confusing (and
>> I had already typed half a reply that this isn't what was discussed
>> until I properly looked at the next hunk), and hence I wanted to
>> understand the motivation. And btw., I'm not convinced it would
>> need to be put there anyway later: I don't view the used
>> mechanism as a good (read: extensible) one to deal with what
>> would be holes in the array above. Indeed we can't leave them
>> uninitialized (as that would mean p2m_ram_rw), but I think we
>> should better find a way to initialize _all_ unused slots without
>> requiring an initializer for each of them. Sadly the desire to allow
>> compilation with clang prohibits the most natural solution:
>>
>> static const p2m_type_t memtype[] = {
>> [0 ...  - 1] = p2m_invalid,
> 
> Not sure if this will compile? Can have a try. :)

gcc will like it, but as said clang won't (afair).

>> [HVMMEM_ram_rw]  = p2m_ram_rw,
>> [HVMMEM_ram_ro]  = p2m_ram_ro,
>> [HVMMEM_mmio_dm] = p2m_mmio_dm,
>> };
>>
>> Maybe we could do (altering the second hunk of this patch)
>>
>> @@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>   ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
>>  goto setmemtype_fail;
>>
>> -if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
>> +BUILD_BUG_ON(p2m_ram_rw);
>> +BUILD_BUG_ON(HVMMEM_ram_rw);
>> +if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
>> + (a.hvmmem_type && !memtype[a.hvmmem_type]) )
> 
> I guess by !memtype[a.hvmmem_type] you are trying to check if it's
> p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
> should be checked like memtype[a.hvmmem_type] < 0 and initialize the
> holes with -1.

No. As said, I want to avoid explicit initializers for unused slots,
and hence it has to be zero that gets checked against.

> But I still wonder is this really necessary? Because we only have one
> hole in this array in the forseeable future.

How do you know?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Yu, Zhang]

Thanks Jan. And I admire your rigorous thought. :)

On 4/28/2016 6:57 PM, Jan Beulich wrote:

On 28.04.16 at 12:42,  wrote:

On 28/04/16 11:22, Jan Beulich wrote:

On 28.04.16 at 10:29,  wrote:

@@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op,

XEN_GUEST_HANDLE_PARAM(void) arg)

 [HVMMEM_ram_rw]  = p2m_ram_rw,
 [HVMMEM_ram_ro]  = p2m_ram_ro,
 [HVMMEM_mmio_dm] = p2m_mmio_dm,
-[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
+[HVMMEM_unused] = p2m_invalid


Why don't you simply delete the old line, without replacement?




Well, I did not delete the old line, because in my coming patch(the
p2m renaming code), I'm planning to introduce the HVMMEM_ioreq_server,
which is HVMMEM_unused+1. And I do not want the check of a.hvmmem_type
against HVMMEN_unused later in this routine appear in that patch.


That might have been slightly cleaner; but we're going to have to put it
back as soon as the development window opens anyway, so I don't really
see the point of going through the effort of respinning the patch again.

Would you be willing to ack this version anyway?


I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:

static const p2m_type_t memtype[] = {
[0 ...  - 1] = p2m_invalid,


Not sure if this will compile? Can have a try. :)


[HVMMEM_ram_rw]  = p2m_ram_rw,
[HVMMEM_ram_ro]  = p2m_ram_ro,
[HVMMEM_mmio_dm] = p2m_mmio_dm,
};

Maybe we could do (altering the second hunk of this patch)

@@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
  ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
 goto setmemtype_fail;

-if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+BUILD_BUG_ON(p2m_ram_rw);
+BUILD_BUG_ON(HVMMEM_ram_rw);
+if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+ (a.hvmmem_type && !memtype[a.hvmmem_type]) )


I guess by !memtype[a.hvmmem_type] you are trying to check if it's
p2m_invalid? But p2m_ram_rw is 0, and p2m_invalid is 1. So may be it
should be checked like memtype[a.hvmmem_type] < 0 and initialize the
holes with -1.

But I still wonder is this really necessary? Because we only have one
hole in this array in the forseeable future.


 goto setmemtype_fail;

 while ( a.nr > start_iter )

Jan




B.R.
Yu

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10] xSplice v1 design and implementation.

[Wei Liu]

Jan informed me on IRC that this series is ready to go in (with minor
adjustments). I also went through this series briefly and was satisfied
with it, so:

Release-acked-by: Wei Liu 

Feel free to stick that to later versions as well.

Thanks everyone!

Wei.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

[Jan Beulich]

>>> On 28.04.16 at 13:17,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 28 April 2016 10:50
>> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>>   !(data & PCI_MSIX_VECTOR_BITMASK) )
>> +{
>>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
>> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
>> +}
>> +}
>> +else if ( (size == 4 || size == 8) && !r->df &&
>> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
>> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) 
>> )
> 
> That's quite an if statement. Any chance of making it more decipherable? I 

I don't see how I could be doing this.

> also think it's worth putting the restrictions you outline in the commit in a 
> comment here so that it's clear that the code is not trying to handle all 
> corner cases.

Sure. Question is whether by mixing code and comments things
would get better readable (to at least somewhat address your
request above), or whether that instead would make things
worse. Thoughts?

>> +{
>> +BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
>> + (PCI_MSIX_ENTRY_SIZE - 1));
>> +
>> +v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
>> +addr + size * r->count - 4;
>> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
>> +r->data + size * r->count - 4;
> 
> Does there need to be any explicit type promotion here since r->data is 
> uint64_t?

No, because both size and r->count did already get bounds
checked to very narrow ranges.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 22/24] xsplice/xen_replace_world: Test-case for XSPLICE_ACTION_REPLACE

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> --- a/xen/arch/x86/test/Makefile
> +++ b/xen/arch/x86/test/Makefile
> @@ -7,15 +7,18 @@ CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ 
> print "0x"$$2}')
>  
>  XSPLICE := xen_hello_world.xsplice
>  XSPLICE_BYE := xen_bye_world.xsplice
> +XSPLICE_REPLACE := xen_replace_world.xsplice
>  
>  default: xsplice
>  
>  install: xsplice
>   $(INSTALL_DATA) $(XSPLICE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
>   $(INSTALL_DATA) $(XSPLICE_BYE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE)
> + $(INSTALL_DATA) $(XSPLICE_REPLACE) 
> $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_REPLACE)
>  uninstall:
>   rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
>   rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE)
> + rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_REPLACE)
>  
>  .PHONY: clean
>  clean::

Note how you (not using *.xsplice there) forget to clean the new
binary here?

> @@ -73,6 +76,13 @@ $(XSPLICE_BYE): $(XSPLICE) config.h xen_bye_world_func.o 
> xen_bye_world.o hello_w
>   $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_BYE) \
>   $(filter %.o,$^)
>  
> +xen_replace_world.o: config.h
> +
> +.PHONY: $(XSPLICE_REPLACE)
> +$(XSPLICE_REPLACE): config.h xen_replace_world_func.o xen_replace_world.o 
> note.o
> + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_REPLACE) \
> + $(filter %.o,$^)

Same here again: Ack with suitable adjustments.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 21/24] xsplice: Stacking build-id dependency checking.

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> --- a/xen/arch/x86/test/Makefile
> +++ b/xen/arch/x86/test/Makefile
> @@ -6,17 +6,20 @@ CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ 
> print "0x"$$2}')
>  .PHONY: default
>  
>  XSPLICE := xen_hello_world.xsplice
> +XSPLICE_BYE := xen_bye_world.xsplice
>  
>  default: xsplice
>  
>  install: xsplice
>   $(INSTALL_DATA) $(XSPLICE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
> + $(INSTALL_DATA) $(XSPLICE_BYE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE)
>  uninstall:
>   rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
> + rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE)
>  
>  .PHONY: clean
>  clean::
> - rm -f *.o .*.o.d $(XSPLICE) config.h
> + rm -f *.o .*.o.d $(XSPLICE) $(XSPLICE_BYE) config.h *.bin

While you can't do this in the uninstall target above, here using
*.xsplice would seem to be better (and then maybe do this right
away in the patch introducing the rule).

> +$(XSPLICE_BYE): $(XSPLICE) config.h xen_bye_world_func.o xen_bye_world.o 
> hello_world_note.o
> + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_BYE) \
> + $(filter %.o,$^)

Same here regarding config.h and $(filter ...), i.e. with that adjusted
Acked-by: Jan Beulich 

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 10/24] x86/xen_hello_world.xsplice: Test payload for patching 'xen_extra_version'.

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> --- /dev/null
> +++ b/xen/arch/x86/test/Makefile
> @@ -0,0 +1,42 @@
> +include $(XEN_ROOT)/Config.mk
> +
> +CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
> +CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
> +
> +.PHONY: default
> +
> +XSPLICE := xen_hello_world.xsplice
> +
> +default: xsplice
> +
> +install: xsplice
> + $(INSTALL_DATA) $(XSPLICE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
> +uninstall:
> + rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE)
> +
> +.PHONY: clean
> +clean::
> + rm -f *.o .*.o.d $(XSPLICE) config.h
> +
> +#
> +# To compute these values we need the binary files: xen-syms
> +# and xen_hello_world_func.o to be already compiled.
> +#
> +.PHONY: config.h
> +config.h: OLD_CODE=$(call CODE_ADDR,$(BASEDIR)/xen-syms,xen_extra_version)
> +config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version)
> +config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world)
> +config.h: xen_hello_world_func.o
> + (set -e; \
> +  echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \
> +  echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)"; \
> +  echo "#define OLD_CODE $(OLD_CODE)") > $@
> +
> +xen_hello_world.o: config.h
> +
> +.PHONY: $(XSPLICE)
> +$(XSPLICE): config.h xen_hello_world_func.o xen_hello_world.o

Th odd config.h dependency is still there, despite xen_hello_world.o
now properly depending on the header?

> + $(LD) $(LDFLAGS) -r -o $(XSPLICE) $(filter %.o,$^)

And if you (hopefully) drop it, please don't forget to the also ditch
the $(filter ...) here.

With those adjustments
Acked-by: Jan Beulich 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Overlaped PIO with multiple ioreq_server (Xen4.6.1)

[Paul Durrant]

> -Original Message-
> From: Martin Cerveny [mailto:mar...@c-home.cz]
> Sent: 28 April 2016 12:17
> To: Paul Durrant
> Cc: George Dunlap; Martin Cerveny; xen-de...@lists.xensource.com; Paolo
> Bonzini
> Subject: RE: [Xen-devel] Overlaped PIO with multiple ioreq_server
> (Xen4.6.1)
> 
> Hello.
> 
> On Thu, 28 Apr 2016, Paul Durrant wrote:
> 
> >> -Original Message-
> >> From: George Dunlap
> >> Sent: 28 April 2016 09:51
> >> To: Martin Cerveny
> >> Cc: xen-de...@lists.xensource.com; Paolo Bonzini; Paul Durrant
> >> Subject: Re: [Xen-devel] Overlaped PIO with multiple ioreq_server
> >> (Xen4.6.1)
> >>
> >> On Wed, Apr 27, 2016 at 8:38 PM, Martin Cerveny 
> >> wrote:
> >>> Hello.
> >>>
> >>> I have problem with multiple ioreq_servers
> >>> server 1 (emulates VGA) and server 2 (qemu).
> >>>
> >>> Emulation VGA server maps VGA PIO registers (3c0-3cf, 3b4-3b5 ...)
> >>> Qemu maps "all" PIO space (0-)
> >>> (ref:
> >>> http://xenbits.xen.org/gitweb/?p=qemu-
> >>
> xen.git;a=blob;f=exec.c;h=46fe70ed49f85d0638061aa5b09f1f9d521b0bd3;hb
> >> =18f2ce4bfe67f9b38143d9d96207e49c92b6881c#l2007
> >>> )
> >>> Xen does not check overlap errors between ioreq_servers
> >>> (ref:
> >>>
> >>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm
> >>
> .c;h=186e01e3b05a0264e8f4538b226da2feed50d11a;hb=d77bac5c064ffb9dbb
> >> 5b89b55b89853f1b784ebf#l1252
> >>> )
> >>> Xen choose "first match"
> >>> (ref:
> >>>
> >>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm
> >>
> .c;h=186e01e3b05a0264e8f4538b226da2feed50d11a;hb=d77bac5c064ffb9dbb
> >> 5b89b55b89853f1b784ebf#l2594
> >>> )
> >>>
> >>> In my case all requests to VGA PIO are sent to qemu (qemu VGA is
> disabled
> >>> with parameter "-display none"/"-vga none") and dropped.
> >>> Emulation VGA server receives only memory updates (eg. a-b).
> >>>
> >>> Is this problem resolved in updates (actual code looks the same
> (ioreq.c)) ?
> >>> Is there any prioritization between ioreq_servers (but it is probably bad
> >>> idea) ?
> >>> Should the IO mapping check overlap between ioreq_servers (but it is
> >>> probably bad idea) ?
> >>> Should the qemu IO map only emulated areas (why it needs all ?) ?
> >>
> >> I think the idea was that devicemodels should only request IO ports
> >> for devices they actually intend to emulate.  It sounds like this is
> >> an unfinished implementation inside of qemu.
> >>
> >
> > Does QEMU really map all of PIO space? That's not the behaviour I
> observed last time I looked. The memory_region_init_io() function just
> initializes QEMU's internal handlers IIRC; I think the IO ranges get 
> registered
> with Xen when the individual device models initialize. If you look in
> xen_common.h (in QEMU) then you'll note that there are tracepoints on all
> the map/unmap calls, so if you use an events list such as the following:
> >
> > xen_map_mmio_range
> > xen_unmap_mmio_range
> > xen_map_portio_range
> > xen_unmap_portio_range
> > xen_map_pcidev
> > xen_unmap_pcidev
> > xen_ioreq_server_create
> > xen_ioreq_server_destroy
> > xen_ioreq_server_state
> >
> > then you'll be able to see all the individual range registrations and
> deregistrations as well the ioreq server lifecycle.
> 
> Yes, I traced the problem. Used
> hvm_map_io_range_to_ioreq_server/hvm_unmap_io_range_from_ioreq_s
> erver
> (type, start, end).
> 
> My iorq_server begin:
> 
> (XEN) XXX map: 1 a a
> (XEN) XXX map: 1 b b
> (XEN) XXX map: 0 3c0 3cf
> (XEN) XXX map: 0 3b4 3b5
> (XEN) XXX map: 0 3d4 3d5
> (XEN) XXX map: 0 3ba 3ba
> (XEN) XXX map: 0 3da 3da
> (XEN) XXX map: 0 1ce 1d1
> (XEN) XXX map: 0 ff80 ff83
> 
> Qemu continues:
> 
> 
> (XEN) XXX map: 0 0  <- cited ref
> (XEN) XXX map: 1 fee0 feef
> (XEN) XXX unmap: 0 0 
> (XEN) XXX map: 0 0 cf7
> (XEN) XXX map: 0 cf8 cfb
> (XEN) XXX map: 0 cfc 
> (XEN) XXX unmap: 0 cfc   <- constatnly remapping to fill the unused
> space
> (XEN) XXX map: 0 cfc cff
> (XEN) XXX map: 0 d00 

... and again here. That really needs fixing.

> (XEN) XXX map: 2 0 0
> (XEN) XXX unmap: 0 cf8 cfb
> (XEN) XXX map: 0 cf8 cf8
> (XEN) XXX map: 0 cf9 cf9
> (XEN) XXX map: 0 cfa cfb
> ...
> (XEN) XXX unmap: 0 3f6 3f6
> (XEN) XXX map: 0 3f6 3f6
> (XEN) XXX unmap: 0 f1 1ef
> (XEN) XXX map: 0 f1 16f
> (XEN) XXX map: 0 170 177
> (XEN) XXX map: 0 178 1ef
> (XEN) XXX unmap: 0 1f8 3f0
> (XEN) XXX map: 0 1f8 375
> (XEN) XXX map: 0 376 376
> (XEN) XXX map: 0 377 3f0 <- final mapped range (colision to vga)
> (XEN) XXX map: 2 9 9
> 
> 
> > FWIW, I have my own vga/kbd/mouse emulator which I've happily used
> alongside QEMU (see
> http://xenbits.xen.org/gitweb/?p=people/pauldu/demu.git;a=shortlog;h=re
> fs/heads/console), although it's been a while since I last tested it.
> 
> Maybe you are lucky, qemu is registered before your own demu emulator.

I guess I was lucky.

> I used for testing your "demu" 2 years ago, now extending Citrix "vgpu",
> 

Re: [Xen-devel] xen.git build system (Re: [HACKATHON] Toolstack session)

[Wei Liu]

On Wed, Apr 27, 2016 at 06:03:54PM +0100, George Dunlap wrote:
> On 26/04/16 14:44, Wei Liu wrote:
> > Hi all
> > 
> > I spent some time this morning to work out the details of xen.git build
> > system.
> > 
> > * How build system works at the moment?
> >   1. Stubdom.mk.in and Tools.mk.in define FETCHER variable.
> >   2. m4/fetcher.m4 checks for wget or ftp, which becomes FETCHER.
> >   3. StdGNU.mk defines GIT. It can be overwritten by setting envar
> >  when building.
> >   4. scripts/git-checkout.sh is used to checkout git tree.
> >   5. Invocation of git-checkout.sh in Makefile, tools/Makefile and
> >  tools/firmware/Makefile.
> >   6. Direct invocation of GIT in Makefile, tools/Makefile,
> >  tools/firmware/Makefile in the subtree force update targets.
> >   7. stubdom/Makefile and tools/firmware/etherboot/Makefile invoke FETCHER.
> > 
> > * What will be cloned?
> >   1. mini-os
> >   2. qemu-trad
> >   3. qemu-xen -- can be skipped with --with-system-qemu
> >   4. seabios -- can be skipped with --with-system-seabios
> >   5. ovmf -- can be skipped with --with-system-ovmf
> > 
> > * What needs to be fetched?
> >   1. Stubdom needs:
> >  - newlib
> >  - zlib
> >  - libpci
> >  - lwip
> >  - gmp
> >  - polarssl
> >  - tpmemu
> >  - ocaml
> >  - grub
> >   2. tools/firmware/etherboot needs:
> >  - ipxe
> >   A dumb way of dealing with these tarballs might be just to commit them
> >   in tree. That way we can just eliminate fetcher all together.
> 
> Commit the tarballs in-tree?
> 

The extracted source code, actually.

> I don't think we want to do that; but we certainly could consider
> including them in our release tarball.
> 

This would do, too.

> >> Wei: what downstream consumers expect from the build system. Xen has a top 
> >> level makefile that builds everything, by pulling other projects source 
> >> code. Trying to make it cleaner.
> >>
> >> George: someone recommended to pull grub2 into the Xen build system, but 
> >> it 
> >> was seen as too much. Try to remove other pieces that Xen build system 
> >> pull 
> >> in in order to perform a build. Package Raisin in a way that includes all 
> >> the needed dependencies (source).
> >>
> >> Doug: Gentoo/Yocto build system based on the one from Debian. It's not 
> >> good 
> >> to pull things from the internet when performing a build. Yocto build 
> >> system 
> >> disables network access when performing a build, custom patches are needed 
> >> in order to fix that. XenServer has to do the same.
> >>
> > 
> > I can provide patches to add a --disable-download configure option. That
> > would basically stub out GIT and FETCHER to be /bin/false (Linux) or
> > /usr/bin/false (*BSD). The default would still be --enable-download.
> > 
> > Is such big switch good enough as first step?
> 
> I'm not sure that we necessarily need a "--disable-download" option.
> The thing that has to be patched out is that configure will fail if it
> can't find wget.
> 

It will be a side effect of --disable-download because FETCHER will be
set to false.  I like to make things explicit in this case.

Can you elaborate on how every thing fit together in your vision? There
must be something else that you have not expressed explicitly to make
sure the whole build environment (xen build system + the build host)
won't fetch external stuff.

I'm open to ideas on how things -- xen build system and build host --
fit together and make adequate adjustment to the xen build system. But
unfortunately I also envisage different people have different views on
how the world functions.

> >> How to fix:
> >>  - Everything controlled by the Xen build system, make it clear what will 
> >> be 
> >> downloaded, have a target to download the required sources.
> > 
> > What I don't really get is whether this implies a dedicated target to
> > download components (and recursively search for dependencies) *and*
> > place them in the right place in xen.git build system. This option
> > doesn't seem maintainable to me. The anticipation is that  we might add
> > more stuff in xen.git. We can't track what every package needs and
> > provide some options to work out where to put those dependencies.
> > 
> > I think having xen.git build system only track top-level components
> > (such as seabios, ovmf etc) is doable.
> 
> I don't think anyone was suggesting that we download all the
> sub-dependencies of everything recursively from scratch.  The
> expectation is that Xen will build in a "typical" distro environment,
> and that we'll download only things that we develop (xen, minios,
> qemu-trad, qemu-upstream, minios), things we expect a typical distro not
> to have (seabios, ovmf, ipxe), or things that need custom patches /
> recompilation (newlib and all the components recompiled against newlib
> for stubdom).
> 
> It might be worth going back to revisit some of these decisions --
> seabios is available in many distros now, and I don't think we've had

Re: [Xen-devel] [PATCH v10 08/24] xsplice: Implement payload loading

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> From: Ross Lagerwall 
> 
> Add support for loading xsplice payloads. This is somewhat similar to
> the Linux kernel module loader, implementing the following steps:
> - Verify the elf file.
> - Parse the elf file.
> - Allocate a region of memory mapped within a free area of
>   [xen_virt_end, XEN_VIRT_END].
> - Copy allocated sections into the new region. Split them in three
>   regions - .text, .data, and .rodata. MUST have at least .text.
> - Resolve section symbols. All other symbols must be absolute addresses.
>   (Note that patch titled "xsplice,symbols: Implement symbol name resolution
>on address" implements that)
> - Perform relocations.
> - Secure the the regions (.text,.data,.rodata) with proper permissions.
> 
> We capitalize on the vmalloc callback API (see patch titled:
> "rm/x86/vmap: Add v[z|m]alloc_xen, and vm_init_type") to allocate
> a region of memory within the [xen_virt_end, XEN_VIRT_END] for the code.
> 
> We also use the "x86/mm: Introduce modify_xen_mappings()"
> to change the virtual address page-table permissions.
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Konrad Rzeszutek Wilk 
> Acked-by: Julien Grall 

Reviewed-by: Jan Beulich 
with one remark (not strictly calling for a change):

> +static int move_payload(struct payload *payload, struct xsplice_elf *elf)
> +{
> +void *text_buf, *ro_buf, *rw_buf;
> +unsigned int i;
> +size_t size = 0;
> +unsigned int *offset;
> +int rc = 0;
> +
> +offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
> +if ( !offset )
> +return -ENOMEM;
> +
> +/* Compute size of different regions. */
> +for ( i = 1; i < elf->hdr->e_shnum; i++ )
> +{
> +/*
> + * Do nothing. These are .rel.text, rel.*, .symtab, .strtab,
> + * and .shstrtab. For the non-relocate we allocate and copy these
> + * via other means - and the .rel we can ignore as we only use it
> + * once during loading.
> + */
> +if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
> +{
> +offset[i] = UINT_MAX;
> +continue;
> +}

You could even have avoided the need for braces and "continue"
by making ...

> +if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&

... this "else if".

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 10:50
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 3/3] x86/vMSI-X: also snoop REP MOVS
> 
> ... as at least certain versions of Windows use such to update the
> MSI-X table. However, to not overly complicate the logic for now
> - only EFLAGS.DF=0 is being handled,
> - only updates not crossing MSI-X table entry boundaries are handled.
> 
> Signed-off-by: Jan Beulich 
> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -351,9 +351,10 @@ static int msixtbl_range(struct vcpu *v,
>  ASSERT(r->type == IOREQ_TYPE_COPY);
>  if ( r->dir == IOREQ_WRITE )
>  {
> +unsigned int size = r->size;
> +
>  if ( !r->data_is_ptr )
>  {
> -unsigned int size = r->size;
>  uint64_t data = r->data;
> 
>  if ( size == 8 )
> @@ -366,7 +367,22 @@ static int msixtbl_range(struct vcpu *v,
>   ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
>   !(data & PCI_MSIX_VECTOR_BITMASK) )
> +{
>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa = 0;
> +}
> +}
> +else if ( (size == 4 || size == 8) && !r->df &&
> +  r->count && r->count <= PCI_MSIX_ENTRY_SIZE / size &&
> +  !((addr + (size * r->count)) & (PCI_MSIX_ENTRY_SIZE - 1)) )

That's quite an if statement. Any chance of making it more decipherable? I also 
think it's worth putting the restrictions you outline in the commit in a 
comment here so that it's clear that the code is not trying to handle all 
corner cases.

> +{
> +BUILD_BUG_ON((PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET + 4) &
> + (PCI_MSIX_ENTRY_SIZE - 1));
> +
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_address =
> +addr + size * r->count - 4;
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa =
> +r->data + size * r->count - 4;

Does there need to be any explicit type promotion here since r->data is 
uint64_t?

  Paul

>  }
>  }
> 
> @@ -471,6 +487,7 @@ out:
>  for_each_vcpu ( d, v )
>  {
>  if ( (v->pause_flags & VPF_blocked_in_xen) &&
> + !v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa &&
>   v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
>   (gtable + msi_desc->msi_attrib.entry_nr *
> PCI_MSIX_ENTRY_SIZE +
> @@ -551,9 +568,29 @@ void msixtbl_pt_cleanup(struct domain *d
>  void msix_write_completion(struct vcpu *v)
>  {
>  unsigned long ctrl_address = v-
> >arch.hvm_vcpu.hvm_io.msix_unmask_address;
> +unsigned long snoop_addr = v-
> >arch.hvm_vcpu.hvm_io.msix_snoop_address;
> 
>  v->arch.hvm_vcpu.hvm_io.msix_snoop_address = 0;
> 
> +if ( !ctrl_address && snoop_addr &&
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa )
> +{
> +const struct msi_desc *desc;
> +uint32_t data;
> +
> +rcu_read_lock(_rcu_lock);
> +desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, snoop_addr),
> +snoop_addr);
> +rcu_read_unlock(_rcu_lock);
> +
> +if ( desc &&
> + hvm_copy_from_guest_phys(,
> +  v->arch.hvm_vcpu.hvm_io.msix_snoop_gpa,
> +  sizeof(data)) == HVMCOPY_okay &&
> + !(data & PCI_MSIX_VECTOR_BITMASK) )
> +ctrl_address = snoop_addr;
> +}
> +
>  if ( !ctrl_address )
>  return;
> 
> --- unstable.orig/xen/include/asm-x86/hvm/vcpu.h  2016-04-27
> 14:47:25.0 +0200
> +++ unstable/xen/include/asm-x86/hvm/vcpu.h   2016-04-25
> 16:04:48.0 +0200
> @@ -86,6 +86,7 @@ struct hvm_vcpu_io {
> 
>  unsigned long msix_unmask_address;
>  unsigned long msix_snoop_address;
> +unsigned long msix_snoop_gpa;
> 
>  const struct g2m_ioport *g2m_ioport;
>  };
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 07/24] xsplice: Add helper elf routines

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> From: Ross Lagerwall 
> 
> Add Elf routines and data structures in preparation for loading an
> xSplice payload.
> 
> We make an assumption that the max number of sections an ELF payload
> can have is 64. We can in future make this be dependent on the
> names of the sections and verifying against a list, but for right now
> this suffices.
> 
> Also we a whole lot of checks to make sure that the ELF payload
> file is not corrupted nor that the offsets point past the file.
> 
> For most of the checks we print an message if the hypervisor is built
> with debug enabled.
> 
> Signed-off-by: Ross Lagerwall 
> Signed-off-by: Konrad Rzeszutek Wilk 
> Acked-by: Ian Jackson 

Reviewed-by: Jan Beulich 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] Overlaped PIO with multiple ioreq_server (Xen4.6.1)

[Martin Cerveny]

Hello.

On Thu, 28 Apr 2016, Paul Durrant wrote:


-Original Message-
From: George Dunlap
Sent: 28 April 2016 09:51
To: Martin Cerveny
Cc: xen-de...@lists.xensource.com; Paolo Bonzini; Paul Durrant
Subject: Re: [Xen-devel] Overlaped PIO with multiple ioreq_server
(Xen4.6.1)

On Wed, Apr 27, 2016 at 8:38 PM, Martin Cerveny 
wrote:

Hello.

I have problem with multiple ioreq_servers
server 1 (emulates VGA) and server 2 (qemu).

Emulation VGA server maps VGA PIO registers (3c0-3cf, 3b4-3b5 ...)
Qemu maps "all" PIO space (0-)
(ref:
http://xenbits.xen.org/gitweb/?p=qemu-

xen.git;a=blob;f=exec.c;h=46fe70ed49f85d0638061aa5b09f1f9d521b0bd3;hb
=18f2ce4bfe67f9b38143d9d96207e49c92b6881c#l2007

)
Xen does not check overlap errors between ioreq_servers
(ref:


http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm
.c;h=186e01e3b05a0264e8f4538b226da2feed50d11a;hb=d77bac5c064ffb9dbb
5b89b55b89853f1b784ebf#l1252

)
Xen choose "first match"
(ref:


http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/x86/hvm/hvm
.c;h=186e01e3b05a0264e8f4538b226da2feed50d11a;hb=d77bac5c064ffb9dbb
5b89b55b89853f1b784ebf#l2594

)

In my case all requests to VGA PIO are sent to qemu (qemu VGA is disabled
with parameter "-display none"/"-vga none") and dropped.
Emulation VGA server receives only memory updates (eg. a-b).

Is this problem resolved in updates (actual code looks the same (ioreq.c)) ?
Is there any prioritization between ioreq_servers (but it is probably bad
idea) ?
Should the IO mapping check overlap between ioreq_servers (but it is
probably bad idea) ?
Should the qemu IO map only emulated areas (why it needs all ?) ?


I think the idea was that devicemodels should only request IO ports
for devices they actually intend to emulate.  It sounds like this is
an unfinished implementation inside of qemu.



Does QEMU really map all of PIO space? That's not the behaviour I observed last 
time I looked. The memory_region_init_io() function just initializes QEMU's 
internal handlers IIRC; I think the IO ranges get registered with Xen when the 
individual device models initialize. If you look in xen_common.h (in QEMU) then 
you'll note that there are tracepoints on all the map/unmap calls, so if you 
use an events list such as the following:

xen_map_mmio_range
xen_unmap_mmio_range
xen_map_portio_range
xen_unmap_portio_range
xen_map_pcidev
xen_unmap_pcidev
xen_ioreq_server_create
xen_ioreq_server_destroy
xen_ioreq_server_state

then you'll be able to see all the individual range registrations and 
deregistrations as well the ioreq server lifecycle.


Yes, I traced the problem. Used 
hvm_map_io_range_to_ioreq_server/hvm_unmap_io_range_from_ioreq_server
(type, start, end).

My iorq_server begin:

(XEN) XXX map: 1 a a
(XEN) XXX map: 1 b b
(XEN) XXX map: 0 3c0 3cf
(XEN) XXX map: 0 3b4 3b5
(XEN) XXX map: 0 3d4 3d5
(XEN) XXX map: 0 3ba 3ba
(XEN) XXX map: 0 3da 3da
(XEN) XXX map: 0 1ce 1d1
(XEN) XXX map: 0 ff80 ff83

Qemu continues:


(XEN) XXX map: 0 0  <- cited ref
(XEN) XXX map: 1 fee0 feef
(XEN) XXX unmap: 0 0 
(XEN) XXX map: 0 0 cf7
(XEN) XXX map: 0 cf8 cfb
(XEN) XXX map: 0 cfc 
(XEN) XXX unmap: 0 cfc   <- constatnly remapping to fill the unused 
space
(XEN) XXX map: 0 cfc cff
(XEN) XXX map: 0 d00 
(XEN) XXX map: 2 0 0
(XEN) XXX unmap: 0 cf8 cfb
(XEN) XXX map: 0 cf8 cf8
(XEN) XXX map: 0 cf9 cf9
(XEN) XXX map: 0 cfa cfb
...
(XEN) XXX unmap: 0 3f6 3f6
(XEN) XXX map: 0 3f6 3f6
(XEN) XXX unmap: 0 f1 1ef
(XEN) XXX map: 0 f1 16f
(XEN) XXX map: 0 170 177
(XEN) XXX map: 0 178 1ef
(XEN) XXX unmap: 0 1f8 3f0 
(XEN) XXX map: 0 1f8 375
(XEN) XXX map: 0 376 376 
(XEN) XXX map: 0 377 3f0 <- final mapped range (colision to vga)

(XEN) XXX map: 2 9 9



FWIW, I have my own vga/kbd/mouse emulator which I've happily used alongside 
QEMU (see 
http://xenbits.xen.org/gitweb/?p=people/pauldu/demu.git;a=shortlog;h=refs/heads/console),
 although it's been a while since I last tested it.


Maybe you are lucky, qemu is registered before your own demu emulator.
I used for testing your "demu" 2 years ago, now extending Citrix "vgpu", 
all was fine up to xen 4.5.2 (with qemu 2.0.2) but problem begin when I 
switched to 4.6.1 (with qemu 2.2.1), but it maybe lucky timing in 
registration.


M.C>


 Paul


 -George




___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 02/24] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op

[Jan Beulich]

>>> On 27.04.16 at 21:26,  wrote:
> The implementation does not actually do any patching.
> 
> It just adds the framework for doing the hypercalls,
> keeping track of ELF payloads, and the basic operations:
>  - query which payloads exist,
>  - query for specific payloads,
>  - check*1, apply*1, replace*1, and unload payloads.
> 
> *1: Which of course in this patch are nops.
> 
> The functionality is disabled on ARM until all arch
> components are implemented.
> 
> Also by default it is disabled until the implementation
> is in place.
> 
> We also use recursive spinlocks to so that the find_payload
> function does not need to have a 'lock' and 'non-lock' variant.
> 
> Signed-off-by: Konrad Rzeszutek Wilk 
> Signed-off-by: Ross Lagerwall 
> Reviewed-by: Andrew Cooper 

This contradicts ...
> v10:
> Dropped Reviewed-by.

... this. Also I continue to be puzzled why I'm not getting Cc-ed on
this patch.

Reviewed-by: Jan Beulich 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH for 4.7] Remove HVMMEM_mmio_write_dm from the public interface.

[Jan Beulich]

>>> On 28.04.16 at 12:42,  wrote:
> On 28/04/16 11:22, Jan Beulich wrote:
> On 28.04.16 at 10:29,  wrote:
>>> @@ -5529,7 +5527,7 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>  [HVMMEM_ram_rw]  = p2m_ram_rw,
>>>  [HVMMEM_ram_ro]  = p2m_ram_ro,
>>>  [HVMMEM_mmio_dm] = p2m_mmio_dm,
>>> -[HVMMEM_mmio_write_dm] = p2m_mmio_write_dm
>>> +[HVMMEM_unused] = p2m_invalid
>> 
>> Why don't you simply delete the old line, without replacement?
> 
> That might have been slightly cleaner; but we're going to have to put it
> back as soon as the development window opens anyway, so I don't really
> see the point of going through the effort of respinning the patch again.
> 
> Would you be willing to ack this version anyway?

I have no problem doing so (and in fact I have it on my to by
committed list already), it is just looked slightly confusing (and
I had already typed half a reply that this isn't what was discussed
until I properly looked at the next hunk), and hence I wanted to
understand the motivation. And btw., I'm not convinced it would
need to be put there anyway later: I don't view the used
mechanism as a good (read: extensible) one to deal with what
would be holes in the array above. Indeed we can't leave them
uninitialized (as that would mean p2m_ram_rw), but I think we
should better find a way to initialize _all_ unused slots without
requiring an initializer for each of them. Sadly the desire to allow
compilation with clang prohibits the most natural solution:

static const p2m_type_t memtype[] = {
[0 ...  - 1] = p2m_invalid,
[HVMMEM_ram_rw]  = p2m_ram_rw,
[HVMMEM_ram_ro]  = p2m_ram_ro,
[HVMMEM_mmio_dm] = p2m_mmio_dm,
};

Maybe we could do (altering the second hunk of this patch)

@@ -5553,7 +5551,10 @@ long do_hvm_op(unsigned long op, 
XEN_GUEST_HANDLE_PARAM(void) arg)
  ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
 goto setmemtype_fail;
 
-if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
+BUILD_BUG_ON(p2m_ram_rw);
+BUILD_BUG_ON(HVMMEM_ram_rw);
+if ( a.hvmmem_type >= ARRAY_SIZE(memtype) ||
+ (a.hvmmem_type && !memtype[a.hvmmem_type]) )
 goto setmemtype_fail;
 
 while ( a.nr > start_iter )

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3] tools: detect appropriate debug optimization level

[Wei Liu]

On Tue, Apr 26, 2016 at 09:38:45AM -0500, Doug Goldstein wrote:
> When building debug use -Og as the optimization level if its available,
> otherwise retain the use of -O0. -Og has been added by GCC to enable all
> optimizations that to not affect debugging while retaining full
> debugability.
> 
> Signed-off-by: Doug Goldstein 

I think Ian's comment has been addressed in this version:

Acked-by: Wei Liu 

I plan to accept this patch for 4.7, the risk is rather low anyway.

I will wait a bit before committing in case Ian has more to say.

> ---
> change since v2:
> - switch back to cc-option-add to not call cc-option on every invocation
> change since v1:
> - switch to cc-option to only specify -O0 if -Og isn't supported
> ---
>  tools/Rules.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/Rules.mk b/tools/Rules.mk
> index 9ef0b47..1b79a6e 100644
> --- a/tools/Rules.mk
> +++ b/tools/Rules.mk
> @@ -138,6 +138,7 @@ SHLIB_libxenvchan  = $(SHDEPS_libxenvchan) 
> -Wl,-rpath-link=$(XEN_LIBVCHAN)
>  ifeq ($(debug),y)
>  # Disable optimizations and enable debugging information for macros
>  CFLAGS += -O0 -g3
> +$(call cc-option-add,CFLAGS,CC,-Og)
>  # But allow an override to -O0 in case Python enforces -D_FORTIFY_SOURCE=.
>  PY_CFLAGS += $(PY_NOOPT_CFLAGS)
>  endif
> -- 
> 2.7.3
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7): Rename p2m_mmio_write_dm to p2m_ioreq_server.

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 11:03
> To: Paul Durrant
> Cc: Andrew Cooper; George Dunlap; Wei Liu; JunNakajima; Kevin Tian;
> Zhiyuan Lv; Zhang Yu; xen-devel@lists.xen.org; Keir (Xen.org); Tim (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v3 1/3] x86/ioreq server(patch for 4.7):
> Rename p2m_mmio_write_dm to p2m_ioreq_server.
> 
> >>> On 28.04.16 at 09:14,  wrote:
> >> From: Yu, Zhang [mailto:yu.c.zh...@linux.intel.com]
> >> Sent: 28 April 2016 03:47
> >> Besides, do you think it acceptable we just replace p2m_mmio_write_dm
> >> with p2m_ioreq_server in next version, or should we remove this p2m
> >> type and define p2m_ioreq_server with a different value? :)
> >>
> >
> > p2m_ioreq_server becomes defunct after the aforementioned patch. With
> that
> > patch applied there will be no way to set that type on a p2m entry so it
> > should be ok to re-use the type.
> 
> s/p2m_ioreq_server/p2m_mmio_write_dm/ ?
> 

Yes, that's what I meant.

  Paul

> Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 2/3] x86/vMSI-X: also snoop qword writes

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 10:50
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 2/3] x86/vMSI-X: also snoop qword writes
> 
> ... the high half of which may be a write to the Vector Control field.
> This gets things in sync again with msixtbl_write().
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Paul Durrant 

> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -336,6 +336,7 @@ out:
>  static int msixtbl_range(struct vcpu *v, unsigned long addr)
>  {
>  const struct msi_desc *desc;
> +const ioreq_t *r;
> 
>  rcu_read_lock(_rcu_lock);
>  desc = msixtbl_addr_to_desc(msixtbl_find_entry(v, addr), addr);
> @@ -344,17 +345,29 @@ static int msixtbl_range(struct vcpu *v,
>  if ( desc )
>  return 1;
> 
> -if ( (addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET )
> +r = >arch.hvm_vcpu.hvm_io.io_req;
> +if ( r->state != STATE_IOREQ_READY || r->addr != addr )
> +return 0;
> +ASSERT(r->type == IOREQ_TYPE_COPY);
> +if ( r->dir == IOREQ_WRITE )
>  {
> -const ioreq_t *r = >arch.hvm_vcpu.hvm_io.io_req;
> +if ( !r->data_is_ptr )
> +{
> +unsigned int size = r->size;
> +uint64_t data = r->data;
> 
> -if ( r->state != STATE_IOREQ_READY || r->addr != addr )
> -return 0;
> -ASSERT(r->type == IOREQ_TYPE_COPY);
> -if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
> - && !(r->data & PCI_MSIX_VECTOR_BITMASK) )
> -v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +if ( size == 8 )
> +{
> +BUILD_BUG_ON(!(PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET & 4));
> +data >>= 32;
> +addr += size = 4;
> +}
> +if ( size == 4 &&
> + ((addr & (PCI_MSIX_ENTRY_SIZE - 1)) ==
> +  PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) &&
> + !(data & PCI_MSIX_VECTOR_BITMASK) )
> +v->arch.hvm_vcpu.hvm_io.msix_snoop_address = addr;
> +}
>  }
> 
>  return 0;
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 28 April 2016 10:49
> To: xen-devel
> Cc: Andrew Cooper; Paul Durrant
> Subject: [PATCH 1/3] x86/vMSI-X: add further checks to snoop logic
> 
> msixtbl_range(), as any other MMIO ->check() handlers, may get called
> with other than the base address of an access - avoid the snoop logic
> considering those.
> 
> Also avoid considering vCPU-s not blocked in the hypervisor in
> msixtbl_pt_register(), just to be on the safe side.
> 
> Signed-off-by: Jan Beulich 

Reviewed-by: Paul Durrant 

> 
> --- a/xen/arch/x86/hvm/vmsi.c
> +++ b/xen/arch/x86/hvm/vmsi.c
> @@ -349,7 +349,7 @@ static int msixtbl_range(struct vcpu *v,
>  {
>  const ioreq_t *r = >arch.hvm_vcpu.hvm_io.io_req;
> 
> -if ( r->state != STATE_IOREQ_READY )
> +if ( r->state != STATE_IOREQ_READY || r->addr != addr )
>  return 0;
>  ASSERT(r->type == IOREQ_TYPE_COPY);
>  if ( r->dir == IOREQ_WRITE && r->size == 4 && !r->data_is_ptr
> @@ -457,7 +457,8 @@ out:
> 
>  for_each_vcpu ( d, v )
>  {
> -if ( v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
> +if ( (v->pause_flags & VPF_blocked_in_xen) &&
> + v->arch.hvm_vcpu.hvm_io.msix_snoop_address ==
>   (gtable + msi_desc->msi_attrib.entry_nr *
> PCI_MSIX_ENTRY_SIZE +
>PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET) )
> 
> 


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel