Re: [Xen-devel] [PATCH v3 09/16] efi: explicitly define efi struct in xen/arch/x86/efi/stub.c

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> Existing solution does not allocate space for this symbol and any
> references to acpi20, etc. does not make sense. As I saw any efi.*
> references are protected by relevant ifs but we should not do that
> because it makes code very fragile. If somebody does not know how
> efi symbol is created he/she may assume that it always represent
> valid structure and do invalid references somewhere.

I do not view this as a valid reason for the change.

> Additionally, following patch adds efi struct flags member which
> is used during runtime to differentiate between legacy BIOS and
> EFI platforms and multiboot2 and EFI native loader. So, efi symbol
> have to proper representation in ELF and PE Xen image. Hence,
> define efi struct in xen/arch/x86/efi/stub.c and remove efi
> symbol from ld script.

Only this one is, afaic. The only request here would be to replace
"following" by e.g. "a subsequent", to make the description
independent of whether the two patches get committed together.

> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -8,6 +8,14 @@
>  const bool_t efi_enabled = 0;
>  #endif
>  
> +struct efi __read_mostly efi = {
> + .acpi= EFI_INVALID_TABLE_ADDR,
> + .acpi20  = EFI_INVALID_TABLE_ADDR,
> + .mps = EFI_INVALID_TABLE_ADDR,
> + .smbios  = EFI_INVALID_TABLE_ADDR,
> + .smbios3 = EFI_INVALID_TABLE_ADDR
> +};

I don't view duplicating this here as a good approach - you'd better
move the existing instance elsewhere. If this was a temporary thing
(until a later patch), it might be acceptable, but since building without
EFI support will need to remain an option (for people using older tool
chains), I don't expect a later patch to remove this.

Jan


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

Re: [Xen-devel] [PATCH v3 10/16] efi: create efi_enabled()

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> --- a/xen/arch/x86/efi/stub.c
> +++ b/xen/arch/x86/efi/stub.c
> @@ -4,11 +4,8 @@
>  #include 
>  #include 
>  
> -#ifndef efi_enabled
> -const bool_t efi_enabled = 0;
> -#endif
> -
>  struct efi __read_mostly efi = {
> + .flags   = 0, /* Initialized later. */

This is pointless to add - the field will get zero-initialized anyway.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -934,6 +934,10 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>  char *option_str;
>  bool_t use_cfg_file;
>  
> +#ifndef CONFIG_ARM /* Disabled until runtime services implemented. */
> +set_bit(EFI_PLATFORM, &efi.flags);
> +#endif

Surely this can be __set_bit()? It's also hard to see what setting this
flag has got to do with runtime services. But more on this below.

> @@ -42,11 +38,12 @@ UINT64 __read_mostly efi_boot_remain_var_store_size;
>  UINT64 __read_mostly efi_boot_max_var_size;
>  
>  struct efi __read_mostly efi = {
> - .acpi   = EFI_INVALID_TABLE_ADDR,
> - .acpi20 = EFI_INVALID_TABLE_ADDR,
> - .mps= EFI_INVALID_TABLE_ADDR,
> - .smbios = EFI_INVALID_TABLE_ADDR,
> - .smbios3 = EFI_INVALID_TABLE_ADDR,
> + .flags   = 0, /* Initialized later. */
> + .acpi= EFI_INVALID_TABLE_ADDR,
> + .acpi20  = EFI_INVALID_TABLE_ADDR,
> + .mps = EFI_INVALID_TABLE_ADDR,
> + .smbios  = EFI_INVALID_TABLE_ADDR,
> + .smbios3 = EFI_INVALID_TABLE_ADDR
>  };

This, again, is an unnecessary hunk. And in no case should you drop
the trailing comma - that's there for a reason.

> --- a/xen/include/xen/efi.h
> +++ b/xen/include/xen/efi.h
> @@ -2,15 +2,17 @@
>  #define __XEN_EFI_H__
>  
>  #ifndef __ASSEMBLY__
> +#include 
>  #include 
>  #endif
>  
> -extern const bool_t efi_enabled;
> -
>  #define EFI_INVALID_TABLE_ADDR (~0UL)
>  
> +#define EFI_PLATFORM 0

So what does "platform" mean? Did you consider using the more fine
grained set of flags Linux uses nowadays? That would also eliminate
the odd connection to runtime services mentioned earlier.

And please add a comment making clear that these values are bit
positions to be used in the flags field below. I might also help to
move this right next to the structure field.

> @@ -40,6 +42,12 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *);
>  int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *);
>  int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *);
>  
> +/* Test whether the above EFI_* bits are enabled. */

The comment leaves open which EFI_* values you actually refer to.
Hence another option would be to move those #define-s here.

> +static inline bool_t efi_enabled(int feature)

unsigned int

> +{
> +return test_bit(feature, &efi.flags) != 0;
> +}

Please use the more conventional !! found elsewhere in our code.

Jan


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

Re: [Xen-devel] [PATCH v3 11/16] efi: build xen.gz with EFI code

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -1,14 +1,9 @@
>  CFLAGS += -fshort-wchar
>  
> -obj-y += stub.o
> -
> -create = test -e $(1) || touch -t 19990101 $(1)
> -
>  efi := y$(shell rm -f disabled)
>  efi := $(if $(efi),$(shell $(CC) $(filter-out $(CFLAGS-y) .%.d,$(CFLAGS)) -c 
> check.c 2>disabled && echo y))
>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
> check.o 2>disabled && echo y))
> -efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); 
> $(call create,runtime.o)))
> +efi := $(if $(efi),$(shell rm disabled)y)
>  
> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
> -
> -stub.o: $(extra-y)
> +obj-y := stub.o
> +obj-$(efi) := boot.init.o compat.o relocs-dummy.o runtime.o

I assume/hope all these adjustments work for all intended cases, but
they quite clearly leave stale bits in xen/arch/x86/Rules.mk: Its
references to efi/*.o should all go away now afaict.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -1244,6 +1244,9 @@ void __init efi_init_memory(void)
>  } *extra, *extra_head = NULL;
>  #endif
>  
> +if ( !efi_enabled(EFI_PLATFORM) )
> +return;

Arguably such checks would then better be put at the call site,
allowing the respective stubs to just BUG().

Also - what's your rule for where to put such efi_enabled() checks?
I would have expected them to get added to everything that has
a counterpart in stubs.c, but things like efi_get_time() or
efi_{halt,reset}_system() don't get any added. If those are
unreachable, I'd at least expect respective ASSERT()s to get added
there.

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -167,6 +167,9 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
>  {
>  unsigned int i, n;
>  
> +if ( !efi_enabled(EFI_PLATFORM) )
> +return -EOPNOTSUPP;

Please do not introduce behavioral differences to the current stub
implementations: This and ...

> @@ -301,6 +304,9 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *op)
>  EFI_STATUS status = EFI_NOT_STARTED;
>  int rc = 0;
>  
> +if ( !efi_enabled(EFI_PLATFORM) )
> +return -EOPNOTSUPP;

... this return -ENOSYS there.

Jan


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

Re: [Xen-devel] [PATCH v5 01/10] vt-d: fix the IOMMU flush issue

[Xu, Quan]

On May 23, 2016 11:43 PM, Jan Beulich  wrote:
> >>> On 23.05.16 at 17:22,  wrote:
> > On May 23, 2016 9:31 PM, Jan Beulich  wrote:
> >> >>> On 18.05.16 at 10:08,  wrote:
> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> > @@ -557,14 +557,16 @@ static void iommu_flush_all(void)
> >> >  }
> >> >  }
> >> >
> >> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long
> gfn,
> >> > -int dma_old_pte_present, unsigned int page_count)
> >> > +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned long
> gfn,
> >> > + bool_t dma_old_pte_present,
> >> > + unsigned int page_count)
> >>
> >> I realize you say so in the overview mail, but the continuing lack of
> >> __must_check here causes review trouble again. And I have a hard time
> >> seeing how adding these annotations right away would "disrupt the
> >> order", as long as the series is properly ordered / broken up.
> >>
> >
> > If I add __must_check annotations here right now, e.g.
> >
> > -static void intel_iommu_iotlb_flush()
> > +static int __must_check iommu_flush_iotlb_pages()
> >
> > ...
> >
> > @@ -179,8 +179,9 @@ struct iommu_ops {
> > -void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int
> > page_count);
> > +int __must_check (*iotlb_flush)(struct domain *d, unsigned long
> > + gfn,
> > unsigned int page_count);
> > ...
> > }
> >
> > Should belong  to here too.
> 
> Correct. And where's the problem?
>

IMO it is not a big deal..

I think this makes this patch 1 fat.. why not focus on the positive propagation 
value from IOMMU flush interfaces in this patch.
If we are clear I will add annotation and rename them in another patches, it is 
acceptable to me.

Furthermore, we also need to add (from patch 4):

-static void dma_pte_clear_one(struct domain *domain, u64 addr)
+static int __must_check dma_pte_clear_one(struct domain *domain, u64 addr)
{
...
-__intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
+rc = __intel_iommu_iotlb_flush(domain, addr >> PAGE_SHIFT_4K, 1, 1);
...
}

In this patch.

 
> >> > +DMA_CCMD_MASK_NOBIT, 1);
> >> > +
> >> > +/*
> >> > + * The current logic for rc returns:
> >> > + *   - positive  invoke iommu_flush_write_buffer to flush cache.
> >> > + *   - zero  success.
> >> > + *   - negative  failure. Continue to flush IOMMU IOTLB on a best
> >> > + *   effort basis.
> >> > + */
> >> > +if ( rc <= 0 )
> >> >  {
> >> >  int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> >> > -iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> >> > +
> >> > +rc = iommu_flush_iotlb_dsi(iommu, 0, 1, flush_dev_iotlb);
> >>
> >> If rc was negative before this call, you may end up returning success
> > without
> >> having been successful. Furthermore I think it was you who last time
> >> round reminded me that
> >> iommu_flush_iotlb_dsi() can also return 1, which you don't take care of.
> >>
> >
> > Yes, the iommu_flush_iotlb_dsi() can also return 1.
> > Look at the call tree, at the beginning of
> > flush_context_qi()/flush_iotlb_qi(), or
> > flush_context_reg()/flush_iotlb_reg()..
> >
> > If rc was negative when we call iommu_flush_context_device(), it is
> > impossible to return 1 for iommu_flush_iotlb_dsi().
> 
> This is far from obvious, so please add a respective ASSERT() if you want to
> rely on that.
> 
> > IMO, furthermore, this should not belong  to comment.
> 
> ???

Think twice, I will add comments and a respective ASSERT()..

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

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

[Jan Beulich]

>>> On 25.05.16 at 08:41,  wrote:
> On May 24, 2016 4:22 PM, Jan Beulich  wrote:
>> >>> On 18.05.16 at 10:08,  wrote:
>> >  static int device_power_down(void)
>> >  {
>> > -console_suspend();
>> > +if ( console_suspend() )
>> > +return SAVED_CONSOLE;
>> 
>> I said so on the previous round, and I need to repeat it now: If
>> console_suspend() fails, you saved _nothing_.
>> 
> 
> Ah, we may have some different views for SAVED_*, which I mean has been 
> saved and we are no need to resume.
> 
> e.g. if console_suspend() fails, I did return SAVED_CONSOLE, reading my 
> patch again, and I really resume nothing at all.
> 
> device_power_up()
> {
>  ...
> +case SAVED_CONSOLE:
> +break;
> ...
> }
> 
> 
> I know we can also propagate SAVED_NONE for console_suspend() failure, then 
> we need adjust device_power_up() relevantly.

My main point is that the names of these enumerators should reflect
their purpose. If one reads "SAVED_CONSOLE", then (s)he should be
allowed this to mean that console state was saved (and hence needs
to be restored upon error / resume).

>> > -time_suspend();
>> > +if ( time_suspend() )
>> > +return SAVED_TIME;
>> >
>> > -i8259A_suspend();
>> > +if ( i8259A_suspend() )
>> > +return SAVED_I8259A;
>> >
>> > +/* ioapic_suspend cannot fail */
>> >  ioapic_suspend();
>> >
>> > -iommu_suspend();
>> > +if ( iommu_suspend() )
>> > +return SAVED_IOMMU;
>> >
>> > -lapic_suspend();
>> > +if ( lapic_suspend() )
>> > +return SAVED_LAPIC;
>> >
>> >  return 0;
>> 
>> And this silently means SAVED_NONE, whereas here you saved everything.
>> Yielding clearly bogus code ...
>>
> 
> 
>  '0' is just on success here.  Look at the condition where we call 
> device_power_up():
> 
> +if ( error > 0 )
> +device_power_up(error);
> 
> Then, it is not bogus code.

See above: Zero should not mean both "nothing saved" and "saved
everything".

>> Also, having come here - did I miss iommu_flush_iotlb_global() gaining a
>> __must_check annotation somewhere? 
> 
> I will add __must_check annotation to iommu_flush_iotlb_global().
> 
>> And the struct iommu_flush pointers
>> and handlers? And, by analogy, iommu_flush_context_*()?
> 
> I am better only add __must_check annotation to flush->iotlb and handlers,
> but leaving flush->context/handers and  iommu_flush_context_*() as are in 
> current patch set..
> the coming patch set will fix them.

I don't follow the logic behind this: The purpose of this series is to
make sure flushing errors get properly bubbled up, which includes
adding __must_check annotations. I'm not saying this needs to
happen in this patch, but it should happen in this series (and please
following the same basic model: A caller or a __must_check function
should either already be __must_check, or should become so at the
same time).

Jan


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

Re: [Xen-devel] [PATCH v5 01/10] vt-d: fix the IOMMU flush issue

[Jan Beulich]

>>> On 25.05.16 at 10:04,  wrote:
> On May 23, 2016 11:43 PM, Jan Beulich  wrote:
>> >>> On 23.05.16 at 17:22,  wrote:
>> > On May 23, 2016 9:31 PM, Jan Beulich  wrote:
>> >> >>> On 18.05.16 at 10:08,  wrote:
>> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> > @@ -557,14 +557,16 @@ static void iommu_flush_all(void)
>> >> >  }
>> >> >  }
>> >> >
>> >> > -static void __intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> gfn,
>> >> > -int dma_old_pte_present, unsigned int page_count)
>> >> > +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned long
>> gfn,
>> >> > + bool_t dma_old_pte_present,
>> >> > + unsigned int page_count)
>> >>
>> >> I realize you say so in the overview mail, but the continuing lack of
>> >> __must_check here causes review trouble again. And I have a hard time
>> >> seeing how adding these annotations right away would "disrupt the
>> >> order", as long as the series is properly ordered / broken up.
>> >>
>> >
>> > If I add __must_check annotations here right now, e.g.
>> >
>> > -static void intel_iommu_iotlb_flush()
>> > +static int __must_check iommu_flush_iotlb_pages()
>> >
>> > ...
>> >
>> > @@ -179,8 +179,9 @@ struct iommu_ops {
>> > -void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned int
>> > page_count);
>> > +int __must_check (*iotlb_flush)(struct domain *d, unsigned long
>> > + gfn,
>> > unsigned int page_count);
>> > ...
>> > }
>> >
>> > Should belong  to here too.
>> 
>> Correct. And where's the problem?
>>
> 
> IMO it is not a big deal..
> 
> I think this makes this patch 1 fat.. why not focus on the positive 
> propagation value from IOMMU flush interfaces in this patch.
> If we are clear I will add annotation and rename them in another patches, it 
> is acceptable to me.

The patch getting too large is easy to deal with: Split it at a reasonable
boundary. It's one thing that I want to be clear: Any conversion of a
void return type of some function to non-void should be accompanied
by it at the same time becoming __must_check. I dislike having to
repeat yet again what I have been saying a number of times: Without
doing so, it is harder for you as the person writing the patch to verify
all callers deal with errors, and it's perhaps even harder for reviewers
to verify you did.

Jan


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

Re: [Xen-devel] [PATCH v3 12/16 - RFC] x86/efi: create new early memory allocator

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> There is a problem with place_string() which is used as early memory
> allocator. It gets memory chunks starting from start symbol and
> going down. Sadly this does not work when Xen is loaded using multiboot2
> protocol because start lives on 1 MiB address. So, I tried to use
> mem_lower address calculated by GRUB2. However, it works only on some
> machines. There are machines in the wild (e.g. Dell PowerEdge R820)
> which uses first ~640 KiB for boot services code or data... :-(((
> 
> In case of multiboot2 protocol we need that place_string() only allocate
> memory chunk for EFI memory map. However, I think that it should be fixed
> instead of making another function used just in one case. I thought about
> two solutions.
> 
> 1) We could use native EFI allocation functions (e.g. AllocatePool()
>or AllocatePages()) to get memory chunk. However, later (somewhere
>in __start_xen()) we must copy its contents to safe place or reserve
>this in e820 memory map and map it in Xen virtual address space.
>In later case we must also care about conflicts with e.g. crash
>kernel regions which could be quite difficult.

I don't see why that would be: Simply use an allocation type that
doesn't lead to the area getting consumed as normal RAM. Nor do
I see the kexec collision potential. Furthermore (and I think I've
said so before) ARM is already using AllocatePool() - just with an
unsuitable memory type -, so doing so on x86 too would allow for
efi_arch_allocate_mmap_buffer() to go away.

> Jan Beulich added 1b) Do away with efi_arch_allocate_mmap_buffer() and use
>AllocatePages() uniformly, perhaps with a per-arch specified memory type
>(by means of which you can control whether the memory contents will remain
>preserved until the time you want to look at it). That will eliminate the
>only place_string() you're concerned about, with a patch with better
>diffstat (largely due to the questionable arch hook gone).
> 
> However, this solution does not solve conflicts problem described in #1
> because EFI memory map is needed during Xen runtime after init phase.
> So, finally we would get back to #1. Hmmm... Should I check how Linux
> and others cope with that problem?

Ah, here you mention it actually. Yet you don't explain what conflict
potential you see once using EfiRuntimeServicesData for the allocation.

Jan


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

Re: [Xen-devel] [PATCH v5 01/10] vt-d: fix the IOMMU flush issue

[Xu, Quan]

On May 25, 2016 4:30 PM, Jan Beulich  wrote:
> >>> On 25.05.16 at 10:04,  wrote:
> > On May 23, 2016 11:43 PM, Jan Beulich  wrote:
> >> >>> On 23.05.16 at 17:22,  wrote:
> >> > On May 23, 2016 9:31 PM, Jan Beulich  wrote:
> >> >> >>> On 18.05.16 at 10:08,  wrote:
> >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c
> >> >> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> >> > @@ -557,14 +557,16 @@ static void iommu_flush_all(void)
> >> >> >  }
> >> >> >  }
> >> >> >
> >> >> > -static void __intel_iommu_iotlb_flush(struct domain *d,
> >> >> > unsigned long
> >> gfn,
> >> >> > -int dma_old_pte_present, unsigned int page_count)
> >> >> > +static int __intel_iommu_iotlb_flush(struct domain *d, unsigned
> >> >> > +long
> >> gfn,
> >> >> > + bool_t dma_old_pte_present,
> >> >> > + unsigned int page_count)
> >> >>
> >> >> I realize you say so in the overview mail, but the continuing lack
> >> >> of __must_check here causes review trouble again. And I have a
> >> >> hard time seeing how adding these annotations right away would
> >> >> "disrupt the order", as long as the series is properly ordered / broken 
> >> >> up.
> >> >>
> >> >
> >> > If I add __must_check annotations here right now, e.g.
> >> >
> >> > -static void intel_iommu_iotlb_flush()
> >> > +static int __must_check iommu_flush_iotlb_pages()
> >> >
> >> > ...
> >> >
> >> > @@ -179,8 +179,9 @@ struct iommu_ops {
> >> > -void (*iotlb_flush)(struct domain *d, unsigned long gfn, unsigned 
> >> > int
> >> > page_count);
> >> > +int __must_check (*iotlb_flush)(struct domain *d, unsigned
> >> > + long gfn,
> >> > unsigned int page_count);
> >> > ...
> >> > }
> >> >
> >> > Should belong  to here too.
> >>
> >> Correct. And where's the problem?
> >>
> >
> > IMO it is not a big deal..
> >
> > I think this makes this patch 1 fat.. why not focus on the positive
> > propagation value from IOMMU flush interfaces in this patch.
> > If we are clear I will add annotation and rename them in another
> > patches, it is acceptable to me.
> 
> The patch getting too large is easy to deal with: Split it at a reasonable
> boundary. It's one thing that I want to be clear: Any conversion of a void
> return type of some function to non-void should be accompanied by it at the
> same time becoming __must_check. I dislike having to repeat yet again what I
> have been saying a number of times: Without doing so, it is harder for you as
> the person writing the patch to verify all callers deal with errors, and it's
> perhaps even harder for reviewers to verify you did.
> 

It is clear to me, but I was lock of attention on reviewers part.
I'll follow your suggestion from now on.. thanks.

Quan


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

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

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-ovmf-amd64 17 guest-start/debianhvm.repeat fail REGR. 
vs. 94748

version targeted for testing:
 ovmf e4979beee9e5d334d97fd8e2c79670ad08587bc6
baseline version:
 ovmf dc99315b8732b6e3032d01319d3f534d440b43d0

Last test of basis94748  2016-05-24 22:43:25 Z0 days
Failing since 94750  2016-05-25 03:43:08 Z0 days2 attempts
Testing same since94753  2016-05-25 05:48:13 Z0 days1 attempts


People who touched revisions under test:
  Hao Wu 
  Marvin H?user 
  Marvin Haeuser 
  Yonghong Zhu 

jobs:
 build-amd64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-i386   pass
 build-amd64-libvirt  pass
 build-i386-libvirt   pass
 build-amd64-pvopspass
 build-i386-pvops pass
 test-amd64-amd64-xl-qemuu-ovmf-amd64 pass
 test-amd64-i386-xl-qemuu-ovmf-amd64  fail



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


Not pushing.


commit e4979beee9e5d334d97fd8e2c79670ad08587bc6
Author: Yonghong Zhu 
Date:   Fri May 6 15:20:23 2016 +0800

BaseTools/GenFds: enhance to get TOOL_CHAIN_TAG and TARGET value

