Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Jan Beulich]

On 07.08.2019 16:31, Juergen Gross wrote:
> --- a/xen/common/spinlock.c
> +++ b/xen/common/spinlock.c
> @@ -13,9 +13,9 @@
>   
>   static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
>   
> -static void check_lock(struct lock_debug *debug)
> +static void check_lock(union lock_debug *debug)
>   {
> -int irq_safe = !local_irq_is_enabled();
> +unsigned short irq_safe = !local_irq_is_enabled();

bool? Seeing your heavy use of "unsigned short" I realize that
my CodingStyle change committed yesterday still wasn't precise
enough.

> @@ -43,18 +43,21 @@ static void check_lock(struct lock_debug *debug)
>  */
>  if ( unlikely(debug->irq_safe != irq_safe) )
>  {
> -int seen = cmpxchg(&debug->irq_safe, -1, irq_safe);
> +union lock_debug seen, new = { 0 };
>   
> -if ( seen == !irq_safe )
> +new.irq_safe = irq_safe;
> +seen.val = cmpxchg(&debug->val, 0x, new.val);

This 0x should be connected (by way of at least a #define) to
the one used in _LOCK_DEBUG.

> +if ( !seen.unused && seen.irq_safe == !irq_safe )

While "unused" makes sufficient sense here, ...

> --- a/xen/include/xen/spinlock.h
> +++ b/xen/include/xen/spinlock.h
> @@ -7,14 +7,20 @@
>   #include 
>   
>   #ifndef NDEBUG
> -struct lock_debug {
> -s16 irq_safe; /* +1: IRQ-safe; 0: not IRQ-safe; -1: don't know yet */
> +union lock_debug {
> +unsigned short val;
> +struct {
> +unsigned short unused:1;

... it gives a rather misleading impression of this being an unused
field here. How about e.g. "unseen" instead?

> +unsigned short irq_safe:1;
> +unsigned short pad:2;
> +unsigned short cpu:12;
> +};
>   };

Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 3/6] [RFC] xen/common: Introduce _xrealloc function

[Jan Beulich]

(I'm sorry if you receive duplicates of this, but I've got a reply
back from our mail system that several of the recipients did not
have their host names resolved correctly on the first attempt.)

On 07.08.2019 20:36, Oleksandr wrote:
>> There's one more thing for the re-alloc case though (besides
>> cosmetic aspects): The incoming pointer should also be verified
>> to be of correct type.
> 
> Jan, how this could be technically implemented, or are these any existing 
> examples in Xen?

See x86's copy_to_guest_offset(), for example. To get the compiler
to emit a warning (at least), a (typically otherwise dead)
comparison of pointers is commonly used.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC] Re-working the patch submission guide

[Viktor Mitin]

On Thu, Aug 8, 2019 at 9:45 AM Lars Kurth  wrote:

> On the one hand, the testing relates to patches
> submission (mean patches should be tested before submission anyway),
> but on the other hand, testing details are not about the patches
> submission process. In any case, we do not have any explicit
> documentation about patches testing for now. Is it correct?
>
> No, not really.
> OSSTEST can be run locally but is hard to do
> XTF would be a good option, but does not work on Arm

What is XTF?
One more option is to use Qemu for Xen x86 and/or Arm manual/automatic
tests. It is not hard to do, but it is not documented explicitly for
now.

Thanks

>
> Thank you again for improving Xen documentation.
>
> You are welcome
>
> Lars
>

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen: print lock profile info in panic()

[Jan Beulich]

On 07.08.2019 16:31, Juergen Gross wrote:
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -53,6 +53,7 @@ config SPINLOCK_DEBUG
>   
>   config LOCK_PROFILE
>   bool "Lock Profiling"
> + select SPINLOCK_DEBUG
>   ---help---
> Lock profiling allows you to see how often locks are taken and 
> blocked.
> You can use serial console to print (and reset) using 'l' and 'L'

In which case, for sensible user experience, the selected
option should come after this one. The way it is now afaict
there'll be a prompt for SPINLOCK_DEBUG, the user may say
"no", just to find that because of saying "yes" here it'll
be turned on anyway. Whereas with switched ordering there'll
be no prompt for the debug option at all (again afaict) if
the profiling option was set to "yes".

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

[Juergen Gross]

On 08.08.19 08:34, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:

--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -44,6 +44,13 @@ config COVERAGE
    If unsure, say N here.
+config SPINLOCK_DEBUG
+    bool "Spinlock debugging"
+    default DEBUG
+    ---help---
+  Enable debugging features of spinlock handling.  Some additional
+  checks will be performed when acquiring and releasing locks.
+
  config LOCK_PROFILE


While the pre-existing LOCK_PROFILE suggests the opposite, I'd
still like to propose that we uniformly name all debugging
options CONFIG_DEBUG_* (rather than having the DEBUG at the
end). Thoughts?


Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm
touching it anyway.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen: modify lock profiling interface

[Jan Beulich]

On 07.08.2019 16:31, Juergen Gross wrote:
> @@ -463,31 +464,67 @@ int spinlock_profile_control(struct 
> xen_sysctl_lockprof_op *pc)
>   return rc;
>   }
>   
> -void _lock_profile_register_struct(
> -int32_t type, struct lock_profile_qhead *qhead, int32_t idx, char *name)
> +static struct lock_profile_anc *find_prof_anc(const char *name)
>   {
> -qhead->idx = idx;
> +struct lock_profile_anc *anc;
> +
> +for ( anc = lock_profile_ancs; anc; anc = anc->next )
> +if ( !strcmp(anc->name, name) )
> +return anc;
> +return NULL;
> +}
> +
> +void _lock_profile_register_struct(struct lock_profile_qhead *qhead,
> +   int32_t idx, const char *name)
> +{
> +struct lock_profile_anc *anc;
> +
>   spin_lock(&lock_profile_lock);
> -qhead->head_q = lock_profile_ancs[type].head_q;
> -lock_profile_ancs[type].head_q = qhead;
> -lock_profile_ancs[type].name = name;
> +
> +anc = find_prof_anc(name);
> +if ( !anc )
> +{
> +anc = xzalloc(struct lock_profile_anc);
> +anc->name = name;
> +anc->next = lock_profile_ancs;
> +lock_profile_ancs = anc;
> +}

This is an imo fundamental weakness of the new model: You now
require a dynamic memory allocation (of which you don't even
check that it was successful). Right now the path above will
only be taken at boot time, but that's not stated anywhere as
a restriction (afaics), and hence doesn't need to remain this
way.

Furthermore "name" now serves two purposes when previously it
served just one. This is best noticeable with the function's
use in domain_create(): Previously that set up the class
"per-domain" with an initial instance "struct domain". This
did provide for someone later going and also registering
another per-domain structure (e.g. struct p2m_domain) as
another "per-domain" class instance. Then again maybe I'm not
correctly understanding the original intentions ...

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC] Re-working the patch submission guide

[Andrew Cooper]

On 08/08/2019 08:07, Viktor Mitin wrote:
> On Thu, Aug 8, 2019 at 9:45 AM Lars Kurth  wrote:
>
>> On the one hand, the testing relates to patches
>> submission (mean patches should be tested before submission anyway),
>> but on the other hand, testing details are not about the patches
>> submission process. In any case, we do not have any explicit
>> documentation about patches testing for now. Is it correct?
>>
>> No, not really.
>> OSSTEST can be run locally but is hard to do
>> XTF would be a good option, but does not work on Arm
> What is XTF?

http://xenbits.xen.org/docs/xtf/

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Jan Beulich]

On 08.08.2019 02:52, Marek Marczykowski-Górecki  wrote:
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -315,8 +315,10 @@ static void __init efi_arch_handle_cmdline(CHAR16 
>> *image_name,
>>   name.s = "xen";
>>   place_string(&mbi.cmdline, name.s);
>>   
>> -if ( mbi.cmdline )
>> +if ( mbi.cmdline ) {
>>   mbi.flags |= MBI_CMDLINE;
>> +efi_early_parse_cmdline(mbi.cmdline);
> 
> Compiler complains here, because mbi.cmdline is u32 (int vs pointer, and
> also a different size). What is the proper way to make compiler happy
> here? "(const char *)(uint64_t)" doesn't seems right.

Well, if the conversion to a pointer is correct here (i.e.
the specified address has a mapping), then
(const char*)(unsigned long) would be the canonical way to
go; using uintptr_t would also be an option, but we make
pretty little use of it.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

[Jan Beulich]

On 08.08.2019 09:23, Juergen Gross wrote:
> On 08.08.19 08:34, Jan Beulich wrote:
>> On 07.08.2019 16:31, Juergen Gross wrote:
>>> --- a/xen/Kconfig.debug
>>> +++ b/xen/Kconfig.debug
>>> @@ -44,6 +44,13 @@ config COVERAGE
>>>     If unsure, say N here.
>>> +config SPINLOCK_DEBUG
>>> +    bool "Spinlock debugging"
>>> +    default DEBUG
>>> +    ---help---
>>> +  Enable debugging features of spinlock handling.  Some additional
>>> +  checks will be performed when acquiring and releasing locks.
>>> +
>>>   config LOCK_PROFILE
>>
>> While the pre-existing LOCK_PROFILE suggests the opposite, I'd
>> still like to propose that we uniformly name all debugging
>> options CONFIG_DEBUG_* (rather than having the DEBUG at the
>> end). Thoughts?
> 
> Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm
> touching it anyway.

One more thing: Perhaps it would better be DEBUG_LOCK (i.e.
without "SPIN") or DEBUG_LOCKS, to also allow it to cover r/w
locks, should anyone want to instrument them as well.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Juergen Gross]

On 08.08.19 08:58, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:

--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -13,9 +13,9 @@
   
   static atomic_t spin_debug __read_mostly = ATOMIC_INIT(0);
   
-static void check_lock(struct lock_debug *debug)

+static void check_lock(union lock_debug *debug)
   {
-int irq_safe = !local_irq_is_enabled();
+unsigned short irq_safe = !local_irq_is_enabled();


bool? Seeing your heavy use of "unsigned short" I realize that
my CodingStyle change committed yesterday still wasn't precise
enough.


I wanted to be consistent with the type used in the structure.
I can switch to bool if you like that better.




@@ -43,18 +43,21 @@ static void check_lock(struct lock_debug *debug)
  */
  if ( unlikely(debug->irq_safe != irq_safe) )
  {
-int seen = cmpxchg(&debug->irq_safe, -1, irq_safe);
+union lock_debug seen, new = { 0 };
   
-if ( seen == !irq_safe )

+new.irq_safe = irq_safe;
+seen.val = cmpxchg(&debug->val, 0x, new.val);


This 0x should be connected (by way of at least a #define) to
the one used in _LOCK_DEBUG.


Okay.




+if ( !seen.unused && seen.irq_safe == !irq_safe )


While "unused" makes sufficient sense here, ...


--- a/xen/include/xen/spinlock.h
+++ b/xen/include/xen/spinlock.h
@@ -7,14 +7,20 @@
   #include 
   
   #ifndef NDEBUG

-struct lock_debug {
-s16 irq_safe; /* +1: IRQ-safe; 0: not IRQ-safe; -1: don't know yet */
+union lock_debug {
+unsigned short val;
+struct {
+unsigned short unused:1;


... it gives a rather misleading impression of this being an unused
field here. How about e.g. "unseen" instead?


Yes, that seems to be the better choice.




+unsigned short irq_safe:1;
+unsigned short pad:2;
+unsigned short cpu:12;
+};
   };


Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.


But that would increase the size of the union to 4 bytes instead of 2.
So at least pad and cpu must be unsigned short or (better) uint16_t.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Jan Beulich]

On 08.08.2019 09:51, Juergen Gross wrote:
> On 08.08.19 08:58, Jan Beulich wrote:
>> On 07.08.2019 16:31, Juergen Gross wrote:
>>> +    unsigned short irq_safe:1;
>>> +    unsigned short pad:2;
>>> +    unsigned short cpu:12;
>>> +    };
>>>    };
>>
>> Do we have an implied assumption somewhere that unsigned short is
>> exactly 16 bits wide? I think "val" wants to be uint16_t here (as
>> you really mean "exactly 16 bits"), the two boolean fields want
>> to be bool, and the remaining two ones unsigned int.
> 
> But that would increase the size of the union to 4 bytes instead of 2.
> So at least pad and cpu must be unsigned short or (better) uint16_t.

Oh, right.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] dom0less + sched=null => broken in staging

[George Dunlap]

On 8/7/19 7:22 PM, Stefano Stabellini wrote:
> Hi Dario, George,
> 
> Dom0less with sched=null is broken on staging, it simply hangs soon
> after Xen is finished loading things. My impression is that vcpus are
> not actually started. I did a git bisection and it pointed to:
> 
> commit d545f1d6c2519a183ed631cfca7aff0baf29fde5 (refs/bisect/bad)
> Author: Dario Faggioli 
> Date:   Mon Aug 5 11:50:55 2019 +0100
> 
> xen: sched: deal with vCPUs being or becoming online or offline

That's Dario's patch -- Dario, can you take a look?

Stefano, how urgent is it for things to work for you -- i.e., at what
point do you want to consider reverting the patch?

 -George

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] xen: modify lock profiling interface

[Juergen Gross]

On 08.08.19 09:32, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:

@@ -463,31 +464,67 @@ int spinlock_profile_control(struct 
xen_sysctl_lockprof_op *pc)
   return rc;
   }
   
-void _lock_profile_register_struct(

-int32_t type, struct lock_profile_qhead *qhead, int32_t idx, char *name)
+static struct lock_profile_anc *find_prof_anc(const char *name)
   {
-qhead->idx = idx;
+struct lock_profile_anc *anc;
+
+for ( anc = lock_profile_ancs; anc; anc = anc->next )
+if ( !strcmp(anc->name, name) )
+return anc;
+return NULL;
+}
+
+void _lock_profile_register_struct(struct lock_profile_qhead *qhead,
+   int32_t idx, const char *name)
+{
+struct lock_profile_anc *anc;
+
   spin_lock(&lock_profile_lock);
-qhead->head_q = lock_profile_ancs[type].head_q;
-lock_profile_ancs[type].head_q = qhead;
-lock_profile_ancs[type].name = name;
+
+anc = find_prof_anc(name);
+if ( !anc )
+{
+anc = xzalloc(struct lock_profile_anc);
+anc->name = name;
+anc->next = lock_profile_ancs;
+lock_profile_ancs = anc;
+}


This is an imo fundamental weakness of the new model: You now
require a dynamic memory allocation (of which you don't even
check that it was successful). Right now the path above will
only be taken at boot time, but that's not stated anywhere as
a restriction (afaics), and hence doesn't need to remain this
way.


Yes, allocation success must be checked, of course.

Adding a memory allocation in the path should be no real problem,
as normally registering a struct is accompanied by registering
the locks in that struct, which is already doing some memory
allocation. I should mention that in the comment section for the
usage of lock profiling.


Furthermore "name" now serves two purposes when previously it
served just one. This is best noticeable with the function's
use in domain_create(): Previously that set up the class
"per-domain" with an initial instance "struct domain". This
did provide for someone later going and also registering
another per-domain structure (e.g. struct p2m_domain) as
another "per-domain" class instance. Then again maybe I'm not
correctly understanding the original intentions ...


While the printout might be not as clear as desired, it will still
be correct. In both cases the "print" value will be used. The now
omitted "type" parameter was only used to decide whether to print
the index and for knowing what to print in the xenlockprof tool.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Jan Beulich]

On 08.08.2019 02:31, Marek Marczykowski-Górecki  wrote:
> When booting Xen via xen.efi, there is /mapbs option to workaround
> certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> cmdline for the same effect and parse it very early in the
> multiboot2+EFI boot path.
> 
> Normally cmdline is parsed after relocating MB2 structure, which happens
> too late. To have efi= parsed early enough, save cmdline pointer in
> head.S and pass it as yet another argument to efi_multiboot2(). This
> way we avoid introducing yet another MB2 structure parser.

I can where you're coming from here, but I'm not at all happy to
see the amount of assembly code further grow.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -886,7 +886,7 @@ disable it (edid=no). This option should not normally be 
> required
>   except for debugging purposes.
>   
>   ### efi
> -= List of [ rs=, attr=no|uc ]
> += List of [ rs=, attr=no|uc, mapbs= ]
>   
>   Controls for interacting with the system Extended Firmware Interface.
>   
> @@ -899,6 +899,10 @@ Controls for interacting with the system Extended 
> Firmware Interface.
>   leave the memory regions unmapped, while `attr=uc` will map them as 
> fully
>   uncacheable.
>   
> +*   The `mapbs=` boolean controls whether EfiBootServices{Code,Data} should
> +remain mapped after Exit() BootServices call. By default those memory 
> regions
> +will not be mapped after Exit() BootServices call.

There are restrictions necessary (see below) which should be
mentioned here imo.

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -315,8 +315,10 @@ static void __init efi_arch_handle_cmdline(CHAR16 
> *image_name,
>   name.s = "xen";
>   place_string(&mbi.cmdline, name.s);
>   
> -if ( mbi.cmdline )
> +if ( mbi.cmdline ) {
>   mbi.flags |= MBI_CMDLINE;
> +efi_early_parse_cmdline(mbi.cmdline);
> +}

Why? This is the xen.efi boot path, isn't it? (Also, if this
change was to stay, the opening brace would need to go on its
own line.)

> @@ -685,6 +688,9 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTable
>   
>   efi_init(ImageHandle, SystemTable);
>   
> +if (cmdline)
> +efi_early_parse_cmdline(cmdline);

Style again (missing blanks in if()).

> @@ -1412,16 +1417,32 @@ static int __init parse_efi_param(const char *s)
>  else
>  rc = -EINVAL;
>  }
> +else if ( (val = parse_boolean("mapbs", s, ss)) >= 0 )
> +{
> +map_bs = val;
> +}

This may _not_ be accepted when called the "normal" way, since it
would then fail to affect efi_arch_process_memory_map(), but it
would affect efi_init_memory(). I therefore think you don't want
to call this function from efi_early_parse_cmdline(), and instead
simply ignore the option here.

Also (again if for some reason the change was to stay as is) -
stray braces.

>  else
>  rc = -EINVAL;
>  
>  s = ss + 1;
> -} while ( *ss );
> +/*
> + * End parsing on both '\0' and ' ',
> + * to make efi_early_parse_cmdline simpler.
> + */
> +} while ( *ss && *ss != ' ');
>   
>  return rc;
>  }
>  custom_param("efi", parse_efi_param);
>   
> +/* Extract efi= param early in the boot */
> +static void __init efi_early_parse_cmdline(const char *cmdline)
> +{
> +const char *efi_opt = strstr(cmdline, "efi=");
> +if ( efi_opt )

Blank line missing above here.

> +parse_efi_param(efi_opt + 4);
> +}

What about multiple "efi=" on the command line? And what about
a (currently bogus) "abcefi=" on the command line, or yet some
other pattern wrongly matching the string you search for?

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/4] xen: add new CONFIG_SPINLOCK_DEBUG option

[Juergen Gross]

On 08.08.19 09:35, Jan Beulich wrote:

On 08.08.2019 09:23, Juergen Gross wrote:

On 08.08.19 08:34, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:

--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -44,6 +44,13 @@ config COVERAGE
     If unsure, say N here.
+config SPINLOCK_DEBUG
+    bool "Spinlock debugging"
+    default DEBUG
+    ---help---
+  Enable debugging features of spinlock handling.  Some additional
+  checks will be performed when acquiring and releasing locks.
+
   config LOCK_PROFILE


While the pre-existing LOCK_PROFILE suggests the opposite, I'd
still like to propose that we uniformly name all debugging
options CONFIG_DEBUG_* (rather than having the DEBUG at the
end). Thoughts?


Fine with me. I can rename the LOCK_PROFILE option in patch 4 as I'm
touching it anyway.


One more thing: Perhaps it would better be DEBUG_LOCK (i.e.
without "SPIN") or DEBUG_LOCKS, to also allow it to cover r/w
locks, should anyone want to instrument them as well.


Yes. I'll switch to DEBUG_LOCKS.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/4] xen: print lock profile info in panic()

[Juergen Gross]

On 08.08.19 09:09, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:

--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -53,6 +53,7 @@ config SPINLOCK_DEBUG
   
   config LOCK_PROFILE

bool "Lock Profiling"
+   select SPINLOCK_DEBUG
---help---
  Lock profiling allows you to see how often locks are taken and 
blocked.
  You can use serial console to print (and reset) using 'l' and 'L'


In which case, for sensible user experience, the selected
option should come after this one. The way it is now afaict
there'll be a prompt for SPINLOCK_DEBUG, the user may say
"no", just to find that because of saying "yes" here it'll
be turned on anyway. Whereas with switched ordering there'll
be no prompt for the debug option at all (again afaict) if
the profiling option was set to "yes".


Reordering is fine with me.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Andrew Cooper]

On 08/08/2019 05:50, Juergen Gross wrote:
> On 07.08.19 20:11, Andrew Cooper wrote:
>>
>> 
>> Its not exactly the easiest to dump to follow.
>>
>> First of all - I don't see why the hold/block time are printed like
>> that.  It
>> might be a holdover from the 32bit build, pre PRId64 support.  They
>> should
>> probably use PRI_stime anyway.
>
> Fine with me.
>
>> The domid rendering is unfortunate.  Ideally we'd use %pd but that would
>> involve rearranging the logic to get a struct domain* in hand. 
>> Seeing as
>> you're the last person to modify this code, how hard would that be to
>> do?
>
> It would completely break the struct type agnostic design.

Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
would be a trivial adjustment in the vsnprintf code, and could plausibly
be useful elsewhere where we have a domid and not a domain pointer.

>
> I have a debug patch adding credit2 run-queue lock. It requires to just
> add 5 lines of code (and this includes moving the spinlock_init() out of
> an irq-off section). It will produce:
>
> (XEN) credit2-runq 0 lock: addr=830413351010, lockval=de00ddf, cpu=6
> (XEN)   lock: 896287(:00FAA6B2), block:  819(:9C80)

What is the 0 here?

>
>> We have several global locks called lock:
>>
>>    (XEN) Global lock: addr=82d0808181e0, lockval=10001, cpu=4095
>>    (XEN)   lock:   1(:01322165), block:   
>> 0(:)
>>    (XEN) Global lock: addr=82d080817cc0, lockval=100010, cpu=4095
>>    (XEN)   lock:   0(:), block:   
>> 0(:)
>>    (XEN) Global lock: addr=82d080817800, lockval=, cpu=4095
>>    (XEN)   lock:   0(:), block:   
>> 0(:)
>>    (XEN) Global lock: addr=82d080817780, lockval=, cpu=4095
>>    (XEN)   lock:   0(:), block:   
>> 0(:)
>>    (XEN) Global lock: addr=82d080817510, lockval=, cpu=4095
>>    (XEN)   lock:   0(:), block:   
>> 0(:)
>>
>> The second one in particular has corrupt data, seeing has it has been
>> taken
>> and released several times without lock_cnt increasing.
>
> The lock might have been taken/released before lock profiling has been
> initialized.

What is there to initialise?  It all looks statically set up.

>
>> For sanity sake, we should enforce unique naming of any lock
>> registered for
>> profiling.
>
> This would be every lock inited via DEFINE_SPINLOCK(). I can do a
> followup patch for that purpose.

I was wondering how to do this.  One option which comes to mind is to
put an non-static object into .discard.lock_profile or something, so the
linker will object to repeated symbol names and then throw all of them away.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Andrew Cooper]

On 08/08/2019 07:22, Jan Beulich wrote:
> On 07.08.2019 21:41, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/docs/glossary.rst
>> @@ -0,0 +1,37 @@
>> +Glossary
>> +
>> +
>> +.. Terms should appear in alphabetical order
>> +
>> +.. glossary::
>> +
>> +   control domain
>> + A :term:`domain`, commonly dom0, with the permission and
>> responsibility
>> + to create and manage other domains on the system.
>> +
>> +   domain
>> + A domain is Xen's unit of resource ownership, and generally has
>> at the
>> + minimum some RAM and virtual CPUs.
>> +
>> + The terms :term:`domain` and :term:`guest` are commonly used
>> + interchangeably, but they mean subtly different things.
>> +
>> + A guest is a single virtual machine.
>> +
>> + Consider the case of live migration where, for a period of
>> time, one
>> + guest will be comprised of two domains, while it is in transit.
>> +
>> +   domid
>> + The numeric identifier of a running :term:`domain`.  It is
>> unique to a
>> + single instance of Xen, used as the identifier in various APIs,
>> and is
>> + typically allocated sequentially from 0.
>> +
>> +   guest
>> + See :term:`domain`
>
> I think you want to mention the usual distinction here: Dom0 is,
> while a domain, commonly not considered a guest.

To be honest, I had totally forgotten about that.  I guess now is the
proper time to rehash it in public.

I don't think the way it currently gets used has a clear or coherent set
of rules, because I can't think of any to describe how it does get used.

Either there are a clear and coherent (and simple!) set of rules for
what we mean by "guest", at which point they can live here in the
glossary, or the fuzzy way it is current used should cease.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Julien Grall]

On 07/08/2019 21:28, Oleksandr Tyshchenko wrote:

Hi, Julien.


Hi,


Sorry for the possible format issues.


 > No, it is not disabled. But, ipmmu_irq() uses another mmu->lock. So, I
 > think, there won't be a deadlock.
 >
 > Or I really missed something?
 >
 > If we worry about ipmmu_tlb_invalidate() which is called here (to
 > perform a flush by request from P2M code, which manages a page table)
 > and from the irq handler (to perform a flush to resume address
 > translation), I could use a tasklet to schedule ipmmu_tlb_invalidate()
 > from the irq handler then. This way we would get this serialized. What
 > do you think?

I am afraid a tasklet is not an option. You need to perform the TLB
flush when requested otherwise you are introducing a security issue.

