Re: [Xen-devel] [PATCH v1 1/4] kasan: introduce set_pmd_early_shadow()

[Jürgen Groß]

On 15.01.20 17:32, Sergey Dyasli wrote:

On 15/01/2020 11:09, Jürgen Groß wrote:

On 15.01.20 11:54, Sergey Dyasli wrote:

Hi Juergen,

On 08/01/2020 15:20, Sergey Dyasli wrote:

It is incorrect to call pmd_populate_kernel() multiple times for the
same page table. Xen notices it during kasan_populate_early_shadow():

  (XEN) mm.c:3222:d155v0 mfn 3704b already pinned

This happens for kasan_early_shadow_pte when USE_SPLIT_PTE_PTLOCKS is
enabled. Fix this by introducing set_pmd_early_shadow() which calls
pmd_populate_kernel() only once and uses set_pmd() afterwards.

Signed-off-by: Sergey Dyasli 


Looks like the plan to use set_pmd() directly has failed: it's an
arch-specific function and can't be used in arch-independent code
(as kbuild test robot has proven).

Do you see any way out of this other than disabling SPLIT_PTE_PTLOCKS
for PV KASAN?


Change set_pmd_early_shadow() like the following:

#ifdef CONFIG_XEN_PV
static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
{
 static bool pmd_populated = false;

 if (likely(pmd_populated)) {
 set_pmd(pmd, __pmd(__pa(early_shadow) | _PAGE_TABLE));
 } else {
 pmd_populate_kernel(_mm, pmd, early_shadow);
 pmd_populated = true;
 }
}
#else
static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
{
 pmd_populate_kernel(_mm, pmd, early_shadow);
}
#endif

... and move it to include/xen/xen-ops.h and call it with
lm_alias(kasan_early_shadow_pte) as the second parameter.


Your suggestion to use ifdef is really good, especially now when I
figured out that CONFIG_XEN_PV implies X86. But I don't like the idea
of kasan code calling a non-empty function from xen-ops.h when
CONFIG_XEN_PV is not defined. I'd prefer to keep set_pmd_early_shadow()
in mm/kasan/init.c with the suggested ifdef.


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 v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

Hi

Am 16.01.20 um 07:41 schrieb Daniel Vetter:
> On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote:
>> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
>> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.
>>
>> For drivers that have neither an enable_vblank() callback nor a check()
>> callback, the simple-KMS helpers enable VBLANK generation by default. This
>> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
>> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
>> its own logic for sending these events.
>>
>> Signed-off-by: Thomas Zimmermann 
> 
>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
>> b/drivers/gpu/drm/drm_simple_kms_helper.c
>> index 15fb516ae2d8..4414c7a5b2ce 100644
>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
>> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct 
>> drm_plane *plane,
>>  if (!plane_state->visible)
>>  return 0;
>>  
>> -if (!pipe->funcs || !pipe->funcs->check)
>> -return 0;
>> -
>> -return pipe->funcs->check(pipe, plane_state, crtc_state);
>> +if (pipe->funcs) {
>> +if (pipe->funcs->check)
>> +return pipe->funcs->check(pipe, plane_state,
>> +  crtc_state);
>> +if (pipe->funcs->enable_vblank)
>> +return 0;
>> +}
>> +
>> +/* Drivers without VBLANK support have to fake VBLANK events. As
>> + * there's no check() callback to enable this, set the no_vblank
>> + * field by default.
>> + */
> 
> The ->check callback is right above this comment ... I'm confused.

I guess that comment isn't overly precise. What it means is that
no_vblank would have to be set in check(), but the driver did not
specify a check() function. So it has neither vblank support nor any way
of setting no_vblank. Hence, the simple-kms helper sets no_vblank
automatically.

Maybe something to update for the patchset's v2.

> 
>> +crtc_state->no_vblank = true;
> 
> That's kinda not what I meant with handling this automatically. Instead
> something like this:
> 
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 7cf3cf936547..23d2f51fc1d4 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct 
> drm_crtc *crtc,
>   /* Self refresh should be canceled when a new update is available */
>   state->active = drm_atomic_crtc_effectively_active(state);
>   state->self_refresh_active = false;
> +
> + if (drm_dev_has_vblank(crtc->dev))
> + state->no_vblank = true;
> + else
> + state->no_vblank = false;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);

I think the if/else branches are in the wrong order.

But generally speaking, is it really that easy? The xen driver already
has to work around simple-kms's auto-enabling of no_vblank (see patch
4). Maybe this settings interferes with other drivers as well. At least
the calls for sending fake vblanks should be removed from all affected
drivers.

Best regards
Thomas

>  
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 1659b13b178c..32cab3d3c872 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -81,6 +81,12 @@
>   */
>  #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100
>  
> +/* FIXME roll this out here in this file */
> +bool drm_dev_has_vblank(dev)
> +{
> + return dev->num_crtcs;
> +}
> +
>  static bool
>  drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
> ktime_t *tvblank, bool in_vblank_irq);
> 
> 
> But maybe move the default value to some other/better place in the atomic
> helpers, not sure what the best one is.
> 
> Plus then in the documentation patch also highlight the link between
> crtc_state->no_vblank and drm_dev_has_vblank respectively
> drm_device.num_crtcs.
> 
> That should plug this issue once for all across the board.
> 
> There's still the fun between having the vblank callbacks and the
> drm_vblank setup, but that's a much older can of worms ...
> -Daniel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



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

[Xen-devel] [xen-4.10-testing test] 146101: tolerable trouble: fail/pass/starved - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 144798
 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail never pass
 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install 
fail never pass
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-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-xl-xsm  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  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-libvirt 13 migrate-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-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 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-armhf-armhf-xl  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 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-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 22 leak-check/check  fail never pass
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail 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-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass
 test-amd64-amd64-xl-qemut-win7-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-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass
 test-arm64-arm64-xl-thunderx  2 hosts-allocate   starved  n/a

version targeted for testing:
 xen  49a5d6e92317a7d9acbf0bdbd25b2809dfd84260
baseline version:
 xen  6cb1cb9c63e91b71ce639e7b7cf08ca85d44266f

Last test of basis   144798  2019-12-13 20:32:27 Z   33 days
Testing same since   146076  2020-01-14 14:35:58 Z1 days2 attempts


People who touched revisions under test:
  Julien Grall 

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

Re: [Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Daniel Vetter]

On Wed, Jan 15, 2020 at 01:52:26PM +0100, Thomas Zimmermann wrote:
> In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
> if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.
> 
> For drivers that have neither an enable_vblank() callback nor a check()
> callback, the simple-KMS helpers enable VBLANK generation by default. This
> simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
> DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
> its own logic for sending these events.
> 
> Signed-off-by: Thomas Zimmermann 

> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
> b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 15fb516ae2d8..4414c7a5b2ce 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct 
> drm_plane *plane,
>   if (!plane_state->visible)
>   return 0;
>  
> - if (!pipe->funcs || !pipe->funcs->check)
> - return 0;
> -
> - return pipe->funcs->check(pipe, plane_state, crtc_state);
> + if (pipe->funcs) {
> + if (pipe->funcs->check)
> + return pipe->funcs->check(pipe, plane_state,
> +   crtc_state);
> + if (pipe->funcs->enable_vblank)
> + return 0;
> + }
> +
> + /* Drivers without VBLANK support have to fake VBLANK events. As
> +  * there's no check() callback to enable this, set the no_vblank
> +  * field by default.
> +  */

The ->check callback is right above this comment ... I'm confused.

> + crtc_state->no_vblank = true;

That's kinda not what I meant with handling this automatically. Instead
something like this:


diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 7cf3cf936547..23d2f51fc1d4 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -149,6 +149,11 @@ void __drm_atomic_helper_crtc_duplicate_state(struct 
drm_crtc *crtc,
/* Self refresh should be canceled when a new update is available */
state->active = drm_atomic_crtc_effectively_active(state);
state->self_refresh_active = false;
+
+   if (drm_dev_has_vblank(crtc->dev))
+   state->no_vblank = true;
+   else
+   state->no_vblank = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
 
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 1659b13b178c..32cab3d3c872 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -81,6 +81,12 @@
  */
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100
 
+/* FIXME roll this out here in this file */
+bool drm_dev_has_vblank(dev)
+{
+   return dev->num_crtcs;
+}
+
 static bool
 drm_get_last_vbltimestamp(struct drm_device *dev, unsigned int pipe,
  ktime_t *tvblank, bool in_vblank_irq);


But maybe move the default value to some other/better place in the atomic
helpers, not sure what the best one is.

Plus then in the documentation patch also highlight the link between
crtc_state->no_vblank and drm_dev_has_vblank respectively
drm_device.num_crtcs.

That should plug this issue once for all across the board.

There's still the fun between having the vblank callbacks and the
drm_vblank setup, but that's a much older can of worms ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Xen-devel] Raspberry pi4

[Ivan Sheihets]

Hello guys,

Does anyone run Xen on RPi4 and can help?
I just found 
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg50685.html but 
there doesn't work HDMI and memory limitation.

Best regards,
Ivan Sheihets
Infopulse Ukraine
+38-097-913-10-01

[cid:image001.png@01D0A777.74430340]
www.infopulse.com.ua
P Please don't print this message unless you really need to. Thank you.

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

[Xen-devel] [xen-4.12-testing test] 146100: tolerable FAIL - PUSHED

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-qcow217 guest-localmigrate/x10   fail  like 145017
 test-amd64-i386-xl-pvshim12 guest-start  fail   never pass
 test-amd64-i386-libvirt-xsm  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-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
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-arm64-arm64-xl-seattle  14 saverestore-support-checkfail   never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-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-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-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail never pass
 test-arm64-arm64-xl-xsm  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-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-armhf-armhf-xl-arndale  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-arndale  14 saverestore-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail   never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 14 saverestore-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-win7-amd64 17 guest-stop  fail 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-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail 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-amd64-i386-xl-qemut-win7-amd64 17 guest-stop  fail never pass
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass
 test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail   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-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass

version targeted for testing:
 xen  a5fcafbfbee55261853fba07149c1c795f2baf58
baseline version:
 xen  890711084303e6b56881b836d91273ec56015b5e

Last test of basis   145017  2019-12-20 11:09:27 Z   26 days
Testing same since   146078  2020-01-14 14:36:21 Z1 days2 attempts


People who touched revisions under test:
  Julien Grall 

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

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  b31666c8912bf18d9eff963b06d856e7e818ff34
baseline version:
 xen  cac57fda01d25c079458a146eefd267e3e72e7fc

Last test of basis   146118  2020-01-15 17:01:00 Z0 days
Testing same since   146131  2020-01-15 21:02:49 Z0 days1 attempts


People who touched revisions under test:
  Julien Grall 
  Stefano Stabellini 
  Stefano Stabellini 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   cac57fda01..b31666c891  b31666c8912bf18d9eff963b06d856e7e818ff34 -> smoke

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

Re: [Xen-devel] [PATCH v4] xen-pciback: optionally allow interrupt enable flag writes

[Marek Marczykowski-Górecki]

On Wed, Jan 15, 2020 at 05:32:32PM -0500, Boris Ostrovsky wrote:
> 
> 
> On 1/15/20 11:48 AM, Roger Pau Monné wrote:
> > On Wed, Jan 15, 2020 at 02:46:29AM +0100, Marek Marczykowski-Górecki wrote:
> > > QEMU running in a stubdom needs to be able to set INTX_DISABLE, and the
> > > MSI(-X) enable flags in the PCI config space. This adds an attribute
> > > 'allow_interrupt_control' which when set for a PCI device allows writes
> > > to this flag(s). The toolstack will need to set this for stubdoms.
> > > When enabled, guest (stubdomain) will be allowed to set relevant enable
> > > flags, but only one at a time - i.e. it refuses to enable more than one
> > > of INTx, MSI, MSI-X at a time.
> > > 
> > > This functionality is needed only for config space access done by device
> > > model (stubdomain) serving a HVM with the actual PCI device. It is not
> > > necessary and unsafe to enable direct access to those bits for PV domain
> > > with the device attached. For PV domains, there are separate protocol
> > > messages (XEN_PCI_OP_{enable,disable}_{msi,msix}) for this purpose.
> > > Those ops in addition to setting enable bits, also configure MSI(-X) in
> > > dom0 kernel - which is undesirable for PCI passthrough to HVM guests.
> > > 
> > > This should not introduce any new security issues since a malicious
> > > guest (or stubdom) can already generate MSIs through other ways, see
> > > [1] page 8. Additionally, when qemu runs in dom0, it already have direct
> > > access to those bits.
> > > 
> > > This is the second iteration of this feature. First was proposed as a
> > > direct Xen interface through a new hypercall, but ultimately it was
> > > rejected by the maintainer, because of mixing pciback and hypercalls for
> > > PCI config space access isn't a good design. Full discussion at [2].
> > > 
> > > [1]: 
> > > https://invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> > > [2]: https://xen.markmail.org/thread/smpgpws4umdzizze
> > > 
> > > [part of the commit message and sysfs handling]
> > > Signed-off-by: Simon Gaiser 
> > > [the rest]
> > > Signed-off-by: Marek Marczykowski-Górecki 
> > > 
> > Some minor nits below, but LGTM:
> > 
> > Reviewed-by: Roger Pau Monné 
> > 
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-pciback 
> > > b/Documentation/ABI/testing/sysfs-driver-pciback
> > > index 6a733bfa37e6..566a11f2c12f 100644
> > > --- a/Documentation/ABI/testing/sysfs-driver-pciback
> > > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > > @@ -11,3 +11,16 @@ Description:
> > >   #echo 00:19.0-E0:2:FF > 
> > > /sys/bus/pci/drivers/pciback/quirks
> > >   will allow the guest to read and write to the 
> > > configuration
> > >   register 0x0E.
> > > +
> > > +What:   /sys/bus/pci/drivers/pciback/allow_interrupt_control
> > > +Date:   Jan 2020
> > > +KernelVersion:  5.5
> 
> 5.6
> 
> I can fix this and the things that Roger mentioned while committing if Marek
> is OK with that.

Yes, thanks!

-- 
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

[Xen-devel] [xen-4.9-testing test] 146097: regressions - trouble: fail/pass/starved

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-xl-qemut-win7-amd64 16 guest-localmigrate/x10 fail REGR. vs. 
144758
 test-amd64-amd64-xl-qemuu-ws16-amd64 16 guest-localmigrate/x10 fail REGR. vs. 
144758
 build-i386-xsm6 xen-build  fail in 146075 REGR. vs. 144758

Tests which are failing intermittently (not blocking):
 test-amd64-amd64-xl-qemuu-ovmf-amd64 15 guest-saverestore.2 fail pass in 146075
 test-amd64-amd64-xl-qemuu-win7-amd64 16 guest-localmigrate/x10 fail pass in 
146075

Tests which did not succeed, but are not blocking:
 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked in 
146075 n/a
 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 1 build-check(1) blocked in 146075 
n/a
 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 1 build-check(1) blocked in 146075 
n/a
 test-amd64-i386-libvirt-xsm   1 build-check(1)   blocked in 146075 n/a
 test-amd64-i386-xl-xsm1 build-check(1)   blocked in 146075 n/a
 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 1 build-check(1) blocked 
in 146075 n/a
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail in 146075 blocked in 
144758
 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail in 146075 blocked in 
144758
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop  fail in 146075 like 144758
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144723
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144758
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144758
 test-amd64-amd64-xl-qemut-ws16-amd64 16 guest-localmigrate/x10 fail like 144758
 test-amd64-i386-xl-qemut-ws16-amd64 16 guest-localmigrate/x10 fail like 144758
 test-amd64-i386-libvirt-xsm  13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-amd64-amd64-libvirt 13 migrate-support-checkfail   never pass
 test-amd64-i386-libvirt  13 migrate-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-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail 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-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-seattle  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-seattle  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-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check 
fail never pass
 test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail   never pass
 test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  fail 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-multivcpu 13 migrate-support-checkfail  never pass
 test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail  never pass
 test-armhf-armhf-libvirt 13 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt 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-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-raw 12 migrate-support-checkfail   never pass
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail   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-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-cubietruck 13 migrate-support-checkfail never pass
 test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass
 test-arm64-arm64-xl-credit2  13 

Re: [Xen-devel] [PATCH v4] xen-pciback: optionally allow interrupt enable flag writes

[Boris Ostrovsky]

On 1/15/20 11:48 AM, Roger Pau Monné wrote:

On Wed, Jan 15, 2020 at 02:46:29AM +0100, Marek Marczykowski-Górecki wrote:

QEMU running in a stubdom needs to be able to set INTX_DISABLE, and the
MSI(-X) enable flags in the PCI config space. This adds an attribute
'allow_interrupt_control' which when set for a PCI device allows writes
to this flag(s). The toolstack will need to set this for stubdoms.
When enabled, guest (stubdomain) will be allowed to set relevant enable
flags, but only one at a time - i.e. it refuses to enable more than one
of INTx, MSI, MSI-X at a time.

This functionality is needed only for config space access done by device
model (stubdomain) serving a HVM with the actual PCI device. It is not
necessary and unsafe to enable direct access to those bits for PV domain
with the device attached. For PV domains, there are separate protocol
messages (XEN_PCI_OP_{enable,disable}_{msi,msix}) for this purpose.
Those ops in addition to setting enable bits, also configure MSI(-X) in
dom0 kernel - which is undesirable for PCI passthrough to HVM guests.

This should not introduce any new security issues since a malicious
guest (or stubdom) can already generate MSIs through other ways, see
[1] page 8. Additionally, when qemu runs in dom0, it already have direct
access to those bits.

This is the second iteration of this feature. First was proposed as a
direct Xen interface through a new hypercall, but ultimately it was
rejected by the maintainer, because of mixing pciback and hypercalls for
PCI config space access isn't a good design. Full discussion at [2].

[1]: 
https://invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
[2]: https://xen.markmail.org/thread/smpgpws4umdzizze

[part of the commit message and sysfs handling]
Signed-off-by: Simon Gaiser 
[the rest]
Signed-off-by: Marek Marczykowski-Górecki 

Some minor nits below, but LGTM:

Reviewed-by: Roger Pau Monné 



diff --git a/Documentation/ABI/testing/sysfs-driver-pciback 
b/Documentation/ABI/testing/sysfs-driver-pciback
index 6a733bfa37e6..566a11f2c12f 100644
--- a/Documentation/ABI/testing/sysfs-driver-pciback
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -11,3 +11,16 @@ Description:
  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
  will allow the guest to read and write to the configuration
  register 0x0E.
+
+What:   /sys/bus/pci/drivers/pciback/allow_interrupt_control
+Date:   Jan 2020
+KernelVersion:  5.5


5.6

I can fix this and the things that Roger mentioned while committing if 
Marek is OK with that.


-boris




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

Re: [Xen-devel] [PATCH v4 13/16] Regenerate autotools files

[Rich Persaud]

> On Jan 14, 2020, at 21:42, Marek Marczykowski-Górecki 
>  wrote:
> Since we have those generated files committed to the repo (why?!),
> update them after changing configure.ac.

Is there any reason not to remove the generated configure files?  A developer 
using generated files on system B would be incorporating configuration 
assumptions from system A where the configure script was generated.  If we are 
going to ship configure scripts, do we need to document a "system A" reference 
distro/environment where all configure scripts from Xen will be generated?


Other notes:

1.  Debian autoreconf works in the Xen root directory, but the default 
OpenEmbedded autoreconf uses Gnu libtoolize and fails because some Xen build 
subdirectories don't have configure.ac/.in.   

2.  If OpenEmbedded autoreconf is run only in the tools directory (where it 
works and generates a new tools configure), then root configure (generated from 
older configure.ac) will silently ignore the newer tools configure and write 
config.h _without_ tools-specific config, such as the vchan QMP proxy.

3. If autoreconf runs successfully in the root directory, then tools-specific 
configure is correctly generated and everything works as expected.

This silent failure could be avoided by deleting the generated configure 
scripts.  There may be other failure modes for using System A generated scripts 
on downstream build system B.

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

Re: [Xen-devel] [PATCH 0.5/12] tools/migration: Formatting and style cleanup

[Ian Jackson]

Andrew Cooper writes ("[PATCH 0.5/12] tools/migration: Formatting and style 
cleanup"):
> The code has devating from the prevailing style in many ways.  Adjust spacing,
> indentation, position of operators, layout of multiline comments, removal of
> superfluous comments, constness, trailing commas, and use of unqualified
> 'unsigned'.
> 
> No functional change.

Acked-by: Ian Jackson 

Thanks,
Ian.

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

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

[osstest service owner]

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

Failures :-/ but no regressions.

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-libvirt 13 migrate-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  14 saverestore-support-checkfail   never pass

version targeted for testing:
 xen  cac57fda01d25c079458a146eefd267e3e72e7fc
baseline version:
 xen  b4194711ffaffa5e63d986338fb8d4020fa6bad1

Last test of basis   146091  2020-01-14 19:01:00 Z1 days
Testing same since   146118  2020-01-15 17:01:00 Z0 days1 attempts


People who touched revisions under test:
  Andrew Cooper 
  Ian Jackson 
  Jan Beulich 
  Juergen Gross 
  Tamas K Lengyel 
  Wei Liu 

jobs:
 build-arm64-xsm  pass
 build-amd64  pass
 build-armhf  pass
 build-amd64-libvirt  pass
 test-armhf-armhf-xl  pass
 test-arm64-arm64-xl-xsm  pass
 test-amd64-amd64-xl-qemuu-debianhvm-amd64pass
 test-amd64-amd64-libvirt pass



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

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

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

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


Pushing revision :

To xenbits.xen.org:/home/xen/git/xen.git
   b4194711ff..cac57fda01  cac57fda01d25c079458a146eefd267e3e72e7fc -> smoke

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

Re: [Xen-devel] [PATCH 01/12] libxc/save: Shrink code volume where possible

[Andrew Cooper]

On 14/01/2020 16:48, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 01/12] libxc/save: Shrink code volume where 
> possible"):
>> A property of how the error handling (0 on success, nonzero otherwise)
>> allows these calls to be chained together with the ternary operatior.
> I'm quite surprised to find a suggestion like this coming from you in
> particular.

What probably is relevant is that ?: is a common construct in the
hypervisor, which I suppose does colour my expectation of people knowing
exactly what it means and how it behaves.

> Maybe it would be better to have
> #define MUST(call) ({ rc = (call); if (rc) goto error; })
> and write
> MUST( write_one_vcpu_basic(ctx, i) );
>
> Or just to permit
>rc = write_one_vcpu_basic(ctx, i);if (rc) goto error;
> (ie on a single line).

OTOH, it should come as no surprise that I'd rather drop this patch
entirely than go with these alternatives, both of which detract from
code clarity.  The former for hiding control flow, and the latter for
being atypical layout which unnecessary cognitive load to follow.

~Andrew

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

[Xen-devel] [PATCH 0.5/12] tools/migration: Formatting and style cleanup

[Andrew Cooper]

The code has devating from the prevailing style in many ways.  Adjust spacing,
indentation, position of operators, layout of multiline comments, removal of
superfluous comments, constness, trailing commas, and use of unqualified
'unsigned'.

No functional change.

Signed-off-by: Andrew Cooper 
---
CC: Ian Jackson 
CC: Wei Liu 
---
 tools/libxc/include/xenguest.h |  35 
 tools/libxc/xc_sr_common.c |   9 +--
 tools/libxc/xc_sr_common.h |  10 +--
 tools/libxc/xc_sr_common_x86.c |   5 +-
 tools/libxc/xc_sr_common_x86_pv.c  |  12 +--
 tools/libxc/xc_sr_restore.c|  39 -
 tools/libxc/xc_sr_restore_x86_pv.c |  21 ++---
 tools/libxc/xc_sr_save.c   |  74 -
 tools/libxc/xc_sr_save_x86_hvm.c   |   7 +-
 tools/libxc/xc_sr_save_x86_pv.c| 101 +--
 tools/python/scripts/convert-legacy-stream |  20 ++---
 tools/python/xen/migration/libxc.py| 124 +++--
 tools/python/xen/migration/libxl.py|  39 -
 13 files changed, 235 insertions(+), 261 deletions(-)

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 237603373c..19d828a7f2 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -52,10 +52,11 @@ typedef int (*precopy_policy_t)(struct precopy_stats, void 
*);
 
 /* callbacks provided by xc_domain_save */
 struct save_callbacks {
-/* Called after expiration of checkpoint interval,
+/*
+ * Called after expiration of checkpoint interval,
  * to suspend the guest.
  */
-int (*suspend)(void* data);
+int (*suspend)(void *data);
 
 /*
  * Called before and after every batch of page data sent during
@@ -79,7 +80,7 @@ struct save_callbacks {
  * xc_domain_save then flushes the output buffer, while the
  *  guest continues to run.
  */
-int (*postcopy)(void* data);
+int (*postcopy)(void *data);
 
 /*
  * Called after the memory checkpoint has been flushed
@@ -94,7 +95,7 @@ struct save_callbacks {
  * 0: terminate checkpointing gracefully
  * 1: take another checkpoint
  */
-int (*checkpoint)(void* data);
+int (*checkpoint)(void *data);
 
 /*
  * Called after the checkpoint callback.
@@ -103,13 +104,13 @@ struct save_callbacks {
  * 0: terminate checkpointing gracefully
  * 1: take another checkpoint
  */
-int (*wait_checkpoint)(void* data);
+int (*wait_checkpoint)(void *data);
 
 /* Enable qemu-dm logging dirty pages to xen */
 int (*switch_qemu_logdirty)(uint32_t domid, unsigned enable, void *data); 
/* HVM only */
 
 /* to be provided as the last argument to each callback function */
-void* data;
+void *data;
 };
 
 /* Type of stream.  Plain, or using a continuous replication protocol? */
@@ -138,22 +139,24 @@ int xc_domain_save(xc_interface *xch, int io_fd, uint32_t 
dom,
 
 /* callbacks provided by xc_domain_restore */
 struct restore_callbacks {
-/* Called after a new checkpoint to suspend the guest.
- */
-int (*suspend)(void* data);
+/* Called after a new checkpoint to suspend the guest. */
+int (*suspend)(void *data);
 
-/* Called after the secondary vm is ready to resume.
+/*
+ * Called after the secondary vm is ready to resume.
  * Callback function resumes the guest & the device model,
  * returns to xc_domain_restore.
  */
-int (*postcopy)(void* data);
+int (*postcopy)(void *data);
 
-/* A checkpoint record has been found in the stream.
- * returns: */
+/*
+ * A checkpoint record has been found in the stream.
+ * returns:
+ */
 #define XGR_CHECKPOINT_ERROR0 /* Terminate processing */
 #define XGR_CHECKPOINT_SUCCESS  1 /* Continue reading more data from the 
stream */
 #define XGR_CHECKPOINT_FAILOVER 2 /* Failover and resume VM */
-int (*checkpoint)(void* data);
+int (*checkpoint)(void *data);
 
 /*
  * Called after the checkpoint callback.
@@ -162,7 +165,7 @@ struct restore_callbacks {
  * 0: terminate checkpointing gracefully
  * 1: take another checkpoint
  */
-int (*wait_checkpoint)(void* data);
+int (*wait_checkpoint)(void *data);
 
 /*
  * callback to send store gfn and console gfn to xl
@@ -173,7 +176,7 @@ struct restore_callbacks {
 void *data);
 
 /* to be provided as the last argument to each callback function */
-void* data;
+void *data;
 };
 
 /**
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index 46fd928de2..dd9a11b4b5 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -4,7 +4,7 @@
 
 #include 
 
-static const char *dhdr_types[] =
+static const char *const dhdr_types[] =
 {
 [DHDR_TYPE_X86_PV]  = "x86 PV",
 [DHDR_TYPE_X86_HVM] = "x86 HVM",
@@ -18,7 +18,7 @@ const char 

Re: [Xen-devel] [PATCH] ARM/boot: Don't poison 'current' during early boot

[Julien Grall]

Hi,

On 15/01/2020 18:43, Andrew Cooper wrote:

This logic was inherited from x86 (which was updated several times since).
Unlike x86 (at the time) however, while NULL isn't mapped in ARM, 0xf000
is, making this actively dangerous.

Drop the logic entirely, and leave 'current' as NULL during early boot.

Signed-off-by: Andrew Cooper 


Thank you for the cleanup!

Acked-by: Julien Grall 

Cheers,


---
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
---
  xen/arch/arm/setup.c | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3c899cd4a0..9dd3738d44 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -798,9 +798,6 @@ void __init start_xen(unsigned long boot_phys_offset,
  percpu_init_areas();
  set_processor_id(0); /* needed early, for smp_processor_id() */
  
-set_current((struct vcpu *)0xf000); /* debug sanity */

-idle_vcpu[0] = current;
-
  setup_virtual_regions(NULL, NULL);
  /* Initialize traps early allow us to get backtrace when an error 
occurred */
  init_traps();



--
Julien Grall

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

[Xen-devel] [PATCH] xen/vcpu: Improve sanity checks in vcpu_create()

[Andrew Cooper]

The BUG_ON() is confusing to follow.  The (!is_idle_domain(d) || vcpu_id) part
is a vestigial remnant of architectures poisioning idle_vcpu[0] with non-NULL
pointers.

Now that idle_vcpu[0] is NULL on all architectures, and d->max_vcpus specified
before vcpu_create() is called, we can properly range check the requested
vcpu_id.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
---
 xen/common/domain.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0b1103fdb2..ee3f9ffd3e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -139,7 +139,19 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int 
vcpu_id)
 {
 struct vcpu *v;
 
-BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]);
+/*
+ * Sanity check some input expectations:
+ * - vcpu_id should be bounded by d->max_vcpus, and not previously
+ *   allocated.
+ * - VCPUs should be tightly packed and allocated in ascending order,
+ *   except for the idle domain which may vary based on PCPU numbering.
+ */
+if ( vcpu_id >= d->max_vcpus || d->vcpu[vcpu_id] ||
+ (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) )
+{
+ASSERT_UNREACHABLE();
+return NULL;
+}
 
 if ( (v = alloc_vcpu_struct(d)) == NULL )
 return NULL;