when user don't set TOOL_CHAIN_TAG and TARGET by –D Flag, then GenFds
would report failure for format:
FILE DATA = $(OUTPUT_DIRECTORY)/$(TARGET)_$(TOOL_CHAIN_TAG)/testfile
so this patch enhance to get the TOOL_CHAIN_TAG and TARGET value by
following priority (high to low): 1. the Macro value set by -D Flag;
2. Get the value by the -t/-b option. 3. get the value from target.txt
file. Besides, this patch also remove the error checking for missing
-t/-b option.

Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Yonghong Zhu 
Reviewed-by: Liming Gao 

commit 07fb9c264400d7ca2c14d3d8076102584038eb96
Author: Hao Wu 
Date:   Mon May 23 11:40:30 2016 +0800

MdeModulePkg RamDiskDxe: VALID_ARCH cleanup to list supported options

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu 
Reviewed-by: Feng Tian 

commit bd3fc8133b2b17ad2e0427d1bf6b44b08cf2f3b2
Author: Marvin H?user 
Date:   Fri May 20 03:04:02 2016 +0800

ShellPkg/App: Fix memory leak and save resources.

1) RunSplitCommand() allocates the initial SplitStdOut via
   CreateFileInterfaceMem(). Free SplitStdIn after the swap to fix
   the memory leak.

2) In RunSplitCommand(), SplitStdOut is checked for equality with
   StdIn. This cannot happen due to the if-check within the swap.
   Hence remove it.

3) UefiMain() doesn't free SplitList. Delete all list entries and
   reinitialize the list when in DEBUG. This does not include the
   CreateFileInterfaceMem()-allocated SplitStd mentioned in 1), so
   keep the ASSERT() until resolved.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Marvin Haeuser 
Reviewed-by: Qiu Shumin 

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Mon, 23 May 2016, Julien Grall wrote:
> Note, that XENMAPSPACE_dev_mmio has been introduced in Xen 4.7 (which is due
> in a couple of weeks) and part of the stable ABI. So if it is not possible to
> relax the memory attribute, it might be worth to think fixing/reverting the
> hypercall for 4.7. Otherwise we would have to introduce a new one in the next
> release.

FYI the Linux side missed the Linux 4.7 merge window and it is now
queued for 4.8. Therefore theoretically could still be changed, but I
would be careful with any doing major changes at this point.

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

Re: [Xen-devel] [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> @@ -100,19 +107,29 @@ multiboot2_header_end:
>  gdt_boot_descr:
>  .word   6*8-1
>  .long   sym_phys(trampoline_gdt)
> +.long   0 /* Needed for 64-bit lgdt */
> +
> +cs32_switch_addr:
> +.long   sym_phys(cs32_switch)
> +.word   BOOT_CS32
>  
>  .Lbad_cpu_msg: .asciz "ERR: Not a 64-bit CPU!"
>  .Lbad_ldr_msg: .asciz "ERR: Not a Multiboot bootloader!"
> +.Lbad_ldr_mb2: .asciz "ERR: On EFI platform use latest Multiboot2 compatible 
> bootloader!"

What is "latest" going to mean 5 years from now?

>  .section .init.text, "ax", @progbits
>  
>  bad_cpu:
>  mov $(sym_phys(.Lbad_cpu_msg)),%esi # Error message
> -jmp print_err
> +mov $0xB8000,%edi   # VGA framebuffer
> +jmp 1f
>  not_multiboot:
>  mov $(sym_phys(.Lbad_ldr_msg)),%esi # Error message
> -print_err:
> -mov $0xB8000,%edi  # VGA framebuffer
> +mov $0xB8000,%edi   # VGA framebuffer
> +jmp 1f
> +mb2_too_old:
> +mov $(sym_phys(.Lbad_ldr_mb2)),%esi # Error message
> +xor %edi,%edi   # No VGA framebuffer

Leaving aside that "framebuffer" really is a bad term here (we're
talking of text mode output after all), limiting the output to serial
isn't going to be very helpful in the field, I'm afraid. Even more so
that there's no guarantee for a UART to be at port 0x3f8. That's
not much of a problem for the other two messages as people are
unlikely to try to boot Xen on an unsuitable system, but I view it
as quite possible for Xen to be tried to get booted with an
unsuitable grub2.

IOW - this needs a better solution, presumably using EFI boot
service output functions.

> @@ -130,6 +149,130 @@ print_err:
>  .Lhalt: hlt
>  jmp .Lhalt
>  
> +.code64
> +
> +__efi64_start:

As long as we have split files under boot/, I think I'd prefer 64-bit
code to only go into x86_64.S.

> +cld
> +
> +/* Check for Multiboot2 bootloader. */
> +cmp $MULTIBOOT2_BOOTLOADER_MAGIC,%eax
> +je  efi_multiboot2_proto
> +
> +/* Jump to not_multiboot after switching CPU to x86_32 mode. */
> +lea not_multiboot(%rip),%rdi
> +jmp x86_32_switch

What I've said above would also eliminate the need to switch to
32-bit mode just for emitting an error message and halting the
system.

> +efi_multiboot2_proto:

.Lefi_multiboot2_proto

> +/*
> + * Multiboot2 information address is 32-bit,
> + * so, zero higher half of %rbx.
> + */
> +mov %ebx,%ebx

Wait, no - that's a protocol bug then. We're being entered in 64-bit
mode here, so registers should be in 64-bit clean state.

> +/* Skip Multiboot2 information fixed part. */
> +lea (MB2_fixed_sizeof+MULTIBOOT2_TAG_ALIGN-1)(%rbx),%rcx
> +and $~(MULTIBOOT2_TAG_ALIGN-1),%rcx

Or if there really was a reason to do the above, and if there is a
reason not to assume this data is located below 4Gb, then
calculations like this could avoid the REX64 prefix by using %ecx.

> +0:
> +/* Get EFI SystemTable address from Multiboot2 information. */
> +cmpl$MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> +jne 1f
> +
> +mov MB2_efi64_st(%rcx),%rsi
> +
> +/* Do not clear BSS twice and do not go into real mode. */
> +movb$1,skip_realmode(%rip)

How is the setting of skip_realmode related to the clearing of BSS?
Oh, I've found the connection below (albeit see there for its
validity), but I think mentioning this here is more confusing than
clarifying.

> +jmp 3f
> +
> +1:
> +/* Get EFI ImageHandle address from Multiboot2 information. */
> +cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> +jne 2f
> +
> +mov MB2_efi64_ih(%rcx),%rdi
> +jmp 3f
> +
> +2:
> +/* Is it the end of Multiboot2 information? */
> +cmpl$MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
> +je  run_bs
> +
> +3:
> +/* Go to next Multiboot2 information tag. */
> +add MB2_tag_size(%rcx),%ecx
> +add $(MULTIBOOT2_TAG_ALIGN-1),%rcx
> +and $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
> +jmp 0b

At least down to here it looks like this would better live in a C helper
function. Did you consider this?

> +run_bs:
> +push%rax
> +push%rdi
> +
> +/*
> + * Initialize BSS (no nasty surprises!).
> + * It must be done earlier than in BIOS case
> + * because efi_multiboot2() touches it.
> + */
> +lea __bss_start(%rip),%rdi
> +lea __bss_end(%rip),%rcx
> +sub %rdi,%rcx
> +shr $3,%rcx
> +xor %eax,%eax
> +rep stosq

Please let's not repeat pre-existing mistakes: REP is not an
instructi

Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[Andrew Cooper]