This is because as soon as a region is unmapped in the page table, we
remove the drop the reference on any page backing that region. When the
reference is dropped to zero, the page can be reallocated to another
domain or even Xen. If the TLB flush happen after, then the guest may
still be able to access the page for a short time if the translation has
been cached by the any TLB (IOMMU, Processor).



I understand this. I am not proposing to delay a requested by P2M code TLB flush 
in any case. I just propose to issue TLB flush (which we have to perform in case 
of page faults, to resolve error condition and resume translations) from a 
tasklet rather than from interrupt handler directly. This is the TLB flush I am 
speaking about:


https://github.com/otyshchenko1/xen/blob/ipmmu_upstream2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L598

Sorry if I was unclear.


My mistake, I misread what you wrote.

I found the flush in the renesas-bsp and not Linux upstream but it is not clear 
why this is actually required. You are not fixing any translation error. So what 
this flush will do?


Regarding the placement of the flush, then if you execute in a tasklet it will 
likely be done later on when the IRQ has been acknowledge. What's the 
implication to delay it?


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Jan Beulich]

On 08.08.2019 10:43, Andrew Cooper wrote:
> On 08/08/2019 07:22, Jan Beulich wrote:
>> On 07.08.2019 21:41, Andrew Cooper wrote:
>>> --- /dev/null
>>> +++ b/docs/glossary.rst
>>> @@ -0,0 +1,37 @@
>>> +Glossary
>>> +
>>> +
>>> +.. Terms should appear in alphabetical order
>>> +
>>> +.. glossary::
>>> +
>>> +   control domain
>>> + A :term:`domain`, commonly dom0, with the permission and
>>> responsibility
>>> + to create and manage other domains on the system.
>>> +
>>> +   domain
>>> + A domain is Xen's unit of resource ownership, and generally has
>>> at the
>>> + minimum some RAM and virtual CPUs.
>>> +
>>> + The terms :term:`domain` and :term:`guest` are commonly used
>>> + interchangeably, but they mean subtly different things.
>>> +
>>> + A guest is a single virtual machine.
>>> +
>>> + Consider the case of live migration where, for a period of
>>> time, one
>>> + guest will be comprised of two domains, while it is in transit.
>>> +
>>> +   domid
>>> + The numeric identifier of a running :term:`domain`.  It is
>>> unique to a
>>> + single instance of Xen, used as the identifier in various APIs,
>>> and is
>>> + typically allocated sequentially from 0.
>>> +
>>> +   guest
>>> + See :term:`domain`
>>
>> I think you want to mention the usual distinction here: Dom0 is,
>> while a domain, commonly not considered a guest.
> 
> To be honest, I had totally forgotten about that.  I guess now is the
> proper time to rehash it in public.
> 
> I don't think the way it currently gets used has a clear or coherent set
> of rules, because I can't think of any to describe how it does get used.
> 
> Either there are a clear and coherent (and simple!) set of rules for
> what we mean by "guest", at which point they can live here in the
> glossary, or the fuzzy way it is current used should cease.

What's fuzzy about Dom0 not being a guest (due to being a part of the
host instead)?

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [freebsd-master test] 139801: all pass - PUSHED

[osstest service owner]

flight 139801 freebsd-master real [real]
http://logs.test-lab.xenproject.org/osstest/logs/139801/

Perfect :-)
All tests in this flight passed as required
version targeted for testing:
 freebsd  37b420014fdd0f6e1ea5aecd05f04ed0a33fa5b7
baseline version:
 freebsd  17a1fc80d578803ae6318e7e297fc0b5605fba29

Last test of basis   139729  2019-08-05 09:19:34 Z2 days
Testing same since   139801  2019-08-07 09:20:46 Z0 days1 attempts


People who touched revisions under test:
  asomers 
  bz 
  cem 
  delphij 
  emaste 
  glebius 
  ian 
  imp 
  jeff 
  jhb 
  jhibbits 
  jilles 
  kevans 
  kib 
  luporl 
  markj 
  mav 
  mckusick 
  ngie 
  oshogbo 
  thj 
  tsoome 
  tuexen 
  vangyzen 

jobs:
 build-amd64-freebsd-againpass
 build-amd64-freebsd  pass
 build-amd64-xen-freebsd  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 :

To xenbits.xen.org:/home/xen/git/freebsd.git
   17a1fc80d57..37b420014fd  37b420014fdd0f6e1ea5aecd05f04ed0a33fa5b7 -> 
tested/master

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V4] tools/libxl: Add iothread support for COLO

[Anthony PERARD]

On Thu, Aug 08, 2019 at 02:44:42AM +, Zhang, Chen wrote:
> Ping...
> Any comments?

For your information, that patch have been pushed, see commit
174db28bb823e8c98c319fdbdc6d4cbe1050ba14 ;-)

Cheers,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Jan Beulich]

On 08.08.2019 10:33, Andrew Cooper wrote:
> On 08/08/2019 05:50, Juergen Gross wrote:
>> On 07.08.19 20:11, Andrew Cooper wrote:
>>>
>>> 
>>> Its not exactly the easiest to dump to follow.
>>>
>>> First of all - I don't see why the hold/block time are printed like
>>> that.  It
>>> might be a holdover from the 32bit build, pre PRId64 support.  They
>>> should
>>> probably use PRI_stime anyway.
>>
>> Fine with me.
>>
>>> The domid rendering is unfortunate.  Ideally we'd use %pd but that would
>>> involve rearranging the logic to get a struct domain* in hand.
>>> Seeing as
>>> you're the last person to modify this code, how hard would that be to
>>> do?
>>
>> It would completely break the struct type agnostic design.
> 
> Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
> would be a trivial adjustment in the vsnprintf code, and could plausibly
> be useful elsewhere where we have a domid and not a domain pointer.

At the risk of upsetting / annoying you: A domid_t would again
better be formatted via %od (and without the need to take the
address of a respective variable). In the case here it would
have the minor additional benefit of conserving on format string
length.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Julien Grall]

Hi Jan,

On 08/08/2019 10:04, Jan Beulich wrote:

On 08.08.2019 10:43, Andrew Cooper wrote:

On 08/08/2019 07:22, Jan Beulich wrote:

On 07.08.2019 21:41, Andrew Cooper wrote:

--- /dev/null
+++ b/docs/glossary.rst
@@ -0,0 +1,37 @@
+Glossary
+
+
+.. Terms should appear in alphabetical order
+
+.. glossary::
+
+   control domain
+ A :term:`domain`, commonly dom0, with the permission and
responsibility
+ to create and manage other domains on the system.
+
+   domain
+ A domain is Xen's unit of resource ownership, and generally has
at the
+ minimum some RAM and virtual CPUs.
+
+ The terms :term:`domain` and :term:`guest` are commonly used
+ interchangeably, but they mean subtly different things.
+
+ A guest is a single virtual machine.
+
+ Consider the case of live migration where, for a period of
time, one
+ guest will be comprised of two domains, while it is in transit.
+
+   domid
+ The numeric identifier of a running :term:`domain`.  It is
unique to a
+ single instance of Xen, used as the identifier in various APIs,
and is
+ typically allocated sequentially from 0.
+
+   guest
+ See :term:`domain`


I think you want to mention the usual distinction here: Dom0 is,
while a domain, commonly not considered a guest.


To be honest, I had totally forgotten about that.  I guess now is the
proper time to rehash it in public.

I don't think the way it currently gets used has a clear or coherent set
of rules, because I can't think of any to describe how it does get used.

Either there are a clear and coherent (and simple!) set of rules for
what we mean by "guest", at which point they can live here in the
glossary, or the fuzzy way it is current used should cease.


What's fuzzy about Dom0 not being a guest (due to being a part of the
host instead)?
Dom0 is not part of the host if you are using an hardware domain. I actually 
thought we renamed everything in Xen from Dom0 to hwdom to avoid the confusion.


I also would rather avoid to treat "dom0" as not a guest. In normal setup this 
is a more privilege guest, in other setup this may just be a normal guest (think 
about partitioning).


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/5] xen/arm: use the physical number of gic lines for boot domains

[Julien Grall]

Hi Stefano,

On 07/08/2019 19:42, Stefano Stabellini wrote:

On Tue, 15 Jan 2019, Julien Grall wrote:

Hi Stefano,

On 1/3/19 7:07 PM, Stefano Stabellini wrote:

On Mon, 24 Dec 2018, Julien Grall wrote:

Hi,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:

We don't have a clear way to know how many virtual SPIs we need for the
boot domains. For simplicity, allocate as many as natively supported,
just like for dom0.


This will potentially allocate a lot of unused interrupts and a waste of
memory. So is it the correct solution?

For instance, we would request the user to provide the number of
interrupts.


Unfortunately, this has to happen much earlier than when we parse user
supplied device tree options. We could make it an hypervisor command
line parameter but it would be nasty.


Why does this value needs to be in the supplied device-tree. Can't it be part
of the node in the host DT?


You mean as an option under the xen,domain node? Yes, I think that would
be possible. Like:

  nr_spis = <0x64>;

And if it is missing we default back to gic_number_lines() - 32. That
could be a good idea.


Yes something like that. I am not necessarily asking to have this option for 
this series. But at least the commit message should not say it is difficult to 
do it.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Marek Marczykowski-Górecki]

On Thu, Aug 08, 2019 at 08:21:54AM +, Jan Beulich wrote:
> On 08.08.2019 02:31, Marek Marczykowski-Górecki  wrote:
> > When booting Xen via xen.efi, there is /mapbs option to workaround
> > certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> > map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> > cmdline for the same effect and parse it very early in the
> > multiboot2+EFI boot path.
> > 
> > Normally cmdline is parsed after relocating MB2 structure, which happens
> > too late. To have efi= parsed early enough, save cmdline pointer in
> > head.S and pass it as yet another argument to efi_multiboot2(). This
> > way we avoid introducing yet another MB2 structure parser.
> 
> I can where you're coming from here, but I'm not at all happy to
> see the amount of assembly code further grow.

I need to add some anyway, because otherwise efi_multiboot2() don't have
pointer to MB2 structure. But yes, it would probably be less new asm
code. Just to be clear: do you prefer third MB2 parser instead of adding
this into the one in head.S?

> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -886,7 +886,7 @@ disable it (edid=no). This option should not normally 
> > be required
> >   except for debugging purposes.
> >   
> >   ### efi
> > -= List of [ rs=, attr=no|uc ]
> > += List of [ rs=, attr=no|uc, mapbs= ]
> >   
> >   Controls for interacting with the system Extended Firmware Interface.
> >   
> > @@ -899,6 +899,10 @@ Controls for interacting with the system Extended 
> > Firmware Interface.
> >   leave the memory regions unmapped, while `attr=uc` will map them as 
> > fully
> >   uncacheable.
> >   
> > +*   The `mapbs=` boolean controls whether EfiBootServices{Code,Data} should
> > +remain mapped after Exit() BootServices call. By default those memory 
> > regions
> > +will not be mapped after Exit() BootServices call.
> 
> There are restrictions necessary (see below) which should be
> mentioned here imo.
> 
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -315,8 +315,10 @@ static void __init efi_arch_handle_cmdline(CHAR16 
> > *image_name,
> >   name.s = "xen";
> >   place_string(&mbi.cmdline, name.s);
> >   
> > -if ( mbi.cmdline )
> > +if ( mbi.cmdline ) {
> >   mbi.flags |= MBI_CMDLINE;
> > +efi_early_parse_cmdline(mbi.cmdline);
> > +}
> 
> Why? This is the xen.efi boot path, isn't it? 

Yes, as explained in commit message, this is to make it less confusing
what option can be used when. To say "efi=mapbs does X" instead of
"efi=mapbs does X, but only if Y, Z and K".

> (Also, if this
> change was to stay, the opening brace would need to go on its
> own line.)
> 
> > @@ -685,6 +688,9 @@ void __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> > EFI_SYSTEM_TABLE *SystemTable
> >   
> >   efi_init(ImageHandle, SystemTable);
> >   
> > +if (cmdline)
> > +efi_early_parse_cmdline(cmdline);
> 
> Style again (missing blanks in if()).
> 
> > @@ -1412,16 +1417,32 @@ static int __init parse_efi_param(const char *s)
> >  else
> >  rc = -EINVAL;
> >  }
> > +else if ( (val = parse_boolean("mapbs", s, ss)) >= 0 )
> > +{
> > +map_bs = val;
> > +}
> 
> This may _not_ be accepted when called the "normal" way, since it
> would then fail to affect efi_arch_process_memory_map(), but it
> would affect efi_init_memory().

What do you mean? Have I missed some EFI boot code path? Where it would
miss to affect efi_arch_process_memory_map() ?

> I therefore think you don't want
> to call this function from efi_early_parse_cmdline(), and instead
> simply ignore the option here.
> 
> Also (again if for some reason the change was to stay as is) -
> stray braces.
> 
> >  else
> >  rc = -EINVAL;
> >  
> >  s = ss + 1;
> > -} while ( *ss );
> > +/*
> > + * End parsing on both '\0' and ' ',
> > + * to make efi_early_parse_cmdline simpler.
> > + */
> > +} while ( *ss && *ss != ' ');
> >   
> >  return rc;
> >  }
> >  custom_param("efi", parse_efi_param);
> >   
> > +/* Extract efi= param early in the boot */
> > +static void __init efi_early_parse_cmdline(const char *cmdline)
> > +{
> > +const char *efi_opt = strstr(cmdline, "efi=");
> > +if ( efi_opt )
> 
> Blank line missing above here.
> 
> > +parse_efi_param(efi_opt + 4);
> > +}
> 
> What about multiple "efi=" on the command line? And what about
> a (currently bogus) "abcefi=" on the command line, or yet some
> other pattern wrongly matching the string you search for?

Good points, I'll extend this function. Unless you can suggest some
existing function that could be used this early instead?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-p

Re: [Xen-devel] [PATCH 5/5] xen/arm: add dom0less device assignment info to docs

[Julien Grall]

Hi Stefano,

On 07/08/2019 22:01, Stefano Stabellini wrote:

On Tue, 15 Jan 2019, Julien Grall wrote:

On 1/3/19 10:07 PM, Stefano Stabellini wrote:

On Mon, 24 Dec 2018, Julien Grall wrote:

Hi Stefano,

On 12/5/18 5:28 PM, Stefano Stabellini wrote:

Signed-off-by: Stefano Stabellini 
---
docs/misc/arm/device-tree/booting.txt | 108
++
1 file changed, 108 insertions(+)

diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index 317a9e9..f5aaf8f 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -226,3 +226,111 @@ chosen {
};
};
};
+
+
+Device Assignment
+=
+
+Device Assignment (Passthrough) is supported by adding another module,
+alongside the kernel and ramdisk, with the device tree fragment
+corresponding to the device node to assign to the guest.
+
+The dtb sub-node should have the following properties:
+
+- compatible
+
+"multiboot,dtb"


I would prefer "multiboot,device-tree"


I renamed it



+
+- reg
+
+Specifies the physical address of the device tree binary fragment
+RAM and its length.
+
+As an example:
+
+module@0xc00 {
+compatible = "multiboot,dtb", "multiboot,module";
+reg = <0x0 0xc00 0xff>;
+};
+
+The DTB fragment (loaded in memory at 0xc00 in the example above)
+should follow the convention explained in
docs/misc/arm/passthrough.txt.
+The DTB fragment will be added to the guest device tree, so that the
+guest kernel will be able to discover the device.
+
+In addition, the following properties for each device node in the
device
+tree fragment will be used for the device assignment setup:
+
+- reg
+
+  The reg property specifying the address and size of the device
memory.
+  The device memory will be automatically mapped to the guest domain
+  with a 1:1 mapping (pseudo-physical address == physical address).


As said in a previous patch, I don't think this is correct to impose 1:1.
The
user is neither in control of the HW memory map nor the Guest memory map.
So
not many people are going to be able to use it without hacking Xen.


Yes, I'll fix this (and a couple of other issues) by introducing a new
"xen,reg" property, instead of trying to reuse the existing reg
property.



+
+- interrupts
+
+  The interrupts property specifies the interrupt of the device. They
+  are automatically routed to the guest domain with virtual irqs ==
+  physical irqs.
+
+- interrupt-parent
+
+  It contains a reference to the interrupt controller node. It should
be
+  65000, corresponding to GUEST_PHANDLE_GIC.


We managed to get away in the toolstack with this property. So why do you
need
it for the hypervisor? Furthermore, this would forbid to passthrough any
other
interrupt controller to the guest.


The toolstack does use GUEST_PHANDLE_GIC today for passthrough
interrupts, see tools/libxl/libxl_arm.c:make_root_properties and
docs/misc/arm/passthrough.txt:

* The interrupt-parent property will be added by the toolstack in the
  root node;


You misunderstood my point here. The toolstack is adding the property for the
user. So why does you require the user to add this property for Dom0less case?


I did misunderstand. interrupt-parent came from the example I had at hand, 
which had
already the property even if it is unnecessary. I comfirmed that it is
superflous and I am happy to remove it.

FYI dtc throws a warning if interrupt-parent is missing:

: Warning (interrupts_property): Missing interrupt-parent for 
/passthrough/ethernet@ff0e

It makes me guess that is why it was added to the example I had.


Hmmm, I didn't remember DTC were throwing a warning.

What I want to avoid is writing in the documentation the phandle value. The 
value has been chosen in random and we have no guarantee the phandle will not be 
used by DTC in the future.


The solution I can think of is requesting the user to add the following snippet 
in the partial DT.


interrupt-parent = &gic;

gic {
};

This will let DTC to define the phandle. Xen can then lookup for the patch /gic 
and re-use the phandle for the guest GIC node.


What do you think?

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Juergen Gross]

On 08.08.19 10:33, Andrew Cooper wrote:

On 08/08/2019 05:50, Juergen Gross wrote:

On 07.08.19 20:11, Andrew Cooper wrote:



Its not exactly the easiest to dump to follow.

First of all - I don't see why the hold/block time are printed like
that.  It
might be a holdover from the 32bit build, pre PRId64 support.  They
should
probably use PRI_stime anyway.


Fine with me.


The domid rendering is unfortunate.  Ideally we'd use %pd but that would
involve rearranging the logic to get a struct domain* in hand.
Seeing as
you're the last person to modify this code, how hard would that be to
do?


It would completely break the struct type agnostic design.


Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
would be a trivial adjustment in the vsnprintf code, and could plausibly
be useful elsewhere where we have a domid and not a domain pointer.


Lock profiling has no knowledge that it is working on a struct domain.
It is just working on a lock in a struct and an index to differentiate
the struct instance. Using the domid as the index is just for making it
more easy to understand the printout.

I wouldn't want e.g. a per-event lock to be named "IDLE" just because
it happens to be index 32767.





I have a debug patch adding credit2 run-queue lock. It requires to just
add 5 lines of code (and this includes moving the spinlock_init() out of
an irq-off section). It will produce:

(XEN) credit2-runq 0 lock: addr=830413351010, lockval=de00ddf, cpu=6
(XEN)   lock: 896287(:00FAA6B2), block:  819(:9C80)


What is the 0 here?


The runq number.






We have several global locks called lock:

    (XEN) Global lock: addr=82d0808181e0, lockval=10001, cpu=4095
    (XEN)   lock:   1(:01322165), block:
0(:)
    (XEN) Global lock: addr=82d080817cc0, lockval=100010, cpu=4095
    (XEN)   lock:   0(:), block:
0(:)
    (XEN) Global lock: addr=82d080817800, lockval=, cpu=4095
    (XEN)   lock:   0(:), block:
0(:)
    (XEN) Global lock: addr=82d080817780, lockval=, cpu=4095
    (XEN)   lock:   0(:), block:
0(:)
    (XEN) Global lock: addr=82d080817510, lockval=, cpu=4095
    (XEN)   lock:   0(:), block:
0(:)

The second one in particular has corrupt data, seeing has it has been
taken
and released several times without lock_cnt increasing.


The lock might have been taken/released before lock profiling has been
initialized.


What is there to initialise?  It all looks statically set up.


lock->profile is set only in lock_prof_init().






For sanity sake, we should enforce unique naming of any lock
registered for
profiling.


This would be every lock inited via DEFINE_SPINLOCK(). I can do a
followup patch for that purpose.


I was wondering how to do this.  One option which comes to mind is to
put an non-static object into .discard.lock_profile or something, so the
linker will object to repeated symbol names and then throw all of them away.


I could just drop the "static" in the _LOCK_PROFILE_PTR() macro.
At the same time I should move the ".lockprofile.data" section in the
linker scripts to the init part maybe.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Andrew Cooper]

On 08/08/2019 01:31, Marek Marczykowski-Górecki wrote:
> When booting Xen via xen.efi, there is /mapbs option to workaround
> certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> cmdline for the same effect and parse it very early in the
> multiboot2+EFI boot path.
>
> Normally cmdline is parsed after relocating MB2 structure, which happens
> too late. To have efi= parsed early enough, save cmdline pointer in
> head.S and pass it as yet another argument to efi_multiboot2(). This
> way we avoid introducing yet another MB2 structure parser.
>
> To keep consistency, handle efi= parameter early in xen.efi too, both in
> xen.efi command line and cfg file.
>
> Signed-off-by: Marek Marczykowski-Górecki 

I'm very sorry to do this to you, but I'm going to object to the patch
(in principle.  I think the command line option itself is fine but...)

What does Linux do differently here?

It is actively damaging to the Xen community to users to force users
tweak command lines in order to boot/recover their system, and it looks
especially embarrassing when other OSes cope automatically.  We have
compatibility for all kinds of other firmware screw-ups, except EFI it
seems, and this needs to change.

So while I have no objection to the option per say, I don't think this
patch is reasonable as a "fix" to the problem as far as end users are
concerned.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Andrew Cooper]

On 08/08/2019 10:08, Jan Beulich wrote:
> On 08.08.2019 10:33, Andrew Cooper wrote:
>> On 08/08/2019 05:50, Juergen Gross wrote:
>>> On 07.08.19 20:11, Andrew Cooper wrote:
 
 Its not exactly the easiest to dump to follow.

 First of all - I don't see why the hold/block time are printed like
 that.  It
 might be a holdover from the 32bit build, pre PRId64 support.  They
 should
 probably use PRI_stime anyway.
>>> Fine with me.
>>>
 The domid rendering is unfortunate.  Ideally we'd use %pd but that would
 involve rearranging the logic to get a struct domain* in hand.
 Seeing as
 you're the last person to modify this code, how hard would that be to
 do?
>>> It would completely break the struct type agnostic design.
>> Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
>> would be a trivial adjustment in the vsnprintf code, and could plausibly
>> be useful elsewhere where we have a domid and not a domain pointer.
> At the risk of upsetting / annoying you:

Yes you really have

> A domid_t would again
> better be formatted via %od (and without the need to take the
> address of a respective variable). In the case here it would
> have the minor additional benefit of conserving on format string
> length.

There are concrete reasons why it is a terrible idea, because none of
this has anything to do with octal, and that %od has a well defined
meaning which is not related to domid's.  There is also a very good
reason why all the magic is hidden behind one single formatter.

You seem hell bent on making it actively confusing and complicated to
write and read printk()'s, and I am not willing to lumber anyone
developing on Xen with this cognitive complexity.

I am stick to death of having the same over and over, so let me stop any
further wasting of time and be absolutely crystal clear.

NACK to any form of overloading %o

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [linux-linus test] 139792: regressions - FAIL

[osstest service owner]

flight 139792 linux-linus real [real]
http://logs.test-lab.xenproject.org/osstest/logs/139792/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-examine   8 reboot   fail REGR. vs. 133580
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
133580
 test-amd64-i386-xl-raw7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-win10-i386  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-freebsd10-i386  7 xen-boot   fail REGR. vs. 133580
 test-amd64-i386-qemut-rhel6hvm-intel  7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-boot fail REGR. vs. 
133580
 test-amd64-i386-freebsd10-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-boot fail REGR. vs. 
133580
 test-amd64-i386-qemuu-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 133580
 test-amd64-i386-qemuu-rhel6hvm-intel  7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-libvirt-pair 10 xen-boot/src_hostfail REGR. vs. 133580
 test-amd64-i386-libvirt-pair 11 xen-boot/dst_hostfail REGR. vs. 133580
 test-amd64-i386-libvirt   7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-qemut-rhel6hvm-amd  7 xen-boot   fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-ovmf-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-qemut-win7-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-qemut-win10-i386  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-ws16-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-qemut-debianhvm-amd64  7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl-pvshim 7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl-qemut-ws16-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-libvirt-xsm   7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-win7-amd64  7 xen-boot  fail REGR. vs. 133580
 test-amd64-i386-xl-xsm7 xen-boot fail REGR. vs. 133580
 test-amd64-i386-pair 10 xen-boot/src_hostfail REGR. vs. 133580
 test-amd64-i386-pair 11 xen-boot/dst_hostfail REGR. vs. 133580
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-boot fail REGR. vs. 
133580
 test-amd64-i386-xl-shadow 7 xen-boot fail REGR. vs. 133580
 test-arm64-arm64-examine11 examine-serial/bootloader fail REGR. vs. 133580
 build-armhf-pvops 6 kernel-build fail REGR. vs. 133580
 test-arm64-arm64-libvirt-xsm 16 guest-start/debian.repeat fail REGR. vs. 133580