-- 
2.11.0


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

[Xen-devel] [PATCH] ARM/boot: Don't poison 'current' during early boot

[Andrew Cooper]

This logic was inherited from x86 (which was updated several times since).
Unlike x86 (at the time) however, while NULL isn't mapped in ARM, 0xf000
is, making this actively dangerous.

Drop the logic entirely, and leave 'current' as NULL during early boot.

Signed-off-by: Andrew Cooper 
---
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
---
 xen/arch/arm/setup.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3c899cd4a0..9dd3738d44 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -798,9 +798,6 @@ void __init start_xen(unsigned long boot_phys_offset,
 percpu_init_areas();
 set_processor_id(0); /* needed early, for smp_processor_id() */
 
-set_current((struct vcpu *)0xf000); /* debug sanity */
-idle_vcpu[0] = current;
-
 setup_virtual_regions(NULL, NULL);
 /* Initialize traps early allow us to get backtrace when an error occurred 
*/
 init_traps();
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v6 02/11] error: auto propagated local_err

[Greg Kurz]

On Fri, 10 Jan 2020 22:41:49 +0300
Vladimir Sementsov-Ogievskiy  wrote:

> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> 
> To achieve these goals, we need to add invocation of the macro at start
> of functions, which needs error_prepend/error_append_hint (1.); add
> invocation of the macro at start of functions which do
> local_err+error_propagate scenario the check errors, drop local errors
> from them and just use *errp instead (2., 3.).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
> 

LGTM

Reviewed-by: Greg Kurz 

> CC: Cornelia Huck 
> CC: Eric Blake 
> CC: Kevin Wolf 
> CC: Max Reitz 
> CC: Greg Kurz 
> CC: Stefan Hajnoczi 
> CC: Stefano Stabellini 
> CC: Anthony Perard 
> CC: Paul Durrant 
> CC: "Philippe Mathieu-Daudé" 
> CC: Laszlo Ersek 
> CC: Gerd Hoffmann 
> CC: Stefan Berger 
> CC: Markus Armbruster 
> CC: Michael Roth 
> CC: qemu-bl...@nongnu.org
> CC: xen-devel@lists.xenproject.org
> 
>  include/qapi/error.h | 84 +++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index fa8d51fd6d..532b9afb9e 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -78,7 +78,7 @@
>   * Call a function treating errors as fatal:
>   * foo(arg, _fatal);
>   *
> - * Receive an error and pass it on to the caller:
> + * Receive an error and pass it on to the caller (DEPRECATED*):
>   * Error *err = NULL;
>   * foo(arg, );
>   * if (err) {
> @@ -98,6 +98,50 @@
>   * foo(arg, errp);
>   * for readability.
>   *
> + * DEPRECATED* This pattern is deprecated now, use ERRP_AUTO_PROPAGATE macro
> + * instead (defined below).
> + * It's deprecated because of two things:
> + *
> + * 1. Issue with error_abort & error_propagate: when we wrap error_abort by
> + * local_err+error_propagate, resulting coredump will refer to 
> error_propagate
> + * and not to the place where error happened.
> + *
> + * 2. A lot of extra code of the same pattern
> + *
> + * How to update old code to use ERRP_AUTO_PROPAGATE?
> + *
> + * All you need is to add ERRP_AUTO_PROPAGATE() invocation at function start,
> + * than you may safely dereference errp to check errors and do not need any
> + * additional local Error variables or calls to error_propagate().
> + *
> + * Example:
> + *
> + * old code
> + *
> + * void fn(..., Error **errp) {
> + * Error *err = NULL;
> + * foo(arg, );
> + * if (err) {
> + * handle the error...
> + * error_propagate(errp, err);
> + * return;
> + * }
> + * ...
> + * }
> + *
> + * updated code
> + *
> + * void fn(..., Error **errp) {
> + * ERRP_AUTO_PROPAGATE();
> + * foo(arg, errp);
> + * if (*errp) {
> + * handle the error...
> + * return;
> + * }
> + * ...
> + * }
> + *
> + *
>   * Receive and accumulate multiple errors (first one wins):
>   * Error *err = NULL, *local_err = NULL;
>   * foo(arg, );
> @@ -348,6 +392,44 @@ void error_set_internal(Error **errp,
>  ErrorClass err_class, const char *fmt, ...)
>  GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +Error *local_err;
> +Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function which use
> + * Error **errp parameter to report error. It's needed only in cases where we
> + * want to use error_prepend, error_append_hint or dereference *errp. It's
> + * still safe (but useless) in other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see 

Re: [Xen-devel] [PATCH] get-maintainer.pl: Dont fall over when L: contains a display name

[Lars Kurth]

I should have added more people to this change. The issue without this fix is 
that entries such as

L: xen-devel 

break get_maintainer.pl and thus add_maintainers.pl

Lars

On 10/01/2020, 21:19, "Lars Kurth"  wrote:

From: Lars Kurth 

Prior to this change e-mail addresses of the form "display name
" would result into empty output. Also see
https://lists.xenproject.org/archives/html/xen-devel/2020-01/msg00753.html

Signed-off-by: Lars Kurth 
---
CC: jgr...@suse.com
---
 scripts/get_maintainer.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 2e661f47d8..48e07370e8 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -1073,7 +1073,7 @@ sub add_categories {
my $ptype = $1;
my $pvalue = $2;
if ($ptype eq "L") {
-   my $list_address = $pvalue;
+   my ($list_name, $list_address) = parse_email($pvalue);  
  
my $list_additional = "";
my $list_role = get_list_role($i);
 
-- 
2.13.0



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

Re: [Xen-devel] xen 4.13 + kernel 5.4.11 'APIC Error ... FATAL PAGE FAULT' on reboot? non-Xen reboot's ok.

[PGNet Dev]

On 1/15/20 9:21 AM, Andrew Cooper wrote:
> That is the command line for dom0 which is a VM.  You need the Xen
> hypervisor command line.

thx. done.
 
> You'll need to edit xen-4.13.0_04-lp151.688.cfg which will be somewhere
> on the ESP (wherever that is mounted in an openSUSE system).

verifying,

cat /boot/efi/EFI/opensuse/xen-4.13.0_04-lp151.688.cfg
[global]

[config.1]
options=dom0=pvh ... reboot=a
kernel=...

now, on restart,

...
[  OK  ] Reached target Shutdown.
[  137.682171] watchdog: watchdog0: watchdog did not stop!
[  139.373683] watchdog: watchdog0: watchdog did not stop!
dracut Warning: Killing all remaining processes
mdadm: stopped /dev/md4
mdadm: stopped /dev/md3
mdadm: stopped /dev/md2
mdadm: stopped /dev/md1
mdadm: stopped /dev/md0
Rebooting.
[  144.908520] reboot: Restarting system
(XEN) [2020-01-15 17:38:25] Hardware Dom0 shutdown: rebooting machine
(XEN) [2020-01-15 17:38:25] APIC error on CPU0: 40(00)
(XEN) [2020-01-15 17:38:25] Resetting with ACPI MEMORY or I/O RESET_REG.

and reboot proceeds ...

the error's still there, but without the trace/noise


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

Re: [Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[Wei Liu]

On Wed, Jan 15, 2020 at 09:25:53PM +0530, madhuparnabhowmi...@gmail.com wrote:
> From: Madhuparna Bhowmik 
> 
> list_for_each_entry_rcu has built-in RCU and lock checking.
> Pass cond argument to list_for_each_entry_rcu.
> 
> Signed-off-by: Madhuparna Bhowmik 

Acked-by: Wei Liu 

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

Re: [Xen-devel] xen 4.13 + kernel 5.4.11 'APIC Error ... FATAL PAGE FAULT' on reboot? non-Xen reboot's ok.

[Andrew Cooper]

On 15/01/2020 17:19, PGNet Dev wrote:
> hi
>
> On 1/15/20 9:10 AM, Andrew Cooper wrote:
>>> Is this a known/fixable issue?
>> The APIC errors aren't fatal.  They need looking into and addressing in
>> due course.
>>
>> The real crash is EFI firmware falling over a NULL pointer which is
>> wildly known issue.  Fixing it requires following the Linux approach
>> which is to not use EFI reboot unless absolutely necessary.
>>
>> You can work around it with reboot=a on the command line, but actually
>> fixing this in Xen is probably never going to happen because I've lost
>> interest in trying to arguing that default behaviour like the above is a
>> bad thing which we should code around.
> currently, here,
>
> cat /proc/cmdline
>   root=/dev/mapper/VG0-ROOT softlevel=xen rd.shell mds=full l1tf=flush 
> rd.debug=0 rd.udev.log_priority=debug rd.auto=1 dolvm 
> lvmwait=/dev/mapper/VG0-ROOT root=/dev/mapper/VG0-ROOT rootfstype=ext4 
> rootflags=journal_checksum noresume nomodeset nouveau.modeset=1 
> video=vesa:off video=efifb:1024x768 xencons=xvc console=tty0 console=hvc0 
> pcie_aspm=off mce=off fsck.mode=skip fsck.repair=preen reboot=acpi 
> clocksource=xen intel_iommu=on apparmor=0 plymouth.enable=0 
> scsi_mod.use_blk_mq=1 elevator=mq-deadline cpuidle cpufreq=xen:ondemand 
> net.ifnames=1 biosdevname=0 showopts noquiet log_buf_len=10M 
> print_fatal_signals=1 systemd.log_level=info systemd.log_target=kmsg 
> earlyprintk=xen,keep audit=0
>
> note the
>
>   reboot=acpi
>
> already there.
>
> something else I'm missing, perhaps?

That is the command line for dom0 which is a VM.  You need the Xen
hypervisor command line.

You'll need to edit xen-4.13.0_04-lp151.688.cfg which will be somewhere
on the ESP (wherever that is mounted in an openSUSE system).

~Andrew

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

Re: [Xen-devel] xen 4.13 + kernel 5.4.11 'APIC Error ... FATAL PAGE FAULT' on reboot? non-Xen reboot's ok.

[PGNet Dev]

hi

On 1/15/20 9:10 AM, Andrew Cooper wrote:
>> Is this a known/fixable issue?
> 
> The APIC errors aren't fatal.  They need looking into and addressing in
> due course.
> 
> The real crash is EFI firmware falling over a NULL pointer which is
> wildly known issue.  Fixing it requires following the Linux approach
> which is to not use EFI reboot unless absolutely necessary.
> 
> You can work around it with reboot=a on the command line, but actually
> fixing this in Xen is probably never going to happen because I've lost
> interest in trying to arguing that default behaviour like the above is a
> bad thing which we should code around.

currently, here,

cat /proc/cmdline
root=/dev/mapper/VG0-ROOT softlevel=xen rd.shell mds=full l1tf=flush 
rd.debug=0 rd.udev.log_priority=debug rd.auto=1 dolvm 
lvmwait=/dev/mapper/VG0-ROOT root=/dev/mapper/VG0-ROOT rootfstype=ext4 
rootflags=journal_checksum noresume nomodeset nouveau.modeset=1 video=vesa:off 
video=efifb:1024x768 xencons=xvc console=tty0 console=hvc0 pcie_aspm=off 
mce=off fsck.mode=skip fsck.repair=preen reboot=acpi clocksource=xen 
intel_iommu=on apparmor=0 plymouth.enable=0 scsi_mod.use_blk_mq=1 
elevator=mq-deadline cpuidle cpufreq=xen:ondemand net.ifnames=1 biosdevname=0 
showopts noquiet log_buf_len=10M print_fatal_signals=1 systemd.log_level=info 
systemd.log_target=kmsg earlyprintk=xen,keep audit=0

note the

reboot=acpi

already there.

something else I'm missing, perhaps?



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

[Xen-devel] [xen-unstable test] 146094: regressions - FAIL

[osstest service owner]

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

Regressions :-(

Tests which did not succeed and are blocking,
including tests which could not be run:
 test-amd64-i386-qemuu-rhel6hvm-intel 11 guest-stop   fail REGR. vs. 146058

Tests which did not succeed, but are not blocking:
 test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10  fail blocked in 146058
 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 146058
 test-armhf-armhf-libvirt 14 saverestore-support-checkfail  like 146058
 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 146058
 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 146058
 test-amd64-amd64-qemuu-nested-intel 17 debian-hvm-install/l1/l2 fail like 
146058
 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail  like 146058
 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 146058
 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 146058
 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 146058
 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 146058
 test-amd64-i386-xl-pvshim12 guest-start  fail   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 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-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-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2  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-credit1  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl-credit2  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-credit1  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail   never pass
 test-arm64-arm64-xl  13 migrate-support-checkfail   never pass
 test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail   never pass
 test-arm64-arm64-xl  14 saverestore-support-checkfail   never pass
 test-arm64-arm64-libvirt-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-cubietruck 13 migrate-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 14 saverestore-support-checkfail never pass
 test-armhf-armhf-libvirt 13 migrate-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  13 migrate-support-checkfail   never pass
 test-armhf-armhf-xl  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-raw 12 migrate-support-checkfail   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-i386-xl-qemut-ws16-amd64 17 guest-stop  fail never pass

version targeted for testing:
 xen  b4194711ffaffa5e63d986338fb8d4020fa6bad1
baseline version:
 xen  03bfe526ecadc86f31eda433b91dc90be0563919

Last test of basis   146058  2020-01-14 01:51:38 Z1 days
Testing same since   146094  2020-01-14 21:36:19 Z0 days1 attempts


People who touched 

Re: [Xen-devel] [XEN PATCH] linkfarm: Exclude .*.tmp

[Ian Jackson]

Anthony PERARD writes ("[XEN PATCH] linkfarm: Exclude .*.tmp"):
> Exclude intermidiate files .*.tmp from the linkfarm, those are
> generated by %.o:%.c rules in xen/Rules.mk when
> CONFIG_ENFORCE_UNIQUE_SYMBOLS=y.
> 
> Signed-off-by: Anthony PERARD 

Acked-by: Ian Jackson 

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

Re: [Xen-devel] xen 4.13 + kernel 5.4.11 'APIC Error ... FATAL PAGE FAULT' on reboot? non-Xen reboot's ok.

[Andrew Cooper]

On 15/01/2020 16:52, PGNet Dev wrote:
> dev @distro suggested I post this here ...
>
> I've a recently upgraded Xen & Kernel on
>
>   lsb_release -rd
>   Description:openSUSE Leap 15.1
>   Release:15.1
>
> Atm, I'm running
>
>   Xen 4.13.0_04
>
> server, on EFI hardware + Intel Xeon E3 CPU, with kernel 
>
>   5.4.11-24.g2d02eb4-default
>
> It boots as always, with no issue
>
>   Welcome to GRUB!
>
>   Please press t to show the boot menu on this console
>   Xen 4.13.0_04-lp151.688 (c/s ) EFI loader
>   Using configuration file 'xen-4.13.0_04-lp151.688.cfg'
>   vmlinuz-5.4.11-24.g2d02eb4-default: 
> 0x8b7c-0x8c04efb8
>   initrd-5.4.11-24.g2d02eb4-default: 0x8a4a5000-0x8b7bfe28
>   0x:0x00:0x19.0x0: ROM: 0x1 bytes at 0x928a9018
>   0x:0x04:0x00.0x0: ROM: 0x8000 bytes at 0x928a0018
>   0x:0x10:0x00.0x0: ROM: 0x10800 bytes at 0x92885018
>__  __  
>\ \/ /___ _ __  
> \  // _ \ '_ \ 
> /  \  __/ | | |
>/_/\_\___|_| |_|
>
>_  __ _  ___ ___  _  _  _   _   _   _____  
>  ___  
>   | || |  / |___ / / _ \   / _ \| || || |_ __ / | ___|/ | / /_  ( _ ) 
> ( _ ) 
>   | || |_ | | |_ \| | | | | | | | || |_ __| | '_ \| |___ \| || '_ \ / _ \ 
> / _ \ 
>   |__   _|| |___) | |_| | | |_| |__   _|__| | |_) | |___) | || (_) | (_) 
> | (_) |
>  |_|(_)_|(_)___/___\___/   |_||_| .__/|_|/|_(_)___/ \___/ 
> \___/ 
>   |_|   |_|   
>   
>   (XEN) [0026c8dc8909] Xen version 4.13.0_04-lp151.688 
> (abu...@suse.de) (gcc (SUSE Linux) 9.2.1 20200109 [gcc-9-branch revi
>   sion 280039]) debug=n  Wed Jan  8 11:43:04 UTC 2020
>   (XEN) [0026cbd609dc] Latest ChangeSet: 
>   (XEN) [0026cc9505ea] Bootloader: EFI
>   (XEN) [0026cd46f20f] Command line: dom0=pvh dom0-iommu=map-reserved 
> dom0_mem=4016M,max:4096M bootscrub=false dom0_max_vcp
>   us=4 vga=gfx-1920x1080x16 com1=115200,8n1,pci console=com1,vga 
> console_timestamps console_to_ring conring_size=64 sched=credit2 ucode=scan 
> log_buf_len=16M loglvl=warning guest_loglvl=none/warning noreboot=false 
> iommu=verbose sync_console=false
>   ...
>
> on exec of cmdline shutdown from shell,
>
>   shutdown -r now
>
> the system DOES reboot, but first throws an APIC error -- only if running 
> Xen, reboot with no-hypervisor has not probs
>
> 1st step, here's the current, relevant _log_ trace
>
>   ...
>   [  OK  ] Reached target Shutdown.
>   [  343.932856] watchdog: watchdog0: watchdog did not stop!
>   [  346.871303] watchdog: watchdog0: watchdog did not stop!
>   dracut Warning: Killing all remaining processes
>   mdadm: stopped /dev/md4
>   mdadm: stopped /dev/md3
>   mdadm: stopped /dev/md2
>   mdadm: stopped /dev/md1
>   mdadm: stopped /dev/md0
>   Rebooting.
>   [  352.396918] reboot: Restarting system
>   (XEN) [2020-01-15 15:01:26] Hardware Dom0 shutdown: rebooting machine
>   (XEN) [2020-01-15 15:01:26] APIC error on CPU0: 40(00)
>   (XEN) [2020-01-15 15:01:26] [ Xen-4.13.0_04-lp151.688  x86_64  
> debug=n   Not tainted ]
>   (XEN) [2020-01-15 15:01:26] CPU:0
>   (XEN) [2020-01-15 15:01:26] RIP:e008:[<>] 
> 
>   (XEN) [2020-01-15 15:01:26] RFLAGS: 00010202   CONTEXT: 
> hypervisor
>   (XEN) [2020-01-15 15:01:26] rax: 0286   rbx: 
>    rcx: 
>   (XEN) [2020-01-15 15:01:26] rdx: 9e5ca7a0   rsi: 
>    rdi: 
>   (XEN) [2020-01-15 15:01:26] rbp:    rsp: 
> 83008ca2fa48   r8:  83008ca2fa90
>   (XEN) [2020-01-15 15:01:26] r9:  83008ca2fa80   r10: 
>    r11: 
>   (XEN) [2020-01-15 15:01:26] r12:    r13: 
> 83008ca2fb00   r14: 83008ca2
>   (XEN) [2020-01-15 15:01:26] r15:    cr0: 
> 80050033   cr4: 001526e0
>   (XEN) [2020-01-15 15:01:26] cr3: 0008492ed000   cr2: 
> eef3f286
>   (XEN) [2020-01-15 15:01:26] fsb:    gsb: 
>    gss: 
>   (XEN) [2020-01-15 15:01:26] ds:    es:    fs:    gs:    
> ss:    cs: e008
>   (XEN) [2020-01-15 15:01:26] Xen code around <> 
> () [fault on access]:
>   (XEN) [2020-01-15 15:01:26]  -- -- -- -- -- -- -- -- <00> 80 00 f0 f3 
> ee 00 f0 c3 e2 00 f0 f3 ee 00 f0
>   (XEN) [2020-01-15 15:01:26] Xen stack trace from rsp=83008ca2fa48:
>   (XEN) [2020-01-15 15:01:26]9e5ca3c9 82d08036681f 
> 82d08036682b 
>   (XEN) [2020-01-15 15:01:26]

Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice

[George Dunlap]

On 1/15/20 12:22 PM, Lars Kurth wrote:
> @George, @Ian: let me know whether this is better and addresses your
> concerns

This looks good to me.

One side note:

>  The way how a reviewer expresses feedback, has a big impact on how the author

"The way how X happens" isn't grammatically correct; it's actually
redundant.  You can say, "The way a reviewer expresses feedback" (no
"how"); or if that seems ambiguous for some reason, you can say, "The
way *that* a reviewer expresses feedback", or "The way *in which* a
reviewer expresses feedback...".

Alternately, you could say "How a reviewer expresses feedback has a big
impact..."

 -George

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

[Xen-devel] [XEN PATCH v3 5/6] xen: Use $(CONFIG_CC_IS_CLANG) instead of $(clang) in Makefile

[Anthony PERARD]

Kconfig can check if $(CC) is clang or not, if it is
CONFIG_CC_IS_CLANG will be set.

With that patch, the hypervisor can be built using clang by running
`make CC=clang CXX=clang++` without needed to provide an extra clang
parameter.

`make clang=y` still works as Config.mk will set CC and CXX.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---
 xen/Rules.mk | 8 
 xen/arch/x86/Rules.mk| 2 +-
 xen/common/coverage/Makefile | 2 +-
 xen/include/Makefile | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index d053dbd26526..fcdafd029342 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -64,7 +64,7 @@ CFLAGS += -pipe -D__XEN__ -include 
$(BASEDIR)/include/xen/config.h
 CFLAGS-$(CONFIG_DEBUG_INFO) += -g
 CFLAGS += '-D__OBJECT_FILE__="$@"'
 
-ifneq ($(clang),y)
+ifneq ($(CONFIG_CC_IS_CLANG),y)
 # Clang doesn't understand this command line argument, and doesn't appear to
 # have an suitable alternative.  The resulting compiled binary does function,
 # but has an excessively large symbol table.
@@ -126,7 +126,7 @@ subdir-all := $(subdir-y) $(subdir-n)
 $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): CFLAGS += 
-DINIT_SECTIONS_ONLY
 
 ifeq ($(CONFIG_COVERAGE),y)
-ifeq ($(clang),y)
+ifeq ($(CONFIG_CC_IS_CLANG),y)
 COV_FLAGS := -fprofile-instr-generate -fcoverage-mapping
 else
 COV_FLAGS := -fprofile-arcs -ftest-coverage
@@ -143,7 +143,7 @@ endif
 
 ifeq ($(CONFIG_LTO),y)
 CFLAGS += -flto
-LDFLAGS-$(clang) += -plugin LLVMgold.so
+LDFLAGS-$(CONFIG_CC_IS_CLANG) += -plugin LLVMgold.so
 # Would like to handle all object files as bitcode, but objects made from
 # pure asm are in a different format and have to be collected separately.
 # Mirror the directory tree, collecting them as built_in_bin.o.
@@ -197,7 +197,7 @@ SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR))
 %.o: %.c Makefile
 ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