On 24/05/16 23:48, Andy Lutomirski wrote:
> In aa1acff356bb ("x86/xen: Probe target addresses in
> set_aliased_prot() before the hypercall"), I added an explicit probe
> to work around a hypercall issue.  The code can be simplified by
> using probe_kernel_read.
>
> Cc: Andrew Cooper 
> Cc: Boris Ostrovsky 
> Cc: David Vrabel 
> Cc: Jan Beulich 
> Cc: Konrad Rzeszutek Wilk 
> Cc: xen-devel 
> Signed-off-by: Andy Lutomirski 

Reviewed-by: Andrew Cooper 

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Tue, 24 May 2016, Julien Grall wrote:
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> > On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> > > On 23/05/16 15:02, Edgar E. Iglesias wrote:
> > > > On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> > > > > (CC Wei Liu)
> > > > > 
> > > > > On 23/05/16 12:56, Edgar E. Iglesias wrote:
> > > > > > On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> > > > > > > On 20/05/16 16:51, Edgar E. Iglesias wrote:
> > > > > > > > From: "Edgar E. Iglesias" 
> > > > > > > > 
> > > > > > > > This series adds support for mapping mmio-sram nodes into dom0
> > > > > > > > as MEMORY, cached and with RWX perms.
> > > > > > > 
> > > > > > > Can you explain why you chose to map those nodes as MEMORY, cached
> > > > > > > and with
> > > > > > > RWX perms?
> > > > > > 
> > > > > > My understanding is that these mmio-sram nodes are allowed to be
> > > > > > treated as
> > > > > > Normal memory by the guest OS.
> > > > > > Guests could potentially do any kind of memory like operations on
> > > > > > them.
> > > > > > 
> > > > > > In our specific case, dom0 won't execute code from these regions but
> > > > > > Linux/dom0 ends up using standard memcpy/memset/x functions (not
> > > > > > memcpy_fromio and friends) on the regions.
> > > > > 
> > > > > I looked at the generic sram driver in Linux (drivers/misc/sram.c)
> > > > > which
> > > > > actually use memcpy_{to,from}io. So how you driver differs from the
> > > > > generic
> > > > > one? What the SRAM will contain?
> > > > 
> > > > We intend to use that same driver to map the memory but mmio-sram
> > > > nodes allow you to assign the regions to other device-drivers.
> > > > 
> > > > Some examples:
> > > > Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> > > > arch/arm/boot/dts/orion5x.dtsi
> > > > drivers/crypto/mv_cesa.c
> > > > 
> > > > The cover letter for the sram driver had an example aswell allthough
> > > > the function names have changed since (it's of_gen_pool_get now):
> > > > https://lwn.net/Articles/537728/
> > > > 
> > > > Nothing explicitely says that the regions can be assumed to be mapped
> > > > as Normal memory, but on Linux they do get mapped as Mormal WC mem
> > > > (unless the no-memory-wc prop is set on the node).
> > > > The marvell-cesa example also uses plain memset on the sram.
> > > 
> > > I am a bit confused with this example. From my understanding of
> > > mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area
> > > (see
> > > gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> > > 
> > > However, memcpy_{from,to}io should be used when dealing with MMIO (the
> > > field
> > > sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> > > related to marvell/cesa.
> > 
> > 
> > Yeah, I'm started to get confused too. Maybe they just forgot the memset
> > in drivers/crypto/mv_cesa.c.
> > 
> > There are other examples though, that don't do fromio/toio at all.
> > Documentation/devicetree/bindings/media/coda.txt
> > drivers/media/platform/coda/coda-common.c
> > 
> > Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> > mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.

I agree and it has always been the intention.


> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.
> 
> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory attribute
> to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

That would work for device tree, but we still need to rely on the
hypercall for ACPI systems.

Given that it is not easy to add an additional parameter to
XENMEM_add_to_physmap_range, I think we'll have to provide a new
hypercall to allow setting attributes other than the Xen default. That
could be done in Xen 4.8 and Linux >= 4.9.

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

[Xen-devel] [qemu-upstream-4.3-testing test] 94754: trouble: blocked/broken

[osstest service owner]

flight 94754 qemu-upstream-4.3-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/94754/

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. 80927
 build-amd64   3 host-install(3) broken REGR. vs. 80927
 build-amd64-pvops 3 host-install(3) broken REGR. vs. 80927
 build-i3863 host-install(3) broken REGR. vs. 80927

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked  n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-pv1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pygrub   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl   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-pv   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pair 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

version targeted for testing:
 qemuuc97c20f71240a538a19cb6b0e598bc1bbd5168f1
baseline version:
 qemuu10c1b763c26feb645627a1639e722515f3e1e876

Last test of basis80927  2016-02-06 13:30:02 Z  108 days
Testing same since93977  2016-05-10 11:09:16 Z   14 days   54 attempts


People who touched revisions under test:
  Gerd Hoffmann 
  Stefano Stabellini 

jobs:
 build-amd64  broken  
 build-i386   broken  
 build-amd64-libvirt  blocked 
 build-i386-libvirt   blocked 
 build-amd64-pvopsbroken  
 build-i386-pvops broken  
 test-amd64-amd64-xl  blocked 
 test-amd64-i386-xl   blocked 
 test-amd64-i386-qemuu-rhel6hvm-amd   blocked 
 test-amd64-amd64-xl-qemuu-debianhvm-amd64blocked 
 test-amd64-i386-xl-qemuu-debianhvm-amd64 blocked 
 test-amd64-i386-freebsd10-amd64  blocked 
 test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked 
 test-amd64-i386-xl-qemuu-ovmf-amd64  blocked 
 test-amd64-amd64-xl-qemuu-win7-amd64 blocked 
 test-amd64-i386-xl-qemuu-win7-amd64  blocked 
 test-amd64-amd64-xl-credit2  blocked 
 test-amd64-i386-freebsd10-i386   blocked 
 test-amd64-i386-qemuu-rhel6hvm-intel blocked 
 test-amd64-amd64-libvirt blocked 
 test-amd64-i386-libvirt  blocked 
 test-amd64-amd64-xl-multivcpublocked 
 test-amd64-amd64-pairblo

Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[David Vrabel]

On 24/05/16 23:48, Andy Lutomirski wrote:
> In aa1acff356bb ("x86/xen: Probe target addresses in
> set_aliased_prot() before the hypercall"), I added an explicit probe
> to work around a hypercall issue.  The code can be simplified by
> using probe_kernel_read.

Acked-by: David Vrabel 

David

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Stefano,

On 25/05/16 10:43, Stefano Stabellini wrote:

For SRAM it would be normal memory uncached (?) when the property
"no-memory-wc" is not present, else TBD.

I suspect we would have to relax more MMIOs in the future. Rather than
providing a function to map, the code is very similar except the memory
attribute, I suggest to provide a list of compatible with the memory attribute
to use.

All the children node would inherit the memory attribute of the parent.

What do you think?


That would work for device tree, but we still need to rely on the
hypercall for ACPI systems.

Given that it is not easy to add an additional parameter to
XENMEM_add_to_physmap_range, I think we'll have to provide a new
hypercall to allow setting attributes other than the Xen default. That
could be done in Xen 4.8 and Linux >= 4.9.


There is no need to introduce a new hypercall. The 
XENMEM_add_to_physmap_batch contains an unused field ('foreign_id', to 
be renamed) for mapping device MMIOs (see Jan's mail [1]).


XENMEM_add_to_physmap will always map with the default memory attribute 
(Device_nGnRnE) and if the kernel want to use another memory attribute, 
it will have to use XENMEM_add_to_physmap_batch.


With the plan suggested in [2], there are no modifications required in 
Linux for the moment.


Regards,

[1] 
http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02341.html
[2] 
http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02347.html


--
Julien Grall

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Stefano Stabellini]

On Wed, 25 May 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/16 10:43, Stefano Stabellini wrote:
> > > For SRAM it would be normal memory uncached (?) when the property
> > > "no-memory-wc" is not present, else TBD.
> > > 
> > > I suspect we would have to relax more MMIOs in the future. Rather than
> > > providing a function to map, the code is very similar except the memory
> > > attribute, I suggest to provide a list of compatible with the memory
> > > attribute
> > > to use.
> > > 
> > > All the children node would inherit the memory attribute of the parent.
> > > 
> > > What do you think?
> > 
> > That would work for device tree, but we still need to rely on the
> > hypercall for ACPI systems.
> > 
> > Given that it is not easy to add an additional parameter to
> > XENMEM_add_to_physmap_range, I think we'll have to provide a new
> > hypercall to allow setting attributes other than the Xen default. That
> > could be done in Xen 4.8 and Linux >= 4.9.
> 
> There is no need to introduce a new hypercall. The XENMEM_add_to_physmap_batch
> contains an unused field ('foreign_id', to be renamed) for mapping device
> MMIOs (see Jan's mail [1]).
> 
> XENMEM_add_to_physmap will always map with the default memory attribute
> (Device_nGnRnE) and if the kernel want to use another memory attribute, it
> will have to use XENMEM_add_to_physmap_batch.
> 
> With the plan suggested in [2], there are no modifications required in Linux
> for the moment.
> 
> Regards,
> 
> [1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02341.html
> [2] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02347.html

I read the separate thread. Sounds good.

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

Re: [Xen-devel] [xen-unstable bisection] complete test-amd64-amd64-xl-qemuu-ovmf-amd64

[Wei Liu]

On Tue, May 24, 2016 at 06:50:23PM +0100, Wei Liu wrote:
> On Sun, May 22, 2016 at 05:37:51AM +, osstest service owner wrote:
> > branch xen-unstable
> > xenbranch xen-unstable
> > job test-amd64-amd64-xl-qemuu-ovmf-amd64
> > testid guest-start/debianhvm.repeat
> > 
> > Tree: linux git://xenbits.xen.org/linux-pvops.git
> > Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> > Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
> > Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> > Tree: xen git://xenbits.xen.org/xen.git
> > 
> > *** Found and reproduced problem changeset ***
> > 
> >   Bug is in tree:  xen git://xenbits.xen.org/xen.git
> >   Bug introduced:  1542efcea893df874b13b1ea78101e1ff6a55c41
> >   Bug not present: c32381352cce9744e640bf239d2704dae4af4fc7
> >   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/94689/
> > 
> > 
> >   commit 1542efcea893df874b13b1ea78101e1ff6a55c41
> >   Author: Wei Liu 
> >   Date:   Wed May 18 11:48:25 2016 +0100
> >   
> >   Config.mk: update ovmf changeset
> >   
> >   Signed-off-by: Wei Liu 
> > 
> 
> I did some tests and analysis today.
> 
> * Manual tests
> 
> Seconds between starting a guest to receiving ping, test three times
> 
>   xl create guest.cfg ;\
>   s=`date +%s`; date --date="@$s"; \
>   while true;  do \
>ping -c 1  -q -W 1 172.16.147.190 2>&1 1>/dev/null;\
>if [ $? = 0 ]; then break; fi ;\
>done;\
>   e=`date +%s`; date --date="@$e";\
>   expr $e - $s
> 
>   merlot0 tg06
> old ovmf, 5000mb ram 98 99 9633 32 31
> old ovmf, 768mb  ram 97 100 100  31 31 32
> new ovmf, 5000mb ram 158 158 157 25 26 25
> new ovmf, 768mb  ram 151 156 160 26 25 25
> 
> Old ovmf refers to 52a99493 (currently in master)
> New ovmf refers to b41ef325 (the fingered one)
> 
> Tg06 and merlot0 have the same changeset git:983aae0.
> 
> Note that the guest runs on tg06 has a different version of Debian, so it is
> not really comparable to the guest on merlot0.  Also note that we can't
> extrapolate from my manual test that osstest will or will not see timeout on
> merlot0 because the technique to test that is not the same.
> 
> The conclusions are: we now know the results are consistent and the guest
> memory size doesn't affect the time taken to start the guest.
> 
> * Osstest report
> 
> Pick the ovmf flight that tested the fingered changeset:
> 
> http://logs.test-lab.xenproject.org/osstest/logs/94519/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> 
> 2016-05-18 05:40:26 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> link/ip/tcp: ok. (185s) 
> 2016-05-18 05:44:13 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> link/ip/tcp: ok. (184s) 
> 2016-05-18 05:47:55 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> link/ip/tcp: ok. (184s) 
> ...
> 
> The time out for checking if a guest is up is 200 seconds so 180 seconds 
> should
> be fine.
> 
> The new ovmf failure reported by bisector is the controller timed out when
> trying to check if the guest is up.
> 
> 
> http://logs.test-lab.xenproject.org/osstest/logs/94689/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> 
> The old ovmf passed on merlot0:
> 
> 
> http://logs.test-lab.xenproject.org/osstest/logs/94680/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> 2016-05-22 02:49:57 Z guest debianhvm.guest.osstest 5a:36:0e:d8:00:01 22 
> link/ip/tcp: ok. (141s) 
> 
> The old ovmf passed on other machine:
> 
> 
> http://logs.test-lab.xenproject.org/osstest/logs/94580/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> 
> 2016-05-19 22:45:39 Z guest debianhvm.guest.osstest 5a:36:0e:74:00:3c 22 
> link/ip/tcp: ok. (122s) 
> 
> The two numbers suggest that merlot is slower than the other machine.
> 
> Pick one of the recent test report for OVMF:
> 
> http://logs.test-lab.xenproject.org/osstest/logs/94739/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> 
> The same metric (guest creation to guest responding to network traffic) is a
> lot shorter (on a non-merlot machine):
> 
> 2016-05-24 14:21:59 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> link/ip/tcp: ok. (49s) 
> 2016-05-24 14:23:28 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> link/ip/tcp: ok. (49s) 
> 2016-05-24 14:25:03 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> link/ip/tcp: ok. (48s) 
> 2016-05-24 14:26:45 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> link/ip/tcp: ok. (48s) 
> ...
> 
> It looks like it's getting better.
> 
> I don't have a conclusion on this issue because I can't eliminate all
> variables.  I'm inclined to push another a newer ovmf changeset to see what
> happens, because:
> 
> 1. merlot is slower than other machine, the time difference is about 20s.
> 2. new ovmf on other machine already takes ~180s to come up (less than 20s to
>200s timeout).
> 3. the time taken to co

Re: [Xen-devel] [xen-unstable test] 94746: regressions - FAIL

[Wei Liu]

On Wed, May 25, 2016 at 06:28:43AM +, osstest service owner wrote:
> flight 94746 xen-unstable real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/94746/
> 
> Regressions :-(
> 
> Tests which did not succeed and are blocking,
> including tests which could not be run:
>  test-armhf-armhf-libvirt-qcow2  5 xen-install fail REGR. vs. 
> 94740
>  test-armhf-armhf-xl-vhd   5 xen-install   fail REGR. vs. 
> 94740
> 

This is likely to be intermittent.

2016-05-24 23:46:27 Z executing scp ...  
/home/logs/logs/94746/build-armhf/build/dist.tar.gz 
root@172.16.144.44:/root/extract_dist.tar.gz 
Write failed: Broken pipe
lost connection

E: Failed to fetch 
http://ftp.debian.org/debian/pool/main/l/llvm-toolchain-3.5/libllvm3.5_3.5-10_armhf.deb
Could not connect to cache:3143 (172.16.148.6). - connect (113: No route to 
host)

Wei.

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

Re: [Xen-devel] [xen-unstable bisection] complete test-amd64-amd64-xl-qemuu-ovmf-amd64

[Anthony PERARD]

On Wed, May 25, 2016 at 11:10:21AM +0100, Wei Liu wrote:
> On Tue, May 24, 2016 at 06:50:23PM +0100, Wei Liu wrote:
> > On Sun, May 22, 2016 at 05:37:51AM +, osstest service owner wrote:
> > > branch xen-unstable
> > > xenbranch xen-unstable
> > > job test-amd64-amd64-xl-qemuu-ovmf-amd64
> > > testid guest-start/debianhvm.repeat
> > > 
> > > Tree: linux git://xenbits.xen.org/linux-pvops.git
> > > Tree: linuxfirmware git://xenbits.xen.org/osstest/linux-firmware.git
> > > Tree: qemu git://xenbits.xen.org/qemu-xen-traditional.git
> > > Tree: qemuu git://xenbits.xen.org/qemu-xen.git
> > > Tree: xen git://xenbits.xen.org/xen.git
> > > 
> > > *** Found and reproduced problem changeset ***
> > > 
> > >   Bug is in tree:  xen git://xenbits.xen.org/xen.git
> > >   Bug introduced:  1542efcea893df874b13b1ea78101e1ff6a55c41
> > >   Bug not present: c32381352cce9744e640bf239d2704dae4af4fc7
> > >   Last fail repro: http://logs.test-lab.xenproject.org/osstest/logs/94689/
> > > 
> > > 
> > >   commit 1542efcea893df874b13b1ea78101e1ff6a55c41
> > >   Author: Wei Liu 
> > >   Date:   Wed May 18 11:48:25 2016 +0100
> > >   
> > >   Config.mk: update ovmf changeset
> > >   
> > >   Signed-off-by: Wei Liu 
> > > 
> > 
> > I did some tests and analysis today.
> > 
> > * Manual tests
> > 
> > Seconds between starting a guest to receiving ping, test three times
> > 
> >   xl create guest.cfg ;\
> >   s=`date +%s`; date --date="@$s"; \
> >   while true;  do \
> >ping -c 1  -q -W 1 172.16.147.190 2>&1 1>/dev/null;\
> >if [ $? = 0 ]; then break; fi ;\
> >done;\
> >   e=`date +%s`; date --date="@$e";\
> >   expr $e - $s
> > 
> >   merlot0 tg06
> > old ovmf, 5000mb ram 98 99 9633 32 31
> > old ovmf, 768mb  ram 97 100 100  31 31 32
> > new ovmf, 5000mb ram 158 158 157 25 26 25
> > new ovmf, 768mb  ram 151 156 160 26 25 25
> > 
> > Old ovmf refers to 52a99493 (currently in master)
> > New ovmf refers to b41ef325 (the fingered one)
> > 
> > Tg06 and merlot0 have the same changeset git:983aae0.
> > 
> > Note that the guest runs on tg06 has a different version of Debian, so it is
> > not really comparable to the guest on merlot0.  Also note that we can't
> > extrapolate from my manual test that osstest will or will not see timeout on
> > merlot0 because the technique to test that is not the same.
> > 
> > The conclusions are: we now know the results are consistent and the guest
> > memory size doesn't affect the time taken to start the guest.
> > 
> > * Osstest report
> > 
> > Pick the ovmf flight that tested the fingered changeset:
> > 
> > http://logs.test-lab.xenproject.org/osstest/logs/94519/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> > 
> > 2016-05-18 05:40:26 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> > link/ip/tcp: ok. (185s) 
> > 2016-05-18 05:44:13 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> > link/ip/tcp: ok. (184s) 
> > 2016-05-18 05:47:55 Z guest debianhvm.guest.osstest 5a:36:0e:37:00:01 22 
> > link/ip/tcp: ok. (184s) 
> > ...
> > 
> > The time out for checking if a guest is up is 200 seconds so 180 seconds 
> > should
> > be fine.
> > 
> > The new ovmf failure reported by bisector is the controller timed out when
> > trying to check if the guest is up.
> > 
> > 
> > http://logs.test-lab.xenproject.org/osstest/logs/94689/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> > 
> > The old ovmf passed on merlot0:
> > 
> > 
> > http://logs.test-lab.xenproject.org/osstest/logs/94680/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> > 2016-05-22 02:49:57 Z guest debianhvm.guest.osstest 5a:36:0e:d8:00:01 
> > 22 link/ip/tcp: ok. (141s) 
> > 
> > The old ovmf passed on other machine:
> > 
> > 
> > http://logs.test-lab.xenproject.org/osstest/logs/94580/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> > 
> > 2016-05-19 22:45:39 Z guest debianhvm.guest.osstest 5a:36:0e:74:00:3c 
> > 22 link/ip/tcp: ok. (122s) 
> > 
> > The two numbers suggest that merlot is slower than the other machine.
> > 
> > Pick one of the recent test report for OVMF:
> > 
> > http://logs.test-lab.xenproject.org/osstest/logs/94739/test-amd64-amd64-xl-qemuu-ovmf-amd64/17.ts-repeat-test.log
> > 
> > The same metric (guest creation to guest responding to network traffic) is a
> > lot shorter (on a non-merlot machine):
> > 
> > 2016-05-24 14:21:59 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> > link/ip/tcp: ok. (49s) 
> > 2016-05-24 14:23:28 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> > link/ip/tcp: ok. (49s) 
> > 2016-05-24 14:25:03 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> > link/ip/tcp: ok. (48s) 
> > 2016-05-24 14:26:45 Z guest debianhvm.guest.osstest 5a:36:0e:13:00:02 22 
> > link/ip/tcp: ok. (48s) 
> > ...
> > 
> > It looks like it's getting better.
> > 
> > I don't have a conclusion on this issue because I can't eliminate

Re: [Xen-devel] [PATCH v3 13/16 - RFC] x86: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 25.05.16 at 11:32,  wrote:
 On 15.04.16 at 14:33,  wrote:
>> @@ -221,6 +372,13 @@ trampoline_setup:
>>  add $12,%esp/* Remove reloc() args from stack. */
>>  mov %eax,sym_phys(multiboot_ptr)
>>  
>> +/*
>> + * Do not zero BSS on EFI platform here.
>> + * It was initialized earlier.
>> + */
>> +cmpb$1,sym_phys(skip_realmode)
>> +je  1f
> 
> So what if skip_realmode is set on a legacy BIOS system because
> of the command line option or the use of TBOOT?

Oh, I see - this is still before command line parsing ran. The
commentary on this double purpose should be improved, I
think.

Jan


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

Re: [Xen-devel] [PATCH v3 14/16] x86/boot: implement early command line parser in C

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> --- a/xen/arch/x86/boot/build32.lds
> +++ b/xen/arch/x86/boot/build32.lds
> @@ -25,6 +25,7 @@ SECTIONS
>  *(.text)
>  *(.text.*)
>  *(.rodata)
> +*(.rodata.*)
>}

Interesting - didn't you say you don't want this for now?

> --- /dev/null
> +++ b/xen/arch/x86/boot/cmdline.c
> @@ -0,0 +1,357 @@
> +/*
> + * Copyright (c) 2015, 2016 Oracle and/or its affiliates. All rights 
> reserved.
> + *  Daniel Kiper 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see .
> + *
> + * strlen(), strncmp(), strspn() and strcspn() were copied from
> + * Linux kernel source (linux/lib/string.c).

Any reason you can't just #include ".../common/string.c" here?

> +/*
> + * Space and TAB are obvious delimiters. However, I am
> + * adding "\n" and "\r" here too. Just in case when
> + * crazy bootloader/user put them somewhere.
> + */
> +#define DELIM_CHARS  " \n\r\t"
> +#define DELIM_CHARS_COMMADELIM_CHARS ","

static const char[] variables (or really just one, with the comma put
first and the non-comma variant indexing into that variable by 1)?

> +#define __packed __attribute__((__packed__))

No way to include compiler.h here?

> +/*
> + * Compiler is not able to optimize regular strlen()
> + * if argument is well known string during build.
> + * Hence, introduce optimized strlen_opt().
> + */
> +#define strlen_opt(s) (sizeof(s) - 1)

Do we really care in this code?

> +/* Keep in sync with trampoline.S:early_boot_opts label! */
> +typedef struct __packed {
> +u8 skip_realmode;
> +u8 opt_edd;
> +u8 opt_edid;
> +u16 boot_vid_mode;
> +u16 vesa_width;
> +u16 vesa_height;
> +u16 vesa_depth;
> +} early_boot_opts_t;

This "keeping in sync" should be automated in some way, e.g. via
a new header and suitable macroization.

> +static int strtoi(const char *s, const char *stop, const char **next)
> +{
> +int base = 10, i, ores = 0, res = 0;

You don't even handle a '-' on the numbers here, so all the variables
and the function return type should be unsigned int afaict. And the
function name perhaps be strtoui().

> +if ( *s == '0' )
> +  base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
> +
> +for ( ; *s != '\0'; ++s )
> +{
> +for ( i = 0; stop && stop[i] != '\0'; ++i )
> +if ( *s == stop[i] )
> +goto out;

strchr()?

> +if ( *s < '0' || (*s > '7' && base == 8) )
> +{
> +res = -1;
> +goto out;
> +}
> +
> +if ( *s > '9' && (base != 16 || tolower(*s) < 'a' || tolower(*s) > 
> 'f') )
> +{
> +res = -1;
> +goto out;
> +}
> +
> +res *= base;
> +res += (tolower(*s) >= 'a') ? (tolower(*s) - 'a' + 10) : (*s - '0');

With the four instances, how about latching tolower(*s) into a local
variable?

> +static u8 skip_realmode(const char *cmdline)
> +{
> +return !!find_opt(cmdline, "no-real-mode", 0) || !!find_opt(cmdline, 
> "tboot=", 1);

The || makes the two !! pointless.

Also please settle on which type you want to use for boolean
(find_opt()'s last parameter is "int", yet here you use "u8"), and
perhaps make yourself a bool_t.

> +static void vga_parse(const char *cmdline, early_boot_opts_t *ebo)
> +{
> +const char *c;
> +int tmp;
> +
> +c = find_opt(cmdline, "vga=", 1);
> +
> +if ( !c )
> +return;
> +
> +ebo->boot_vid_mode = ASK_VGA;
> +
> +if ( !strmaxcmp(c, "current", DELIM_CHARS_COMMA) )
> +ebo->boot_vid_mode = VIDEO_CURRENT_MODE;
> +else if ( !strsubcmp(c, "text-80x") )
> +{
> +c += strlen_opt("text-80x");
> +ebo->boot_vid_mode = rows2vmode(strtoi(c, DELIM_CHARS_COMMA, NULL));
> +}
> +else if ( !strsubcmp(c, "gfx-") )
> +{
> +tmp = strtoi(c + strlen_opt("gfx-"), "x", &c);
> +
> +if ( tmp < 0 || tmp > U16_MAX )
> +return;
> +
> +ebo->vesa_width = tmp;
> +
> +/*
> + * Increment c outside of strtoi() because otherwise some
> + * compiler may complain with following message:
> + * warning: operation on ‘c’ may be undefined.
> + */
> +++c;
> +tmp = strtoi(c, "x", &c);

The comment is pointless - the operation is firmly undefined if you
put it in the strtoi() invocation.

> +

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> Hi Edgar,

Hi Julien,

> 
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> >>On 23/05/16 15:02, Edgar E. Iglesias wrote:
> >>>On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> (CC Wei Liu)
> 
> On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>From: "Edgar E. Iglesias" 
> >>>
> >>>This series adds support for mapping mmio-sram nodes into dom0
> >>>as MEMORY, cached and with RWX perms.
> >>
> >>Can you explain why you chose to map those nodes as MEMORY, cached and 
> >>with
> >>RWX perms?
> >
> >My understanding is that these mmio-sram nodes are allowed to be treated 
> >as
> >Normal memory by the guest OS.
> >Guests could potentially do any kind of memory like operations on them.
> >
> >In our specific case, dom0 won't execute code from these regions but
> >Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >memcpy_fromio and friends) on the regions.
> 
> I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> actually use memcpy_{to,from}io. So how you driver differs from the 
> generic
> one? What the SRAM will contain?
> >>>
> >>>We intend to use that same driver to map the memory but mmio-sram
> >>>nodes allow you to assign the regions to other device-drivers.
> >>>
> >>>Some examples:
> >>>Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> >>>arch/arm/boot/dts/orion5x.dtsi
> >>>drivers/crypto/mv_cesa.c
> >>>
> >>>The cover letter for the sram driver had an example aswell allthough
> >>>the function names have changed since (it's of_gen_pool_get now):
> >>>https://lwn.net/Articles/537728/
> >>>
> >>>Nothing explicitely says that the regions can be assumed to be mapped
> >>>as Normal memory, but on Linux they do get mapped as Mormal WC mem
> >>>(unless the no-memory-wc prop is set on the node).
> >>>The marvell-cesa example also uses plain memset on the sram.
> >>
> >>I am a bit confused with this example. From my understanding of
> >>mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> >>gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> >>
> >>However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> >>sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> >>related to marvell/cesa.
> >
> >
> >Yeah, I'm started to get confused too. Maybe they just forgot the memset
> >in drivers/crypto/mv_cesa.c.
> >
> >There are other examples though, that don't do fromio/toio at all.
> >Documentation/devicetree/bindings/media/coda.txt
> >drivers/media/platform/coda/coda-common.c
> >
> >Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> >mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.

Yes, I agree with the principle.
The questionable part is what attributes are considered safe for specific
nodes.

> 
> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.

That sounds good, it would solve our problems.


> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory
> attribute to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

That sounds doable. I'll put something together addressing all your comments
and post new RFC.

Best regards,
Edgar

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

[Xen-devel] [PATCH OSSTEST] make-flight: don't create ovmf tests for seabios branch

[Wei Liu]

Signed-off-by: Wei Liu 
---
 make-flight | 1 +
 1 file changed, 1 insertion(+)

diff --git a/make-flight b/make-flight
index f324f6a..18e5bc3 100755
--- a/make-flight
+++ b/make-flight
@@ -122,6 +122,7 @@ job_create_test_filter_callback () {
   *) return 1;;
   esac
   case $job in
+  *-qemuu-ovmf-*) return 1;;
   *-qemuu-*) ;;
   *) return 1;;
   esac
-- 
2.1.4


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

Re: [Xen-devel] [PATCH v3 15/16 - RFC] x86: make Xen early boot code relocatable

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> Every multiboot protocol (regardless of version) compatible image must
> specify its load address (in ELF or multiboot header). Multiboot protocol
> compatible loader have to load image at specified address. However, there
> is no guarantee that the requested memory region (in case of Xen it starts
> at 1 MiB and ends at 17 MiB) where image should be loaded initially is a RAM
> and it is free (legacy BIOS platforms are merciful for Xen but I found at
> least one EFI platform on which Xen load address conflicts with EFI boot
> services; it is Dell PowerEdge R820 with latest firmware). To cope with
> that problem we must make Xen early boot code relocatable. This patch does
> that.

I don't follow: If we have to specify a load address, and if the
loader is required to put us there or fail, how does the code being
made relocatable help? And how does moving ourselves from 1Mb
to 2Mb make it any less likely that the designated address range is
actually free? Perhaps it's just the description that's misleading
here...

Even with that resolved I expect - as a result of the discussion on
the hackathon - quite a bit of change to this patch, so I don't view
actually reviewing the code as usefully spent time.

Jan


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

Re: [Xen-devel] Question about the best practice to install two versions of Xen toolstack on the same machine

[Wei Liu]

On Tue, May 24, 2016 at 04:47:38PM -0400, Meng Xu wrote:
> Hi Olaf,
> 
> Thank you very much for your suggestion!
> 
> On Fri, May 20, 2016 at 4:52 AM, Olaf Hering  wrote:
> > On Thu, May 19, Meng Xu wrote:
> >
> >> Does anyone try to install two version of Xen toolstack on the same 
> >> machine?
> >
> > I do that. See the INSTALL file which has examples at the end:
> >
> > * To build a private copy of tools and xen:
> > configure --prefix=/odd/path --sysconfdir=/odd/path/etc --enable-rpath
> > make
> > sudo make install BOOT_DIR=/ood/path/boot EFI_DIR=/odd/path/efi
> >
> 
> I'm wondering if  BOOT_DIR=/ood/path/boot and EFI_DIR=/odd/path/efi is a must?
> I'm using the grub2 to boot the kernel (so that I can remotely control
> which grub entry I will get into).
> 
> I put the xen image to /boot and the system can boot up. The xl
> toolstack seems working well.
> 
> However, the dom0's name is null.
> 
> When I run /ood/path/etc/init.d/xencommons restart , it reports the
> following message:
> 
> Stopping xenconsoled
> Stopping QEMU
> WARNING: Not stopping xenstored, as it cannot be restarted.
> Starting xenconsoled...
> Starting QEMU as disk backend for dom0
> 
> I guess there is something wrong with the xenstored's configuration?
> Did you happen to experience this issue before?
> 
> 

I think xenstored is running fine.

If you're using systemd, there should be a service called xen-init-dom0
that needs to be started. That small program does a bunch of things
including setting the name for Dom0 in xenstore.

Wei.

> Thanks and Best Regards,
> 
> Meng
> ---
> Meng Xu
> PhD Student in Computer and Information Science
> University of Pennsylvania
> http://www.cis.upenn.edu/~mengxu/
> 
> ___
> 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

[Xen-devel] [xen-unstable-coverity test] 94759: all pass - PUSHED

[osstest service owner]

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

Perfect :-)
All tests in this flight passed
version targeted for testing:
 xen  4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51
baseline version:
 xen  ed2cddb66b40cacdefd2e7920bde578d8da67fa9

Last test of basis94700  2016-05-22 09:19:04 Z3 days
Testing same since94759  2016-05-25 09:18:41 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Anthony PERARD 
  Dario Faggioli 
  Ian Jackson 
  Jan Beulich 
  Wei Liu 

jobs:
 coverity-amd64   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-coverity
+ revision=4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51
+ . ./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-coverity 4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51
+ branch=xen-unstable-coverity
+ revision=4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51
+ . ./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-coverity
+ qemuubranch=qemu-upstream-unstable-coverity
+ qemuubranch=qemu-upstream-unstable
+ '[' xxen = xlinux ']'
+ linuxbranch=
+ '[' xqemu-upstream-unstable = x ']'
+ select_prevxenbranch
++ ./cri-getprevxenbranch xen-unstable-coverity
+ prevxenbranch=xen-4.6-testing
+ '[' x4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51 = 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 $c{"GitCacheProxy"} or die $!;
'
+++ local cache=git://cache:9419/
+++ '[' xgit://cache:9419/ '!=' x ']'
+++ echo 
'git://cache:9419/https://github.com/rumpkernel/rumpkernel-netbsd-src%20[fetch=try]'
++ : 
'git://cache:9419/https://github.com/rumpkernel/rumpkernel-netbsd-src%20[fetch=try]'
++ : git
++ : git://git.seabios.org/seabios.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/seabios.git
++ : git://xenbits.xen.org/osstest/seabios.git
++ : https://github.com/tianocore/edk2.git
++ : osst...@xenbits.xen.org:/home/xen/git/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/ovmf.git
++ : git://xenbits.xen.org/osstest/linux-firmw

Re: [Xen-devel] [PATCH v3 16/16] x86: add multiboot2 protocol support for relocatable images

[Jan Beulich]

>>> On 15.04.16 at 14:33,  wrote:
> Add multiboot2 protocol support for relocatable images. Only GRUB2 with
> "multiboot2: Add support for relocatable images" patch understands
> that feature. Older multiboot protocol (regardless of version)
> compatible loaders ignore it and everything works as usual.

So with that I'm now sure that the previous patch is in need of a
better description.

> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -79,6 +79,13 @@ multiboot2_header_start:
>  /* Align modules at page boundry. */
>  mb2ht_init MB2_HT(MODULE_ALIGN), MB2_HT(REQUIRED)
>  
> +/* Load address preference. */
> +mb2ht_init MB2_HT(RELOCATABLE), MB2_HT(OPTIONAL), \
> +   sym_offset(start), /* Min load address. */ \
> +   0x, /* Max load address (4 GiB - 1). */ \

Hardly - that would allow us to be loaded at 4G - 2M, no matter
how large the image. Or else the comment is misleading.

> @@ -178,30 +185,39 @@ efi_multiboot2_proto:
>  and $~(MULTIBOOT2_TAG_ALIGN-1),%rcx
>  
>  0:
> +/* Get Xen image load base address from Multiboot2 information. */
> +cmpl$MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%rcx)
> +jne 1f
> +
> +mov MB2_load_base_addr(%rcx),%r15d
> +sub $XEN_IMG_OFFSET,%r15
> +jmp 4f

Why do we need to read this from the table? Can't we easily calculate
this ourselves?

> +1:
>  /* Get EFI SystemTable address from Multiboot2 information. */
>  cmpl$MULTIBOOT2_TAG_TYPE_EFI64,MB2_tag_type(%rcx)
> -jne 1f
> +jne 2f
>  
>  mov MB2_efi64_st(%rcx),%rsi
>  
>  /* Do not clear BSS twice and do not go into real mode. */
>  movb$1,skip_realmode(%rip)
> -jmp 3f
> +jmp 4f
>  
> -1:
> +2:
>  /* Get EFI ImageHandle address from Multiboot2 information. */
>  cmpl$MULTIBOOT2_TAG_TYPE_EFI64_IH,MB2_tag_type(%rcx)
> -jne 2f
> +jne 3f
>  
>  mov MB2_efi64_ih(%rcx),%rdi
> -jmp 3f
> +jmp 4f
>  
> -2:
> +3:
>  /* Is it the end of Multiboot2 information? */
>  cmpl$MULTIBOOT2_TAG_TYPE_END,MB2_tag_type(%rcx)
>  je  run_bs
>  
> -3:
> +4:
>  /* Go to next Multiboot2 information tag. */
>  add MB2_tag_size(%rcx),%ecx
>  add $(MULTIBOOT2_TAG_ALIGN-1),%rcx

See why numeric labels are bad in situations like this? The (much)
earlier patch should use .L labels here, and the patch here then
should simply follow suit.

> @@ -313,14 +329,23 @@ multiboot2_proto:
>  and $~(MULTIBOOT2_TAG_ALIGN-1),%ecx
>  
>  0:
> +/* Get Xen image load base address from Multiboot2 information. */
> +cmpl$MULTIBOOT2_TAG_TYPE_LOAD_BASE_ADDR,MB2_tag_type(%ecx)
> +jne 1f
> +
> +mov MB2_load_base_addr(%ecx),%esi
> +sub $XEN_IMG_OFFSET,%esi
> +jmp 3f

The redundancy once again suggests some form of abstraction
(helper function, macro, ...).

Jan


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

Re: [Xen-devel] x86 PAT fixes for stable

[Ed Swierk]

On Tue, May 24, 2016 at 11:29 PM, Ingo Molnar  wrote:
> Do they apply, build and boot cleanly in that order on top of v4.4, v4.5 and 
> v4.6?
> If yes then:
>
>   Acked-by: Ingo Molnar 

I confirm that they do so on top of v4.4.

Acked-by: Ed Swierk 

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

Re: [Xen-devel] [Xen-users] Hypervisor trap when dumping register

[Julien Grall]

(Move the thread to Xen-devel)

On 26/05/16 02:47, Chenxiao Zhao wrote:

On 5/25/2016 2:37 AM, Julien Grall wrote:


[...]


root@linaro-alip:~# (XEN) *** Serial input -> Xen (type 'CTRL-x' three
times to
switch input to DOM0)
(XEN) 'd' pressed -> dumping registers
(XEN)
(XEN) *** Dumping CPU0 host state: ***
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00245f10Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8


I was able to reproduce it with the latest RC on Juno r2. I will 
investigate it.



--
Julien Grall

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

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

[George Dunlap]

On Fri, Apr 29, 2016 at 6:42 PM, 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.

Hey Tamas,

Sorry for the long delay in getting back to you on this.

So the main issue here (correct me if I'm wrong) is the locking
discipline: namely, men_sharing_share_pages():
- Grabs the hostp2m lock
- Grabs the appropriate domain memsharing locks
- Calls set_shared_p2m_entry(), which ends up calling ept_set_entry(),
which (when altp2m is active) grabs the altp2mlist and altp2m locks.

This causes an ASSERT(), since the altp2mlist lock is ahead of the
memsharing locks in the list.

But having taken a closer look at the code, I'm not sure the change is
quite correct.  Please correct me if I've misread something:

mem_sharing_share_pages() is passed two  pairs -- the
 (which I assume stands for "shared gfn") and 
(which I assume stands for "copy"); and it
1) Looks up smfn and cmfn, which back sgfn and cmfn respectively
2) Looks up cmfn, which backs cgfn then replaces all gfn entries which
point to cmfn with smfn (updating accounting as appropriate)

But this change will only call p2m_altp2m_propagate_change() for the
original cgfn -- any other gfns which are backed by cmfn will not have
the corresponding altp2m entries propagated properly.

This sort of mistake is easy to make, which is why I think we should
try to always update the altp2ms in ept_set_entry() if we can, to
minimize the opportunity for making this sort of mistake.

Is there ever a reason to grab the altp2m lock and *then* grab the
sharing lock?  Could we just move the sharing lock up between the p2m
lock and the altp2mlist lock instead?

 -George

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

[Xen-devel] [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Currently, XENMAPSPACE_dev_mmio maps MMIO regions using one of the most
restrictive memory attribute (Device_nGnRE).

Signed-off-by: Julien Grall 
---
 xen/include/public/memory.h | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index b023046..ece72d2 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
 #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. 
*/
 #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
 * XENMEM_add_to_physmap_batch only. */
-#define XENMAPSPACE_dev_mmio 5 /* device mmio region */
+#define XENMAPSPACE_dev_mmio 5 /* device mmio region
+  On ARM, the region will be mapped
+  in stage-2 using the memory attribute
+  Device_nGnRE. */
 /* ` } */
 
 /*
-- 
1.9.1


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

[Xen-devel] [for-4.7 0/2] xen:

[Julien Grall]

Hello all,

This series is based on the thread [1]. To allow future extension, the new
space dev_mmio should have all unused fields marked as reserved.

The first patch enforces the value of the field 'foreign_id'. The second
document the behavior of dev_mmio for ARM.

This series is candidate for Xen 4.7, the first patch ensure a clean ABI
for the new space which will allow future extension.

Regards,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02328.html

Julien Grall (2):
  xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for
dev_mmio
  xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

 xen/arch/arm/mm.c   | 4 
 xen/common/memory.c | 6 --
 xen/include/public/memory.h | 7 +--
 3 files changed, 13 insertions(+), 4 deletions(-)

-- 
1.9.1


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

[Xen-devel] [for-4.7 1/2] xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Julien Grall]

The field 'foreign_id' is not used when the space is dev_mmio. As the
space is not yet part of the stable ABI, the field is marked as reserved
for future use.

The value should always be 0, other values will return -ENOSYS.

Note that the code would need some rework (such as renaming the field
'foreign_id' to a generic name), however the release of Xen 4.7 is
really close. The rework will be done for the next release.

Signed-off-by: Julien Grall 
---
 xen/arch/arm/mm.c   | 4 
 xen/common/memory.c | 6 --
 xen/include/public/memory.h | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b46e23e..83edfa1 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1143,6 +1143,10 @@ int xenmem_add_to_physmap_one(
 break;
 }
 case XENMAPSPACE_dev_mmio:
+/* The field 'foreign_domid' is reserved for future use */
+if ( foreign_domid )
+return -ENOSYS;
+
 rc = map_dev_mmio_region(d, gpfn, 1, idx);
 return rc;
 
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 644f81a..6bc52ac 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -639,9 +639,11 @@ static int xenmem_add_to_physmap(struct domain *d,
 {
 unsigned int done = 0;
 long rc = 0;
+/* The field 'foreign_id' should be 0 when mapping MMIO. */
+domid_t inv = (xatp->space != XENMAPSPACE_dev_mmio) ? DOMID_INVALID : 0;
 
 if ( xatp->space != XENMAPSPACE_gmfn_range )
-return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
+return xenmem_add_to_physmap_one(d, xatp->space, inv,
  xatp->idx, xatp->gpfn);
 
 if ( xatp->size < start )
@@ -658,7 +660,7 @@ static int xenmem_add_to_physmap(struct domain *d,
 
 while ( xatp->size > done )
 {
-rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
+rc = xenmem_add_to_physmap_one(d, xatp->space, inv,
xatp->idx, xatp->gpfn);
 if ( rc < 0 )
 break;
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index fe52ee1..b023046 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -259,7 +259,7 @@ struct xen_add_to_physmap_batch {
 
 /* Number of pages to go through */
 uint16_t size;
-domid_t foreign_domid; /* IFF gmfn_foreign */
+domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for other spaces. 
*/
 
 /* Indexes into space being mapped. */
 XEN_GUEST_HANDLE(xen_ulong_t) idxs;
-- 
1.9.1


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

[Xen-devel] xen: XENMEM_add_physmap_batch: Mark 'foreign_is' as reserved for dev_mmio (WAS Re: [for-4.7 0/2] xen:)

[Julien Grall]

Sorry I forgot to put a proper title.

On 25/05/16 12:41, Julien Grall wrote:

Hello all,

This series is based on the thread [1]. To allow future extension, the new
space dev_mmio should have all unused fields marked as reserved.

The first patch enforces the value of the field 'foreign_id'. The second
document the behavior of dev_mmio for ARM.

This series is candidate for Xen 4.7, the first patch ensure a clean ABI
for the new space which will allow future extension.

Regards,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02328.html

Julien Grall (2):
   xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for
 dev_mmio
   xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

  xen/arch/arm/mm.c   | 4 
  xen/common/memory.c | 6 --
  xen/include/public/memory.h | 7 +--
  3 files changed, 13 insertions(+), 4 deletions(-)



--
Julien Grall

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

Re: [Xen-devel] [Xen-users] Hypervisor trap when dumping register

[Julien Grall]

On 25/05/16 12:24, Julien Grall wrote:

(Move the thread to Xen-devel)

On 26/05/16 02:47, Chenxiao Zhao wrote:

On 5/25/2016 2:37 AM, Julien Grall wrote:


[...]


root@linaro-alip:~# (XEN) *** Serial input -> Xen (type 'CTRL-x' three
times to
switch input to DOM0)
(XEN) 'd' pressed -> dumping registers
(XEN)
(XEN) *** Dumping CPU0 host state: ***
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00245f10Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8Hypervisor Trap. HSR=0x9607 EC=0x25
IL=1 Syndrome=0x7
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) [ Xen-4.7.0-rc  arm64  debug=y  Tainted:C ]
(XEN) CPU:0
(XEN) PC: 00231dc8


I was able to reproduce it with the latest RC on Juno r2. I will
investigate it.


The bisector fingered the patch:

commit 2aa925be84293b44ad587ed117184ace61b41dd6
Author: Konrad Rzeszutek Wilk 
Date:   Thu Mar 10 16:35:50 2016 -0500

arm/x86: Use struct virtual_region to do bug, symbol, and (x86) 
exception tables lookup.


During execution of the hypervisor we have two regions of
executable code - stext -> _etext, and _sinittext -> _einitext.

The later is not needed after bootup.

We also have various built-in macros and functions to search
in between those two swaths depending on the state of the system.

That is either for bug_frames, exceptions (x86) or symbol
names for the instruction.

With xSplice in the picture - we need a mechanism for new payloads
to searched as well for all of this.

Originally we had extra 'if (xsplice)...' but that gets
a bit tiring and does not hook up nicely.

This 'struct virtual_region' and virtual_region_list provide a
mechanism to search for the bug_frames, exception table,
and symbol names entries without having various calls in
other sub-components in the system.

Code which wishes to participate in bug_frames and exception table
entries search has to only use two public APIs:
 - register_virtual_region
 - unregister_virtual_region

to let the core code know.

If the ->lookup_symbol is not then the default internal symbol lookup
mechanism is used.

Suggested-by: Andrew Cooper 
Signed-off-by: Konrad Rzeszutek Wilk 
Reviewed-by: Andrew Cooper 
Acked-by: Julien Grall  [ARM]
A

Re: [Xen-devel] [PATCH 0/4] dma-mapping: Constify dma_attrs

[Krzysztof Kozlowski]

On 05/24/2016 11:09 AM, Christoph Hellwig wrote:
> I think this is moving into the wrong direction.  The right fix here
> is to get of all the dma_attrs boilerplate code and just replace it
> with a simple enum dma_flags.  This would simplify both the callers
> and most importantly the wrappers for the flag-less versions a lot.

The dma attrs are additive so maybe not an enum but an unsigned long and
#defines:

#define DMA_ATTR_WRITE_BARRIER  0x0001u
#define DMA_ATTR_WEAK_ORDERING  0x0002u
#define DMA_ATTR_WRITE_COMBINE  0x0004u
...

The intrusiveness of it would be similar but indeed looks simpler - when
reading the code and when setting the dma_attrs.

If that seems reasonable, I will send a follow up with new approach.

Thanks for feedback!

Best regards,
Krzysztof

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

Re: [Xen-devel] [PATCH 4/4] xen/arm: arm64: Remove MPIDR multiprocessing extensions check

[Peng Fan]

Hi Wei,

On Wed, May 25, 2016 at 10:10:11AM +0800, Wei Chen wrote:
>In ARM64, the MPIDR multiprocessing extensions bit is reserved to 1.
>So, the value check for this bit is no longer necessary on ARM64.

From ARM DDI0487A.G, I found the U bit for MPIDR_EL1:
"
Indicates a Uniprocessor system, as distinct from PE 0 in a multiprocessor 
system. The possible
values of this bit are:
0 Processor is part of a multiprocessor system.
1 Processor is part of a uniprocessor system.
"

It's not reserved to 1. Which doc are you refering to?

Regards,
Peng.

>
>Signed-off-by: Wei Chen 
>---
> xen/arch/arm/arm64/head.S | 1 -
> 1 file changed, 1 deletion(-)
>
>diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>index 3090beb..91e2817 100644
>--- a/xen/arch/arm/arm64/head.S
>+++ b/xen/arch/arm/arm64/head.S
>@@ -267,7 +267,6 @@ common_start:
>   * find that multiprocessor extensions 
> are
>   * present and the system is SMP  */
> mrs   x0, mpidr_el1
>-tbz   x0, _MPIDR_SMP, 1f /* Multiprocessor extension not 
>supported? */
> tbnz  x0, _MPIDR_UP, 1f  /* Uniprocessor system? */
> 
> ldr   x13, =(~MPIDR_HWID_MASK)
>-- 
>2.7.4
>
>
>___
>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 0/3] VMX: Properly handle pi descriptor and per-cpu blocking list

[Wu, Feng]

> -Original Message-
> From: Dario Faggioli [mailto:dario.faggi...@citrix.com]
> Sent: Tuesday, May 24, 2016 10:02 PM
> To: Wu, Feng ; Jan Beulich 
> Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin
> ; xen-devel@lists.xen.org; konrad.w...@oracle.com;
> k...@xen.org
> Subject: Re: [PATCH 0/3] VMX: Properly handle pi descriptor and per-cpu
> blocking list
> 
> On Tue, 2016-05-24 at 10:07 +, Wu, Feng wrote:
> > > See, for instance, cpu_disable_scheduler() in schedule.c. What we
> > > do is
> > > go over all the vcpus of all domains of either the system or the
> > > cpupool, and force the ones that we found with v->processor set to
> > > the
> > > pCPU that is going down, to perform migration (system_state will be
> > > different than SYS_STATE_suspend, and we hence take the 'else'
> > > branch).
> > >
> > > Considering that the pCPU is no longer part of the relevant
> > > bitmask-s
> > > during the migration, the vCPUs will figure out that they just
> > > can't
> > > stay there, and move somewhere else.
> >
> > Thanks a lot for the elaboration, it is really helpful.
> >
> NP :-)
> 
> > > Note, however, that this happens for running and runnable vCPUs.
> >
> > I don't quite understand this, do you mean cpu_disable_scheduler()
> > only handle running and runnable vCPUs, I tried to find some hints
> > from the code, but I didn't get it. Could you please give some more
> > information about this?
> >
> It goes through all the vcpus of all domains, and does not check or
> care whether they are running, runnable or blocked.
> 
> Let's look at this in some more details. So, let's assume that
> processor 5 is going away, and that you have the following vcpus
> around:
> 
>  d0v0 : v->processor = 5, running on cpu 5
>  d0v1 : v->processor = 4, running on cpu 4
>  d1v0 : v->processor = 5, runnable but not running
>  d2v3 : v->processor = 5, blocked
> 
> for d0v0, we do:
>   cpu_disable_scheduler(5)
>     set_bit(_VPF_migrating, d0v0->pause_flags);
>     vcpu_sleep_nosync(d0v0);
>       SCHED_OP(sleep, d0v0);
>         csched_vcpu_sleep(d0v0)
>           cpu_raise_softirq(5, SCHEDULE_SOFTIRQ);
>     vcpu_migrate(d0v0);
>       if ( v->is_running || ...) // assume v->is_running is true
>         return
>     ...
>     ... <--- scheduling occurs on processor 5
>     ...
>     context_saved(d0v0)
>       vcpu_migrate(d0v0);
>           //is_running is 0, so _VPF_migrating gets cleared
>         vcpu_move_locked(d0v0, new_cpu);
>         vcpu_wake(d0v0);
>           SCHED_OP(wake, d0v0);
>             csched_vcpu_wake(d0v0)
>               __runq_insert(d0v0);
>               __runq_tickle(d0v0);
> 
> for d0v1, we do:
>   cpu_disable_scheduler(5)
>     if ( d0v1->processor != 5 )
>       continue
> 
> for d1v0, we do:
>   cpu_disable_scheduler(5)
>     set_bit(_VPF_migrating, d1v0->pause_flags);
>     vcpu_sleep_nosync(d1v0);
>       SCHED_OP(sleep, d1v0);
>         csched_vcpu_sleep(d1v0)
>           __runq_remove(d1v0);
>     vcpu_migrate(d1v0);
>       if ( d1v0->is_running ||
>            !test_and_clear_bit(_VPF_migrating, d1v0->pause_flags)
>           // false, but clears the _VPF_migrating flag
>       vcpu_move_locked(d1v0, new_cpu);
>       vcpu_wake(v);
>         SCHED_OP(wake, d1v0);
>           csched_vcpu_wake(d1v0)
>             __runq_insert(d1v0);
>             __runq_tickle(d1v0);
> 
> for d2v3, we do:
>   cpu_disable_scheduler(5)
>     set_bit(_VPF_migrating, d2v3-
> >pause_flags);
>     vcpu_sleep_nosync(d2v3);
>       SCHED_OP(sleep, d2v3);
> 
>       csched_vcpu_sleep(d2v3)
> [1]       // Nothing!
> 
> vcpu_migrate(d2v3);
>       if ( d2v3->is_running ||
> 
>  !test_and_clear_bit(_VPF_migrating, d2v3->pause_flags)
>           //
> false, but clears the _VPF_migrating flag
> [*]   vcpu_move_locked(d2v3,
> new_cpu);
>       vcpu_wake(d2v3);
> [2]     // Nothing!
> 
> > > If a
> > > vCPU is blocker, there is nothing to do, and in fact, nothing
> > > happens
> > > (as vcpu_sleep_nosync() and vcpu_wake() are NOP in that case).
> >
> > What do you mean by saying ' vcpu_sleep_nosync() and vcpu_wake()
> > are NOP '?
> >
> See [1] and [2] above.
> 
> > > For
> > > those vCPUs, as soon as they wake up, they'll figure out that their
> > > own
> > > v->processor is not there any longer, and will move somewhere else.
> >
> > So basically, when vCPU is blocking, it has no impact to the blocking
> > vcpu
> > when 'v->processor' is removed. When the vCPU is waken up, it will
> > find
> > another pCPU to run, since the original 'v->processor' is down and no
> > longer in the cpu bitmask, right?
> >
> Yes, that was my point.
> 
> _However_, as you can see at [*] above, it must be noted that even
> those vcpus that blocked while running on a certain processor (5 in the
> example), indeed have a chance to have their
> v->processor changed to something that is still online (something
> different than 5), as a consequence of that processor going away.
> 
> Whether this is useful/enough 

Re: [Xen-devel] [PATCH for-4.7 1/2] hotplug: Fix xendomains lock path for RHEL-based systems

[George Dunlap]

On Wed, May 11, 2016 at 12:14 PM, George Dunlap
 wrote:
> Commit c996572 changed the LOCKFILE path from a check between two
> hardcoded paths (/var/lock/subsys/ or /var/lock) to using the
> XEN_LOCK_DIR variable designated at configure time.  Since
> XEN_LOCK_DIR doesn't (and shouldn't) have the 'subsys' postfix, this
> effectively moves all the lock files by default to /var/lock instead.
>
> Unfortunately, this breaks xendomains on RedHat-based SYSV init
> systems.  RedHat-based SYSV init systems try to only call "${SERVICE}
> shutdown" on systems which actually have an actively running
> component; and they use the existence of /var/lock/subsys/${SERVICE}
> to determine which systems are running.
>
> Changing XEN_LOCK_DIR to /var/lock/subsys is not suitable, as only
> system services like xendomains should create lockfiles there; other
> locks (such as the console locks) should be created in /var/lock
> instead.
>
> Instead, re-instate the check for the subsys/ subdirectory of the lock
> directory in the xendomains script.
>
> Signed-off-by: George Dunlap 

This should be backported to 4.6.

 -George

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

Re: [Xen-devel] [PATCH for-4.7 2/2] tools/xendomains: Create lockfile on start unconditionally

[George Dunlap]

On Wed, May 11, 2016 at 12:14 PM, George Dunlap
 wrote:
> At the moment, the xendomains init script will only create a lockfile
> if when started, it actually does something -- either tries to restore
> a previously saved domain as a result of XENDOMAINS_RESTORE, or tries
> to create a domain as a result of XENDOMAINS_AUTO.
>
> RedHat-based SYSV init systems try to only call "${SERVICE} shutdown"
> on systems which actually have an actively running component; and they
> use the existence of /var/lock/subsys/${SERVICE} to determine which
> systems are running.
>
> This means that at the moment, on RedHat-based SYSV systems (such as
> CentOS 6), if you enable xendomains, and have XENDOMAINS_RESTORE set
> to "true", but don't happen to start a VM, then your running VMs will
> not be suspended on shutdown.
>
> Since the lockfile doesn't really have any other effect than to
> prevent duplicate starting, just create it unconditionally every time
> we start the xendomains script.
>
> The other option would have been to touch the lockfile if
> XENDOMAINS_RESTORE was true regardless of whether there were any
> domains to be restored.  But this would mean that if you started with
> the xendomains script active but XENDOMAINS_RESTORE set to "false",
> and then changed it to "true", then xendomains would still not run the
> next time you shut down.  This seems to me to violate the principle of
> least surprise.
>
> Signed-off-by: George Dunlap 

And this should probably be backported as far back as we're still
doing bugfixes (which I guess would be 4.6 and 4.5?)

 -George

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

Re: [Xen-devel] [for-4.7 1/2] xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Jan Beulich]

>>> On 25.05.16 at 13:41,  wrote:
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1143,6 +1143,10 @@ int xenmem_add_to_physmap_one(
>  break;
>  }
>  case XENMAPSPACE_dev_mmio:
> +/* The field 'foreign_domid' is reserved for future use */
> +if ( foreign_domid )
> +return -ENOSYS;

This should return -EINVAL or maybe -EOPNOTSUPP, but
definitely not -ENOSYS.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -639,9 +639,11 @@ static int xenmem_add_to_physmap(struct domain *d,
>  {
>  unsigned int done = 0;
>  long rc = 0;
> +/* The field 'foreign_id' should be 0 when mapping MMIO. */
> +domid_t inv = (xatp->space != XENMAPSPACE_dev_mmio) ? DOMID_INVALID : 0;

This is a bad type for something that now isn't a domain ID anymore.
Please use u16 or even better unsigned int. Eventually we should
fix xenmem_add_to_physmap_one()'s respective parameter type
accordingly.

Also I think the condition would better be space == gmfn_foreign.

> @@ -658,7 +660,7 @@ static int xenmem_add_to_physmap(struct domain *d,
>  
>  while ( xatp->size > done )
>  {
> -rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
> +rc = xenmem_add_to_physmap_one(d, xatp->space, inv,
> xatp->idx, xatp->gpfn);

This instance you could actually leave alone (as it's dealing with
XENMAPSPACE_gmfn_range only).

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -259,7 +259,7 @@ struct xen_add_to_physmap_batch {
>  
>  /* Number of pages to go through */
>  uint16_t size;
> -domid_t foreign_domid; /* IFF gmfn_foreign */
> +domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for other 
> spaces. */

I wonder whether we shouldn't fix up the structure here right away,
instead of deferring that to after 4.7. After all, as above, we don't
really want a domain ID here generally anymore, so this should
either become "u16 aux" (or some such) or a union (all of course only
for new enough __XEN_INTERFACE_VERSION__).

Plus I think we will want this to be IN/OUT, such that if the
implementation, rather than failing, uses a replacement attribute,
that could be communicated back. Of course that would matter
only if we don't go the union route mentioned above.

Wei, would that be still acceptable for 4.7?

Jan


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

Re: [Xen-devel] [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Jan Beulich]

>>> On 25.05.16 at 13:41,  wrote:
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
>  #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap 
> only. */
>  #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
>  * XENMEM_add_to_physmap_batch only. */
> -#define XENMAPSPACE_dev_mmio 5 /* device mmio region */
> +#define XENMAPSPACE_dev_mmio 5 /* device mmio region
> +  On ARM, the region will be mapped
> +  in stage-2 using the memory attribute
> +  Device_nGnRE. */

Since this is unimplemented on x86, may I suggest to make this "ARM
only; the region will be ..."?

Also - is Device_nGnRE a term uniform between ARM32 and ARM64?

Jan


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

[Xen-devel] [for-4.7] xen/arm: Don't call setup_virtual_regions multiple time

[Julien Grall]

The commit 2aa925be84293b44ad587ed117184ace61b41dd6 "arm/x86: Use struct
virtual_region to do bug, symbol, and (x86) exception tables lookup."
has introduced virtual_region. The call to initialize those regions is
made in init_traps which is called during each CPU bring up.

This will result to register multiple time the same region and Xen crash
when an address is looked up.

This can be fixed by moving the call to setup_virtual_region directly in
start_xen.

Signed-off-by: Julien Grall 
Reported-by: Chenxia Zhao 

---

Cc: Konrad Rzeszutek Wilk 

This is a bug fix for Xen 4.7. Without this change, any use of
virtual_region (printing a symbol) could lead to a crash in Xen.
---
 xen/arch/arm/setup.c | 1 +
 xen/arch/arm/traps.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 09ff1ea..9bc11c4 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -722,6 +722,7 @@ void __init start_xen(unsigned long boot_phys_offset,
 set_current((struct vcpu *)0xf000); /* debug sanity */
 idle_vcpu[0] = current;
 
+setup_virtual_regions(NULL, NULL);
 /* Initialize traps early allow us to get backtrace when an error occurred 
*/
 init_traps();
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1828ea1..aa3e3c2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -102,8 +102,6 @@ integer_param("debug_stack_lines", debug_stack_lines);
 
 void init_traps(void)
 {
-setup_virtual_regions(NULL, NULL);
-
 /* Setup Hyp vector base */
 WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2);
 
-- 
1.9.1


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

[Xen-devel] [PATCH for-4.7] xl: use xstrdup in cpurange_parse

[Wei Liu]

This ensures buf is always valid when it is passed to strtok_r.

CID: 1291936

Signed-off-by: Wei Liu 
---
Cc: Ian Jackson 

This is a backport candidate.
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 03ab644..d8530f0 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -847,7 +847,7 @@ static int update_cpumap_range(const char *str, 
libxl_bitmap *cpumap)
  */
 static int cpurange_parse(const char *cpu, libxl_bitmap *cpumap)
 {
-char *ptr, *saveptr = NULL, *buf = strdup(cpu);
+char *ptr, *saveptr = NULL, *buf = xstrdup(cpu);
 int rc = 0;
 
 for (ptr = strtok_r(buf, ",", &saveptr); ptr;
-- 
2.1.4


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

Re: [Xen-devel] [PATCH OSSTEST] make-flight: don't create ovmf tests for seabios branch

[Ian Jackson]

Wei Liu writes ("[PATCH OSSTEST] make-flight: don't create ovmf tests for 
seabios branch"):
> Signed-off-by: Wei Liu 

Acked-by: Ian Jackson 

I have queued this in a branch that will see it pushed at some point.

Thanks,
Ian.

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

Re: [Xen-devel] [PATCH 0/3] VMX: Properly handle pi descriptor and per-cpu blocking list

[Wu, Feng]

> -Original Message-
> From: Dario Faggioli [mailto:dario.faggi...@citrix.com]
> Sent: Tuesday, May 24, 2016 10:47 PM
> To: Wu, Feng ; Jan Beulich 
> Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin
> ; xen-devel@lists.xen.org; konrad.w...@oracle.com;
> k...@xen.org
> Subject: Re: [PATCH 0/3] VMX: Properly handle pi descriptor and per-cpu
> blocking list
> 
> On Tue, 2016-05-24 at 13:33 +, Wu, Feng wrote:
> > > From: Wu, Feng
> > > > From: Dario Faggioli [mailto:dario.faggi...@citrix.com]
> > > >
> > > > If a
> > > > vCPU is blocker, there is nothing to do, and in fact, nothing
> > > > happens
> > > > (as vcpu_sleep_nosync() and vcpu_wake() are NOP in that case).
> > > What do you mean by saying ' vcpu_sleep_nosync() and vcpu_wake()
> > > are NOP '?
> >
> > I think I understand what you meant above now.
> >
> Right. In any case, see the email I've just sent, with a detailed
> breakdown of the situation and of what actually happens.
> 
> > Do you think the following idea makes sense?
> >
> > When a pCPU is unplugged, we can just remove the vcpus on the
> > associated per-cpu blocking list, then we can choose another online
> > cpu, set the right NDST value, and put the vCPU the new per-cpu list?
> >
> Well, this does make sense to me, but the point is how you do that. I
> mean, how do you get to execute some PI adjustment code, from cpu-
> teardown code?
> 
> Right now, for what seemed to be necessary until now, we have the
> arch_vcpu_block(). Do we need more? If yes where?
> 
> From looking only at schedule.c, we already have arch_move_irqs(), can
> we take advantage of it?

I think we can add the logic in vmx_cpu_dead(). I've already have a draft
patch, and I will send it out to your guys to have a review later!

Thanks,
Feng

> 
> Regards,
> Dario
> --
> <> (Raistlin Majere)
> -
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

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

Re: [Xen-devel] [for-4.7] xen/arm: Don't call setup_virtual_regions multiple time

[Wei Liu]

Should be "multiple times" in title.

On Wed, May 25, 2016 at 02:14:06PM +0100, Julien Grall wrote:
> The commit 2aa925be84293b44ad587ed117184ace61b41dd6 "arm/x86: Use struct
> virtual_region to do bug, symbol, and (x86) exception tables lookup."
> has introduced virtual_region. The call to initialize those regions is
> made in init_traps which is called during each CPU bring up.
> 
> This will result to register multiple time the same region and Xen crash
> when an address is looked up.
> 
> This can be fixed by moving the call to setup_virtual_region directly in
> start_xen.
> 
> Signed-off-by: Julien Grall 
> Reported-by: Chenxia Zhao 
> 
> ---
> 
> Cc: Konrad Rzeszutek Wilk 
> 
> This is a bug fix for Xen 4.7. Without this change, any use of
> virtual_region (printing a symbol) could lead to a crash in Xen.

Yes, this needs fixing.

And of course this is all ARM code and you're the maintainer so I'm fine
with this going in:

Release-acked-by: Wei Liu 

> ---
>  xen/arch/arm/setup.c | 1 +
>  xen/arch/arm/traps.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 09ff1ea..9bc11c4 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -722,6 +722,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  set_current((struct vcpu *)0xf000); /* debug sanity */
>  idle_vcpu[0] = current;
>  
> +setup_virtual_regions(NULL, NULL);
>  /* Initialize traps early allow us to get backtrace when an error 
> occurred */
>  init_traps();
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1828ea1..aa3e3c2 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -102,8 +102,6 @@ integer_param("debug_stack_lines", debug_stack_lines);
>  
>  void init_traps(void)
>  {
> -setup_virtual_regions(NULL, NULL);
> -
>  /* Setup Hyp vector base */
>  WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2);
>  
> -- 
> 1.9.1
> 

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

Re: [Xen-devel] [PATCH 4/4] xen/arm: arm64: Remove MPIDR multiprocessing extensions check

[Julien Grall]

Hello Peng,

On 25/05/16 13:37, Peng Fan wrote:

On Wed, May 25, 2016 at 10:10:11AM +0800, Wei Chen wrote:

In ARM64, the MPIDR multiprocessing extensions bit is reserved to 1.
So, the value check for this bit is no longer necessary on ARM64.


 From ARM DDI0487A.G, I found the U bit for MPIDR_EL1:
"
Indicates a Uniprocessor system, as distinct from PE 0 in a multiprocessor 
system. The possible
values of this bit are:
0 Processor is part of a multiprocessor system.
1 Processor is part of a uniprocessor system.
"

It's not reserved to 1. Which doc are you refering to?


Please read carefully the patch. The check on MPDIR_EL1.U is kept, only 
the check to the RES1 bit ('M' for Aarch32) is removed.


Regards,

--
Julien Grall

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Edgar E. Iglesias]

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
> Hi Edgar,
> 
> On 23/05/2016 16:42, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 04:13:53PM +0100, Julien Grall wrote:
> >>On 23/05/16 15:02, Edgar E. Iglesias wrote:
> >>>On Mon, May 23, 2016 at 02:02:39PM +0100, Julien Grall wrote:
> (CC Wei Liu)
> 
> On 23/05/16 12:56, Edgar E. Iglesias wrote:
> >On Mon, May 23, 2016 at 11:29:31AM +0100, Julien Grall wrote:
> >>On 20/05/16 16:51, Edgar E. Iglesias wrote:
> >>>From: "Edgar E. Iglesias" 
> >>>
> >>>This series adds support for mapping mmio-sram nodes into dom0
> >>>as MEMORY, cached and with RWX perms.
> >>
> >>Can you explain why you chose to map those nodes as MEMORY, cached and 
> >>with
> >>RWX perms?
> >
> >My understanding is that these mmio-sram nodes are allowed to be treated 
> >as
> >Normal memory by the guest OS.
> >Guests could potentially do any kind of memory like operations on them.
> >
> >In our specific case, dom0 won't execute code from these regions but
> >Linux/dom0 ends up using standard memcpy/memset/x functions (not
> >memcpy_fromio and friends) on the regions.
> 
> I looked at the generic sram driver in Linux (drivers/misc/sram.c) which
> actually use memcpy_{to,from}io. So how you driver differs from the 
> generic
> one? What the SRAM will contain?
> >>>
> >>>We intend to use that same driver to map the memory but mmio-sram
> >>>nodes allow you to assign the regions to other device-drivers.
> >>>
> >>>Some examples:
> >>>Documentation/devicetree/bindings/crypto/marvell-cesa.txt
> >>>arch/arm/boot/dts/orion5x.dtsi
> >>>drivers/crypto/mv_cesa.c
> >>>
> >>>The cover letter for the sram driver had an example aswell allthough
> >>>the function names have changed since (it's of_gen_pool_get now):
> >>>https://lwn.net/Articles/537728/
> >>>
> >>>Nothing explicitely says that the regions can be assumed to be mapped
> >>>as Normal memory, but on Linux they do get mapped as Mormal WC mem
> >>>(unless the no-memory-wc prop is set on the node).
> >>>The marvell-cesa example also uses plain memset on the sram.
> >>
> >>I am a bit confused with this example. From my understanding of
> >>mv_cesa_get_ram, cp->sram can point either to a normal memory (?) area (see
> >>gen_pool_dma_alloc) or a Device_nGnRE area (see devm_ioremap_resource).
> >>
> >>However, memcpy_{from,to}io should be used when dealing with MMIO (the field
> >>sram has the __iomem attribute). See the commit 0f3304dc from Russel King
> >>related to marvell/cesa.
> >
> >
> >Yeah, I'm started to get confused too. Maybe they just forgot the memset
> >in drivers/crypto/mv_cesa.c.
> >
> >There are other examples though, that don't do fromio/toio at all.
> >Documentation/devicetree/bindings/media/coda.txt
> >drivers/media/platform/coda/coda-common.c
> >
> >Allthough ofcourse, these could also be wrong. Maybe I've missunderstood how
> >mmio-sram is supposed to be used.
> 
> I have talked about the memory attribute around me and the consensus is we
> should use the most relaxed mode that does not have any security implication
> or undefined behavior for a given device.
> 
> For SRAM it would be normal memory uncached (?) when the property
> "no-memory-wc" is not present, else TBD.
> 
> I suspect we would have to relax more MMIOs in the future. Rather than
> providing a function to map, the code is very similar except the memory
> attribute, I suggest to provide a list of compatible with the memory
> attribute to use.
> 
> All the children node would inherit the memory attribute of the parent.
> 
> What do you think?

Hi again,

Looking a little closer, the place where the generic list of matches and
attributes doesn't work well is when trying to deal with the no-memory-wc
property available only in mmio-sram nodes.

We'd really need an mmio-sram specific check in that case. Either
explicitely open coded in domain_build.c or something along the lines
f the .map method. Or did you have other ideas in mind?

Best regards,
Edgar

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

Re: [Xen-devel] [PATCH for-4.7] xl: use xstrdup in cpurange_parse

[Andrew Cooper]

On 25/05/16 14:23, Wei Liu wrote:
> This ensures buf is always valid when it is passed to strtok_r.
>
> CID: 1291936
>
> Signed-off-by: Wei Liu 
> ---
> Cc: Ian Jackson 

Reviewed-by: Andrew Cooper 

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

Re: [Xen-devel] [PATCH for-4.7] xl: use xstrdup in cpurange_parse

[Ian Jackson]

Wei Liu writes ("[PATCH for-4.7] xl: use xstrdup in cpurange_parse"):
> This ensures buf is always valid when it is passed to strtok_r.
> 
> CID: 1291936
> 
> Signed-off-by: Wei Liu 

Acked-by: Ian Jackson 

> This is a backport candidate.

Really ?  malloc failing is vanishingly rare in practice nowadays and
the consequence is a crash (which is what xstrdup achieves anyway).

Ian.

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

Re: [Xen-devel] [PATCH for-4.7] xl: use xstrdup in cpurange_parse

[Wei Liu]

On Wed, May 25, 2016 at 02:33:41PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH for-4.7] xl: use xstrdup in cpurange_parse"):
> > This ensures buf is always valid when it is passed to strtok_r.
> > 
> > CID: 1291936
> > 
> > Signed-off-by: Wei Liu 
> 
> Acked-by: Ian Jackson 
> 
> > This is a backport candidate.
> 
> Really ?  malloc failing is vanishingly rare in practice nowadays and
> the consequence is a crash (which is what xstrdup achieves anyway).
> 

This is a fair argument. I will leave the judgement to you. If you don't
think this is worth backporting I won't insist. :-)

> Ian.

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

Re: [Xen-devel] [for-4.7] xen/arm: Don't call setup_virtual_regions multiple time

[Wei Liu]

On Wed, May 25, 2016 at 02:14:06PM +0100, Julien Grall wrote:
> The commit 2aa925be84293b44ad587ed117184ace61b41dd6 "arm/x86: Use struct
> virtual_region to do bug, symbol, and (x86) exception tables lookup."
> has introduced virtual_region. The call to initialize those regions is
> made in init_traps which is called during each CPU bring up.
> 
> This will result to register multiple time the same region and Xen crash
> when an address is looked up.
> 
> This can be fixed by moving the call to setup_virtual_region directly in
> start_xen.
> 
> Signed-off-by: Julien Grall 
> Reported-by: Chenxia Zhao 
> 

Also fwiw:
Reviewed-by: Wei Liu 

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

Re: [Xen-devel] [for-4.7] xen/arm: Don't call setup_virtual_regions multiple time

[Konrad Rzeszutek Wilk]

On Wed, May 25, 2016 at 02:14:06PM +0100, Julien Grall wrote:
> The commit 2aa925be84293b44ad587ed117184ace61b41dd6 "arm/x86: Use struct
> virtual_region to do bug, symbol, and (x86) exception tables lookup."
> has introduced virtual_region. The call to initialize those regions is
> made in init_traps which is called during each CPU bring up.
> 
> This will result to register multiple time the same region and Xen crash
> when an address is looked up.

AAh, and that would explain why I didn't see it when I ran it under
the emulator - I couldn't boot it with more than one CPU (the TIMER bug)!

> 
> This can be fixed by moving the call to setup_virtual_region directly in
> start_xen.
> 
> Signed-off-by: Julien Grall 
> Reported-by: Chenxia Zhao 

Reviewed-by: Konrad Rzeszutek Wilk 
> 
> ---
> 
> Cc: Konrad Rzeszutek Wilk 
> 
> This is a bug fix for Xen 4.7. Without this change, any use of
> virtual_region (printing a symbol) could lead to a crash in Xen.
> ---
>  xen/arch/arm/setup.c | 1 +
>  xen/arch/arm/traps.c | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 09ff1ea..9bc11c4 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -722,6 +722,7 @@ void __init start_xen(unsigned long boot_phys_offset,
>  set_current((struct vcpu *)0xf000); /* debug sanity */
>  idle_vcpu[0] = current;
>  
> +setup_virtual_regions(NULL, NULL);
>  /* Initialize traps early allow us to get backtrace when an error 
> occurred */
>  init_traps();
>  
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1828ea1..aa3e3c2 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -102,8 +102,6 @@ integer_param("debug_stack_lines", debug_stack_lines);
>  
>  void init_traps(void)
>  {
> -setup_virtual_regions(NULL, NULL);
> -
>  /* Setup Hyp vector base */
>  WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2);
>  
> -- 
> 1.9.1
> 

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

Re: [Xen-devel] [PATCH 0/4] xen/arm: arm64: Widen register access to mpidr to 64-bits

[Julien Grall]

Hi Wei,

Please try to send all the patches with the cover letter as references. 
git send-email should do it if you send all the patches at once.


This is really helpful to with mail client supporting thread.

Regards,

On 25/05/16 03:08, Wei Chen wrote:

In ARM64 the MPIDR register is mapped to MPIDR_EL1, and the register
bits are expanded to 64-bits. But Xen 64-bit ARM code treats this it
as 32-bit register.
We have to provide correct accessing to this register to avoid
unexpected issues that is caused by incorrect MPIDR value.

Wei Chen (4):
   xen/arm: Change the variable type of cpu_logical_map to register_t
   xen/arm: Make AFFINITY_MASK generate correct mask for level3
   xen:arm: arm64: Add correct MPIDR_HWID_MASK value for ARM64
   xen/arm: arm64: Remove MPIDR multiprocessing extensions check

  xen/arch/arm/arm64/head.S   |  3 +--
  xen/arch/arm/gic-v3.c   |  2 +-
  xen/arch/arm/smpboot.c  | 13 +++--
  xen/include/asm-arm/processor.h |  9 +++--
  4 files changed, 16 insertions(+), 11 deletions(-)



--
Julien Grall

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

Re: [Xen-devel] [PATCH for-4.7] xl: use xstrdup in cpurange_parse

[Wei Liu]

On Wed, May 25, 2016 at 02:23:56PM +0100, Wei Liu wrote:
> This ensures buf is always valid when it is passed to strtok_r.
> 
> CID: 1291936
> 
> Signed-off-by: Wei Liu 

Pushed. Thanks everyone.

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

Re: [Xen-devel] [for-4.7] xen/arm: Don't call setup_virtual_regions multiple time

[Wei Liu]

On Wed, May 25, 2016 at 09:37:09AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, May 25, 2016 at 02:14:06PM +0100, Julien Grall wrote:
> > The commit 2aa925be84293b44ad587ed117184ace61b41dd6 "arm/x86: Use struct
> > virtual_region to do bug, symbol, and (x86) exception tables lookup."
> > has introduced virtual_region. The call to initialize those regions is
> > made in init_traps which is called during each CPU bring up.
> > 
> > This will result to register multiple time the same region and Xen crash
> > when an address is looked up.
> 
> AAh, and that would explain why I didn't see it when I ran it under
> the emulator - I couldn't boot it with more than one CPU (the TIMER bug)!
> 
> > 
> > This can be fixed by moving the call to setup_virtual_region directly in
> > start_xen.
> > 
> > Signed-off-by: Julien Grall 
> > Reported-by: Chenxia Zhao 
> 
> Reviewed-by: Konrad Rzeszutek Wilk 

Pushed. Thanks everyone.

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

[Xen-devel] [qemu-upstream-4.3-testing test] 94761: trouble: blocked/broken

[osstest service owner]

flight 94761 qemu-upstream-4.3-testing real [real]
http://logs.test-lab.xenproject.org/osstest/logs/94761/

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. 80927
 build-amd64   3 host-install(3) broken REGR. vs. 80927
 build-amd64-pvops 3 host-install(3) broken REGR. vs. 80927
 build-i3863 host-install(3) broken REGR. vs. 80927

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt  1 build-check(1)   blocked  n/a
 test-amd64-amd64-amd64-pvgrub  1 build-check(1)   blocked  n/a
 build-amd64-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-amd64-i386-pvgrub  1 build-check(1)   blocked  n/a
 test-amd64-i386-freebsd10-i386  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-ovmf-amd64  1 build-check(1)  blocked n/a
 test-amd64-i386-xl-qemuu-winxpsp3-vcpus1  1 build-check(1) blocked n/a
 test-amd64-i386-libvirt   1 build-check(1)   blocked  n/a
 test-amd64-i386-pv1 build-check(1)   blocked  n/a
 test-amd64-i386-qemuu-rhel6hvm-intel  1 build-check(1) blocked n/a
 test-amd64-i386-pair  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-debianhvm-amd64  1 build-check(1) blocked n/a
 test-amd64-i386-qemuu-rhel6hvm-amd  1 build-check(1)   blocked n/a
 test-amd64-i386-freebsd10-amd64  1 build-check(1)   blocked  n/a
 test-amd64-i386-xl1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-raw1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-win7-amd64  1 build-check(1)  blocked n/a
 build-i386-libvirt1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-amd64-libvirt-vhd  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pygrub   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl   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-pv   1 build-check(1)   blocked  n/a
 test-amd64-amd64-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-amd64-amd64-pair 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

version targeted for testing:
 qemuuc97c20f71240a538a19cb6b0e598bc1bbd5168f1
baseline version:
 qemuu10c1b763c26feb645627a1639e722515f3e1e876

Last test of basis80927  2016-02-06 13:30:02 Z  109 days
Testing same since93977  2016-05-10 11:09:16 Z   15 days   55 attempts


People who touched revisions under test:
  Gerd Hoffmann 
  Stefano Stabellini 

jobs:
 build-amd64  broken  
 build-i386   broken  
 build-amd64-libvirt  blocked 
 build-i386-libvirt   blocked 
 build-amd64-pvopsbroken  
 build-i386-pvops broken  
 test-amd64-amd64-xl  blocked 
 test-amd64-i386-xl   blocked 
 test-amd64-i386-qemuu-rhel6hvm-amd   blocked 
 test-amd64-amd64-xl-qemuu-debianhvm-amd64blocked 
 test-amd64-i386-xl-qemuu-debianhvm-amd64 blocked 
 test-amd64-i386-freebsd10-amd64  blocked 
 test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked 
 test-amd64-i386-xl-qemuu-ovmf-amd64  blocked 
 test-amd64-amd64-xl-qemuu-win7-amd64 blocked 
 test-amd64-i386-xl-qemuu-win7-amd64  blocked 
 test-amd64-amd64-xl-credit2  blocked 
 test-amd64-i386-freebsd10-i386   blocked 
 test-amd64-i386-qemuu-rhel6hvm-intel blocked 
 test-amd64-amd64-libvirt blocked 
 test-amd64-i386-libvirt  blocked 
 test-amd64-amd64-xl-multivcpublocked 
 test-amd64-amd64-pairblo

Re: [Xen-devel] [PATCH 1/4] xen/arm: Change the variable type of cpu_logical_map to register_t

[Julien Grall]

Hi Wei,

On 25/05/16 03:09, Wei Chen wrote:

The cpu_logical_map is used to store CPU hardware ID from MPIDR_EL1 or
from CPU node of DT. Currently, the cpu_logical_map is using the u32 as
its variable type. It can work properly while Xen is running on ARM32,
because the hardware ID is 32-bits. While Xen is running on ARM64, the
hardware ID expands to 64-bits and then the cpu_logical_map will overflow.

Change the the variable type of cpu_logical_map to register_t will make


NIT: s/the the/the/


cpu_logical_map to store hardware ID correctly on ARM32 and ARM64.


s/ID/IDs/



Signed-off-by: Wei Chen 


The code looks good to me. With the typos fixed:

Acked-by: Julien Grall 

Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 2/4] xen/arm: Make AFFINITY_MASK generate correct mask for level3

[Julien Grall]

Hi Wei,

On 25/05/16 03:09, Wei Chen wrote:

The original affinity shift bits algorithm in AFFINITY_MASK is buggy,
it could not generate correct affinity shift bits of level3.
The macro MPIDR_LEVEL_SHIFT can calculate level3 affinity shift bits
correctly. We use this macro in AFFINITY_MASK to generate correct
mask for level3.
Signed-off-by: Wei Chen 


Reviewed-by: Julien Grall 

Regards,

--
Julien Grall

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

Re: [Xen-devel] Xen Security Advisory 180 (CVE-2014-3672) - Unrestricted qemu logging

[George Dunlap]

On Mon, May 23, 2016 at 6:09 PM, Xen.org security team  wrote:
> RESOLUTION
> ==
>
> Applying the appropriate attached patch resolves this issue.
>
> The patches adopt a simple and rather crude approach which is
> effective at resolving the security issue in the context of a Xen
> device model.  They may not be appropriate for adoption upstream or in
> other contexts.

This is indeed a rather crude approach; but for our upcoming 4.7
release, what's the plan?  Do we have time to generalize xenconsoled
to handle this sort of logging, and if so, who is going to do that
work?

 -George

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

Re: [Xen-devel] [for-4.7 1/2] xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Wei Liu]

On Wed, May 25, 2016 at 07:12:30AM -0600, Jan Beulich wrote:
> >>> On 25.05.16 at 13:41,  wrote:
> > --- a/xen/arch/arm/mm.c
> > +++ b/xen/arch/arm/mm.c
> > @@ -1143,6 +1143,10 @@ int xenmem_add_to_physmap_one(
> >  break;
> >  }
> >  case XENMAPSPACE_dev_mmio:
> > +/* The field 'foreign_domid' is reserved for future use */
> > +if ( foreign_domid )
> > +return -ENOSYS;
> 
> This should return -EINVAL or maybe -EOPNOTSUPP, but
> definitely not -ENOSYS.
> 
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -639,9 +639,11 @@ static int xenmem_add_to_physmap(struct domain *d,
> >  {
> >  unsigned int done = 0;
> >  long rc = 0;
> > +/* The field 'foreign_id' should be 0 when mapping MMIO. */
> > +domid_t inv = (xatp->space != XENMAPSPACE_dev_mmio) ? DOMID_INVALID : 
> > 0;
> 
> This is a bad type for something that now isn't a domain ID anymore.
> Please use u16 or even better unsigned int. Eventually we should
> fix xenmem_add_to_physmap_one()'s respective parameter type
> accordingly.
> 
> Also I think the condition would better be space == gmfn_foreign.
> 
> > @@ -658,7 +660,7 @@ static int xenmem_add_to_physmap(struct domain *d,
> >  
> >  while ( xatp->size > done )
> >  {
> > -rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
> > +rc = xenmem_add_to_physmap_one(d, xatp->space, inv,
> > xatp->idx, xatp->gpfn);
> 
> This instance you could actually leave alone (as it's dealing with
> XENMAPSPACE_gmfn_range only).
> 
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -259,7 +259,7 @@ struct xen_add_to_physmap_batch {
> >  
> >  /* Number of pages to go through */
> >  uint16_t size;
> > -domid_t foreign_domid; /* IFF gmfn_foreign */
> > +domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for other 
> > spaces. */
> 
> I wonder whether we shouldn't fix up the structure here right away,
> instead of deferring that to after 4.7. After all, as above, we don't
> really want a domain ID here generally anymore, so this should
> either become "u16 aux" (or some such) or a union (all of course only
> for new enough __XEN_INTERFACE_VERSION__).
> 
> Plus I think we will want this to be IN/OUT, such that if the
> implementation, rather than failing, uses a replacement attribute,
> that could be communicated back. Of course that would matter
> only if we don't go the union route mentioned above.
> 
> Wei, would that be still acceptable for 4.7?
> 

Sure. It's a simple in term of code change and should be fairly easy to
review.

Wei.

> Jan
> 

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

Re: [Xen-devel] [PATCH 3/4] xen:arm: arm64: Add correct MPIDR_HWID_MASK value for ARM64

[Julien Grall]

Hi Wei,

Title: "Fix the MPIDR_HWID_MASK value for ARM64".

On 25/05/16 03:09, Wei Chen wrote:

Current MPIDR_HWID_MASK is using the bit definition of ARM32 MPIDR.


s/Current/Currently/


This value is not correct while Xen is running on ARM64.


I think s/while/when/


Now, we add a correct value for this marco on ARM64. But this value


s/marco/macro/


is not a valid 64-bit immediate which can be encoded in mov instruction.
So we have to use ldr to load this value to register.


You need to explain what is the valid value, i.e there is 4 level of 
affinity whilst AArch32 has only 3. It would be good to mention the spec 
too.




Signed-off-by: Wei Chen 
---
  xen/arch/arm/arm64/head.S   | 2 +-
  xen/include/asm-arm/processor.h | 4 
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index d5831f2..3090beb 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -270,7 +270,7 @@ common_start:
  tbz   x0, _MPIDR_SMP, 1f /* Multiprocessor extension not 
supported? */
  tbnz  x0, _MPIDR_UP, 1f  /* Uniprocessor system? */

-mov   x13, #(~MPIDR_HWID_MASK)
+ldr   x13, =(~MPIDR_HWID_MASK)
  bic   x24, x0, x13   /* Mask out flags to get CPU ID */
  1:

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index b4cce7e..284ad6a 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -18,7 +18,11 @@
  #define MPIDR_SMP   (_AC(1,U) << _MPIDR_SMP)
  #define MPIDR_AFF0_SHIFT(0)
  #define MPIDR_AFF0_MASK (_AC(0xff,U) << MPIDR_AFF0_SHIFT)
+#ifdef CONFIG_ARM_64
+#define MPIDR_HWID_MASK _AC(0xff00ff,UL)
+#else
  #define MPIDR_HWID_MASK _AC(0xff,U)
+#endif
  #define MPIDR_INVALID   (~MPIDR_HWID_MASK)
  #define MPIDR_LEVEL_BITS(8)




Regards,

--
Julien Grall

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

Re: [Xen-devel] [PATCH 4/4] xen/arm: arm64: Remove MPIDR multiprocessing extensions check

[Julien Grall]

Hi Wei,

On 25/05/16 03:10, Wei Chen wrote:

In ARM64, the MPIDR multiprocessing extensions bit is reserved to 1.


Well, technically the bit is unamed for ARM64. So I would make clear 
that the name is from AArch32. Something along:


"The bit 31 (former bit Multiprocessing extensions bit in AArch32) is 
always RES1 for AArch64."



So, the value check for this bit is no longer necessary on ARM64.

Signed-off-by: Wei Chen 
---
  xen/arch/arm/arm64/head.S | 1 -
  1 file changed, 1 deletion(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 3090beb..91e2817 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -267,7 +267,6 @@ common_start:
* find that multiprocessor extensions 
are
* present and the system is SMP  */
  mrs   x0, mpidr_el1
-tbz   x0, _MPIDR_SMP, 1f /* Multiprocessor extension not 
supported? */
  tbnz  x0, _MPIDR_UP, 1f  /* Uniprocessor system? */

  ldr   x13, =(~MPIDR_HWID_MASK)



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] docs: update xl manpage about {block, network}-attach command

[Wei Liu]

On Wed, May 25, 2016 at 03:15:23PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH for-4.7] docs: update xl manpage about 
> {block,network}-attach command"):
> > State that only attaching PV interface is supported.
> ...
> >  Create a new virtual block device.  This will trigger a hotplug event
> > -for the guest.
> > +for the guest. Note that only attaching PV block device is supported,
> > +the B field is ignored.
> 
> But the vdev field specifies more than just the device type.  So I
> think this is wrong.

I guess to make things simpler I can just delete the "" part?

"Note that only attaching PV block device is supported"

> 
> Ian.

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

Re: [Xen-devel] [PATCH for-4.7] docs: update xl manpage about {block, network}-attach command

[Ian Jackson]

Wei Liu writes ("[PATCH for-4.7] docs: update xl manpage about 
{block,network}-attach command"):
> State that only attaching PV interface is supported.
...
>  Create a new virtual block device.  This will trigger a hotplug event
> -for the guest.
> +for the guest. Note that only attaching PV block device is supported,
> +the B field is ignored.

But the vdev field specifies more than just the device type.  So I
think this is wrong.

Ian.

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

Re: [Xen-devel] [RFC for-4.8 0/6] xen/arm: Add support for mapping mmio-sram nodes into dom0

[Julien Grall]

Hi Edgar,

On 25/05/16 14:29, Edgar E. Iglesias wrote:

On Tue, May 24, 2016 at 08:44:41PM +0100, Julien Grall wrote:
Looking a little closer, the place where the generic list of matches and
attributes doesn't work well is when trying to deal with the no-memory-wc
property available only in mmio-sram nodes.

We'd really need an mmio-sram specific check in that case. Either
explicitely open coded in domain_build.c or something along the lines
f the .map method. Or did you have other ideas in mind?


How about extending the function dt_match_node and the structure 
dt_device_match to check the existence (or not) of a property?


Regards,

--
Julien Grall

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

Re: [Xen-devel] [for-4.7 1/2] xen: XENMEM_add_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Julien Grall]

Hi Jan,

On 25/05/16 14:12, Jan Beulich wrote:

On 25.05.16 at 13:41,  wrote:

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1143,6 +1143,10 @@ int xenmem_add_to_physmap_one(
  break;
  }
  case XENMAPSPACE_dev_mmio:
+/* The field 'foreign_domid' is reserved for future use */
+if ( foreign_domid )
+return -ENOSYS;


This should return -EINVAL or maybe -EOPNOTSUPP, but
definitely not -ENOSYS.


I will use -EOPNOTSUPP.




--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -639,9 +639,11 @@ static int xenmem_add_to_physmap(struct domain *d,
  {
  unsigned int done = 0;
  long rc = 0;
+/* The field 'foreign_id' should be 0 when mapping MMIO. */
+domid_t inv = (xatp->space != XENMAPSPACE_dev_mmio) ? DOMID_INVALID : 0;


This is a bad type for something that now isn't a domain ID anymore.
Please use u16 or even better unsigned int. Eventually we should
fix xenmem_add_to_physmap_one()'s respective parameter type
accordingly.

Also I think the condition would better be space == gmfn_foreign.


Ok.




@@ -658,7 +660,7 @@ static int xenmem_add_to_physmap(struct domain *d,

  while ( xatp->size > done )
  {
-rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
+rc = xenmem_add_to_physmap_one(d, xatp->space, inv,
 xatp->idx, xatp->gpfn);


This instance you could actually leave alone (as it's dealing with
XENMAPSPACE_gmfn_range only).


--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -259,7 +259,7 @@ struct xen_add_to_physmap_batch {

  /* Number of pages to go through */
  uint16_t size;
-domid_t foreign_domid; /* IFF gmfn_foreign */
+domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for other spaces. 
*/


I wonder whether we shouldn't fix up the structure here right away,
instead of deferring that to after 4.7. After all, as above, we don't
really want a domain ID here generally anymore, so this should
either become "u16 aux" (or some such) or a union (all of course only
for new enough __XEN_INTERFACE_VERSION__).

Plus I think we will want this to be IN/OUT, such that if the
implementation, rather than failing, uses a replacement attribute,
that could be communicated back. Of course that would matter
only if we don't go the union route mentioned above.


I will give a look to use an union.

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] docs: update xl manpage about {block, network}-attach command

[Ian Jackson]

Wei Liu writes ("Re: [PATCH for-4.7] docs: update xl manpage about 
{block,network}-attach command"):
> On Wed, May 25, 2016 at 03:15:23PM +0100, Ian Jackson wrote:
...
> > But the vdev field specifies more than just the device type.  So I
> > think this is wrong.
> 
> I guess to make things simpler I can just delete the "" part?
> 
> "Note that only attaching PV block device is supported"

How about

  Note that only PV block devices are supported by block-attach.
  Requests to attach emulated devices (eg, vdev=hdc) will result in
  only the PV view being available to the guest.

Ian.

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

Re: [Xen-devel] [PATCH for-4.7] docs: update xl manpage about {block, network}-attach command

[Wei Liu]

On Wed, May 25, 2016 at 03:28:50PM +0100, Ian Jackson wrote:
> Wei Liu writes ("Re: [PATCH for-4.7] docs: update xl manpage about 
> {block,network}-attach command"):
> > On Wed, May 25, 2016 at 03:15:23PM +0100, Ian Jackson wrote:
> ...
> > > But the vdev field specifies more than just the device type.  So I
> > > think this is wrong.
> > 
> > I guess to make things simpler I can just delete the "" part?
> > 
> > "Note that only attaching PV block device is supported"
> 
> How about
> 
>   Note that only PV block devices are supported by block-attach.
>   Requests to attach emulated devices (eg, vdev=hdc) will result in
>   only the PV view being available to the guest.
> 

LGTM. I will update the patch.

Wei.

> Ian.

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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Ian Jackson]

Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit 
default_ioport_* accesses"):
> Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA:
> ACPI 2.0, Hardware: Add access_width/bit_offset support for
> acpi_hw_write()") result in guests issuing 32-bit accesses to IO space.
> 
> QEMU needs to be able to handle them.

I'm kind of missing something here.  If the specification has recently
been updated to permit this, why should old hardware support it ?

(I tried to find the Linux upstream git commit you're referring to but
my linux.git is up to date and it seems not to be fetching within a
reasonable time, so I thought I would reply now.)

Ian.

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

[Xen-devel] balloon_mutex lockdep complaint at HVM domain destroy

[Ed Swierk]

The following lockdep dump occurs whenever I destroy an HVM domain, on
Linux 4.4 Dom0 with CONFIG_XEN_BALLOON=n on recent stable Xen 4.5.

Any clues whether this is a real potential deadlock, or how to silence
it if not?

==
[ INFO: RECLAIM_FS-safe -> RECLAIM_FS-unsafe lock order detected ]
4.4.11-grsec #1 Not tainted
--
qemu-system-i38/3338 [HC0[0]:SC0[0]:HE1:SE1] is trying to acquire:
 (balloon_mutex){+.+.+.}, at: [] 
81430ac3

and this task is already holding:
 (&priv->lock){+.+.-.}, at: [] 8143c77f
which would create a new lock dependency:
 (&priv->lock){+.+.-.} -> (balloon_mutex){+.+.+.}

but this new dependency connects a RECLAIM_FS-irq-safe lock:
 (&priv->lock){+.+.-.}
... which became RECLAIM_FS-irq-safe at:
  [<__lock_acquire at lockdep.c:2839>] 810becc5
  [] 810c0ac9
  [] 816d1b4c
  [] 8143c3d4
  [] 8143c450
  [<__mmu_notifier_invalidate_page at mmu_notifier.c:183>] 8119de42
  [] 811840c2
  [] 81185051
  [] 81185497
  [] 811599b7
  [] 8115a489
  [] 8115af3a
  [] 8115b1bb
  [] 8115c1e4
  [] 8108eccc
  [] 816d706e

to a RECLAIM_FS-irq-unsafe lock:
 (balloon_mutex){+.+.+.}
... which became RECLAIM_FS-irq-unsafe at:
...  [] 810bdd69
  [] 810c12f9
  [<__alloc_pages_nodemask at page_alloc.c:3248>] 8114e0d1
  [] 81199b36
  [] 8143030e
  [] 81430c94
  [] 8142f362
  [] 8143d208
  [] 811da630
  [] 811daa64
  [] 816d6cba

other info that might help us debug this:

 Possible interrupt unsafe locking scenario:

   CPU0CPU1
   
  lock(balloon_mutex);
   local_irq_disable();
   lock(&priv->lock);
   lock(balloon_mutex);
  
lock(&priv->lock);

 *** DEADLOCK ***

1 lock held by qemu-system-i38/3338:
 #0:  (&priv->lock){+.+.-.}, at: [] 
8143c77f

the dependencies between RECLAIM_FS-irq-safe lock and the holding lock:
-> (&priv->lock){+.+.-.} ops: 8996 {
   HARDIRQ-ON-W at:
[<__lock_acquire at lockdep.c:2818>] 810bec71
[] 810c0ac9
[] 816d1b4c
[] 8143c3d4
[<__mmu_notifier_invalidate_range_start at 
mmu_notifier.c:197>] 8119d72a
[] 
81172051
[] 81173e55
[] 81175f26
[<__do_page_fault at fault.c:1491>] 8105676f
[] 81056a49
[] 816d87e8
[] 811c4074
[] 
816d6cba
   SOFTIRQ-ON-W at:
[<__lock_acquire at lockdep.c:2822>] 810bec9e
[] 810c0ac9
[] 816d1b4c
[] 8143c3d4
[<__mmu_notifier_invalidate_range_start at 
mmu_notifier.c:197>] 8119d72a
[] 
81172051
[] 81173e55
[] 81175f26
[<__do_page_fault at fault.c:1491>] 8105676f
[] 81056a49
[] 816d87e8
[] 811c4074
[] 
816d6cba
   IN-RECLAIM_FS-W at:
   [<__lock_acquire at lockdep.c:2839>] 810becc5
   [] 810c0ac9
   [] 816d1b4c
   [] 8143c3d4
   [] 8143c450
   [<__mmu_notifier_invalidate_page at mmu_notifier.c:183>] 
8119de42
   [] 
811840c2
   [] 81185051
   [] 81185497
   [] 811599b7
   [] 
8115a489
   [] 8115af3a
   [] 8115b1bb
   [] 8115c1e4
   [] 8108eccc
   [] 816d706e
   INITIAL USE at:
   [<__lock_acquire at lockdep.c:3171>] 810be85c
   [] 810c0ac9
   [] 816d1b4c
   [] 8143c3d4
   [<__mmu_notifier_invalidate_range_start at 
mmu_notifier.c:197>] 8119d72a
   [] 
81172051
   [] 81173e55
   [] 81175f26
   [<__do_page_fault at fault.c:1491>] 8105676f
   [] 81056a49
   [] 816d87e8
 

Re: [Xen-devel] [PATCH for-4.7] docs: update xl manpage about {block, network}-attach command

[Wei Liu]

On Wed, May 25, 2016 at 03:31:19PM +0100, Wei Liu wrote:
> On Wed, May 25, 2016 at 03:28:50PM +0100, Ian Jackson wrote:
> > Wei Liu writes ("Re: [PATCH for-4.7] docs: update xl manpage about 
> > {block,network}-attach command"):
> > > On Wed, May 25, 2016 at 03:15:23PM +0100, Ian Jackson wrote:
> > ...
> > > > But the vdev field specifies more than just the device type.  So I
> > > > think this is wrong.
> > > 
> > > I guess to make things simpler I can just delete the "" part?
> > > 
> > > "Note that only attaching PV block device is supported"
> > 
> > How about
> > 
> >   Note that only PV block devices are supported by block-attach.
> >   Requests to attach emulated devices (eg, vdev=hdc) will result in
> >   only the PV view being available to the guest.
> > 
> 
> LGTM. I will update the patch.
> 

This is the updated patch.

I will commit it soon.

---8<--
From 1bca93e2326e5a644f00de80a72bcab181587489 Mon Sep 17 00:00:00 2001
From: Wei Liu 
Date: Mon, 23 May 2016 12:07:20 +0100
Subject: [PATCH for-4.7 v2] docs: update xl manpage about 
{block,network}-attach command

State that only attaching PV interface is supported.

Signed-off-by: Wei Liu 
Signed-off-by: Ian Jackson 
---
Use Ian's wording about block-attach and add his S-o-B.
---
 docs/man/xl.pod.1 | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 9887f1b..f4dc32c 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1286,6 +1286,10 @@ effect to the guest OS is much the same as any hotplug 
event.
 Create a new virtual block device.  This will trigger a hotplug event
 for the guest.
 
+Note that only PV block devices are supported by block-attach.
+Requests to attach emulated devices (eg, vdev=hdc) will result in only
+the PV view being available to the guest.
+
 B
 
 =over 4
@@ -1369,6 +1373,8 @@ B string in the domain config file. See L and
 L
 for more informations.
 
+Note that only attaching PV network interface is supported.
+
 =item B I I
 
 Removes the network device from the domain specified by I.
-- 
2.1.4


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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit 
default_ioport_* accesses"):
> Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit 
> default_ioport_* accesses"):
> > Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA:
> > ACPI 2.0, Hardware: Add access_width/bit_offset support for
> > acpi_hw_write()") result in guests issuing 32-bit accesses to IO space.
> > 
> > QEMU needs to be able to handle them.
> 
> I'm kind of missing something here.  If the specification has recently
> been updated to permit this, why should old hardware support it ?
> 
> (I tried to find the Linux upstream git commit you're referring to but
> my linux.git is up to date and it seems not to be fetching within a
> reasonable time, so I thought I would reply now.)

I have looked at this commit now and I am none the wiser.

It says just "This patch adds access_width/bit_offset support in
acpi_hw_write()".  I also looked at the two linked messages:
  https://github.com/acpica/acpica/commit/48eea5e7
  https://bugs.acpica.org/show_bug.cgi?id=1240
and none of this explains why this supported is needed in a
our deep-frozen ancient branch.

Thanks,
Ian.

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

Re: [Xen-devel] [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Hi Jan,

On 25/05/16 14:14, Jan Beulich wrote:

On 25.05.16 at 13:41,  wrote:

--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -220,7 +220,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
  #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. 
*/
  #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
  * XENMEM_add_to_physmap_batch only. */
-#define XENMAPSPACE_dev_mmio 5 /* device mmio region */
+#define XENMAPSPACE_dev_mmio 5 /* device mmio region
+  On ARM, the region will be mapped
+  in stage-2 using the memory attribute
+  Device_nGnRE. */


Since this is unimplemented on x86, may I suggest to make this "ARM
only; the region will be ..."?


Sounds good.



Also - is Device_nGnRE a term uniform between ARM32 and ARM64?


Actually it should be Device-nGnRE to match with the spec. This has been 
introduced on Armv8. Previously for Armv7, the name was "Device", 
although the behavior is exactly the same.


I could say:

"ARM only; the region will be mapped in Stage-2 using the memory 
attribute 'Device-nGnRE' (previously named 'Device' on ARMv7)".


Regards,

--
Julien Grall

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

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

[osstest service owner]

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

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  4074e4ebe9115ac4986f963a13feada3e0560460
baseline version:
 xen  4504b7d1a6594c8ad4b1ecdab15d30feca7eaa51

Last test of basis94744  2016-05-24 17:01:59 Z0 days
Testing same since94764  2016-05-25 13:00:57 Z0 days1 attempts


People who touched revisions under test:
  Dario Faggioli 

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=4074e4ebe9115ac4986f963a13feada3e0560460
+ . ./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 
4074e4ebe9115ac4986f963a13feada3e0560460
+ branch=xen-unstable-smoke
+ revision=4074e4ebe9115ac4986f963a13feada3e0560460
+ . ./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
+ '[' x4074e4ebe9115ac4986f963a13feada3e0560460 = 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 $c{"GitCacheProxy"} or die $!;
'
+++ local cache=git://cache:9419/
+++ '[' xgit://cache:9419/ '!=' x ']'
+++ echo 
'git://cache:9419/https://gith

Re: [Xen-devel] Xen Security Advisory 180 (CVE-2014-3672) - Unrestricted qemu logging

[Wei Liu]

On Wed, May 25, 2016 at 03:04:40PM +0100, George Dunlap wrote:
> On Mon, May 23, 2016 at 6:09 PM, Xen.org security team  
> wrote:
> > RESOLUTION
> > ==
> >
> > Applying the appropriate attached patch resolves this issue.
> >
> > The patches adopt a simple and rather crude approach which is
> > effective at resolving the security issue in the context of a Xen
> > device model.  They may not be appropriate for adoption upstream or in
> > other contexts.
> 
> This is indeed a rather crude approach; but for our upcoming 4.7
> release, what's the plan?  Do we have time to generalize xenconsoled
> to handle this sort of logging, and if so, who is going to do that
> work?
> 

I this it's going to be a bit intrusive at this point to change
xenconsoled to do that. However it should be too hard to test.
We also need people to test and review it. All in all it seems time is
very tight.

We can at least apply this patch to our own tree.

Wei.

>  -George
> 
> ___
> 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] [for-4.7 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Jan Beulich]

>>> On 25.05.16 at 16:43,  wrote:
> I could say:
> 
> "ARM only; the region will be mapped in Stage-2 using the memory 
> attribute 'Device-nGnRE' (previously named 'Device' on ARMv7)".

SGTM

Jan


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

[Xen-devel] [PATCH] x86/compat: correct SMEP/SMAP NOPs patching

[Jan Beulich]

Correct the number of single byte NOPs we want to be replaced in case
neither SMEP nor SMAP are available.

Also simplify the expression adding these NOPs - at that location .
equals .Lcr4_orig, and removing that part of the expression fixes a
bogus ".space or fill with negative value, ignored" warning by very old
gas (which actually is what made me look at those constructs again).

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -175,7 +175,7 @@ compat_bad_hypercall:
 ENTRY(compat_restore_all_guest)
 ASSERT_INTERRUPTS_DISABLED
 .Lcr4_orig:
-.skip (.Lcr4_alt_end - .Lcr4_alt) - (. - .Lcr4_orig), 0x90
+.skip .Lcr4_alt_end - .Lcr4_alt, 0x90
 .Lcr4_orig_end:
 .pushsection .altinstr_replacement, "ax"
 .Lcr4_alt:
@@ -200,7 +200,8 @@ ENTRY(compat_restore_all_guest)
 jne   1b
 .Lcr4_alt_end:
 .section .altinstructions, "a"
-altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, 12, 0
+altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, \
+ (.Lcr4_orig_end - .Lcr4_orig), 0
 altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMEP, \
  (.Lcr4_orig_end - .Lcr4_orig), \
  (.Lcr4_alt_end - .Lcr4_alt)



x86/compat: correct SMEP/SMAP NOPs patching

Correct the number of single byte NOPs we want to be replaced in case
neither SMEP nor SMAP are available.

Also simplify the expression adding these NOPs - at that location .
equals .Lcr4_orig, and removing that part of the expression fixes a
bogus ".space or fill with negative value, ignored" warning by very old
gas (which actually is what made me look at those constructs again).

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -175,7 +175,7 @@ compat_bad_hypercall:
 ENTRY(compat_restore_all_guest)
 ASSERT_INTERRUPTS_DISABLED
 .Lcr4_orig:
-.skip (.Lcr4_alt_end - .Lcr4_alt) - (. - .Lcr4_orig), 0x90
+.skip .Lcr4_alt_end - .Lcr4_alt, 0x90
 .Lcr4_orig_end:
 .pushsection .altinstr_replacement, "ax"
 .Lcr4_alt:
@@ -200,7 +200,8 @@ ENTRY(compat_restore_all_guest)
 jne   1b
 .Lcr4_alt_end:
 .section .altinstructions, "a"
-altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, 12, 0
+altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, \
+ (.Lcr4_orig_end - .Lcr4_orig), 0
 altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMEP, \
  (.Lcr4_orig_end - .Lcr4_orig), \
  (.Lcr4_alt_end - .Lcr4_alt)
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Boris Ostrovsky]

On 05/25/2016 10:35 AM, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit 
> default_ioport_* accesses"):
>> Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit 
>> default_ioport_* accesses"):
>>> Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA:
>>> ACPI 2.0, Hardware: Add access_width/bit_offset support for
>>> acpi_hw_write()") result in guests issuing 32-bit accesses to IO space.
>>>
>>> QEMU needs to be able to handle them.
>> I'm kind of missing something here.  If the specification has recently
>> been updated to permit this, why should old hardware support it ?
>>
>> (I tried to find the Linux upstream git commit you're referring to but
>> my linux.git is up to date and it seems not to be fetching within a
>> reasonable time, so I thought I would reply now.)
> I have looked at this commit now and I am none the wiser.
>
> It says just "This patch adds access_width/bit_offset support in
> acpi_hw_write()".  I also looked at the two linked messages:
>   https://github.com/acpica/acpica/commit/48eea5e7
>   https://bugs.acpica.org/show_bug.cgi?id=1240
> and none of this explains why this supported is needed in a
> our deep-frozen ancient branch.

IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's
Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the
patch it used register's bit_width and now it will use access_size.
According to the spec access_size 0 means undefined/legacy access.

I just looked at what hvmloader provides and at least for FADT
address_size is 0. And I wonder whether ACPICA uses 4-byte-access for
these cases.

So maybe instead of trying to patch qemu-trad I should see if I can make
hvmloader provide proper access size. Let me poke at that.

-boris


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

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

[Xu, Quan]

On May 25, 2016 4:07 PM, Jan Beulich  wrote:
> >>> On 25.05.16 at 08:41,  wrote:
> > On May 24, 2016 4:22 PM, Jan Beulich  wrote:
> >> >>> On 18.05.16 at 10:08,  wrote:
> >> >  static int device_power_down(void)  {
> >> > -console_suspend();
> >> > +if ( console_suspend() )
> >> > +return SAVED_CONSOLE;
> >>
> >> I said so on the previous round, and I need to repeat it now: If
> >> console_suspend() fails, you saved _nothing_.
> >>
> >
> > Ah, we may have some different views for SAVED_*, which I mean has
> > been saved and we are no need to resume.
> >
> > e.g. if console_suspend() fails, I did return SAVED_CONSOLE, reading
> > my patch again, and I really resume nothing at all.
> >
> > device_power_up()
> > {
> >  ...
> > +case SAVED_CONSOLE:
> > +break;
> > ...
> > }
> >
> >
> > I know we can also propagate SAVED_NONE for console_suspend() failure,
> > then we need adjust device_power_up() relevantly.
> 
> My main point is that the names of these enumerators should reflect their
> purpose. If one reads "SAVED_CONSOLE", then (s)he should be allowed this
> to mean that console state was saved (and hence needs to be restored upon
> error / resume).
> 


I'll follow this.. it is much better than what I understand.


> >> > -time_suspend();
> >> > +if ( time_suspend() )
> >> > +return SAVED_TIME;
> >> >
> >> > -i8259A_suspend();
> >> > +if ( i8259A_suspend() )
> >> > +return SAVED_I8259A;
> >> >
> >> > +/* ioapic_suspend cannot fail */
> >> >  ioapic_suspend();
> >> >
> >> > -iommu_suspend();
> >> > +if ( iommu_suspend() )
> >> > +return SAVED_IOMMU;
> >> >
> >> > -lapic_suspend();
> >> > +if ( lapic_suspend() )
> >> > +return SAVED_LAPIC;
> >> >
> >> >  return 0;
> >>
> >> And this silently means SAVED_NONE, whereas here you saved everything.
> >> Yielding clearly bogus code ...
> >>
> >
> >
> >  '0' is just on success here.  Look at the condition where we call
> > device_power_up():
> >
> > +if ( error > 0 )
> > +device_power_up(error);
> >
> > Then, it is not bogus code.
> 
> See above: Zero should not mean both "nothing saved" and "saved
> everything".
> 
> >> Also, having come here - did I miss iommu_flush_iotlb_global()
> >> gaining a __must_check annotation somewhere?
> >
> > I will add __must_check annotation to iommu_flush_iotlb_global().
> >
> >> And the struct iommu_flush pointers
> >> and handlers? And, by analogy, iommu_flush_context_*()?
> >
> > I am better only add __must_check annotation to flush->iotlb and
> > handlers, but leaving flush->context/handers and
> > iommu_flush_context_*() as are in current patch set..
> > the coming patch set will fix them.
> 
> I don't follow the logic behind this: The purpose of this series is to make 
> sure
> flushing errors get properly bubbled up, which includes adding __must_check
> annotations. I'm not saying this needs to happen in this patch, but it should
> happen in this series

I will add a patch..

> (and please following the same basic model: A caller or a
> __must_check function should either already be __must_check, or should
> become so at the same time).
> 

Agreed.

Quan


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

Re: [Xen-devel] ARM Xen Bug #45: Is there a solution?

[Dirk Behme]

Hi Julien,

On 24.05.2016 22:05, Julien Grall wrote:



On 24/05/2016 14:39, Dirk Behme wrote:

Hi Julien,


Hello Dirk,


On 23.05.2016 22:15, Julien Grall wrote:

Hello Dirk,


is there a solution for

arm: domain 0 disables clocks which are in fact being used
http://bugs.xenproject.org/xen/bug/45

?

On an ARM based board I have to use 'clk_ignore_unused' preventing
that
Dom0 disables the UART clock for the console UART configured with
console=hvc0.


There is no better solution than passing "clk_ignore_unused" on the
kernel command line so far.



What would be the solution for this issue? The

"propagate any clock related properties from the UART
node into the Xen hypervisor node"

mentioned in the ticket?


That is correct. Xen would copy the property "clocks" of the UART into
the hypervisor node.

DOM0 would then parse the clocks associated to this node and mark them
as used by Xen (I think CLK_IGNORE_UNUSED could do the job for us).



I've started to look into this:

I'd think in arm_uart.c in dt_uart_init() after

if ( !dev )

we know the UART node we are looking for. From this we have to read 
the clock configuration.


To be clarified: How to read the clock configuration? I couldn't find 
any convenient function dt_device_get_clock() or similar for that.


Now, we have the clocks we are looking for.

These are needed in domain_build.c in make_hypervisor_node(), then.

To be clarified: How to pass the clock configuration from arm_uart.c 
to domain_build.c (and not break the non-dt / non-ARM platforms)?


Any ideas or comments?


Best regards

Dirk


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

Re: [Xen-devel] x86 PAT fixes for stable

[Ed Swierk]

...along with 
https://git.kernel.org/cgit/linux/kernel/git/xen/tip.git/commit/?id=702f9260
which should also go into v4.4, IMO.


On Wed, May 25, 2016 at 4:17 AM, Ed Swierk  wrote:
> On Tue, May 24, 2016 at 11:29 PM, Ingo Molnar  wrote:
>> Do they apply, build and boot cleanly in that order on top of v4.4, v4.5 and 
>> v4.6?
>> If yes then:
>>
>>   Acked-by: Ingo Molnar 
>
> I confirm that they do so on top of v4.4.
>
> Acked-by: Ed Swierk 

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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Ian Jackson]

Boris Ostrovsky writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit 
default_ioport_* accesses"):
> IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's
> Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the
> patch it used register's bit_width and now it will use access_size.
> According to the spec access_size 0 means undefined/legacy access.

I see.  (Well, sort of.)

> I just looked at what hvmloader provides and at least for FADT
> address_size is 0. And I wonder whether ACPICA uses 4-byte-access for
> these cases.

If 0 is "undefined/legacy access", shouldn't it be using the
register's bit width ?  Ie, isn't this then a bug in ACPICA ?

> So maybe instead of trying to patch qemu-trad I should see if I can make
> hvmloader provide proper access size. Let me poke at that.

If this "access size" attribute is new, things should work without it,
surely ?

Ian.

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

Re: [Xen-devel] [PATCH v5 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

[Xu, Quan]

On May 23, 2016 10:19 PM, Jan Beulich  wrote:
> >>> On 18.05.16 at 10:08,  wrote:
> 
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2463,11 +2463,12 @@ static int __put_page_type(struct page_info
> > *page,  }
> >
> >
> > -static int __get_page_type(struct page_info *page, unsigned long type,
> > -   int preemptible)
> > +static int __must_check __get_page_type(struct page_info *page,
> 
> Such a __must_check is relatively pointless when all existing callers already
> check the return value (and surely all code inside mm.c knows it needs to), 
> and
> you don't at the same time make the non-static functions propagating its
> return value also __must_check.

I will drop this __must_check annotation.

> Hence I think best is to limit your effort to IOMMU functions for this patch 
> set.
>

Good idea.
 
> > +unsigned long type,
> > +int preemptible)
> >  {
> >  unsigned long nx, x, y = page->u.inuse.type_info;
> > -int rc = 0;
> > +int rc = 0, ret = 0;
> >
> >  ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
> >
> > @@ -2578,11 +2579,11 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> >  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >  {
> >  if ( (x & PGT_type_mask) == PGT_writable_page )
> > -iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
> > +ret = iommu_unmap_page(d, mfn_to_gmfn(d,
> > + page_to_mfn(page)));
> >  else if ( type == PGT_writable_page )
> > -iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > -   page_to_mfn(page),
> > -   IOMMUF_readable|IOMMUF_writable);
> > +ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
> > + page_to_mfn(page),
> > +
> > + IOMMUF_readable|IOMMUF_writable);
> >  }
> >  }
> >
> > @@ -2599,6 +2600,9 @@ static int __get_page_type(struct page_info
> *page, unsigned long type,
> >  if ( (x & PGT_partial) && !(nx & PGT_partial) )
> >  put_page(page);
> >
> > +if ( !rc )
> > +rc = ret;
> 
> I know I've seen this a couple of time already, but with the special purpose 
> of
> "ret" I really wonder whether a more specific name wouldn't be warranted -
> e.g. "iommu_rc" or "iommu_ret".


rc is return value for this function, and no directly association with IOMMU 
related code ( rc is only for alloc_page_type() ).
So the rc cannot be "iommu_rc"..

ret can be "iommu_ret", but I think the pair 'rc' / 'ret' may look good.


> 
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -658,7 +658,7 @@ bool_t ept_handle_misconfig(uint64_t gpa)
> >   *
> >   * Returns: 0 for success, -errno for failure
> >   */
> > -static int
> > +static int __must_check
> >  ept_set_entry(struct p2m_domain *p2m, unsigned long gfn, mfn_t mfn,
> 
> Now adding the annotation here, without also (first) adding it to the p2m
> method which this gets used for (and which is this function's sole purpose), 
> is
> not useful at all. Please remember - we don't want this annotation because it
> looks good, but in order to make sure errors don't get wrongly ignored. That's
> why, from the beginning, I have been telling you that adding such annotations
> to other than new code means doing it top down (which you clearly don't do
> here).
> 

I will drop this __must_check annotation.

> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -171,20 +171,33 @@ void __hwdom_init iommu_hwdom_init(struct
> domain *d)
> >  {
> >  struct page_info *page;
> >  unsigned int i = 0;
> > +int rc = 0;
> > +
> >  page_list_for_each ( page, &d->page_list )
> >  {
> >  unsigned long mfn = page_to_mfn(page);
> >  unsigned long gfn = mfn_to_gmfn(d, mfn);
> >  unsigned int mapping = IOMMUF_readable;
> > +int ret;
> >
> >  if ( ((page->u.inuse.type_info & PGT_count_mask) == 0) ||
> >   ((page->u.inuse.type_info & PGT_type_mask)
> >== PGT_writable_page) )
> >  mapping |= IOMMUF_writable;
> > -hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +ret = hd->platform_ops->map_page(d, gfn, mfn, mapping);
> > +
> > +if ( unlikely(ret) )
> > +rc = ret;
> 
> This should be good enough, but I think it would be better if, just like
> elsewhere, you returned the first error instead of the last one.
> 

I will also apply it to this series.


(Jan, I know It is not an easy work to review these little things. I very 
appreciate it. Thank you!! )
Quan

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

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

[Tamas K Lengyel]

On May 25, 2016 05:27, "George Dunlap"  wrote:
>
> On Fri, Apr 29, 2016 at 6:42 PM, 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.
>
> Hey Tamas,
>
> Sorry for the long delay in getting back to you on this.

No problem, thanks for taking a closer look!

>
> So the main issue here (correct me if I'm wrong) is the locking
> discipline: namely, men_sharing_share_pages():
> - Grabs the hostp2m lock
> - Grabs the appropriate domain memsharing locks
> - Calls set_shared_p2m_entry(), which ends up calling ept_set_entry(),
> which (when altp2m is active) grabs the altp2mlist and altp2m locks.
>
> This causes an ASSERT(), since the altp2mlist lock is ahead of the
> memsharing locks in the list.
>
> But having taken a closer look at the code, I'm not sure the change is
> quite correct.  Please correct me if I've misread something:
>
> mem_sharing_share_pages() is passed two  pairs -- the
>  (which I assume stands for "shared gfn") and 
> (which I assume stands for "copy"); and it

Here s/c stands for source/client.

> 1) Looks up smfn and cmfn, which back sgfn and cmfn respectively
> 2) Looks up cmfn, which backs cgfn then replaces all gfn entries which
> point to cmfn with smfn (updating accounting as appropriate)

Hm, I might have missed that. Where does it do the lookup for all other
cgfns backed by this cmfn?

> But this change will only call p2m_altp2m_propagate_change() for the
> original cgfn -- any other gfns which are backed by cmfn will not have
> the corresponding altp2m entries propagated properly.

Right, if there is some other place where it does sharing in the back we
would have to propagate that change.

> This sort of mistake is easy to make, which is why I think we should
> try to always update the altp2ms in ept_set_entry() if we can, to
> minimize the opportunity for making this sort of mistake.
>
> Is there ever a reason to grab the altp2m lock and *then* grab the
> sharing lock?  Could we just move the sharing lock up between the p2m
> lock and the altp2mlist lock instead?
>

I can't think of a scenario where we would get to sharing from altp2m with
altp2m locking first. Not sure what you mean by moving the sharing lock up
though. The problem is that sharing already has the lock by the time altp2m
tries to lock, so we could pass that info down to make altp2m aware it
needs no locking. It would require extending a bunch of functions though
with an extra input that is barely ever used..

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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Boris Ostrovsky]

On 05/25/2016 11:22 AM, Ian Jackson wrote:
> Boris Ostrovsky writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit 
> default_ioport_* accesses"):
>> IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's
>> Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the
>> patch it used register's bit_width and now it will use access_size.
>> According to the spec access_size 0 means undefined/legacy access.
> I see.  (Well, sort of.)
>
>> I just looked at what hvmloader provides and at least for FADT
>> address_size is 0. And I wonder whether ACPICA uses 4-byte-access for
>> these cases.
> If 0 is "undefined/legacy access", shouldn't it be using the
> register's bit width ?  Ie, isn't this then a bug in ACPICA ?
>
>> So maybe instead of trying to patch qemu-trad I should see if I can make
>> hvmloader provide proper access size. Let me poke at that.
> If this "access size" attribute is new, things should work without it,
> surely ?

This is what the spec says:

AccessSize evaluates to an 8-bit integer that specifies the size of data
values used when accessing the
address space as follows:
0 - Undefined (legacy)
1 - Byte access
2 - Word access
3 - DWord access
4 - QWord access
The 8-bit field DescriptorName . _ASZ is automatically created in order
to refer to this portion of the
resource descriptor. See _ASZ (page 368) for more information. For
backwards compatibility, the
AccesSize parameter is optional when invoking the Register macro. If the
AccessSize parameter is
not supplied then the AccessSize field will be set to zero. In this
case, OSPM will assume the access
size.

I don't think I understand what the last sentence means. Does it imply
that SW can do whatever it thinks is appropriate?


-boris

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

Re: [Xen-devel] Xen Security Advisory 180 (CVE-2014-3672) - Unrestricted qemu logging

[Wei Liu]

On Wed, May 25, 2016 at 03:51:23PM +0100, Wei Liu wrote:
> On Wed, May 25, 2016 at 03:04:40PM +0100, George Dunlap wrote:
> > On Mon, May 23, 2016 at 6:09 PM, Xen.org security team  
> > wrote:
> > > RESOLUTION
> > > ==
> > >
> > > Applying the appropriate attached patch resolves this issue.
> > >
> > > The patches adopt a simple and rather crude approach which is
> > > effective at resolving the security issue in the context of a Xen
> > > device model.  They may not be appropriate for adoption upstream or in
> > > other contexts.
> > 
> > This is indeed a rather crude approach; but for our upcoming 4.7
> > release, what's the plan?  Do we have time to generalize xenconsoled
> > to handle this sort of logging, and if so, who is going to do that
> > work?
> > 
> 
> I this it's going to be a bit intrusive at this point to change
> xenconsoled to do that. However it should be too hard to test.
> We also need people to test and review it. All in all it seems time is
> very tight.
> 

I just read the code of virtlogd and xenconsoled.

I think xenconsoled is missing at least things.

From a higher level:

1. Abstraction of rotating file.
2. Abstraction of client.
3. IPC interface to libxl -- presumably we need to create a socket.

Then we need to write code in libxl to use it. That then involves
inventing a protocol to pass the file name to xenconsoled (assuming we
still want one file per qemu).

Wei.

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

Re: [Xen-devel] [PATCH v3] xen-hvm: ignore background I/O sections

[Anthony PERARD]

On Mon, May 09, 2016 at 05:31:20PM +0100, Paul Durrant wrote:
> Since Xen will correctly handle accesses to unimplemented I/O ports (by
> returning all 1's for reads and ignoring writes) there is no need for
> QEMU to register backgroud I/O sections.
> 
> This patch therefore adds checks to xen_io_add/del so that sections with
> memory-region ops pointing at 'unassigned_io_ops' are ignored.
> 
> Signed-off-by: Paul Durrant 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paolo Bonzini 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

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

[Xen-devel] [for-4.7 v2 0/2] xen: XENMEM_add_to_phymap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Julien Grall]

Hello all,

This series is based on the thread [1]. To allow future extension, the new
space dev_mmio should have all unused fields marked as reserved.

This series is candidate for Xen 4.7, the first patch ensure a clean ABI
for the new space which will allow future extension.

See in each patch for all the changes.

Regards,

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-05/msg02328.html

Julien Grall (2):
  xen: XENMEM_add_to_physmap_batch: Mark 'foreign_id' as reserved for
dev_mmio
  xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

 xen/arch/arm/mm.c   |  9 +++--
 xen/arch/x86/mm.c   |  4 ++--
 xen/common/compat/memory.c  |  2 ++
 xen/common/memory.c | 12 +---
 xen/include/public/memory.h | 16 ++--
 xen/include/xen/mm.h|  3 ++-
 6 files changed, 36 insertions(+), 10 deletions(-)

-- 
1.9.1


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

[Xen-devel] [for-4.7 v2 2/2] xen/arm: Document the behavior of XENMAPSPACE_dev_mmio

[Julien Grall]

Currently, XENMAPSPACE_dev_mmio maps MMIO regions using one of the most
restrictive memory attribute (Device-nGnRE).

Signed-off-by: Julien Grall 

---
Changes in v2:
- s/Device_nGnRE/Device-nGnRE/ to match the spec
- Update the comment to mention the name for ARMv7.
---
 xen/include/public/memory.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index a5ab139..cfb427f 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -220,7 +220,11 @@ DEFINE_XEN_GUEST_HANDLE(xen_machphys_mapping_t);
 #define XENMAPSPACE_gmfn_range   3 /* GMFN range, XENMEM_add_to_physmap only. 
*/
 #define XENMAPSPACE_gmfn_foreign 4 /* GMFN from another dom,
 * XENMEM_add_to_physmap_batch only. */
-#define XENMAPSPACE_dev_mmio 5 /* device mmio region */
+#define XENMAPSPACE_dev_mmio 5 /* device mmio region
+  ARM only; the region will be mapped
+  in Stage-2 using the memory attribute
+  "Device-nGnRE" (previously named
+  "Device" on ARMv7) */
 /* ` } */
 
 /*
-- 
1.9.1


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

Re: [Xen-devel] [PATCH v5 03/10] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping

[Jan Beulich]

>>> On 25.05.16 at 17:34,  wrote:
> On May 23, 2016 10:19 PM, Jan Beulich  wrote:
>> >>> On 18.05.16 at 10:08,  wrote:
>> > +unsigned long type,
>> > +int preemptible)
>> >  {
>> >  unsigned long nx, x, y = page->u.inuse.type_info;
>> > -int rc = 0;
>> > +int rc = 0, ret = 0;
>> >
>> >  ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2)));
>> >
>> > @@ -2578,11 +2579,11 @@ static int __get_page_type(struct page_info
>> *page, unsigned long type,
>> >  if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
>> >  {
>> >  if ( (x & PGT_type_mask) == PGT_writable_page )
>> > -iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page)));
>> > +ret = iommu_unmap_page(d, mfn_to_gmfn(d,
>> > + page_to_mfn(page)));
>> >  else if ( type == PGT_writable_page )
>> > -iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
>> > -   page_to_mfn(page),
>> > -   IOMMUF_readable|IOMMUF_writable);
>> > +ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)),
>> > + page_to_mfn(page),
>> > +
>> > + IOMMUF_readable|IOMMUF_writable);
>> >  }
>> >  }
>> >
>> > @@ -2599,6 +2600,9 @@ static int __get_page_type(struct page_info
>> *page, unsigned long type,
>> >  if ( (x & PGT_partial) && !(nx & PGT_partial) )
>> >  put_page(page);
>> >
>> > +if ( !rc )
>> > +rc = ret;
>> 
>> I know I've seen this a couple of time already, but with the special purpose 
>> of
>> "ret" I really wonder whether a more specific name wouldn't be warranted -
>> e.g. "iommu_rc" or "iommu_ret".
> 
> 
> rc is return value for this function, and no directly association with IOMMU 
> related code ( rc is only for alloc_page_type() ).
> So the rc cannot be "iommu_rc"..
> 
> ret can be "iommu_ret", but I think the pair 'rc' / 'ret' may look good.

Well, I'm not entirely opposed to keeping the names, but I continue
to think that while at the call sites the shorter name is reasonable, it
is quite a bit less clear at the point where you conditionally update rc.

Jan


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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Jan Beulich]

>>> On 25.05.16 at 17:08,  wrote:
> On 05/25/2016 10:35 AM, Ian Jackson wrote:
>> Ian Jackson writes ("Re: [PATCH qemu-traditional] ioreq: Support 32-bit 
> default_ioport_* accesses"):
>>> Boris Ostrovsky writes ("[PATCH qemu-traditional] ioreq: Support 32-bit 
> default_ioport_* accesses"):
 Recent changes in ACPICA (specifically, Linux commit 66b1ed5aa8dd ("ACPICA:
 ACPI 2.0, Hardware: Add access_width/bit_offset support for
 acpi_hw_write()") result in guests issuing 32-bit accesses to IO space.

 QEMU needs to be able to handle them.
>>> I'm kind of missing something here.  If the specification has recently
>>> been updated to permit this, why should old hardware support it ?
>>>
>>> (I tried to find the Linux upstream git commit you're referring to but
>>> my linux.git is up to date and it seems not to be fetching within a
>>> reasonable time, so I thought I would reply now.)
>> I have looked at this commit now and I am none the wiser.
>>
>> It says just "This patch adds access_width/bit_offset support in
>> acpi_hw_write()".  I also looked at the two linked messages:
>>   https://github.com/acpica/acpica/commit/48eea5e7 
>>   https://bugs.acpica.org/show_bug.cgi?id=1240 
>> and none of this explains why this supported is needed in a
>> our deep-frozen ancient branch.
> 
> IIUIC, the Linux/ACPICA patch makes ACPICA use correct field in ACPI's
> Generic Address Structure (section 5.2.3.2 in the 6.0 spec). Before the
> patch it used register's bit_width and now it will use access_size.
> According to the spec access_size 0 means undefined/legacy access.
> 
> I just looked at what hvmloader provides and at least for FADT
> address_size is 0. And I wonder whether ACPICA uses 4-byte-access for
> these cases.
> 
> So maybe instead of trying to patch qemu-trad I should see if I can make
> hvmloader provide proper access size. Let me poke at that.

But don't forget about the risk of breaking other OSes with any
kind of change like this one.

Jan


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

[Xen-devel] [for-4.7 v2 1/2] xen: XENMEM_add_to_physmap_batch: Mark 'foreign_id' as reserved for dev_mmio

[Julien Grall]

The field 'foreign_id' is not used when the space is dev_mmio. As the
space is not yet part of the stable ABI, the field is marked as reserved
for future use.

The value should always be 0, other values will return -EOPNOTSUPP.

Signed-off-by: Julien Grall 

---
Changes in v2:
- Return -EOPNOTSUPP rather than -ENOSYS
- Introduce a union in the sturcutre xenmem_add_to_physmap_batch
---
 xen/arch/arm/mm.c   |  9 +++--
 xen/arch/x86/mm.c   |  4 ++--
 xen/common/compat/memory.c  |  2 ++
 xen/common/memory.c | 12 +---
 xen/include/public/memory.h | 10 +-
 xen/include/xen/mm.h|  3 ++-
 6 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b46e23e..79f4310 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1047,7 +1047,7 @@ void share_xen_page_with_privileged_guests(
 int xenmem_add_to_physmap_one(
 struct domain *d,
 unsigned int space,
-domid_t foreign_domid,
+union add_to_physmap_batch_extra extra,
 unsigned long idx,
 xen_pfn_t gpfn)
 {
@@ -1103,7 +1103,8 @@ int xenmem_add_to_physmap_one(
 {
 struct domain *od;
 p2m_type_t p2mt;
-od = rcu_lock_domain_by_any_id(foreign_domid);
+
+od = rcu_lock_domain_by_any_id(extra.foreign_domid);
 if ( od == NULL )
 return -ESRCH;
 
@@ -1143,6 +1144,10 @@ int xenmem_add_to_physmap_one(
 break;
 }
 case XENMAPSPACE_dev_mmio:
+/* extra should be 0. Reserved for future use. */
+if ( extra.res0 )
+return -EOPNOTSUPP;
+
 rc = map_dev_mmio_region(d, gpfn, 1, idx);
 return rc;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 03a4d5b..0b67103 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4769,7 +4769,7 @@ static int handle_iomem_range(unsigned long s, unsigned 
long e, void *p)
 int xenmem_add_to_physmap_one(
 struct domain *d,
 unsigned int space,
-domid_t foreign_domid,
+union add_to_physmap_batch_extra extra,
 unsigned long idx,
 xen_pfn_t gpfn)
 {
@@ -4830,7 +4830,7 @@ int xenmem_add_to_physmap_one(
 break;
 }
 case XENMAPSPACE_gmfn_foreign:
-return p2m_add_foreign(d, idx, gpfn, foreign_domid);
+return p2m_add_foreign(d, idx, gpfn, extra.foreign_domid);
 default:
 break;
 }
diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c
index 19a914d..e56e187 100644
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -253,6 +253,8 @@ int compat_memory_op(unsigned int cmd, 
XEN_GUEST_HANDLE_PARAM(void) compat)
 unsigned int size = cmp.atpb.size;
 xen_ulong_t *idxs = (void *)(nat.atpb + 1);
 xen_pfn_t *gpfns = (void *)(idxs + limit);
+enum XLAT_add_to_physmap_batch_u u =
+XLAT_add_to_physmap_batch_u_res0;
 
 if ( copy_from_guest(&cmp.atpb, compat, 1) ||
  !compat_handle_okay(cmp.atpb.idxs, size) ||
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 644f81a..b1ac8b9 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -639,9 +639,15 @@ static int xenmem_add_to_physmap(struct domain *d,
 {
 unsigned int done = 0;
 long rc = 0;
+union add_to_physmap_batch_extra extra;
+
+if ( xatp->space != XENMAPSPACE_gmfn_foreign )
+extra.res0 = 0;
+else
+extra.foreign_domid = DOMID_INVALID;
 
 if ( xatp->space != XENMAPSPACE_gmfn_range )
-return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
+return xenmem_add_to_physmap_one(d, xatp->space, extra,
  xatp->idx, xatp->gpfn);
 
 if ( xatp->size < start )
@@ -658,7 +664,7 @@ static int xenmem_add_to_physmap(struct domain *d,
 
 while ( xatp->size > done )
 {
-rc = xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID,
+rc = xenmem_add_to_physmap_one(d, xatp->space, extra,
xatp->idx, xatp->gpfn);
 if ( rc < 0 )
 break;
@@ -719,7 +725,7 @@ static int xenmem_add_to_physmap_batch(struct domain *d,
 }
 
 rc = xenmem_add_to_physmap_one(d, xatpb->space,
-   xatpb->foreign_domid,
+   xatpb->u,
idx, gpfn);
 
 if ( unlikely(__copy_to_guest_offset(xatpb->errs, 0, &rc, 1)) )
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index fe52ee1..a5ab139 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -259,7 +259,15 @@ struct xen_add_to_physmap_batch {
 
 /* Number of pages to go through */
 uint16_t size;
-domid_t foreign_domid; /* IFF gmfn_foreign */
+
+#if __XEN_INTERFACE_VERSION__ < 0x00040700
+domid_t foreign_domid; /* IFF gmfn_foreign. Should be 0 for ot

Re: [Xen-devel] [PATCH] x86/compat: correct SMEP/SMAP NOPs patching

[Wei Liu]

On Wed, May 25, 2016 at 09:00:49AM -0600, Jan Beulich wrote:
> Correct the number of single byte NOPs we want to be replaced in case
> neither SMEP nor SMAP are available.
> 
> Also simplify the expression adding these NOPs - at that location .
> equals .Lcr4_orig, and removing that part of the expression fixes a
> bogus ".space or fill with negative value, ignored" warning by very old
> gas (which actually is what made me look at those constructs again).
> 
> Signed-off-by: Jan Beulich 
> 
> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -175,7 +175,7 @@ compat_bad_hypercall:
>  ENTRY(compat_restore_all_guest)
>  ASSERT_INTERRUPTS_DISABLED
>  .Lcr4_orig:
> -.skip (.Lcr4_alt_end - .Lcr4_alt) - (. - .Lcr4_orig), 0x90
> +.skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>  .Lcr4_orig_end:
>  .pushsection .altinstr_replacement, "ax"
>  .Lcr4_alt:
> @@ -200,7 +200,8 @@ ENTRY(compat_restore_all_guest)
>  jne   1b
>  .Lcr4_alt_end:
>  .section .altinstructions, "a"
> -altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, 12, > 0
> +altinstruction_entry .Lcr4_orig, .Lcr4_orig, X86_FEATURE_ALWAYS, \
> + (.Lcr4_orig_end - .Lcr4_orig), 0
>  altinstruction_entry .Lcr4_orig, .Lcr4_alt, X86_FEATURE_SMEP, \
>   (.Lcr4_orig_end - .Lcr4_orig), \
>   (.Lcr4_alt_end - .Lcr4_alt)
> 
> 
> 

FWIW:
Reviewed-by: Wei Liu 

And subject to review from Andrew:

Release-acked-by: Wei Liu 


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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Jan Beulich]

>>> On 25.05.16 at 17:36,  wrote:
> This is what the spec says:
> 
> AccessSize evaluates to an 8-bit integer that specifies the size of data
> values used when accessing the
> address space as follows:
> 0 - Undefined (legacy)
> 1 - Byte access
> 2 - Word access
> 3 - DWord access
> 4 - QWord access
> The 8-bit field DescriptorName . _ASZ is automatically created in order
> to refer to this portion of the
> resource descriptor. See _ASZ (page 368) for more information. For
> backwards compatibility, the
> AccesSize parameter is optional when invoking the Register macro. If the
> AccessSize parameter is
> not supplied then the AccessSize field will be set to zero. In this
> case, OSPM will assume the access
> size.
> 
> I don't think I understand what the last sentence means. Does it imply
> that SW can do whatever it thinks is appropriate?

I think so, yes.

Jan


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

Re: [Xen-devel] [PATCH] xen: Clean up includes

[Anthony PERARD]

On Tue, May 24, 2016 at 04:27:18PM +0100, Peter Maydell wrote:
> Clean up includes so that osdep.h is included first and headers
> which it implies are not included manually.
> 
> This commit was created with scripts/clean-includes.
> 
> Signed-off-by: Peter Maydell 

Acked-by: Anthony PERARD 

-- 
Anthony PERARD

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

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

[George Dunlap]

On Wed, May 25, 2016 at 4:31 PM, Tamas K Lengyel  wrote:
>
> On May 25, 2016 05:27, "George Dunlap"  wrote:
>>
>> On Fri, Apr 29, 2016 at 6:42 PM, 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.
>>
>> Hey Tamas,
>>
>> Sorry for the long delay in getting back to you on this.
>
> No problem, thanks for taking a closer look!
>
>>
>> So the main issue here (correct me if I'm wrong) is the locking
>> discipline: namely, men_sharing_share_pages():
>> - Grabs the hostp2m lock
>> - Grabs the appropriate domain memsharing locks
>> - Calls set_shared_p2m_entry(), which ends up calling ept_set_entry(),
>> which (when altp2m is active) grabs the altp2mlist and altp2m locks.
>>
>> This causes an ASSERT(), since the altp2mlist lock is ahead of the
>> memsharing locks in the list.
>>
>> But having taken a closer look at the code, I'm not sure the change is
>> quite correct.  Please correct me if I've misread something:
>>
>> mem_sharing_share_pages() is passed two  pairs -- the
>>  (which I assume stands for "shared gfn") and 
>> (which I assume stands for "copy"); and it
>
> Here s/c stands for source/client.
>
>> 1) Looks up smfn and cmfn, which back sgfn and cmfn respectively
>> 2) Looks up cmfn, which backs cgfn then replaces all gfn entries which
>> point to cmfn with smfn (updating accounting as appropriate)
>
> Hm, I might have missed that. Where does it do the lookup for all other
> cgfns backed by this cmfn?

I was looking at the loop in the middle of the function:

while ( (gfn = rmap_iterate(cpage, &ri)) != NULL) {
 ...
}

I haven't chased it down, but it looks like this walks the reverse map
of all gfns which map cpage; and for each such gfn it finds it:
* removes the cpage -> gfn rmap
* Adds an spage -> gfn map
* Reduces the type count of cpage
* Sets the p2m entry for that gfn to the smfn (rather than cmfn).

Obviously the common case is that the number of mappings is exactly 1;
but we need to either ensure that this is always true, or we need to
handle the case where it's not true. :-)

>> But this change will only call p2m_altp2m_propagate_change() for the
>> original cgfn -- any other gfns which are backed by cmfn will not have
>> the corresponding altp2m entries propagated properly.
>
> Right, if there is some other place where it does sharing in the back we
> would have to propagate that change.
>
>> This sort of mistake is easy to make, which is why I think we should
>> try to always update the altp2ms in ept_set_entry() if we can, to
>> minimize the opportunity for making this sort of mistake.
>>
>> Is there ever a reason to grab the altp2m lock and *then* grab the
>> sharing lock?  Could we just move the sharing lock up between the p2m
>> lock and the altp2mlist lock instead?
>>
>
> I can't think of a scenario where we would get to sharing from altp2m with
> altp2m locking first. Not sure what you mean by moving the sharing lock up
> though. The problem is that sharing already has the lock by the time altp2m
> tries to lock, so we could pass that info down to make altp2m aware it needs
> no locking. It would require extending a bunch of functions though with an
> extra input that is barely ever used..

If you have altp2m there are three locks.  There's one p2m lock for
the "host" p2m (that is, Xen's idea of what the mapping should look
like).  Then there's the altp2mlist lock, which protects the *list* of
altp2ms; then each altp2m itself has its own lock.  These are defined
in mm-lock.h and  must be grabbed in that order: p2m before
altp2mlist, altp2mlist before altp2m.

I assume that the memsharing code is grabbing the hostp2m lock (it
should be anyway), then grabbing the memsharing locks. This is allowed
because the memsharing locks are defined after the p2m lock in
mm-lock.h.  But then when updating the p2m entry, if you have an
altp2m active, it then tries to grab the altp2mlist lock so it can
iterate over the altp2ms.  Since the altp2mlist lock is *before* the
sharing lock in mm-lock.h, this triggers an assert.

Is that not what your issue is?

 -George

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

Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 32-bit default_ioport_* accesses

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH qemu-traditional] ioreq: Support 
32-bit default_ioport_* accesses"):
> On 25.05.16 at 17:36,  wrote:
> > AccesSize parameter is optional when invoking the Register macro. If the
> > AccessSize parameter is
> > not supplied then the AccessSize field will be set to zero. In this
> > case, OSPM will assume the access
> > size.
> > 
> > I don't think I understand what the last sentence means. Does it imply
> > that SW can do whatever it thinks is appropriate?
> 
> I think so, yes.

I think this question can only be resolved de jure by looking at what
previous ACPI specifications (before this AccessSize field) said.

But, I think: de facto, what is going on here is that ACPICA and hence
Linux have changed their behaviour in a way that is not compatible
with at least some existing "hardware".  Is this not arguably a
compatibility defect Linux ?

It would surely be better to make Linux do whatever it did before,
when AccessSize is not supplied.  That will avoid breaking any other
things (whether or not those other things are de jure broken according
to previous specs).  It will also avoid us having to make changes our
ACPI tables which themselves come with a risk of compatibility
problems.

Ian.

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