Tests which did not succeed, but are not blocking:
 test-armhf-armhf-examine  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-multivcpu  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-vhd   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-rtds  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt-raw  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-arndale   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-cubietruck  1 build-check(1)   blocked  n/a
 test-armhf-armhf-libvirt  1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit1   1 build-check(1)   blocked  n/a
 test-armhf-armhf-xl-credit2   1 build-check(1)   blocked  n/a
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm  7 xen-boot fail baseline untested
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm  7 xen-boot fail baseline untested
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 133580
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 133580
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 133580
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 133580
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 09:43:01AM +0100, Andrew Cooper wrote:
> On 08/08/2019 07:22, Jan Beulich wrote:
> > On 07.08.2019 21:41, Andrew Cooper wrote:
> >> --- /dev/null
> >> +++ b/docs/glossary.rst
> >> @@ -0,0 +1,37 @@
> >> +Glossary
> >> +
> >> +
> >> +.. Terms should appear in alphabetical order
> >> +
> >> +.. glossary::
> >> +
> >> +   control domain
> >> + A :term:`domain`, commonly dom0, with the permission and
> >> responsibility
> >> + to create and manage other domains on the system.
> >> +
> >> +   domain
> >> + A domain is Xen's unit of resource ownership, and generally has
> >> at the
> >> + minimum some RAM and virtual CPUs.
> >> +
> >> + The terms :term:`domain` and :term:`guest` are commonly used
> >> + interchangeably, but they mean subtly different things.
> >> +
> >> + A guest is a single virtual machine.
> >> +
> >> + Consider the case of live migration where, for a period of
> >> time, one
> >> + guest will be comprised of two domains, while it is in transit.
> >> +
> >> +   domid
> >> + The numeric identifier of a running :term:`domain`.  It is
> >> unique to a
> >> + single instance of Xen, used as the identifier in various APIs,
> >> and is
> >> + typically allocated sequentially from 0.
> >> +
> >> +   guest
> >> + See :term:`domain`
> >
> > I think you want to mention the usual distinction here: Dom0 is,
> > while a domain, commonly not considered a guest.
> 
> To be honest, I had totally forgotten about that.  I guess now is the
> proper time to rehash it in public.
> 
> I don't think the way it currently gets used has a clear or coherent set
> of rules, because I can't think of any to describe how it does get used.
> 
> Either there are a clear and coherent (and simple!) set of rules for
> what we mean by "guest", at which point they can live here in the
> glossary, or the fuzzy way it is current used should cease.

I've always referred to dom0 as a privileged guest, but a guest
after all. I think this is one of the differences of Xen vs KVM or
other hosted hypervisors.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [qemu-upstream-unstable test] 139794: tolerable FAIL - PUSHED

[osstest service owner]

flight 139794 qemu-upstream-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/139794/

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 139379
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 139379
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 139379
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 139379
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass

version targeted for testing:
 qemuudbf360567a7da50db4d2f9bde4649aba21aa8106
baseline version:
 qemuu1bcf484fa9f451cc8c290fe80fd0e764199ca81c

Last test of basis   139379  2019-07-26 15:37:05 Z   12 days
Testing same since   139764  2019-08-06 10:38:59 Z1 days2 attempts


People who touched revisions under test:
  Daniel P. Berrangé 
  Gerd Hoffmann 
  Laurent Vivier 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 build-amd64  pass
 build-arm64  pass
 build-armhf  

Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Oleksandr]

Hi,


Hi, Julien.





Sorry for the possible format issues.


 > No, it is not disabled. But, ipmmu_irq() uses another 
mmu->lock. So, I

 > think, there won't be a deadlock.
 >
 > Or I really missed something?
 >
 > If we worry about ipmmu_tlb_invalidate() which is called here (to
 > perform a flush by request from P2M code, which manages a page 
table)

 > and from the irq handler (to perform a flush to resume address
 > translation), I could use a tasklet to schedule 
ipmmu_tlb_invalidate()
 > from the irq handler then. This way we would get this 
serialized. What

 > do you think?

    I am afraid a tasklet is not an option. You need to perform the TLB
    flush when requested otherwise you are introducing a security issue.

    This is because as soon as a region is unmapped in the page 
table, we
    remove the drop the reference on any page backing that region. 
When the

    reference is dropped to zero, the page can be reallocated to another
    domain or even Xen. If the TLB flush happen after, then the guest 
may
    still be able to access the page for a short time if the 
translation has

    been cached by the any TLB (IOMMU, Processor).



I understand this. I am not proposing to delay a requested by P2M 
code TLB flush in any case. I just propose to issue TLB flush (which 
we have to perform in case of page faults, to resolve error condition 
and resume translations) from a tasklet rather than from interrupt 
handler directly. This is the TLB flush I am speaking about:


https://github.com/otyshchenko1/xen/blob/ipmmu_upstream2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L598 



Sorry if I was unclear.


My mistake, I misread what you wrote.

I found the flush in the renesas-bsp and not Linux upstream but it is 
not clear why this is actually required. You are not fixing any 
translation error. So what this flush will do?


Regarding the placement of the flush, then if you execute in a tasklet 
it will likely be done later on when the IRQ has been acknowledge. 
What's the implication to delay it?



Looks like, there is no need to put this flush into a tasklet. As I 
understand from Shimoda-san's answer it is OK to call flush here.


So, my worry about calling ipmmu_tlb_invalidate() directly from the 
interrupt handler is not actual anymore.

--
This is my understanding regarding the flush purpose here. This code, 
just follows the TRM, no more no less,
which mentions about a need to flush TLB after clearing error status 
register and updating a page table entries (which, I assume, means to 
resolve a reason why translation/page fault error actually have 
happened) to resume address translation request.


But, with one remark, as you have already noted, we are not trying to 
handle/fix this fault (update page table entries), driver doesn't manage 
page table and is not aware what the page table is. What is more, it is 
unclear what actually need to be fixed in the page table which is a CPU 
page table as the same time.


I have heard there is a break-before-make sequence when updating the 
page table. So, if device in a domain is issuing DMA somewhere in the 
middle of updating a page table, theoretically, we might hit into this 
fault. In this case the page table is correct and we don't need to fix 
anything...   Being honest, I have never seen a fault caused by 
break-before-make sequence.




Cheers,


--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Andrew Cooper]

On 08/08/2019 10:36, Juergen Gross wrote:
> On 08.08.19 10:33, Andrew Cooper wrote:
>> On 08/08/2019 05:50, Juergen Gross wrote:
>>> On 07.08.19 20:11, Andrew Cooper wrote:

 
 Its not exactly the easiest to dump to follow.

 First of all - I don't see why the hold/block time are printed like
 that.  It
 might be a holdover from the 32bit build, pre PRId64 support.  They
 should
 probably use PRI_stime anyway.
>>>
>>> Fine with me.
>>>
 The domid rendering is unfortunate.  Ideally we'd use %pd but that
 would
 involve rearranging the logic to get a struct domain* in hand.
 Seeing as
 you're the last person to modify this code, how hard would that be to
 do?
>>>
>>> It would completely break the struct type agnostic design.
>>
>> Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
>> would be a trivial adjustment in the vsnprintf code, and could plausibly
>> be useful elsewhere where we have a domid and not a domain pointer.
>
> Lock profiling has no knowledge that it is working on a struct domain.
> It is just working on a lock in a struct and an index to differentiate
> the struct instance. Using the domid as the index is just for making it
> more easy to understand the printout.
>
> I wouldn't want e.g. a per-event lock to be named "IDLE" just because
> it happens to be index 32767.

Ok, but clearly there is something which distinguishes domain indices
from other indices?

>
>>
>>>
 We have several global locks called lock:

     (XEN) Global lock: addr=82d0808181e0, lockval=10001, cpu=4095
     (XEN)   lock:   1(:01322165), block:
 0(:)
     (XEN) Global lock: addr=82d080817cc0, lockval=100010, cpu=4095
     (XEN)   lock:   0(:), block:
 0(:)
     (XEN) Global lock: addr=82d080817800, lockval=, cpu=4095
     (XEN)   lock:   0(:), block:
 0(:)
     (XEN) Global lock: addr=82d080817780, lockval=, cpu=4095
     (XEN)   lock:   0(:), block:
 0(:)
     (XEN) Global lock: addr=82d080817510, lockval=, cpu=4095
     (XEN)   lock:   0(:), block:
 0(:)

 The second one in particular has corrupt data, seeing has it has been
 taken
 and released several times without lock_cnt increasing.
>>>
>>> The lock might have been taken/released before lock profiling has been
>>> initialized.
>>
>> What is there to initialise?  It all looks statically set up.
>
> lock->profile is set only in lock_prof_init().

Ah - so it is.  I wonder if this can be done at compile time?  Its just
a backreference for the forward reference which is done at.

(Wow this code is complicated to follow).  I think it can be done with a
forward declaration of static struct lock_profile
__lock_profile_data_##l and passing something other than NULL into
_SPIN_LOCK_UNLOCKED(), but that will break the ability to do static
DEFINE_SPINLOCK().

Probably not worth messing with this now, but perhaps something to think
over.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 12/35] OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct

[Anthony PERARD]

On Wed, Aug 07, 2019 at 04:35:58PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote:
> > Check if there's a start of the day struct provided to PVH guest, save
> > the ACPI RSDP address for later.
> > 
> > This patch import import arch-x86/hvm/start_info.h from xen.git.
> 
> You seem to change the types when importing start_info.h, is that
> absolutely necessary?

I guess one could try to map xen's types to EDKII's type with typedefs,
but I'm not sur how well that would work. Importing the xen headers is
documented so changing the types is fairly easy, see
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/IndustryStandard/Xen/README

Also, changing the header further might have been something useful to
do, we could have match EDKII's naming convention and source files would
have looked a bit less weird.

> From my experience working with different projects when importing such
> headers it's better to keep them verbatim, this makes importing future
> versions from upstream easier.
>
> I have a comment below, but it's more like a question.
> 
> > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> > index 5c7d7ddc1c..b366139a0a 100644
> > --- a/OvmfPkg/XenPlatformPei/Xen.c
> > +++ b/OvmfPkg/XenPlatformPei/Xen.c
> > @@ -25,6 +25,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include "Platform.h"
> >  #include "Xen.h"
> > @@ -86,6 +87,7 @@ XenConnect (
> >UINT32 XenVersion;
> >EFI_XEN_OVMF_INFO *Info;
> >CHAR8 Sig[sizeof (Info->Signature) + 1];
> > +  UINT32 *PVHResetVectorData;
> >  
> >AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL);
> >mXenInfo.HyperPages = AllocatePages (TransferPages);
> > @@ -121,6 +123,29 @@ XenConnect (
> >  mXenHvmloaderInfo = NULL;
> >}
> >  
> > +  mXenInfo.RsdpPvh = NULL;
> > +
> > +  //
> > +  // Locate and use information from the start of day structure if we have
> > +  // booted via the PVH entry point.
> > +  //
> > +
> > +  PVHResetVectorData = (VOID *)(UINTN) PcdGet32 
> > (PcdXenPvhStartOfDayStructPtr);
> > +  //
> > +  // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm
> > +  //
> > +  if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) {
> > +struct hvm_start_info *HVMStartInfo;
> > +
> > +HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0];
> > +if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) {
> > +  ASSERT (HVMStartInfo->rsdp_paddr != 0);
> > +  if (HVMStartInfo->rsdp_paddr != 0) {
> > +mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr;
> 
> I guess you can do this because OVMF has an identity map of virtual
> addresses to physical addresses?

I think so, yes. I know that early code does created page table like
that, but I don't know if later code rework those page table or not.

> I wonder the size of such identity map, and whether you need to check
> that the rsdp address is indeed inside of that region before
> converting it to a pointer. The same would apply to
> PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's
> always < 4GB, which I assume OVMF always has identity mapped?

PcdXenPvhStartOfDayStructPtr is safe because OVMF owns it. As for the
rspd_paddr* and the HVMStartInfo*, I need to check. As you say, it's
probably fine as long as it's <4GB.

I've looked at the comment here:
https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L94
This mean that the code executed in the patch (accessing the hvm start
info struct) is executed while the id map is setup up to 4GB. So as long
as the struct is below 4G, it's fine.

As for the RSDP, I think that pointer is accessed much later, when a
different page table is setup, I think that would be that code:
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
But I'm not sure how much is setup. But I'm guessing that whatever is
pointed by RSDP, it will be in the 1:1, because we tell the UEFI about
it while parsing the e820.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Julien Grall]

On 08/08/2019 08:51, Juergen Gross wrote:

On 08.08.19 08:58, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:
Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.


But that would increase the size of the union to 4 bytes instead of 2.
So at least pad and cpu must be unsigned short or (better) uint16_t.


How about bool irq_safe:1?

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V4] tools/libxl: Add iothread support for COLO

[Zhang, Chen]

Thank you Anthony PERARD.

Thanks
Zhang Chen


> -Original Message-
> From: Anthony PERARD [mailto:anthony.per...@citrix.com]
> Sent: Thursday, August 8, 2019 5:08 PM
> To: Zhang, Chen 
> Cc: Ian Jackson ; Wei Liu ; xen-
> de...@lists.xenproject.org; Zhang Chen 
> Subject: Re: [PATCH V4] tools/libxl: Add iothread support for COLO
> 
> On Thu, Aug 08, 2019 at 02:44:42AM +, Zhang, Chen wrote:
> > Ping...
> > Any comments?
> 
> For your information, that patch have been pushed, see commit
> 174db28bb823e8c98c319fdbdc6d4cbe1050ba14 ;-)
> 
> Cheers,
> 
> --
> Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Andrew Cooper]

On 08/08/2019 10:13, Julien Grall wrote:
> Hi Jan,
>
> On 08/08/2019 10:04, Jan Beulich wrote:
>> On 08.08.2019 10:43, Andrew Cooper wrote:
>>> On 08/08/2019 07:22, Jan Beulich wrote:
 On 07.08.2019 21:41, Andrew Cooper wrote:
> --- /dev/null
> +++ b/docs/glossary.rst
> @@ -0,0 +1,37 @@
> +Glossary
> +
> +
> +.. Terms should appear in alphabetical order
> +
> +.. glossary::
> +
> +   control domain
> + A :term:`domain`, commonly dom0, with the permission and
> responsibility
> + to create and manage other domains on the system.
> +
> +   domain
> + A domain is Xen's unit of resource ownership, and generally has
> at the
> + minimum some RAM and virtual CPUs.
> +
> + The terms :term:`domain` and :term:`guest` are commonly used
> + interchangeably, but they mean subtly different things.
> +
> + A guest is a single virtual machine.
> +
> + Consider the case of live migration where, for a period of
> time, one
> + guest will be comprised of two domains, while it is in transit.
> +
> +   domid
> + The numeric identifier of a running :term:`domain`.  It is
> unique to a
> + single instance of Xen, used as the identifier in various APIs,
> and is
> + typically allocated sequentially from 0.
> +
> +   guest
> + See :term:`domain`

 I think you want to mention the usual distinction here: Dom0 is,
 while a domain, commonly not considered a guest.
>>>
>>> To be honest, I had totally forgotten about that.  I guess now is the
>>> proper time to rehash it in public.
>>>
>>> I don't think the way it currently gets used has a clear or coherent
>>> set
>>> of rules, because I can't think of any to describe how it does get
>>> used.
>>>
>>> Either there are a clear and coherent (and simple!) set of rules for
>>> what we mean by "guest", at which point they can live here in the
>>> glossary, or the fuzzy way it is current used should cease.
>>
>> What's fuzzy about Dom0 not being a guest (due to being a part of the
>> host instead)?
> Dom0 is not part of the host if you are using an hardware domain.

Quite.  Even if you are using a hardware domain, dom0 is the control
domain.  Is this part of the host or not?

What about device driver domains?

To answer Jan's question, what is "fuzzy" is the fact that there is not
a clear definition.

> I actually thought we renamed everything in Xen from Dom0 to hwdom to
> avoid the 
>
> I also would rather avoid to treat "dom0" as not a guest. In normal
> setup this is a more privilege guest, in other setup this may just be
> a normal guest (think about partitioning).

People thinking that dom0 isn't a VM is a massive problem for
understanding Xen's architecture, which is why I phrased some of
introduction the way I did.

It is a mistake which we need to take steps to address in our literature.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v8 25/28] x86_32/asm: add ENDs to some functions and relabel with SYM_CODE_*

[Jiri Slaby]

All these are functions which are invoked from elsewhere, but they are
not typical C functions. So we annotate them using the new
SYM_CODE_START. All these were not balanced with any END, so mark their
ends by SYM_CODE_END, appropriatelly.

Signed-off-by: Jiri Slaby 
Reviewed-by: Boris Ostrovsky  [xen bits]
Reviewed-by: Rafael J. Wysocki  [hibernate]
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: "Rafael J. Wysocki" 
Cc: Len Brown 
Cc: Pavel Machek 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: linux...@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/entry/entry_32.S| 3 ++-
 arch/x86/kernel/acpi/wakeup_32.S | 7 ---
 arch/x86/kernel/ftrace_32.S  | 3 ++-
 arch/x86/kernel/head_32.S| 3 ++-
 arch/x86/power/hibernate_asm_32.S| 6 --
 arch/x86/realmode/rm/trampoline_32.S | 6 --
 arch/x86/xen/xen-asm_32.S| 7 ---
 7 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index 4900a6a5e125..64fe7aa50ad2 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -847,9 +847,10 @@ SYM_ENTRY(__begin_SYSENTER_singlestep_region, 
SYM_L_GLOBAL, SYM_A_NONE)
  * Xen doesn't set %esp to be precisely what the normal SYSENTER
  * entry point expects, so fix it up before using the normal path.
  */
-ENTRY(xen_sysenter_target)
+SYM_CODE_START(xen_sysenter_target)
addl$5*4, %esp  /* remove xen-provided frame */
jmp .Lsysenter_past_esp
+SYM_CODE_END(xen_sysenter_target)
 #endif
 
 /*
diff --git a/arch/x86/kernel/acpi/wakeup_32.S b/arch/x86/kernel/acpi/wakeup_32.S
index 427249292aef..daf88f8143c5 100644
--- a/arch/x86/kernel/acpi/wakeup_32.S
+++ b/arch/x86/kernel/acpi/wakeup_32.S
@@ -9,8 +9,7 @@
.code32
ALIGN
 
-ENTRY(wakeup_pmode_return)
-wakeup_pmode_return:
+SYM_CODE_START(wakeup_pmode_return)
movw$__KERNEL_DS, %ax
movw%ax, %ss
movw%ax, %fs
@@ -39,6 +38,7 @@ wakeup_pmode_return:
# jump to place where we left off
movlsaved_eip, %eax
jmp *%eax
+SYM_CODE_END(wakeup_pmode_return)
 
 bogus_magic:
jmp bogus_magic
@@ -72,7 +72,7 @@ restore_registers:
popfl
ret
 
-ENTRY(do_suspend_lowlevel)
+SYM_CODE_START(do_suspend_lowlevel)
callsave_processor_state
callsave_registers
pushl   $3
@@ -87,6 +87,7 @@ ret_point:
callrestore_registers
callrestore_processor_state
ret
+SYM_CODE_END(do_suspend_lowlevel)
 
 .data
 ALIGN
diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S
index 219be1309c37..a43ed4c0402d 100644
--- a/arch/x86/kernel/ftrace_32.S
+++ b/arch/x86/kernel/ftrace_32.S
@@ -89,7 +89,7 @@ WEAK(ftrace_stub)
ret
 END(ftrace_caller)
 
-ENTRY(ftrace_regs_caller)
+SYM_CODE_START(ftrace_regs_caller)
/*
 * We're here from an mcount/fentry CALL, and the stack frame looks 
like:
 *
@@ -163,6 +163,7 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
popl%eax
 
jmp .Lftrace_ret
+SYM_CODE_END(ftrace_regs_caller)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 ENTRY(ftrace_graph_caller)
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 2d5390d84467..bfd713034e9b 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -64,7 +64,7 @@ RESERVE_BRK(pagetables, INIT_MAP_SIZE)
  * can.
  */
 __HEAD
-ENTRY(startup_32)
+SYM_CODE_START(startup_32)
movl pa(initial_stack),%ecx

/* test KEEP_SEGMENTS flag to see if the bootloader is asking
@@ -172,6 +172,7 @@ num_subarch_entries = (. - subarch_entries) / 4
 #else
jmp .Ldefault_entry
 #endif /* CONFIG_PARAVIRT */
+SYM_CODE_END(startup_32)
 
 #ifdef CONFIG_HOTPLUG_CPU
 /*
diff --git a/arch/x86/power/hibernate_asm_32.S 
b/arch/x86/power/hibernate_asm_32.S
index 6fe383002125..a19ed3d23185 100644
--- a/arch/x86/power/hibernate_asm_32.S
+++ b/arch/x86/power/hibernate_asm_32.S
@@ -35,7 +35,7 @@ ENTRY(swsusp_arch_suspend)
ret
 ENDPROC(swsusp_arch_suspend)
 
-ENTRY(restore_image)
+SYM_CODE_START(restore_image)
/* prepare to jump to the image kernel */
movlrestore_jump_address, %ebx
movlrestore_cr3, %ebp
@@ -45,9 +45,10 @@ ENTRY(restore_image)
/* jump to relocated restore code */
movlrelocated_restore_code, %eax
jmpl*%eax
+SYM_CODE_END(restore_image)
 
 /* code below has been relocated to a safe page */
-ENTRY(core_restore_code)
+SYM_CODE_START(core_restore_code)
movltemp_pgt, %eax
movl%eax, %cr3
 
@@ -77,6 +78,7 @@ copy_loop:
 
 done:
jmpl*%ebx
+SYM_CODE_END(core_restore_code)
 
/* code below belongs to the image kernel */
.align PAGE_SIZE
diff --git a/arch/x86/realmode/rm/trampoline_32.S 
b/arch/x86/realmode/rm/trampoline_32.S
index ff00594a2ed0..3fad907a

[Xen-devel] [PATCH v8 23/28] x86_64/asm: change all ENTRY+END to SYM_CODE_*

[Jiri Slaby]

Here, we change all assembly code which is marked using END (and not
ENDPROC). We switch all these to appropriate new markings SYM_CODE_START
and SYM_CODE_END.

Signed-off-by: Jiri Slaby 
Reviewed-by: Boris Ostrovsky  [xen bits]
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/entry/entry_64.S| 52 
 arch/x86/entry/entry_64_compat.S |  8 ++---
 arch/x86/kernel/ftrace_64.S  |  4 +--
 arch/x86/kernel/head_64.S| 12 
 arch/x86/xen/xen-asm_64.S|  8 ++---
 arch/x86/xen/xen-head.S  |  8 ++---
 6 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 6d6cb84952ff..bd04476d432d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -46,11 +46,11 @@
 .section .entry.text, "ax"
 
 #ifdef CONFIG_PARAVIRT
-ENTRY(native_usergs_sysret64)
+SYM_CODE_START(native_usergs_sysret64)
UNWIND_HINT_EMPTY
swapgs
sysretq
-END(native_usergs_sysret64)
+SYM_CODE_END(native_usergs_sysret64)
 #endif /* CONFIG_PARAVIRT */
 
 .macro TRACE_IRQS_FLAGS flags:req
@@ -142,7 +142,7 @@ END(native_usergs_sysret64)
  * with them due to bugs in both AMD and Intel CPUs.
  */
 
-ENTRY(entry_SYSCALL_64)
+SYM_CODE_START(entry_SYSCALL_64)
UNWIND_HINT_EMPTY
/*
 * Interrupts are off on entry.
@@ -273,13 +273,13 @@ syscall_return_via_sysret:
popq%rdi
popq%rsp
USERGS_SYSRET64
-END(entry_SYSCALL_64)
+SYM_CODE_END(entry_SYSCALL_64)
 
 /*
  * %rdi: prev task
  * %rsi: next task
  */
-ENTRY(__switch_to_asm)
+SYM_CODE_START(__switch_to_asm)
UNWIND_HINT_FUNC
/*
 * Save callee-saved registers
@@ -321,7 +321,7 @@ ENTRY(__switch_to_asm)
popq%rbp
 
jmp __switch_to
-END(__switch_to_asm)
+SYM_CODE_END(__switch_to_asm)
 
 /*
  * A newly forked process directly context switches into this address.
@@ -330,7 +330,7 @@ END(__switch_to_asm)
  * rbx: kernel thread func (NULL for user thread)
  * r12: kernel thread arg
  */
-ENTRY(ret_from_fork)
+SYM_CODE_START(ret_from_fork)
UNWIND_HINT_EMPTY
movq%rax, %rdi
callschedule_tail   /* rdi: 'prev' task parameter */
@@ -357,14 +357,14 @@ ENTRY(ret_from_fork)
 */
movq$0, RAX(%rsp)
jmp 2b
-END(ret_from_fork)
+SYM_CODE_END(ret_from_fork)
 
 /*
  * Build the entry stubs with some assembler magic.
  * We pack 1 stub into every 8-byte block.
  */
.align 8
-ENTRY(irq_entries_start)
+SYM_CODE_START(irq_entries_start)
 vector=FIRST_EXTERNAL_VECTOR
 .rept (FIRST_SYSTEM_VECTOR - FIRST_EXTERNAL_VECTOR)
UNWIND_HINT_IRET_REGS
@@ -373,10 +373,10 @@ ENTRY(irq_entries_start)
.align  8
vector=vector+1
 .endr
-END(irq_entries_start)
+SYM_CODE_END(irq_entries_start)
 
.align 8
-ENTRY(spurious_entries_start)
+SYM_CODE_START(spurious_entries_start)
 vector=FIRST_SYSTEM_VECTOR
 .rept (NR_VECTORS - FIRST_SYSTEM_VECTOR)
UNWIND_HINT_IRET_REGS
@@ -385,7 +385,7 @@ ENTRY(spurious_entries_start)
.align  8
vector=vector+1
 .endr
-END(spurious_entries_start)
+SYM_CODE_END(spurious_entries_start)
 
 .macro DEBUG_ENTRY_ASSERT_IRQS_OFF
 #ifdef CONFIG_DEBUG_ENTRY
@@ -511,7 +511,7 @@ END(spurious_entries_start)
  * | return address|
  * ++
  */
-ENTRY(interrupt_entry)
+SYM_CODE_START(interrupt_entry)
UNWIND_HINT_FUNC
ASM_CLAC
cld
@@ -577,7 +577,7 @@ ENTRY(interrupt_entry)
TRACE_IRQS_OFF
 
ret
-END(interrupt_entry)
+SYM_CODE_END(interrupt_entry)
 _ASM_NOKPROBE(interrupt_entry)
 
 