$(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp
-ifeq ($(clang),y)
+ifeq ($(CONFIG_CC_IS_CLANG),y)
$(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
 else
$(OBJCOPY) --redefine-sym $(https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [XEN PATCH v3 0/6] xen: Kconfig update with few extra

[Anthony PERARD]

Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git 
br.build-system-xen-kconfig-v3

v3:
- change in patch 2. gcc-version.sh now behave like clang-version.sh.
otherwise, the series should be ready.

v2:
- nit changes in patch 1 and 2.

Cheers,

Anthony PERARD (6):
  xen: Update Kconfig to Linux v5.4
  xen: Have Kconfig check $(CC)'s version
  xen: Import cc-ifversion from Kbuild
  xen: Move CONFIG_INDIRECT_THUNK to Kconfig
  xen: Use $(CONFIG_CC_IS_CLANG) instead of $(clang) in Makefile
  xen: Move GCC_HAS_VISIBILITY_ATTRIBUTE to Kconfig and common

 Config.mk |5 -
 docs/misc/kconfig-language.rst|  701 +
 docs/misc/kconfig-language.txt|  395 ---
 docs/misc/kconfig-macro-language.rst  |  247 ++
 docs/misc/{kconfig.txt => kconfig.rst}|  185 +-
 xen/Kconfig   |   34 +-
 xen/Makefile  |6 +-
 xen/Rules.mk  |9 +-
 xen/arch/arm/Kconfig  |2 +-
 xen/arch/arm/Rules.mk |4 -
 xen/arch/x86/Kconfig  |3 +
 xen/arch/x86/Rules.mk |   11 +-
 xen/common/Kconfig|   12 +-
 xen/common/coverage/Makefile  |   10 +-
 xen/include/Makefile  |2 +-
 xen/include/xen/compiler.h|2 +-
 xen/scripts/Kbuild.include|7 +
 xen/scripts/Kconfig.include   |   39 +
 xen/scripts/clang-version.sh  |   19 +
 xen/scripts/gcc-version.sh|   25 +
 xen/tools/kconfig/.gitignore  |3 +-
 xen/tools/kconfig/Makefile|  268 +-
 xen/tools/kconfig/Makefile.host   |  121 +-
 xen/tools/kconfig/Makefile.kconfig|   52 +-
 xen/tools/kconfig/conf.c  |  191 +-
 xen/tools/kconfig/confdata.c  |  491 ++--
 xen/tools/kconfig/expr.c  |  213 +-
 xen/tools/kconfig/expr.h  |  108 +-
 xen/tools/kconfig/gconf-cfg.sh|   30 +
 xen/tools/kconfig/gconf.c |   39 +-
 xen/tools/kconfig/images.c|   34 +-
 xen/tools/kconfig/images.h|   33 +
 xen/tools/kconfig/lexer.l |  471 +++
 xen/tools/kconfig/list.h  |1 +
 xen/tools/kconfig/lkc.h   |   38 +-
 xen/tools/kconfig/lkc_proto.h |   21 +-
 xen/tools/kconfig/lxdialog/.gitignore |4 -
 xen/tools/kconfig/lxdialog/BIG.FAT.WARNING|2 +-
 xen/tools/kconfig/lxdialog/check-lxdialog.sh  |   91 -
 xen/tools/kconfig/lxdialog/checklist.c|   15 +-
 xen/tools/kconfig/lxdialog/dialog.h   |   17 +-
 xen/tools/kconfig/lxdialog/inputbox.c |   18 +-
 xen/tools/kconfig/lxdialog/menubox.c  |   15 +-
 xen/tools/kconfig/lxdialog/textbox.c  |   15 +-
 xen/tools/kconfig/lxdialog/util.c |   15 +-
 xen/tools/kconfig/lxdialog/yesno.c|   15 +-
 xen/tools/kconfig/mconf-cfg.sh|   47 +
 xen/tools/kconfig/mconf.c |   27 +-
 xen/tools/kconfig/menu.c  |  288 +-
 xen/tools/kconfig/merge_config.sh |   87 +-
 xen/tools/kconfig/nconf-cfg.sh|   47 +
 xen/tools/kconfig/nconf.c |   42 +-
 xen/tools/kconfig/nconf.gui.c |   30 +-
 xen/tools/kconfig/nconf.h |9 +-
 xen/tools/kconfig/{zconf.y => parser.y}   |  409 ++-
 xen/tools/kconfig/preprocess.c|  574 
 xen/tools/kconfig/qconf-cfg.sh|   32 +
 xen/tools/kconfig/qconf.cc|  750 +++--
 xen/tools/kconfig/qconf.h |  153 +-
 xen/tools/kconfig/streamline_config.pl|   53 +-
 xen/tools/kconfig/symbol.c|  295 +-
 xen/tools/kconfig/tests/auto_submenu/Kconfig  |   52 +
 .../kconfig/tests/auto_submenu/__init__.py|   13 +
 .../tests/auto_submenu/expected_stdout|   10 +
 xen/tools/kconfig/tests/choice/Kconfig|   56 +
 xen/tools/kconfig/tests/choice/__init__.py|   41 +
 .../tests/choice/alldef_expected_config   |5 +
 .../tests/choice/allmod_expected_config   |9 +
 .../tests/choice/allno_expected_config|5 +
 .../tests/choice/allyes_expected_config   |9 +
 .../tests/choice/oldask0_expected_stdout  |   10 +
 xen/tools/kconfig/tests/choice/oldask1_config |2 +
 .../tests/choice/oldask1_expected_stdout  |   15 +
 .../tests/choice_value_with_m_dep/Kconfig |   21 +
 .../tests/choice_value_with_m_dep/__init__.py |   16 +
 .../tests/choice_value_with_m_dep/config  |2 +
 .../choice_value_with_m_dep/expected_config   |3 +
 

[Xen-devel] [XEN PATCH v3 6/6] xen: Move GCC_HAS_VISIBILITY_ATTRIBUTE to Kconfig and common

[Anthony PERARD]

The check for $(CC) -fvisibility=hidden is done by both arm and x86,
so the patch also move the check to the common area.

The check doesn't check if $(CC) is gcc, and clang can accept that
option as well, so s/GCC/CC/ is done to the define name.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---
 xen/Kconfig| 4 
 xen/arch/arm/Rules.mk  | 4 
 xen/arch/x86/Rules.mk  | 5 -
 xen/include/xen/compiler.h | 2 +-
 4 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 57427927abf0..073042f46730 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -23,6 +23,10 @@ config CLANG_VERSION
int
default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
 
+# -fvisibility=hidden reduces -fpic cost, if it's available
+config CC_HAS_VISIBILITY_ATTRIBUTE
+   def_bool $(cc-option,-fvisibility=hidden)
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index 3d9a0ed357bc..022a3a6f82ba 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -18,10 +18,6 @@ CFLAGS-$(CONFIG_ARM_32) += -mcpu=cortex-a15
 CFLAGS-$(CONFIG_ARM_64) += -mcpu=generic
 CFLAGS-$(CONFIG_ARM_64) += -mgeneral-regs-only # No fp registers etc
 
-ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
-CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
-endif
-
 EARLY_PRINTK := n
 
 ifeq ($(CONFIG_DEBUG),y)
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index b98e14e28c5a..e69b8e697cc0 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -65,11 +65,6 @@ CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 # SSE setup for variadic function calls.
 CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
 
-# -fvisibility=hidden reduces -fpic cost, if it's available
-ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
-CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
-endif
-
 # Compile with thunk-extern, indirect-branch-register if avaiable.
 ifeq ($(CONFIG_INDIRECT_THUNK),y)
 CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index ff6c0f5cdd18..8c846261d241 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -78,7 +78,7 @@
 #define __must_be_array(a) \
   BUILD_BUG_ON_ZERO(__builtin_types_compatible_p(typeof(a), typeof([0])))
 
-#ifdef GCC_HAS_VISIBILITY_ATTRIBUTE
+#ifdef CONFIG_CC_HAS_VISIBILITY_ATTRIBUTE
 /* Results in more efficient PIC code (no indirections through GOT or PLT). */
 #pragma GCC visibility push(hidden)
 #endif
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH v3 2/6] xen: Have Kconfig check $(CC)'s version

[Anthony PERARD]

This import several files from Linux v5.3
 - scripts/Kconfig.include
 - scripts/clang-version.sh
 - scripts/gcc-version.sh
 and several config values from from Linux's init/Kconfig file.
But gcc-version.sh have been modified to return "0" when $CC isn't
GCC, like clang-version.sh do.

Files are copied into scripts/ directory because that's were the files
are found in Linux tree, and also because we are going to import more
of Kbuild from Linux which is located in scripts/.

CONFIG_GCC_VERSION and CONFIG_CC_IS_CLANG are going to be use in
follow-up patches.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---

Notes:
v3:
- Have gcc-version behave like clang-version and return 0 when the
  compiler tested isn't the expected one.

v2:
- move the export CC* earlier in xen/Makefile, and add CXX to the list.

 xen/Kconfig  | 16 +++
 xen/Makefile |  2 ++
 xen/scripts/Kconfig.include  | 39 
 xen/scripts/clang-version.sh | 19 ++
 xen/scripts/gcc-version.sh   | 25 +++
 5 files changed, 101 insertions(+)
 create mode 100644 xen/scripts/Kconfig.include
 create mode 100755 xen/scripts/clang-version.sh
 create mode 100755 xen/scripts/gcc-version.sh

diff --git a/xen/Kconfig b/xen/Kconfig
index 01067326b4e7..57427927abf0 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -4,9 +4,25 @@
 #
 mainmenu "Xen/$(SRCARCH) $(XEN_FULLVERSION) Configuration"
 
+source "scripts/Kconfig.include"
+
 config BROKEN
bool
 
+config CC_IS_GCC
+   def_bool $(success,$(CC) --version | head -n 1 | grep -q gcc)
+
+config GCC_VERSION
+   int
+   default $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
+
+config CC_IS_CLANG
+   def_bool $(success,$(CC) --version | head -n 1 | grep -q clang)
+
+config CLANG_VERSION
+   int
+   default $(shell,$(BASEDIR)/scripts/clang-version.sh $(CC))
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index efbe9605e52b..c326fee5880e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -18,6 +18,8 @@ export XEN_CONFIG_EXPERT ?= n
 PYTHON_INTERPRETER := $(word 1,$(shell which python3 python python2 
2>/dev/null) python)
 export PYTHON  ?= $(PYTHON_INTERPRETER)
 
+export CC CXX LD
+
 export BASEDIR := $(CURDIR)
 export XEN_ROOT := $(BASEDIR)/..
 
diff --git a/xen/scripts/Kconfig.include b/xen/scripts/Kconfig.include
new file mode 100644
index ..8221095ca34b
--- /dev/null
+++ b/xen/scripts/Kconfig.include
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# Kconfig helper macros
+
+# Convenient variables
+comma   := ,
+quote   := "
+squote  := '
+empty   :=
+space   := $(empty) $(empty)
+dollar  := $
+right_paren := )
+left_paren  := (
+
+# $(if-success,,,)
+# Return  if  exits with 0,  otherwise.
+if-success = $(shell,{ $(1); } >/dev/null 2>&1 && echo "$(2)" || echo "$(3)")
+
+# $(success,)
+# Return y if  exits with 0, n otherwise
+success = $(if-success,$(1),y,n)
+
+# $(failure,)
+# Return n if  exits with 0, y otherwise
+failure = $(if-success,$(1),n,y)
+
+# $(cc-option,)
+# Return y if the compiler supports , n otherwise
+cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o 
/dev/null)
+
+# $(ld-option,)
+# Return y if the linker supports , n otherwise
+ld-option = $(success,$(LD) -v $(1))
+
+# check if $(CC) and $(LD) exist
+$(error-if,$(failure,command -v $(CC)),compiler '$(CC)' not found)
+$(error-if,$(failure,command -v $(LD)),linker '$(LD)' not found)
+
+# gcc version including patch level
+gcc-version := $(shell,$(BASEDIR)/scripts/gcc-version.sh $(CC))
diff --git a/xen/scripts/clang-version.sh b/xen/scripts/clang-version.sh
new file mode 100755
index ..6fabf0695761
--- /dev/null
+++ b/xen/scripts/clang-version.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# clang-version clang-command
+#
+# Print the compiler version of `clang-command' in a 5 or 6-digit form
+# such as `50001' for clang-5.0.1 etc.
+
+compiler="$*"
+
+if ! ( $compiler --version | grep -q clang) ; then
+   echo 0
+   exit 1
+fi
+
+MAJOR=$(echo __clang_major__ | $compiler -E -x c - | tail -n 1)
+MINOR=$(echo __clang_minor__ | $compiler -E -x c - | tail -n 1)
+PATCHLEVEL=$(echo __clang_patchlevel__ | $compiler -E -x c - | tail -n 1)
+printf "%d%02d%02d\\n" $MAJOR $MINOR $PATCHLEVEL
diff --git a/xen/scripts/gcc-version.sh b/xen/scripts/gcc-version.sh
new file mode 100755
index ..b3261949dea5
--- /dev/null
+++ b/xen/scripts/gcc-version.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# gcc-version gcc-command
+#
+# Print the gcc version of `gcc-command' in a 5 or 6-digit form
+# such as `29503' for gcc-2.95.3, `30301' for gcc-3.3.1, etc.
+
+compiler="$*"
+
+if [ ${#compiler} -eq 0 ]; then
+   echo "Error: No compiler specified." >&2
+   printf "Usage:\n\t$0 \n" 

[Xen-devel] [XEN PATCH v3 3/6] xen: Import cc-ifversion from Kbuild

[Anthony PERARD]

This is in preparation of importing Kbuild to build Xen. We won't be
able to include Config.mk so we will need a replacement for the macro
`cc-ifversion'.

This patch imports parts of "scripts/Kbuild.include" from Linux v5.4,
the macro cc-ifversion. It makes use of CONFIG_GCC_VERSION that
Kconfig now provides.

Since they are no other use of Xen's `cc-ifversion' macro, we can
remove it.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---
 Config.mk| 5 -
 xen/Rules.mk | 1 +
 xen/common/coverage/Makefile | 8 
 xen/scripts/Kbuild.include   | 7 +++
 4 files changed, 12 insertions(+), 9 deletions(-)
 create mode 100644 xen/scripts/Kbuild.include

diff --git a/Config.mk b/Config.mk
index 35d66e5e121a..65649d6122d1 100644
--- a/Config.mk
+++ b/Config.mk
@@ -121,11 +121,6 @@ define cc-ver-check-closure
 endif
 endef
 
-# cc-ifversion: Check compiler version and take branch accordingly
-# Usage $(call cc-ifversion,lt,0x040700,string_if_y,string_if_n)
-cc-ifversion = $(shell [ $(call cc-ver,$(CC),$(1),$(2)) = "y" ] \
-   && echo $(3) || echo $(4))
-
 # Require GCC v4.1+
 check-$(gcc) = $(call cc-ver-check,CC,0x040100,"Xen requires at least gcc-4.1")
 $(eval $(check-y))
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 5aba841b0a95..d053dbd26526 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -2,6 +2,7 @@
 -include $(BASEDIR)/include/config/auto.conf
 
 include $(XEN_ROOT)/Config.mk
+include $(BASEDIR)/scripts/Kbuild.include
 
 
 ifneq ($(origin crash_debug),undefined)
diff --git a/xen/common/coverage/Makefile b/xen/common/coverage/Makefile
index 46c78d1086d6..b509e51f960b 100644
--- a/xen/common/coverage/Makefile
+++ b/xen/common/coverage/Makefile
@@ -1,10 +1,10 @@
 obj-y += coverage.o
 ifneq ($(clang),y)
 obj-y += gcov_base.o gcov.o
-obj-y += $(call cc-ifversion,lt,0x040700, \
-   gcc_3_4.o, $(call cc-ifversion,lt,0x040900, \
-   gcc_4_7.o, $(call cc-ifversion,lt,0x05, \
-   gcc_4_9.o, $(call cc-ifversion,lt,0x07, \
+obj-y += $(call cc-ifversion,-lt,0407, \
+   gcc_3_4.o, $(call cc-ifversion,-lt,0409, \
+   gcc_4_7.o, $(call cc-ifversion,-lt,0500, \
+   gcc_4_9.o, $(call cc-ifversion,-lt,0700, \
gcc_5.o, gcc_7.o
 else
 obj-y += llvm.o
diff --git a/xen/scripts/Kbuild.include b/xen/scripts/Kbuild.include
new file mode 100644
index ..a5c462fd9777
--- /dev/null
+++ b/xen/scripts/Kbuild.include
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+# kbuild: Generic definitions
+
+# cc-ifversion
+# Usage:  EXTRA_CFLAGS += $(call cc-ifversion, -lt, 0402, -O1)
+cc-ifversion = $(shell [ $(CONFIG_GCC_VERSION)0 $(1) $(2)000 ] && echo $(3) || 
echo $(4))
-- 
Anthony PERARD


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

[Xen-devel] [XEN PATCH v3 4/6] xen: Move CONFIG_INDIRECT_THUNK to Kconfig

[Anthony PERARD]

Now that Kconfig has the capability to run shell command when
generating CONFIG_* we can use it in some cases to test CFLAGS.

CONFIG_INDIRECT_THUNK is a good example that wants to exist both in
Makefile and as a C macro, which Kconfig do. So use Kconfig to
generate CONFIG_INDIRECT_THUNK and have the CFLAGS depends on that.

Signed-off-by: Anthony PERARD 
Acked-by: Andrew Cooper 
---
 xen/arch/x86/Kconfig  | 3 +++
 xen/arch/x86/Rules.mk | 4 +---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index f853c0456404..8149362bdef3 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -31,6 +31,9 @@ config ARCH_DEFCONFIG
string
default "arch/x86/configs/x86_64_defconfig"
 
+config INDIRECT_THUNK
+   def_bool $(cc-option,-mindirect-branch-register)
+
 menu "Architecture Features"
 
 source "arch/Kconfig"
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 92fdbe9d6822..a2c257fb95b2 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -71,11 +71,9 @@ CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
 endif
 
 # Compile with thunk-extern, indirect-branch-register if avaiable.
-ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)
+ifeq ($(CONFIG_INDIRECT_THUNK),y)
 CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
-CFLAGS += -DCONFIG_INDIRECT_THUNK
 CFLAGS += -fno-jump-tables
-export CONFIG_INDIRECT_THUNK=y
 endif
 
 # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
-- 
Anthony PERARD


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

[Xen-devel] xen 4.13 + kernel 5.4.11 'APIC Error ... FATAL PAGE FAULT' on reboot? non-Xen reboot's ok.

[PGNet Dev]

dev @distro suggested I post this here ...

I've a recently upgraded Xen & Kernel on

lsb_release -rd
Description:openSUSE Leap 15.1
Release:15.1

Atm, I'm running

Xen 4.13.0_04

server, on EFI hardware + Intel Xeon E3 CPU, with kernel 

5.4.11-24.g2d02eb4-default

It boots as always, with no issue

Welcome to GRUB!

Please press t to show the boot menu on this console
Xen 4.13.0_04-lp151.688 (c/s ) EFI loader
Using configuration file 'xen-4.13.0_04-lp151.688.cfg'
vmlinuz-5.4.11-24.g2d02eb4-default: 
0x8b7c-0x8c04efb8
initrd-5.4.11-24.g2d02eb4-default: 0x8a4a5000-0x8b7bfe28
0x:0x00:0x19.0x0: ROM: 0x1 bytes at 0x928a9018
0x:0x04:0x00.0x0: ROM: 0x8000 bytes at 0x928a0018
0x:0x10:0x00.0x0: ROM: 0x10800 bytes at 0x92885018
 __  __  
 \ \/ /___ _ __  
  \  // _ \ '_ \ 
  /  \  __/ | | |
 /_/\_\___|_| |_|
 
 _  __ _  ___ ___  _  _  _   _   _   _____  
 ___  
| || |  / |___ / / _ \   / _ \| || || |_ __ / | ___|/ | / /_  ( _ ) 
( _ ) 
| || |_ | | |_ \| | | | | | | | || |_ __| | '_ \| |___ \| || '_ \ / _ \ 
/ _ \ 
|__   _|| |___) | |_| | | |_| |__   _|__| | |_) | |___) | || (_) | (_) 
| (_) |
   |_|(_)_|(_)___/___\___/   |_||_| .__/|_|/|_(_)___/ \___/ 
\___/ 
|_|   |_|   
  
(XEN) [0026c8dc8909] Xen version 4.13.0_04-lp151.688 
(abu...@suse.de) (gcc (SUSE Linux) 9.2.1 20200109 [gcc-9-branch revi
sion 280039]) debug=n  Wed Jan  8 11:43:04 UTC 2020
(XEN) [0026cbd609dc] Latest ChangeSet: 
(XEN) [0026cc9505ea] Bootloader: EFI
(XEN) [0026cd46f20f] Command line: dom0=pvh dom0-iommu=map-reserved 
dom0_mem=4016M,max:4096M bootscrub=false dom0_max_vcp
us=4 vga=gfx-1920x1080x16 com1=115200,8n1,pci console=com1,vga 
console_timestamps console_to_ring conring_size=64 sched=credit2 ucode=scan 
log_buf_len=16M loglvl=warning guest_loglvl=none/warning noreboot=false 
iommu=verbose sync_console=false
...

on exec of cmdline shutdown from shell,

shutdown -r now

the system DOES reboot, but first throws an APIC error -- only if running Xen, 
reboot with no-hypervisor has not probs

1st step, here's the current, relevant _log_ trace

...
[  OK  ] Reached target Shutdown.
[  343.932856] watchdog: watchdog0: watchdog did not stop!
[  346.871303] watchdog: watchdog0: watchdog did not stop!
dracut Warning: Killing all remaining processes
mdadm: stopped /dev/md4
mdadm: stopped /dev/md3
mdadm: stopped /dev/md2
mdadm: stopped /dev/md1
mdadm: stopped /dev/md0
Rebooting.
[  352.396918] reboot: Restarting system
(XEN) [2020-01-15 15:01:26] Hardware Dom0 shutdown: rebooting machine
(XEN) [2020-01-15 15:01:26] APIC error on CPU0: 40(00)
(XEN) [2020-01-15 15:01:26] [ Xen-4.13.0_04-lp151.688  x86_64  
debug=n   Not tainted ]
(XEN) [2020-01-15 15:01:26] CPU:0
(XEN) [2020-01-15 15:01:26] RIP:e008:[<>] 

(XEN) [2020-01-15 15:01:26] RFLAGS: 00010202   CONTEXT: 
hypervisor
(XEN) [2020-01-15 15:01:26] rax: 0286   rbx: 
   rcx: 
(XEN) [2020-01-15 15:01:26] rdx: 9e5ca7a0   rsi: 
   rdi: 
(XEN) [2020-01-15 15:01:26] rbp:    rsp: 
83008ca2fa48   r8:  83008ca2fa90
(XEN) [2020-01-15 15:01:26] r9:  83008ca2fa80   r10: 
   r11: 
(XEN) [2020-01-15 15:01:26] r12:    r13: 
83008ca2fb00   r14: 83008ca2
(XEN) [2020-01-15 15:01:26] r15:    cr0: 
80050033   cr4: 001526e0
(XEN) [2020-01-15 15:01:26] cr3: 0008492ed000   cr2: 
eef3f286
(XEN) [2020-01-15 15:01:26] fsb:    gsb: 
   gss: 
(XEN) [2020-01-15 15:01:26] ds:    es:    fs:    gs:    
ss:    cs: e008
(XEN) [2020-01-15 15:01:26] Xen code around <> 
() [fault on access]:
(XEN) [2020-01-15 15:01:26]  -- -- -- -- -- -- -- -- <00> 80 00 f0 f3 
ee 00 f0 c3 e2 00 f0 f3 ee 00 f0
(XEN) [2020-01-15 15:01:26] Xen stack trace from rsp=83008ca2fa48:
(XEN) [2020-01-15 15:01:26]9e5ca3c9 82d08036681f 
82d08036682b 
(XEN) [2020-01-15 15:01:26] 83008ca2fa88 
 001526e0
(XEN) [2020-01-15 15:01:26]82d0802758cd 

Re: [Xen-devel] [PATCH v4] xen-pciback: optionally allow interrupt enable flag writes

[Roger Pau Monné]

On Wed, Jan 15, 2020 at 02:46:29AM +0100, Marek Marczykowski-Górecki wrote:
> QEMU running in a stubdom needs to be able to set INTX_DISABLE, and the
> MSI(-X) enable flags in the PCI config space. This adds an attribute
> 'allow_interrupt_control' which when set for a PCI device allows writes
> to this flag(s). The toolstack will need to set this for stubdoms.
> When enabled, guest (stubdomain) will be allowed to set relevant enable
> flags, but only one at a time - i.e. it refuses to enable more than one
> of INTx, MSI, MSI-X at a time.
> 
> This functionality is needed only for config space access done by device
> model (stubdomain) serving a HVM with the actual PCI device. It is not
> necessary and unsafe to enable direct access to those bits for PV domain
> with the device attached. For PV domains, there are separate protocol
> messages (XEN_PCI_OP_{enable,disable}_{msi,msix}) for this purpose.
> Those ops in addition to setting enable bits, also configure MSI(-X) in
> dom0 kernel - which is undesirable for PCI passthrough to HVM guests.
> 
> This should not introduce any new security issues since a malicious
> guest (or stubdom) can already generate MSIs through other ways, see
> [1] page 8. Additionally, when qemu runs in dom0, it already have direct
> access to those bits.
> 
> This is the second iteration of this feature. First was proposed as a
> direct Xen interface through a new hypercall, but ultimately it was
> rejected by the maintainer, because of mixing pciback and hypercalls for
> PCI config space access isn't a good design. Full discussion at [2].
> 
> [1]: 
> https://invisiblethingslab.com/resources/2011/Software%20Attacks%20on%20Intel%20VT-d.pdf
> [2]: https://xen.markmail.org/thread/smpgpws4umdzizze
> 
> [part of the commit message and sysfs handling]
> Signed-off-by: Simon Gaiser 
> [the rest]
> Signed-off-by: Marek Marczykowski-Górecki 

Some minor nits below, but LGTM:

Reviewed-by: Roger Pau Monné 

> ---
> Changes in v4:
>  - fix incorrect variable used
>  - don't enable INTx when already enabled
> Changes in v3:
>  - return bitmap (or negative error) from
>xen_pcibk_get_interrupt_type(), to implicitly handle cases when
>multiple interrupt types are already enabled - disallow enabling in
>that case
>  - add documentation
> Changes in v2:
>  - introduce xen_pcibk_get_interrupt_type() to deduplicate current
>INTx/MSI/MSI-X state check
>  - fix checking MSI/MSI-X state on devices not supporting it
> ---
>  .../ABI/testing/sysfs-driver-pciback  | 13 +++
>  drivers/xen/xen-pciback/conf_space.c  | 36 
>  drivers/xen/xen-pciback/conf_space.h  |  7 ++
>  .../xen/xen-pciback/conf_space_capability.c   | 88 +++
>  drivers/xen/xen-pciback/conf_space_header.c   | 19 
>  drivers/xen/xen-pciback/pci_stub.c| 66 ++
>  drivers/xen/xen-pciback/pciback.h |  1 +
>  7 files changed, 230 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-pciback 
> b/Documentation/ABI/testing/sysfs-driver-pciback
> index 6a733bfa37e6..566a11f2c12f 100644
> --- a/Documentation/ABI/testing/sysfs-driver-pciback
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -11,3 +11,16 @@ Description:
>  #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
>  will allow the guest to read and write to the configuration
>  register 0x0E.
> +
> +What:   /sys/bus/pci/drivers/pciback/allow_interrupt_control
> +Date:   Jan 2020
> +KernelVersion:  5.5
> +Contact:xen-devel@lists.xenproject.org
> +Description:
> +List of devices which can have interrupt control flag (INTx,
> +MSI, MSI-X) set by a connected guest. It is meant to be set
> +only when the guest is a stubdomain hosting device model 
> (qemu)
> +and the actual device is assigned to a HVM. It is not safe
> +(similar to permissive attribute) to set for a devices 
> assigned
> +to a PV guest. The device is automatically removed from this
> +list when the connected pcifront terminates.
> diff --git a/drivers/xen/xen-pciback/conf_space.c 
> b/drivers/xen/xen-pciback/conf_space.c
> index 60111719b01f..7697001e8ffc 100644
> --- a/drivers/xen/xen-pciback/conf_space.c
> +++ b/drivers/xen/xen-pciback/conf_space.c
> @@ -286,6 +286,42 @@ int xen_pcibk_config_write(struct pci_dev *dev, int 
> offset, int size, u32 value)
>   return xen_pcibios_err_to_errno(err);
>  }
>  
> +int xen_pcibk_get_interrupt_type(struct pci_dev *dev)

const for *dev.

> +{
> + int err;
> + u16 val;
> + int ret = 0;
> +
> + err = pci_read_config_word(dev, PCI_COMMAND, );
> + if (err)
> + return err;
> + if (!(val & PCI_COMMAND_INTX_DISABLE))
> + ret |= INTERRUPT_TYPE_INTX;
> +
> + /* Do not trust dev->msi(x)_enabled here, as enabling could be 

[Xen-devel] [XEN PATCH] linkfarm: Exclude .*.tmp

[Anthony PERARD]

Exclude intermidiate files .*.tmp from the linkfarm, those are
generated by %.o:%.c rules in xen/Rules.mk when
CONFIG_ENFORCE_UNIQUE_SYMBOLS=y.

Signed-off-by: Anthony PERARD 
---
 tools/firmware/xen-dir/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/firmware/xen-dir/Makefile b/tools/firmware/xen-dir/Makefile
index 5fa1cf22f1c1..5413f8679012 100644
--- a/tools/firmware/xen-dir/Makefile
+++ b/tools/firmware/xen-dir/Makefile
@@ -17,6 +17,7 @@ DEP_FILES=$(foreach i, $(LINK_FILES), $(XEN_ROOT)/$(i))
 # Exclude some intermediate files and final build products
 LINK_EXCLUDES := '*.[isoa]' '.*.d' '.*.d2' '.config'
 LINK_EXCLUDES += '*.map' 'xen' 'xen.gz' 'xen.efi' 'xen-syms'
+LINK_EXCLUDES += '.*.tmp'
 
 # This is all a giant mess and doesn't really work.
 #
-- 
Anthony PERARD


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

Re: [Xen-devel] [PATCH 17/20] tools/libx[cl]: Plumb static_data_done() up into libxl

[Andrew Cooper]

On 14/01/2020 17:30, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 17/20] tools/libx[cl]: Plumb static_data_done() 
> up into libxl"):
>>  /* callbacks provided by xc_domain_restore */
>>  struct restore_callbacks {
>> +/*
>> + * Called once the STATIC_DATA_END record has been received/inferred.
>> + * Passes in the blocks of static data which have not been received, 
>> which
>> + * the higher level toolstack must provide backwards compatibility for.
>> + */
>> +#define XGR_SDD_MISSING_CPUID (1 << 0)
>> +#define XGR_SDD_MISSING_MSR   (1 << 1)
>> +int (*static_data_done)(unsigned int missing, void *data);
> This is a bit weird, isn't it ?  I mean: if these blocks of data *are*
> received then libxc handles them; but if they are not, libxc's caller
> must do so.
>
> I appreciate that the interface at the top of libxc is already rather
> complex and uneven but this doesn't seem to be helping...

There are several things going on here.

One is the control flow marker of "We reached the end of the static
data".  A higher level toolstack needs to know this unconditionally,
which is why the callback is mandatory.

For v2 compatibility, its callers cope with "this is where an end of
static data would be in a v3 stream", but that abstracted away so the
higher level toolstack doesn't know or need to care.

The missing parameter is "p.s. here are the things we were expecting but
didn't get - you need to pick up the pieces".  For now, it is synonymous
with "here is a v2 stream without CPUID data", but that won't be
accurate in the future if/when new static data records get retrofitted.

~Andrew

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

Re: [Xen-devel] [PATCH v1 1/4] kasan: introduce set_pmd_early_shadow()

[Sergey Dyasli]

On 15/01/2020 11:09, Jürgen Groß wrote:
> On 15.01.20 11:54, Sergey Dyasli wrote:
>> Hi Juergen,
>>
>> On 08/01/2020 15:20, Sergey Dyasli wrote:
>>> It is incorrect to call pmd_populate_kernel() multiple times for the
>>> same page table. Xen notices it during kasan_populate_early_shadow():
>>>
>>>  (XEN) mm.c:3222:d155v0 mfn 3704b already pinned
>>>
>>> This happens for kasan_early_shadow_pte when USE_SPLIT_PTE_PTLOCKS is
>>> enabled. Fix this by introducing set_pmd_early_shadow() which calls
>>> pmd_populate_kernel() only once and uses set_pmd() afterwards.
>>>
>>> Signed-off-by: Sergey Dyasli 
>>
>> Looks like the plan to use set_pmd() directly has failed: it's an
>> arch-specific function and can't be used in arch-independent code
>> (as kbuild test robot has proven).
>>
>> Do you see any way out of this other than disabling SPLIT_PTE_PTLOCKS
>> for PV KASAN?
>
> Change set_pmd_early_shadow() like the following:
>
> #ifdef CONFIG_XEN_PV
> static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
> {
> static bool pmd_populated = false;
>
> if (likely(pmd_populated)) {
> set_pmd(pmd, __pmd(__pa(early_shadow) | _PAGE_TABLE));
> } else {
> pmd_populate_kernel(_mm, pmd, early_shadow);
> pmd_populated = true;
> }
> }
> #else
> static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
> {
> pmd_populate_kernel(_mm, pmd, early_shadow);
> }
> #endif
>
> ... and move it to include/xen/xen-ops.h and call it with
> lm_alias(kasan_early_shadow_pte) as the second parameter.

Your suggestion to use ifdef is really good, especially now when I
figured out that CONFIG_XEN_PV implies X86. But I don't like the idea
of kasan code calling a non-empty function from xen-ops.h when
CONFIG_XEN_PV is not defined. I'd prefer to keep set_pmd_early_shadow()
in mm/kasan/init.c with the suggested ifdef.

--
Thanks,
Sergey

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

Re: [Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Jan Beulich]

On 15.01.2020 17:29, Andrew Cooper wrote:
> On 15/01/2020 16:27, Jan Beulich wrote:
>> On 15.01.2020 15:36, Andrew Cooper wrote:
>>> On 15/01/2020 10:26, Jan Beulich wrote:
 While it has been me to introduce this, the use of | there has become
 (and perhaps was from the very beginning) misleading. Rather than
 avoiding the right side of it when linking the xen.efi intermediate file
 at a different base address, make the expression cope with that case,
 thus verifying placement on every step.

 Signed-off-by: Jan Beulich 
>>> Acked-by: Andrew Cooper  as this is simply a
>>> rearranging, but...
>>>
 --- a/xen/arch/x86/xen.lds.S
 +++ b/xen/arch/x86/xen.lds.S
 @@ -351,8 +351,8 @@ SECTIONS
.comment 0 : { *(.comment) }
  }
  
 -ASSERT(__image_base__ > XEN_VIRT_START |
 -   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
 +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + 
 __XEN_VIRT_START -
 +  NR_CPUS * PAGE_SIZE,
>>> ... doesn't this want a stubs_per_page term?  We don't have 4k per cpu.
>> Good point - let me see if I can fix this at this same occasion.
>> I guess when introducing these pages I had one per CPU initially,
>> and then forgot to update the assertion here (it being too strict
>> of course is better than it being too lax).
> 
> With some suitable term, feel free to up my A-by to R-by.

Let me rather post what I have, because there's going to be some
ugliness (and hence room for mistakes) because of the rounding
needed. But thanks for the offer.

Jan

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

Re: [Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Jan Beulich]

On 15.01.2020 15:36, Andrew Cooper wrote:
> On 15/01/2020 10:26, Jan Beulich wrote:
>> While it has been me to introduce this, the use of | there has become
>> (and perhaps was from the very beginning) misleading. Rather than
>> avoiding the right side of it when linking the xen.efi intermediate file
>> at a different base address, make the expression cope with that case,
>> thus verifying placement on every step.
>>
>> Signed-off-by: Jan Beulich 
> 
> Acked-by: Andrew Cooper  as this is simply a
> rearranging, but...
> 
>>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -351,8 +351,8 @@ SECTIONS
>>.comment 0 : { *(.comment) }
>>  }
>>  
>> -ASSERT(__image_base__ > XEN_VIRT_START |
>> -   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
>> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
>> +  NR_CPUS * PAGE_SIZE,
> 
> ... doesn't this want a stubs_per_page term?  We don't have 4k per cpu.

Good point - let me see if I can fix this at this same occasion.
I guess when introducing these pages I had one per CPU initially,
and then forgot to update the assertion here (it being too strict
of course is better than it being too lax).

Jan

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

Re: [Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Andrew Cooper]

On 15/01/2020 16:27, Jan Beulich wrote:
> On 15.01.2020 15:36, Andrew Cooper wrote:
>> On 15/01/2020 10:26, Jan Beulich wrote:
>>> While it has been me to introduce this, the use of | there has become
>>> (and perhaps was from the very beginning) misleading. Rather than
>>> avoiding the right side of it when linking the xen.efi intermediate file
>>> at a different base address, make the expression cope with that case,
>>> thus verifying placement on every step.
>>>
>>> Signed-off-by: Jan Beulich 
>> Acked-by: Andrew Cooper  as this is simply a
>> rearranging, but...
>>
>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -351,8 +351,8 @@ SECTIONS
>>>.comment 0 : { *(.comment) }
>>>  }
>>>  
>>> -ASSERT(__image_base__ > XEN_VIRT_START |
>>> -   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
>>> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START 
>>> -
>>> +  NR_CPUS * PAGE_SIZE,
>> ... doesn't this want a stubs_per_page term?  We don't have 4k per cpu.
> Good point - let me see if I can fix this at this same occasion.
> I guess when introducing these pages I had one per CPU initially,
> and then forgot to update the assertion here (it being too strict
> of course is better than it being too lax).

With some suitable term, feel free to up my A-by to R-by.

~Andrew

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 14:44, Roger Pau Monné wrote:
> On Wed, Jan 15, 2020 at 01:49:22PM +0100, Jan Beulich wrote:
>> What I'm then worried about is too
>> little progress observable by guests. The PV time protocol
>> ought to be fine in this regard (and consumers of raw TSC values
>> are on their own anyway), but wouldn't you need to update TSC
>> offsets of HVM guests in order to compensate for the elapsed
>> time?
> 
> That will be done when the HVM vCPU gets scheduled in as part of the
> update_vcpu_system_time call AFAICT. cstate_restore_tsc will always be
> called with the idle vCPU context, and hence there's always going to
> be a vCPU switch before scheduling anything else.

Which step would this be? All I see is a call to hvm_scale_tsc().
In time.c only tsc_set_info() calls hvm_set_tsc_offset().

Jan

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

Re: [Xen-devel] [PATCH 16/20] tools/libxl: Simplify callback handling in libxl-save-helper

[Andrew Cooper]

On 14/01/2020 17:27, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 16/20] tools/libxl: Simplify callback handling 
> in libxl-save-helper"):
>> The {save,restore}_callback helpers can have their scope reduced vastly,
> This part is OK with me although it would have been nicer to review if
> the the move and the rename were separate patches.  I don't know why
> it is valuable.
>
>> and helper_setcallbacks_{save,restore}() doesn't need to use a
>> ternary operator to write 0 (meaning NULL) into an already zeroed
>> structure.
> Is this unrelated ?  I think so.

This change is specifically to make the generated C easier to follow,
because I had to debug it yet again.

>>  my $c_cb = "cbs->$name";
>>  $f_more_sr->("if ($c_cb) cbflags |= $c_v;\n", $enumcallbacks);
>> -$f_more_sr->("$c_cb = (cbflags & $c_v) ? ${encode}_${name} : 
>> 0;\n",
>> +$f_more_sr->("if (cbflags & $c_v) $c_cb = ${encode}_${name};\n",
>>   $setcallbacks);
> It is a long time since I edited this code but I think your reasoning
> is "cbs is already zero on entry because it is static; therefore
> cbs->$name must be null, so there is no need to write 0 into it in the
> else case".

Correct.

>
> However, the line you are touching is preceded by "if ($c_cb)" which
> only makes sense if the variable might be non-null.
>
> So something is not right here.

This is all perl to me, but the two adjacent-looking lines of C don't
end up adjacent in the generated code.

The first line ends up in
libxl__srm_callout_enumcallbacks_{save,restore}() (in libxl.so), while
the second line ends up in helper_setcallbacks_{save,restore}() (in
libxl-save-helper).

~Andrew

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

Re: [Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Wei Liu]

On Wed, Jan 15, 2020 at 02:36:24PM +, Andrew Cooper wrote:
> On 15/01/2020 10:26, Jan Beulich wrote:
> > While it has been me to introduce this, the use of | there has become
> > (and perhaps was from the very beginning) misleading. Rather than
> > avoiding the right side of it when linking the xen.efi intermediate file
> > at a different base address, make the expression cope with that case,
> > thus verifying placement on every step.
> >
> > Signed-off-by: Jan Beulich 
> 
> Acked-by: Andrew Cooper  as this is simply a
> rearranging, but...
> 
> >
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -351,8 +351,8 @@ SECTIONS
> >.comment 0 : { *(.comment) }
> >  }
> >  
> > -ASSERT(__image_base__ > XEN_VIRT_START |
> > -   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
> > +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START 
> > -
> > +  NR_CPUS * PAGE_SIZE,
> 
> ... doesn't this want a stubs_per_page term?  We don't have 4k per cpu.
> 
> Wei: FYI, if you do introduce executable fixmap entries, this ASSERT()
> wants adjusting by NR_X_FIXMAP * PAGE_SIZE as well.

It is a bit problematic to do that if NR_X_FIXMAP is calculated from the
last enum element (see FIXADDR_SIZE)

I can expose a constant for the largest possible numbers of executable
fixed mappings to put in the ASSERT.

Wei.

> 
> ~Andrew

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

[Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[madhuparnabhowmik04]

From: Madhuparna Bhowmik 

list_for_each_entry_rcu has built-in RCU and lock checking.
Pass cond argument to list_for_each_entry_rcu.

Signed-off-by: Madhuparna Bhowmik 
---
 drivers/net/xen-netback/hash.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
index 10d580c3dea3..6b7532f7c936 100644
--- a/drivers/net/xen-netback/hash.c
+++ b/drivers/net/xen-netback/hash.c
@@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag,
 
found = false;
oldest = NULL;
-   list_for_each_entry_rcu(entry, >hash.cache.list, link) {
+   list_for_each_entry_rcu(entry, >hash.cache.list, link,
+   lockdep_is_held(>hash.cache.lock)) {
/* Make sure we don't add duplicate entries */
if (entry->len == len &&
memcmp(entry->tag, tag, len) == 0)
@@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif)
 
spin_lock_irqsave(>hash.cache.lock, flags);
 
-   list_for_each_entry_rcu(entry, >hash.cache.list, link) {
+   list_for_each_entry_rcu(entry, >hash.cache.list, link,
+   lockdep_is_held(>hash.cache.lock)) {
list_del_rcu(>link);
vif->hash.cache.count--;
kfree_rcu(entry, rcu);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH 12/12] libxc/save: Write X86_{CPUID, MSR}_DATA records

[Andrew Cooper]

On 14/01/2020 17:21, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 12/12] libxc/save: Write X86_{CPUID,MSR}_DATA 
> records"):
>> With all other plumbing in place, obtain the CPU Policy from Xen and
>> write it into the migration stream.
> This looks good to me but:
>
> This patch may need revision to handle the results of our discussion
> about the ?: error handling idiom.
>
> And I am still missing the text discussing compatibility.  Maybe I
> have just overlooked it ?

In all cases with migration development, the receive side logic
(previous patch) has to come before the save side logic (this patch), or
the result will break bisection with the receive side choking on an
unknown record type.

From the "whole series" point of view, compatibility is also the
destination side discarding the data because libxl still needs its order
of CPUID handling shuffling to cope.

~Andrew

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

Re: [Xen-devel] [PATCH 10/12] docs/migration: Specify X86_{CPUID, MSR}_POLICY records

[Andrew Cooper]

On 14/01/2020 16:12, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 10/12] docs/migration: Specify 
> X86_{CPUID,MSR}_POLICY records"):
>> The migration stream is split into records with no playload (markers
>> with external control flow meaning), and data records, which have a payload.
> I remember thinking at the time you specified this (some time ago, in
> migration v2) that this was anomalous.

It was, and remains, very deliberate.

> Whether a record is a marker ought to be inferred from its type.

All records have explicit semantics as specified by their types.  This
includes the semantics as to whether it shall have zero or non-zero payload.

A data record with no payload is nonsensical.  It is prohibited
specifically because it helps the protocol verification logic spot bugs,
and we really did spot several hypercall (preexiting) and save-side bugs
because of this rule.

If a plausible use for payload-less data appears, then we can take a
judgement call as to whether it outweighs the utility of improved error
detection.  Making this change would require a change to the spec, and
an adjustment to the pre-exiting receive side logic.

~Andrew

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

Re: [Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

On Wed, Jan 15, 2020 at 8:35 PM Wei Liu  wrote:

> On Wed, Jan 15, 2020 at 07:48:40PM +0530, madhuparnabhowmi...@gmail.com
> wrote:
> > From: Madhuparna Bhowmik 
> >
> > list_for_each_entry_rcu has built-in RCU and lock checking.
> > Pass cond argument to list_for_each_entry_rcu.
> >
> > Signed-off-by: Madhuparna Bhowmik 
>
> You seem to have dropped the second hunk which modified
> xenvif_flush_hash, is that a mistake?
>

I am sorry again, Yes I forgot to add the second hunk.
I will send the final patch with both the hunks in a while.

Thank you,
Madhuparna

Wei.
>
> > ---
> >  drivers/net/xen-netback/hash.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/xen-netback/hash.c
> b/drivers/net/xen-netback/hash.c
> > index 10d580c3dea3..3f9783f70a75 100644
> > --- a/drivers/net/xen-netback/hash.c
> > +++ b/drivers/net/xen-netback/hash.c
> > @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const
> u8 *tag,
> >
> >   found = false;
> >   oldest = NULL;
> > - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> > + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> > + lockdep_is_held(>hash.cache.lock)) {
> >   /* Make sure we don't add duplicate entries */
> >   if (entry->len == len &&
> >   memcmp(entry->tag, tag, len) == 0)
> > --
> > 2.17.1
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

On Wed, Jan 15, 2020 at 8:34 PM Wei Liu  wrote:

> On Wed, Jan 15, 2020 at 07:36:38PM +0530, Madhuparna Bhowmik wrote:
> [...]
> >
> > > The surrounding code makes it pretty clear that the lock is already
> held
> > > by the time list_for_each_entry_rcu is called, yet the checking
> involved
> > > in lockdep_is_held is not trivial, so I'm afraid I don't consider this
> a
> > > strict improvement over the existing code.
> > >
> > > Actually,  we want to make CONFIG_PROVE_LIST_RCU enabled by default.
>
> I think you meant CONFIG_PROVE_RCU_LIST.
>
> I am sorry about this. Yes, I meant  CONFIG_PROVE_RCU_LIST.

> And if the cond argument is not passed when the usage of
> > list_for_each_entry_rcu()
> > is outside of rcu_read_lock(), it will lead to a false positive.
> > Therefore, I think this patch is required.
>
> Fair enough.
>
> Thank you,
Madhuparna


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

Re: [Xen-devel] [PATCH 10/12] docs/migration: Specify X86_{CPUID, MSR}_POLICY records

[Andrew Cooper]

On 14/01/2020 16:08, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 10/12] docs/migration: Specify 
> X86_{CPUID,MSR}_POLICY records"):
>> These two records move blobs from the XEN_DOMCTL_{get,set}_cpu_policy
>> hypercall.
> We had an extensive IRL discussion recently about the compatibility
> implications of this.  Is that written down somewhere ?  I was
> expecting to see it in this patch.

Sadly clairvoyance isn't a skill I'm terribly good at.  (This email
predates our conversation by 2 weeks or so.)

v2 of the series will have appropriate adjustments, although none of it
was relevant to this patch specifically.

~Andrew

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

Re: [Xen-devel] [PATCH 05/12] tools/migration: Drop IHDR_VERSION constant from libxc and python

[Andrew Cooper]

On 14/01/2020 16:05, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 05/12] tools/migration: Drop IHDR_VERSION 
> constant from libxc and python"):
>> Migration v3 is in the process of being introduced, meaning that the code has
>> to cope with both versions.  Use an explicit 2 for now.
>>
>> For the verify-stream-v2 and convert-legacy-stream scripts, update text to 
>> say
>> "v2 (or later)".  What matters is the distinction vs legacy streams.
> How about introducing
>   enum { IHDR_VERSION_2 = 2 }
> or some such ?
>
> In C it can be hard otherwise to find all the relevant tests.  Being
> able to grep for IHDR_VERSION would help.  So I would prefer manifest
> constants of some kind to unvarnished integers.

There is exactly (and only ever) one place where this constant is
checked.  This is a consequence of it being read out of a pipe.

It is highly unlikely that the code will gain a second check.

~Andrew

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

Re: [Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[Wei Liu]

On Wed, Jan 15, 2020 at 07:48:40PM +0530, madhuparnabhowmi...@gmail.com wrote:
> From: Madhuparna Bhowmik 
> 
> list_for_each_entry_rcu has built-in RCU and lock checking.
> Pass cond argument to list_for_each_entry_rcu.
> 
> Signed-off-by: Madhuparna Bhowmik 

You seem to have dropped the second hunk which modified
xenvif_flush_hash, is that a mistake?

Wei.

> ---
>  drivers/net/xen-netback/hash.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
> index 10d580c3dea3..3f9783f70a75 100644
> --- a/drivers/net/xen-netback/hash.c
> +++ b/drivers/net/xen-netback/hash.c
> @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 
> *tag,
>  
>   found = false;
>   oldest = NULL;
> - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> + lockdep_is_held(>hash.cache.lock)) {
>   /* Make sure we don't add duplicate entries */
>   if (entry->len == len &&
>   memcmp(entry->tag, tag, len) == 0)
> -- 
> 2.17.1
> 

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

Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking

[Wei Liu]

On Wed, Jan 15, 2020 at 07:36:38PM +0530, Madhuparna Bhowmik wrote:
[...]
> 
> > The surrounding code makes it pretty clear that the lock is already held
> > by the time list_for_each_entry_rcu is called, yet the checking involved
> > in lockdep_is_held is not trivial, so I'm afraid I don't consider this a
> > strict improvement over the existing code.
> >
> > Actually,  we want to make CONFIG_PROVE_LIST_RCU enabled by default.

I think you meant CONFIG_PROVE_RCU_LIST.

> And if the cond argument is not passed when the usage of
> list_for_each_entry_rcu()
> is outside of rcu_read_lock(), it will lead to a false positive.
> Therefore, I think this patch is required.

Fair enough.

Wei.

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Andrew Cooper]

On 15/01/2020 12:54, Jan Beulich wrote:
> On 15.01.2020 13:47, Igor Druzhinin wrote:
>> On 15/01/2020 12:39, Jan Beulich wrote:
>>> On 15.01.2020 13:28, Igor Druzhinin wrote:
 On 15/01/2020 11:32, Jan Beulich wrote:
> On 14.01.2020 20:36, Igor Druzhinin wrote:
>> If ITSC is not available on CPU (e.g if running nested as PV shim)
>> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
>> all AMD and some old Intel processors. In which case TSC would need to
>> be restored on CPU from platform time by Xen upon exiting deep C-states.
> How does waking from deep C states correspond to the PV shim? I notice
> that cstate_restore_tsc() gets called irrespective of the C state being
> exited, so I wonder whether there's room for improvement there
> independent of the issue at hand. As far as this change is concerned,
> I think you want to drop the notion of "deep" from the description.
 I'm not familiar with what to call "deep C-state" so for me it was anything
 higher than C1. If you prefer "deep" to be dropped - so be it.
>>> "Higher than C1" may be fine (albeit I vaguely recall TSC issues
>>> starting with C3 only), but at least mwait_idle() calls the
>>> function even for C1. As to the PV shim - does it know about any
>>> C-states at all (beyond HLT-invoked C1)?
>> Yes, PV-shim knows about C states as it looks they are tied to
>> processor ID in some cases. For AMD specifically C2 is HLT.
> The AMD part is pretty new, and is - afaict - an exception compared
> to everything else. Under PVH there's no respective ACPI data (iirc),
> and we also don't surface MONITOR/MWAIT to HVM/PVH guests, making
> mwait_idle_probe() bail early. I wonder whether this special
> behavior on AMD Fam17 should be suppressed in this case.

Anything which knows it is virtualised should use hypercalls to yield,
and failing that, hlt.

A VM isn't going to get a plausible idea of C/P states (give or take our
dom0 special case seeing the real APCI tables which is still an open
problem.)

~Andrew

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 13:23, Roger Pau Monné wrote:
> On Wed, Jan 15, 2020 at 12:36:08PM +, Igor Druzhinin wrote:
>> On 15/01/2020 09:47, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
 If ITSC is not available on CPU (e.g if running nested as PV shim)
 then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
 all AMD and some old Intel processors. In which case TSC would need to
 be restored on CPU from platform time by Xen upon exiting deep C-states.

 As platform time might be behind the last TSC stamp recorded for the
 current CPU, invariant of TSC stamp being always behind local TSC counter
 is violated. This has an effect of get_s_time() going negative resulting
 in eventual system hang or crash.

 Fix this issue by updating local TSC stamp along with TSC counter write.
>>>
>>> Thanks! I haven't seen such issue because I've been running the shim
>>> with nomigrate in order to prevent the vTSC overhead.
>>>

 Signed-off-by: Igor Druzhinin 
 ---
 This caused reliable hangs of shim domains with multiple vCPUs on all AMD
 systems. The problem got also reproduced on bare-metal by artifically
 masking ITSC feature bit. The proposed fix has been verified for both
 cases.
 ---
  xen/arch/x86/time.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

 diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
 index e79cb4d..f6b26f8 100644
 --- a/xen/arch/x86/time.c
 +++ b/xen/arch/x86/time.c
 @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
  
  void cstate_restore_tsc(void)
  {
 +struct cpu_time *t = _cpu(cpu_time);
 +
  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
  return;
  
 -write_tsc(stime2tsc(read_platform_stime(NULL)));
 +t->stamp.master_stime = read_platform_stime(NULL);
 +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
 +t->stamp.local_stime = t->stamp.master_stime;
 +
 +write_tsc(t->stamp.local_tsc);
>>>
>>> In order to avoid the TSC write (and the likely associated vmexit),
>>> could you instead do:
>>>
>>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
>>> t->stamp.local_tsc = rdtsc_ordered();
>>
>> I think in that case RDTSC might return something behind platform time
>> which is not right I guess.
> 
> The TSC and the platform time are completely independent from Xen's
> PoV, you can have a platform time not based on the TSC (ie: PIT, HPET
> or PM), and hence there's no direct relation between both.
> 
> The TSC is used as a way to get an approximate platform time based on
> the last platform time value and the TSC delta between than value and
> the current TSC value, I assume that's done because reading the TSC is
> much cheaper than reading the platform time.
> 
> As long as the platform time and the TSC stamps are both updated at the
> same time it should be fine.

I see your point. I'll test your approach and get back here with the results.

Igor

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

Re: [Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Andrew Cooper]

On 15/01/2020 10:26, Jan Beulich wrote:
> While it has been me to introduce this, the use of | there has become
> (and perhaps was from the very beginning) misleading. Rather than
> avoiding the right side of it when linking the xen.efi intermediate file
> at a different base address, make the expression cope with that case,
> thus verifying placement on every step.
>
> Signed-off-by: Jan Beulich 

Acked-by: Andrew Cooper  as this is simply a
rearranging, but...

>
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -351,8 +351,8 @@ SECTIONS
>.comment 0 : { *(.comment) }
>  }
>  
> -ASSERT(__image_base__ > XEN_VIRT_START |
> -   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
> +ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
> +  NR_CPUS * PAGE_SIZE,

... doesn't this want a stubs_per_page term?  We don't have 4k per cpu.

Wei: FYI, if you do introduce executable fixmap entries, this ASSERT()
wants adjusting by NR_X_FIXMAP * PAGE_SIZE as well.

~Andrew

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

[Xen-devel] [PATCH] net: xen-netback: hash.c: Use built-in RCU list checking

[madhuparnabhowmik04]

From: Madhuparna Bhowmik 

list_for_each_entry_rcu has built-in RCU and lock checking.
Pass cond argument to list_for_each_entry_rcu.

Signed-off-by: Madhuparna Bhowmik 
---
 drivers/net/xen-netback/hash.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
index 10d580c3dea3..3f9783f70a75 100644
--- a/drivers/net/xen-netback/hash.c
+++ b/drivers/net/xen-netback/hash.c
@@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag,
 
found = false;
oldest = NULL;
-   list_for_each_entry_rcu(entry, >hash.cache.list, link) {
+   list_for_each_entry_rcu(entry, >hash.cache.list, link,
+   lockdep_is_held(>hash.cache.lock)) {
/* Make sure we don't add duplicate entries */
if (entry->len == len &&
memcmp(entry->tag, tag, len) == 0)
-- 
2.17.1


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

[Xen-devel] [PATCH v2 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Andrew Cooper]

Despite being vaguely aware, the difference between PAGE_HYPERVISOR in ASM and
C code has nevertheless caused several bugs I should have known better about,
and contributed to review confusion.

There are exactly 4 uses of these constants in asm code (and one is shortly
going to disappear).

Instead of creating the constants which behave differently between ASM and C
code, expose all the constants and use non-ambiguous non-NX ones in ASM.
Adjust the hiding to just _PAGE_NX, which contains a C ternary expression.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 

v2:
 * Hide _PAGE_NX
---
 xen/arch/x86/boot/head.S  |  2 +-
 xen/arch/x86/boot/x86_64.S|  6 +++---
 xen/include/asm-x86/page.h|  4 
 xen/include/asm-x86/x86_64/page.h | 17 +
 4 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index d246e374f1..563bb19056 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -674,7 +674,7 @@ trampoline_setup:
  * the transition into long mode), using 2M superpages.
  */
 lea sym_esi(start),%ebx
-lea 
(1<= 0xa0 && pfn < 0xc0
-.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_UCMINUS | MAP_SMALL_PAGES
+.quad (pfn << PAGE_SHIFT) | __PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL | 
MAP_SMALL_PAGES
 .else
-.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR | MAP_SMALL_PAGES
+.quad (pfn << PAGE_SHIFT) | PAGE_HYPERVISOR_RWX | MAP_SMALL_PAGES
 .endif
 pfn = pfn + 1
 .endr
@@ -89,7 +89,7 @@ GLOBAL(l2_xenmap)
 .quad 0
 idx = 1
 .rept 7
-.quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + 
(PAGE_HYPERVISOR | _PAGE_PSE)
+.quad sym_offs(__image_base__) + (idx << L2_PAGETABLE_SHIFT) + 
(PAGE_HYPERVISOR_RWX | _PAGE_PSE)
 idx = idx + 1
 .endr
 .fill L2_PAGETABLE_ENTRIES - 8, 8, 0
diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 05a8b1efa6..a3c76a403b 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -316,7 +316,11 @@ void efi_update_l4_pgtable(unsigned int l4idx, 
l4_pgentry_t);
 #define _PAGE_AVAIL_AC(0xE00,U)
 #define _PAGE_PSE_PAT  _AC(0x1000,U)
 #define _PAGE_AVAIL_HIGH (_AC(0x7ff, U) << 12)
+
+#ifndef __ASSEMBLY__
+/* Dependency on NX being available can't be expressed. */
 #define _PAGE_NX   (cpu_has_nx ? _PAGE_NX_BIT : 0)
+#endif
 
 #define PAGE_CACHE_ATTRS (_PAGE_PAT | _PAGE_PCD | _PAGE_PWT)
 
diff --git a/xen/include/asm-x86/x86_64/page.h 
b/xen/include/asm-x86/x86_64/page.h
index 4fe0205553..9876634881 100644
--- a/xen/include/asm-x86/x86_64/page.h
+++ b/xen/include/asm-x86/x86_64/page.h
@@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
 #define PAGE_HYPERVISOR_RX  (__PAGE_HYPERVISOR_RX  | _PAGE_GLOBAL)
 #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL)
 
-#ifdef __ASSEMBLY__
-/* Dependency on NX being available can't be expressed. */
-# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
-# define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC  | _PAGE_GLOBAL)
-#else
-# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
-# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
-  _PAGE_GLOBAL | _PAGE_NX)
-# define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC | \
-  _PAGE_GLOBAL | _PAGE_NX)
-#endif
+#define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
+#define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
+ _PAGE_GLOBAL | _PAGE_NX)
+#define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC | \
+ _PAGE_GLOBAL | _PAGE_NX)
 
 #endif /* __X86_64_PAGE_H__ */
 
-- 
2.11.0


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

Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking

[Madhuparna Bhowmik]

On Wed, Jan 15, 2020 at 7:26 PM Wei Liu  wrote:

> Thanks for the patch.
>
> There is a typo in the subject line. It should say xen-netback, not
> xen-netbank.
>
> Hi,

I am sorry about this, I will send this patch again.


> On Wed, Jan 15, 2020 at 06:11:28PM +0530, madhuparnabhowmi...@gmail.com
> wrote:
> > From: Madhuparna Bhowmik 
> >
> > list_for_each_entry_rcu has built-in RCU and lock checking.
> > Pass cond argument to list_for_each_entry_rcu.
> >
> > Signed-off-by: Madhuparna Bhowmik 
> > ---
> >  drivers/net/xen-netback/hash.c | 6 --
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/xen-netback/hash.c
> b/drivers/net/xen-netback/hash.c
> > index 10d580c3dea3..30709bc9d170 100644
> > --- a/drivers/net/xen-netback/hash.c
> > +++ b/drivers/net/xen-netback/hash.c
> > @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const
> u8 *tag,
> >
> >   found = false;
> >   oldest = NULL;
> > - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> > + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> > +
>  lockdep_is_held(>hash.cache.lock)) {
>
> There are probably too many tabs here. Indentation looks wrong.
>
> I will correct this when I resend this patch.


> The surrounding code makes it pretty clear that the lock is already held
> by the time list_for_each_entry_rcu is called, yet the checking involved
> in lockdep_is_held is not trivial, so I'm afraid I don't consider this a
> strict improvement over the existing code.
>
> Actually,  we want to make CONFIG_PROVE_LIST_RCU enabled by default.
And if the cond argument is not passed when the usage of
list_for_each_entry_rcu()
is outside of rcu_read_lock(), it will lead to a false positive.
Therefore, I think this patch is required.
Let me know if you have any objections.

Thank you,
Madhuparna


> If there is something I misunderstood, let me know.
>
> Wei.
>
> >   /* Make sure we don't add duplicate entries */
> >   if (entry->len == len &&
> >   memcmp(entry->tag, tag, len) == 0)
> > @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif)
> >
> >   spin_lock_irqsave(>hash.cache.lock, flags);
> >
> > - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> > + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> > +
>  lockdep_is_held(>hash.cache.lock)) {
> >   list_del_rcu(>link);
> >   vif->hash.cache.count--;
> >   kfree_rcu(entry, rcu);
> > --
> > 2.17.1
> >
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking

[Wei Liu]

Thanks for the patch.

There is a typo in the subject line. It should say xen-netback, not
xen-netbank.

On Wed, Jan 15, 2020 at 06:11:28PM +0530, madhuparnabhowmi...@gmail.com wrote:
> From: Madhuparna Bhowmik 
> 
> list_for_each_entry_rcu has built-in RCU and lock checking.
> Pass cond argument to list_for_each_entry_rcu.
> 
> Signed-off-by: Madhuparna Bhowmik 
> ---
>  drivers/net/xen-netback/hash.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
> index 10d580c3dea3..30709bc9d170 100644
> --- a/drivers/net/xen-netback/hash.c
> +++ b/drivers/net/xen-netback/hash.c
> @@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 
> *tag,
>  
>   found = false;
>   oldest = NULL;
> - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> + 
> lockdep_is_held(>hash.cache.lock)) {

There are probably too many tabs here. Indentation looks wrong.

The surrounding code makes it pretty clear that the lock is already held
by the time list_for_each_entry_rcu is called, yet the checking involved
in lockdep_is_held is not trivial, so I'm afraid I don't consider this a
strict improvement over the existing code.

If there is something I misunderstood, let me know.

Wei.

>   /* Make sure we don't add duplicate entries */
>   if (entry->len == len &&
>   memcmp(entry->tag, tag, len) == 0)
> @@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif)
>  
>   spin_lock_irqsave(>hash.cache.lock, flags);
>  
> - list_for_each_entry_rcu(entry, >hash.cache.list, link) {
> + list_for_each_entry_rcu(entry, >hash.cache.list, link,
> + 
> lockdep_is_held(>hash.cache.lock)) {
>   list_del_rcu(>link);
>   vif->hash.cache.count--;
>   kfree_rcu(entry, rcu);
> -- 
> 2.17.1
> 

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Roger Pau Monné]

On Wed, Jan 15, 2020 at 01:49:22PM +0100, Jan Beulich wrote:
> On 15.01.2020 12:53, Roger Pau Monné wrote:
> > On Wed, Jan 15, 2020 at 12:40:27PM +0100, Jan Beulich wrote:
> >> On 15.01.2020 10:47, Roger Pau Monné wrote:
> >>> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
>  --- a/xen/arch/x86/time.c
>  +++ b/xen/arch/x86/time.c
>  @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>   
>   void cstate_restore_tsc(void)
>   {
>  +struct cpu_time *t = _cpu(cpu_time);
>  +
>   if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>   return;
>   
>  -write_tsc(stime2tsc(read_platform_stime(NULL)));
>  +t->stamp.master_stime = read_platform_stime(NULL);
>  +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
>  +t->stamp.local_stime = t->stamp.master_stime;
>  +
>  +write_tsc(t->stamp.local_tsc);
> >>>
> >>> In order to avoid the TSC write (and the likely associated vmexit),
> >>> could you instead do:
> >>>
> >>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> >>> t->stamp.local_tsc = rdtsc_ordered();
> >>>
> >>> I think it should achieve the same as it syncs the local TSC stamp and
> >>> times, would avoid the TSC write and slightly simplifies the logic.
> >>
> >> Wouldn't this result in guests possibly observing the TSC moving
> >> backwards?
> > 
> > Isn't local_tsc storing a TSC value read from the same CPU always, and
> > hence could only go backwards if rdtsc actually goes backwards?
> 
> For one I have to admit I was (mistakenly) thinking of wakeup
> from S states more than that from C states. So assuming the
> TSC indeed only stops (but won't get e.g. restarted), backwards
> moves ought to be excluded.

Even if the TSC was restarted I think my proposed approach should be
fine. The only requirement is that the stored TSC stamp must always be
behind than the value returned by rdtsc. See get_s_time_fixed: as
long as the delta is positive the returned time should be correct.

> What I'm then worried about is too
> little progress observable by guests. The PV time protocol
> ought to be fine in this regard (and consumers of raw TSC values
> are on their own anyway), but wouldn't you need to update TSC
> offsets of HVM guests in order to compensate for the elapsed
> time?

That will be done when the HVM vCPU gets scheduled in as part of the
update_vcpu_system_time call AFAICT. cstate_restore_tsc will always be
called with the idle vCPU context, and hence there's always going to
be a vCPU switch before scheduling anything else.

> > Ie: cpu_frequency_change seems to do something similar, together with
> > a re-adjusting of the time scale, but doesn't perform any TSC write.
> 
> A P-state change at most alters the the tick rate, but wouldn't
> stop or even reset the TSC (afaict).

Right, just wanted to point out that the cpu_time stamp can be
updated without having to write to the TSC. Anyway, not sure it's very
relevant or useful, so forget this reference.

Roger.

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Roger Pau Monné]

On Wed, Jan 15, 2020 at 12:36:08PM +, Igor Druzhinin wrote:
> On 15/01/2020 09:47, Roger Pau Monné wrote:
> > On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
> >> If ITSC is not available on CPU (e.g if running nested as PV shim)
> >> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
> >> all AMD and some old Intel processors. In which case TSC would need to
> >> be restored on CPU from platform time by Xen upon exiting deep C-states.
> >>
> >> As platform time might be behind the last TSC stamp recorded for the
> >> current CPU, invariant of TSC stamp being always behind local TSC counter
> >> is violated. This has an effect of get_s_time() going negative resulting
> >> in eventual system hang or crash.
> >>
> >> Fix this issue by updating local TSC stamp along with TSC counter write.
> > 
> > Thanks! I haven't seen such issue because I've been running the shim
> > with nomigrate in order to prevent the vTSC overhead.
> > 
> >>
> >> Signed-off-by: Igor Druzhinin 
> >> ---
> >> This caused reliable hangs of shim domains with multiple vCPUs on all AMD
> >> systems. The problem got also reproduced on bare-metal by artifically
> >> masking ITSC feature bit. The proposed fix has been verified for both
> >> cases.
> >> ---
> >>  xen/arch/x86/time.c | 8 +++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> >> index e79cb4d..f6b26f8 100644
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
> >>  
> >>  void cstate_restore_tsc(void)
> >>  {
> >> +struct cpu_time *t = _cpu(cpu_time);
> >> +
> >>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
> >>  return;
> >>  
> >> -write_tsc(stime2tsc(read_platform_stime(NULL)));
> >> +t->stamp.master_stime = read_platform_stime(NULL);
> >> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
> >> +t->stamp.local_stime = t->stamp.master_stime;
> >> +
> >> +write_tsc(t->stamp.local_tsc);
> > 
> > In order to avoid the TSC write (and the likely associated vmexit),
> > could you instead do:
> > 
> > t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> > t->stamp.local_tsc = rdtsc_ordered();
> 
> I think in that case RDTSC might return something behind platform time
> which is not right I guess.

The TSC and the platform time are completely independent from Xen's
PoV, you can have a platform time not based on the TSC (ie: PIT, HPET
or PM), and hence there's no direct relation between both.

The TSC is used as a way to get an approximate platform time based on
the last platform time value and the TSC delta between than value and
the current TSC value, I assume that's done because reading the TSC is
much cheaper than reading the platform time.

As long as the platform time and the TSC stamps are both updated at the
same time it should be fine.

Roger.

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

Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Jan Beulich]

On 15.01.2020 13:53, Andrew Cooper wrote:
> On 14/01/2020 16:25, Jan Beulich wrote:
>>> --- a/xen/include/asm-x86/x86_64/page.h
>>> +++ b/xen/include/asm-x86/x86_64/page.h
>>> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
>>>  #define PAGE_HYPERVISOR_RX  (__PAGE_HYPERVISOR_RX  | _PAGE_GLOBAL)
>>>  #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL)
>>>  
>>> -#ifdef __ASSEMBLY__
>>> -/* Dependency on NX being available can't be expressed. */
>>> -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX
>>> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
>>> -# define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC  | _PAGE_GLOBAL)
>>> -#else
>>>  # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
>>>  # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
>>>_PAGE_GLOBAL | _PAGE_NX)
>>>  # define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC | \
>>>_PAGE_GLOBAL | _PAGE_NX)
>> ... I'm afraid the assembler error resulting from someone actually
>> (and mistakenly) using one of the constants making use of _PAGE_NX
>> is going to be rather cryptic. Which in turn may motivate people
>> to actually try to make _PAGE_NX "work" in assembly code. Therefore
>> I'd like to ask that together with the changes here _PAGE_NX's
>> #define be framed by #ifndef __ASSEMBLY__, such that any
>> diagnostic, if it mentions a symbol name, would name the actual
>> problem, rather than a derived one.
> 
> I can do this, but it doesn't make the error any less cryptic.
> 
> With _PAGE_NX hidden:
> 
> head.S: Assembler messages:
> head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|'

This is something that could be improved in the future in gas
(by simply naming the symbol found to be *UND*).

> With it visible:
> 
> head.S: Assembler messages:
> head.S:677: Error: invalid character '?' in operand 1

This, otoh, can't sensibly be expected to see an improvement.
(Well, perhaps the full line could be quoted, but that's true
for about every parsing error.)

Jan

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

Re: [Xen-devel] [PATCH v2 0/4] Use no_vblank property for drivers without VBLANK

[Hans de Goede]

Hi,

On 15-01-2020 13:52, Thomas Zimmermann wrote:

(Resending because I did not cc dri-devel properly.)

Instead of faking VBLANK events by themselves, drivers without VBLANK
support can enable drm_crtc_vblank.no_vblank and let DRM do the rest.
The patchset makes this official and converts over several drivers.

Ast already uses the functionality and just needs a cleanup. Cirrus can
be converted easily by setting the field in the check() callback and
removing the existing VBLANK code. For most other simple-KMS drivers
without enable_vblank() and check(), simple-KMS helpers can enable the
faked VBLANK by default. The only exception is Xen, which comes with
its own VBLANK logic and should rather to disable no_vblank.

v2:
* document functionality (Daniel)
* cleanup ast (Daniel)
* let simple-kms handle no_vblank where possible


Entire series looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans





Thomas Zimmermann (4):
   drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events
   drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()
   drm/cirrus: Let DRM core send VBLANK events
   drm/simple-kms: Let DRM core send VBLANK events by default

  drivers/gpu/drm/ast/ast_mode.c  |  4 ++--
  drivers/gpu/drm/bochs/bochs_kms.c   |  9 -
  drivers/gpu/drm/cirrus/cirrus.c | 10 ++
  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
  drivers/gpu/drm/drm_mipi_dbi.c  |  9 -
  drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++
  drivers/gpu/drm/tiny/gm12u320.c |  9 -
  drivers/gpu/drm/tiny/ili9225.c  |  9 -
  drivers/gpu/drm/tiny/repaper.c  |  9 -
  drivers/gpu/drm/tiny/st7586.c   |  9 -
  drivers/gpu/drm/udl/udl_modeset.c   | 11 ---
  drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +
  include/drm/drm_crtc.h  |  9 +++--
  include/drm/drm_simple_kms_helper.h |  7 +--
  14 files changed, 47 insertions(+), 84 deletions(-)

--
2.24.1




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

[Xen-devel] [PATCH] net: xen-netbank: hash.c: Use built-in RCU list checking

[madhuparnabhowmik04]

From: Madhuparna Bhowmik 

list_for_each_entry_rcu has built-in RCU and lock checking.
Pass cond argument to list_for_each_entry_rcu.

Signed-off-by: Madhuparna Bhowmik 
---
 drivers/net/xen-netback/hash.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c
index 10d580c3dea3..30709bc9d170 100644
--- a/drivers/net/xen-netback/hash.c
+++ b/drivers/net/xen-netback/hash.c
@@ -51,7 +51,8 @@ static void xenvif_add_hash(struct xenvif *vif, const u8 *tag,
 
found = false;
oldest = NULL;
-   list_for_each_entry_rcu(entry, >hash.cache.list, link) {
+   list_for_each_entry_rcu(entry, >hash.cache.list, link,
+   
lockdep_is_held(>hash.cache.lock)) {
/* Make sure we don't add duplicate entries */
if (entry->len == len &&
memcmp(entry->tag, tag, len) == 0)
@@ -102,7 +103,8 @@ static void xenvif_flush_hash(struct xenvif *vif)
 
spin_lock_irqsave(>hash.cache.lock, flags);
 
-   list_for_each_entry_rcu(entry, >hash.cache.list, link) {
+   list_for_each_entry_rcu(entry, >hash.cache.list, link,
+   
lockdep_is_held(>hash.cache.lock)) {
list_del_rcu(>link);
vif->hash.cache.count--;
kfree_rcu(entry, rcu);
-- 
2.17.1


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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 13:47, Igor Druzhinin wrote:
> On 15/01/2020 12:39, Jan Beulich wrote:
>> On 15.01.2020 13:28, Igor Druzhinin wrote:
>>> On 15/01/2020 11:32, Jan Beulich wrote:
 On 14.01.2020 20:36, Igor Druzhinin wrote:
> If ITSC is not available on CPU (e.g if running nested as PV shim)
> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
> all AMD and some old Intel processors. In which case TSC would need to
> be restored on CPU from platform time by Xen upon exiting deep C-states.

 How does waking from deep C states correspond to the PV shim? I notice
 that cstate_restore_tsc() gets called irrespective of the C state being
 exited, so I wonder whether there's room for improvement there
 independent of the issue at hand. As far as this change is concerned,
 I think you want to drop the notion of "deep" from the description.
>>>
>>> I'm not familiar with what to call "deep C-state" so for me it was anything
>>> higher than C1. If you prefer "deep" to be dropped - so be it.
>>
>> "Higher than C1" may be fine (albeit I vaguely recall TSC issues
>> starting with C3 only), but at least mwait_idle() calls the
>> function even for C1. As to the PV shim - does it know about any
>> C-states at all (beyond HLT-invoked C1)?
> 
> Yes, PV-shim knows about C states as it looks they are tied to
> processor ID in some cases. For AMD specifically C2 is HLT.

The AMD part is pretty new, and is - afaict - an exception compared
to everything else. Under PVH there's no respective ACPI data (iirc),
and we also don't surface MONITOR/MWAIT to HVM/PVH guests, making
mwait_idle_probe() bail early. I wonder whether this special
behavior on AMD Fam17 should be suppressed in this case.

Jan

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

Re: [Xen-devel] [PATCH 2/4] x86/page: Remove bifurcated PAGE_HYPERVISOR constant

[Andrew Cooper]

On 14/01/2020 16:25, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/x86_64/page.h
>> +++ b/xen/include/asm-x86/x86_64/page.h
>> @@ -172,18 +172,11 @@ static inline intpte_t put_pte_flags(unsigned int x)
>>  #define PAGE_HYPERVISOR_RX  (__PAGE_HYPERVISOR_RX  | _PAGE_GLOBAL)
>>  #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL)
>>  
>> -#ifdef __ASSEMBLY__
>> -/* Dependency on NX being available can't be expressed. */
>> -# define PAGE_HYPERVISOR PAGE_HYPERVISOR_RWX
>> -# define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | _PAGE_GLOBAL)
>> -# define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC  | _PAGE_GLOBAL)
>> -#else
>>  # define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW
>>  # define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
>>_PAGE_GLOBAL | _PAGE_NX)
>>  # define PAGE_HYPERVISOR_UC  (__PAGE_HYPERVISOR_UC | \
>>_PAGE_GLOBAL | _PAGE_NX)
> ... I'm afraid the assembler error resulting from someone actually
> (and mistakenly) using one of the constants making use of _PAGE_NX
> is going to be rather cryptic. Which in turn may motivate people
> to actually try to make _PAGE_NX "work" in assembly code. Therefore
> I'd like to ask that together with the changes here _PAGE_NX's
> #define be framed by #ifndef __ASSEMBLY__, such that any
> diagnostic, if it mentions a symbol name, would name the actual
> problem, rather than a derived one.

I can do this, but it doesn't make the error any less cryptic.

With _PAGE_NX hidden:

head.S: Assembler messages:
head.S:677: Error: invalid operands (*ABS* and *UND* sections) for `|'

With it visible:

head.S: Assembler messages:
head.S:677: Error: invalid character '?' in operand 1

I'm not aware of any way to get a useful symbol name out of the error.

~Andrew

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

[Xen-devel] [PATCH v2 3/4] drm/cirrus: Let DRM core send VBLANK events

[Thomas Zimmermann]

In drm_atomic_helper_fake_vblank(), the DRM core sends out VBLANK
events if struct drm_crtc_state.no_vblank is enabled. Replace cirrus'
VBLANK events with the DRM core's functionality.

v2:
* set struct_drm_crtc_state.no_vblank in cirrus_pipe_check()

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/cirrus/cirrus.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
index 248c9f765c45..5ff15e8a2a0a 100644
--- a/drivers/gpu/drm/cirrus/cirrus.c
+++ b/drivers/gpu/drm/cirrus/cirrus.c
@@ -38,7 +38,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #define DRIVER_NAME "cirrus"
 #define DRIVER_DESC "qemu cirrus vga"
@@ -404,6 +403,8 @@ static int cirrus_pipe_check(struct drm_simple_display_pipe 
*pipe,
 {
struct drm_framebuffer *fb = plane_state->fb;
 
+   crtc_state->no_vblank = true;
+
if (!fb)
return 0;
return cirrus_check_size(fb->width, fb->height, fb);
@@ -434,13 +435,6 @@ static void cirrus_pipe_update(struct 
drm_simple_display_pipe *pipe,
 
if (drm_atomic_helper_damage_merged(old_state, state, ))
cirrus_fb_blit_rect(pipe->plane.state->fb, );
-
-   if (crtc->state->event) {
-   spin_lock_irq(>dev->event_lock);
-   drm_crtc_send_vblank_event(crtc, crtc->state->event);
-   crtc->state->event = NULL;
-   spin_unlock_irq(>dev->event_lock);
-   }
 }
 
 static const struct drm_simple_display_pipe_funcs cirrus_pipe_funcs = {
-- 
2.24.1


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

[Xen-devel] [PATCH v2 1/4] drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events

[Thomas Zimmermann]

Drivers for CRTC hardware without support for VBLANK interrupts can set
struct drm_crtc_state.no_vblank and let DRM's atomic commit helpers
generate the VBLANK events automatically. Document this in order to make
it official.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/drm_atomic_helper.c | 4 +++-
 include/drm/drm_crtc.h  | 9 +++--
 include/drm/drm_simple_kms_helper.h | 7 +--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 4511c2e07bb9..ce30a37971e4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2215,7 +2215,9 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
  * when a job is queued, and any change to the pipeline that does not touch the
  * connector is leading to timeouts when calling
  * drm_atomic_helper_wait_for_vblanks() or
- * drm_atomic_helper_wait_for_flip_done().
+ * drm_atomic_helper_wait_for_flip_done(). In addition to writeback
+ * connectors, this function can also fake VBLANK events for CRTCs without
+ * VBLANK interrupt.
  *
  * This is part of the atomic helper support for nonblocking commits, see
  * drm_atomic_helper_setup_commit() for an overview.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 5e9b15a0e8c5..01caf5160596 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -179,7 +179,9 @@ struct drm_crtc_state {
 * In this case the VBLANK event is only generated when a job is queued
 * to the writeback connector, and we want the core to fake VBLANK
 * events when this part of the pipeline hasn't changed but others had
-* or when the CRTC and connectors are being disabled.
+* or when the CRTC and connectors are being disabled. In addition to
+* writeback connectors, this function can also fake VBLANK events for
+* CRTCs without VBLANK interrupt.
 *
 * __drm_atomic_helper_crtc_duplicate_state() will not reset the value
 * from the current state, the CRTC driver is then responsible for
@@ -335,7 +337,10 @@ struct drm_crtc_state {
 *  - Events for disabled CRTCs are not allowed, and drivers can ignore
 *that case.
 *
-* This can be handled by the drm_crtc_send_vblank_event() function,
+* For very simple hardware without VBLANK interrupt, enabling
+*  drm_crtc_state.no_vblank makes DRM's atomic commit helpers
+* send the event at an appropriate time. For more complex hardware this
+* can be handled by the drm_crtc_send_vblank_event() function,
 * which the driver should call on the provided event upon completion of
 * the atomic commit. Note that if the driver supports vblank signalling
 * and timestamping the vblank counters and timestamps must agree with
diff --git a/include/drm/drm_simple_kms_helper.h 
b/include/drm/drm_simple_kms_helper.h
index 15afee9cf049..e253ba7bea9d 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -100,8 +100,11 @@ struct drm_simple_display_pipe_funcs {
 * This is the function drivers should submit the
 * _pending_vblank_event from. Using either
 * drm_crtc_arm_vblank_event(), when the driver supports vblank
-* interrupt handling, or drm_crtc_send_vblank_event() directly in case
-* the hardware lacks vblank support entirely.
+* interrupt handling, or drm_crtc_send_vblank_event() for more
+* complex case. In case the hardware lacks vblank support entirely,
+* drivers can set  drm_crtc_state.no_vblank in
+*  drm_simple_display_pipe_funcs.check and let DRM's
+* atomic helper fake a vblank event.
 */
void (*update)(struct drm_simple_display_pipe *pipe,
   struct drm_plane_state *old_plane_state);
-- 
2.24.1


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

[Xen-devel] [PATCH v2 0/4] Use no_vblank property for drivers without VBLANK

[Thomas Zimmermann]

(Resending because I did not cc dri-devel properly.)

Instead of faking VBLANK events by themselves, drivers without VBLANK
support can enable drm_crtc_vblank.no_vblank and let DRM do the rest.
The patchset makes this official and converts over several drivers.

Ast already uses the functionality and just needs a cleanup. Cirrus can
be converted easily by setting the field in the check() callback and
removing the existing VBLANK code. For most other simple-KMS drivers
without enable_vblank() and check(), simple-KMS helpers can enable the
faked VBLANK by default. The only exception is Xen, which comes with
its own VBLANK logic and should rather to disable no_vblank.

v2:
* document functionality (Daniel)
* cleanup ast (Daniel)
* let simple-kms handle no_vblank where possible

Thomas Zimmermann (4):
  drm: Document struct drm_crtc_state.no_vblank for faking VBLANK events
  drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()
  drm/cirrus: Let DRM core send VBLANK events
  drm/simple-kms: Let DRM core send VBLANK events by default

 drivers/gpu/drm/ast/ast_mode.c  |  4 ++--
 drivers/gpu/drm/bochs/bochs_kms.c   |  9 -
 drivers/gpu/drm/cirrus/cirrus.c | 10 ++
 drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
 drivers/gpu/drm/drm_mipi_dbi.c  |  9 -
 drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++
 drivers/gpu/drm/tiny/gm12u320.c |  9 -
 drivers/gpu/drm/tiny/ili9225.c  |  9 -
 drivers/gpu/drm/tiny/repaper.c  |  9 -
 drivers/gpu/drm/tiny/st7586.c   |  9 -
 drivers/gpu/drm/udl/udl_modeset.c   | 11 ---
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +
 include/drm/drm_crtc.h  |  9 +++--
 include/drm/drm_simple_kms_helper.h |  7 +--
 14 files changed, 47 insertions(+), 84 deletions(-)

--
2.24.1


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

[Xen-devel] [PATCH v2 4/4] drm/simple-kms: Let DRM core send VBLANK events by default

[Thomas Zimmermann]

In drm_atomic_helper_fake_vblank() the DRM core sends out VBLANK events
if struct drm_crtc_state.no_vblank is enabled in the check() callbacks.

For drivers that have neither an enable_vblank() callback nor a check()
callback, the simple-KMS helpers enable VBLANK generation by default. This
simplifies bochs, udl, several tiny drivers, and drivers based upon MIPI
DPI helpers. The driver for Xen explicitly disables no_vblank, as it has
its own logic for sending these events.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/bochs/bochs_kms.c   |  9 -
 drivers/gpu/drm/drm_mipi_dbi.c  |  9 -
 drivers/gpu/drm/drm_simple_kms_helper.c | 19 +++
 drivers/gpu/drm/tiny/gm12u320.c |  9 -
 drivers/gpu/drm/tiny/ili9225.c  |  9 -
 drivers/gpu/drm/tiny/repaper.c  |  9 -
 drivers/gpu/drm/tiny/st7586.c   |  9 -
 drivers/gpu/drm/udl/udl_modeset.c   | 11 ---
 drivers/gpu/drm/xen/xen_drm_front_kms.c | 13 +
 9 files changed, 28 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c 
b/drivers/gpu/drm/bochs/bochs_kms.c
index 3f0006c2470d..ff275faee88d 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -7,7 +7,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "bochs.h"
 
@@ -57,16 +56,8 @@ static void bochs_pipe_update(struct drm_simple_display_pipe 
*pipe,
  struct drm_plane_state *old_state)
 {
struct bochs_device *bochs = pipe->crtc.dev->dev_private;
-   struct drm_crtc *crtc = >crtc;
 
bochs_plane_update(bochs, pipe->plane.state);
-
-   if (crtc->state->event) {
-   spin_lock_irq(>dev->event_lock);
-   drm_crtc_send_vblank_event(crtc, crtc->state->event);
-   crtc->state->event = NULL;
-   spin_unlock_irq(>dev->event_lock);
-   }
 }
 
 static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = {
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 16bff1be4b8a..13b753cb3f67 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -24,7 +24,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 
 #define MIPI_DBI_MAX_SPI_READ_SPEED 200 /* 2MHz */
@@ -299,18 +298,10 @@ void mipi_dbi_pipe_update(struct drm_simple_display_pipe 
*pipe,
  struct drm_plane_state *old_state)
 {
struct drm_plane_state *state = pipe->plane.state;
-   struct drm_crtc *crtc = >crtc;
struct drm_rect rect;
 
if (drm_atomic_helper_damage_merged(old_state, state, ))
mipi_dbi_fb_dirty(state->fb, );
-
-   if (crtc->state->event) {
-   spin_lock_irq(>dev->event_lock);
-   drm_crtc_send_vblank_event(crtc, crtc->state->event);
-   spin_unlock_irq(>dev->event_lock);
-   crtc->state->event = NULL;
-   }
 }
 EXPORT_SYMBOL(mipi_dbi_pipe_update);
 
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 15fb516ae2d8..4414c7a5b2ce 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -146,10 +146,21 @@ static int drm_simple_kms_plane_atomic_check(struct 
drm_plane *plane,
if (!plane_state->visible)
return 0;
 
-   if (!pipe->funcs || !pipe->funcs->check)
-   return 0;
-
-   return pipe->funcs->check(pipe, plane_state, crtc_state);
+   if (pipe->funcs) {
+   if (pipe->funcs->check)
+   return pipe->funcs->check(pipe, plane_state,
+ crtc_state);
+   if (pipe->funcs->enable_vblank)
+   return 0;
+   }
+
+   /* Drivers without VBLANK support have to fake VBLANK events. As
+* there's no check() callback to enable this, set the no_vblank
+* field by default.
+*/
+   crtc_state->no_vblank = true;
+
+   return 0;
 }
 
 static void drm_simple_kms_plane_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 94fb1f593564..a48173441ae0 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -22,7 +22,6 @@
 #include 
 #include 
 #include 
-#include 
 
 static bool eco_mode;
 module_param(eco_mode, bool, 0644);
@@ -610,18 +609,10 @@ static void gm12u320_pipe_update(struct 
drm_simple_display_pipe *pipe,
 struct drm_plane_state *old_state)
 {
struct drm_plane_state *state = pipe->plane.state;
-   struct drm_crtc *crtc = >crtc;
struct drm_rect rect;
 
if (drm_atomic_helper_damage_merged(old_state, state, ))
gm12u320_fb_mark_dirty(pipe->plane.state->fb, );
-
-   if (crtc->state->event) {
-   

[Xen-devel] [PATCH v2 2/4] drm/ast: Set struct drm_crtc_state.no_vblank in atomic_check()

[Thomas Zimmermann]

CRTC state properties should be computed in atomic_check(). Do so for
the no_vblank field.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/ast/ast_mode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 34608f0499eb..ef7a0b08cc05 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -800,6 +800,8 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc 
*crtc,
return -EINVAL;
}
 
+   state->no_vblank = true;
+
ast_state = to_ast_crtc_state(state);
 
format = ast_state->format;
@@ -833,8 +835,6 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc 
*crtc,
struct ast_vbios_mode_info *vbios_mode_info;
struct drm_display_mode *adjusted_mode;
 
-   crtc->state->no_vblank = true;
-
ast_state = to_ast_crtc_state(crtc->state);
 
format = ast_state->format;
-- 
2.24.1


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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 12:53, Roger Pau Monné wrote:
> On Wed, Jan 15, 2020 at 12:40:27PM +0100, Jan Beulich wrote:
>> On 15.01.2020 10:47, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
 --- a/xen/arch/x86/time.c
 +++ b/xen/arch/x86/time.c
 @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
  
  void cstate_restore_tsc(void)
  {
 +struct cpu_time *t = _cpu(cpu_time);
 +
  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
  return;
  
 -write_tsc(stime2tsc(read_platform_stime(NULL)));
 +t->stamp.master_stime = read_platform_stime(NULL);
 +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
 +t->stamp.local_stime = t->stamp.master_stime;
 +
 +write_tsc(t->stamp.local_tsc);
>>>
>>> In order to avoid the TSC write (and the likely associated vmexit),
>>> could you instead do:
>>>
>>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
>>> t->stamp.local_tsc = rdtsc_ordered();
>>>
>>> I think it should achieve the same as it syncs the local TSC stamp and
>>> times, would avoid the TSC write and slightly simplifies the logic.
>>
>> Wouldn't this result in guests possibly observing the TSC moving
>> backwards?
> 
> Isn't local_tsc storing a TSC value read from the same CPU always, and
> hence could only go backwards if rdtsc actually goes backwards?

For one I have to admit I was (mistakenly) thinking of wakeup
from S states more than that from C states. So assuming the
TSC indeed only stops (but won't get e.g. restarted), backwards
moves ought to be excluded. What I'm then worried about is too
little progress observable by guests. The PV time protocol
ought to be fine in this regard (and consumers of raw TSC values
are on their own anyway), but wouldn't you need to update TSC
offsets of HVM guests in order to compensate for the elapsed
time?

> Ie: cpu_frequency_change seems to do something similar, together with
> a re-adjusting of the time scale, but doesn't perform any TSC write.

A P-state change at most alters the the tick rate, but wouldn't
stop or even reset the TSC (afaict).

Jan

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 12:39, Jan Beulich wrote:
> On 15.01.2020 13:28, Igor Druzhinin wrote:
>> On 15/01/2020 11:32, Jan Beulich wrote:
>>> On 14.01.2020 20:36, Igor Druzhinin wrote:
 If ITSC is not available on CPU (e.g if running nested as PV shim)
 then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
 all AMD and some old Intel processors. In which case TSC would need to
 be restored on CPU from platform time by Xen upon exiting deep C-states.
>>>
>>> How does waking from deep C states correspond to the PV shim? I notice
>>> that cstate_restore_tsc() gets called irrespective of the C state being
>>> exited, so I wonder whether there's room for improvement there
>>> independent of the issue at hand. As far as this change is concerned,
>>> I think you want to drop the notion of "deep" from the description.
>>
>> I'm not familiar with what to call "deep C-state" so for me it was anything
>> higher than C1. If you prefer "deep" to be dropped - so be it.
> 
> "Higher than C1" may be fine (albeit I vaguely recall TSC issues
> starting with C3 only), but at least mwait_idle() calls the
> function even for C1. As to the PV shim - does it know about any
> C-states at all (beyond HLT-invoked C1)?

Yes, PV-shim knows about C states as it looks they are tied to
processor ID in some cases. For AMD specifically C2 is HLT.

Igor

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 13:31, Igor Druzhinin wrote:
> On 15/01/2020 12:25, Igor Druzhinin wrote:
>> On 15/01/2020 11:40, Jan Beulich wrote:
>>> On 15.01.2020 10:47, Roger Pau Monné wrote:
 On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>  
>  void cstate_restore_tsc(void)
>  {
> +struct cpu_time *t = _cpu(cpu_time);
> +
>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>  return;
>  
> -write_tsc(stime2tsc(read_platform_stime(NULL)));
> +t->stamp.master_stime = read_platform_stime(NULL);
> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
> +t->stamp.local_stime = t->stamp.master_stime;
> +
> +write_tsc(t->stamp.local_tsc);

 In order to avoid the TSC write (and the likely associated vmexit),
 could you instead do:

 t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
 t->stamp.local_tsc = rdtsc_ordered();

 I think it should achieve the same as it syncs the local TSC stamp and
 times, would avoid the TSC write and slightly simplifies the logic.
>>>
>>> Wouldn't this result in guests possibly observing the TSC moving
>>> backwards?
>>
>> Yes, I think so. Would restoring from TSC stamp if it's higher than
>> platform time better you think?
>>
> 
> Ignore my reply. I was thinking you're asking whether the original code
> would do such a thing. Although I'm concerned if what you say actually
> applies to the original code as well. Would you think the existing logic
> handles it already?

I think the original code won't guarantee to backwards move at
all, but it also wouldn't produce large backwards jumps. It's
large ones I was mainly thinking of when replying to Roger.
But let me also reply back to him...

Jan

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

Re: [Xen-devel] [PATCH] xen/list: Remove prefetching

[Andrew Cooper]

On 15/01/2020 11:17, Jan Beulich wrote:
> On 14.01.2020 21:35, Andrew Cooper wrote:
>> Xen inherited its list infrastructure from Linux.  One area where has fallen
>> behind is that of prefetching, which as it turns out is a performance penalty
>> in most cases.
>>
>> Prefetch of NULL on x86 is now widely measured to have glacial performance
>> properties, and will unconditionally hit on every hlist use due to the
>> termination condition.
>>
>> Cross-port the following Linux patches:
>>
>>   75d65a425c (2011) "hlist: remove software prefetching in hlist iterators"
>>   e66eed651f (2011) "list: remove prefetching from regular list iterators"
>>   c0d15cc7ee (2013) "linked-list: Remove __list_for_each"
> Just as an observation (not an objection), the 2nd of these says
> "normally the downsides are bigger than the upsides", which makes
> it unbelievably clear what these supposed downsides are. I can
> accept prefetches through NULL to be harmful. I can also accept
> prefetches on single entry lists to not be very useful. But does
> this also render them useless on long lists with not overly much
> cache churn done by the body of the iteration loop?

Yes.

Prefetch is only useful when you're making an access which none of the
hardware prefetchers can predict, and that the costs (extra instruction,
L1 cache perturbance, and tying up the pagewalker for a while) are
outweighed by the perf improvement from not stalling against the access.

A programmer cannot figure this out by just looking at the C.  The
details are micro-architectural, and based on rare and unpredictable
data access patterns.  (Incorrectly) tying up the pagewalker early can
be far more detrimental to performance than to have forward speculation
pull it in at the next time that there is available micro-architectural
resource to do so.

> Wouldn't it
> at least be worthwhile to have list_for_each_prefetch() retaining
> prior behavior, and use it in places where prefetching can be
> deemed to help?

No, I don't think so.  The repetitive pattern of a loop is easy for
hardware to spot.

The cases where prefetching helps in practice are the one-off totally
unpredictable accesses which are suddenly going to block all other
instructions in flight, *and* you are not going to incur a TLB miss in
the short term.

This is why I made the prefetch() suggestion for your svm_load_segs()
code.  The memory operand is used once per context switch, so very
likely to have fallen out of the cache and TLB, and VMLOAD is
microcoded, so a stalling black box as far as forward speculation goes. 
As the code leading up to it is operating in hot TLB mappings, the
pagewalker is free ahead of time to complete the fill.

There are cases where prefetch() really makes a difference, but they are
rare and the hardware vendors have already optimised the common data
access patterns in programs.

It is also highly telling that in nearly a decade, Linux still hasn't
found a case warranting the re-introduction of prefetches on the loop
entry metadata.

Of course, if someone does find a case, we can reconsider, but I doubt
it will ever come up, and misuse of such a list iterator can easily do
more damage than good.

~Andrew

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 13:28, Igor Druzhinin wrote:
> On 15/01/2020 11:32, Jan Beulich wrote:
>> On 14.01.2020 20:36, Igor Druzhinin wrote:
>>> If ITSC is not available on CPU (e.g if running nested as PV shim)
>>> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
>>> all AMD and some old Intel processors. In which case TSC would need to
>>> be restored on CPU from platform time by Xen upon exiting deep C-states.
>>
>> How does waking from deep C states correspond to the PV shim? I notice
>> that cstate_restore_tsc() gets called irrespective of the C state being
>> exited, so I wonder whether there's room for improvement there
>> independent of the issue at hand. As far as this change is concerned,
>> I think you want to drop the notion of "deep" from the description.
> 
> I'm not familiar with what to call "deep C-state" so for me it was anything
> higher than C1. If you prefer "deep" to be dropped - so be it.

"Higher than C1" may be fine (albeit I vaguely recall TSC issues
starting with C3 only), but at least mwait_idle() calls the
function even for C1. As to the PV shim - does it know about any
C-states at all (beyond HLT-invoked C1)?

Jan

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 09:47, Roger Pau Monné wrote:
> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
>> If ITSC is not available on CPU (e.g if running nested as PV shim)
>> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
>> all AMD and some old Intel processors. In which case TSC would need to
>> be restored on CPU from platform time by Xen upon exiting deep C-states.
>>
>> As platform time might be behind the last TSC stamp recorded for the
>> current CPU, invariant of TSC stamp being always behind local TSC counter
>> is violated. This has an effect of get_s_time() going negative resulting
>> in eventual system hang or crash.
>>
>> Fix this issue by updating local TSC stamp along with TSC counter write.
> 
> Thanks! I haven't seen such issue because I've been running the shim
> with nomigrate in order to prevent the vTSC overhead.
> 
>>
>> Signed-off-by: Igor Druzhinin 
>> ---
>> This caused reliable hangs of shim domains with multiple vCPUs on all AMD
>> systems. The problem got also reproduced on bare-metal by artifically
>> masking ITSC feature bit. The proposed fix has been verified for both
>> cases.
>> ---
>>  xen/arch/x86/time.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
>> index e79cb4d..f6b26f8 100644
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>>  
>>  void cstate_restore_tsc(void)
>>  {
>> +struct cpu_time *t = _cpu(cpu_time);
>> +
>>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>>  return;
>>  
>> -write_tsc(stime2tsc(read_platform_stime(NULL)));
>> +t->stamp.master_stime = read_platform_stime(NULL);
>> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
>> +t->stamp.local_stime = t->stamp.master_stime;
>> +
>> +write_tsc(t->stamp.local_tsc);
> 
> In order to avoid the TSC write (and the likely associated vmexit),
> could you instead do:
> 
> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> t->stamp.local_tsc = rdtsc_ordered();

I think in that case RDTSC might return something behind platform time
which is not right I guess.

Igor

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 12:25, Igor Druzhinin wrote:
> On 15/01/2020 11:40, Jan Beulich wrote:
>> On 15.01.2020 10:47, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
 --- a/xen/arch/x86/time.c
 +++ b/xen/arch/x86/time.c
 @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
  
  void cstate_restore_tsc(void)
  {
 +struct cpu_time *t = _cpu(cpu_time);
 +
  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
  return;
  
 -write_tsc(stime2tsc(read_platform_stime(NULL)));
 +t->stamp.master_stime = read_platform_stime(NULL);
 +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
 +t->stamp.local_stime = t->stamp.master_stime;
 +
 +write_tsc(t->stamp.local_tsc);
>>>
>>> In order to avoid the TSC write (and the likely associated vmexit),
>>> could you instead do:
>>>
>>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
>>> t->stamp.local_tsc = rdtsc_ordered();
>>>
>>> I think it should achieve the same as it syncs the local TSC stamp and
>>> times, would avoid the TSC write and slightly simplifies the logic.
>>
>> Wouldn't this result in guests possibly observing the TSC moving
>> backwards?
> 
> Yes, I think so. Would restoring from TSC stamp if it's higher than
> platform time better you think?
> 

Ignore my reply. I was thinking you're asking whether the original code
would do such a thing. Although I'm concerned if what you say actually
applies to the original code as well. Would you think the existing logic
handles it already?

Igor

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 11:32, Jan Beulich wrote:
> On 14.01.2020 20:36, Igor Druzhinin wrote:
>> If ITSC is not available on CPU (e.g if running nested as PV shim)
>> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
>> all AMD and some old Intel processors. In which case TSC would need to
>> be restored on CPU from platform time by Xen upon exiting deep C-states.
> 
> How does waking from deep C states correspond to the PV shim? I notice
> that cstate_restore_tsc() gets called irrespective of the C state being
> exited, so I wonder whether there's room for improvement there
> independent of the issue at hand. As far as this change is concerned,
> I think you want to drop the notion of "deep" from the description.

I'm not familiar with what to call "deep C-state" so for me it was anything
higher than C1. If you prefer "deep" to be dropped - so be it.

Igor

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Igor Druzhinin]

On 15/01/2020 11:40, Jan Beulich wrote:
> On 15.01.2020 10:47, Roger Pau Monné wrote:
>> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
>>> --- a/xen/arch/x86/time.c
>>> +++ b/xen/arch/x86/time.c
>>> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>>>  
>>>  void cstate_restore_tsc(void)
>>>  {
>>> +struct cpu_time *t = _cpu(cpu_time);
>>> +
>>>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>>>  return;
>>>  
>>> -write_tsc(stime2tsc(read_platform_stime(NULL)));
>>> +t->stamp.master_stime = read_platform_stime(NULL);
>>> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
>>> +t->stamp.local_stime = t->stamp.master_stime;
>>> +
>>> +write_tsc(t->stamp.local_tsc);
>>
>> In order to avoid the TSC write (and the likely associated vmexit),
>> could you instead do:
>>
>> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
>> t->stamp.local_tsc = rdtsc_ordered();
>>
>> I think it should achieve the same as it syncs the local TSC stamp and
>> times, would avoid the TSC write and slightly simplifies the logic.
> 
> Wouldn't this result in guests possibly observing the TSC moving
> backwards?

Yes, I think so. Would restoring from TSC stamp if it's higher than
platform time better you think?

Igor

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

Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice

[Lars Kurth]

On 15/01/2020, 10:47, "George Dunlap"  wrote:

On 1/13/20 9:21 PM, Lars Kurth wrote:
> 
> 
> On 13/01/2020, 19:54, "George Dunlap"  wrote:
> 
> 
> > On Dec 30, 2019, at 7:32 PM, Lars Kurth  
wrote:
> > 
> > From: Lars Kurth 
> > 
> > This guide covers the bulk on Best Practice related to code review
> > It primarily focusses on code review interactions
> > It also covers how to deal with Misunderstandings and Cultural
> > Differences
> > 
> > +### Avoid opinion: stick to the facts
> 
> In my talk on this subject I said “Avoid *inflammatory language*”.  
At some level it’s good to have strong opinions on what code should look like.  
It’s not opinions that are a problem, or even expressing opinions, but 
expressing them in a provocative or inflammatory way.
> 
> Let me look at this again: I don't feel strongly about it
> 
> I changed the title because I felt that the bulk of the 
> example is actually about sticking to the facts an opinion 
> and the inflammatory element was secondary. So it felt more
> natural to me to change the title.

Right; the point though specifically is that people's natural, and
probably healthy response to poorly-written code, or to
inconsiderately-written patch series in any way, is to use charged
language.  I wouldn't call any code "garbage", but code submitted is
sometimes actually terrible, fragile, spaghetti, inefficient, racy,
messy -- whatever bad things you can say about it -- and any
well-trained developer will have the same opinion.

[snip]

I think people should be able to pick up what we mean from the reasoning
and from the examples.

I attached a conversation log on IRC and a diff against this snippet of the 
code for a resolution

‹lars_kurth›  gwd: I just read your feedback on the CoC. I now agree with your 
argument that "Avoid opinion:  stick to the facts" is a bad heading for that 
section 
‹lars_kurth›  gwd: however I still dont like “Avoid *inflammatory language*” - 
I am wondering whether "Avoid language that triggers a negative response" would 
be better 
‹gwd›   What is it you don't like about "inflammatory"? 
‹lars_kurth›  Also, I think I need to re-write some of the bridging paragraphs 
to fit the title 
‹gwd›  (Not arguing for 'inflammatory' per se, but knowing what you don't like 
about it helps if I'm trying to find an alternative) 
‹lars_kurth›  Firstly it is now somewhat politically charged (in some 
cultures), secondly I am not sure how well it translates and how clear it is to 
non-native english speakers 
‹gwd›  Any opinions on the other words I suggested? 
‹lars_kurth›  Provocative seems ok 
* Diziet reads the thread.
‹lars_kurth›  "charged"? "loaded"? seems too generic 
‹lars_kurth›  "derogatory"? "contemptuous"? seems to be too harsh and infer too 
much bad intent
‹Diziet›  "avoid ... emotive" maybe ? [11:18:15][11:18:31]  
‹Diziet›  "avoid derogatory or emotive language" ? 
‹lars_kurth›  Diziet, gwd: I think emotive is good and we can add derogatory 
‹gwd›  Doesn't "emotive" include positive emotions? "This patch is amazing, 
thank you" is a lot better than "This patch effictively simplifies this 
codebase very well, thank you". :-) 
‹lars_kurth›  That is true 
‹lars_kurth›  The same would be true for charged and loaded 
‹Diziet›  gwd: Hrm
‹Diziet›  To be unambiguous I think only "negatively charged" will do. You 
can't have "negatively emotive" or some such. 
‹Diziet›  You could say "avoid emotive criticism" 
‹gwd›  I feel like "charged" is used more often for negative things.
‹lars_kurth›  OK. Let's stick with Inflammatory and I can replace "Key to this 
is what we call **stick to the facts**. The same is true when a patch author is 
responding to a comment from a reviewer." in the first paragraph with a 
sentence that clarifies that the intention is to avoid triggering negativity 
‹lars_kurth›  I am going to draft some text for this section and send it in 
response rather than doing a new version for now 
‹gwd›  +
‹Diziet›  I think `derogatory' and `emotive criticism' and `negatively charged' 
are all better than `inflammatory'. 
‹Diziet›  But `inflammatory' will do.
‹lars_kurth›  The section as it is comes across as a little clumsy (in that it 
doesn't flow well
‹lars_kurth›  As an aside: does anyone know how I can redact text in markdown? 
I guess I can just add "" for words I dont want to show 
‹Diziet›  https://stackoverflow.com/questions/4823468/comments-in-markdown 
‹gwd›  lars_kurth: That's what I would do. (Although I would use [], which are 
more traditional for edits to quoted text.)
‹Diziet›  Oh I see, we're talking about the oebxra gbrf thing
‹Diziet›  I would just write literally [redacted].
‹Diziet›  Or rot13 it as I just did but the audience of the CoC will have no 
idea what that is even if you add a 

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Roger Pau Monné]

On Wed, Jan 15, 2020 at 12:40:27PM +0100, Jan Beulich wrote:
> On 15.01.2020 10:47, Roger Pau Monné wrote:
> > On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
> >> --- a/xen/arch/x86/time.c
> >> +++ b/xen/arch/x86/time.c
> >> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
> >>  
> >>  void cstate_restore_tsc(void)
> >>  {
> >> +struct cpu_time *t = _cpu(cpu_time);
> >> +
> >>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
> >>  return;
> >>  
> >> -write_tsc(stime2tsc(read_platform_stime(NULL)));
> >> +t->stamp.master_stime = read_platform_stime(NULL);
> >> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
> >> +t->stamp.local_stime = t->stamp.master_stime;
> >> +
> >> +write_tsc(t->stamp.local_tsc);
> > 
> > In order to avoid the TSC write (and the likely associated vmexit),
> > could you instead do:
> > 
> > t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> > t->stamp.local_tsc = rdtsc_ordered();
> > 
> > I think it should achieve the same as it syncs the local TSC stamp and
> > times, would avoid the TSC write and slightly simplifies the logic.
> 
> Wouldn't this result in guests possibly observing the TSC moving
> backwards?

Isn't local_tsc storing a TSC value read from the same CPU always, and
hence could only go backwards if rdtsc actually goes backwards?

Ie: cpu_frequency_change seems to do something similar, together with
a re-adjusting of the time scale, but doesn't perform any TSC write.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 15.01.2020 10:47, Roger Pau Monné wrote:
> On Tue, Jan 14, 2020 at 07:36:21PM +, Igor Druzhinin wrote:
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -955,10 +955,16 @@ u64 stime2tsc(s_time_t stime)
>>  
>>  void cstate_restore_tsc(void)
>>  {
>> +struct cpu_time *t = _cpu(cpu_time);
>> +
>>  if ( boot_cpu_has(X86_FEATURE_NONSTOP_TSC) )
>>  return;
>>  
>> -write_tsc(stime2tsc(read_platform_stime(NULL)));
>> +t->stamp.master_stime = read_platform_stime(NULL);
>> +t->stamp.local_tsc = stime2tsc(t->stamp.master_stime);
>> +t->stamp.local_stime = t->stamp.master_stime;
>> +
>> +write_tsc(t->stamp.local_tsc);
> 
> In order to avoid the TSC write (and the likely associated vmexit),
> could you instead do:
> 
> t->stamp.local_stime = t->stamp.master_stime = read_platform_stime(NULL);
> t->stamp.local_tsc = rdtsc_ordered();
> 
> I think it should achieve the same as it syncs the local TSC stamp and
> times, would avoid the TSC write and slightly simplifies the logic.

Wouldn't this result in guests possibly observing the TSC moving
backwards?

Jan

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

Re: [Xen-devel] [PATCH] x86/time: update TSC stamp on restore from deep C-state

[Jan Beulich]

On 14.01.2020 20:36, Igor Druzhinin wrote:
> If ITSC is not available on CPU (e.g if running nested as PV shim)
> then X86_FEATURE_NONSTOP_TSC is not advertised in certain cases, i.e.
> all AMD and some old Intel processors. In which case TSC would need to
> be restored on CPU from platform time by Xen upon exiting deep C-states.

How does waking from deep C states correspond to the PV shim? I notice
that cstate_restore_tsc() gets called irrespective of the C state being
exited, so I wonder whether there's room for improvement there
independent of the issue at hand. As far as this change is concerned,
I think you want to drop the notion of "deep" from the description.

Jan

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

Re: [Xen-devel] [PATCH] xen/list: Remove prefetching

[Andrew Cooper]

On 15/01/2020 10:39, Roger Pau Monné wrote:
> On Tue, Jan 14, 2020 at 08:35:45PM +, Andrew Cooper wrote:
>> Xen inherited its list infrastructure from Linux.  One area where has fallen
>> behind is that of prefetching, which as it turns out is a performance penalty
>> in most cases.
>>
>> Prefetch of NULL on x86 is now widely measured to have glacial performance
>> properties, and will unconditionally hit on every hlist use due to the
>> termination condition.
>>
>> Cross-port the following Linux patches:
>>
>>   75d65a425c (2011) "hlist: remove software prefetching in hlist iterators"
>>   e66eed651f (2011) "list: remove prefetching from regular list iterators"
>>   c0d15cc7ee (2013) "linked-list: Remove __list_for_each"
>>
>> to Xen, which results in the following net diffstat on x86:
>>
>>   add/remove: 0/1 grow/shrink: 27/83 up/down: 576/-1648 (-1072)
>>
>> (The code additions comes from a few now-inlined functions, and slightly
>> different basic block padding.)
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper 
> Reviewed-by: Roger Pau Monné 
>
> Has this gone through some XenRT performance testing to assert there
> are not regressions performance wise?

No.  The Linux measurements are still valid observations.

~Andrew

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

Re: [Xen-devel] [PATCH] xen/list: Remove prefetching

[Jan Beulich]

On 14.01.2020 21:35, Andrew Cooper wrote:
> Xen inherited its list infrastructure from Linux.  One area where has fallen
> behind is that of prefetching, which as it turns out is a performance penalty
> in most cases.
> 
> Prefetch of NULL on x86 is now widely measured to have glacial performance
> properties, and will unconditionally hit on every hlist use due to the
> termination condition.
> 
> Cross-port the following Linux patches:
> 
>   75d65a425c (2011) "hlist: remove software prefetching in hlist iterators"
>   e66eed651f (2011) "list: remove prefetching from regular list iterators"
>   c0d15cc7ee (2013) "linked-list: Remove __list_for_each"

Just as an observation (not an objection), the 2nd of these says
"normally the downsides are bigger than the upsides", which makes
it unbelievably clear what these supposed downsides are. I can
accept prefetches through NULL to be harmful. I can also accept
prefetches on single entry lists to not be very useful. But does
this also render them useless on long lists with not overly much
cache churn done by the body of the iteration loop? Wouldn't it
at least be worthwhile to have list_for_each_prefetch() retaining
prior behavior, and use it in places where prefetching can be
deemed to help?

Jan

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

[Xen-devel] Issues/improvements performing flush of guest TLBs

[Roger Pau Monné]

Hello,

For the last days I've been trying to figure out how to properly
perform flushes of guest TLBs from the hypervisor. We currently
provide a hypercall to HVM guests (HVMOP_flush_tlbs). The hypercall
however pauses all vCPUs that require a flush and then call
paging_update_cr3, which is highly inefficient. The performance of
such implementation on a non-overloaded environment seems to be on par
with the guest issuing IPIs and performing cr3/cr4 writes in order to
flush, which makes the point of providing such hypercall moot.

I would like to provide hooks (in the paging_mode struct) for the HAP
and Shadow paging modes in order to perform guest TLB flushes:

 - HAP: depends on whether ASID/VPID is in use. If not in use the
   TLBs will be flushed on each vmexit/vmenter. If in use changing the
   VPID/ASID or flushing the specific VPID/ASID should be enough. This
   requires calling hvm_asid_flush_vcpu for each vCPU to be flushed
   and issuing an IPI to the currently running vCPUs in order to
   trigger a vmexit that will sync the VPID/ASID with the value in the
   vmcs/vmcb. Ie:

for_each_vcpu( v, d )
hvm_asid_flush_vcpu(v);
on_selected_cpus();

 - Shadow: it's not clear to me exactly which parts of sh_update_cr3
   are needed in order to perform a guest TLB flush. I think calling:

#if (SHADOW_OPTIMIZATIONS & SHOPT_VIRTUAL_TLB)
/* No longer safe to use cached gva->gfn translations */
vtlb_flush(v);
#endif
#if (SHADOW_OPTIMIZATIONS & SHOPT_OUT_OF_SYNC)
/* Need to resync all the shadow entries on a TLB flush. */
shadow_resync_current_vcpu(v);
#endif

if ( is_hvm_domain(d) )
/*
 * Linear mappings might be cached in non-root mode when ASID/VPID is
 * in use and hence they need to be flushed here.
 */
hvm_asid_flush_vcpu(v);

   Should be enough but I'm not very familiar with the shadow code,
   and hence would like some feedback from someone more familiar with
   shadow in order to assert exactly what's required to perform a
   guest TLB flush.

   Also, AFAICT sh_update_cr3 is not safe to be called on vCPUs
   currently running on remote pCPUs, albeit there are no assertions
   to that end. It's also not clear which parts of sh_update_cr3 are
   safe to be called while the vCPU is running.

FWIW, there also seems to be a lot of unneeded flushes of HVM guests
TLB, as do_tlb_flush will unconditionally clear all HVM guest TLBs on
the pCPU by calling hvm_asid_flush_core which I don't think it's
necessary/intended by quite a lot of the Xen TLB flush callers. I
guess this would also warrant a different discussion, as there seems
to be room for improvement in this area.

Thanks, Roger.

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

Re: [Xen-devel] [PATCH v1 1/4] kasan: introduce set_pmd_early_shadow()

[Jürgen Groß]

On 15.01.20 11:54, Sergey Dyasli wrote:

Hi Juergen,

On 08/01/2020 15:20, Sergey Dyasli wrote:

It is incorrect to call pmd_populate_kernel() multiple times for the
same page table. Xen notices it during kasan_populate_early_shadow():

 (XEN) mm.c:3222:d155v0 mfn 3704b already pinned

This happens for kasan_early_shadow_pte when USE_SPLIT_PTE_PTLOCKS is
enabled. Fix this by introducing set_pmd_early_shadow() which calls
pmd_populate_kernel() only once and uses set_pmd() afterwards.

Signed-off-by: Sergey Dyasli 


Looks like the plan to use set_pmd() directly has failed: it's an
arch-specific function and can't be used in arch-independent code
(as kbuild test robot has proven).

Do you see any way out of this other than disabling SPLIT_PTE_PTLOCKS
for PV KASAN?


Change set_pmd_early_shadow() like the following:

#ifdef CONFIG_XEN_PV
static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
{
static bool pmd_populated = false;

if (likely(pmd_populated)) {
set_pmd(pmd, __pmd(__pa(early_shadow) | _PAGE_TABLE));
} else {
pmd_populate_kernel(_mm, pmd, early_shadow);
pmd_populated = true;
}
}
#else
static inline void set_pmd_early_shadow(pmd_t *pmd, pte_t *early_shadow)
{
pmd_populate_kernel(_mm, pmd, early_shadow);
}
#endif

... and move it to include/xen/xen-ops.h and call it with
lm_alias(kasan_early_shadow_pte) as the second parameter.


Juergen

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

Re: [Xen-devel] Having a DOM-U guest with 1:1 mapping in the second stage MMU.

[Julien Grall]

On 14/01/2020 21:39, Jorge Pereira wrote:

Hi Guys,


Hello,



I’m currently using XEN in order to run side-by-side a DOM-0 with a 
DOM-U guest. My use-case scenario requires in the DOM-U direct access to 
some dma-capable devices such ethernet and some GPUs.


Since our target platform (i.MX8MM) does not support IOMMU, we can’t 
assign dma-capable devices to the DOM-U guest because XEN does not 
create 1:1 mapping for that guest in the 2^nd stage MMU. So, 
guest-virtual addresses are different than the physical ones.


Bear in mind this setup is going to be insecure unless you have another 
way to prevent your passthrough-ed device to access memory it should not 
(e.g an MPU).



Is it possible to have 1:1 mapping for DOM-U guests?


It is not possible at the moment. There are been various effort to try 
to do it, but I have always push back as this is actively defeating the 
purposing of an hypervisor.


This would be a different story if we had support for MPU in Xen.

If not, I’m 
interested to know what would be the estimated effort to support this 
feature?


I think you have someone else in NXP looking at 1:1 mapping for Xen (in 
CC). I provided to Andrei some tips how to get 1:1 mapping for DomU 
using dom0less in December (see [1]). So you may want to sync-up with 
him here.


If you are looking at 1:1 DomU using xl, then it is going to require 
more work as the hypercall allocating memory is based on guest frame 
number. There was a thread on the ML a few years ago, I can try to dig 
it down if you are interested.


Cheers,

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01364.html




Thanks in advance,

Cheers,

Jorge


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



--
Julien Grall

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

Re: [Xen-devel] [PATCH v1 4/4] xen/netback: Fix grant copy across page boundary with KASAN

[Sergey Dyasli]

On 09/01/2020 10:33, Vlastimil Babka wrote:
> On 1/8/20 4:21 PM, Sergey Dyasli wrote:
>> From: Ross Lagerwall 
>>
>> When KASAN (or SLUB_DEBUG) is turned on, the normal expectation that
>> allocations are aligned to the next power of 2 of the size does not
>> hold.
>
> Hmm, really? They should after 59bb47985c1d ("mm, sl[aou]b: guarantee
> natural alignment for kmalloc(power-of-two)"), i.e. since 5.4.
>
> But actually the guarantee is only for precise power of two sizes given
> to kmalloc(). Allocations of sizes that end up using the 96 or 192 bytes
> kmalloc cache have no such guarantee. But those might then cross page
> boundary also without SLUB_DEBUG.

That's interesting to know. It's certainly not the case for 4.19 kernel
for which PV KASAN was initially developed. But I guess this means that
only patch description needs updating.

>
>> Therefore, handle grant copies that cross page boundaries.
>>
>> Signed-off-by: Ross Lagerwall 
>> Signed-off-by: Sergey Dyasli 

--
Thanks,
Sergey

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

Re: [Xen-devel] [PATCH v4 11/16] tools: add simple vchan-socket-proxy

[Jan Beulich]

On 15.01.2020 03:39, Marek Marczykowski-Górecki wrote:
> Add a simple proxy for tunneling socket connection over vchan. This is
> based on existing vchan-node* applications, but extended with socket
> support. vchan-socket-proxy serves both as a client and as a server,
> depending on parameters. It can be used to transparently communicate
> with an application in another domian that normally expose UNIX socket
> interface. Specifically, it's written to communicate with qemu running
> within stubdom.
> 
> Server mode listens for vchan connections and when one is opened,
> connects to a pointed UNIX socket.  Client mode listens on UNIX
> socket and when someone connects, opens a vchan connection.  Only
> a single connection at a time is supported.
> 
> Additionally, socket can be provided as a number - in which case it's
> interpreted as already open FD (in case of UNIX listening socket -
> listen() needs to be already called). Or "-" meaning stdin/stdout - in
> which case it is reduced to vchan-node2 functionality.
> 
> Example usage:
> 
> 1. (in dom0) vchan-socket-proxy --mode=client 
> /local/domain//data/vchan/1234 /run/qemu.(DOMID)
> 
> 2. (in DOMID) vchan-socket-proxy --mode=server 0
>/local/domain//data/vchan/1234 /run/qemu.(DOMID)
> 
> This will listen on /run/qemu.(DOMID) in dom0 and whenever connection is
> made, it will connect to DOMID, where server process will connect to
> /run/qemu.(DOMID) there. When client disconnects, vchan connection is
> terminated and server vchan-socket-proxy process also disconnects from
> qemu.
> 
> Signed-off-by: Marek Marczykowski-Górecki 
> ---
>  .gitignore  |   1 +-

I guess this is why various non-tool-stack maintainers have
been Cc-ed. It would have been nice if you had stripped the
unnecessary Cc-s. I don't think ./MAINTAINERS can properly
express a suitable rule of Cc REST if the adjustment is not
simply accompanying the addition of some new output file.

>  tools/libvchan/Makefile |   7 +-
>  tools/libvchan/init.c.rej   |  60 -

Now since I've been Cc-ed, I'd like to ask what this is about.

Jan

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

Re: [Xen-devel] [PATCH v1 1/4] kasan: introduce set_pmd_early_shadow()

[Sergey Dyasli]

Hi Juergen,

On 08/01/2020 15:20, Sergey Dyasli wrote:
> It is incorrect to call pmd_populate_kernel() multiple times for the
> same page table. Xen notices it during kasan_populate_early_shadow():
>
> (XEN) mm.c:3222:d155v0 mfn 3704b already pinned
>
> This happens for kasan_early_shadow_pte when USE_SPLIT_PTE_PTLOCKS is
> enabled. Fix this by introducing set_pmd_early_shadow() which calls
> pmd_populate_kernel() only once and uses set_pmd() afterwards.
>
> Signed-off-by: Sergey Dyasli 

Looks like the plan to use set_pmd() directly has failed: it's an
arch-specific function and can't be used in arch-independent code
(as kbuild test robot has proven).

Do you see any way out of this other than disabling SPLIT_PTE_PTLOCKS
for PV KASAN?

--
Thanks,
Sergey

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

Re: [Xen-devel] [PATCH v4 6/7] Add guide on Communication Best Practice

[George Dunlap]

On 1/13/20 9:21 PM, Lars Kurth wrote:
> 
> 
> On 13/01/2020, 19:54, "George Dunlap"  wrote:
> 
> 
> > On Dec 30, 2019, at 7:32 PM, Lars Kurth  
> wrote:
> > 
> > From: Lars Kurth 
> > 
> > This guide covers the bulk on Best Practice related to code review
> > It primarily focusses on code review interactions
> > It also covers how to deal with Misunderstandings and Cultural
> > Differences
> > 
> > +### Avoid opinion: stick to the facts
> 
> In my talk on this subject I said “Avoid *inflammatory language*”.  At 
> some level it’s good to have strong opinions on what code should look like.  
> It’s not opinions that are a problem, or even expressing opinions, but 
> expressing them in a provocative or inflammatory way.
> 
> Let me look at this again: I don't feel strongly about it
> 
> I changed the title because I felt that the bulk of the 
> example is actually about sticking to the facts an opinion 
> and the inflammatory element was secondary. So it felt more
> natural to me to change the title.

Right; the point though specifically is that people's natural, and
probably healthy response to poorly-written code, or to
inconsiderately-written patch series in any way, is to use charged
language.  I wouldn't call any code "garbage", but code submitted is
sometimes actually terrible, fragile, spaghetti, inefficient, racy,
messy -- whatever bad things you can say about it -- and any
well-trained developer will have the same opinion.

It's not a problem at all to have opinions on code; I think that's a
prerequisite for being a good developer.  It's also not a problem at all
to say, "This code is great" or something positive about the submitter;
nor is it a problem to talk *together* about something not written by
the submitter ("Wow, this code you're trying to fix is a mess.")  The
point specifically is to avoid things which are likely to provoke a
negative emotional response in the submitter.

> But then looking at the definition of inflammatory language,
> aka  "an inflammatory question or an inflammatory statement
> would be one which would somehow predispose the listeners
> towards a subject in an unreasonable, prejudiced way."
> It is clearly also true that the example is inflammatory.
> 
> I think I may have tripped over an area where there is no good
> language match: the German translations of inflammatory
> aufrührerisch & aufwieglerisch have an element of rebellion
> and mischief to them (at least when I grew up).

"Provocative"? "charged"? "loaded"?  "derogatory"? "contemptuous"?

> I am wondering though, whether it is necessary to include 
> a definition of an inflammatory question or an inflammatory
> statement if we stick with it in the title

I think people should be able to pick up what we mean from the reasoning
and from the examples.

 -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/x86: Rework inclusion between struct pirq and struct hvm_pirq_dpci

[Jan Beulich]

On 14.01.2020 18:03, Julien Grall wrote:
> On 14/01/2020 16:50, Jan Beulich wrote:
>> On 14.01.2020 17:26, Julien Grall wrote:
>>> On 14/01/2020 16:08, Jan Beulich wrote:
 On 13.01.2020 22:33, Julien Grall wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -29,7 +29,8 @@
>
>bool hvm_domain_use_pirq(const struct domain *d, const struct pirq 
> *pirq)
>{
> -return is_hvm_domain(d) && pirq && pirq->arch.hvm.emuirq != 
> IRQ_UNBOUND;
> +return is_hvm_domain(d) && pirq &&
> +const_pirq_dpci(pirq)->emuirq != IRQ_UNBOUND;
>}
>
>/* Must be called with hvm_domain->irq_lock hold */
> @@ -396,7 +397,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, 
> uint32_t data)
>struct pirq *info = pirq_info(d, pirq);
>
>/* if it is the first time, allocate the pirq */
> -if ( !info || info->arch.hvm.emuirq == IRQ_UNBOUND )
> +if ( !info || pirq_dpci(info)->emuirq == IRQ_UNBOUND )
>{
>int rc;
>
> @@ -409,7 +410,7 @@ int hvm_inject_msi(struct domain *d, uint64_t addr, 
> uint32_t data)
>if ( !info )
>return -EBUSY;
>}
> -else if ( info->arch.hvm.emuirq != IRQ_MSI_EMU )
> +else if ( pirq_dpci(info)->emuirq != IRQ_MSI_EMU )
>return -EINVAL;
>send_guest_pirq(d, info);
>return 0;

 All of these uses (and others further down) make pretty clear
 that the emuirq field doesn't belong in the structure you put it
 in - the 'd' in dpci stands for "direct" afaik, and the field is
 for a certain variant of emulation of interrupt delivery into
 guests, i.e. not really pass-through focused at all.
>>>
>>> I am happy to keep emuirq in struct pirq if you are happy with slightly
>>> increasing the size allocated on PV.
>>>
>>> The main thing I want to get rid of is the weird allocation size we do
>>> today.
>>
>> While I understand this, to be honest I'd rather not see the size
>> grow for no good (to PV) reason. I don't think the current model is
>> _this_ bad.
> 
> Well, I did lost two days debugging a problem because of the allocation 
> (the memory were getting corrupted randomly). The comment you added may 
> help to avoid this problem but I still think that trying to allocate 
> half a pirq is a pretty bad idea.

To me, not significantly different from your container_of() approach.

>> But if you really want to push for it, why can't the
>> two parts continue to live in a wrapper HVM structure, just like
>> they do today?
> 
> I am not sure what you are suggesting here. Could you extend your thought?

Right now we have

struct arch_pirq {
int irq;
union {
struct hvm_pirq {
int emuirq;
struct hvm_pirq_dpci dpci;
} hvm;
};
};

What I'm suggesting is to keep

struct hvm_pirq {
 int emuirq;
 struct hvm_pirq_dpci dpci;
};

and add struct arch_pirq into there. Arguably it could even
be first in there, thus allowing xfree() to free the whole
thing no matter whether passed a struct hvm_pirq * or a
struct arch_pirq * (and eliminating the need for a per-
arch abstraction of the freeing).

> @@ -171,8 +172,26 @@ struct hvm_pirq_dpci {
>struct hvm_gmsi_info gmsi;
>struct timer timer;
>struct list_head softirq_list;
> +int emuirq;
> +struct pirq pirq;
>};
>
> +#define pirq_dpci(p)\
> +((p) ? container_of(p, struct hvm_pirq_dpci, pirq) : NULL)
> +#define const_pirq_dpci(p)  \
> +((p) ? container_of(p, const struct hvm_pirq_dpci, pirq) : NULL)
> +
> +#define dpci_pirq(pd) (&(pd)->pirq)
> +
> +#define domain_pirq_to_emuirq(d, p) ({  \
> +struct pirq *__pi = pirq_info(d, p);\
> +__pi ? pirq_dpci(__pi)->emuirq : IRQ_UNBOUND;   \
> +})
> +#define domain_emuirq_to_pirq(d, emuirq) ({ \
> +void *__ret = radix_tree_lookup(&(d)->arch.hvm.emuirq_pirq, emuirq);\
> +__ret ? radix_tree_ptr_to_int(__ret) : IRQ_UNBOUND; \
> +})

 While for the latter you merely move the bogus double-leading-
 underscore macro local variable (which on this occasion I'd
 like to ask anyway to be changed), you actively introduce a
 new similar name space violation in the domain_pirq_to_emuirq().
>>>
>>> AFAIK, there is nothing in the coding style forbidding your "bogus"
>>> naming. So I just followed the rest of the code.
>>
>> Our coding style document is not to re-iterate C 

Re: [Xen-devel] [PATCH] xen/list: Remove prefetching

[Roger Pau Monné]

On Tue, Jan 14, 2020 at 08:35:45PM +, Andrew Cooper wrote:
> Xen inherited its list infrastructure from Linux.  One area where has fallen
> behind is that of prefetching, which as it turns out is a performance penalty
> in most cases.
> 
> Prefetch of NULL on x86 is now widely measured to have glacial performance
> properties, and will unconditionally hit on every hlist use due to the
> termination condition.
> 
> Cross-port the following Linux patches:
> 
>   75d65a425c (2011) "hlist: remove software prefetching in hlist iterators"
>   e66eed651f (2011) "list: remove prefetching from regular list iterators"
>   c0d15cc7ee (2013) "linked-list: Remove __list_for_each"
> 
> to Xen, which results in the following net diffstat on x86:
> 
>   add/remove: 0/1 grow/shrink: 27/83 up/down: 576/-1648 (-1072)
> 
> (The code additions comes from a few now-inlined functions, and slightly
> different basic block padding.)
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper 

Reviewed-by: Roger Pau Monné 

Has this gone through some XenRT performance testing to assert there
are not regressions performance wise?

Thanks, Roger.

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

[Xen-devel] [PATCH] x86: refine link time stub area related assertion

[Jan Beulich]

While it has been me to introduce this, the use of | there has become
(and perhaps was from the very beginning) misleading. Rather than
avoiding the right side of it when linking the xen.efi intermediate file
at a different base address, make the expression cope with that case,
thus verifying placement on every step.

Signed-off-by: Jan Beulich 

--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -351,8 +351,8 @@ SECTIONS
   .comment 0 : { *(.comment) }
 }
 
-ASSERT(__image_base__ > XEN_VIRT_START |
-   __2M_rwdata_end <= XEN_VIRT_END - NR_CPUS * PAGE_SIZE,
+ASSERT(__2M_rwdata_end <= XEN_VIRT_END - XEN_VIRT_START + __XEN_VIRT_START -
+  NR_CPUS * PAGE_SIZE,
"Xen image overlaps stubs area")
 
 #ifdef CONFIG_KEXEC

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

Re: [Xen-devel] [RFC PATCH 0/3] Live update boot memory management

[Julien Grall]

On 15/01/2020 07:40, David Woodhouse wrote:

On Tue, 2020-01-14 at 16:29 +, Julien Grall wrote:

That's the point in appending an IND_WRITE64 operation to the kimage
stream. The actual write is done in the last gasp of kexec_reloc()
after Xen#1 is quiescent, on the way into purgatory.


I was not sure what you meant by IND_WRITE64. Maybe I should have asked
it first :). Thank you for the explanation!


Don't you often find an email is made easier to understand by the
addition of a few lines of unified diff of assembler code...?


It definitely helps. I tend to prefer diff over a long paragraph trying 
to explain the same :).


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] xen/arm: during efi boot, improve the check for usable memory

[Julien Grall]

Hi Stefano,

On 14/01/2020 23:31, Stefano Stabellini wrote:

When booting via EFI, the EFI memory map has information about memory
regions and their type. Improve the check for the type and attribute of
each memory region to figure out whether it is usable memory or not.
This patch brings the check on par with Linux v5.5-rc6 and makes more
memory reusable as normal memory by Xen (except that Linux also reuses
EFI_PERSISTENT_MEMORY, which we do not).

Specifically, this patch also reuses memory marked as
EfiLoaderCode/Data, and it uses both Attribute and Type for the check
(Attribute needs to be EFI_MEMORY_WB).

Reported-by: Roman Shaposhnik 
Signed-off-by: Stefano Stabellini 
CC: Julien Grall 


Acked-by: Julien Grall 

Cheers,



---
Changes in v2:
- improve commit message
- do not allocate memory marked as EFI_PERSISTENT_MEMORY
- do not change map_bs' behavior
---
  xen/arch/arm/efi/efi-boot.h | 11 +++
  1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index d7bf934077..6527cb0bdf 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -149,10 +149,13 @@ static EFI_STATUS __init 
efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
  
  for ( Index = 0; Index < (mmap_size / desc_size); Index++ )

  {
-if ( desc_ptr->Type == EfiConventionalMemory ||
- (!map_bs &&
-  (desc_ptr->Type == EfiBootServicesCode ||
-   desc_ptr->Type == EfiBootServicesData)) )
+if ( desc_ptr->Attribute & EFI_MEMORY_WB &&
+ (desc_ptr->Type == EfiConventionalMemory ||
+  desc_ptr->Type == EfiLoaderCode ||
+  desc_ptr->Type == EfiLoaderData ||
+  (!map_bs &&
+   (desc_ptr->Type == EfiBootServicesCode ||
+desc_ptr->Type == EfiBootServicesData))) )
  {
  if ( !meminfo_add_bank(, desc_ptr) )
  {



--
Julien Grall

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