@@ -793,7 +793,7 @@ _ASM_NOKPROBE(common_interrupt)
  * APIC interrupts.
  */
 .macro apicinterrupt3 num sym do_sym
-ENTRY(\sym)
+SYM_CODE_START(\sym)
UNWIND_HINT_IRET_REGS
pushq   $~(\num)
 .Lcommon_\sym:
@@ -801,7 +801,7 @@ ENTRY(\sym)
UNWIND_HINT_REGS indirect=1
call\do_sym /* rdi points to pt_regs */
jmp ret_from_intr
-END(\sym)
+SYM_CODE_END(\sym)
 _ASM_NOKPROBE(\sym)
 .endm
 
@@ -966,7 +966,7 @@ apicinterrupt IRQ_WORK_VECTOR   
irq_work_interrupt  smp_irq_work_interrupt
  * #DF: if the thread stack is somehow unusable, we'll still get a useful OOPS.
  */
 .macro idtentry sym do_sym has_error_code:req paranoid=0 shift_ist=-1 
ist_offset=0 create_gap=0 read_cr2=0
-ENTRY(\sym)
+SYM_CODE_START(\sym)
UNWIND_HINT_IRET_REGS offset=\has_error_code*8
 
/* Sanity check */
@@ -1016,7 +1016,7 @@ ENTRY(\sym)
.endif
 
 _ASM_NOKPROBE(\sym)
-END(\sym)
+SYM_CODE_END(\sym)
 .endm
 
 idtentry divide_error  do_divide_error 
has_error_code=0
@@ -1133,7 +1133,7 @@ SYM_CODE_END(xen_do_

Re: [Xen-devel] [edk2-devel] [PATCH v4 20/35] OvmfPkg/XenPlatformPei: Introduce XenPvhDetected

[Anthony PERARD]

On Wed, Aug 07, 2019 at 05:03:46PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:29PM +0100, Anthony PERARD wrote:
> > +BOOLEAN
> > +XenPvhDetected (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // This function should only be used after XenConnect
> > +  //
> > +  ASSERT (mXenInfo.VersionMajor != 0);
> 
> That's IMO dangerous. Using the version as an indication that
> XenConnect has run seems like a bad idea, since returning a major
> version of 0 is a valid number to return. Can't you check against
> something else that doesn't depends on hypervisor provided data? (ie:
> like some allocations or such that happen in XenConnect)
> 
> A paranoid could provider could even return major == 0 and minor == 0
> in order to attempt to hide the Xen version used, since guests are not
> supposed to infer anything from the Xen version, available hypervisor
> features are reported by other means.

I'm sure a paranoid provider wouldn't use a debug build of OVMF :-). So
that assert doesn't matter. There's nothing dangerous in a `nop'! :-D

But I could use mXenInfo.HyperPages instead.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v8 09/28] x86/asm: annotate aliases

[Jiri Slaby]

_key_expansion_128 is an alias to _key_expansion_256a, __memcpy to
memcpy, xen_syscall32_target to xen_sysenter_target, and so on. Annotate
them all using the new SYM_FUNC_START_ALIAS, SYM_FUNC_START_LOCAL_ALIAS,
and SYM_FUNC_END_ALIAS. This will make the tools generating the
debuginfo happy.

Signed-off-by: Jiri Slaby 
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Reviewed-by: Juergen Gross  [xen parts]
Cc: 
Cc: 
---
 arch/x86/crypto/aesni-intel_asm.S | 5 ++---
 arch/x86/lib/memcpy_64.S  | 4 ++--
 arch/x86/lib/memmove_64.S | 4 ++--
 arch/x86/lib/memset_64.S  | 4 ++--
 arch/x86/xen/xen-asm_64.S | 4 ++--
 5 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_asm.S 
b/arch/x86/crypto/aesni-intel_asm.S
index e0a5fb462a0a..9d8d5f2296e1 100644
--- a/arch/x86/crypto/aesni-intel_asm.S
+++ b/arch/x86/crypto/aesni-intel_asm.S
@@ -1757,8 +1757,7 @@ ENDPROC(aesni_gcm_finalize)
 #endif
 
 
-.align 4
-_key_expansion_128:
+SYM_FUNC_START_LOCAL_ALIAS(_key_expansion_128)
 SYM_FUNC_START_LOCAL(_key_expansion_256a)
pshufd $0b, %xmm1, %xmm1
shufps $0b0001, %xmm0, %xmm4
@@ -1769,8 +1768,8 @@ SYM_FUNC_START_LOCAL(_key_expansion_256a)
movaps %xmm0, (TKEYP)
add $0x10, TKEYP
ret
-ENDPROC(_key_expansion_128)
 SYM_FUNC_END(_key_expansion_256a)
+SYM_FUNC_END_ALIAS(_key_expansion_128)
 
 SYM_FUNC_START_LOCAL(_key_expansion_192a)
pshufd $0b01010101, %xmm1, %xmm1
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 92748660ba51..57a64266ba69 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -28,7 +28,7 @@
  * Output:
  * rax original destination
  */
-ENTRY(__memcpy)
+SYM_FUNC_START_ALIAS(__memcpy)
 ENTRY(memcpy)
ALTERNATIVE_2 "jmp memcpy_orig", "", X86_FEATURE_REP_GOOD, \
  "jmp memcpy_erms", X86_FEATURE_ERMS
@@ -42,7 +42,7 @@ ENTRY(memcpy)
rep movsb
ret
 ENDPROC(memcpy)
-ENDPROC(__memcpy)
+SYM_FUNC_END_ALIAS(__memcpy)
 EXPORT_SYMBOL(memcpy)
 EXPORT_SYMBOL(__memcpy)
 
diff --git a/arch/x86/lib/memmove_64.S b/arch/x86/lib/memmove_64.S
index bbec69d8223b..50c1648311b3 100644
--- a/arch/x86/lib/memmove_64.S
+++ b/arch/x86/lib/memmove_64.S
@@ -26,7 +26,7 @@
  */
 .weak memmove
 
-ENTRY(memmove)
+SYM_FUNC_START_ALIAS(memmove)
 ENTRY(__memmove)
 
/* Handle more 32 bytes in loop */
@@ -208,6 +208,6 @@ ENTRY(__memmove)
 13:
retq
 ENDPROC(__memmove)
-ENDPROC(memmove)
+SYM_FUNC_END_ALIAS(memmove)
 EXPORT_SYMBOL(__memmove)
 EXPORT_SYMBOL(memmove)
diff --git a/arch/x86/lib/memset_64.S b/arch/x86/lib/memset_64.S
index 9bc861c71e75..927ac44d34aa 100644
--- a/arch/x86/lib/memset_64.S
+++ b/arch/x86/lib/memset_64.S
@@ -19,7 +19,7 @@
  *
  * rax   original destination
  */
-ENTRY(memset)
+SYM_FUNC_START_ALIAS(memset)
 ENTRY(__memset)
/*
 * Some CPUs support enhanced REP MOVSB/STOSB feature. It is recommended
@@ -43,8 +43,8 @@ ENTRY(__memset)
rep stosb
movq %r9,%rax
ret
-ENDPROC(memset)
 ENDPROC(__memset)
+SYM_FUNC_END_ALIAS(memset)
 EXPORT_SYMBOL(memset)
 EXPORT_SYMBOL(__memset)
 
diff --git a/arch/x86/xen/xen-asm_64.S b/arch/x86/xen/xen-asm_64.S
index ebf610b49c06..45c1249f370d 100644
--- a/arch/x86/xen/xen-asm_64.S
+++ b/arch/x86/xen/xen-asm_64.S
@@ -167,13 +167,13 @@ ENDPROC(xen_sysenter_target)
 
 #else /* !CONFIG_IA32_EMULATION */
 
-ENTRY(xen_syscall32_target)
+SYM_FUNC_START_ALIAS(xen_syscall32_target)
 ENTRY(xen_sysenter_target)
lea 16(%rsp), %rsp  /* strip %rcx, %r11 */
mov $-ENOSYS, %rax
pushq $0
jmp hypercall_iret
-ENDPROC(xen_syscall32_target)
 ENDPROC(xen_sysenter_target)
+SYM_FUNC_END_ALIAS(xen_syscall32_target)
 
 #endif /* CONFIG_IA32_EMULATION */
-- 
2.22.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v8 01/28] linkage: new macros for assembler symbols

[Jiri Slaby]

Introduce new C macros for annotations of functions and data in
assembly. There is a long-standing mess in macros like ENTRY, END,
ENDPROC and similar. They are used in different manners and sometimes
incorrectly.

So introduce macros with clear use to annotate assembly as follows:

a) Support macros for the ones below
   SYM_T_FUNC -- type used by assembler to mark functions
   SYM_T_OBJECT -- type used by assembler to mark data
   SYM_T_NONE -- type used by assembler to mark entries of unknown type

   They are defined as STT_FUNC, STT_OBJECT, and STT_NOTYPE
   respectively. According to the gas manual, this is the most portable
   way. I am not sure about other assemblers, so we can switch this back
   to %function and %object if this turns into a problem. Architectures
   can also override them by something like ", @function" if they need.

   SYM_A_ALIGN, SYM_A_NONE -- align the symbol?
   SYM_L_GLOBAL, SYM_L_WEAK, SYM_L_LOCAL -- linkage of symbols

b) Mostly internal annotations, used by the ones below
   SYM_ENTRY -- use only if you have to (for non-paired symbols)
   SYM_START -- use only if you have to (for paired symbols)
   SYM_END -- use only if you have to (for paired symbols)

c) Annotations for code
   SYM_INNER_LABEL_ALIGN -- only for labels in the middle of code
   SYM_INNER_LABEL -- only for labels in the middle of code

   SYM_FUNC_START_LOCAL_ALIAS -- use where there are two local names for
one function
   SYM_FUNC_START_ALIAS -- use where there are two global names for one
function
   SYM_FUNC_END_ALIAS -- the end of LOCAL_ALIASed or ALIASed function

   SYM_FUNC_START -- use for global functions
   SYM_FUNC_START_NOALIGN -- use for global functions, w/o alignment
   SYM_FUNC_START_LOCAL -- use for local functions
   SYM_FUNC_START_LOCAL_NOALIGN -- use for local functions, w/o
alignment
   SYM_FUNC_START_WEAK -- use for weak functions
   SYM_FUNC_START_WEAK_NOALIGN -- use for weak functions, w/o alignment
   SYM_FUNC_END -- the end of SYM_FUNC_START_LOCAL, SYM_FUNC_START,
SYM_FUNC_START_WEAK, ...

   For functions with special (non-C) calling conventions:
   SYM_CODE_START -- use for non-C (special) functions
   SYM_CODE_START_NOALIGN -- use for non-C (special) functions, w/o
alignment
   SYM_CODE_START_LOCAL -- use for local non-C (special) functions
   SYM_CODE_START_LOCAL_NOALIGN -- use for local non-C (special)
functions, w/o alignment
   SYM_CODE_END -- the end of SYM_CODE_START_LOCAL or SYM_CODE_START

d) For data
   SYM_DATA_START -- global data symbol
   SYM_DATA_START_LOCAL -- local data symbol
   SYM_DATA_END -- the end of the SYM_DATA_START symbol
   SYM_DATA_END_LABEL -- the labeled end of SYM_DATA_START symbol
   SYM_DATA -- start+end wrapper around simple global data
   SYM_DATA_LOCAL -- start+end wrapper around simple local data

==

The macros allow to pair starts and ends of functions and mark functions
correctly in the output ELF objects.

All users of the old macros in x86 are converted to use these in further
patches.

[v2]
* use SYM_ prefix and sane names
* add SYM_START and SYM_END and parametrize all the macros

[v3]
* add SYM_DATA, SYM_DATA_LOCAL, and SYM_DATA_END_LABEL

[v4]
* add _NOALIGN versions of some macros
* add _CODE_ derivates of _FUNC_ macros

[v5]
* drop "SIMPLE" from data annotations
* switch NOALIGN and ALIGN variants of inner labels
* s/visibility/linkage/; s@SYM_V_@SYM_L_@
* add Documentation

[v6]
* fixed typos found by Randy Dunlap
* remove doubled INNER_LABEL macros, one pair was unused

[v8]
* use lkml.kernel.org for links
* link the docs from index.rst (by bpetkov)
* fixed typos on the docs

Signed-off-by: Jiri Slaby 
Cc: Andrew Morton 
Cc: Boris Ostrovsky 
Cc: h...@zytor.com
Cc: Ingo Molnar 
Cc: jpoim...@redhat.com
Cc: Juergen Gross 
Cc: Len Brown 
Cc: Linus Torvalds 
Cc: linux-ker...@vger.kernel.org
Cc: linux...@vger.kernel.org
Cc: mi...@redhat.com
Cc: Pavel Machek 
Cc: Peter Zijlstra 
Cc: "Rafael J. Wysocki" 
Cc: Thomas Gleixner 
Cc: xen-devel@lists.xenproject.org
Cc: x...@kernel.org
---
 Documentation/asm-annotations.rst | 217 ++
 Documentation/index.rst   |   8 +
 arch/x86/include/asm/linkage.h|  10 +-
 include/linux/linkage.h   | 245 +-
 4 files changed, 469 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/asm-annotations.rst

diff --git a/Documentation/asm-annotations.rst 
b/Documentation/asm-annotations.rst
new file mode 100644
index ..a967648e4325
--- /dev/null
+++ b/Documentation/asm-annotations.rst
@@ -0,0 +1,217 @@
+Assembler Annotations
+=
+
+Copyright (c) 2017-2019 Jiri Slaby
+
+This document describes the new macros for annotation of data and code in
+assembly. In particular, it contains information about ``SYM_FUNC_START``,
+``SYM_FUNC_END``, ``SYM_CODE_START``, and similar.
+
+Rationale
+-
+Some code like entries, trampolines, or boot code need

[Xen-devel] [PATCH v8 22/28] x86_64/asm: add ENDs to some functions and relabel with SYM_CODE_*

[Jiri Slaby]

All these are functions which are invoked from elsewhere, but they are
not typical C functions. So we annotate them using the new
SYM_CODE_START. All these were not balanced with any END, so mark their
ends by SYM_CODE_END appropriatelly too.

Signed-off-by: Jiri Slaby 
Reviewed-by: Boris Ostrovsky  [xen bits]
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/boot/compressed/head_64.S   |  6 --
 arch/x86/platform/olpc/xo1-wakeup.S  |  3 ++-
 arch/x86/power/hibernate_asm_64.S|  6 --
 arch/x86/realmode/rm/reboot.S|  3 ++-
 arch/x86/realmode/rm/trampoline_64.S | 10 +++---
 arch/x86/realmode/rm/wakeup_asm.S|  3 ++-
 arch/x86/xen/xen-asm_64.S|  6 --
 7 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/arch/x86/boot/compressed/head_64.S 
b/arch/x86/boot/compressed/head_64.S
index 70a25e0fa71e..362bd01d3bfb 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -250,7 +250,7 @@ ENDPROC(efi32_stub_entry)
 
.code64
.org 0x200
-ENTRY(startup_64)
+SYM_CODE_START(startup_64)
/*
 * 64bit entry is 0x200 and it is ABI so immutable!
 * We come here either from startup_32 or directly from a
@@ -442,6 +442,7 @@ trampoline_return:
  */
leaqrelocated(%rbx), %rax
jmp *%rax
+SYM_CODE_END(startup_64)
 
 #ifdef CONFIG_EFI_STUB
 
@@ -571,7 +572,7 @@ adjust_got:
  * ECX contains the base address of the trampoline memory.
  * Non zero RDX means trampoline needs to enable 5-level paging.
  */
-ENTRY(trampoline_32bit_src)
+SYM_CODE_START(trampoline_32bit_src)
/* Set up data and stack segments */
movl$__KERNEL_DS, %eax
movl%eax, %ds
@@ -634,6 +635,7 @@ ENTRY(trampoline_32bit_src)
movl%eax, %cr0
 
lret
+SYM_CODE_END(trampoline_32bit_src)
 
.code64
 SYM_FUNC_START_LOCAL_NOALIGN(paging_enabled)
diff --git a/arch/x86/platform/olpc/xo1-wakeup.S 
b/arch/x86/platform/olpc/xo1-wakeup.S
index 5fee3a2c2fd4..75f4faff8468 100644
--- a/arch/x86/platform/olpc/xo1-wakeup.S
+++ b/arch/x86/platform/olpc/xo1-wakeup.S
@@ -90,7 +90,7 @@ restore_registers:
 
ret
 
-ENTRY(do_olpc_suspend_lowlevel)
+SYM_CODE_START(do_olpc_suspend_lowlevel)
callsave_processor_state
callsave_registers
 
@@ -110,6 +110,7 @@ ret_point:
callrestore_registers
callrestore_processor_state
ret
+SYM_CODE_END(do_olpc_suspend_lowlevel)
 
 .data
 saved_gdt: .long   0,0
diff --git a/arch/x86/power/hibernate_asm_64.S 
b/arch/x86/power/hibernate_asm_64.S
index a4d5eb0a7ece..4057cd5af7e2 100644
--- a/arch/x86/power/hibernate_asm_64.S
+++ b/arch/x86/power/hibernate_asm_64.S
@@ -52,7 +52,7 @@ ENTRY(swsusp_arch_suspend)
ret
 ENDPROC(swsusp_arch_suspend)
 
-ENTRY(restore_image)
+SYM_CODE_START(restore_image)
/* prepare to jump to the image kernel */
movqrestore_jump_address(%rip), %r8
movqrestore_cr3(%rip), %r9
@@ -67,9 +67,10 @@ ENTRY(restore_image)
/* jump to relocated restore code */
movqrelocated_restore_code(%rip), %rcx
jmpq*%rcx
+SYM_CODE_END(restore_image)
 
/* code below has been relocated to a safe page */
-ENTRY(core_restore_code)
+SYM_CODE_START(core_restore_code)
/* switch to temporary page tables */
movq%rax, %cr3
/* flush TLB */
@@ -97,6 +98,7 @@ ENTRY(core_restore_code)
 .Ldone:
/* jump to the restore_registers address from the image header */
jmpq*%r8
+SYM_CODE_END(core_restore_code)
 
 /* code below belongs to the image kernel */
.align PAGE_SIZE
diff --git a/arch/x86/realmode/rm/reboot.S b/arch/x86/realmode/rm/reboot.S
index 424826afb501..f10515b10e0a 100644
--- a/arch/x86/realmode/rm/reboot.S
+++ b/arch/x86/realmode/rm/reboot.S
@@ -19,7 +19,7 @@
  */
.section ".text32", "ax"
.code32
-ENTRY(machine_real_restart_asm)
+SYM_CODE_START(machine_real_restart_asm)
 
 #ifdef CONFIG_X86_64
/* Switch to trampoline GDT as it is guaranteed < 4 GiB */
@@ -63,6 +63,7 @@ SYM_INNER_LABEL(machine_real_restart_paging_off, SYM_L_GLOBAL)
movl%ecx, %gs
movl%ecx, %ss
ljmpw   $8, $1f
+SYM_CODE_END(machine_real_restart_asm)
 
 /*
  * This is 16-bit protected mode code to disable paging and the cache,
diff --git a/arch/x86/realmode/rm/trampoline_64.S 
b/arch/x86/realmode/rm/trampoline_64.S
index c1aeab1dae25..251758ed7443 100644
--- a/arch/x86/realmode/rm/trampoline_64.S
+++ b/arch/x86/realmode/rm/trampoline_64.S
@@ -38,7 +38,7 @@
.code16
 
.balign PAGE_SIZE
-ENTRY(trampoline_start)
+SYM_CODE_START(trampoline_start)
cli # We should be safe anyway
wbinvd
 
@@ -78,12 +78,14 @@ ENTRY(trampoline_start)
 no_longmode:
hlt
jmp no_longmode
+S

[Xen-devel] [PATCH v8 20/28] x86/asm: make some functions local

[Jiri Slaby]

There is a couple of assembly functions, which are invoked only locally
in the file they are defined. In C, we mark them "static". In assembly,
annotate them using SYM_{FUNC,CODE}_START_LOCAL (and switch their
ENDPROC to SYM_{FUNC,CODE}_END too). Whether we use FUNC or CODE,
depends on whether ENDPROC or END was used for a particular function
before.

Signed-off-by: Jiri Slaby 
Cc: "H. Peter Anvin" 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: x...@kernel.org
Cc: Matt Fleming 
Cc: Ard Biesheuvel 
Cc: linux-...@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/boot/compressed/efi_thunk_64.S |  8 
 arch/x86/entry/entry_64.S   | 21 +++--
 arch/x86/lib/copy_page_64.S |  4 ++--
 arch/x86/lib/memcpy_64.S| 12 ++--
 arch/x86/lib/memset_64.S|  8 
 arch/x86/platform/efi/efi_thunk_64.S| 12 ++--
 arch/x86/platform/pvh/head.S|  4 ++--
 7 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_thunk_64.S 
b/arch/x86/boot/compressed/efi_thunk_64.S
index d66000d23921..31312070db22 100644
--- a/arch/x86/boot/compressed/efi_thunk_64.S
+++ b/arch/x86/boot/compressed/efi_thunk_64.S
@@ -99,12 +99,12 @@ ENTRY(efi64_thunk)
ret
 ENDPROC(efi64_thunk)
 
-ENTRY(efi_exit32)
+SYM_FUNC_START_LOCAL(efi_exit32)
movqfunc_rt_ptr(%rip), %rax
push%rax
mov %rdi, %rax
ret
-ENDPROC(efi_exit32)
+SYM_FUNC_END(efi_exit32)
 
.code32
 /*
@@ -112,7 +112,7 @@ ENDPROC(efi_exit32)
  *
  * The stack should represent the 32-bit calling convention.
  */
-ENTRY(efi_enter32)
+SYM_FUNC_START_LOCAL(efi_enter32)
movl$__KERNEL_DS, %eax
movl%eax, %ds
movl%eax, %es
@@ -172,7 +172,7 @@ ENTRY(efi_enter32)
btsl$X86_CR0_PG_BIT, %eax
movl%eax, %cr0
lret
-ENDPROC(efi_enter32)
+SYM_FUNC_END(efi_enter32)
 
.data
.balign 8
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 50c19edbf639..6d6cb84952ff 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -1099,7 +1099,8 @@ idtentry hypervisor_callback xen_do_hypervisor_callback 
has_error_code=0
  * existing activation in its critical region -- if so, we pop the current
  * activation and restart the handler using the previous one.
  */
-ENTRY(xen_do_hypervisor_callback)  /* 
do_hypervisor_callback(struct *pt_regs) */
+/* do_hypervisor_callback(struct *pt_regs) */
+SYM_CODE_START_LOCAL(xen_do_hypervisor_callback)
 
 /*
  * Since we don't modify %rdi, evtchn_do_upall(struct *pt_regs) will
@@ -1117,7 +1118,7 @@ ENTRY(xen_do_hypervisor_callback) /* 
do_hypervisor_callback(struct *pt_regs) */
callxen_maybe_preempt_hcall
 #endif
jmp error_exit
-END(xen_do_hypervisor_callback)
+SYM_CODE_END(xen_do_hypervisor_callback)
 
 /*
  * Hypervisor uses this for application faults while it executes.
@@ -1212,7 +1213,7 @@ idtentry machine_checkdo_mce  
has_error_code=0paranoid=1
  * Use slow, but surefire "are we in kernel?" check.
  * Return: ebx=0: need swapgs on exit, ebx=1: otherwise
  */
-ENTRY(paranoid_entry)
+SYM_CODE_START_LOCAL(paranoid_entry)
UNWIND_HINT_FUNC
cld
PUSH_AND_CLEAR_REGS save_ret=1
@@ -1239,7 +1240,7 @@ ENTRY(paranoid_entry)
SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg=%rax save_reg=%r14
 
ret
-END(paranoid_entry)
+SYM_CODE_END(paranoid_entry)
 
 /*
  * "Paranoid" exit path from exception stack.  This is invoked
@@ -1253,7 +1254,7 @@ END(paranoid_entry)
  *
  * On entry, ebx is "no swapgs" flag (1: don't need swapgs, 0: need it)
  */
-ENTRY(paranoid_exit)
+SYM_CODE_START_LOCAL(paranoid_exit)
UNWIND_HINT_REGS
DISABLE_INTERRUPTS(CLBR_ANY)
TRACE_IRQS_OFF_DEBUG
@@ -1270,12 +1271,12 @@ ENTRY(paranoid_exit)
RESTORE_CR3 scratch_reg=%rbx save_reg=%r14
 .Lparanoid_exit_restore:
jmp restore_regs_and_return_to_kernel
-END(paranoid_exit)
+SYM_CODE_END(paranoid_exit)
 
 /*
  * Save all registers in pt_regs, and switch GS if needed.
  */
-ENTRY(error_entry)
+SYM_CODE_START_LOCAL(error_entry)
UNWIND_HINT_FUNC
cld
PUSH_AND_CLEAR_REGS save_ret=1
@@ -1350,16 +1351,16 @@ ENTRY(error_entry)
callfixup_bad_iret
mov %rax, %rsp
jmp .Lerror_entry_from_usermode_after_swapgs
-END(error_entry)
+SYM_CODE_END(error_entry)
 
-ENTRY(error_exit)
+SYM_CODE_START_LOCAL(error_exit)
UNWIND_HINT_REGS
DISABLE_INTERRUPTS(CLBR_ANY)
TRACE_IRQS_OFF
testb   $3, CS(%rsp)
jz  retint_kernel
jmp retint_user
-END(error_exit)
+SYM_CODE_END(error_exit)
 
 /*
  * Runs on exception stack.  Xen PV does not go through this path at all,
diff --git a/arch/x86/lib/copy_page_64.S b/arch/x86/lib/copy_page_64.S
index fd2d09afa097..f505870bd93b 100644
--- a/arch

[Xen-devel] [PATCH v8 14/28] xen/pvh: annotate data appropriatelly

[Jiri Slaby]

Use the new SYM_DATA_START_LOCAL, and SYM_DATA_END* macros to have:
   8 OBJECT  LOCAL  DEFAULT6 gdt
  000832 OBJECT  LOCAL  DEFAULT6 gdt_start
  0028 0 OBJECT  LOCAL  DEFAULT6 gdt_end
  0028   256 OBJECT  LOCAL  DEFAULT6 early_stack
  0128 0 OBJECT  LOCAL  DEFAULT6 early_stack

Signed-off-by: Jiri Slaby 
Reviewed-by: Boris Ostrovsky 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: "H. Peter Anvin" 
Cc: x...@kernel.org
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/platform/pvh/head.S | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/platform/pvh/head.S b/arch/x86/platform/pvh/head.S
index 1f8825bbaffb..4e63480bb223 100644
--- a/arch/x86/platform/pvh/head.S
+++ b/arch/x86/platform/pvh/head.S
@@ -150,11 +150,12 @@ END(pvh_start_xen)
 
.section ".init.data","aw"
.balign 8
-gdt:
+SYM_DATA_START_LOCAL(gdt)
.word gdt_end - gdt_start
.long _pa(gdt_start)
.word 0
-gdt_start:
+SYM_DATA_END(gdt)
+SYM_DATA_START_LOCAL(gdt_start)
.quad 0x/* NULL descriptor */
 #ifdef CONFIG_X86_64
.quad GDT_ENTRY(0xa09a, 0, 0xf) /* PVH_CS_SEL */
@@ -163,15 +164,14 @@ gdt_start:
 #endif
.quad GDT_ENTRY(0xc092, 0, 0xf) /* PVH_DS_SEL */
.quad GDT_ENTRY(0x4090, 0, 0x18)/* PVH_CANARY_SEL */
-gdt_end:
+SYM_DATA_END_LABEL(gdt_start, SYM_L_LOCAL, gdt_end)
 
.balign 16
-canary:
-   .fill 48, 1, 0
+SYM_DATA_LOCAL(canary, .fill 48, 1, 0)
 
-early_stack:
+SYM_DATA_START_LOCAL(early_stack)
.fill BOOT_STACK_SIZE, 1, 0
-early_stack_end:
+SYM_DATA_END_LABEL(early_stack, SYM_L_LOCAL, early_stack_end)
 
ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY,
 _ASM_PTR (pvh_start_xen - __START_KERNEL_map))
-- 
2.22.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v8 24/28] x86_64/asm: change all ENTRY+ENDPROC to SYM_FUNC_*

[Jiri Slaby]

These are all functions which are invoked from elsewhere, so we annotate
them as global using the new SYM_FUNC_START. And their ENDPROC's by
SYM_FUNC_END.

And make sure ENTRY/ENDPROC is not defined on X86_64, given these were
the last users.

Signed-off-by: Jiri Slaby 
Reviewed-by: Rafael J. Wysocki  [hibernate]
Reviewed-by: Boris Ostrovsky  [xen bits]
Cc: "H. Peter Anvin" 
Cc: Thomas Gleixner 
Cc: Ingo Molnar 
Cc: x...@kernel.org
Cc: Herbert Xu 
Cc: "David S. Miller" 
Cc: "Rafael J. Wysocki" 
Cc: Len Brown 
Cc: Pavel Machek 
Cc: Matt Fleming 
Cc: Ard Biesheuvel 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: linux-cry...@vger.kernel.org
Cc: linux...@vger.kernel.org
Cc: linux-...@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
---
 arch/x86/boot/compressed/efi_thunk_64.S  |  4 +-
 arch/x86/boot/compressed/head_64.S   | 16 +++---
 arch/x86/boot/compressed/mem_encrypt.S   |  8 +--
 arch/x86/crypto/aegis128-aesni-asm.S | 28 -
 arch/x86/crypto/aes_ctrby8_avx-x86_64.S  | 12 ++--
 arch/x86/crypto/aesni-intel_asm.S| 60 ++--
 arch/x86/crypto/aesni-intel_avx-x86_64.S | 32 +--
 arch/x86/crypto/blowfish-x86_64-asm_64.S | 16 +++---
 arch/x86/crypto/camellia-aesni-avx-asm_64.S  | 24 
 arch/x86/crypto/camellia-aesni-avx2-asm_64.S | 24 
 arch/x86/crypto/camellia-x86_64-asm_64.S | 16 +++---
 arch/x86/crypto/cast5-avx-x86_64-asm_64.S| 16 +++---
 arch/x86/crypto/cast6-avx-x86_64-asm_64.S| 24 
 arch/x86/crypto/chacha-avx2-x86_64.S | 12 ++--
 arch/x86/crypto/chacha-avx512vl-x86_64.S | 12 ++--
 arch/x86/crypto/chacha-ssse3-x86_64.S| 12 ++--
 arch/x86/crypto/crc32-pclmul_asm.S   |  4 +-
 arch/x86/crypto/crc32c-pcl-intel-asm_64.S|  4 +-
 arch/x86/crypto/crct10dif-pcl-asm_64.S   |  4 +-
 arch/x86/crypto/des3_ede-asm_64.S|  8 +--
 arch/x86/crypto/ghash-clmulni-intel_asm.S|  8 +--
 arch/x86/crypto/nh-avx2-x86_64.S |  4 +-
 arch/x86/crypto/nh-sse2-x86_64.S |  4 +-
 arch/x86/crypto/poly1305-avx2-x86_64.S   |  4 +-
 arch/x86/crypto/poly1305-sse2-x86_64.S   |  8 +--
 arch/x86/crypto/serpent-avx-x86_64-asm_64.S  | 24 
 arch/x86/crypto/serpent-avx2-asm_64.S| 24 
 arch/x86/crypto/serpent-sse2-x86_64-asm_64.S |  8 +--
 arch/x86/crypto/sha1_avx2_x86_64_asm.S   |  4 +-
 arch/x86/crypto/sha1_ni_asm.S|  4 +-
 arch/x86/crypto/sha1_ssse3_asm.S |  4 +-
 arch/x86/crypto/sha256-avx-asm.S |  4 +-
 arch/x86/crypto/sha256-avx2-asm.S|  4 +-
 arch/x86/crypto/sha256-ssse3-asm.S   |  4 +-
 arch/x86/crypto/sha256_ni_asm.S  |  4 +-
 arch/x86/crypto/sha512-avx-asm.S |  4 +-
 arch/x86/crypto/sha512-avx2-asm.S|  4 +-
 arch/x86/crypto/sha512-ssse3-asm.S   |  4 +-
 arch/x86/crypto/twofish-avx-x86_64-asm_64.S  | 24 
 arch/x86/crypto/twofish-x86_64-asm_64-3way.S |  8 +--
 arch/x86/crypto/twofish-x86_64-asm_64.S  |  8 +--
 arch/x86/entry/entry_64.S| 10 ++--
 arch/x86/entry/entry_64_compat.S |  4 +-
 arch/x86/kernel/acpi/wakeup_64.S |  8 +--
 arch/x86/kernel/ftrace_64.S  | 20 +++
 arch/x86/kernel/irqflags.S   |  8 +--
 arch/x86/kvm/vmx/vmenter.S   | 12 ++--
 arch/x86/lib/checksum_32.S   |  8 +--
 arch/x86/lib/clear_page_64.S | 12 ++--
 arch/x86/lib/cmpxchg16b_emu.S|  4 +-
 arch/x86/lib/cmpxchg8b_emu.S |  4 +-
 arch/x86/lib/copy_page_64.S  |  4 +-
 arch/x86/lib/copy_user_64.S  | 16 +++---
 arch/x86/lib/csum-copy_64.S  |  4 +-
 arch/x86/lib/getuser.S   | 16 +++---
 arch/x86/lib/hweight.S   |  8 +--
 arch/x86/lib/iomap_copy_64.S |  4 +-
 arch/x86/lib/memcpy_64.S |  4 +-
 arch/x86/lib/memmove_64.S|  4 +-
 arch/x86/lib/memset_64.S |  4 +-
 arch/x86/lib/msr-reg.S   |  8 +--
 arch/x86/lib/putuser.S   | 16 +++---
 arch/x86/lib/retpoline.S |  4 +-
 arch/x86/mm/mem_encrypt_boot.S   |  8 +--
 arch/x86/platform/efi/efi_stub_64.S  |  4 +-
 arch/x86/platform/efi/efi_thunk_64.S |  4 +-
 arch/x86/power/hibernate_asm_64.S|  8 +--
 arch/x86/xen/xen-asm.S   | 28 -
 arch/x86/xen/xen-asm_64.S| 16 +++---
 include/linux/linkage.h  |  4 ++
 70 files changed, 379 insertions(+), 375 deletions(-)

diff --git a/arch/x86/boot/compressed/efi_thunk_64.S 
b/arch/x86/boot/compressed/efi_thunk_64.S
index 31312070db22..593913692d16 100644
--- a/arch/x86/boot/compressed/efi_thunk_64.S
+++ b/arch/x86/boot/compressed/efi_thunk_64.S
@@ -23,7 +23,7 @@
 
.code64
.text
-

Re: [Xen-devel] [edk2-devel] [PATCH v4 12/35] OvmfPkg/XenPlatformPei: Grab RSDP from PVH guest start of day struct

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 11:26:41AM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 04:35:58PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:21PM +0100, Anthony PERARD wrote:
> > > Check if there's a start of the day struct provided to PVH guest, save
> > > the ACPI RSDP address for later.
> > > 
> > > This patch import import arch-x86/hvm/start_info.h from xen.git.
> > 
> > You seem to change the types when importing start_info.h, is that
> > absolutely necessary?
> 
> I guess one could try to map xen's types to EDKII's type with typedefs,
> but I'm not sur how well that would work. Importing the xen headers is
> documented so changing the types is fairly easy, see
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/IndustryStandard/Xen/README
> 
> Also, changing the header further might have been something useful to
> do, we could have match EDKII's naming convention and source files would
> have looked a bit less weird.

Ack, didn't know there was a README for this procedure.

> > From my experience working with different projects when importing such
> > headers it's better to keep them verbatim, this makes importing future
> > versions from upstream easier.
> >
> > I have a comment below, but it's more like a question.
> > 
> > > diff --git a/OvmfPkg/XenPlatformPei/Xen.c b/OvmfPkg/XenPlatformPei/Xen.c
> > > index 5c7d7ddc1c..b366139a0a 100644
> > > --- a/OvmfPkg/XenPlatformPei/Xen.c
> > > +++ b/OvmfPkg/XenPlatformPei/Xen.c
> > > @@ -25,6 +25,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include "Platform.h"
> > >  #include "Xen.h"
> > > @@ -86,6 +87,7 @@ XenConnect (
> > >UINT32 XenVersion;
> > >EFI_XEN_OVMF_INFO *Info;
> > >CHAR8 Sig[sizeof (Info->Signature) + 1];
> > > +  UINT32 *PVHResetVectorData;
> > >  
> > >AsmCpuid (XenLeaf + 2, &TransferPages, &TransferReg, NULL, NULL);
> > >mXenInfo.HyperPages = AllocatePages (TransferPages);
> > > @@ -121,6 +123,29 @@ XenConnect (
> > >  mXenHvmloaderInfo = NULL;
> > >}
> > >  
> > > +  mXenInfo.RsdpPvh = NULL;
> > > +
> > > +  //
> > > +  // Locate and use information from the start of day structure if we 
> > > have
> > > +  // booted via the PVH entry point.
> > > +  //
> > > +
> > > +  PVHResetVectorData = (VOID *)(UINTN) PcdGet32 
> > > (PcdXenPvhStartOfDayStructPtr);
> > > +  //
> > > +  // That magic value is written in XenResetVector/Ia32/XenPVHMain.asm
> > > +  //
> > > +  if (PVHResetVectorData[1] == SIGNATURE_32 ('X', 'P', 'V', 'H')) {
> > > +struct hvm_start_info *HVMStartInfo;
> > > +
> > > +HVMStartInfo = (VOID *)(UINTN) PVHResetVectorData[0];
> > > +if (HVMStartInfo->magic == XEN_HVM_START_MAGIC_VALUE) {
> > > +  ASSERT (HVMStartInfo->rsdp_paddr != 0);
> > > +  if (HVMStartInfo->rsdp_paddr != 0) {
> > > +mXenInfo.RsdpPvh = (VOID *)(UINTN)HVMStartInfo->rsdp_paddr;
> > 
> > I guess you can do this because OVMF has an identity map of virtual
> > addresses to physical addresses?
> 
> I think so, yes. I know that early code does created page table like
> that, but I don't know if later code rework those page table or not.
> 
> > I wonder the size of such identity map, and whether you need to check
> > that the rsdp address is indeed inside of that region before
> > converting it to a pointer. The same would apply to
> > PcdXenPvhStartOfDayStructPtr, but that's likely safe because it's
> > always < 4GB, which I assume OVMF always has identity mapped?
> 
> PcdXenPvhStartOfDayStructPtr is safe because OVMF owns it. As for the
> rspd_paddr* and the HVMStartInfo*, I need to check. As you say, it's
> probably fine as long as it's <4GB.
> 
> I've looked at the comment here:
> https://github.com/tianocore/edk2/blob/master/OvmfPkg/ResetVector/Ia32/PageTables64.asm#L94
> This mean that the code executed in the patch (accessing the hvm start
> info struct) is executed while the id map is setup up to 4GB. So as long
> as the struct is below 4G, it's fine.

Yes, the start_info struct is guaranteed to be below 4GB because the
physical memory address of it is passed on a register when starting
the kernel in 32bit protected mode, so the address cannot be greater
than 4GB or it would be truncated.

> As for the RSDP, I think that pointer is accessed much later, when a
> different page table is setup, I think that would be that code:
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> But I'm not sure how much is setup. But I'm guessing that whatever is
> pointed by RSDP, it will be in the 1:1, because we tell the UEFI about
> it while parsing the e820.

OK, as long as we know it's safe to access. Note it's quite likely the
rsdp is also below 4GB anyway, but better be safe than sorry.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 22/35] OvmfPkg/XenPlatformPei: no hvmloader: get the E820 table via hypercall

[Anthony PERARD]

On Wed, Aug 07, 2019 at 05:14:33PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:31PM +0100, Anthony PERARD wrote:
> > When the Xen PVH entry point has been used, hvmloader hasn't run and
> > hasn't prepared an E820 table. The only way left to get an E820 table
> > is to ask Xen via an hypercall, the call can only be made once so keep
> > the result cached for later.
> 
> I think we agreed that the above is not true, and that the memory
> map can be fetched as many times as desired using the hypercall
> interface.

Yes, I'll change the commit message. How about:

  When the Xen PVH entry point has been used, hvmloader hasn't run and
  hasn't prepared an E820 table. The only way left to get an E820 table
  is to ask Xen via an hypercall.  We keep the result cached to avoid
  making a second hypercall which would give the same result.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 20/35] OvmfPkg/XenPlatformPei: Introduce XenPvhDetected

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 11:38:13AM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 05:03:46PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:29PM +0100, Anthony PERARD wrote:
> > > +BOOLEAN
> > > +XenPvhDetected (
> > > +  VOID
> > > +  )
> > > +{
> > > +  //
> > > +  // This function should only be used after XenConnect
> > > +  //
> > > +  ASSERT (mXenInfo.VersionMajor != 0);
> > 
> > That's IMO dangerous. Using the version as an indication that
> > XenConnect has run seems like a bad idea, since returning a major
> > version of 0 is a valid number to return. Can't you check against
> > something else that doesn't depends on hypervisor provided data? (ie:
> > like some allocations or such that happen in XenConnect)
> > 
> > A paranoid could provider could even return major == 0 and minor == 0
> > in order to attempt to hide the Xen version used, since guests are not
> > supposed to infer anything from the Xen version, available hypervisor
> > features are reported by other means.
> 
> I'm sure a paranoid provider wouldn't use a debug build of OVMF :-). So
> that assert doesn't matter. There's nothing dangerous in a `nop'! :-D
> 
> But I could use mXenInfo.HyperPages instead.

It's just a nit, and TBH it's quite unlikely for anyone to report a
major version of 0, it's just that if you have something else to
assert for initialization it might be safer.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 22/35] OvmfPkg/XenPlatformPei: no hvmloader: get the E820 table via hypercall

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 11:41:18AM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 05:14:33PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:31PM +0100, Anthony PERARD wrote:
> > > When the Xen PVH entry point has been used, hvmloader hasn't run and
> > > hasn't prepared an E820 table. The only way left to get an E820 table
> > > is to ask Xen via an hypercall, the call can only be made once so keep
> > > the result cached for later.
> > 
> > I think we agreed that the above is not true, and that the memory
> > map can be fetched as many times as desired using the hypercall
> > interface.
> 
> Yes, I'll change the commit message. How about:
> 
>   When the Xen PVH entry point has been used, hvmloader hasn't run and
>   hasn't prepared an E820 table. The only way left to get an E820 table
>   is to ask Xen via an hypercall.  We keep the result cached to avoid
>   making a second hypercall which would give the same result.

LGTM, thanks.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Jan Beulich]

On 08.08.2019 11:13, Julien Grall wrote:
> Hi Jan,
> 
> On 08/08/2019 10:04, Jan Beulich wrote:
>> On 08.08.2019 10:43, Andrew Cooper wrote:
>>> On 08/08/2019 07:22, Jan Beulich wrote:
 On 07.08.2019 21:41, Andrew Cooper wrote:
> --- /dev/null
> +++ b/docs/glossary.rst
> @@ -0,0 +1,37 @@
> +Glossary
> +
> +
> +.. Terms should appear in alphabetical order
> +
> +.. glossary::
> +
> +   control domain
> + A :term:`domain`, commonly dom0, with the permission and
> responsibility
> + to create and manage other domains on the system.
> +
> +   domain
> + A domain is Xen's unit of resource ownership, and generally has
> at the
> + minimum some RAM and virtual CPUs.
> +
> + The terms :term:`domain` and :term:`guest` are commonly used
> + interchangeably, but they mean subtly different things.
> +
> + A guest is a single virtual machine.
> +
> + Consider the case of live migration where, for a period of
> time, one
> + guest will be comprised of two domains, while it is in transit.
> +
> +   domid
> + The numeric identifier of a running :term:`domain`.  It is
> unique to a
> + single instance of Xen, used as the identifier in various APIs,
> and is
> + typically allocated sequentially from 0.
> +
> +   guest
> + See :term:`domain`

 I think you want to mention the usual distinction here: Dom0 is,
 while a domain, commonly not considered a guest.
>>>
>>> To be honest, I had totally forgotten about that.  I guess now is the
>>> proper time to rehash it in public.
>>>
>>> I don't think the way it currently gets used has a clear or coherent set
>>> of rules, because I can't think of any to describe how it does get used.
>>>
>>> Either there are a clear and coherent (and simple!) set of rules for
>>> what we mean by "guest", at which point they can live here in the
>>> glossary, or the fuzzy way it is current used should cease.
>>
>> What's fuzzy about Dom0 not being a guest (due to being a part of the
>> host instead)?
> Dom0 is not part of the host if you are using an hardware domain.

It's still the control domain then, and hence still part of the host.

> I actually thought we renamed everything in Xen from Dom0 to hwdom
> to avoid the confusion.

As to variable naming - mostly, I think.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Oleksandr Tyshchenko]

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.

Signed-off-by: Oleksandr Tyshchenko 
---
I have heard there is a "possible" case when the IOMMU could be accessible by 
DOM0.
So, I think, for this to work we need to create corresponding DT nodes in the DT
at least.
---
 xen/arch/arm/domain_build.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d67f7d4..ff88099 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1403,9 +1403,11 @@ static int __init handle_node(struct domain *d, struct 
kernel_info *kinfo,
 
 /*
  * Even if the IOMMU device is not used by Xen, it should not be
- * passthrough to DOM0
+ * passthrough to DOM0. The exception here is the fact that IOMMU
+ * has been forcibly disabled in bootargs "iommu=0" in order to let
+ * the IOMMU be accessible by DOM0.
  */
-if ( device_get_class(node) == DEVICE_IOMMU )
+if ( device_get_class(node) == DEVICE_IOMMU && iommu_enable )
 {
 dt_dprintk(" IOMMU, skip it\n");
 return 0;
-- 
2.7.4


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:
> From: Oleksandr Tyshchenko 
> 
> Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
> forcibly disabled in bootargs (e.g. "iommu=0") in order to let
> the IOMMU be accessible by DOM0.
> 
> Signed-off-by: Oleksandr Tyshchenko 
> ---
> I have heard there is a "possible" case when the IOMMU could be accessible by 
> DOM0.
> So, I think, for this to work we need to create corresponding DT nodes in the 
> DT
> at least.

dom0 on ARM being an autotranslated guest I'm not sure how it's going
to program the DMA remapping in the iommu, since it doesn't know the
mfns of the memory it uses at all, hence I don't see the point in
exposing the hardware iommu to dom0 unless there's some emulation done
to make dom0 able to access it.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 3/6] [RFC] xen/common: Introduce _xrealloc function

[Oleksandr]

On 08.08.19 10:05, Jan Beulich wrote:

Hi Jan


(I'm sorry if you receive duplicates of this, but I've got a reply
back from our mail system that several of the recipients did not
have their host names resolved correctly on the first attempt.)


Absolutely no problem.



Jan, how this could be technically implemented, or are these any existing 
examples in Xen?

See x86's copy_to_guest_offset(), for example. To get the compiler
to emit a warning (at least), a (typically otherwise dead)
comparison of pointers is commonly used.



Thank you for the pointer. It is clear now.


--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v4 23/35] OvmfPkg/XenPlatformPei: Rework memory detection

[Anthony PERARD]

On Wed, Aug 07, 2019 at 05:34:32PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:32PM +0100, Anthony PERARD wrote:
> > When running as a Xen PVH guest, there is no CMOS to read the memory
> > size from.  Rework GetSystemMemorySize(Below|Above)4gb() so they can
> > work without CMOS by reading the e820 table.
> > 
> > Rework XenPublishRamRegions to also care for the reserved and ACPI
> > entry in the e820 table. The region that was added by InitializeXen()
> > isn't needed as that same entry is in the e820 table provided by
> > hvmloader.
> > 
> > MTRR settings aren't modified anymore, on HVM it's already done by
> > hvmloader, on PVH it is supposed to have sane default. MTRR will need
> > to be done properly but keeping what's already been done by programmes
>   ^ firmware

I've used programmes instead of firmware because in case of PVH, OVMF is
the first firmware to run, libxl (and what ever it called) is what
causes an MTRR to be setup, no firmware are involved in that.

> > +//
> > +// Round up the start address, and round down the end address.
> > +//
> > +Base = ALIGN_VALUE (Entry->BaseAddr, (UINT64)EFI_PAGE_SIZE);
> > +End = (Entry->BaseAddr + Entry->Length) & ~(UINT64)EFI_PAGE_MASK;
> > +
> > +switch (Entry->Type) {
> > +case EfiAcpiAddressRangeMemory:
> > +  AddMemoryRangeHob (Base, End);
> > +  break;
> > +case EfiAcpiAddressRangeACPI:
> > +  AddReservedMemoryRangeHob (Base, End, FALSE);
> > +  break;
> > +case EfiAcpiAddressRangeReserved:
> > +  if (Base < LocalApic && LocalApic < End) {
> 
> Don't you also need to check for equality? In case such region starts
> at Base == LocalApic?
> 
> I guess it doesn't matter that much since this is to workaround a
> specific issue with hvmloader, but I would like to see this sorted out
> in hvmloader so that there's no clash anymore.

Indeed, it doesn't matter to much, so I've been lazy. But Laszlo have
pointed that out as well, so there were going to be a patch to make the
workaround better. But I feel like I'm going to need to repost the
series, so I'm probably going to fix that as well.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [qemu-mainline test] 139796: regressions - FAIL

[osstest service owner]

flight 139796 qemu-mainline real [real]
http://logs.test-lab.xenproject.org/osstest/logs/139796/

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemuu-debianhvm-amd64  7 xen-boot fail REGR. vs. 139766

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 139766
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 139766
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 139766
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 139766
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 139766
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-arm64-arm64-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit2  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-credit1  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-armhf-armhf-xl-vhd  12 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-vhd  13 saverestore-support-checkfail   never pass
 test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass
 test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass

version targeted for testing:
 qemuu864ab314f1d924129d06ac7b571f105a2b76a4b2
baseline version:
 qemuu9bb68d34dda9be60335e73e65c8fb61bca035362

Last test of basis   139766  2019-08-06 11:37:45 Z1 days
Testing same since   139796  2019-08-07 07:30:34 Z1 days1 attempts


People who touched revisions under test:
  Cornelia Huck 
  Max Reitz 
  Peter Maydell 
  Vladimir Sementsov-Ogievskiy 

jobs:
 build-amd64-xsm  pass
 build-arm64-xsm  pass
 build-i386-xsm   pass
 buil

Re: [Xen-devel] Terminology for "guest" - Was: [PATCH] docs/sphinx: Introduction

[Rich Persaud]

On Aug 8, 2019, at 06:49, Jan Beulich  wrote:
> 
>> On 08.08.2019 11:13, Julien Grall wrote:
>> Hi Jan,
>> 
>>> On 08/08/2019 10:04, Jan Beulich wrote:
 On 08.08.2019 10:43, Andrew Cooper wrote:
> On 08/08/2019 07:22, Jan Beulich wrote:
>> On 07.08.2019 21:41, Andrew Cooper wrote:
>> --- /dev/null
>> +++ b/docs/glossary.rst
>> @@ -0,0 +1,37 @@
>> +Glossary
>> +
>> +
>> +.. Terms should appear in alphabetical order
>> +
>> +.. glossary::
>> +
>> +   control domain
>> + A :term:`domain`, commonly dom0, with the permission and
>> responsibility
>> + to create and manage other domains on the system.
>> +
>> +   domain
>> + A domain is Xen's unit of resource ownership, and generally has
>> at the
>> + minimum some RAM and virtual CPUs.
>> +
>> + The terms :term:`domain` and :term:`guest` are commonly used
>> + interchangeably, but they mean subtly different things.
>> +
>> + A guest is a single virtual machine.
>> +
>> + Consider the case of live migration where, for a period of
>> time, one
>> + guest will be comprised of two domains, while it is in transit.
>> +
>> +   domid
>> + The numeric identifier of a running :term:`domain`.  It is
>> unique to a
>> + single instance of Xen, used as the identifier in various APIs,
>> and is
>> + typically allocated sequentially from 0.
>> +
>> +   guest
>> + See :term:`domain`
> 
> I think you want to mention the usual distinction here: Dom0 is,
> while a domain, commonly not considered a guest.
 
 To be honest, I had totally forgotten about that.  I guess now is the
 proper time to rehash it in public.
 
 I don't think the way it currently gets used has a clear or coherent set
 of rules, because I can't think of any to describe how it does get used.
 
 Either there are a clear and coherent (and simple!) set of rules for
 what we mean by "guest", at which point they can live here in the
 glossary, or the fuzzy way it is current used should cease.
>>> 
>>> What's fuzzy about Dom0 not being a guest (due to being a part of the
>>> host instead)?
>> Dom0 is not part of the host if you are using an hardware domain.
> 
> It's still the control domain then, and hence still part of the host.

With disaggregation and dom0less (how might we describe that term in the 
intro?) for edge/embedded Xen systems, there could be a mode where the control 
domain has never had privilege over the domain that handles the physical TPM, 
or the provider of the virtual TPM:  

  https://lists.gt.net/xen/devel/557782

Rich___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Oleksandr]

On 08.08.19 14:01, Roger Pau Monné wrote:

Hi, Roger.



On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.

Signed-off-by: Oleksandr Tyshchenko 
---
I have heard there is a "possible" case when the IOMMU could be accessible by 
DOM0.
So, I think, for this to work we need to create corresponding DT nodes in the DT
at least.

dom0 on ARM being an autotranslated guest I'm not sure how it's going
to program the DMA remapping in the iommu, since it doesn't know the
mfns of the memory it uses at all, hence I don't see the point in
exposing the hardware iommu to dom0 unless there's some emulation done
to make dom0 able to access it.


Currently, Dom0 on ARM is always 1:1 mapped (gfn == mfn). However, I 
don't really know how long this assumption it is going to be true.


Being honest, I don't need this use-case at the moment. I have 
experimented with the code which creates DT nodes for Dom0 a bit, and 
recollected


about the possibility for the IOMMU to be accessible by Dom0. If this 
patch doesn't have any real value, I would be ok to just drop it.




Thanks, Roger.


--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Marek Marczykowski-Górecki]

On Thu, Aug 08, 2019 at 10:40:36AM +0100, Andrew Cooper wrote:
> On 08/08/2019 01:31, Marek Marczykowski-Górecki wrote:
> > When booting Xen via xen.efi, there is /mapbs option to workaround
> > certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
> > map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
> > cmdline for the same effect and parse it very early in the
> > multiboot2+EFI boot path.
> >
> > Normally cmdline is parsed after relocating MB2 structure, which happens
> > too late. To have efi= parsed early enough, save cmdline pointer in
> > head.S and pass it as yet another argument to efi_multiboot2(). This
> > way we avoid introducing yet another MB2 structure parser.
> >
> > To keep consistency, handle efi= parameter early in xen.efi too, both in
> > xen.efi command line and cfg file.
> >
> > Signed-off-by: Marek Marczykowski-Górecki 
> 
> I'm very sorry to do this to you, but I'm going to object to the patch
> (in principle.  I think the command line option itself is fine but...)
> 
> What does Linux do differently here?

I've found this comment:
/*
 * The UEFI specification makes it clear that the operating system is
 * free to do whatever it wants with boot services code after
 * ExitBootServices() has been called. Ignoring this recommendation a
 * significant bunch of EFI implementations continue calling into boot
 * services code (SetVirtualAddressMap). In order to work around such
 * buggy implementations we reserve boot services region during EFI
 * init and make sure it stays executable. Then, after
 * SetVirtualAddressMap(), it is discarded.
 *
 * However, some boot services regions contain data that is required
 * by drivers, so we need to track which memory ranges can never be
 * freed. This is done by tagging those regions with the
 * EFI_MEMORY_RUNTIME attribute.
 *
 * Any driver that wants to mark a region as reserved must use
 * efi_mem_reserve() which will insert a new EFI memory descriptor
 * into efi.memmap (splitting existing regions if necessary) and tag
 * it with EFI_MEMORY_RUNTIME.
 */

So, for start, Linux has "/mapbs" enabled by default. But as you see in
the other thread, it isn't enough. Given the above comment, I more and
more think it is also about SetVirtualAddressMap() call. According to a
git log, SetVirtualAddressMap is not called in Xen, as it's incompatible
with kexec (can be called only once). Looking at Linux code, this is
worked around by passing efi memory regions from the old map to the new
one - at least this is my understanding of kexec_enter_virtual_mode() in
arch/x86/platform/efi/efi.c. For example this comment:

/*
* Map efi regions which were passed via setup_data. The virt_addr is a
* fixed addr which was used in first kernel of a kexec boot.
*/

For my use case, I don't care about kexec, so I'd happily enable
SetVirtualAddressMap() call (it's under #ifdef), but according to
comments and that it wasn't touched since 2011, I don't expect it work
anymore.


> It is actively damaging to the Xen community to users to force users
> tweak command lines in order to boot/recover their system, and it looks
> especially embarrassing when other OSes cope automatically.  We have
> compatibility for all kinds of other firmware screw-ups, except EFI it
> seems, and this needs to change.

I _guess_ calling SetVirtualAddressMap() would help here, but it is much
more complex change than I'd like to do now.
What do you think about adding this option _and_ a heuristic when to
enable it automatically? In this case I'd say to start with
vendor=Lenovo based on my experience...

> So while I have no objection to the option per say, I don't think this
> patch is reasonable as a "fix" to the problem as far as end users are
> concerned.
> 
> ~Andrew

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?


signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 02:23:51PM +0300, Oleksandr wrote:
> 
> On 08.08.19 14:01, Roger Pau Monné wrote:
> 
> Hi, Roger.
> 
> 
> > On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:
> > > From: Oleksandr Tyshchenko 
> > > 
> > > Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
> > > forcibly disabled in bootargs (e.g. "iommu=0") in order to let
> > > the IOMMU be accessible by DOM0.
> > > 
> > > Signed-off-by: Oleksandr Tyshchenko 
> > > ---
> > > I have heard there is a "possible" case when the IOMMU could be 
> > > accessible by DOM0.
> > > So, I think, for this to work we need to create corresponding DT nodes in 
> > > the DT
> > > at least.
> > dom0 on ARM being an autotranslated guest I'm not sure how it's going
> > to program the DMA remapping in the iommu, since it doesn't know the
> > mfns of the memory it uses at all, hence I don't see the point in
> > exposing the hardware iommu to dom0 unless there's some emulation done
> > to make dom0 able to access it.
> 
> Currently, Dom0 on ARM is always 1:1 mapped (gfn == mfn). However, I don't
> really know how long this assumption it is going to be true.

Yes, I didn't had this in mind when writing the above reply. With
identity mapping in second stage translation it's indeed true that
dom0 might be able to somehow manage an iommu, but I don't think it's
a good idea to rely on this bodge (the identity mappings), and hence I
would advise against exposing the native iommu to dom0.

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Jan Beulich]

On 08.08.2019 11:45, Andrew Cooper wrote:
> On 08/08/2019 10:08, Jan Beulich wrote:
>> On 08.08.2019 10:33, Andrew Cooper wrote:
>>> On 08/08/2019 05:50, Juergen Gross wrote:
 On 07.08.19 20:11, Andrew Cooper wrote:
> 
> Its not exactly the easiest to dump to follow.
>
> First of all - I don't see why the hold/block time are printed like
> that.  It
> might be a holdover from the 32bit build, pre PRId64 support.  They
> should
> probably use PRI_stime anyway.
 Fine with me.

> The domid rendering is unfortunate.  Ideally we'd use %pd but that would
> involve rearranging the logic to get a struct domain* in hand.
> Seeing as
> you're the last person to modify this code, how hard would that be to
> do?
 It would completely break the struct type agnostic design.
>>> Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
>>> would be a trivial adjustment in the vsnprintf code, and could plausibly
>>> be useful elsewhere where we have a domid and not a domain pointer.
>> At the risk of upsetting / annoying you:
> 
> Yes you really have
> 
>> A domid_t would again
>> better be formatted via %od (and without the need to take the
>> address of a respective variable). In the case here it would
>> have the minor additional benefit of conserving on format string
>> length.
> 
> There are concrete reasons why it is a terrible idea, because none of
> this has anything to do with octal, and that %od has a well defined
> meaning which is not related to domid's.

I'm curious to learn what well defined meaning %od has.

>  There is also a very good
> reason why all the magic is hidden behind one single formatter.
> 
> You seem hell bent on making it actively confusing and complicated to
> write and read printk()'s, and I am not willing to lumber anyone
> developing on Xen with this cognitive complexity.
> 
> I am stick to death of having the same over and over, so let me stop any
> further wasting of time and be absolutely crystal clear.
> 
> NACK to any form of overloading %o

In which case, if I took a similar position for the PCI SBDF
formatting using %pp, we'd be in a dead end. Waste of time or not
- while you have made crystal clear why you personally dislike
overloading %o, afaic you've not provided any non-subjective
(i.e. truly technical) reasons, which would be what might help
convince me of sticking to just %p overloading.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Juergen Gross]

On 08.08.19 12:28, Julien Grall wrote:



On 08/08/2019 08:51, Juergen Gross wrote:

On 08.08.19 08:58, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:
Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.


But that would increase the size of the union to 4 bytes instead of 2.
So at least pad and cpu must be unsigned short or (better) uint16_t.


How about bool irq_safe:1?


I didn't question that, but OTOH I'm not sure doing something like:

struct {
  bool unseen:1;
  bool irq_safe:1;
  uint16_t pad:2;
  uint16_t cpu:12;
}

is guaranteed to be 2 bytes long. I think pad will be still put into the
first byte, but the compiler might decide that the 4 bits left won't be
able to hold the next 12 bits so it could start a new uint16_t at offset
2.

Moving the bool types to the end of the struct would avoid that IMHO.


Juergen


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Jan Beulich]

On 08.08.2019 12:28, Julien Grall wrote:
> On 08/08/2019 08:51, Juergen Gross wrote:
>> On 08.08.19 08:58, Jan Beulich wrote:
>>> On 07.08.2019 16:31, Juergen Gross wrote:
>>> Do we have an implied assumption somewhere that unsigned short is
>>> exactly 16 bits wide? I think "val" wants to be uint16_t here (as
>>> you really mean "exactly 16 bits"), the two boolean fields want
>>> to be bool, and the remaining two ones unsigned int.
>>
>> But that would increase the size of the union to 4 bytes instead of 2.
>> So at least pad and cpu must be unsigned short or (better) uint16_t.
> 
> How about bool irq_safe:1?

That's what I had suggested, indeed. Jürgen's response was for
my "unsigned int" suggestion towards the two non-boolean fields.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/4] enhance lock debugging

[Juergen Gross]

On 08.08.19 12:20, Andrew Cooper wrote:

On 08/08/2019 10:36, Juergen Gross wrote:

On 08.08.19 10:33, Andrew Cooper wrote:

On 08/08/2019 05:50, Juergen Gross wrote:

On 07.08.19 20:11, Andrew Cooper wrote:



Its not exactly the easiest to dump to follow.

First of all - I don't see why the hold/block time are printed like
that.  It
might be a holdover from the 32bit build, pre PRId64 support.  They
should
probably use PRI_stime anyway.


Fine with me.


The domid rendering is unfortunate.  Ideally we'd use %pd but that
would
involve rearranging the logic to get a struct domain* in hand.
Seeing as
you're the last person to modify this code, how hard would that be to
do?


It would completely break the struct type agnostic design.


Ok.  As an alternatively, how about %pdr which takes a raw domid?  It
would be a trivial adjustment in the vsnprintf code, and could plausibly
be useful elsewhere where we have a domid and not a domain pointer.


Lock profiling has no knowledge that it is working on a struct domain.
It is just working on a lock in a struct and an index to differentiate
the struct instance. Using the domid as the index is just for making it
more easy to understand the printout.

I wouldn't want e.g. a per-event lock to be named "IDLE" just because
it happens to be index 32767.


Ok, but clearly there is something which distinguishes domain indices
from other indices?


Not for lock profiling.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Jan Beulich]

On 08.08.2019 13:53, Juergen Gross wrote:
> On 08.08.19 12:28, Julien Grall wrote:
>> On 08/08/2019 08:51, Juergen Gross wrote:
>>> On 08.08.19 08:58, Jan Beulich wrote:
 On 07.08.2019 16:31, Juergen Gross wrote:
 Do we have an implied assumption somewhere that unsigned short is
 exactly 16 bits wide? I think "val" wants to be uint16_t here (as
 you really mean "exactly 16 bits"), the two boolean fields want
 to be bool, and the remaining two ones unsigned int.
>>>
>>> But that would increase the size of the union to 4 bytes instead of 2.
>>> So at least pad and cpu must be unsigned short or (better) uint16_t.
>>
>> How about bool irq_safe:1?
> 
> I didn't question that, but OTOH I'm not sure doing something like:
> 
> struct {
>    bool unseen:1;
>    bool irq_safe:1;
>    uint16_t pad:2;
>    uint16_t cpu:12;
> }
> 
> is guaranteed to be 2 bytes long. I think pad will be still put into the
> first byte, but the compiler might decide that the 4 bits left won't be
> able to hold the next 12 bits so it could start a new uint16_t at offset
> 2.
> 
> Moving the bool types to the end of the struct would avoid that IMHO.

Moving the two bool-s further down will also simplify extraction and
insertion of the "cpu" field.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part2 v2 3/6] create-diff-object: Add is_special_section() helper function function

[Pawel Wieczorkiewicz]

This function determines, based on the given section name, if the
sections belongs to the special sections category.

Add more special sections to special_sections[] along with a new
undefined_group_size() helper function returning 0 (undefined).
New special sections are: .altinstr_replacement.

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Andra-Irina Paraschiv 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Norbert Manthey 
---
v2:
* Applied suggestions from Ross:
  - const for the parameter
  - reusing special_sections array rather than duplicating it
* moving the function from common.c to create-diff-object.c
* strictly checking for full section names
---
 create-diff-object.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index 4699ba0..0df3fea 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1045,6 +1045,18 @@ static struct special_section special_sections[] = {
{},
 };
 
+static int is_special_section(const struct section *sec)
+{
+   struct special_section *special;
+
+   for (special = special_sections; special->name; special++) {
+   if (!strcmp(sec->name, special->name))
+   return true;
+   }
+
+   return false;
+}
+
 static int should_keep_rela_group(struct section *sec, int start, int size)
 {
struct rela *rela;
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Julien Grall]

On 08/08/2019 12:23, Oleksandr wrote:


On 08.08.19 14:01, Roger Pau Monné wrote:

Hi, Roger.



On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.


I don't think your code is doing what you expect... If iommu=0, then Xen will 
not lookup for IOMMUs (iommu_hardware_setup() will not be called). So none of 
the device will have DEVICE_IOMMU set and hence they are already given to dom0.


But I think it is wrong to give the IOMMUs to Dom0 when iommu=0. This is not the 
goal of this option. If you want to passthrough the IOMMU to Dom0, then you 
should use the parameter iommu_hwdom_passthrough.


However, I agree with Roger that giving the IOMMU to dom0 is a pretty bad idea. 
So this should be fixed.




Signed-off-by: Oleksandr Tyshchenko 
---
I have heard there is a "possible" case when the IOMMU could be accessible by 
DOM0.

So, I think, for this to work we need to create corresponding DT nodes in the DT
at least.

dom0 on ARM being an autotranslated guest I'm not sure how it's going
to program the DMA remapping in the iommu, since it doesn't know the
mfns of the memory it uses at all, hence I don't see the point in
exposing the hardware iommu to dom0 unless there's some emulation done
to make dom0 able to access it.


Currently, Dom0 on ARM is always 1:1 mapped (gfn == mfn). However, I don't 
really know how long this assumption it is going to be true.


The 1:1 mapped is only correct for Dom0 RAM. Any foreign mapping will not be 
mapped 1:1.


We actually have code in Linux to keep track of the foreign mapping as any DMA 
access should be using the machine physical address (and not Dom0 physical address).


This brings some headache when IOMMU is used in Xen because we have to add a 1:1 
mapping for foreign page so you can still DMA in it. This will be fun trying to 
fix XSA-300 because of that...


Ideally the 1:1 mapping should only be used when necessary. Unfortunately this 
is not trivial to remove. For a first, Linux is assuming the 1:1 mapping so you 
need to teach Linux to not assume that anymore. So we need to know if the kernel 
is able to deal with it when building dom0.


Furthermore, having an IOMMU on a platform sadly doesn't mean all DMA-capable 
devices will be behind it. This is a bit difficult to find out in Xen.


In short, this is quite a mess to resolve :/.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part2 v2 4/6] livepatch-build: detect special section group sizes

[Pawel Wieczorkiewicz]

Hard-coding the special section group sizes is unreliable. Instead,
determine them dynamically by finding the related struct definitions
in the DWARF metadata.

This is a livepatch backport of kpatch upstream commit [1]:
kpatch-build: detect special section group sizes 170449847136a48b19fc

Xen only deals with alt_instr, bug_frame and exception_table_entry
structures, so sizes of these structures are obtained from xen-syms.

This change is needed since with recent Xen the alt_instr structure
has changed size from 12 to 14 bytes.

[1] 
https://github.com/jpoimboe/kpatch/commit/170449847136a48b19fcceb19c1d4d257d386b56

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Martin Mazein 
---
v2:
* Applied suggestions from Ross:
  - moved the livepatch-build hunk after build_full() call and
re-using newly built xen-syms
  - dropped the redundant bug_frames_3_group_size()
---
 create-diff-object.c | 50 --
 livepatch-build  | 22 ++
 2 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index 0df3fea..c6183c3 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -958,12 +958,42 @@ static void kpatch_mark_constant_labels_same(struct 
kpatch_elf *kelf)
}
 }
 
-static int bug_frames_0_group_size(struct kpatch_elf *kelf, int offset) { 
return 8; }
-static int bug_frames_1_group_size(struct kpatch_elf *kelf, int offset) { 
return 8; }
-static int bug_frames_2_group_size(struct kpatch_elf *kelf, int offset) { 
return 8; }
-static int bug_frames_3_group_size(struct kpatch_elf *kelf, int offset) { 
return 16; }
-static int ex_table_group_size(struct kpatch_elf *kelf, int offset) { return 
8; }
-static int altinstructions_group_size(struct kpatch_elf *kelf, int offset) { 
return 12; }
+static int bug_frames_group_size(struct kpatch_elf *kelf, int offset)
+{
+   static int size = 0;
+   char *str;
+   if (!size) {
+   str = getenv("BUG_STRUCT_SIZE");
+   size = str ? atoi(str) : 8;
+   }
+
+   return size;
+}
+
+static int ex_table_group_size(struct kpatch_elf *kelf, int offset)
+{
+   static int size = 0;
+   char *str;
+   if (!size) {
+   str = getenv("EX_STRUCT_SIZE");
+   size = str ? atoi(str) : 8;
+   }
+
+   return size;
+}
+
+static int altinstructions_group_size(struct kpatch_elf *kelf, int offset)
+{
+   static int size = 0;
+   char *str;
+   if (!size) {
+   str = getenv("ALT_STRUCT_SIZE");
+   size = str ? atoi(str) : 12;
+   }
+
+   log_debug("altinstr_size=%d\n", size);
+   return size;
+}
 
 /*
  * The rela groups in the .fixup section vary in size.  The beginning of each
@@ -1016,19 +1046,19 @@ static int fixup_group_size(struct kpatch_elf *kelf, 
int offset)
 static struct special_section special_sections[] = {
{
.name   = ".bug_frames.0",
-   .group_size = bug_frames_0_group_size,
+   .group_size = bug_frames_group_size,
},
{
.name   = ".bug_frames.1",
-   .group_size = bug_frames_1_group_size,
+   .group_size = bug_frames_group_size,
},
{
.name   = ".bug_frames.2",
-   .group_size = bug_frames_2_group_size,
+   .group_size = bug_frames_group_size,
},
{
.name   = ".bug_frames.3",
-   .group_size = bug_frames_3_group_size,
+   .group_size = bug_frames_group_size,
},
{
.name   = ".fixup",
diff --git a/livepatch-build b/livepatch-build
index 3c4bf13..7068faf 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -315,6 +315,28 @@ if [ "${SKIP}" != "build" ]; then
 echo "Perform full initial build with ${CPUS} CPU(s)..."
 build_full
 
+echo "Reading special section data"
+# Using xen-syms built in the previous step by build_full().
+SPECIAL_VARS=$(readelf -wi "$OUTPUT/xen-syms" |
+   gawk --non-decimal-data '
+   BEGIN { a = b = e = 0 }
+   a == 0 && /DW_AT_name.* alt_instr/ {a = 1; next}
+   b == 0 && /DW_AT_name.* bug_frame/ {b = 1; next}
+   e == 0 && /DW_AT_name.* exception_table_entry/ {e = 1; next}
+   a == 1 {printf("export ALT_STRUCT_SIZE=%d\n", $4); a = 2}
+   b == 1 {printf("export BUG_STRUCT_SIZE=%d\n", $4); b = 2}
+   e == 1 {printf("export EX_STRUCT_SIZE=%d\n", $4); e = 2}
+   a == 2 && b == 2 && e == 2 {exit}')
+[[ -n $SPECIAL_VARS ]] && eval "$SPECIAL_VARS"
+if [[ -z $ALT_STRUCT_SIZE ]] || [[ -z $BUG_STRUCT_SIZE ]] || [[ -z 
$EX_STRUCT_SIZE ]]; then
+die "can't find special struct size"
+fi
+for i in $ALT_STRUCT_SIZE $BUG_STRUC

Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Oleksandr]

Hi, Julien.


I am afraid, I don't know a correct answer for this question. I would 
leave this to maintainers.


I just followed sample copyright notice for GPL v2 License according 
to the document:


http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=CONTRIBUTING


The file CONTRIBUTING is only giving example of common example of 
license. So I think this is fine to use SPDX, the more they are 
already used. The only request is to put either SDPX or the full-blown 
text but not the two :). Lars, any objection?


I am quite in favor of SPDX because it is easier to find out the 
license. With the full-blown text, the text may slightly vary between 
licenses. For instance, the only difference between GPLv2 and GPLv2+ 
is ",or (at your option) any later version". I let you imagine how it 
can be easy to miss it when reviewing ;).


We had a discussion last year about using SPDX in Xen code base but I 
never got the time to formally suggest it.


I tried to locate files in Xen where SPDX is used. After finding only 
nospec.h I got an incorrect opinion this is not popular in Xen))



Just to clarify. So the title for the driver should be the following (if 
there are no objections):


// SPDX-License-Identifier: GPL-2.0
/*
 * xen/drivers/passthrough/arm/ipmmu-vmsa.c
 *
 * Driver for the Renesas IPMMU-VMSA found in R-Car Gen3 SoCs.
 *
 * Copyright (C) 2014-2019 Renesas Electronics Corporation
 *
 * The IPMMU-VMSA is VMSA-compatible I/O Memory Management Unit (IOMMU)
 * which provides address translation and access protection functionalities
 * to processing units and interconnect networks.
 *
 * Please note, current driver is supposed to work only with newest 
R-Car Gen3
 * SoCs revisions which IPMMU hardware supports stage 2 translation 
table format

 * and is able to use CPU's P2M table as is.
 *
 * Based on Linux's IPMMU-VMSA driver from Renesas BSP:
 *    drivers/iommu/ipmmu-vmsa.c
 * you can found at:
 *    url: 
git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git

 *    branch: v4.14.75-ltsi/rcar-3.9.6
 *    commit: e206eb5b81a60e64c35fbc3a999b1a0db2b98044
 * and Xen's SMMU driver:
 *    xen/drivers/passthrough/arm/smmu.c
 *
 * Copyright (C) 2016-2019 EPAM Systems Inc.
 */


Answer to myself:

Looks like, the same I have to do with all newly added files in this 
series (iommu_fwspec, etc).




+    /*
+ * Destroy Root IPMMU domain which context is mapped to this 
Xen domain

+ * if exits.
+ */
+    if ( xen_domain->root_domain )
+    ipmmu_free_root_domain(xen_domain->root_domain);
+
+    spin_unlock(&xen_domain->lock);
+
+    /*
+ * We assume that all master devices have already been 
detached from
+ * this Xen domain and there must be no associated Cache IPMMU 
domains

+ * in use.
+ */
+    ASSERT(list_empty(&xen_domain->cache_domains));

I think this should be in the spin lock held by &xen_domain->lock.


OK. Will put spin_unlock after it.


The spin_lock is actually pointless here. This is done when the domain 
is destroyed, so nobody should touch it.


If you think concurrent access can still happen, then you are going to 
be in deep trouble as you free the xen_domain (and therefore the 
spinlock) below :).


Indeed, this is pointless. We don't really expect any other operations 
with the domain which is being destroyed. No assign/deassign devices, no 
flush, no map, nothing...







--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part2 v2 5/6] create-diff-object: Add new entries to special sections array array

[Pawel Wieczorkiewicz]

Handle .livepatch.hooks* and .altinstr_replacement sections as the
special sections with assigned group_size resolution function.
By default each .livepatch.hooks* sections' entry is 8 bytes long (a
pointer). The .altinstr_replacement section has undefined group_size.

Allow to specify different .livepatch.hooks* section entry size using
shell environment variable HOOK_STRUCT_SIZE.

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Andra-Irina Paraschiv 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Norbert Manthey 
---
v2:
* Applied suggestions from Ross and neccessary changes enforced by
  previous patch of the series:
  - fixed indentation
  - used log_debug() instead of printf()
  - added aux. function undefined_group_size() returning 0 for a
undefined group_size
  - added .altinstr_replacement to the special_sections array and
fixed its group_size to undefined (0).
---
 create-diff-object.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index c6183c3..8365af0 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -995,6 +995,24 @@ static int altinstructions_group_size(struct kpatch_elf 
*kelf, int offset)
return size;
 }
 
+static int livepatch_hooks_group_size(struct kpatch_elf *kelf, int offset)
+{
+   static int size = 0;
+   char *str;
+   if (!size) {
+   str = getenv("HOOK_STRUCT_SIZE");
+   size = str ? atoi(str) : 8;
+   }
+
+   log_debug("livepatch_hooks_size=%d\n", size);
+   return size;
+}
+
+static int undefined_group_size(struct kpatch_elf *kelf, int offset)
+{
+   return 0;
+}
+
 /*
  * The rela groups in the .fixup section vary in size.  The beginning of each
  * .fixup rela group is referenced by the .ex_table section. To find the size
@@ -1072,6 +1090,18 @@ static struct special_section special_sections[] = {
.name   = ".altinstructions",
.group_size = altinstructions_group_size,
},
+   {
+   .name   = ".altinstr_replacement",
+   .group_size = undefined_group_size,
+   },
+   {
+   .name   = ".livepatch.hooks.load",
+   .group_size = livepatch_hooks_group_size,
+   },
+   {
+   .name   = ".livepatch.hooks.unload",
+   .group_size = livepatch_hooks_group_size,
+   },
{},
 };
 
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part2 v2 6/6] create-diff-object: Do not include all .rodata sections

[Pawel Wieczorkiewicz]

Older versions of GCC did not split .rodata.str sections by function.
Because of that, the entire section was always included.
The livepatch-build-tools commit [1] fixed patch creation and kept
including all .rodata.str sections, in order to maintain existing
behavior for GCC 6.1+.
This means all .rodata.str sections are always included by default,
regardless of whether they are needed or not.

During stacked hotpatch builds it leads to unnecessary accumulation of
the .rodata.str sections as each and every consecutive hotpatch module
contains all the .rodata.str sections of previous modules.

To prevent this situation, mark the .rodata.str sections for inclusion
only if they are referenced by any of the current hotpatch symbols (or
a corresponding RELA section).

Extend patchability verification to detect all non-standard, non-rela,
non-debug and non-special sections that are not referenced by any of
the symbols or RELA sections.

[1] 2af6f1aa6233 Fix patch creation with GCC 6.1+

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Andra-Irina Paraschiv 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Norbert Manthey 
---
v2:
* Made the commit message more precise and accurate (based on
  Ross' comments to the v1 patch)
* Kept lines limited to 80 chars
---
 create-diff-object.c | 30 --
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index 8365af0..4252175 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1350,8 +1350,7 @@ static void kpatch_include_standard_elements(struct 
kpatch_elf *kelf)
 
list_for_each_entry(sec, &kelf->sections, list) {
/* include these sections even if they haven't changed */
-   if (is_standard_section(sec) ||
-   should_include_str_section(sec->name)) {
+   if (is_standard_section(sec)) {
sec->include = 1;
if (sec->secsym)
sec->secsym->include = 1;
@@ -1362,6 +1361,20 @@ static void kpatch_include_standard_elements(struct 
kpatch_elf *kelf)
list_entry(kelf->symbols.next, struct symbol, list)->include = 1;
 }
 
+static void kpatch_include_standard_string_elements(struct kpatch_elf *kelf)
+{
+   struct section *sec;
+
+   list_for_each_entry(sec, &kelf->sections, list) {
+   if (should_include_str_section(sec->name) &&
+   is_referenced_section(sec, kelf)) {
+   sec->include = 1;
+   if (sec->secsym)
+   sec->secsym->include = 1;
+   }
+   }
+}
+
 #define inc_printf(fmt, ...) \
log_debug("%*s" fmt, recurselevel, "", ##__VA_ARGS__);
 
@@ -1541,6 +1554,17 @@ static void kpatch_verify_patchability(struct kpatch_elf 
*kelf)
errs++;
}
 
+   if (sec->include) {
+   if (!is_standard_section(sec) && !is_rela_section(sec) 
&&
+   !is_debug_section(sec) && !is_special_section(sec)) 
{
+   if (!is_referenced_section(sec, kelf)) {
+   log_normal("section %s included, but 
not referenced\n",
+  sec->name);
+   errs++;
+   }
+   }
+   }
+
/*
 * ensure we aren't including .data.* or .bss.*
 * (.data.unlikely is ok b/c it only has __warned vars)
@@ -2072,6 +2096,8 @@ int main(int argc, char *argv[])
kpatch_include_debug_sections(kelf_patched);
log_debug("Include hook elements\n");
kpatch_include_hook_elements(kelf_patched);
+   log_debug("Include standard string elements\n");
+   kpatch_include_standard_string_elements(kelf_patched);
log_debug("Include new globals\n");
new_globals_exist = kpatch_include_new_globals(kelf_patched);
log_debug("new_globals_exist = %d\n", new_globals_exist);
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] EFI: add efi=mapbs option and parse efi= early

[Jan Beulich]

On 08.08.2019 11:40, Andrew Cooper wrote:
> On 08/08/2019 01:31, Marek Marczykowski-Górecki wrote:
>> When booting Xen via xen.efi, there is /mapbs option to workaround
>> certain platform issues (added in f36886bdf4 "EFI/early: add /mapbs to
>> map EfiBootServices{Code,Data}"). Add support for efi=mapbs on Xen
>> cmdline for the same effect and parse it very early in the
>> multiboot2+EFI boot path.
>>
>> Normally cmdline is parsed after relocating MB2 structure, which happens
>> too late. To have efi= parsed early enough, save cmdline pointer in
>> head.S and pass it as yet another argument to efi_multiboot2(). This
>> way we avoid introducing yet another MB2 structure parser.
>>
>> To keep consistency, handle efi= parameter early in xen.efi too, both in
>> xen.efi command line and cfg file.
>>
>> Signed-off-by: Marek Marczykowski-Górecki 
> 
> I'm very sorry to do this to you, but I'm going to object to the patch
> (in principle.  I think the command line option itself is fine but...)
> 
> What does Linux do differently here?
> 
> It is actively damaging to the Xen community to users to force users
> tweak command lines in order to boot/recover their system, and it looks
> especially embarrassing when other OSes cope automatically.  We have
> compatibility for all kinds of other firmware screw-ups, except EFI it
> seems, and this needs to change.
> 
> So while I have no objection to the option per say, I don't think this
> patch is reasonable as a "fix" to the problem as far as end users are
> concerned.

And your suggestion then is to always map (and not use) all boot
services memory, contrary to all intentions of the EFI spec?

As to your more general rant: I think it is wrong to have more
than very simple and low overhead workarounds enabled "just in
case". The vast majority of the workarounds we have in place
are keyed to specific versions of something, or e.g. DMI
properties of systems. I certainly wouldn't mind DMI based
enabling of workarounds like the one here, but I'm going to
continue to push back on attempts to cripple the EFI code just
because _some_ systems don't have a sensible EFI implementation.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Julien Grall]

Hi,

Removing Lars there is no need to spam him with technical discussion :)

On 08/08/2019 11:14, Oleksandr wrote:




Hi,


Hi, Julien.





Sorry for the possible format issues.


 > No, it is not disabled. But, ipmmu_irq() uses another mmu->lock. So, I
 > think, there won't be a deadlock.
 >
 > Or I really missed something?
 >
 > If we worry about ipmmu_tlb_invalidate() which is called here (to
 > perform a flush by request from P2M code, which manages a page table)
 > and from the irq handler (to perform a flush to resume address
 > translation), I could use a tasklet to schedule ipmmu_tlb_invalidate()
 > from the irq handler then. This way we would get this serialized. What
 > do you think?

    I am afraid a tasklet is not an option. You need to perform the TLB
    flush when requested otherwise you are introducing a security issue.

    This is because as soon as a region is unmapped in the page table, we
    remove the drop the reference on any page backing that region. When the
    reference is dropped to zero, the page can be reallocated to another
    domain or even Xen. If the TLB flush happen after, then the guest may
    still be able to access the page for a short time if the translation has
    been cached by the any TLB (IOMMU, Processor).



I understand this. I am not proposing to delay a requested by P2M code TLB 
flush in any case. I just propose to issue TLB flush (which we have to 
perform in case of page faults, to resolve error condition and resume 
translations) from a tasklet rather than from interrupt handler directly. 
This is the TLB flush I am speaking about:


https://github.com/otyshchenko1/xen/blob/ipmmu_upstream2/xen/drivers/passthrough/arm/ipmmu-vmsa.c#L598 



Sorry if I was unclear.


My mistake, I misread what you wrote.

I found the flush in the renesas-bsp and not Linux upstream but it is not 
clear why this is actually required. You are not fixing any translation error. 
So what this flush will do?


Regarding the placement of the flush, then if you execute in a tasklet it will 
likely be done later on when the IRQ has been acknowledge. What's the 
implication to delay it?



Looks like, there is no need to put this flush into a tasklet. As I understand 
from Shimoda-san's answer it is OK to call flush here.


So, my worry about calling ipmmu_tlb_invalidate() directly from the interrupt 
handler is not actual anymore.

--
This is my understanding regarding the flush purpose here. This code, just 
follows the TRM, no more no less,
which mentions about a need to flush TLB after clearing error status register 
and updating a page table entries (which, I assume, means to resolve a reason 
why translation/page fault error actually have happened) to resume address 
translation request.


Well, I don't have the TRM... so my point of reference is Linux. Why does 
upstream not do the TLB flush?


I have been told this is an errata on the IPMMU. Is it related to the series 
posted on linux-iommu [1]?




But, with one remark, as you have already noted, we are not trying to handle/fix 
this fault (update page table entries), driver doesn't manage page table and is 
not aware what the page table is. What is more, it is unclear what actually need 
to be fixed in the page table which is a CPU page table as the same time.


I have heard there is a break-before-make sequence when updating the page table. 
So, if device in a domain is issuing DMA somewhere in the middle of updating a 
page table, theoretically, we might hit into this fault. In this case the page 
table is correct and we don't need to fix anything...   Being honest, I have 
never seen a fault caused by break-before-make sequence.


Ok, so it looks like you are trying to fix [1]. My first concern here is there 
are no ground for someone without access to the TRM why this is done.


Furthermore, AFAICT, the patch series never reached upstream. So is it present 
on all revision of GEN3?


Cheers,

[1] 
https://lore.kernel.org/linux-iommu/1485348842-23712-1-git-send-email-yoshihiro.shimoda...@renesas.com/


--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part3 v2 2/3] create-diff-object: Extend patchability verification: STN_UNDEF

[Pawel Wieczorkiewicz]

During verification check if all sections do not contain any entries
with undefined symbols (STN_UNDEF). This situation can happen when a
section is copied over from its original object to a patched object,
but various symbols related to the section are not copied along.
This scenario happens typically during stacked hotpatches creation
(between 2 different hotpatch modules).

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Martin Pohlack 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Norbert Manthey 
Reviewed-by: Andra-Irina Paraschiv 
---
v2:
* Refactored code by creating a new function kpatch_section_has_undef_symbols()
  for the complicated multi-loop code of kpatch_verify_patchability().
  Hopefully this makes code more readable and easier to maintain.
* Kept lines limits to 80 chars (whereever possible)
* Detection of an undefined symbol counts as a single error
---
 create-diff-object.c | 66 
 1 file changed, 66 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index 41adb09..e905131 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1531,6 +1531,61 @@ static void kpatch_print_changes(struct kpatch_elf *kelf)
}
 }
 
+static inline int get_section_entry_size(const struct section *sec,
+struct kpatch_elf *kelf)
+{
+   int entry_size;
+
+   /*
+* Base sections typically do not define fixed size elements.
+* Detect section's element size in case it's a special section.
+* Otherwise, skip it due to an unknown sh_entsize.
+*/
+   entry_size = sec->sh.sh_entsize;
+   if (entry_size == 0) {
+   struct special_section *special;
+
+   /* Find special section group_size. */
+   for (special = special_sections; special->name; special++) {
+   if (!strcmp(sec->name, special->name))
+   return special->group_size(kelf, 0);
+   }
+}
+
+   return entry_size;
+}
+
+static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
+   const struct section *sec)
+{
+   int offset, entry_size;
+   struct rela *rela;
+   size_t d_size;
+
+   entry_size = get_section_entry_size(sec, kelf);
+   if (entry_size == 0)
+   return false;
+
+   d_size = sec->base->data->d_size;
+   for ( offset = 0; offset < d_size; offset += entry_size ) {
+   list_for_each_entry(rela, &sec->relas, list) {
+   if (rela->offset < offset ||
+   rela->offset >= offset + entry_size) {
+   continue;
+   }
+
+   if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+   (!rela->sym->include && rela->sym->status == SAME)) 
{
+   log_normal("section %s has an entry with a 
STN_UNDEF symbol: %s\n",
+  sec->name, rela->sym->name ?: 
"none");
+   return true;
+   }
+   }
+   }
+
+   return false;
+}
+
 static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 {
struct section *sec;
@@ -1563,6 +1618,17 @@ static void kpatch_verify_patchability(struct kpatch_elf 
*kelf)
errs++;
}
}
+
+   /* Check if a RELA section does not contain any entries 
with
+* undefined symbols (STN_UNDEF). This situation can 
happen
+* when a section is copied over from its original 
object to
+* a patched object, but various symbols related to the 
section
+* are not copied along.
+*/
+   if (is_rela_section(sec)) {
+   if (kpatch_section_has_undef_symbols(kelf, sec))
+   errs++;
+   }
}
 
/*
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] xen/arm: assign devices to boot domains

[Julien Grall]

Hi Stefano,

On 07/08/2019 23:46, Stefano Stabellini wrote:

On Tue, 15 Jan 2019, Julien Grall wrote:

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index cc6b464..d48f77e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2094,6 +2094,88 @@ static int __init construct_domain(struct domain
*d,
struct kernel_info *kinfo)
return 0;
}
+static int __init scan_pt_node(const void *pfdt,
+   int nodeoff, const char *name, int
depth,
+   u32 address_cells, u32 size_cells,
+   void *data)


Is it really necessary to parse the device-tree twice?


I don't think I understand this comment. The device tree fragment is not
scanned twice, just once I think. Am I missing something?


The previous patch is going through the DT to copy the properties over. This
patch introduce a new function to go over the DT to find the Device to attach.
So you are parsing the DT twice. Is there any reason for that?


I got your question now.  I spent quite some time to merge the two
paths, the first one used to copy the fragment, the second one used to
parse it, into a single function. It is difficult because the
information convenient to use for one case, it is not convenient for the
other (specifically figuring out when to call fdt_end_node when called
from device_tree_for_each_node.) I managed to do it though, in the next
version there will be only one parsing of the fragment.


I wasn't necessarily asking to merge the two. I wanted to understand whether we 
can avoid two parsing yet keeping the code simple. Sorry if it wasn't clear enough.


[...]


Furthermore, this is assuming all the nodes in the fragment will be
under the GIC controller.  You may want to passthrough a interrupt
controller (i.e GPIO) to the guest and the related device.


I don't think that the non-GIC scenario is supported today. I don't
think it works even without dom0less. I wrote more about this as a reply
to the other email.

I believe this works out-of-box with the guest. If we take the example of the
GPIO controller, it may be behind the GIC. You only care to route those
interrupts used by GPIO and MMIO associated. The rest can be describe normally
in the DT.

Of course, this means that you need to passthrough all devices using the GPIO
controller to that guest.

So I still think you need to check whether the interrupts belongs the GIC or
not.


I think I understand what you meant now. I could add a check before
routing any interrupts to the guest, to make sure that they are GIC
interrupts. However, the way to do that check I believe would be using
the "interrupt-parent" property, but we have just discussed about
removing it.


I have suggested a way to keep it and avoid writing down the phandle value.



So, if the user provides a fragment that doesn't have any
"interrupt-parent" properties, how can I check that the interrupts are
GIC interrupts?


The problem here is you are re-using the guest "interrupts" property for finding 
out the host interrupts. Firstly, this does not cater the case where guest DT is 
using the property "interrupt-extended".


It feels to me that we should look at the "interrupts" property from the host DT 
and map them 1:1 (i.e irq == virq). The property in the partial DT would just be 
replicating the values from the host DT.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/4] xen/spinlocks: in debug builds store cpu holding the lock

[Juergen Gross]

On 08.08.19 14:18, Jan Beulich wrote:

On 08.08.2019 13:53, Juergen Gross wrote:

On 08.08.19 12:28, Julien Grall wrote:

On 08/08/2019 08:51, Juergen Gross wrote:

On 08.08.19 08:58, Jan Beulich wrote:

On 07.08.2019 16:31, Juergen Gross wrote:
Do we have an implied assumption somewhere that unsigned short is
exactly 16 bits wide? I think "val" wants to be uint16_t here (as
you really mean "exactly 16 bits"), the two boolean fields want
to be bool, and the remaining two ones unsigned int.


But that would increase the size of the union to 4 bytes instead of 2.
So at least pad and cpu must be unsigned short or (better) uint16_t.


How about bool irq_safe:1?


I didn't question that, but OTOH I'm not sure doing something like:

struct {
    bool unseen:1;
    bool irq_safe:1;
    uint16_t pad:2;
    uint16_t cpu:12;
}

is guaranteed to be 2 bytes long. I think pad will be still put into the
first byte, but the compiler might decide that the 4 bits left won't be
able to hold the next 12 bits so it could start a new uint16_t at offset
2.

Moving the bool types to the end of the struct would avoid that IMHO.


Moving the two bool-s further down will also simplify extraction and
insertion of the "cpu" field.


Okay, lets reverse above struct.


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [livepatch-build-tools part3 v2 3/3] create-diff-object: Strip all undefined entires of known size

[Pawel Wieczorkiewicz]

The patched ELF object file contains all sections and symbols as
resulted from the compilation. However, certain symbols may not be
copied over to the resulting object file, due to being unchanged or
not included for other reasons.
In such situation the resulting object file has the entire sections
copied along (with all their entries unchanged), while some of the
corresponding symbols are not copied along at all.
This leads to having incorrect undefined (STN_UNDEF) entries in the
final hotpatch ELF file.

The newly added function livepatch_strip_undefined_elements() detects
and removes all undefined RELA entries as well as their corresponding
PROGBITS section entries.
Since the sections may contain elements of unknown size (sh.sh_entsize
== 0), perform the strip only on sections with well defined entry
sizes.

After replacing the stripped rela list, it is assumed that the next
invocation of the kpatch_rebuild_rela_section_data() will adjust all
section header parameters according to the current state.

Signed-off-by: Pawel Wieczorkiewicz 
Reviewed-by: Martin Pohlack 
Reviewed-by: Bjoern Doebel 
Reviewed-by: Norbert Manthey 
Reviewed-by: Andra-Irina Paraschiv 
---
v2:
* Kept lines limits to 80 chars (whereever possible)
* Fixed commit message
---
 create-diff-object.c | 131 ++-
 1 file changed, 129 insertions(+), 2 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index e905131..f352704 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1555,6 +1555,13 @@ static inline int get_section_entry_size(const struct 
section *sec,
return entry_size;
 }
 
+/* Check if RELA entry has undefined or unchanged/not-included symbols. */
+static inline bool has_rela_undefined_symbol(const struct rela *rela)
+{
+   return (GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+  (!rela->sym->include && (rela->sym->status == SAME));
+}
+
 static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
const struct section *sec)
 {
@@ -1574,8 +1581,7 @@ static int kpatch_section_has_undef_symbols(struct 
kpatch_elf *kelf,
continue;
}
 
-   if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
-   (!rela->sym->include && rela->sym->status == SAME)) 
{
+   if (has_rela_undefined_symbol(rela)) {
log_normal("section %s has an entry with a 
STN_UNDEF symbol: %s\n",
   sec->name, rela->sym->name ?: 
"none");
return true;
@@ -1989,6 +1995,125 @@ static void livepatch_create_patches_sections(struct 
kpatch_elf *kelf,
 
 }
 
+/*
+ * The patched ELF object file contains all sections and symbols as resulted
+ * from the compilation. However, certain symbols may not be copied over to
+ * the resulting object file, due to being unchanged or not included for other
+ * reasons.
+ * In such situation the resulting object file has the entire sections copied
+ * along (with all their entries unchanged), while some of the corresponding
+ * symbols are not copied along at all.
+ * This leads to having incorrect dummy (STN_UNDEF) entries in the final
+ * hotpatch ELF file.
+ * This functions removes all undefined entries of known size from both
+ * RELA and PROGBITS sections of the patched elf object.
+ */
+static void livepatch_strip_undefined_elements(struct kpatch_elf *kelf)
+{
+   struct section *sec;
+
+   list_for_each_entry(sec, &kelf->sections, list) {
+   struct rela *rela, *safe;
+   int src_offset = 0, dst_offset = 0;
+   int entry_size, align, aligned_size;
+   char *src, *dst;
+   LIST_HEAD(newrelas);
+
+   /* use RELA section to find all its undefined entries */
+   if (!is_rela_section(sec))
+   continue;
+
+   /* only known, fixed-size entries can be stripped */
+   entry_size = get_section_entry_size(sec->base, kelf);
+   if (entry_size == 0)
+   continue;
+
+   /* alloc buffer for new base section */
+   dst = malloc(sec->base->sh.sh_size);
+   if (!dst)
+   ERROR("malloc");
+
+   /*
+* Iterate through all entries of a corresponding base section
+* for this RELA section.
+*/
+   for ( src = sec->base->data->d_buf;
+ src_offset < sec->base->sh.sh_size;
+ src_offset += entry_size ) {
+   bool found_valid = false;
+
+   list_for_each_entry_safe(rela, safe, &sec->relas, list) 
{
+   /*
+* Check all RELA elements looking for
+

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Oleksandr]

Hi, Julien, Roger.







On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.


I don't think your code is doing what you expect... If iommu=0, then 
Xen will not lookup for IOMMUs (iommu_hardware_setup() will not be 
called). So none of the device will have DEVICE_IOMMU set and hence 
they are already given to dom0.


But I think it is wrong to give the IOMMUs to Dom0 when iommu=0. This 
is not the goal of this option. If you want to passthrough the IOMMU 
to Dom0, then you should use the parameter iommu_hwdom_passthrough.


However, I agree with Roger that giving the IOMMU to dom0 is a pretty 
bad idea. So this should be fixed.



I fully agree with the arguments provided that it is a bad idea. So, 
please consider that patch as not relevant.



But, I am not sure I follow the last sentence:

>>> If iommu=0, then Xen will not lookup for IOMMUs 
(iommu_hardware_setup() will not be called). So none of the device will 
have DEVICE_IOMMU set and hence they are already given to dom0.


I can see that devices have DEVICE_IOMMU set. Although, the IOMMU driver 
is not in use, it is present and compatible matches. So, even if 
iommu=0, the IOMMU devices are not given to Dom0, because of skipped. Or 
I missed something?



--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Julien Grall]

On 08/08/2019 14:03, Oleksandr wrote:


Hi, Julien, Roger.


Hi,









On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.


I don't think your code is doing what you expect... If iommu=0, then Xen will 
not lookup for IOMMUs (iommu_hardware_setup() will not be called). So none of 
the device will have DEVICE_IOMMU set and hence they are already given to dom0.


But I think it is wrong to give the IOMMUs to Dom0 when iommu=0. This is not 
the goal of this option. If you want to passthrough the IOMMU to Dom0, then 
you should use the parameter iommu_hwdom_passthrough.


However, I agree with Roger that giving the IOMMU to dom0 is a pretty bad 
idea. So this should be fixed.



I fully agree with the arguments provided that it is a bad idea. So, please 
consider that patch as not relevant.



But, I am not sure I follow the last sentence:

 >>> If iommu=0, then Xen will not lookup for IOMMUs (iommu_hardware_setup() 
will not be called). So none of the device will have DEVICE_IOMMU set and hence 
they are already given to dom0.


I can see that devices have DEVICE_IOMMU set. Although, the IOMMU driver is not 
in use, it is present and compatible matches. So, even if iommu=0, the IOMMU 
devices are not given to Dom0, because of skipped. Or I missed something?


I can't see how iommu_hardware_setup() can be called on staging when iommu=0 as 
this is protected by a if ( iommu_enable ).


Can you please give a stack trace how this is called and the version you use? 
WARN() should do it for you.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2] evtchn: make support for different ABIs tunable

[Jan Beulich]

On 07.08.2019 19:42, Eslam Elnikety wrote:
> Support for FIFO event channel ABI was first introduced in Xen 4.4
> (see 88910061ec6). Make this support tunable, since the choice of which
> event channel ABI has implications for hibernation. Consider resuming a
> pre Xen 4.4 hibernated Linux guest. During resume from hibernation, there
> are two kernels involved: the "boot" kernel and the "resume" kernel. The
> guest boot kernel defaults to FIFO ABI and instructs Xen via an
> EVTCHNOP_init_control to switch from 2L to FIFO.

This should only be "may default to" - there's nothing making this
to be the case unconditionally afaict. Another option would be to
start the sentence "If the guest boot kernel defaults ...".

> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1170,6 +1170,11 @@ long do_event_channel_op(int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>   
>   case EVTCHNOP_init_control: {
>   struct evtchn_init_control init_control;
> +
> +/* Fail init_control for domains that must use 2l ABI */
> +if ( current->domain->options & XEN_DOMCTL_CDF_disable_fifo )
> +return -EPERM;
> +
>   if ( copy_from_guest(&init_control, arg, 1) != 0 )
>   return -EFAULT;
>   rc = evtchn_fifo_init_control(&init_control);

I think the check would better go into evtchn_fifo_init_control(),
at least as long as the setting really is FIFO-centric. Also the
comment will become stale the moment a 3rd evtchn mechanism appears
- it shouldn't mention 2L as the setting isn't "2-level only". Then
again you may actually want it to behave like this, and hence be
named accordingly.

Irrespective of these remarks, and along the lines of comments on
the v1 thread, introducing wider control that would also allow
disabling 2-level (for HVM guests) would seem better to me. This
would then hopefully be coded up in a way that would make extending
it straightforward, once a 3rd mechanism appears.

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 29/35] OvmfPkg/OvmfXen: Override PcdFSBClock to Xen vLAPIC timer frequency

[Anthony PERARD]

On Wed, Aug 07, 2019 at 05:54:51PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:38PM +0100, Anthony PERARD wrote:
> > PcdFSBClock is used by SecPeiDxeTimerLibCpu, the TimerLib
> > implementation. It will also be used by XenTimerDxe. Override
> > PcdFSBClock to match Xen vLAPIC timer frequency.
> 
> I'm kind of surprised that his is not automatically detected?
> 
> Is it a bug in Xen or just always hardcoded on OVMF?

It's hardcoded. Why would you need auto detection when you always run on
the same machine ;-).

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/3] xen/blkback: Use refcount_t for refcount

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 09:11:00PM +0800, Chuhong Yuan wrote:
> Reference counters are preferred to use refcount_t instead of
> atomic_t.
> This is because the implementation of refcount_t can prevent
> overflows and detect possible use-after-free.
> So convert atomic_t ref counters to refcount_t.

Thanks!

I think there are more reference counters in blkback than
the one you fixed. There's also an inflight field in xen_blkif_ring,
and a pendcnt in pending_req which look like possible candidates to
switch to use refcount_t, have you looked into switching those two
also?

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Oleksandr]

Hi










On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.


I don't think your code is doing what you expect... If iommu=0, then 
Xen will not lookup for IOMMUs (iommu_hardware_setup() will not be 
called). So none of the device will have DEVICE_IOMMU set and hence 
they are already given to dom0.


But I think it is wrong to give the IOMMUs to Dom0 when iommu=0. 
This is not the goal of this option. If you want to passthrough the 
IOMMU to Dom0, then you should use the parameter 
iommu_hwdom_passthrough.


However, I agree with Roger that giving the IOMMU to dom0 is a 
pretty bad idea. So this should be fixed.



I fully agree with the arguments provided that it is a bad idea. So, 
please consider that patch as not relevant.



But, I am not sure I follow the last sentence:

 >>> If iommu=0, then Xen will not lookup for IOMMUs 
(iommu_hardware_setup() will not be called). So none of the device 
will have DEVICE_IOMMU set and hence they are already given to dom0.


I can see that devices have DEVICE_IOMMU set. Although, the IOMMU 
driver is not in use, it is present and compatible matches. So, even 
if iommu=0, the IOMMU devices are not given to Dom0, because of 
skipped. Or I missed something?


I can't see how iommu_hardware_setup() can be called on staging when 
iommu=0 as this is protected by a if ( iommu_enable ).


Can you please give a stack trace how this is called and the version 
you use? WARN() should do it for you.



iommu_hardware_setup() is not called. But, devices have DEVICE_IOMMU 
set, even if "iommu=0". I am based on "7d1460c xen/arm: optee: fix 
compilation with GCC 4.8" + Stefano's reserved-memory series + my IPMMU 
series.



--
Regards,

Oleksandr Tyshchenko


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 29/35] OvmfPkg/OvmfXen: Override PcdFSBClock to Xen vLAPIC timer frequency

[Roger Pau Monné]

On Thu, Aug 08, 2019 at 02:28:32PM +0100, Anthony PERARD wrote:
> On Wed, Aug 07, 2019 at 05:54:51PM +0200, Roger Pau Monné wrote:
> > On Mon, Jul 29, 2019 at 04:39:38PM +0100, Anthony PERARD wrote:
> > > PcdFSBClock is used by SecPeiDxeTimerLibCpu, the TimerLib
> > > implementation. It will also be used by XenTimerDxe. Override
> > > PcdFSBClock to match Xen vLAPIC timer frequency.
> > 
> > I'm kind of surprised that his is not automatically detected?
> > 
> > Is it a bug in Xen or just always hardcoded on OVMF?
> 
> It's hardcoded. Why would you need auto detection when you always run on
> the same machine ;-).

I don't think that's part of the HVM/PVH ABI in any way, and if you
hardcode it in OVMF it would prevent Xen from changing the frequency
in the future if such necessity arises. We should try to avoid painting
ourselves into a corner when possible.

Doesn't OVMF have a way to get this from the hardware itself?

Thanks, Roger.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] xen/arm: Let the IOMMU be accessible by Dom0 if forcibly disabled in Xen

[Julien Grall]

Hi,

On 08/08/2019 14:36, Oleksandr wrote:


Hi










On Thu, Aug 08, 2019 at 01:53:23PM +0300, Oleksandr Tyshchenko wrote:

From: Oleksandr Tyshchenko 

Don't skip IOMMU nodes when creating DT for Dom0 if IOMMU has been
forcibly disabled in bootargs (e.g. "iommu=0") in order to let
the IOMMU be accessible by DOM0.


I don't think your code is doing what you expect... If iommu=0, then Xen 
will not lookup for IOMMUs (iommu_hardware_setup() will not be called). So 
none of the device will have DEVICE_IOMMU set and hence they are already 
given to dom0.


But I think it is wrong to give the IOMMUs to Dom0 when iommu=0. This is not 
the goal of this option. If you want to passthrough the IOMMU to Dom0, then 
you should use the parameter iommu_hwdom_passthrough.


However, I agree with Roger that giving the IOMMU to dom0 is a pretty bad 
idea. So this should be fixed.



I fully agree with the arguments provided that it is a bad idea. So, please 
consider that patch as not relevant.



But, I am not sure I follow the last sentence:

 >>> If iommu=0, then Xen will not lookup for IOMMUs (iommu_hardware_setup() 
will not be called). So none of the device will have DEVICE_IOMMU set and 
hence they are already given to dom0.


I can see that devices have DEVICE_IOMMU set. Although, the IOMMU driver is 
not in use, it is present and compatible matches. So, even if iommu=0, the 
IOMMU devices are not given to Dom0, because of skipped. Or I missed something?


I can't see how iommu_hardware_setup() can be called on staging when iommu=0 
as this is protected by a if ( iommu_enable ).


Can you please give a stack trace how this is called and the version you use? 
WARN() should do it for you.



iommu_hardware_setup() is not called. But, devices have DEVICE_IOMMU set, even 
if "iommu=0". I am based on "7d1460c xen/arm: optee: fix compilation with GCC 
4.8" + Stefano's reserved-memory series + my IPMMU series.


Hmmm, what you mean by set is "device_get_class() return DEVICE_IOMMU". This is 
were I got confused and I mixed up with dt_device_set_{protected, used_by}() 
function.


device_get_class() will just lookup for a match and return the class associated. 
So you are right and the node will be skipped in that case. Nothing to modify in 
the current code.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 3/3] xen/blkback: Use refcount_t for refcount

[Chuhong Yuan]

Reference counters are preferred to use refcount_t instead of
atomic_t.
This is because the implementation of refcount_t can prevent
overflows and detect possible use-after-free.
So convert atomic_t ref counters to refcount_t.

Signed-off-by: Chuhong Yuan 
---
 drivers/block/xen-blkback/common.h | 7 ---
 drivers/block/xen-blkback/xenbus.c | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/block/xen-blkback/common.h 
b/drivers/block/xen-blkback/common.h
index 1d3002d773f7..9db5f3586fb4 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -309,7 +310,7 @@ struct xen_blkif {
struct xen_vbd  vbd;
/* Back pointer to the backend_info. */
struct backend_info *be;
-   atomic_trefcnt;
+   refcount_t  refcnt;
/* for barrier (drain) requests */
struct completion   drain_complete;
atomic_tdrain;
@@ -362,10 +363,10 @@ struct pending_req {
 (_v)->bdev->bd_part->nr_sects : \
  get_capacity((_v)->bdev->bd_disk))
 
-#define xen_blkif_get(_b) (atomic_inc(&(_b)->refcnt))
+#define xen_blkif_get(_b) (refcount_inc(&(_b)->refcnt))
 #define xen_blkif_put(_b)  \
do {\
-   if (atomic_dec_and_test(&(_b)->refcnt)) \
+   if (refcount_dec_and_test(&(_b)->refcnt))   \
schedule_work(&(_b)->free_work);\
} while (0)
 
diff --git a/drivers/block/xen-blkback/xenbus.c 
b/drivers/block/xen-blkback/xenbus.c
index 3ac6a5d18071..ecc5f9c5bf3f 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -169,7 +169,7 @@ static struct xen_blkif *xen_blkif_alloc(domid_t domid)
return ERR_PTR(-ENOMEM);
 
blkif->domid = domid;
-   atomic_set(&blkif->refcnt, 1);
+   refcount_set(&blkif->refcnt, 1);
init_completion(&blkif->drain_complete);
INIT_WORK(&blkif->free_work, xen_blkif_deferred_free);
 
-- 
2.20.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] xen/console: Rework HYPERCALL_console_io interface

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> @@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, 
> XEN_GUEST_HANDLE_PARAM(char) buffer)
>   rc = guest_console_write(buffer, count);
>   break;
>   case CONSOLEIO_read:
> +/*
> + * The return value is either the number of characters read or
> + * a negative value in case of error. So we need to prevent
> + * overlap between the two sets.
> + */
> +rc = -E2BIG;
> +if ( (int)count < 0 )
> +break;

A more portable (afaict) approach would be to check against INT_MAX.
Either way
Reviewed-by: Jan Beulich 

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] xen/console: Don't treat NUL character as the end of the buffer

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> @@ -1261,14 +1259,15 @@ void debugtrace_printk(const char *fmt, ...)
>  ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>   
>  va_start(args, fmt);
> -vsnprintf(buf, sizeof(buf), fmt, args);
> +nr = vscnprintf(buf, sizeof(buf), fmt, args);
>  va_end(args);
>  
>  if ( debugtrace_send_to_console )
>  {
> -snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> -serial_puts(sercon_handle, cntbuf);
> -serial_puts(sercon_handle, buf);
> +unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);

While - given the size of cntbuf - the difference is mostly
benign, you using vscnprintf() above calls for you also
using scnprintf() here.

> --- a/xen/include/xen/video.h
> +++ b/xen/include/xen/video.h
> @@ -13,11 +13,11 @@
>  
>  #ifdef CONFIG_VIDEO
>  void video_init(void);
> -extern void (*video_puts)(const char *);
> +extern void (*video_puts)(const char *, size_t nr);
>  void video_endboot(void);
>  #else
>  #define video_init()((void)0)
> -#define video_puts(s)   ((void)0)
> +#define video_puts(s, nr)   ((void)0)

While I don't think there's overly much risk of "s" getting an
argument with side effects passed, I think that for "nr" the
risk is there. May I ask that you evaluate both here, just in
case?

Preferably with these adjustments
Reviewed-by: Jan Beulich 

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 2/4] xen/console: Rework HYPERCALL_console_io interface

[Julien Grall]

On 08/08/2019 14:57, Jan Beulich wrote:

On 05.08.2019 15:29, Julien Grall wrote:

@@ -627,6 +629,15 @@ long do_console_io(int cmd, int count, 
XEN_GUEST_HANDLE_PARAM(char) buffer)
   rc = guest_console_write(buffer, count);
   break;
   case CONSOLEIO_read:
+/*
+ * The return value is either the number of characters read or
+ * a negative value in case of error. So we need to prevent
+ * overlap between the two sets.
+ */
+rc = -E2BIG;
+if ( (int)count < 0 )
+break;


A more portable (afaict) approach would be to check against INT_MAX.


It would be better than the cast. I will update it.


Either way
Reviewed-by: Jan Beulich 


Thank you!

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [RFC 1/6] xen/arm: Re-enable interrupt later in the trap path

[Andrii Anisov]

On 06.08.19 16:09, Andrii Anisov wrote:

p.p.s. I'm looking through freertos as well to get wider look on the available 
approaches


OK, basically Free-RTOS does not account the IRQ time separately. Yet its 
scheduling is very implementation dependent.
Any ideas about other open-source examples available for investigation?

--
Sincerely,
Andrii Anisov.

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 01/26] configure: Define TARGET_ALIGNED_ONLY

[Cornelia Huck]

On Wed, 7 Aug 2019 08:25:37 +
 wrote:

> Rename ALIGNED_ONLY to TARGET_ALIGNED_ONLY for clarity and move
> defines out of target/foo/cpu.h into configure, as we do with
> TARGET_WORDS_BIGENDIAN, so that it is always defined early.
> 
> Poisoned TARGET_ALIGNED_ONLY to prevent use in common code.
> 
> Signed-off-by: Tony Nguyen 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Richard Henderson 
> Reviewed-by: Aleksandar Markovic 
> ---
>  configure | 10 +-
>  include/exec/poison.h |  1 +
>  include/qom/cpu.h |  2 +-
>  target/alpha/cpu.h|  2 --
>  target/hppa/cpu.h |  1 -
>  target/mips/cpu.h |  2 --
>  target/sh4/cpu.h  |  2 --
>  target/sparc/cpu.h|  2 --
>  target/xtensa/cpu.h   |  2 --
>  tcg/tcg.c |  2 +-
>  tcg/tcg.h |  8 +---
>  11 files changed, 17 insertions(+), 17 deletions(-)

Reviewed-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/4] xen/console: Don't treat NUL character as the end of the buffer

[Julien Grall]

Hi Jan,

On 08/08/2019 14:51, Jan Beulich wrote:

On 05.08.2019 15:29, Julien Grall wrote:

@@ -1261,14 +1259,15 @@ void debugtrace_printk(const char *fmt, ...)
  ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
   
  va_start(args, fmt);

-vsnprintf(buf, sizeof(buf), fmt, args);
+nr = vscnprintf(buf, sizeof(buf), fmt, args);
  va_end(args);
  
  if ( debugtrace_send_to_console )

  {
-snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
-serial_puts(sercon_handle, cntbuf);
-serial_puts(sercon_handle, buf);
+unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);


While - given the size of cntbuf - the difference is mostly
benign, you using vscnprintf() above calls for you also
using scnprintf() here.


Good point, it would be safer too. I will update it.




--- a/xen/include/xen/video.h
+++ b/xen/include/xen/video.h
@@ -13,11 +13,11 @@
  
  #ifdef CONFIG_VIDEO

  void video_init(void);
-extern void (*video_puts)(const char *);
+extern void (*video_puts)(const char *, size_t nr);
  void video_endboot(void);
  #else
  #define video_init()((void)0)
-#define video_puts(s)   ((void)0)
+#define video_puts(s, nr)   ((void)0)


While I don't think there's overly much risk of "s" getting an
argument with side effects passed, I think that for "nr" the
risk is there. May I ask that you evaluate both here, just in
case?


Are you happy with the following code (Not yet compiled!):

#define video_ptus(s, nr) ((void)(s), (void)(nr))



Preferably with these adjustments
Reviewed-by: Jan Beulich 


Thank you!

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 3/4] xen/public: Document HYPERCALL_console_io()

[Jan Beulich]

On 05.08.2019 15:29, Julien Grall wrote:
> Currently, OS developpers will have to look at Xen code in order to know
> the parameters of an hypercall and how it is meant to work.
> 
> This is not a trivial task as you may need to have a deep understanding
> of Xen internal.
> 
> This patch attempts to document the behavior of HYPERCALL_console_io() to
> help OS developer.
> 
> Signed-off-by: Julien Grall 

Acked-by: Jan Beulich 
with a couple of nits:

> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -486,7 +486,29 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>   /* ` } */
>   
>   /*
> - * Commands to HYPERVISOR_console_io().
> + * ` int
> + * ` HYPERVISOR_console_io(unsigned int cmd,
> + * `   unsigned int count,
> + * `   char buffer[]);
> + *
> + * @cmd: Command (see below)
> + * @count: Size of the buffer to read/write
> + * @buffer: Pointer in the guest memory
> + *
> + * List of commands:
> + *
> + *  * CONSOLEIO_write: Write the buffer on Xen console.

s/ on / to / ?

> + *  For the hardware domain, all the characters in the buffer will
> + *  be written. Characters will be printed to directly to the

The first "to" looks to be unwanted.

> + *  console.
> + *  For all the other domains, only the printable characters will be
> + *  written. Characters may be buffered until a newline (i.e '\n') is
> + *  found.
> + *  @return 0 on success, otherwise return an error code.
> + *  * CONSOLEIO_read: Attempts to read up @count characters from Xen console.

"... up to @count ..."

> + *  The maximum buffer size (i.e @count) supported is 2GB.

"i.e." or "ie" are the two common forms I'm aware of.

> + *  @return the number of character read on success, otherwise return

"characters"

Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [Qemu-devel] [PATCH v6 02/26] tcg: TCGMemOp is now accelerator independent MemOp

[Cornelia Huck]

On Wed, 7 Aug 2019 08:26:23 +
 wrote:

> Preparation for collapsing the two byte swaps, adjust_endianness and
> handle_bswap, along the I/O path.
> 
> Target dependant attributes are conditionalize upon NEED_CPU_H.

s/conditionalize/conditionalized/ ?

> 
> Signed-off-by: Tony Nguyen 
> Acked-by: David Gibson 
> Reviewed-by: Richard Henderson 
> ---
>  MAINTAINERS |   1 +
>  accel/tcg/cputlb.c  |   2 +-
>  include/exec/memop.h| 110 ++
>  target/alpha/translate.c|   2 +-
>  target/arm/translate-a64.c  |  48 ++--
>  target/arm/translate-a64.h  |   2 +-
>  target/arm/translate-sve.c  |   2 +-
>  target/arm/translate.c  |  32 
>  target/arm/translate.h  |   2 +-
>  target/hppa/translate.c |  14 ++--
>  target/i386/translate.c | 132 
> 
>  target/m68k/translate.c |   2 +-
>  target/microblaze/translate.c   |   4 +-
>  target/mips/translate.c |   8 +-
>  target/openrisc/translate.c |   4 +-
>  target/ppc/translate.c  |  12 +--
>  target/riscv/insn_trans/trans_rva.inc.c |   8 +-
>  target/riscv/insn_trans/trans_rvi.inc.c |   4 +-
>  target/s390x/translate.c|   6 +-
>  target/s390x/translate_vx.inc.c |  10 +--
>  target/sparc/translate.c|  14 ++--
>  target/tilegx/translate.c   |  10 +--
>  target/tricore/translate.c  |   8 +-
>  tcg/README  |   2 +-
>  tcg/aarch64/tcg-target.inc.c|  26 +++
>  tcg/arm/tcg-target.inc.c|  26 +++
>  tcg/i386/tcg-target.inc.c   |  24 +++---
>  tcg/mips/tcg-target.inc.c   |  16 ++--
>  tcg/optimize.c  |   2 +-
>  tcg/ppc/tcg-target.inc.c|  12 +--
>  tcg/riscv/tcg-target.inc.c  |  20 ++---
>  tcg/s390/tcg-target.inc.c   |  14 ++--
>  tcg/sparc/tcg-target.inc.c  |   6 +-
>  tcg/tcg-op.c|  38 -
>  tcg/tcg-op.h|  86 ++---
>  tcg/tcg.c   |   2 +-
>  tcg/tcg.h   | 101 ++--
>  trace/mem-internal.h|   4 +-
>  trace/mem.h |   4 +-
>  39 files changed, 421 insertions(+), 399 deletions(-)
>  create mode 100644 include/exec/memop.h

Acked-by: Cornelia Huck 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH V2 6/6] iommu/arm: Add Renesas IPMMU-VMSA support

[Lars Kurth]

On 07/08/2019, 20:15, "Julien Grall"  wrote:

(+ Lars)

Hi,

On 8/7/19 5:01 PM, Oleksandr wrote:
>>> + * you can found at:
>>> + *url: 
>>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git
>>> + *branch: v4.14.75-ltsi/rcar-3.9.6
>>> + *commit: e206eb5b81a60e64c35fbc3a999b1a0db2b98044
>>> + * and Xen's SMMU driver:
>>> + *xen/drivers/passthrough/arm/smmu.c
>>> + *
>>> + * Copyright (C) 2016-2019 EPAM Systems Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms and conditions of the GNU General Public
>>> + * License, version 2, as published by the Free Software Foundation.
>>> + *
>>> + * 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 
>>> .
>> I don't know that Xen license description rule, but since a few source 
>> files have
>> SPDX-License-Identifier, can we also use it on the driver?
> 
> I am afraid, I don't know a correct answer for this question. I would 
> leave this to maintainers.
> 
> I just followed sample copyright notice for GPL v2 License according to 
> the document:
> 
> http://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=CONTRIBUTING

The file CONTRIBUTING is only giving example of common example of 
license. So I think this is fine to use SPDX, the more they are already 
used. The only request is to put either SDPX or the full-blown text but 
not the two :). Lars, any objection?

I am quite in favor of SPDX because it is easier to find out the 
license. With the full-blown text, the text may slightly vary between 
licenses. For instance, the only difference between GPLv2 and GPLv2+ is 
",or (at your option) any later version". I let you imagine how it can 
be easy to miss it when reviewing ;).

We had a discussion last year about using SPDX in Xen code base but I 
never got the time to formally suggest it.

I did not push it either. 

In the past one of the committers had major objections against SPDX, but after 
a conversation last year and changes to the latest version of SPDX he dropped 
these.

The only remaining objection was to have both SPDX identifier AND a license in 
the same file. The argument against it is: what does it mean if they contradict 
each other? To be fair that is a valid concern.

I am not sure it is a good idea to introduce SPDX piecemeal. It would be much 
better to
a) agree it
b) transform the codebase using a tool
rather than introducing it piecemeal

Lars
 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 29/35] OvmfPkg/OvmfXen: Override PcdFSBClock to Xen vLAPIC timer frequency

[Anthony PERARD]

On Thu, Aug 08, 2019 at 03:44:23PM +0200, Roger Pau Monné wrote:
> On Thu, Aug 08, 2019 at 02:28:32PM +0100, Anthony PERARD wrote:
> > On Wed, Aug 07, 2019 at 05:54:51PM +0200, Roger Pau Monné wrote:
> > > On Mon, Jul 29, 2019 at 04:39:38PM +0100, Anthony PERARD wrote:
> > > > PcdFSBClock is used by SecPeiDxeTimerLibCpu, the TimerLib
> > > > implementation. It will also be used by XenTimerDxe. Override
> > > > PcdFSBClock to match Xen vLAPIC timer frequency.
> > > 
> > > I'm kind of surprised that his is not automatically detected?
> > > 
> > > Is it a bug in Xen or just always hardcoded on OVMF?
> > 
> > It's hardcoded. Why would you need auto detection when you always run on
> > the same machine ;-).
> 
> I don't think that's part of the HVM/PVH ABI in any way, and if you
> hardcode it in OVMF it would prevent Xen from changing the frequency
> in the future if such necessity arises. We should try to avoid painting
> ourselves into a corner when possible.
> 
> Doesn't OVMF have a way to get this from the hardware itself?

So EDKII doesn't have that capability, FSBClock is a build time value
and can't be changed at run time. But OVMF (on KVM or HVM) doesn't use
that value at all, it uses the ACPI timer instead.

We could try to find a better way to get time in OvmfXen without
hardcoding FSBClock, but maybe in a follow-up.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 35/35] OvmfPkg/OvmfXen: use RealTimeClockRuntimeDxe from EmbeddedPkg

[Anthony PERARD]

On Wed, Aug 07, 2019 at 06:09:57PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:44PM +0100, Anthony PERARD wrote:
> > A Xen PVH guest doesn't have a RTC that OVMF would expect, so
> > PcatRealTimeClockRuntimeDxe fails to initialize and prevent the
> > firmware from finish to boot. To prevent that, we will use
> > XenRealTimeClockLib which simply always return the same time.
> > This will work on both Xen PVH and HVM guests.
> 
> Not that this is not correct, but what's the point in requiring a
> clock if it can be faked by always returning the same value?

It's not a clock that is required, it is a library that implements
RealTimeClockLib. Something needs it, so it is provided, even if it is
only to display the "current time".

> It seems like it's usage is not really required, and could indeed be
> dropped altogether?

Way to much work to drop it. Also, I don't work to fork OVMF.

The ARM implementation of OVMF for Xen does the same thing and simply
always return the same value.

> Alternatively, there's the PV clock which is available to PVH guests
> and will return a proper time.

We might need to do that one day I guess, but right now it is just a
nice to have.

Thanks,

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [edk2-devel] [PATCH v4 33/35] OvmfPkg: Introduce XenIoPvhDxe to initialize Grant Tables

[Anthony PERARD]

On Wed, Aug 07, 2019 at 06:07:03PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 29, 2019 at 04:39:42PM +0100, Anthony PERARD wrote:
> > XenIoPvhDxe use XenIoMmioLib to reserve some space to be use by the
> > Grant Tables.
> > 
> > The call is only done if it is necessary, we simply detect if the
> > guest is PVH, as in this case there is currently no PCI bus, and no
> > PCI Xen platform device which would start the XenIoPciDxe and allocate
> > the space for the Grant Tables.
> 
> Since I'm not familiar with OVMF code, where is the grant table
> physical memory coming from then, is it allocated from a hole in the
> memory map?

On HVM, we use the first BAR of the Xen platform PCI device. Since there
is no such thing on PVH, I simply allocate some memory for it, with
AllocateReservedPages() which mean allocate some pages and tell the OS
to not touch them.

We could deallocate these pages and give them back to the OS but that
would need some refactoring.

-- 
Anthony PERARD

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel