[PATCH] arm/efi: Fix null pointer dereference
Fix for commit 60649d443dc395243e74d2b3e05594ac0c43cfe3
that introduces a null pointer dereference when the
fdt_node_offset_by_compatible is called with "fdt"
argument null.
Reported-by: Julien Grall
Fixes: 60649d443d ("arm/efi: Introduce xen,uefi-cfg-load DT property")
Signed-off-by: Luca Fancellu
---
xen/arch/arm/efi/efi-boot.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index a3e46453d4..e63dafac26 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -593,7 +593,8 @@ static bool __init
efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
dtbfile.ptr = fdt;
dtbfile.need_to_free = false; /* Config table memory can't be freed. */
-if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
+if ( fdt &&
+ (fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0) )
{
/* Locate chosen node */
int node = fdt_subnode_offset(fdt, 0, "chosen");
--
2.17.1
[PATCH v5 0/2] arm/efi: Add dom0less support to UEFI boot
This serie introduces a way to start a dom0less setup when Xen is booting as EFI application. Using the device tree it's now possible to fetch from the disk and load in memory all the modules needed to start any domU defined in the DT. Dom0less for now is supported only by the arm architecture. This serie was originally formed by 3 patch, the first one was merged. Luca Fancellu (2): arm/efi: Use dom0less configuration when using EFI boot arm/efi: load dom0 modules from DT using UEFI docs/misc/arm/device-tree/booting.txt | 29 +++ docs/misc/efi.pandoc | 261 +++ xen/arch/arm/efi/efi-boot.h | 344 +- xen/common/efi/boot.c | 55 ++-- 4 files changed, 672 insertions(+), 17 deletions(-) -- 2.17.1
[PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
This patch introduces the support for dom0less configuration
when using UEFI boot on ARM, it permits the EFI boot to
continue if no dom0 kernel is specified but at least one domU
is found.
Introduce the new property "xen,uefi-binary" for device tree boot
module nodes that are subnode of "xen,domain" compatible nodes.
The property holds a string containing the file name of the
binary that shall be loaded by the uefi loader from the filesystem.
Introduce a new call efi_check_dt_boot(...) called during EFI boot
that checks for module to be loaded using device tree.
Architectures that don't support device tree don't have to
provide this function.
Update efi documentation about how to start a dom0less
setup using UEFI
Signed-off-by: Luca Fancellu
Reviewed-by: Bertrand Marquis
Reviewed-by: Stefano Stabellini
---
Changes in v5:
- Removed unneeded variable initialization
- Fixed comment
- Fixed error message for the absence of an initial domain kernel
- changed efi_arch_check_dt_boot to efi_check_dt_boot and add
a stub if CONFIG_HAS_DEVICE_TREE is not declared, updated commit
message about the call introduction in the EFI boot flow.
Changes in v4:
- update uefi,cfg-load to xen,uefi-cfg-load in documentation
- fixed comments and code style
- changed variable name from dt_module_found to dt_modules_found
in boot.c
- removed stub efi_arch_check_dt_boot from x86 code because the
architecture does not support DT, protected call with #ifdef
in the common code.
- add comment to explain the result from efi_arch_check_dt_boot
just looking the common code
- Add space before comment in boot.c
- renamed uefi,binary property to xen,uefi-binary
Changes in v3:
- fixed documentation
- fixed name len in strlcpy
- fixed some style issues
- closed filesystem handle before calling blexit
- passed runtime errors up to the stack instead
of calling blexit
- renamed names and function to make them more
general in prevision to load also Dom0 kernel
and ramdisk from DT
Changes in v2:
- remove array of struct file
- fixed some int types
- Made the code use filesystem even when configuration
file is skipped.
- add documentation of uefi,binary in booting.txt
- add documentation on how to boot all configuration
for Xen using UEFI in efi.pandoc
---
docs/misc/arm/device-tree/booting.txt | 21 ++
docs/misc/efi.pandoc | 203 +
xen/arch/arm/efi/efi-boot.h | 305 +-
xen/common/efi/boot.c | 39 +++-
4 files changed, 556 insertions(+), 12 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index 352b0ec43a..7258e7e1ec 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -190,6 +190,13 @@ The kernel sub-node has the following properties:
Command line parameters for the guest kernel.
+- xen,uefi-binary (UEFI boot only)
+
+String property that specifies the file name to be loaded by the UEFI boot
+for this module. If this is specified, there is no need to specify the reg
+property because it will be created by the UEFI stub on boot.
+This option is needed only when UEFI boot is used.
+
The ramdisk sub-node has the following properties:
- compatible
@@ -201,6 +208,13 @@ The ramdisk sub-node has the following properties:
Specifies the physical address of the ramdisk in RAM and its
length.
+- xen,uefi-binary (UEFI boot only)
+
+String property that specifies the file name to be loaded by the UEFI boot
+for this module. If this is specified, there is no need to specify the reg
+property because it will be created by the UEFI stub on boot.
+This option is needed only when UEFI boot is used.
+
Example
===
@@ -265,6 +279,13 @@ The dtb sub-node should have the following properties:
Specifies the physical address of the device tree binary fragment
RAM and its length.
+- xen,uefi-binary (UEFI boot only)
+
+String property that specifies the file name to be loaded by the UEFI boot
+for this module. If this is specified, there is no need to specify the reg
+property because it will be created by the UEFI stub on boot.
+This option is needed only when UEFI boot is used.
+
As an example:
module@0xc00 {
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index ed85351541..876cd55005 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -167,3 +167,206 @@ sbsign \
--output xen.signed.efi \
xen.unified.efi
```
+
+## UEFI boot and dom0less on ARM
+
+Dom0less feature is supported by ARM and it is possible to use it when Xen is
+started as an EFI application.
+The way to specify the domU domains is by Device Tree as specified in the
+[dom0less](dom0less.html) documentation page under the "Device Tree
+configuration" section, but instead of declaring the reg property in the boot
+module, the user must specify the "xen,uefi-binary" property containi
[PATCH v5 2/2] arm/efi: load dom0 modules from DT using UEFI
Add support to load Dom0 boot modules from
the device tree using the xen,uefi-binary property.
Update documentation about that.
Signed-off-by: Luca Fancellu
Reviewed-by: Bertrand Marquis
Reviewed-by: Stefano Stabellini
Acked-by: Jan Beulich
---
Changes in v5:
- renamed missing uefi,binary string
- used kernel.ptr instead of kernel.addr to be consistent
to the surrounding code
- Changed a comment referring to efi_arch_check_dt_boot
that now is efi_check_dt_boot
Changes in v4:
- Add check to avoid double definition of Dom0 ramdisk
from cfg file and DT
- Fix if conditions indentation in boot.c
- Moved Dom0 kernel verification code after check for
presence for Dom0 or DomU(s)
- Changed uefi,binary property to xen,uefi-binary
Changes in v3:
- new patch
---
docs/misc/arm/device-tree/booting.txt | 8
docs/misc/efi.pandoc | 64 +--
xen/arch/arm/efi/efi-boot.h | 47 ++--
xen/common/efi/boot.c | 16 ---
4 files changed, 123 insertions(+), 12 deletions(-)
diff --git a/docs/misc/arm/device-tree/booting.txt
b/docs/misc/arm/device-tree/booting.txt
index 7258e7e1ec..c6a775f4e8 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -70,6 +70,14 @@ Each node contains the following properties:
priority of this field vs. other mechanisms of specifying the
bootargs for the kernel.
+- xen,uefi-binary (UEFI boot only)
+
+ String property that specifies the file name to be loaded by the UEFI
+ boot for this module. If this is specified, there is no need to specify
+ the reg property because it will be created by the UEFI stub on boot.
+ This option is needed only when UEFI boot is used, the node needs to be
+ compatible with multiboot,kernel or multiboot,ramdisk.
+
Examples
diff --git a/docs/misc/efi.pandoc b/docs/misc/efi.pandoc
index 876cd55005..4abbb5bb82 100644
--- a/docs/misc/efi.pandoc
+++ b/docs/misc/efi.pandoc
@@ -167,6 +167,28 @@ sbsign \
--output xen.signed.efi \
xen.unified.efi
```
+## UEFI boot and Dom0 modules on ARM
+
+When booting using UEFI on ARM, it is possible to specify the Dom0 modules
+directly from the device tree without using the Xen configuration file, here an
+example:
+
+chosen {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ xen,xen-bootargs = "[Xen boot arguments]"
+
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ xen,uefi-binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ xen,uefi-binary = "initrd-3.0.31-0.4-xen";
+ };
+}
## UEFI boot and dom0less on ARM
@@ -326,10 +348,10 @@ chosen {
### Boot Xen, Dom0 and DomU(s)
This configuration is a mix of the two configuration above, to boot this one
-the configuration file must be processed so the /chosen node must have the
-"xen,uefi-cfg-load" property.
+the configuration file can be processed or the Dom0 modules can be read from
+the device tree.
-Here an example:
+Here the first example:
Xen configuration file:
@@ -369,4 +391,40 @@ chosen {
};
```
+Here the second example:
+
+Device tree:
+
+```
+chosen {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ xen,xen-bootargs = "[Xen boot arguments]"
+
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ xen,uefi-binary = "vmlinuz-3.0.31-0.4-xen";
+ bootargs = "[domain 0 command line options]";
+ };
+
+ module@2 {
+ compatible = "multiboot,ramdisk", "multiboot,module";
+ xen,uefi-binary = "initrd-3.0.31-0.4-xen";
+ };
+
+ domU1 {
+ #size-cells = <0x1>;
+ #address-cells = <0x1>;
+ compatible = "xen,domain";
+ cpus = <0x1>;
+ memory = <0x0 0xc>;
+ vpl011;
+ module@1 {
+ compatible = "multiboot,kernel", "multiboot,module";
+ xen,uefi-binary = "Image-domu1.bin";
+ bootargs = "console=ttyAMA0 root=/dev/ram0 rw";
+ };
+ };
+};
+```
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 9c1d400fa6..7a4ebd4128 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -31,8 +31,11 @@ static unsigned int __initdata modules_idx;
#define ERROR_MISSING_DT_PROPERTY (-3)
#define ERROR_RENAME_MODULE_NAME(-4)
#define ERROR_SET_REG_PROPERTY (-5)
+#define ERROR_DOM0_ALREADY_FOUND(-6)
+#define ERROR_DOM0_RAMDISK_FOUND(-7)
#define ERROR_DT_MODULE_DOMU(-1)
#define ERROR_DT_CHOSEN_NODE(-2)
+#define ERROR_DT_MODULE_DOM0(-3)
void noreturn ef
Ping: [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs
On 17.09.2021 13:00, Jan Beulich wrote:
> Hidden devices are associated with DomXEN but usable by the
> hardware domain. Hence they need flushing as well when all devices are
> to have flushes invoked.
>
> While there drop a redundant ATS-enabled check and constify the first
> parameter of the involved function.
>
> Signed-off-by: Jan Beulich
The VT-d side equivalent having gone in a while ago, I think it would
be good to have the AMD side on par.
Jan
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -308,14 +308,11 @@ void amd_iommu_flush_iotlb(u8 devfn, con
> flush_command_buffer(iommu, iommu_dev_iotlb_timeout);
> }
>
> -static void amd_iommu_flush_all_iotlbs(struct domain *d, daddr_t daddr,
> +static void amd_iommu_flush_all_iotlbs(const struct domain *d, daddr_t daddr,
> unsigned int order)
> {
> struct pci_dev *pdev;
>
> -if ( !ats_enabled )
> -return;
> -
> for_each_pdev( d, pdev )
> {
> u8 devfn = pdev->devfn;
> @@ -343,7 +340,16 @@ static void _amd_iommu_flush_pages(struc
> }
>
> if ( ats_enabled )
> +{
> amd_iommu_flush_all_iotlbs(d, daddr, order);
> +
> +/*
> + * Hidden devices are associated with DomXEN but usable by the
> + * hardware domain. Hence they need dealing with here as well.
> + */
> +if ( is_hardware_domain(d) )
> +amd_iommu_flush_all_iotlbs(dom_xen, daddr, order);
> +}
> }
>
> void amd_iommu_flush_all_pages(struct domain *d)
>
>
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
On 11.10.2021 10:03, Luca Fancellu wrote: > This patch introduces the support for dom0less configuration > when using UEFI boot on ARM, it permits the EFI boot to > continue if no dom0 kernel is specified but at least one domU > is found. > > Introduce the new property "xen,uefi-binary" for device tree boot > module nodes that are subnode of "xen,domain" compatible nodes. > The property holds a string containing the file name of the > binary that shall be loaded by the uefi loader from the filesystem. > > Introduce a new call efi_check_dt_boot(...) called during EFI boot > that checks for module to be loaded using device tree. > Architectures that don't support device tree don't have to > provide this function. > > Update efi documentation about how to start a dom0less > setup using UEFI > > Signed-off-by: Luca Fancellu > Reviewed-by: Bertrand Marquis > Reviewed-by: Stefano Stabellini Did you get indication that these are fine to retain with ... > --- > Changes in v5: > - Removed unneeded variable initialization > - Fixed comment > - Fixed error message for the absence of an initial domain kernel > - changed efi_arch_check_dt_boot to efi_check_dt_boot and add > a stub if CONFIG_HAS_DEVICE_TREE is not declared, updated commit > message about the call introduction in the EFI boot flow. ... all of these changes? Every individual change may be minor enough, but their sum makes me wonder. If so (or if at least one of the two gets re-offered) Acked-by: Jan Beulich albeit preferably with ... > --- a/xen/common/efi/boot.c > +++ b/xen/common/efi/boot.c > @@ -166,6 +166,13 @@ static void __init PrintErr(const CHAR16 *s) > StdErr->OutputString(StdErr, (CHAR16 *)s ); > } > > +#ifndef CONFIG_HAS_DEVICE_TREE > +static inline int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) ... the "inline" here dropped. We don't normally add this outside of headers, leaving it to the compiler to decide. In headers it's wanted to avoid "defined by never used" style warnings. Jan
[PATCH] x86/PoD: defer nested P2M flushes
With NPT or shadow in use, the p2m_set_entry() -> p2m_pt_set_entry() ->
write_p2m_entry() -> p2m_flush_nestedp2m() call sequence triggers a lock
order violation when the PoD lock is held around it. Hence such flushing
needs to be deferred. Steal the approach from p2m_change_type_range().
Similarly for EPT I think ept_set_entry() -> ept_sync_domain() ->
ept_sync_domain_prepare() -> p2m_flush_nestedp2m() is affected.
Reported-by: Elliott Mitchell
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -494,6 +495,13 @@ p2m_pod_offline_or_broken_replace(struct
static int
p2m_pod_zero_check_superpage(struct p2m_domain *p2m, gfn_t gfn);
+static void pod_unlock_and_flush(struct p2m_domain *p2m)
+{
+pod_unlock(p2m);
+p2m->defer_nested_flush = false;
+if ( nestedhvm_enabled(p2m->domain) )
+p2m_flush_nestedp2m(p2m->domain);
+}
/*
* This function is needed for two reasons:
@@ -514,6 +522,7 @@ p2m_pod_decrease_reservation(struct doma
gfn_lock(p2m, gfn, order);
pod_lock(p2m);
+p2m->defer_nested_flush = true;
/*
* If we don't have any outstanding PoD entries, let things take their
@@ -665,7 +674,7 @@ out_entry_check:
}
out_unlock:
-pod_unlock(p2m);
+pod_unlock_and_flush(p2m);
gfn_unlock(p2m, gfn, order);
return ret;
}
@@ -1144,8 +1153,10 @@ p2m_pod_demand_populate(struct p2m_domai
* won't start until we're done.
*/
if ( unlikely(d->is_dying) )
-goto out_fail;
-
+{
+pod_unlock(p2m);
+return false;
+}
/*
* Because PoD does not have cache list for 1GB pages, it has to remap
@@ -1167,6 +1178,8 @@ p2m_pod_demand_populate(struct p2m_domai
p2m_populate_on_demand, p2m->default_access);
}
+p2m->defer_nested_flush = true;
+
/* Only reclaim if we're in actual need of more cache. */
if ( p2m->pod.entry_count > p2m->pod.count )
pod_eager_reclaim(p2m);
@@ -1229,8 +1242,9 @@ p2m_pod_demand_populate(struct p2m_domai
__trace_var(TRC_MEM_POD_POPULATE, 0, sizeof(t), &t);
}
-pod_unlock(p2m);
+pod_unlock_and_flush(p2m);
return true;
+
out_of_memory:
pod_unlock(p2m);
@@ -1239,12 +1253,14 @@ out_of_memory:
p2m->pod.entry_count, current->domain->domain_id);
domain_crash(d);
return false;
+
out_fail:
-pod_unlock(p2m);
+pod_unlock_and_flush(p2m);
return false;
+
remap_and_retry:
BUG_ON(order != PAGE_ORDER_2M);
-pod_unlock(p2m);
+pod_unlock_and_flush(p2m);
/*
* Remap this 2-meg region in singleton chunks. See the comment on the
[PATCH v2] x86/PV32: fix physdev_op_compat handling
The conversion of the original code failed to recognize that the 32-bit
compat variant of this (sorry, two different meanings of "compat" here)
needs to continue to invoke the compat handler, not the native one.
Arrange for this by adding yet another #define.
Affected functions (having existed prior to the introduction of the new
hypercall) are PHYSDEVOP_set_iobitmap and PHYSDEVOP_apic_{read,write}.
For all others the operand struct layout doesn't differ.
Fixes: 1252e2823117 ("x86/pv: Export pv_hypercall_table[] rather than working
around it in several ways")
Signed-off-by: Jan Beulich
---
v2: Don't remove do_physdev_op override.
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -12,6 +12,7 @@ EMIT_FILE;
#define physdev_op_t physdev_op_compat_t
#define do_physdev_op compat_physdev_op
#define do_physdev_op_compat(x) compat_physdev_op_compat(_##x)
+#define nativecompat
#define COMPAT
#define _XEN_GUEST_HANDLE(t) XEN_GUEST_HANDLE(t)
[PATCH 0/2] VT-d: correct / extend workaround(s) leaving an IOMMU disabled
This gets us closer to what Linux does, which hopefully improves the experience for people running Xen on affected hardware. Ian - I'm also Cc-ing you since this feels like being on the edge between a new feature and a bug fix. 1: generalize and correct "iommu=no-igfx" handling 2: Tylersburg isoch DMAR unit with no TLB space Jan
[PATCH 1/2] VT-d: generalize and correct "iommu=no-igfx" handling
Linux'es supposedly equivalent "intel_iommu=igfx_off" deals with any
graphics devices (not just Intel ones) while at the same time limiting
the effect to IOMMUs covering only graphics devices. Keying the decision
to leave translation disabled for an IOMMU to merely a magic SBDF tuple
was wrong in the first place - systems may very well have non-graphics
devices at :00:02.0 (ordinary root ports commonly live there, for
example). Any use of igd_drhd_address (and hence is_igd_drhd()) needs
further qualification.
Introduce a new "graphics only" field in struct acpi_drhd_unit and set
it according to device scope parsing outcome. Replace the bad use of
is_igd_drhd() in iommu_enable_translation() by use of this new field.
While adding the new field also convert the adjacent include_all one to
"bool".
Signed-off-by: Jan Beulich
---
I assume an implication is that these devices then may not be passed
through to guests, yet I don't see us enforcing this anywhere.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1494,8 +1494,8 @@ The following options are specific to In
version 6 and greater as Registered-Based Invalidation isn't supported
by them.
-* The `igfx` boolean is active by default, and controls whether the IOMMU in
-front of an Intel Graphics Device is enabled or not.
+* The `igfx` boolean is active by default, and controls whether IOMMUs in
+front of solely graphics devices get enabled or not.
It is intended as a debugging mechanism for graphics issues, and to be
similar to Linux's `intel_iommu=igfx_off` option. If specifying `no-igfx`
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -315,6 +315,7 @@ static int __init acpi_parse_dev_scope(
struct acpi_drhd_unit *drhd = type == DMAR_TYPE ?
container_of(scope, struct acpi_drhd_unit, scope) : NULL;
int depth, cnt, didx = 0, ret;
+bool gfx_only = false;
if ( (cnt = scope_device_count(start, end)) < 0 )
return cnt;
@@ -324,6 +325,8 @@ static int __init acpi_parse_dev_scope(
scope->devices = xzalloc_array(u16, cnt);
if ( !scope->devices )
return -ENOMEM;
+
+gfx_only = drhd && !drhd->include_all;
}
scope->devices_cnt = cnt;
@@ -354,6 +357,7 @@ static int __init acpi_parse_dev_scope(
acpi_scope->bus, sec_bus, sub_bus);
dmar_scope_add_buses(scope, sec_bus, sub_bus);
+gfx_only = false;
break;
case ACPI_DMAR_SCOPE_TYPE_HPET:
@@ -374,6 +378,8 @@ static int __init acpi_parse_dev_scope(
acpi_hpet_unit->dev = path->dev;
acpi_hpet_unit->func = path->fn;
list_add(&acpi_hpet_unit->list, &drhd->hpet_list);
+
+gfx_only = false;
}
break;
@@ -388,6 +394,12 @@ static int __init acpi_parse_dev_scope(
if ( (seg == 0) && (bus == 0) && (path->dev == 2) &&
(path->fn == 0) )
igd_drhd_address = drhd->address;
+
+if ( gfx_only &&
+ pci_conf_read8(PCI_SBDF(seg, bus, path->dev, path->fn),
+PCI_CLASS_DEVICE + 1) != 0x03
+/* PCI_BASE_CLASS_DISPLAY */ )
+gfx_only = false;
}
break;
@@ -408,6 +420,8 @@ static int __init acpi_parse_dev_scope(
acpi_ioapic_unit->ioapic.bdf.dev = path->dev;
acpi_ioapic_unit->ioapic.bdf.func = path->fn;
list_add(&acpi_ioapic_unit->list, &drhd->ioapic_list);
+
+gfx_only = false;
}
break;
@@ -417,11 +431,15 @@ static int __init acpi_parse_dev_scope(
printk(XENLOG_WARNING VTDPREFIX "Unknown scope type %#x\n",
acpi_scope->entry_type);
start += acpi_scope->length;
+gfx_only = false;
continue;
}
scope->devices[didx++] = PCI_BDF(bus, path->dev, path->fn);
start += acpi_scope->length;
- }
+}
+
+if ( drhd && gfx_only )
+drhd->gfx_only = true;
ret = 0;
--- a/xen/drivers/passthrough/vtd/dmar.h
+++ b/xen/drivers/passthrough/vtd/dmar.h
@@ -62,7 +62,8 @@ struct acpi_drhd_unit {
struct list_head list;
u64address; /* register base address of the unit */
u16segment;
-u8 include_all:1;
+bool include_all:1;
+bool gfx_only:1;
struct vtd_iommu *iommu;
struct list_head ioapic_list;
struct list_head hpet_list;
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -751,7 +751,7 @@ static void iommu_enable_translation(str
unsigned long flags;
struct vtd_iommu *iommu = drhd->iommu;
-if ( is_igd_drhd(drhd) )
+if ( drhd->gfx_only )
{
if ( !iommu
[PATCH 2/2] VT-d: Tylersburg isoch DMAR unit with no TLB space
BIOSes, when enabling the dedicated DMAR unit for the sound device,
need to also set a non-zero number of TLB entries in a respective
system management register (VTISOCHCTRL). At least one BIOS is known
to fail to do so, causing the VT-d engine to deadlock when used.
Vaguely based on Linux'es e0fc7e0b4b5e ("intel-iommu: Yet another BIOS
workaround: Isoch DMAR unit with no TLB space").
To limit message string redundancy, fold parts with the IGD quirk logic.
Signed-off-by: Jan Beulich
---
RFC: This requires MMCFG availability before Dom0 starts up, which is
not generally given. We may want/need to e.g. (ab)use the
.enable_device() hook to actually disable translation if MMCFG
accesses become available only in the course of Dom0 booting.
RFC: While following Linux in this regard, I'm not convinced of issuing
the warning about the number of TLB entries when firmware set more
than 16 (I can observe 20 on the on matching system I have access
to.)
I assume an implication is that the device in this case then may not be
passed through to guests, but I don't see us enforcing the same anywhere
for graphics devices when "no-igfx" is in use. Yet here I would want to
follow whatever pre-existing model ...
--- a/xen/drivers/passthrough/vtd/extern.h
+++ b/xen/drivers/passthrough/vtd/extern.h
@@ -100,6 +100,7 @@ int msi_msg_write_remap_rte(struct msi_d
int intel_setup_hpet_msi(struct msi_desc *);
int is_igd_vt_enabled_quirk(void);
+bool is_azalia_tlb_enabled(const struct acpi_drhd_unit *);
void platform_quirks_init(void);
void vtd_ops_preamble_quirk(struct vtd_iommu *iommu);
void vtd_ops_postamble_quirk(struct vtd_iommu *iommu);
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -750,27 +750,43 @@ static void iommu_enable_translation(str
u32 sts;
unsigned long flags;
struct vtd_iommu *iommu = drhd->iommu;
+static const char crash_fmt[] = "%s; crash Xen for security purpose\n";
if ( drhd->gfx_only )
{
+static const char disable_fmt[] = XENLOG_WARNING VTDPREFIX
+ " %s; disabling IGD VT-d engine\n";
+
if ( !iommu_igfx )
{
-printk(XENLOG_INFO VTDPREFIX
- "Passed iommu=no-igfx option. Disabling IGD VT-d
engine.\n");
+printk(disable_fmt, "passed iommu=no-igfx option");
return;
}
if ( !is_igd_vt_enabled_quirk() )
{
+static const char msg[] = "firmware did not enable IGD for VT
properly";
+
if ( force_iommu )
-panic("BIOS did not enable IGD for VT properly, crash Xen for
security purpose\n");
+panic(crash_fmt, msg);
-printk(XENLOG_WARNING VTDPREFIX
- "BIOS did not enable IGD for VT properly. Disabling IGD
VT-d engine.\n");
+printk(disable_fmt, msg);
return;
}
}
+if ( !is_azalia_tlb_enabled(drhd) )
+{
+static const char msg[] = "firmware did not enable TLB for sound
device";
+
+if ( force_iommu )
+panic(crash_fmt, msg);
+
+printk(XENLOG_WARNING VTDPREFIX " %s; disabling ISOCH VT-d engine\n",
+ msg);
+return;
+}
+
/* apply platform specific errata workarounds */
vtd_ops_preamble_quirk(iommu);
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -100,6 +100,69 @@ static void __init cantiga_b3_errata_ini
is_cantiga_b3 = 1;
}
+/*
+ * QUIRK to work around certain BIOSes enabling the ISOCH DMAR unit for the
+ * Azalia sound device, but not giving it any TLB entries, causing it to
+ * deadlock.
+ */
+bool is_azalia_tlb_enabled(const struct acpi_drhd_unit *drhd)
+{
+pci_sbdf_t sbdf;
+unsigned int vtisochctrl;
+
+/* Only dedicated units are of interest. */
+if ( drhd->include_all || drhd->scope.devices_cnt != 1 )
+return true;
+
+/* Check for the specific device. */
+sbdf = PCI_SBDF2(drhd->segment, drhd->scope.devices[0]);
+if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL ||
+ pci_conf_read16(sbdf, PCI_DEVICE_ID) != 0x3a3e )
+return true;
+
+/* Check for the corresponding System Management Registers device. */
+sbdf = PCI_SBDF(drhd->segment, 0, 0x14, 0);
+if ( pci_conf_read16(sbdf, PCI_VENDOR_ID) != PCI_VENDOR_ID_INTEL ||
+ pci_conf_read16(sbdf, PCI_DEVICE_ID) != 0x342e )
+return true;
+
+vtisochctrl = pci_conf_read32(sbdf, 0x188);
+if ( vtisochctrl == 0x )
+{
+printk(XENLOG_WARNING VTDPREFIX
+ " Cannot access VTISOCHCTRL at this time\n");
+return true;
+}
+
+/*
+ * If Azalia DMA is routed to the non-isoch DMAR unit, that's fine in
+ * principle, but not consistent with the ACPI tables.
+ */
+if ( vtisochctrl & 1 )
+{
+printk(XENLOG_WARNING VTD
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
> On 11 Oct 2021, at 09:11, Jan Beulich wrote: > > On 11.10.2021 10:03, Luca Fancellu wrote: >> This patch introduces the support for dom0less configuration >> when using UEFI boot on ARM, it permits the EFI boot to >> continue if no dom0 kernel is specified but at least one domU >> is found. >> >> Introduce the new property "xen,uefi-binary" for device tree boot >> module nodes that are subnode of "xen,domain" compatible nodes. >> The property holds a string containing the file name of the >> binary that shall be loaded by the uefi loader from the filesystem. >> >> Introduce a new call efi_check_dt_boot(...) called during EFI boot >> that checks for module to be loaded using device tree. >> Architectures that don't support device tree don't have to >> provide this function. >> >> Update efi documentation about how to start a dom0less >> setup using UEFI >> >> Signed-off-by: Luca Fancellu >> Reviewed-by: Bertrand Marquis >> Reviewed-by: Stefano Stabellini > > Did you get indication that these are fine to retain with ... > >> --- >> Changes in v5: >> - Removed unneeded variable initialization >> - Fixed comment >> - Fixed error message for the absence of an initial domain kernel >> - changed efi_arch_check_dt_boot to efi_check_dt_boot and add >> a stub if CONFIG_HAS_DEVICE_TREE is not declared, updated commit >> message about the call introduction in the EFI boot flow. > > ... all of these changes? Every individual change may be minor enough, > but their sum makes me wonder. If so (or if at least one of the two > gets re-offered) > Acked-by: Jan Beulich > albeit preferably with ... > >> --- a/xen/common/efi/boot.c >> +++ b/xen/common/efi/boot.c >> @@ -166,6 +166,13 @@ static void __init PrintErr(const CHAR16 *s) >> StdErr->OutputString(StdErr, (CHAR16 *)s ); >> } >> >> +#ifndef CONFIG_HAS_DEVICE_TREE >> +static inline int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) > > ... the "inline" here dropped. We don't normally add this outside of > headers, leaving it to the compiler to decide. In headers it's wanted > to avoid "defined by never used" style warnings. Hi Jan, Ok I can drop it in a next serie and retain your Ack, or is it something that can be done on commit? Cheers, Luca > > Jan
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
On 11.10.2021 10:50, Luca Fancellu wrote: >> On 11 Oct 2021, at 09:11, Jan Beulich wrote: >> On 11.10.2021 10:03, Luca Fancellu wrote: >>> This patch introduces the support for dom0less configuration >>> when using UEFI boot on ARM, it permits the EFI boot to >>> continue if no dom0 kernel is specified but at least one domU >>> is found. >>> >>> Introduce the new property "xen,uefi-binary" for device tree boot >>> module nodes that are subnode of "xen,domain" compatible nodes. >>> The property holds a string containing the file name of the >>> binary that shall be loaded by the uefi loader from the filesystem. >>> >>> Introduce a new call efi_check_dt_boot(...) called during EFI boot >>> that checks for module to be loaded using device tree. >>> Architectures that don't support device tree don't have to >>> provide this function. >>> >>> Update efi documentation about how to start a dom0less >>> setup using UEFI >>> >>> Signed-off-by: Luca Fancellu >>> Reviewed-by: Bertrand Marquis >>> Reviewed-by: Stefano Stabellini >> >> Did you get indication that these are fine to retain with ... >> >>> --- >>> Changes in v5: >>> - Removed unneeded variable initialization >>> - Fixed comment >>> - Fixed error message for the absence of an initial domain kernel >>> - changed efi_arch_check_dt_boot to efi_check_dt_boot and add >>> a stub if CONFIG_HAS_DEVICE_TREE is not declared, updated commit >>> message about the call introduction in the EFI boot flow. >> >> ... all of these changes? Every individual change may be minor enough, >> but their sum makes me wonder. If so (or if at least one of the two >> gets re-offered) >> Acked-by: Jan Beulich >> albeit preferably with ... >> >>> --- a/xen/common/efi/boot.c >>> +++ b/xen/common/efi/boot.c >>> @@ -166,6 +166,13 @@ static void __init PrintErr(const CHAR16 *s) >>> StdErr->OutputString(StdErr, (CHAR16 *)s ); >>> } >>> >>> +#ifndef CONFIG_HAS_DEVICE_TREE >>> +static inline int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) >> >> ... the "inline" here dropped. We don't normally add this outside of >> headers, leaving it to the compiler to decide. In headers it's wanted >> to avoid "defined by never used" style warnings. > > Ok I can drop it in a next serie and retain your Ack, or is it something that > can be done on commit? I guess that's easy enough to do while committing. Provided of course the two R-b get confirmed. Jan
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
Hi Jan, > On 11 Oct 2021, at 09:52, Jan Beulich wrote: > > On 11.10.2021 10:50, Luca Fancellu wrote: >>> On 11 Oct 2021, at 09:11, Jan Beulich wrote: >>> On 11.10.2021 10:03, Luca Fancellu wrote: This patch introduces the support for dom0less configuration when using UEFI boot on ARM, it permits the EFI boot to continue if no dom0 kernel is specified but at least one domU is found. Introduce the new property "xen,uefi-binary" for device tree boot module nodes that are subnode of "xen,domain" compatible nodes. The property holds a string containing the file name of the binary that shall be loaded by the uefi loader from the filesystem. Introduce a new call efi_check_dt_boot(...) called during EFI boot that checks for module to be loaded using device tree. Architectures that don't support device tree don't have to provide this function. Update efi documentation about how to start a dom0less setup using UEFI Signed-off-by: Luca Fancellu Reviewed-by: Bertrand Marquis Reviewed-by: Stefano Stabellini >>> >>> Did you get indication that these are fine to retain with ... >>> --- Changes in v5: - Removed unneeded variable initialization - Fixed comment - Fixed error message for the absence of an initial domain kernel - changed efi_arch_check_dt_boot to efi_check_dt_boot and add a stub if CONFIG_HAS_DEVICE_TREE is not declared, updated commit message about the call introduction in the EFI boot flow. >>> >>> ... all of these changes? Every individual change may be minor enough, >>> but their sum makes me wonder. If so (or if at least one of the two >>> gets re-offered) >>> Acked-by: Jan Beulich >>> albeit preferably with ... >>> --- a/xen/common/efi/boot.c +++ b/xen/common/efi/boot.c @@ -166,6 +166,13 @@ static void __init PrintErr(const CHAR16 *s) StdErr->OutputString(StdErr, (CHAR16 *)s ); } +#ifndef CONFIG_HAS_DEVICE_TREE +static inline int __init efi_check_dt_boot(EFI_FILE_HANDLE dir_handle) >>> >>> ... the "inline" here dropped. We don't normally add this outside of >>> headers, leaving it to the compiler to decide. In headers it's wanted >>> to avoid "defined by never used" style warnings. >> >> Ok I can drop it in a next serie and retain your Ack, or is it something that >> can be done on commit? > > I guess that's easy enough to do while committing. Provided of course > the two R-b get confirmed. I confirm my R-b. Cheers Bertrand > > Jan
[PATCH v4 0/3] Expose PMU to the guests
This patch series is a rework of an already pushed patch exposing PMU to the guests. Since the second version the vpmu parameter is common and prework in the form of reporting availability of vPMU on the hardware is added. The third version of the patch series removes the redundant check from x86 code and modifies the way to define the flags XEN_DOMCTL_CDF and XEN_SYSCTL_PHYSCAP, meaning not to define bit position and mask separately. In the fourth version, the additional check is added so that we fail if vpmu is set in the config file but XEN_SYSCTL_PHYSCAP_vpmu is not available. The current status is that the PMU registers are not virtualized and the physical registers are directly accessible when "vpmu" parameter is enabled in the guest config file. There is no interrupt support and Xen will not save/restore the register values on context switches. This is to be done in the future. Michal Orzel (3): xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu xen/arm: Check for PMU platform support xen: Expose the PMU to the guests docs/man/xl.cfg.5.pod.in | 17 ++ tools/golang/xenlight/helpers.gen.go | 8 + tools/golang/xenlight/types.gen.go | 2 ++ tools/include/libxl.h| 12 +++ tools/libs/light/libxl.c | 1 + tools/libs/light/libxl_create.c | 10 ++ tools/libs/light/libxl_types.idl | 3 ++ tools/ocaml/libs/xc/xenctrl.ml | 2 ++ tools/ocaml/libs/xc/xenctrl.mli | 2 ++ tools/xl/xl_info.c | 5 +-- tools/xl/xl_parse.c | 2 ++ xen/arch/arm/domain.c| 12 +-- xen/arch/arm/setup.c | 1 + xen/common/domain.c | 12 ++- xen/common/sysctl.c | 3 ++ xen/include/asm-arm/cpufeature.h | 49 ++-- xen/include/asm-arm/domain.h | 1 + xen/include/public/domctl.h | 4 ++- xen/include/public/sysctl.h | 6 ++-- xen/include/xen/domain.h | 2 ++ 20 files changed, 143 insertions(+), 11 deletions(-) -- 2.29.0
[PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu
Introduce flag XEN_SYSCTL_PHYSCAP_vpmu which
indicates whether the platform supports vPMU
functionality. Modify Xen and tools accordingly.
Take the opportunity and fix XEN_SYSCTL_PHYSCAP_vmtrace
definition in sysctl.h which wrongly use (1 << 6)
instead of (1u << 6).
Signed-off-by: Michal Orzel
Reviewed-by: Bertrand Marquis
Acked-by: Nick Rosbrook
Reviewed-by: Stefano Stabellini
---
Changes since v3:
-add spaces between brackets and keyword
Changes since v2:
-do not define bit position and mask separately
Changes since v1:
-new in v2
---
tools/golang/xenlight/helpers.gen.go | 2 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h| 6 ++
tools/libs/light/libxl.c | 1 +
tools/libs/light/libxl_types.idl | 1 +
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/xl/xl_info.c | 5 +++--
xen/common/domain.c | 2 ++
xen/common/sysctl.c | 3 +++
xen/include/public/sysctl.h | 6 --
xen/include/xen/domain.h | 2 ++
12 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/tools/golang/xenlight/helpers.gen.go
b/tools/golang/xenlight/helpers.gen.go
index bfc1e7f312..c8669837d8 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -3360,6 +3360,7 @@ x.CapHap = bool(xc.cap_hap)
x.CapShadow = bool(xc.cap_shadow)
x.CapIommuHapPtShare = bool(xc.cap_iommu_hap_pt_share)
x.CapVmtrace = bool(xc.cap_vmtrace)
+x.CapVpmu = bool(xc.cap_vpmu)
return nil}
@@ -3391,6 +3392,7 @@ xc.cap_hap = C.bool(x.CapHap)
xc.cap_shadow = C.bool(x.CapShadow)
xc.cap_iommu_hap_pt_share = C.bool(x.CapIommuHapPtShare)
xc.cap_vmtrace = C.bool(x.CapVmtrace)
+xc.cap_vpmu = C.bool(x.CapVpmu)
return nil
}
diff --git a/tools/golang/xenlight/types.gen.go
b/tools/golang/xenlight/types.gen.go
index 09a3bb67e2..45f2cba3d2 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -1008,6 +1008,7 @@ CapHap bool
CapShadow bool
CapIommuHapPtShare bool
CapVmtrace bool
+CapVpmu bool
}
type Connectorinfo struct {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d698..ec5e3badae 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -502,6 +502,12 @@
*/
#define LIBXL_HAVE_X86_MSR_RELAXED 1
+/*
+ * LIBXL_HAVE_PHYSINFO_CAP_VPMU indicates that libxl_physinfo has a cap_vpmu
+ * field, which indicates the availability of vPMU functionality.
+ */
+#define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libs/light/libxl.c b/tools/libs/light/libxl.c
index 204eb0be2d..a032723fde 100644
--- a/tools/libs/light/libxl.c
+++ b/tools/libs/light/libxl.c
@@ -404,6 +404,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo
*physinfo)
!!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_iommu_hap_pt_share);
physinfo->cap_vmtrace =
!!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vmtrace);
+physinfo->cap_vpmu = !!(xcphysinfo.capabilities & XEN_SYSCTL_PHYSCAP_vpmu);
GC_FREE;
return 0;
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff653a..993e83acca 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -1061,6 +1061,7 @@ libxl_physinfo = Struct("physinfo", [
("cap_shadow", bool),
("cap_iommu_hap_pt_share", bool),
("cap_vmtrace", bool),
+("cap_vpmu", bool),
], dir=DIR_OUT)
libxl_connectorinfo = Struct("connectorinfo", [
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index 7ed1c00e47..7a4030a192 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -122,6 +122,7 @@ type physinfo_cap_flag =
| CAP_Shadow
| CAP_IOMMU_HAP_PT_SHARE
| CAP_Vmtrace
+ | CAP_Vpmu
type physinfo =
{
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 391d4abdf8..6900513e7f 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -107,6 +107,7 @@ type physinfo_cap_flag =
| CAP_Shadow
| CAP_IOMMU_HAP_PT_SHARE
| CAP_Vmtrace
+ | CAP_Vpmu
type physinfo = {
threads_per_core : int;
diff --git a/tools/xl/xl_info.c b/tools/xl/xl_info.c
index 8383e4a6df..2c86b317b7 100644
--- a/tools/xl/xl_info.c
+++ b/tools/xl/xl_info.c
@@ -210,7 +210,7 @@ static void output_physinfo(void)
info.hw_cap[4], info.hw_cap[5], info.hw_cap[6], info.hw_cap[7]
);
-maybe_printf("virt_caps :%s%s%s%s%s%s%s%s\n",
+maybe_printf("virt_caps :%s%s%s%s%s%s%s%s%s\n",
info.cap_pv ? " pv" : "",
info.cap_hvm ? " hvm" : "",
info.cap_hvm && info.cap_hvm_directio ? " hvm_directio" : "",
@@ -218,7 +218,8 @@ static void output_physinfo(void)
info.cap_hap ? " hap" : "",
info.cap_shadow ? " shadow" : "",
[PATCH v4 2/3] xen/arm: Check for PMU platform support
ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
information about PMU support. Replace structure
dbg64/dbg32 with a union and fill in all the
register fields according to document:
ARM Architecture Registers(DDI 0595, 2021-06).
Add macros boot_dbg_feature64/boot_dbg_feature32
to check for a debug feature. Add macro
cpu_has_pmu to check for PMU support.
Signed-off-by: Michal Orzel
Reviewed-by: Stefano Stabellini
Reviewed-by: Bertrand Marquis
---
Changes since v3:
-none
Changes since v2:
-none
Changes since v1:
-new in v2
---
xen/include/asm-arm/cpufeature.h | 49 ++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 5ca09b0bff..4fce23844d 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_ARM_64
#define cpu_feature64(c, feat) ((c)->pfr64.feat)
#define boot_cpu_feature64(feat) (system_cpuinfo.pfr64.feat)
+#define boot_dbg_feature64(feat) (system_cpuinfo.dbg64.feat)
#define cpu_feature64_has_el0_32(c)(cpu_feature64(c, el0) == 2)
@@ -22,6 +23,7 @@
#define cpu_feature32(c, feat) ((c)->pfr32.feat)
#define boot_cpu_feature32(feat) (system_cpuinfo.pfr32.feat)
+#define boot_dbg_feature32(feat) (system_cpuinfo.dbg32.feat)
#define cpu_has_arm (boot_cpu_feature32(arm) == 1)
#define cpu_has_thumb (boot_cpu_feature32(thumb) >= 1)
@@ -32,8 +34,10 @@
#ifdef CONFIG_ARM_32
#define cpu_has_gentimer (boot_cpu_feature32(gentimer) == 1)
+#define cpu_has_pmu (boot_dbg_feature32(perfmon) >= 1)
#else
#define cpu_has_gentimer (1)
+#define cpu_has_pmu (boot_dbg_feature64(pmu_ver) >= 1)
#endif
#define cpu_has_security (boot_cpu_feature32(security) > 0)
@@ -181,8 +185,28 @@ struct cpuinfo_arm {
};
} pfr64;
-struct {
+union {
register_t bits[2];
+struct {
+/* DFR0 */
+unsigned long debug_ver:4;
+unsigned long trace_ver:4;
+unsigned long pmu_ver:4;
+unsigned long brps:4;
+unsigned long __res0:4;
+unsigned long wrps:4;
+unsigned long __res1:4;
+unsigned long ctx_cmps:4;
+unsigned long pms_ver:4;
+unsigned long double_lock:4;
+unsigned long trace_filt:4;
+unsigned long __res2:4;
+unsigned long mtpmu:4;
+unsigned long __res3:12;
+
+/* DFR1 */
+unsigned long __res4:64;
+};
} dbg64;
struct {
@@ -321,8 +345,29 @@ struct cpuinfo_arm {
};
} pfr32;
-struct {
+union {
register_t bits[2];
+struct {
+/* DFR0 */
+unsigned long copdbg:4;
+unsigned long copsdbg:4;
+unsigned long mmapdbg:4;
+unsigned long coptrc:4;
+unsigned long mmaptrc:4;
+unsigned long mprofdbg:4;
+unsigned long perfmon:4;
+unsigned long tracefilt:4;
+#ifdef CONFIG_ARM_64
+unsigned long __res0:32;
+#endif
+
+/* DFR1 */
+unsigned long mtpmu:4;
+unsigned long __res1:28;
+#ifdef CONFIG_ARM_64
+unsigned long __res2:32;
+#endif
+};
} dbg32;
struct {
--
2.29.0
[PATCH v4 3/3] xen: Expose the PMU to the guests
Add parameter vpmu to xl domain configuration syntax
to enable the access to PMU registers by disabling
the PMU traps(currently only for ARM).
The current status is that the PMU registers are not
virtualized and the physical registers are directly
accessible when this parameter is enabled. There is no
interrupt support and Xen will not save/restore the
register values on context switches.
Please note that this feature is experimental.
Signed-off-by: Michal Orzel
Signed-off-by: Julien Grall
Reviewed-by: Bertrand Marquis
---
Changes since v3:
-fail if vpmu is set but not supported
-rebase on top of latest staging
Changes since v2:
-remove redundant check from x86 code
-do not define bit position and mask separately
Changes since v1:
-modify vpmu parameter to be common rather than arch specific
---
docs/man/xl.cfg.5.pod.in | 17 +
tools/golang/xenlight/helpers.gen.go | 6 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h| 6 ++
tools/libs/light/libxl_create.c | 10 ++
tools/libs/light/libxl_types.idl | 2 ++
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/xl/xl_parse.c | 2 ++
xen/arch/arm/domain.c| 12 +---
xen/arch/arm/setup.c | 1 +
xen/common/domain.c | 10 +-
xen/include/asm-arm/domain.h | 1 +
xen/include/public/domctl.h | 4 +++-
14 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 4b1e3028d2..55c4881205 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -690,6 +690,23 @@ default.
B: Acceptable values are platform specific. For Intel Processor
Trace, this value must be a power of 2 between 4k and 16M.
+=item B
+
+Currently ARM only.
+
+Specifies whether to enable the access to PMU registers by disabling
+the PMU traps.
+
+The PMU registers are not virtualized and the physical registers are directly
+accessible when this parameter is enabled. There is no interrupt support and
+Xen will not save/restore the register values on context switches.
+
+vPMU, by design and purpose, exposes system level performance
+information to the guest. Only to be used by sufficiently privileged
+domains. This feature is currently in experimental state.
+
+If this option is not specified then it will default to B.
+
=back
=head2 Devices
diff --git a/tools/golang/xenlight/helpers.gen.go
b/tools/golang/xenlight/helpers.gen.go
index c8669837d8..2449580bad 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed:
%v", err)
}
x.Altp2M = Altp2MMode(xc.altp2m)
x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
+if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
return nil}
@@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed:
%v", err)
}
xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
+if err := x.Vpmu.toC(&xc.vpmu); err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
return nil
}
diff --git a/tools/golang/xenlight/types.gen.go
b/tools/golang/xenlight/types.gen.go
index 45f2cba3d2..b2e8bd1a85 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -521,6 +521,7 @@ MsrRelaxed Defbool
}
Altp2M Altp2MMode
VmtraceBufKb int
+Vpmu Defbool
}
type DomainBuildInfoTypeUnion interface {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ec5e3badae..ee73eb06f1 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -508,6 +508,12 @@
*/
#define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
+/*
+ * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
+ * which allows to enable the access to PMU registers.
+ */
+#define LIBXL_HAVE_VPMU 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..2a0234ec16 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
+libxl_defbool_setdefault(&b_info->vpmu, false);
if (libxl_defbool_val(b_info->device_model_stubdomain) &&
!b_info->device_model_ssidref)
@@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config
*d_config,
create.flags |= XEN_DOMCTL_CDF_nested_virt;
}
+if ( libxl_defbool_val(b_info->vpmu) )
+create.flags |= XEN_DOMCTL_CDF_vpmu;
+
assert(info->passthrough != LIBXL_PASSTHROUGH_DEFAULT);
LOG(DETAIL, "passthrough: %s",
libxl_passthrough_to_string(info-
[linux-linus test] 165459: tolerable FAIL - PUSHED
flight 165459 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/165459/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 165457 Tests which did not succeed, but are not blocking: test-amd64-amd64-examine 4 memdisk-try-append fail like 165450 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 165457 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 165457 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 165457 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 165457 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 165457 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 165457 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 165457 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 165457 test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass version targeted for testing: linux64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc baseline version: linux7fd2bf83d59a2d32e0d596c5d3e623b9a0e7e2d5 Last test of basis 165457 2021-10-10 13:17:03 Z0 days Testing same since 165459 2021-10-11 01:11:38 Z0 days1 attempts People who touched revisions under test: Alexey Kardashevskiy Borislav Petkov Christophe Leroy Cédric Le Goater Jakub Kicinski James Morse Joe Lawrence Johan Almbladh Josh Poimboeuf
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Hi Stefano,
On 08/10/2021 22:46, Stefano Stabellini wrote:
On Fri, 8 Oct 2021, Julien Grall wrote:
Hi Andrew,
On Fri, 8 Oct 2021, 20:07 Andrew Cooper, wrote:
On 06/10/2021 18:40, Rahul Singh wrote:
> Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
> Reject the use of this new flag for x86 as VPCI is not supported for
> DOMU guests for x86.
>
> Signed-off-by: Rahul Singh
> Reviewed-by: Stefano Stabellini
> Acked-by: Christian Lindig
I'm afraid this logic is broken.
There's no matching feature to indicate to the toolstack whether
XEN_DOMCTL_CDF_vpci will be accepted or not. The usual way of doing
this is with a physinfo_cap field.
I am slightly puzzled by this. I am assuming you are referring to
XENVER_get_features which AFAICT is a stable interface. So why should we
use it to describe the presence of an unstable interface?
This flag needs using in Patch 10 to reject attempts to create a VM with
devices when passthrough support is unavailable.
May I ask why we can't rely on the hypervisor to reject the flag when suitable?
Ian, for the 4.16 release, this series either needs completing with the
additional flag implemented, or this patch needs reverting to avoid us
shipping a broken interface.
I fail to see how the interface would be broken... Would you mind to describe a
bit more what could go wrong with this interface?
After chatting with Andrew on IRC, this is my understanding.
Today if pci=[] is specified in the VM config file then
XEN_DOMCTL_CDF_vpci is added. If Xen doesn't support it, Xen returns
an error but libxl/xl won't be able to tell exactly why it fails. So xl
will end up printing a generic VM creation failure. Andrew would like to
see something like the following in libxl:
if ( PCI_devices && !cap.vcpi )
error("Sorry - PCI not supported")
So that the user gets a clear informative error back rather than a
generic VM creation failure.
I can understand this request. However...
Also, this is a requirement for the stable
hypercall interface.
... leaving aside the fact that domctl is clearly not stable today, the
flag would be rejected on hypervisor not supporting vPCI. So I don't
really see how this is a requirement for the stable hypercall interface.
Would you mind providing more details?
I think that's fine and we can implement this request easily by adding
XEN_SYSCTL_PHYSCAP_vpci. Rahul or Bertrand, are you guys happy with
doing that? Otherwise I could take it on.
As a side note, given that PCI passthrough support is actually not yet
complete on ARM, we could even just do the following in xl/libxl:
if ( PCI_devices )
error("Sorry - PCI not supported")
or temporarily remove XEN_DOMCTL_CDF_vpci until PCI passthrough gets
finalized.
Cheers,
--
Julien Grall
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
On Wed, Oct 06, 2021 at 06:40:33PM +0100, Rahul Singh wrote: > Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN. > Reject the use of this new flag for x86 as VPCI is not supported for > DOMU guests for x86. I don't like this approach, XEN_DOMCTL_CDF_vpci should be set for x86 PVH dom0, like we do for any other CDF flags when Xen builds dom0. Things like PVH vs PV get translated into CDF flags by create_dom0, and processed normally by the sanitise_domain_config logic, vPCI should be handled that way. Do you think you could see about fixing this? Thanks, Roger.
Re: [PATCH v5 2/2] tools/xenstore: set open file descriptor limit for xenstored
Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file
descriptor limit for xenstored"):
> On 28.09.21 17:26, Ian Jackson wrote:
> > Juergen Gross writes ("Re: [PATCH v5 2/2] tools/xenstore: set open file
> > descriptor limit for xenstored"):
> >> Hmm, maybe I should just use:
> >>
> >> prlimit --nofile=$XENSTORED_MAX_OPEN_FDS \
> >> $XENSTORED --pid-file @XEN_RUN_DIR@/xenstored.pid $XENSTORED_ARGS
> >
> > I guess that would probably work (although it involves another
> > exec) but I don't understand what's wrong with ulimit, which is a
> > shell builtin.
>
> My main concern with ulimit is that this would set the open file limit
> for _all_ children of the script. I don't think right now this is a real
> problem, but it feels wrong to me (systemd-notify ought to be fine, but
> you never know ...).
Oh, I see. Yes, that is a good point.
So, I think your suggest (quoted above) is good.
Thanks,
Ian.
Re: [PATCH v3 12/16] KVM: Move x86's perf guest info callbacks to generic KVM
On Wed, 22 Sep 2021 01:05:29 +0100,
Sean Christopherson wrote:
>
> Move x86's perf guest callbacks into common KVM, as they are semantically
> identical to arm64's callbacks (the only other such KVM callbacks).
> arm64 will convert to the common versions in a future patch.
>
> Implement the necessary arm64 arch hooks now to avoid having to provide
> stubs or a temporary #define (from x86) to avoid arm64 compilation errors
> when CONFIG_GUEST_PERF_EVENTS=y.
>
> Signed-off-by: Sean Christopherson
> ---
> arch/arm64/include/asm/kvm_host.h | 8 +
> arch/arm64/kvm/arm.c | 5 +++
> arch/x86/include/asm/kvm_host.h | 3 ++
> arch/x86/kvm/x86.c| 53 +++
> include/linux/kvm_host.h | 10 ++
> virt/kvm/kvm_main.c | 44 +
> 6 files changed, 81 insertions(+), 42 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index ed940aec89e0..828b6eaa2c56 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -673,6 +673,14 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t
> fault_ipa);
> void kvm_perf_init(void);
> void kvm_perf_teardown(void);
>
> +#ifdef CONFIG_GUEST_PERF_EVENTS
> +static inline bool kvm_arch_pmi_in_guest(struct kvm_vcpu *vcpu)
Pardon my x86 ignorance, what is PMI? PMU Interrupt?
> +{
> + /* Any callback while a vCPU is loaded is considered to be in guest. */
> + return !!vcpu;
> +}
> +#endif
Do you really need this #ifdef?
> +
> long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu);
> gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu);
> void kvm_update_stolen_time(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index e9a2b8f27792..2b542fdc237e 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -500,6 +500,11 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> return vcpu_mode_priv(vcpu);
> }
>
> +unsigned long kvm_arch_vcpu_get_ip(struct kvm_vcpu *vcpu)
> +{
> + return *vcpu_pc(vcpu);
> +}
> +
> /* Just ensure a guest exit from a particular CPU */
> static void exit_vm_noop(void *info)
> {
The above nits notwithstanding,
Acked-by: Marc Zyngier
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 0/6] gnttab: add per-domain controls
On Wed, Sep 22, 2021 at 10:21:17AM +0200, Roger Pau Monne wrote: > Hello, > > First patch on the series is a trivial change to xenconsoled in order to > use xenforeignmemory stable library in order to map the shared console > ring instead of the unstable libxc interface. It's reviewed and ready to > go in. > > Patches 2 and 3 allow setting the host wide command line `gnttab` option > on a per domain basis. That means selecting the max allowed grant table > version and whether transitive grants are allowed. > > The last 3 patches attempt to implement support for creating guests > without a grant table. This requires some changes to xenstored in order > to partially support guests without a valid ring interface, as the lack > of grant table will prevent C xenstored from mapping the shared ring. > Note this is not an issue for Ocaml xenstored, as it still uses the > foreign memory interface to map the shared ring, and thus won't notice > the lack of grant table support on the domain. > > Thanks, Roger. > > Roger Pau Monne (6): > tools/console: use xenforeigmemory to map console ring > gnttab: allow setting max version per-domain > gnttab: allow per-domain control over transitive grants Ping? The two patches above didn't get any review in either v1 or v2. Patch #1 should be ready to go in AFAICT. Thanks, Roger.
Re: [PATCH] arm/efi: Fix null pointer dereference
Hi Luca,
> On 11 Oct 2021, at 08:56, Luca Fancellu wrote:
>
> Fix for commit 60649d443dc395243e74d2b3e05594ac0c43cfe3
> that introduces a null pointer dereference when the
> fdt_node_offset_by_compatible is called with "fdt"
> argument null.
>
> Reported-by: Julien Grall
> Fixes: 60649d443d ("arm/efi: Introduce xen,uefi-cfg-load DT property")
> Signed-off-by: Luca Fancellu
Reviewed-by: Bertrand Marquis
Cheers
Bertrand
> ---
> xen/arch/arm/efi/efi-boot.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index a3e46453d4..e63dafac26 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -593,7 +593,8 @@ static bool __init
> efi_arch_use_config_file(EFI_SYSTEM_TABLE *SystemTable)
> dtbfile.ptr = fdt;
> dtbfile.need_to_free = false; /* Config table memory can't be freed. */
>
> -if ( fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0 )
> +if ( fdt &&
> + (fdt_node_offset_by_compatible(fdt, 0, "multiboot,module") > 0) )
> {
> /* Locate chosen node */
> int node = fdt_subnode_offset(fdt, 0, "chosen");
> --
> 2.17.1
>
Re: [PATCH v3 14/16] KVM: arm64: Convert to the generic perf callbacks
On Wed, 22 Sep 2021 01:05:31 +0100,
Sean Christopherson wrote:
>
> Drop arm64's version of the callbacks in favor of the callbacks provided
> by generic KVM, which are semantically identical.
>
> Signed-off-by: Sean Christopherson
> ---
> arch/arm64/kvm/perf.c | 34 ++
> 1 file changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm64/kvm/perf.c b/arch/arm64/kvm/perf.c
> index 3e99ac4ab2d6..0b902e0d5b5d 100644
> --- a/arch/arm64/kvm/perf.c
> +++ b/arch/arm64/kvm/perf.c
> @@ -13,45 +13,15 @@
>
> DEFINE_STATIC_KEY_FALSE(kvm_arm_pmu_available);
>
> -static unsigned int kvm_guest_state(void)
> -{
> - struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> - unsigned int state;
> -
> - if (!vcpu)
> - return 0;
> -
> - state = PERF_GUEST_ACTIVE;
> - if (!vcpu_mode_priv(vcpu))
> - state |= PERF_GUEST_USER;
> -
> - return state;
> -}
> -
> -static unsigned long kvm_get_guest_ip(void)
> -{
> - struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> -
> - if (WARN_ON_ONCE(!vcpu))
> - return 0;
> -
> - return *vcpu_pc(vcpu);
> -}
> -
> -static struct perf_guest_info_callbacks kvm_guest_cbs = {
> - .state = kvm_guest_state,
> - .get_ip = kvm_get_guest_ip,
> -};
> -
> void kvm_perf_init(void)
> {
> if (kvm_pmu_probe_pmuver() != 0xf && !is_protected_kvm_enabled())
> static_branch_enable(&kvm_arm_pmu_available);
>
> - perf_register_guest_info_callbacks(&kvm_guest_cbs);
> + kvm_register_perf_callbacks(NULL);
> }
>
> void kvm_perf_teardown(void)
> {
> - perf_unregister_guest_info_callbacks(&kvm_guest_cbs);
> + kvm_unregister_perf_callbacks();
> }
Reviewed-by: Marc Zyngier
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
Hi Luca,
On 11/10/2021 09:03, Luca Fancellu wrote:
+static bool __init is_boot_module(int dt_module_offset)
+{
+if ( (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,kernel") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,ramdisk") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,device-tree") == 0) )
+return true;
A boot module *must* have the compatible "multiboot,module". I would
prefer if we simply check that "multiboot,module" is present.
This will also make easier to add new boot module in the future.
+
+return false;
+}
+
Cheers,
--
Julien Grall
Re: [PATCH v3 15/16] KVM: arm64: Drop perf.c and fold its tiny bits of code into arm.c / pmu.c
On Wed, 22 Sep 2021 01:05:32 +0100,
Sean Christopherson wrote:
>
> Call KVM's (un)register perf callbacks helpers directly from arm.c, and
> move the PMU bits into pmu.c and rename the related helper accordingly.
>
> Signed-off-by: Sean Christopherson
> ---
> arch/arm64/include/asm/kvm_host.h | 3 ---
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/arm.c | 6 --
> arch/arm64/kvm/perf.c | 27 ---
> arch/arm64/kvm/pmu.c | 8
> include/kvm/arm_pmu.h | 1 +
> 6 files changed, 14 insertions(+), 33 deletions(-)
> delete mode 100644 arch/arm64/kvm/perf.c
>
> diff --git a/arch/arm64/include/asm/kvm_host.h
> b/arch/arm64/include/asm/kvm_host.h
> index 828b6eaa2c56..f141ac65f4f1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -670,9 +670,6 @@ unsigned long kvm_mmio_read_buf(const void *buf, unsigned
> int len);
> int kvm_handle_mmio_return(struct kvm_vcpu *vcpu);
> int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa);
>
> -void kvm_perf_init(void);
> -void kvm_perf_teardown(void);
> -
> #ifdef CONFIG_GUEST_PERF_EVENTS
> static inline bool kvm_arch_pmi_in_guest(struct kvm_vcpu *vcpu)
> {
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index 989bb5dad2c8..0bcc378b7961 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -12,7 +12,7 @@ obj-$(CONFIG_KVM) += hyp/
>
> kvm-y := $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/eventfd.o \
>$(KVM)/vfio.o $(KVM)/irqchip.o $(KVM)/binary_stats.o \
> - arm.o mmu.o mmio.o psci.o perf.o hypercalls.o pvtime.o \
> + arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
>inject_fault.o va_layout.o handle_exit.o \
>guest.o debug.o reset.o sys_regs.o \
>vgic-sys-reg-v3.o fpsimd.o pmu.o \
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 2b542fdc237e..48f89d80f464 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1744,7 +1744,9 @@ static int init_subsystems(void)
> if (err)
> goto out;
>
> - kvm_perf_init();
> + kvm_pmu_init();
> + kvm_register_perf_callbacks(NULL);
> +
> kvm_sys_reg_table_init();
>
> out:
> @@ -2160,7 +2162,7 @@ int kvm_arch_init(void *opaque)
> /* NOP: Compiling as a module not supported */
> void kvm_arch_exit(void)
> {
> - kvm_perf_teardown();
> + kvm_unregister_perf_callbacks();
> }
>
> static int __init early_kvm_mode_cfg(char *arg)
> diff --git a/arch/arm64/kvm/perf.c b/arch/arm64/kvm/perf.c
> deleted file mode 100644
> index 0b902e0d5b5d..
> --- a/arch/arm64/kvm/perf.c
> +++ /dev/null
> @@ -1,27 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Based on the x86 implementation.
> - *
> - * Copyright (C) 2012 ARM Ltd.
> - * Author: Marc Zyngier
> - */
> -
> -#include
> -#include
> -
> -#include
> -
> -DEFINE_STATIC_KEY_FALSE(kvm_arm_pmu_available);
> -
> -void kvm_perf_init(void)
> -{
> - if (kvm_pmu_probe_pmuver() != 0xf && !is_protected_kvm_enabled())
> - static_branch_enable(&kvm_arm_pmu_available);
> -
> - kvm_register_perf_callbacks(NULL);
> -}
> -
> -void kvm_perf_teardown(void)
> -{
> - kvm_unregister_perf_callbacks();
> -}
> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 03a6c1f4a09a..d98b57a17043 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -7,6 +7,14 @@
> #include
> #include
>
> +DEFINE_STATIC_KEY_FALSE(kvm_arm_pmu_available);
> +
> +void kvm_pmu_init(void)
> +{
> + if (kvm_pmu_probe_pmuver() != 0xf && !is_protected_kvm_enabled())
> + static_branch_enable(&kvm_arm_pmu_available);
> +}
> +
> /*
> * Given the perf event attributes and system type, determine
> * if we are going to need to switch counters at guest entry/exit.
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 864b9997efb2..42270676498d 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -14,6 +14,7 @@
> #define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>
> DECLARE_STATIC_KEY_FALSE(kvm_arm_pmu_available);
> +void kvm_pmu_init(void);
>
> static __always_inline bool kvm_arm_support_pmu_v3(void)
> {
Note that this patch is now conflicting with e840f42a4992 ("KVM:
arm64: Fix PMU probe ordering"), which was merged in -rc4. Moving the
static key definition to arch/arm64/kvm/pmu-emul.c and getting rid of
kvm_pmu_init() altogether should be enough to resolve it.
With that,
Reviewed-by: Marc Zyngier
M.
--
Without deviation from the norm, progress is not possible.
Re: Ping: [PATCH v2 3/3] AMD/IOMMU: consider hidden devices when flushing device I/O TLBs
On 11/10/2021 09:04, Jan Beulich wrote: On 17.09.2021 13:00, Jan Beulich wrote: Hidden devices are associated with DomXEN but usable by the hardware domain. Hence they need flushing as well when all devices are to have flushes invoked. While there drop a redundant ATS-enabled check and constify the first parameter of the involved function. Signed-off-by: Jan Beulich The VT-d side equivalent having gone in a while ago, I think it would be good to have the AMD side on par. Agreed. Reviewed-by: Paul Durrant
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Andrew Cooper writes ("Re: [PATCH v5 07/11] xen/domctl: Introduce
XEN_DOMCTL_CDF_vpci flag"):
> Ian, for the 4.16 release, this series either needs completing with the
> additional flag implemented, or this patch needs reverting to avoid us
> shipping a broken interface.
I have caught up on this thread. I think (hope?) it's converging.
If not please let me know and maybe I can help.
Can I ask to please be CC'd on the whole series for the patch(es) to
sort this out. Please also make sure that those who commented are
CC'd. I want the fixes that ultimately get committed to be the final
fixes (probably that means they should have consensus).
FTAOD, from a formal release management point of view: I regard those
putative fixes as bugfixes so they can go in after the feature freeze
(which is this Friday). But if suitable fixes don't make it in within
the first few weeks of the freeze (and, as I expect, the maintainers
or I still regard this as an RC bug) then a revert of the new feature
will be the only option.
Ian.
Re: [PATCH v2 0/6] gnttab: add per-domain controls
On 11 Oct 2021, at 10:36, Roger Pau Monne mailto:roger@citrix.com>> wrote: Ping? The two patches above didn't get any review in either v1 or v2. Patch #1 should be ready to go in AFAICT. Acked-by: Christian Lindig mailto:christian.lin...@citrix.com>>
[libvirt test] 165461: regressions - FAIL
flight 165461 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/165461/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt 5ee4f3e1d4f173f7e1b64b745ab9ef5dc8c8f393 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 458 days Failing since151818 2020-07-11 04:18:52 Z 457 days 443 attempts Testing same since 165461 2021-10-11 04:20:09 Z0 days1 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Andika Triwidada Andrea Bolognani Ani Sinha Balázs Meskó Barrett Schonefeld Bastian Germann Bastien Orivel BiaoXiang Ye Bihong Yu Binfeng Wu Bjoern Walk Boris Fiuczynski Brian Turek Bruno Haible Chris Mayo Christian Borntraeger Christian Ehrhardt Christian Kirbach Christian Schoenebeck Cole Robinson Collin Walling Cornelia Huck Cédric Bosdonnat Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé Didik Supriadi dinglimin Dmytro Linkin Eiichi Tsukata Eric Farman Erik Skultety Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation gongwei Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Hela Basa Helmut Grohne Hiroki Narukawa Ian Wienand Jakob Meng Jamie Strandboge Jamie Strandboge Jan Kuparinen jason lee Jean-Baptiste Holcroft Jia Zhou Jianan Gao Jim Fehlig Jin Yan Jinsheng Zhang Jiri Denemark John Ferlan Jonathan Watt Jonathon Jongsma Julio Faracco Justin Gatzen Ján Tomko Kashyap Chamarthy Kevin Locke Kristina Hanicova Laine Stump Laszlo Ersek Lee Yarwood Lei Yang Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Luke Yue Luyao Zhong Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Markus Schade Martin Kletzander Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Meina Li Michal Privoznik Michał Smyk Milo Casagrande Moshe Levi Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nikolay Shirokovskiy Olaf Hering Olesya Gerasimenko Orion Poplawski Pany Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peng Liang Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Richard W.M. Jones Ricky Tigg Robin Lee Roman Bogorodskiy Roman Bolshakov Ryan Gahagan Ryan Schmidt Sam Hartman Scott Shambarger Sebastian Mitterle SeongHyun Jo Shalini Chellathurai Saroja Shaojun Yang Shi Lei simmon Simon Chopin Simon Gaiser Simon Rowe Stefan Bader Stefan Berger Stefan Berger Stefan Hajnoczi Stefan Hajnoczi Szymon Scholz Thomas Huth Tim Wiederhake Tomáš Golembiovský Tomáš Janoušek Tuguoyi Victor Toso Ville Skyttä Vinayak Kale Wang Xin WangJian We
Re: [PATCH v2] x86/PV32: fix physdev_op_compat handling
On Mon, Oct 11, 2021 at 10:20:41AM +0200, Jan Beulich wrote:
> The conversion of the original code failed to recognize that the 32-bit
> compat variant of this (sorry, two different meanings of "compat" here)
> needs to continue to invoke the compat handler, not the native one.
> Arrange for this by adding yet another #define.
>
> Affected functions (having existed prior to the introduction of the new
> hypercall) are PHYSDEVOP_set_iobitmap and PHYSDEVOP_apic_{read,write}.
> For all others the operand struct layout doesn't differ.
>
> Fixes: 1252e2823117 ("x86/pv: Export pv_hypercall_table[] rather than working
> around it in several ways")
> Signed-off-by: Jan Beulich
Reviewed-by: Roger Pau Monné
Thanks, Roger.
Re: [PATCH v4 2/3] xen/arm: Check for PMU platform support
Hi Michal,
On 11/10/2021 10:00, Michal Orzel wrote:
ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
information about PMU support. Replace structure
dbg64/dbg32 with a union and fill in all the
register fields according to document:
ARM Architecture Registers(DDI 0595, 2021-06).
Add macros boot_dbg_feature64/boot_dbg_feature32
to check for a debug feature. Add macro
cpu_has_pmu to check for PMU support.
Signed-off-by: Michal Orzel
Reviewed-by: Stefano Stabellini
Reviewed-by: Bertrand Marquis
---
Changes since v3:
-none
Changes since v2:
-none
Changes since v1:
-new in v2
---
xen/include/asm-arm/cpufeature.h | 49 ++--
1 file changed, 47 insertions(+), 2 deletions(-)
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 5ca09b0bff..4fce23844d 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_ARM_64
#define cpu_feature64(c, feat) ((c)->pfr64.feat)
#define boot_cpu_feature64(feat) (system_cpuinfo.pfr64.feat)
+#define boot_dbg_feature64(feat) (system_cpuinfo.dbg64.feat)
#define cpu_feature64_has_el0_32(c)(cpu_feature64(c, el0) == 2)
@@ -22,6 +23,7 @@
#define cpu_feature32(c, feat) ((c)->pfr32.feat)
#define boot_cpu_feature32(feat) (system_cpuinfo.pfr32.feat)
+#define boot_dbg_feature32(feat) (system_cpuinfo.dbg32.feat)
#define cpu_has_arm (boot_cpu_feature32(arm) == 1)
#define cpu_has_thumb (boot_cpu_feature32(thumb) >= 1)
@@ -32,8 +34,10 @@
#ifdef CONFIG_ARM_32
#define cpu_has_gentimer (boot_cpu_feature32(gentimer) == 1)
+#define cpu_has_pmu (boot_dbg_feature32(perfmon) >= 1)
From my understanding, on Armv7, perfmon == 0 only means PMUv2 is not
present. IOW, it doesn't say whether PMUv1 is supported.
I think it is OK to treat as the PMU is not supported (ARMv8 treat it
like that too), but I would like a comment in the code so it is clear
this is a deliberate choice.
#else
#define cpu_has_gentimer (1)
+#define cpu_has_pmu (boot_dbg_feature64(pmu_ver) >= 1)
#endif
#define cpu_has_security (boot_cpu_feature32(security) > 0)
@@ -181,8 +185,28 @@ struct cpuinfo_arm {
};
} pfr64;
-struct {
+union {
register_t bits[2];
+struct {
+/* DFR0 */
+unsigned long debug_ver:4;
+unsigned long trace_ver:4;
+unsigned long pmu_ver:4;
+unsigned long brps:4;
+unsigned long __res0:4;
+unsigned long wrps:4;
+unsigned long __res1:4;
+unsigned long ctx_cmps:4;
+unsigned long pms_ver:4;
+unsigned long double_lock:4;
+unsigned long trace_filt:4;
+unsigned long __res2:4;
+unsigned long mtpmu:4;
+unsigned long __res3:12;
+
+/* DFR1 */
+unsigned long __res4:64;
+};
} dbg64;
struct {
@@ -321,8 +345,29 @@ struct cpuinfo_arm {
};
} pfr32;
-struct {
+union {
register_t bits[2];
+struct {
+/* DFR0 */
+unsigned long copdbg:4;
+unsigned long copsdbg:4;
+unsigned long mmapdbg:4;
+unsigned long coptrc:4;
+unsigned long mmaptrc:4;
+unsigned long mprofdbg:4;
+unsigned long perfmon:4;
+unsigned long tracefilt:4;
+#ifdef CONFIG_ARM_64
+unsigned long __res0:32;
+#endif
+
+/* DFR1 */
+unsigned long mtpmu:4;
+unsigned long __res1:28;
+#ifdef CONFIG_ARM_64
+unsigned long __res2:32;
+#endif
+};
} dbg32;
struct {
Cheers,
--
Julien Grall
Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
Hi Michal,
On 11/10/2021 10:00, Michal Orzel wrote:
Add parameter vpmu to xl domain configuration syntax
to enable the access to PMU registers by disabling
the PMU traps(currently only for ARM).
The current status is that the PMU registers are not
virtualized and the physical registers are directly
accessible when this parameter is enabled. There is no
interrupt support and Xen will not save/restore the
register values on context switches.
Please note that this feature is experimental.
Signed-off-by: Michal Orzel
Signed-off-by: Julien Grall
Reviewed-by: Bertrand Marquis
---
Changes since v3:
-fail if vpmu is set but not supported
-rebase on top of latest staging
Changes since v2:
-remove redundant check from x86 code
-do not define bit position and mask separately
Changes since v1:
-modify vpmu parameter to be common rather than arch specific
---
docs/man/xl.cfg.5.pod.in | 17 +
tools/golang/xenlight/helpers.gen.go | 6 ++
tools/golang/xenlight/types.gen.go | 1 +
tools/include/libxl.h| 6 ++
tools/libs/light/libxl_create.c | 10 ++
tools/libs/light/libxl_types.idl | 2 ++
tools/ocaml/libs/xc/xenctrl.ml | 1 +
tools/ocaml/libs/xc/xenctrl.mli | 1 +
tools/xl/xl_parse.c | 2 ++
xen/arch/arm/domain.c| 12 +---
xen/arch/arm/setup.c | 1 +
xen/common/domain.c | 10 +-
xen/include/asm-arm/domain.h | 1 +
xen/include/public/domctl.h | 4 +++-
14 files changed, 69 insertions(+), 5 deletions(-)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 4b1e3028d2..55c4881205 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -690,6 +690,23 @@ default.
B: Acceptable values are platform specific. For Intel Processor
Trace, this value must be a power of 2 between 4k and 16M.
+=item B
+
+Currently ARM only.
+
+Specifies whether to enable the access to PMU registers by disabling
+the PMU traps.
+
+The PMU registers are not virtualized and the physical registers are directly
+accessible when this parameter is enabled. There is no interrupt support and
+Xen will not save/restore the register values on context switches.
+
+vPMU, by design and purpose, exposes system level performance
+information to the guest. Only to be used by sufficiently privileged
+domains. This feature is currently in experimental state.
Please update SUPPORT.MD to mention the support of this feature.
+
+If this option is not specified then it will default to B.
+
=back
=head2 Devices
diff --git a/tools/golang/xenlight/helpers.gen.go
b/tools/golang/xenlight/helpers.gen.go
index c8669837d8..2449580bad 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed:
%v", err)
}
x.Altp2M = Altp2MMode(xc.altp2m)
x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
+if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
return nil}
@@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field ArchX86.MsrRelaxed: %v", err)
}
xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
+if err := x.Vpmu.toC(&xc.vpmu); err != nil {
+return fmt.Errorf("converting field Vpmu: %v", err)
+}
return nil
}
diff --git a/tools/golang/xenlight/types.gen.go
b/tools/golang/xenlight/types.gen.go
index 45f2cba3d2..b2e8bd1a85 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -521,6 +521,7 @@ MsrRelaxed Defbool
}
Altp2M Altp2MMode
VmtraceBufKb int
+Vpmu Defbool
}
type DomainBuildInfoTypeUnion interface {
diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index ec5e3badae..ee73eb06f1 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -508,6 +508,12 @@
*/
#define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
+/*
+ * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu parameter,
+ * which allows to enable the access to PMU registers.
+ */
+#define LIBXL_HAVE_VPMU 1
+
/*
* libxl ABI compatibility
*
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..2a0234ec16 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
}
libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
+libxl_defbool_setdefault(&b_info->vpmu, false);
if (libxl_defbool_val(b_info->device_model_stubdomain) &&
!b_info->device_model_ssidref)
@@ -622,6 +623,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config
*d_config,
create.flags |= XEN_DOMCTL_CDF_nested_virt;
}
+if ( libxl_defbool_val(b_info->vpmu) )
+crea
Re: [XEN PATCH v7 18/51] build: fix $(TARGET).efi creation in arch/arm
On 24.08.2021 12:50, Anthony PERARD wrote: > There is no need to try to guess a relative path to the "xen.efi" file, > we can simply use $@. Also, there's no need to use `notdir`, make > already do that work via $(@F). > > Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich As to the subject, I don't think "fix" is appropriate. How about "adjust" or "simplify" or some such? Jan
Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
Hi Jan, On 28/09/2021 13:05, Jan Beulich wrote: On 23.09.2021 11:54, Julien Grall wrote: On 23/09/2021 08:11, Penny Zheng wrote: From: Stefano Stabellini We are passing an extra special boolean flag at domain creation to specify whether we want to the domain to be privileged (i.e. dom0) or not. Another flag will be introduced later in this series. Reserve bits 16-31 from the existing flags bitfield in struct xen_domctl_createdomain for internal Xen usage. I am a bit split with this approach. This feels a bit of a hack to reserve bits for internal purpose in external headers. But at the same time I can see how this is easier to deal with it over repurposing the last argument of domain_create(). I actually have trouble seeing why that's easier. It is a common thing to widen a bool to "unsigned int flags" when more than one control is needed. I was suggesting this is easier for the following two reasons: 1) All the option flags (internal and external) are in a single place. 2) Reduce the risk to make a mistake when widening the field. In particular in the context of backporting. Although, this looks unlikely here. Plus this makes things needlessly harder once (in the future) the low 16 bits are exhausted in the public interface. That's why I suggested this sounds like a hack. At the same time the split between external vs internal option is a bit more a pain for the developper. So I didn't feel pushing for one vs the other. That said, I will not argue against if you want to push for repurposing the last argument. Cheers, -- Julien Grall
Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native UART address and IRQ number for vPL011
On 09/10/2021 09:47, Penny Zheng wrote:
Hi Julien
Hi Penny,
-Original Message-
From: Julien Grall
Sent: Thursday, September 23, 2021 7:14 PM
To: Penny Zheng ; xen-devel@lists.xenproject.org;
sstabell...@kernel.org
Cc: Bertrand Marquis ; Wei Chen
Subject: Re: [PATCH 09/11] xen/arm: if 1:1 direct-map domain use native
UART address and IRQ number for vPL011
On 23/09/2021 08:11, Penny Zheng wrote:
From: Stefano Stabellini
We always use a fix address to map the vPL011 to domains. The address
could be a problem for domains that are directly mapped.
So, for domains that are directly mapped, reuse the address of the
physical UART on the platform to avoid potential clashes.
Do the same for the virtual IRQ number: instead of always using
GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
Signed-off-by: Stefano Stabellini
Signed-off-by: Penny Zheng
---
xen/arch/arm/domain_build.c | 34 +++---
xen/arch/arm/vpl011.c| 34 +++---
xen/include/asm-arm/vpl011.h | 2 ++
3 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 120f1ae575..c92e510ae7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -30,6 +30,7 @@
#include
#include
+#include
static unsigned int __initdata opt_dom0_max_vcpus;
integer_param("dom0_max_vcpus", opt_dom0_max_vcpus); @@ -1942,8
+1943,11 @@ static int __init make_vpl011_uart_node(struct kernel_info
*kinfo)
gic_interrupt_t intr;
__be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
__be32 *cells;
+struct domain *d = kinfo->d;
+char buf[27];
-res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
+snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d-
arch.vpl011.base_addr);
+res = fdt_begin_node(fdt, buf);
if ( res )
return res;
@@ -1953,14 +1957,14 @@ static int __init make_vpl011_uart_node(struct
kernel_info *kinfo)
cells = ®[0];
dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
- GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
+ GUEST_ROOT_SIZE_CELLS,
+ d->arch.vpl011.base_addr,
GUEST_PL011_SIZE);
res = fdt_property(fdt, "reg", reg, sizeof(reg));
if ( res )
return res;
-set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+set_interrupt(intr, d->arch.vpl011.virq, 0xf,
+ DT_IRQ_TYPE_LEVEL_HIGH);
res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
if ( res )
@@ -2670,6 +2674,13 @@ static int __init construct_domU(struct domain
*d,
else
allocate_static_memory(d, &kinfo, node);
+/*
+ * Initialization before creating its device
+ * tree node in prepare_dtb_domU.
+ */
I think it would be better to explain *why* this needs to be done before.
+if ( kinfo.vpl011 )
+rc = domain_vpl011_init(d, NULL);
+
rc = prepare_dtb_domU(d, &kinfo);
if ( rc < 0 )
return rc;
@@ -2678,9 +2689,6 @@ static int __init construct_domU(struct domain
*d,
if ( rc < 0 )
return rc;
-if ( kinfo.vpl011 )
-rc = domain_vpl011_init(d, NULL);
-
return rc;
}
@@ -2723,15 +2731,27 @@ void __init create_domUs(void)
if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
{
+unsigned int vpl011_virq = GUEST_VPL011_SPI;
Coding style: Add a newline here.
d_cfg.arch.nr_spis = gic_number_lines() - 32;
+/*
+ * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map in
+ * set, in which case we'll try to match the hardware.
+ *
+ * Typically, d->arch.vpl011.virq has the vpl011 irq number
+ * but at this point of the boot sequence it is not
+ * initialized yet.
+ */
+if ( direct_map && serial_irq(SERHND_DTUART) > 0 )
+vpl011_virq = serial_irq(SERHND_DTUART);
I think we should not continue if the domain is direct-mapped *and* the IRQ
is not found. Otherwise, this will may just result to potential breakage if
GUEST_VPL011_SPI happens to be used for an HW device.
+
/*
* vpl011 uses one emulated SPI. If vpl011 is requested, make
* sure that we allocate enough SPIs for it.
*/
if ( dt_property_read_bool(node, "vpl011") )
d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
- GUEST_VPL011_SPI - 32 + 1);
+ vpl011_virq - 32 + 1);
}
/*
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index
895f436cc4..10df25f098 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -29,6 +29,7 @@
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
On Wed, Oct 06, 2021 at 06:40:34PM +0100, Rahul Singh wrote:
> The existing VPCI support available for X86 is adapted for Arm.
> When the device is added to XEN via the hyper call
> “PHYSDEVOP_pci_device_add”, VPCI handler for the config space
> access is added to the Xen to emulate the PCI devices config space.
>
> A MMIO trap handler for the PCI ECAM space is registered in XEN
> so that when guest is trying to access the PCI config space,XEN
> will trap the access and emulate read/write using the VPCI and
> not the real PCI hardware.
>
> For Dom0less systems scan_pci_devices() would be used to discover the
> PCI device in XEN and VPCI handler will be added during XEN boots.
>
> Signed-off-by: Rahul Singh
> Reviewed-by: Stefano Stabellini
> ---
> Change in v5:
> - Add pci_cleanup_msi(pdev) in cleanup part.
> - Added Reviewed-by: Stefano Stabellini
> Change in v4:
> - Move addition of XEN_DOMCTL_CDF_vpci flag to separate patch
> Change in v3:
> - Use is_pci_passthrough_enabled() in place of pci_passthrough_enabled
> variable
> - Reject XEN_DOMCTL_CDF_vpci for x86 in arch_sanitise_domain_config()
> - Remove IS_ENABLED(CONFIG_HAS_VPCI) from has_vpci()
> Change in v2:
> - Add new XEN_DOMCTL_CDF_vpci flag
> - modify has_vpci() to include XEN_DOMCTL_CDF_vpci
> - enable vpci support when pci-passthough option is enabled.
> ---
> ---
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/domain.c | 4 ++
> xen/arch/arm/domain_build.c | 3 +
> xen/arch/arm/vpci.c | 102 ++
> xen/arch/arm/vpci.h | 36
> xen/drivers/passthrough/pci.c | 18 ++
> xen/include/asm-arm/domain.h | 7 ++-
> xen/include/asm-x86/pci.h | 2 -
> xen/include/public/arch-arm.h | 7 +++
> xen/include/xen/pci.h | 2 +
> 10 files changed, 179 insertions(+), 3 deletions(-)
> create mode 100644 xen/arch/arm/vpci.c
> create mode 100644 xen/arch/arm/vpci.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 44d7cc81fa..fb9c976ea2 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -7,6 +7,7 @@ ifneq ($(CONFIG_NO_PLAT),y)
> obj-y += platforms/
> endif
> obj-$(CONFIG_TEE) += tee/
> +obj-$(CONFIG_HAS_VPCI) += vpci.o
>
> obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
> obj-y += bootfdt.init.o
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 36138c1b2e..fbb52f78f1 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -39,6 +39,7 @@
> #include
> #include
>
> +#include "vpci.h"
> #include "vuart.h"
>
> DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
> @@ -767,6 +768,9 @@ int arch_domain_create(struct domain *d,
> if ( is_hardware_domain(d) && (rc = domain_vuart_init(d)) )
> goto fail;
>
> +if ( (rc = domain_vpci_init(d)) != 0 )
> +goto fail;
> +
> return 0;
>
> fail:
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c5afbe2e05..f4c89bde8c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3053,6 +3053,9 @@ void __init create_dom0(void)
> if ( iommu_enabled )
> dom0_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>
> +if ( is_pci_passthrough_enabled() )
> +dom0_cfg.flags |= XEN_DOMCTL_CDF_vpci;
I think I'm confused with this. You seem to enable vPCI for dom0, but
then domain_vpci_init will setup traps for the guest virtual ECAM
layout, not the native one that dom0 will be using.
> +
> dom0 = domain_create(0, &dom0_cfg, true);
> if ( IS_ERR(dom0) || (alloc_dom0_vcpu0(dom0) == NULL) )
> panic("Error creating domain 0\n");
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> new file mode 100644
> index 00..76c12b9281
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,102 @@
> +/*
> + * xen/arch/arm/vpci.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include
> +
> +#include
> +
> +#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
> +
> +/* Do some sanity checks. */
> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
> +{
> +/* Check access size. */
> +if ( len > 8 )
> +return false;
> +
> +/* Check that access is size aligned. */
> +if ( (reg & (len - 1)) )
> +return false;
> +
> +return true;
> +}
There's a vpci_access_allowed which I think you could generalize and
use here, there's no need to have this duplicated code.
Thanks, Roger.
Re: [PATCH 0/2] VT-d: correct / extend workaround(s) leaving an IOMMU disabled [and 1 more messages]
Jan Beulich writes ("[PATCH 0/2] VT-d: correct / extend workaround(s) leaving
an IOMMU disabled"):
> Ian - I'm also Cc-ing you since this feels like being on the edge
> between a new feature and a bug fix.
Thanks.
I think 2/ is a new quirk (or, new behaviour for an existing quirk).
I think I am happy to treat that as a bugfix, assuming we are
reasonably confident that most systems (including in particular all
systems without the quirk) will take unchanged codepaths. Is that
right ?
I don't understand 1/. It looks bugfixish to me but I am really not
qualified. I am inclined to defer to your judgement, but it would
help me if you explicitly addressed the overall risks/benefits.
But when reading the patch I did notice one thing that struck me as
undesriable:
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -750,27 +750,43 @@ static void iommu_enable_translation(str
> if ( force_iommu )
> -panic("BIOS did not enable IGD for VT properly, crash Xen
> for security purpose\n");
> +panic(crash_fmt, msg);
...
> +if ( force_iommu )
> +panic(crash_fmt, msg);
Does this really mean that every exit path from
iommu_enable_translation that doesn't enable the iommu has to have a
check for force_iommu ?
That seems like a recipe for missing one. And I think a missed one
would be an XSA. Could we not structure the code some way to avoid
this foreseeable human error ?
Ian.
Re: [XEN PATCH v7 20/51] build: avoid re-executing the main Makefile by introducing build.mk
On 24.08.2021 12:50, Anthony PERARD wrote: > Currently, the xen/Makefile is re-parsed several times: once to start > the build process, and several more time with Rules.mk including it. > This makes it difficult to reason with a Makefile used for several > purpose, and it actually slow down the build process. I'm struggling some with what you want to express here. What does "to reason" refer to? > So this patch introduce "build.mk" which Rules.mk will use when > present instead of the "Makefile" of a directory. (Linux's Kbuild > named that file "Kbuild".) > > We have a few targets to move to "build.mk" identified by them been > build via "make -f Rules.mk" without changing directory. > > As for the main targets like "build", we can have them depends on > there underscore-prefix targets like "_build" without having to use > "Rules.mk" while still retaining the check for unsupported > architecture. (Those main rules are changed to be single-colon as > there should only be a single recipe for them.) > > With nearly everything needed to move to "build.mk" moved, there is a > single dependency left from "Rules.mk": $(TARGET), which is moved to > the main Makefile. I'm having trouble identifying what this describes. Searching for $(TARGET) in the patch doesn't yield any obvious match. Thinking about it, do you perhaps mean the setting of that variable? Is moving that guaranteed to not leave the variable undefined? Or in other words is there no scenario at all where xen/Makefile might get bypassed? (Aiui building an individual .o, .i, or .s would continue to be fine, but it feels like something along these lines might get broken.) > @@ -279,11 +281,13 @@ export CFLAGS_UBSAN > > endif # need-config > > -.PHONY: build install uninstall clean distclean MAP > -build install uninstall debug clean distclean MAP:: > +main-targets := build install uninstall clean distclean MAP > +.PHONY: $(main-targets) > ifneq ($(XEN_TARGET_ARCH),x86_32) > - $(MAKE) -f Rules.mk _$@ > +$(main-targets): %: _% > + @: Isn't the conventional way to express "no commands" via $(main-targets): %: _% ; ? > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -9,8 +9,6 @@ include $(XEN_ROOT)/Config.mk > include $(BASEDIR)/scripts/Kbuild.include > > > -TARGET := $(BASEDIR)/xen > - > # Note that link order matters! Could I talk you into removing yet another blank line at this occasion? > @@ -36,7 +34,9 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ > rodata.cst$(a)) \ > $(foreach r,rel rel.ro,data.$(r).local) > > -include Makefile > +# The filename build.mk has precedence over Makefile > +mk-dir := . What's the goal of this variable? All I can spot for now it that ... > +include $(if $(wildcard > $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile) ... this is harder to read than include $(if $(wildcard ./build.mk),./build.mk,./Makefile) which could be further simplified to include $(if $(wildcard build.mk),build.mk,Makefile) and then maybe altered to include $(firstword $(wildcard build.mk) Makefile) > --- /dev/null > +++ b/xen/build.mk > @@ -0,0 +1,58 @@ > +quiet_cmd_banner = BANNER $@ > +define cmd_banner > +if which figlet >/dev/null 2>&1 ; then \ > + echo " Xen $(XEN_FULLVERSION)" | figlet -f $< > $@.tmp; \ > +else \ > + echo " Xen $(XEN_FULLVERSION)" > $@.tmp; \ > +fi; \ > +mv -f $@.tmp $@ > +endef > + > +.banner: tools/xen.flf FORCE > + $(call if_changed,banner) > +targets += .banner To make the end of the rule more easily recognizable, may I ask that you either insert a blank line after the rule or that you move the += up immediately ahead of the construct? Jan
Re: [PATCH v4 1/3] xen+tools: Introduce XEN_SYSCTL_PHYSCAP_vpmu
Michal Orzel writes ("[PATCH v4 1/3] xen+tools: Introduce
XEN_SYSCTL_PHYSCAP_vpmu"):
> Introduce flag XEN_SYSCTL_PHYSCAP_vpmu which
> indicates whether the platform supports vPMU
> functionality. Modify Xen and tools accordingly.
>
> Take the opportunity and fix XEN_SYSCTL_PHYSCAP_vmtrace
> definition in sysctl.h which wrongly use (1 << 6)
> instead of (1u << 6).
>
> Signed-off-by: Michal Orzel
> Reviewed-by: Bertrand Marquis
> Acked-by: Nick Rosbrook
> Reviewed-by: Stefano Stabellini
Tools:
Acked-by: Ian Jackson
Re: [PATCH 0/2] VT-d: correct / extend workaround(s) leaving an IOMMU disabled [and 1 more messages]
On 11.10.2021 12:56, Ian Jackson wrote:
> Jan Beulich writes ("[PATCH 0/2] VT-d: correct / extend workaround(s) leaving
> an IOMMU disabled"):
>> Ian - I'm also Cc-ing you since this feels like being on the edge
>> between a new feature and a bug fix.
>
> Thanks.
>
> I think 2/ is a new quirk (or, new behaviour for an existing quirk).
> I think I am happy to treat that as a bugfix, assuming we are
> reasonably confident that most systems (including in particular all
> systems without the quirk) will take unchanged codepaths. Is that
> right ?
Yes. According to Linux there's exactly one BIOS flavor known to
exhibit the issue.
> I don't understand 1/. It looks bugfixish to me but I am really not
> qualified. I am inclined to defer to your judgement, but it would
> help me if you explicitly addressed the overall risks/benefits.
Right now our documentation claims similarity to a Linux workaround
without the similarity actually existing in the general case. A
common case (a single integrated graphics device) is handled, but the
perhaps yet more common case of a single add-in graphics devices is
not. Plus the criteria by which a device is determined to be a
graphics one was completely flawed. Hence people in need of the
workaround may find it non-functional. However, since our doc tells
people to report if they have a need to use the option engaging the
workaround, and since we didn't have any such reports in a number
of years, I guess both benefits and possible risks here are of
purely theoretical nature. Note that I've specifically said "possible"
because I can't really see any beyond me not having properly matched
Linux'es equivalent workaround - that workaround has been in place
unchanged for very many years.
> But when reading the patch I did notice one thing that struck me as
> undesriable:
>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -750,27 +750,43 @@ static void iommu_enable_translation(str
>> if ( force_iommu )
>> -panic("BIOS did not enable IGD for VT properly, crash Xen
>> for security purpose\n");
>> +panic(crash_fmt, msg);
> ...
>> +if ( force_iommu )
>> +panic(crash_fmt, msg);
>
> Does this really mean that every exit path from
> iommu_enable_translation that doesn't enable the iommu has to have a
> check for force_iommu ?
>
> That seems like a recipe for missing one. And I think a missed one
> would be an XSA. Could we not structure the code some way to avoid
> this foreseeable human error ?
I'm afraid I don't see a good way to do so, as imo it's desirable to
have separate log messages. IOW something like
if ( ... )
{
msg = "...";
goto dead;
}
doesn't look any better to me. Also leaving individual IOMMUs disabled
should really be the exception anyway.
Jan
Re: [PATCH 01/11] xen: reserve flags for internal usage in xen_domctl_createdomain
On 11.10.2021 12:45, Julien Grall wrote: > Hi Jan, > > On 28/09/2021 13:05, Jan Beulich wrote: >> On 23.09.2021 11:54, Julien Grall wrote: >>> On 23/09/2021 08:11, Penny Zheng wrote: From: Stefano Stabellini We are passing an extra special boolean flag at domain creation to specify whether we want to the domain to be privileged (i.e. dom0) or not. Another flag will be introduced later in this series. Reserve bits 16-31 from the existing flags bitfield in struct xen_domctl_createdomain for internal Xen usage. >>> >>> I am a bit split with this approach. This feels a bit of a hack to >>> reserve bits for internal purpose in external headers. But at the same >>> time I can see how this is easier to deal with it over repurposing the >>> last argument of domain_create(). >> >> I actually have trouble seeing why that's easier. It is a common thing >> to widen a bool to "unsigned int flags" when more than one control is >> needed. > > I was suggesting this is easier for the following two reasons: >1) All the option flags (internal and external) are in a single place. >2) Reduce the risk to make a mistake when widening the field. In > particular in the context of backporting. Although, this looks unlikely > here. > >> Plus this makes things needlessly harder once (in the future) >> the low 16 bits are exhausted in the public interface. > > That's why I suggested this sounds like a hack. At the same time the > split between external vs internal option is a bit more a pain for the > developper. So I didn't feel pushing for one vs the other. That said, I > will not argue against if you want to push for repurposing the last > argument. In which case - Penny, would you please change the patch accordingly? Jan
Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain
On 09/10/2021 10:40, Penny Zheng wrote: Hi Julien Hi Penny, -Original Message- From: Julien Grall Sent: Thursday, September 23, 2021 7:27 PM To: Penny Zheng ; xen-devel@lists.xenproject.org; sstabell...@kernel.org Cc: Bertrand Marquis ; Wei Chen Subject: Re: [PATCH 10/11] xen/arm: device assignment on 1:1 direct-map domain Hi, On 23/09/2021 08:11, Penny Zheng wrote: User could do device passthrough, with "xen,force-assign-without-iommu" in the device tree snippet, on trusted guest through 1:1 direct-map, if IOMMU absent or disabled on hardware. At the moment, it would be possible to passthrough a non-DMA capable device with direct-mapping. After this patch, this is going to be forbidden. In order to achieve that, this patch adds 1:1 direct-map check and disables iommu-related action. Signed-off-by: Penny Zheng --- xen/arch/arm/domain_build.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index c92e510ae7..9a9d2522b7 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -2070,14 +2070,18 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo, if ( res < 0 ) return res; +/* + * If xen_force, we allow assignment of devices without IOMMU protection. + * And if IOMMU is disabled or absent, 1:1 direct-map is necessary > + */ +if ( xen_force && is_domain_direct_mapped(kinfo->d) && + !dt_device_is_protected(node) ) dt_device_is_protected() will be always false unless the device is protected behing an SMMU using the legacy binding. So I don't think this is correct to move this check ahead. In fact.. +return 0; + res = iommu_add_dt_device(node); ... the call should already be a NOP when the IOMMU is disabled or the device is not behind an IOMMU. So can you explain what you are trying to prevent here? If the IOMMU is disabled, iommu_add_dt_device will return 1 as errno. So we could not make it to the xen_force check... I disagree. The check is: if ( res < 0 ) return res; Given that res is 1, we wouldn't return and move to check whether the assignment can be done. So I tried to move all IOMMU action behind xen_force check. Now, device assignment without IOMMU protection is only applicable on direct-map domains, It is fine to assign a non-DMA capable device without direct-mapping. So why do you want to add this restriction? Cheers, -- Julien Grall
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
> On 11 Oct 2021, at 10:39, Julien Grall wrote:
>
> Hi Luca,
>
Hi Julien,
> On 11/10/2021 09:03, Luca Fancellu wrote:
>> +static bool __init is_boot_module(int dt_module_offset)
>> +{
>> +if ( (fdt_node_check_compatible(fdt, dt_module_offset,
>> +"multiboot,kernel") == 0) ||
>> + (fdt_node_check_compatible(fdt, dt_module_offset,
>> +"multiboot,ramdisk") == 0) ||
>> + (fdt_node_check_compatible(fdt, dt_module_offset,
>> +"multiboot,device-tree") == 0) )
>> +return true;
>
> A boot module *must* have the compatible "multiboot,module". I would prefer
> if we simply check that "multiboot,module" is present.
>
> This will also make easier to add new boot module in the future.
I thought that also the XSM policy was a multiboot,module so I checked
explicitly for kernel, ramdisk, device-tree that are supported
by domU.
Do you still think that I should check just for multiboot,module instead?
Cheers,
Luca
>
>> +
>> +return false;
>> +}
>> +
> Cheers,
>
> --
> Julien Grall
Re: [PATCH V5 3/3] xen/arm: Updates for extended regions support
Hi Oleksandr,
On 07/10/2021 21:29, Oleksandr wrote:
@@ -1193,8 +1215,8 @@ static int __init make_hypervisor_node(struct
domain
*d,
u64 start = ext_regions->bank[i].start;
u64 size = ext_regions->bank[i].size;
- dt_dprintk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
- i, start, start + size);
+ printk("Extended region %d: %#"PRIx64"->%#"PRIx64"\n",
+ i, start, start + size);
Also should be PRIpaddr
I thought I needed to change specifier only for variables of type
"paddr_t",
but here "u64".
Sorry, you are right.
I added my reviewed-by and made the small typo changes on commit.
Thanks! In case if you haven't committed the patch yet, let's please
wait for Julien (who asked for this follow-up) to review it.
I went through it. The change looks good to me. No need for...
In any case, I will be able to do another follow-up if needed.
... another follow-up. Thanks for sending a patch to handle my requests! :)
Cheers,
--
Julien Grall
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Hi Stefano,
On 08.10.2021 23:46, Stefano Stabellini wrote:
> On Fri, 8 Oct 2021, Julien Grall wrote:
>> Hi Andrew,
>>
>> On Fri, 8 Oct 2021, 20:07 Andrew Cooper, wrote:
>> On 06/10/2021 18:40, Rahul Singh wrote:
>> > Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
>> > Reject the use of this new flag for x86 as VPCI is not supported for
>> > DOMU guests for x86.
>> >
>> > Signed-off-by: Rahul Singh
>> > Reviewed-by: Stefano Stabellini
>> > Acked-by: Christian Lindig
>>
>> I'm afraid this logic is broken.
>>
>> There's no matching feature to indicate to the toolstack whether
>> XEN_DOMCTL_CDF_vpci will be accepted or not. The usual way of doing
>> this is with a physinfo_cap field.
>>
>>
>> I am slightly puzzled by this. I am assuming you are referring to
>> XENVER_get_features which AFAICT is a stable interface. So why should we
>> use it to describe the presence of an unstable interface?
>>
>>
>> This flag needs using in Patch 10 to reject attempts to create a VM
>> with
>> devices when passthrough support is unavailable.
>>
>>
>> May I ask why we can't rely on the hypervisor to reject the flag when
>> suitable?
>>
>>
>> Ian, for the 4.16 release, this series either needs completing with the
>> additional flag implemented, or this patch needs reverting to avoid us
>> shipping a broken interface.
>>
>>
>> I fail to see how the interface would be broken... Would you mind to
>> describe a bit more what could go wrong with this interface?
>
>
> After chatting with Andrew on IRC, this is my understanding.
>
> Today if pci=[] is specified in the VM config file then
> XEN_DOMCTL_CDF_vpci is added. If Xen doesn't support it, Xen returns
> an error but libxl/xl won't be able to tell exactly why it fails. So xl
> will end up printing a generic VM creation failure. Andrew would like to
> see something like the following in libxl:
>
> if ( PCI_devices && !cap.vcpi )
> error("Sorry - PCI not supported")
>
> So that the user gets a clear informative error back rather than a
> generic VM creation failure. Also, this is a requirement for the stable
> hypercall interface.
>
>
> I think that's fine and we can implement this request easily by adding
> XEN_SYSCTL_PHYSCAP_vpci. Rahul or Bertrand, are you guys happy with
> doing that? Otherwise I could take it on.
>
>
> As a side note, given that PCI passthrough support is actually not yet
> complete on ARM, we could even just do the following in xl/libxl:
>
> if ( PCI_devices )
> error("Sorry - PCI not supported")
>
> or temporarily remove XEN_DOMCTL_CDF_vpci until PCI passthrough gets
> finalized.
>
As Rahul is on leave:
I'm ok to introduce XEN_SYSCTL_PHYSCAP_vpci. I did the same for vpmu so it's ok.
However the problem I have is about setting this cap.
On arm it is easy as we are not supporting vpci at the moment so the cap can be
set to false.
But how to deal with this cap in x86 code? I'm not familiar with x86 so I'm
asking for advice.
Cheers,
Michal
Re: [XEN PATCH v7 21/51] build: set ALL_OBJS to main Makefile; move prelink.o to main Makefile
On 24.08.2021 12:50, Anthony PERARD wrote:
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -271,8 +271,21 @@ CFLAGS += -flto
> LDFLAGS-$(CONFIG_CC_IS_CLANG) += -plugin LLVMgold.so
> endif
>
> +# Note that link order matters!
> +ALL_OBJS-y:= common/built_in.o
> +ALL_OBJS-y+= drivers/built_in.o
> +ALL_OBJS-y+= lib/built_in.o
> +ALL_OBJS-y+= xsm/built_in.o
> +ALL_OBJS-y+= arch/$(TARGET_ARCH)/built_in.o
> +ALL_OBJS-$(CONFIG_CRYPTO) += crypto/built_in.o
> +
> +ALL_LIBS-y:= lib/lib.a
> +
> include $(BASEDIR)/arch/$(TARGET_ARCH)/arch.mk
>
> +export ALL_OBJS := $(ALL_OBJS-y)
> +export ALL_LIBS := $(ALL_LIBS-y)
> +
> # define new variables to avoid the ones defined in Config.mk
> export XEN_CFLAGS := $(CFLAGS)
> export XEN_AFLAGS := $(AFLAGS)
> @@ -393,7 +406,7 @@ $(TARGET): FORCE
> $(MAKE) -f $(BASEDIR)/Rules.mk -C include
> $(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) include
> $(MAKE) -f $(BASEDIR)/Rules.mk
> arch/$(TARGET_ARCH)/include/asm/asm-offsets.h
> - $(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) $@
> + $(MAKE) -f $(BASEDIR)/Rules.mk $@
This merely results in what was previously invoked from here now getting
invoked from the very bottom of build.mk. I'm afraid I don't see why this
is a useful change to make.
> --- a/xen/build.mk
> +++ b/xen/build.mk
> @@ -56,3 +56,27 @@ arch/$(TARGET_ARCH)/include/asm/asm-offsets.h:
> asm-offsets.s
> sed -rne "/^[^#].*==>/{s:.*==>(.*)<==.*:\1:; s: [\$$#]: :; p;}"; \
> echo ""; \
> echo "#endif") <$< >$@
> +
> +# head.o is built by descending into arch/arm/$(TARGET_SUBARCH), depends on
> the
> +# part of $(ALL_OBJS) that will eventually recurse into $(TARGET_SUBARCH)/
> and
> +# build head.o
> +arch/arm/$(TARGET_SUBARCH)/head.o: arch/arm/built_in.o
> +arch/arm/$(TARGET_SUBARCH)/head.o: ;
This previously lived in an Arm-specific file. Moving this here in the
given, still Arm-specific form is imo a no-go when done alongside all
the other good changes you're making. Is there a reason this can't go
into xen/arch/arm/arch.mk?
Jan
Re: [PATCH v5 1/2] arm/efi: Use dom0less configuration when using EFI boot
Hi Luca,
On 11/10/2021 12:23, Luca Fancellu wrote:
On 11 Oct 2021, at 10:39, Julien Grall wrote:
Hi Luca,
Hi Julien,
On 11/10/2021 09:03, Luca Fancellu wrote:
+static bool __init is_boot_module(int dt_module_offset)
+{
+if ( (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,kernel") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,ramdisk") == 0) ||
+ (fdt_node_check_compatible(fdt, dt_module_offset,
+"multiboot,device-tree") == 0) )
+return true;
A boot module *must* have the compatible "multiboot,module". I would prefer if we simply
check that "multiboot,module" is present.
This will also make easier to add new boot module in the future.
I thought that also the XSM policy was a multiboot,module so I checked
explicitly for kernel, ramdisk, device-tree that are supported
by domU.
The XSM policy is indeed a multiboot module and should not be used by
the domU.
Do you still think that I should check just for multiboot,module instead?
Yes please. I think this is not the EFI stub job to check that the most
specific compatible is correct.
Cheers,
--
Julien Grall
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
On 11.10.2021 13:29, Michal Orzel wrote:
> On 08.10.2021 23:46, Stefano Stabellini wrote:
>> On Fri, 8 Oct 2021, Julien Grall wrote:
>>> On Fri, 8 Oct 2021, 20:07 Andrew Cooper, wrote:
>>> On 06/10/2021 18:40, Rahul Singh wrote:
>>> > Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
>>> > Reject the use of this new flag for x86 as VPCI is not supported for
>>> > DOMU guests for x86.
>>> >
>>> > Signed-off-by: Rahul Singh
>>> > Reviewed-by: Stefano Stabellini
>>> > Acked-by: Christian Lindig
>>>
>>> I'm afraid this logic is broken.
>>>
>>> There's no matching feature to indicate to the toolstack whether
>>> XEN_DOMCTL_CDF_vpci will be accepted or not. The usual way of doing
>>> this is with a physinfo_cap field.
>>>
>>>
>>> I am slightly puzzled by this. I am assuming you are referring to
>>> XENVER_get_features which AFAICT is a stable interface. So why should we
>>> use it to describe the presence of an unstable interface?
>>>
>>>
>>> This flag needs using in Patch 10 to reject attempts to create a VM
>>> with
>>> devices when passthrough support is unavailable.
>>>
>>>
>>> May I ask why we can't rely on the hypervisor to reject the flag when
>>> suitable?
>>>
>>>
>>> Ian, for the 4.16 release, this series either needs completing with
>>> the
>>> additional flag implemented, or this patch needs reverting to avoid us
>>> shipping a broken interface.
>>>
>>>
>>> I fail to see how the interface would be broken... Would you mind to
>>> describe a bit more what could go wrong with this interface?
>>
>>
>> After chatting with Andrew on IRC, this is my understanding.
>>
>> Today if pci=[] is specified in the VM config file then
>> XEN_DOMCTL_CDF_vpci is added. If Xen doesn't support it, Xen returns
>> an error but libxl/xl won't be able to tell exactly why it fails. So xl
>> will end up printing a generic VM creation failure. Andrew would like to
>> see something like the following in libxl:
>>
>> if ( PCI_devices && !cap.vcpi )
>> error("Sorry - PCI not supported")
>>
>> So that the user gets a clear informative error back rather than a
>> generic VM creation failure. Also, this is a requirement for the stable
>> hypercall interface.
>>
>>
>> I think that's fine and we can implement this request easily by adding
>> XEN_SYSCTL_PHYSCAP_vpci. Rahul or Bertrand, are you guys happy with
>> doing that? Otherwise I could take it on.
>>
>>
>> As a side note, given that PCI passthrough support is actually not yet
>> complete on ARM, we could even just do the following in xl/libxl:
>>
>> if ( PCI_devices )
>> error("Sorry - PCI not supported")
>>
>> or temporarily remove XEN_DOMCTL_CDF_vpci until PCI passthrough gets
>> finalized.
>>
> As Rahul is on leave:
> I'm ok to introduce XEN_SYSCTL_PHYSCAP_vpci. I did the same for vpmu so it's
> ok.
> However the problem I have is about setting this cap.
> On arm it is easy as we are not supporting vpci at the moment so the cap can
> be set to false.
> But how to deal with this cap in x86 code? I'm not familiar with x86 so I'm
> asking for advice.
As the sysctl is mainly from tool stacks to drive guests (DomU-s), I'd
set it to false for x86 as well. Roger - do you see any reason this
could be needed to express anything Dom0-related?
Jan
Re: [XEN PATCH v7 22/51] build: clean common temporary files from root makefile
On 24.08.2021 12:50, Anthony PERARD wrote:
> Signed-off-by: Anthony PERARD
Trying to synthesize a description:
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -382,6 +382,7 @@ _clean:
> $(MAKE) $(clean) test
> $(MAKE) $(kconfig) clean
> find . \( -name "*.o" -o -name ".*.d" -o -name ".*.d2" \
This was effectively redundant with ...
> + -o -name ".*.o.tmp" -o -name "*~" -o -name "core" \
> -o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec
> rm -f {} \;
> rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi
> $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
> rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
> diff --git a/xen/scripts/Makefile.clean b/xen/scripts/Makefile.clean
> index 027c200c0efc..b6df9e861e6e 100644
> --- a/xen/scripts/Makefile.clean
> +++ b/xen/scripts/Makefile.clean
> @@ -14,10 +14,8 @@ include Makefile
> subdir-all := $(subdir-y) $(subdir-n) $(subdir-) \
>$(patsubst %/,%, $(filter %/, $(obj-y) $(obj-n) $(obj-)))
>
> -DEPS_RM = $(DEPS) $(DEPS_INCLUDE)
... this and its use below.
> .PHONY: clean
> clean:: $(subdir-all)
> - rm -f *.o .*.o.tmp *~ core $(DEPS_RM)
With the command gone, I think the :: should also be converted (back) to
just : then. Then
Reviewed-by: Jan Beulich
Assuming the patch is independent of the earlier still uncommitted ones
(please confirm), I'd be happy to make the adjustment while committing
- as long as you agree, of course.
Jan
Re: [PATCH v5 09/11] xen/arm: Transitional change to build HAS_VPCI on ARM.
On Wed, Oct 06, 2021 at 06:40:35PM +0100, Rahul Singh wrote: > This patch will be reverted once we add support for VPCI MSI/MSIX > support on ARM. > > Signed-off-by: Rahul Singh > Acked-by: Stefano Stabellini > Reviewed-by: Bertrand Marquis Reviewed-by: Roger Pau Monné > --- > Change in v5: none > Change in v4: > - Added Acked-by: Stefano Stabellini > - Added Reviewed-by: Bertrand Marquis > Change in v3: none > Change in v2: Patch introduced in v2 > --- > --- > xen/drivers/vpci/Makefile | 3 ++- > xen/drivers/vpci/header.c | 2 ++ > xen/include/asm-arm/pci.h | 8 > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile > index 55d1bdfda0..1a1413b93e 100644 > --- a/xen/drivers/vpci/Makefile > +++ b/xen/drivers/vpci/Makefile > @@ -1 +1,2 @@ > -obj-y += vpci.o header.o msi.o msix.o > +obj-y += vpci.o header.o > +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c > index ba9a036202..f8cd55e7c0 100644 > --- a/xen/drivers/vpci/header.c > +++ b/xen/drivers/vpci/header.c > @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, > uint16_t cmd, > * FIXME: punching holes after the p2m has been set up might be racy for > * DomU usage, needs to be revisited. > */ > +#ifdef CONFIG_HAS_PCI_MSI > if ( map && !rom_only && vpci_make_msix_hole(pdev) ) > return; > +#endif FWIW, I would also be fine with providing a dummy inline function for vpci_make_msix_hole when !CONFIG_HAS_PCI_MSI, but I assume this is a temporary workaround until MSI is implemented for Arm. Thanks, Roger.
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > ARM architecture does not implement I/O ports. Ignore this call on ARM > to avoid the overhead of making a hypercall just for Xen to return > -ENOSYS. What is the cal trace of this function actually on Arm? AFAICT libxl will only call xc_domain_ioport_permission if there are IO ports explicitly defined in the guest configuration, or if any of the BARs of the PCI device is in the IO space, which is not possible on Arm. Thanks, Roger.
Re: [XEN PATCH v7 23/51] build,include: remove arch-*/*.h public header listing from headers*.chk
On 24.08.2021 12:50, Anthony PERARD wrote: > $(public-y) contains "public/arch-%" but when used by > $(PUBLIC_HEADERS) $(public-y) is filtered-out by the pattern > "public/arch-%". So $(public-y) content is never used. It has been this way from its very introduction, and iirc $(public-y) was meant to be an abstract construct to which other pieces could get added in principle. I'm having a slight preference to keeping things as they are, unless you tell me that this is getting in the way of anything. And to be clear there as well - if there are no other reasons than pure cleanup, and if somebody else approved of the removal, I wouldn't object. Jan > --- a/xen/include/Makefile > +++ b/xen/include/Makefile > @@ -39,9 +39,6 @@ cppflags-$(CONFIG_X86)+= -m32 > > endif > > -public-$(CONFIG_X86) := $(wildcard public/arch-x86/*.h public/arch-x86/*/*.h) > -public-$(CONFIG_ARM) := $(wildcard public/arch-arm/*.h public/arch-arm/*/*.h) > - > .PHONY: all > all: $(headers-y) > > @@ -81,7 +78,7 @@ ifeq ($(XEN_TARGET_ARCH),$(XEN_COMPILE_ARCH)) > > all: headers.chk headers99.chk headers++.chk > > -PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard > public/*.h public/*/*.h) $(public-y)) > +PUBLIC_HEADERS := $(filter-out public/arch-% public/dom0_ops.h, $(wildcard > public/*.h public/*/*.h)) > > PUBLIC_C99_HEADERS := public/io/9pfs.h public/io/pvcalls.h > PUBLIC_ANSI_HEADERS := $(filter-out public/%ctl.h public/xsm/% > public/%hvm/save.h $(PUBLIC_C99_HEADERS), $(PUBLIC_HEADERS)) >
Re: [PATCH v4 2/3] xen/arm: Check for PMU platform support
Hi Julien,
On 11.10.2021 12:17, Julien Grall wrote:
> Hi Michal,
>
> On 11/10/2021 10:00, Michal Orzel wrote:
>> ID_AA64DFR0_EL1/ID_DFR0_EL1 registers provide
>> information about PMU support. Replace structure
>> dbg64/dbg32 with a union and fill in all the
>> register fields according to document:
>> ARM Architecture Registers(DDI 0595, 2021-06).
>>
>> Add macros boot_dbg_feature64/boot_dbg_feature32
>> to check for a debug feature. Add macro
>> cpu_has_pmu to check for PMU support.
>>
>> Signed-off-by: Michal Orzel
>> Reviewed-by: Stefano Stabellini
>> Reviewed-by: Bertrand Marquis
>> ---
>> Changes since v3:
>> -none
>> Changes since v2:
>> -none
>> Changes since v1:
>> -new in v2
>> ---
>> xen/include/asm-arm/cpufeature.h | 49 ++--
>> 1 file changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/cpufeature.h
>> b/xen/include/asm-arm/cpufeature.h
>> index 5ca09b0bff..4fce23844d 100644
>> --- a/xen/include/asm-arm/cpufeature.h
>> +++ b/xen/include/asm-arm/cpufeature.h
>> @@ -4,6 +4,7 @@
>> #ifdef CONFIG_ARM_64
>> #define cpu_feature64(c, feat) ((c)->pfr64.feat)
>> #define boot_cpu_feature64(feat) (system_cpuinfo.pfr64.feat)
>> +#define boot_dbg_feature64(feat) (system_cpuinfo.dbg64.feat)
>> #define cpu_feature64_has_el0_32(c) (cpu_feature64(c, el0) == 2)
>> @@ -22,6 +23,7 @@
>> #define cpu_feature32(c, feat) ((c)->pfr32.feat)
>> #define boot_cpu_feature32(feat) (system_cpuinfo.pfr32.feat)
>> +#define boot_dbg_feature32(feat) (system_cpuinfo.dbg32.feat)
>> #define cpu_has_arm (boot_cpu_feature32(arm) == 1)
>> #define cpu_has_thumb (boot_cpu_feature32(thumb) >= 1)
>> @@ -32,8 +34,10 @@
>> #ifdef CONFIG_ARM_32
>> #define cpu_has_gentimer (boot_cpu_feature32(gentimer) == 1)
>> +#define cpu_has_pmu (boot_dbg_feature32(perfmon) >= 1)
>
> From my understanding, on Armv7, perfmon == 0 only means PMUv2 is not
> present. IOW, it doesn't say whether PMUv1 is supported.
>
> I think it is OK to treat as the PMU is not supported (ARMv8 treat it like
> that too), but I would like a comment in the code so it is clear this is a
> deliberate choice.
>
We are checking if any version of PMU is supported meaning perfmon is >=1.
On ARMv8 any value higher than 0 means that PMU is present. 0 means not
supported.
On ARMv7 the above is not really true. Any value higher than 0 and lower than
15 means the PMU is supported.
So I think I should do:
#define cpu_has_pmu ((boot_dbg_feature32(perfmon) >= 1) && \
(boot_dbg_feature32(perfmon) < 15))
Do you agree?
>> #else
>> #define cpu_has_gentimer (1)
>> +#define cpu_has_pmu (boot_dbg_feature64(pmu_ver) >= 1)
>> #endif
>> #define cpu_has_security (boot_cpu_feature32(security) > 0)
>> @@ -181,8 +185,28 @@ struct cpuinfo_arm {
>> };
>> } pfr64;
>> - struct {
>> + union {
>> register_t bits[2];
>> + struct {
>> + /* DFR0 */
>> + unsigned long debug_ver:4;
>> + unsigned long trace_ver:4;
>> + unsigned long pmu_ver:4;
>> + unsigned long brps:4;
>> + unsigned long __res0:4;
>> + unsigned long wrps:4;
>> + unsigned long __res1:4;
>> + unsigned long ctx_cmps:4;
>> + unsigned long pms_ver:4;
>> + unsigned long double_lock:4;
>> + unsigned long trace_filt:4;
>> + unsigned long __res2:4;
>> + unsigned long mtpmu:4;
>> + unsigned long __res3:12;
>> +
>> + /* DFR1 */
>> + unsigned long __res4:64;
>> + };
>> } dbg64;
>> struct {
>> @@ -321,8 +345,29 @@ struct cpuinfo_arm {
>> };
>> } pfr32;
>> - struct {
>> + union {
>> register_t bits[2];
>> + struct {
>> + /* DFR0 */
>> + unsigned long copdbg:4;
>> + unsigned long copsdbg:4;
>> + unsigned long mmapdbg:4;
>> + unsigned long coptrc:4;
>> + unsigned long mmaptrc:4;
>> + unsigned long mprofdbg:4;
>> + unsigned long perfmon:4;
>> + unsigned long tracefilt:4;
>> +#ifdef CONFIG_ARM_64
>> + unsigned long __res0:32;
>> +#endif
>> +
>> + /* DFR1 */
>> + unsigned long mtpmu:4;
>> + unsigned long __res1:28;
>> +#ifdef CONFIG_ARM_64
>> + unsigned long __res2:32;
>> +#endif
>> + };
>> } dbg32;
>> struct {
>>
>
> Cheers,
>
Cheers,
Michal
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
Hi Roger, On 11.10.2021 11:27, Roger Pau Monné wrote: > On Wed, Oct 06, 2021 at 06:40:33PM +0100, Rahul Singh wrote: >> Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN. >> Reject the use of this new flag for x86 as VPCI is not supported for >> DOMU guests for x86. > > I don't like this approach, XEN_DOMCTL_CDF_vpci should be set for x86 > PVH dom0, like we do for any other CDF flags when Xen builds dom0. > > Things like PVH vs PV get translated into CDF flags by create_dom0, > and processed normally by the sanitise_domain_config logic, vPCI > should be handled that way. > > Do you think you could see about fixing this? > > Thanks, Roger. > As Rahul is on leave: I agree with you. XEN_DOMCTL_CDF_vpci should be set in dom0_cfg.flags in create_dom0 for pvh. We can do such fix in a new patch together with adding physcap for vpci. Cheers, Michal
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
Hi Roger, > On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: > > On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >> ARM architecture does not implement I/O ports. Ignore this call on ARM >> to avoid the overhead of making a hypercall just for Xen to return >> -ENOSYS. > > What is the cal trace of this function actually on Arm? > > AFAICT libxl will only call xc_domain_ioport_permission if there are > IO ports explicitly defined in the guest configuration, or if any of > the BARs of the PCI device is in the IO space, which is not possible > on Arm. PCI devices BARs can be in the IO space as the PCI devices are not Arm specific. There is not ioports on arm so to be used those can be in some cases remapped and accessed as MMIOs or are not possible to use at all. But the IO space does appear when BARs are listed even on Arm. Regards Bertrand > > Thanks, Roger.
[ovmf test] 165462: all pass - PUSHED
flight 165462 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/165462/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 19ee56c4b33faa33078894a6c8495c81c660b8be baseline version: ovmf 769e63999ff5c786d5aac1fd6dbfa4748fbccbc7 Last test of basis 165433 2021-10-08 16:10:05 Z2 days Testing same since 165462 2021-10-11 06:10:05 Z0 days1 attempts People who touched revisions under test: Liu, Zhiguang Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 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/osstest/ovmf.git 769e63999f..19ee56c4b3 19ee56c4b33faa33078894a6c8495c81c660b8be -> xen-tested-master
Re: [PATCH v5 09/11] xen/arm: Transitional change to build HAS_VPCI on ARM.
Hi Roger, > On 11 Oct 2021, at 12:43, Roger Pau Monné wrote: > > On Wed, Oct 06, 2021 at 06:40:35PM +0100, Rahul Singh wrote: >> This patch will be reverted once we add support for VPCI MSI/MSIX >> support on ARM. >> >> Signed-off-by: Rahul Singh >> Acked-by: Stefano Stabellini >> Reviewed-by: Bertrand Marquis > > Reviewed-by: Roger Pau Monné Thanks > >> --- >> Change in v5: none >> Change in v4: >> - Added Acked-by: Stefano Stabellini >> - Added Reviewed-by: Bertrand Marquis >> Change in v3: none >> Change in v2: Patch introduced in v2 >> --- >> --- >> xen/drivers/vpci/Makefile | 3 ++- >> xen/drivers/vpci/header.c | 2 ++ >> xen/include/asm-arm/pci.h | 8 >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile >> index 55d1bdfda0..1a1413b93e 100644 >> --- a/xen/drivers/vpci/Makefile >> +++ b/xen/drivers/vpci/Makefile >> @@ -1 +1,2 @@ >> -obj-y += vpci.o header.o msi.o msix.o >> +obj-y += vpci.o header.o >> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o >> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c >> index ba9a036202..f8cd55e7c0 100644 >> --- a/xen/drivers/vpci/header.c >> +++ b/xen/drivers/vpci/header.c >> @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, >> uint16_t cmd, >> * FIXME: punching holes after the p2m has been set up might be racy for >> * DomU usage, needs to be revisited. >> */ >> +#ifdef CONFIG_HAS_PCI_MSI >> if ( map && !rom_only && vpci_make_msix_hole(pdev) ) >> return; >> +#endif > > FWIW, I would also be fine with providing a dummy inline function for > vpci_make_msix_hole when !CONFIG_HAS_PCI_MSI, but I assume this is a > temporary workaround until MSI is implemented for Arm. Yes this is temporary and MSI support will be added on Arm. Regards Bertrand > > Thanks, Roger.
Re: [PATCH V5 1/3] xen/arm: Introduce gpaddr_bits field to struct xen_arch_domainconfig
On 09.10.21 01:14, Stefano Stabellini wrote: Hi Stefano On Fri, 8 Oct 2021, Oleksandr wrote: On 08.10.21 15:36, Jan Beulich wrote: On 08.10.2021 12:25, Oleksandr wrote: Just a quick question. What do you think can XEN_DOMCTL_getdomaininfo be reused to retrieve gpaddr_bits? I don't see why not at the moment, but maybe there are some implications/concerns which are invisible to me. I see that arch_get_domain_info() is present, so the field will be common, and each arch will write a value it considers appropriate. This could be a good compromise to not add an extra domctl and to not alter domain_create. Technically I think it could be reused. What I'm less certain of is whether the specific piece of information is a good fit there. ok, thank you for your answer. I am also not 100% sure whether it is a *good* fit there, but I cannot say it is not fit at all for being there. I might mistake, but it is almost the same piece of information describing the whole domain as other existing fields in that structure. From a domctl point of view, it looks like XEN_DOMCTL_getdomaininfo could be a decent fit. Looking at the data structure, the arch specific member of struct xen_domctl_getdomaininfo is: struct xen_arch_domainconfig arch_config; which is actually the very same struct used in struct xen_domctl_createdomain for XEN_DOMCTL_createdomain, but somehow it doesn't get populated by neither the x86 nor the ARM version of arch_get_domain_info? In any case, I think we could make use of XEN_DOMCTL_getdomaininfo for this. In that case, I would add a new common field to struct xen_domctl_getdomaininfo after cpupool and above arch_config. Then we can set the field from arch_get_domain_info. Yes, this is what I had in mind, thank you. -- Regards, Oleksandr Tyshchenko
Re: [XEN PATCH v7 24/51] build: prepare to always invoke $(MAKE) from xen/, use $(obj)
On 24.08.2021 12:50, Anthony PERARD wrote: > In a future patch, when building a subdirectory, we will set > "obj=$subdir" rather than change directory. > > Before that, we add "$(obj)" and "$(src)" in as many places as > possible where we will need to know which subdirectory is been built. > "$(obj)" is for files been generated during the build, and "$(src)" is > for files present in the source tree. > > For now, we set both to "." in Rules.mk and Makefile.clean. > > A few places don't tolerate the addition of "./", this is because make > remove the leading "./" in targets and dependencies in rules, so these > will be change later. > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich Nevertheless a couple of remarks: > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile >[...] > @@ -192,25 +192,25 @@ note_file_option ?= $(note_file) > > ifeq ($(XEN_BUILD_PE),y) > extra-y += efi.lds What about this? Does this for some reason also fall into the "cannot be converted yet" group? > @@ -222,14 +222,14 @@ $(TARGET).efi: FORCE > endif > > # These should already have been rebuilt when building the prerequisite of > "prelink.o" > -efi/buildid.o efi/relocs-dummy.o: ; > +$(obj)/efi/buildid.o $(obj)/efi/relocs-dummy.o: ; > > .PHONY: include > include: $(BASEDIR)/arch/x86/include/asm/asm-macros.h > > -asm-macros.i: CFLAGS-y += -D__ASSEMBLY__ -P > +$(obj)/asm-macros.i: CFLAGS-y += -D__ASSEMBLY__ -P > > -$(BASEDIR)/arch/x86/include/asm/asm-macros.h: asm-macros.i Makefile > +$(BASEDIR)/arch/x86/include/asm/asm-macros.h: $(obj)/asm-macros.i > $(src)/Makefile Isn't this $(obj)/include/asm/asm-macros.h ? And in general doesn't use of $(BASEDIR) need to go away then, e.g. ... > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -1,8 +1,8 @@ > obj-bin-y += head.o > > -DEFS_H_DEPS = defs.h $(BASEDIR)/include/xen/stdbool.h > +DEFS_H_DEPS = $(src)/defs.h $(BASEDIR)/include/xen/stdbool.h ... here needing to become $(src)/../../../include/xen/stdbool.h ? > --- a/xen/scripts/Makefile.clean > +++ b/xen/scripts/Makefile.clean > @@ -3,11 +3,14 @@ > # Cleaning up > # == > > +obj := . > +src := $(obj) This repeats what is also getting added to Rules.mk. To prevent the two going out of sync, wouldn't they better live in a central place (e.g. scripts/defs.mk)? Jan
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
Hi Jan,
As Rahul is on leave, I will answer you and make the changes needed.
> On 7 Oct 2021, at 14:43, Jan Beulich wrote:
>
> On 06.10.2021 19:40, Rahul Singh wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include
>> +
>> +#include
>> +
>> +#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
>
> Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
> Also isn't this effectively part of the public interface (along with
> MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?
I will move that in the next version to xen/pci.h and rename it
MMCFG_REG_OFFSET.
Would that be ok ?
>
>> +/* Do some sanity checks. */
>> +static bool vpci_mmio_access_allowed(unsigned int reg, unsigned int len)
>> +{
>> +/* Check access size. */
>> +if ( len > 8 )
>> +return false;
>
> struct hsr_dabt's size field doesn't allow len to be above 8. I could
> see that you may want to sanity check things, but that's not helpful
> if done incompletely. Elsewhere you assume the value to be non-zero,
> and ...
>
>> +/* Check that access is size aligned. */
>> +if ( (reg & (len - 1)) )
>
> ... right here you assume the value to be a power of 2. While I'm not
> a maintainer, I'd still like to suggest consistency: Do all pertinent
> checks or none of them (relying on the caller).
I will remove the check for len > 8 as dabt.size cannot have a value
greater than 3.
But I will have to introduce a check for len > 4 on 32 bit systems (see after).
>
> Independent of this - is bare metal Arm enforcing this same
> alignment restriction (unconditionally)? Iirc on x86 we felt we'd
> better synthesize misaligned accesses.
Unaligned IO access could be synthesise also on arm to but I would
rather not make such a change now without testing it (and there is
also a question of it making sense).
So if it is ok with you I will keep that check and discuss it with Rahul
when he is back. I will add a comment in the code to make that clear.
>
>> +static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>> + register_t *r, void *p)
>> +{
>> +unsigned int reg;
>> +pci_sbdf_t sbdf;
>> +unsigned long data = ~0UL;
>
> What use is this initializer? The error path further down doesn't
> forward the value into *r, and subsequently the value gets fully
> overwritten.
Right I will remove it.
>
>> +unsigned int size = 1U << info->dabt.size;
>> +
>> +sbdf.sbdf = MMCFG_BDF(info->gpa);
>
> This implies segment to be zero. While probably fine for now, I
> wonder if this wouldn't warrant a comment.
I will add the following comment just before:
/* We ignore segment part and always handle segment 0 */
>
>> +reg = REGISTER_OFFSET(info->gpa);
>> +
>> +if ( !vpci_mmio_access_allowed(reg, size) )
>> +return 0;
>> +
>> +data = vpci_read(sbdf, reg, min(4u, size));
>> +if ( size == 8 )
>> +data |= (uint64_t)vpci_read(sbdf, reg + 4, 4) << 32;
>
> Throughout this series I haven't been able to spot where the HAS_VPCI
> Kconfig symbol would get selected. Hence I cannot tell whether all of
> this is Arm64-specific. Otherwise I wonder whether size 8 actually
> can occur on Arm32.
Dabt.size could be 3 even on ARM32 but we should not allow 64bit
access on mmio regions on arm32.
So I will surround this code with ifdef CONFIG_ARM_64 and add a test
for len > 4 to prevent this case on 32bit.
To be completely right we should disable this also for 32bit guests but
this change would be a bit more invasive.
>
>> +static int vpci_mmio_write(struct vcpu *v, mmio_info_t *info,
>> + register_t r, void *p)
>> +{
>> +unsigned int reg;
>> +pci_sbdf_t sbdf;
>> +unsigned long data = r;
>
> A little like in the read function - what use is this local variable?
> Can't you use r directly?
We can and I will remove the data variable.
>
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>> else
>> iommu_enable_device(pdev);
>
> Please note the context above; ...
>
>> +#ifdef CONFIG_ARM
>> +/*
>> + * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>> when
>> + * Dom0 inform XEN to add the PCI devices in XEN.
>> + */
>> +ret = vpci_add_handlers(pdev);
>> +if ( ret )
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
On 11.10.2021 14:41, Bertrand Marquis wrote:
>> On 7 Oct 2021, at 14:43, Jan Beulich wrote:
>> On 06.10.2021 19:40, Rahul Singh wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/arm/vpci.c
>>> @@ -0,0 +1,102 @@
>>> +/*
>>> + * xen/arch/arm/vpci.c
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +#include
>>> +
>>> +#include
>>> +
>>> +#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
>>
>> Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
>> Also isn't this effectively part of the public interface (along with
>> MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?
>
> I will move that in the next version to xen/pci.h and rename it
> MMCFG_REG_OFFSET.
> Would that be ok ?
That would be okay and make sense when put next to MMCFG_BDF(), but
it would not address my comment: That still wouldn't be the public
interface. Otoh you only mimic hardware behavior, so perhaps the
splitting of the address isn't as relevant to put there as would be
GUEST_VPCI_ECAM_{BASE,SIZE}.
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>> else
>>> iommu_enable_device(pdev);
>>
>> Please note the context above; ...
>>
>>> +#ifdef CONFIG_ARM
>>> +/*
>>> + * On ARM PCI devices discovery will be done by Dom0. Add vpci handler
>>> when
>>> + * Dom0 inform XEN to add the PCI devices in XEN.
>>> + */
>>> +ret = vpci_add_handlers(pdev);
>>> +if ( ret )
>>> +{
>>> +printk(XENLOG_ERR "setup of vPCI failed: %d\n", ret);
>>> +pci_cleanup_msi(pdev);
>>> +ret = iommu_remove_device(pdev);
>>> +if ( pdev->domain )
>>> +list_del(&pdev->domain_list);
>>> +free_pdev(pseg, pdev);
>>
>> ... you unconditionally undo the if() part of the earlier conditional;
>> is there any guarantee that the other path can't ever be taken, now
>> and forever? If it can't be taken now (which I think is the case as
>> long as Dom0 wouldn't report the same device twice), then at least some
>> precaution wants taking. Maybe moving your addition into that if()
>> could be an option.
>>
>> Furthermore I continue to wonder whether this ordering is indeed
>> preferable over doing software setup before hardware arrangements. This
>> would address the above issue as well as long as vpci_add_handlers() is
>> idempotent. And it would likely simplify error cleanup.
>
> I agree with you so I will move this code block before iommu part.
>
> But digging deeper into this, I would have 2 questions:
>
> - msi_cleanup was done there after a request from Stefano, but is not
> done in case or iommu error, is there an issue to solve here ?
Maybe, but I'm not sure. This very much depends on what a domain
could in principle do with a partly set-up device. Plus let's
not forget that we're talking of only Dom0 here (for now at least,
i.e. not considering the dom0less case).
But I'd also like to further defer to Stefano.
> Same could also go for the free_pdev ?
I think it's wrong to free_pdev() here. We want to internally keep
record of the device, even if the device ends up unusable. The only
place where free_pdev() ought to be called is imo pci_remove_device().
> - cleanup code was exactly the same as pci_remove_device code.
> Should the question about the path also be checked there ?
I'm sorry, but I'm afraid I don't see what "the path" refers to
here. You can't mean the conditional in pci_add_device() selecting
between iommu_add_device() and iommu_enable_device(), as "remove"
can only mean "remove", never "disable".
Jan
Re: [PATCH 0/2] VT-d: correct / extend workaround(s) leaving an IOMMU disabled [and 1 more messages]
Jan Beulich writes ("Re: [PATCH 0/2] VT-d: correct / extend workaround(s)
leaving an IOMMU disabled [and 1 more messages]"):
> On 11.10.2021 12:56, Ian Jackson wrote:
> > I think 2/ is a new quirk (or, new behaviour for an existing quirk).
> > I think I am happy to treat that as a bugfix, assuming we are
> > reasonably confident that most systems (including in particular all
> > systems without the quirk) will take unchanged codepaths. Is that
> > right ?
>
> Yes. According to Linux there's exactly one BIOS flavor known to
> exhibit the issue.
>
> > I don't understand 1/. It looks bugfixish to me but I am really not
> > qualified. I am inclined to defer to your judgement, but it would
> > help me if you explicitly addressed the overall risks/benefits.
>
> Right now our documentation claims similarity to a Linux workaround
> without the similarity actually existing in the general case. A
> common case (a single integrated graphics device) is handled, but the
> perhaps yet more common case of a single add-in graphics devices is
> not. Plus the criteria by which a device is determined to be a
> graphics one was completely flawed. Hence people in need of the
> workaround may find it non-functional. However, since our doc tells
> people to report if they have a need to use the option engaging the
> workaround, and since we didn't have any such reports in a number
> of years, I guess both benefits and possible risks here are of
> purely theoretical nature. Note that I've specifically said "possible"
> because I can't really see any beyond me not having properly matched
> Linux'es equivalent workaround - that workaround has been in place
> unchanged for very many years.
OK, great. Thanks for the explanation. For the record,
Release-Acked-by: Ian Jackson
> > But when reading the patch I did notice one thing that struck me as
> > undesriable:
...
> > That seems like a recipe for missing one. And I think a missed one
> > would be an XSA. Could we not structure the code some way to avoid
> > this foreseeable human error ?
>
> I'm afraid I don't see a good way to do so, as imo it's desirable to
> have separate log messages. IOW something like
>
> if ( ... )
> {
> msg = "...";
> goto dead;
> }
>
> doesn't look any better to me. Also leaving individual IOMMUs disabled
> should really be the exception anyway.
C does not make this kind of thing easy. I might be tempted to make
an inner function which returned a const char*, with NULL meaning "it
went OK". Oh for a proper sum type...
Ian.
Re: [PATCH v5 07/11] xen/domctl: Introduce XEN_DOMCTL_CDF_vpci flag
On Mon, Oct 11, 2021 at 01:35:18PM +0200, Jan Beulich wrote:
> On 11.10.2021 13:29, Michal Orzel wrote:
> > On 08.10.2021 23:46, Stefano Stabellini wrote:
> >> On Fri, 8 Oct 2021, Julien Grall wrote:
> >>> On Fri, 8 Oct 2021, 20:07 Andrew Cooper,
> >>> wrote:
> >>> On 06/10/2021 18:40, Rahul Singh wrote:
> >>> > Introduce XEN_DOMCTL_CDF_vpci flag to enable VPCI support in XEN.
> >>> > Reject the use of this new flag for x86 as VPCI is not supported
> >>> for
> >>> > DOMU guests for x86.
> >>> >
> >>> > Signed-off-by: Rahul Singh
> >>> > Reviewed-by: Stefano Stabellini
> >>> > Acked-by: Christian Lindig
> >>>
> >>> I'm afraid this logic is broken.
> >>>
> >>> There's no matching feature to indicate to the toolstack whether
> >>> XEN_DOMCTL_CDF_vpci will be accepted or not. The usual way of doing
> >>> this is with a physinfo_cap field.
> >>>
> >>>
> >>> I am slightly puzzled by this. I am assuming you are referring to
> >>> XENVER_get_features which AFAICT is a stable interface. So why should we
> >>> use it to describe the presence of an unstable interface?
> >>>
> >>>
> >>> This flag needs using in Patch 10 to reject attempts to create a VM
> >>> with
> >>> devices when passthrough support is unavailable.
> >>>
> >>>
> >>> May I ask why we can't rely on the hypervisor to reject the flag when
> >>> suitable?
> >>>
> >>>
> >>> Ian, for the 4.16 release, this series either needs completing with
> >>> the
> >>> additional flag implemented, or this patch needs reverting to avoid
> >>> us
> >>> shipping a broken interface.
> >>>
> >>>
> >>> I fail to see how the interface would be broken... Would you mind to
> >>> describe a bit more what could go wrong with this interface?
> >>
> >>
> >> After chatting with Andrew on IRC, this is my understanding.
> >>
> >> Today if pci=[] is specified in the VM config file then
> >> XEN_DOMCTL_CDF_vpci is added. If Xen doesn't support it, Xen returns
> >> an error but libxl/xl won't be able to tell exactly why it fails. So xl
> >> will end up printing a generic VM creation failure. Andrew would like to
> >> see something like the following in libxl:
> >>
> >> if ( PCI_devices && !cap.vcpi )
> >> error("Sorry - PCI not supported")
> >>
> >> So that the user gets a clear informative error back rather than a
> >> generic VM creation failure. Also, this is a requirement for the stable
> >> hypercall interface.
> >>
> >>
> >> I think that's fine and we can implement this request easily by adding
> >> XEN_SYSCTL_PHYSCAP_vpci. Rahul or Bertrand, are you guys happy with
> >> doing that? Otherwise I could take it on.
> >>
> >>
> >> As a side note, given that PCI passthrough support is actually not yet
> >> complete on ARM, we could even just do the following in xl/libxl:
> >>
> >> if ( PCI_devices )
> >> error("Sorry - PCI not supported")
> >>
> >> or temporarily remove XEN_DOMCTL_CDF_vpci until PCI passthrough gets
> >> finalized.
> >>
> > As Rahul is on leave:
> > I'm ok to introduce XEN_SYSCTL_PHYSCAP_vpci. I did the same for vpmu so
> > it's ok.
> > However the problem I have is about setting this cap.
> > On arm it is easy as we are not supporting vpci at the moment so the cap
> > can be set to false.
> > But how to deal with this cap in x86 code? I'm not familiar with x86 so I'm
> > asking for advice.
>
> As the sysctl is mainly from tool stacks to drive guests (DomU-s), I'd
> set it to false for x86 as well. Roger - do you see any reason this
> could be needed to express anything Dom0-related?
I agree, I don't think we should set it to true unless we also support
vPCI for domUs on x86, or else it's just going to confuse the
toolstack.
Thanks, Roger.
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
On Mon, Oct 11, 2021 at 12:11:04PM +, Bertrand Marquis wrote: > Hi Roger, > > > On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: > > > > On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > >> ARM architecture does not implement I/O ports. Ignore this call on ARM > >> to avoid the overhead of making a hypercall just for Xen to return > >> -ENOSYS. > > > > What is the cal trace of this function actually on Arm? > > > > AFAICT libxl will only call xc_domain_ioport_permission if there are > > IO ports explicitly defined in the guest configuration, or if any of > > the BARs of the PCI device is in the IO space, which is not possible > > on Arm. > > PCI devices BARs can be in the IO space as the PCI devices are not > Arm specific. There is not ioports on arm so to be used those can be > in some cases remapped and accessed as MMIOs or are not possible > to use at all. > > But the IO space does appear when BARs are listed even on Arm. Urg, I wonder whether those devices with IO BARs will work correctly under Arm then. How do you know whether the BAR has been remapped from IO space into MMIO? IMO instead of faking a successful return value from xc_domain_ioport_permission we should avoid the call completely in the first place, specially if we need to instead issue a call to xc_domain_iomem_permission. Thanks, Roger.
Re: [XEN PATCH v7 25/51] build: rework test/livepatch/Makefile
On 24.08.2021 12:50, Anthony PERARD wrote:
> This rework the livepatch/Makefile to make it less repetitive and make
> use of the facilities. All the targets to be built are now listed in
> $(extra-y) which will allow Rules.mk to build them without the need of
> a local target in a future patch.
>
> There are some changes/fixes in this patch:
> - when "xen-syms" is used for a target, it is added to the dependency
> list of the target, which allow to rebuild the target when xen-syms
> changes. But if "xen-syms" is missing, make simply fails.
> - modinfo.o wasn't removing it's $@.bin file like the other targets,
> this is now done.
> - The command to build *.livepatch targets as been fixed to use
> $(XEN_LDFLAGS) rather than just $(LDFLAGS) which is a fallout from
> 2740d96efdd3 ("xen/build: have the root Makefile generates the
> CFLAGS")
>
> make will findout the dependencies of the *.livepatch files and thus
> what to built by "looking" at the objects listed in the *-objs
> variables. The actual dependencies is generated by the new
> "multi_depend" macro.
>
> "$(targets)" needs to be updated with the objects listed in the
> different *-objs variables to allow make to load the .*.cmd dependency
> files.
>
> This patch copies the macro "multi_depend" from Linux 5.12.
>
> Signed-off-by: Anthony PERARD
Just two and a half remarks; I'd really like the livepatch maintainers
to properly review this change.
> --- a/xen/scripts/Kbuild.include
> +++ b/xen/scripts/Kbuild.include
> @@ -151,3 +151,12 @@ why =
> \
>
> echo-why = $(call escsq, $(strip $(why)))
Not the least seeing this in context, ...
> endif
> +
> +# Useful for describing the dependency of composite objects
> +# Usage:
> +# $(call multi_depend, multi_used_targets, suffix_to_remove, suffix_to_add)
> +define multi_depend
... I would wish we wouldn't introduce further names with underscores
in them when dashes are valid to be used.
> +$(foreach m, $(notdir $1), \
> + $(eval $(obj)/$m: \
> + $(addprefix $(obj)/, $(foreach s, $3, $($(m:%$(strip $2)=%$(s)))
I'd like to suggest to be consistent here: Either $(s) and then also
$(m) in both places, or $m and then also $s.
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
>[...]
> +$(obj)/%.livepatch: FORCE
> + $(call if_changed,livepatch)
> +
> +$(call multi_depend, $(filter %.livepatch,$(extra-y)), .livepatch, -objs)
> +targets += $(sort $(foreach m,$(basename $(notdir $(filter
> %.livepatch,$(extra-y, \
> +$($(m)-objs)))
I think it would help readability if the 2nd line was properly indented to
reflect the pending open parentheses:
targets += $(sort $(foreach m,$(basename $(notdir $(filter
%.livepatch,$(extra-y, \
$($(m)-objs)))
or (less desirable imo)
targets += $(sort \
$(foreach m,$(basename $(notdir $(filter
%.livepatch,$(extra-y, \
$($(m)-objs)))
Jan
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
Hi Jan,
> On 11 Oct 2021, at 14:09, Jan Beulich wrote:
>
> On 11.10.2021 14:41, Bertrand Marquis wrote:
>>> On 7 Oct 2021, at 14:43, Jan Beulich wrote:
>>> On 06.10.2021 19:40, Rahul Singh wrote:
--- /dev/null
+++ b/xen/arch/arm/vpci.c
@@ -0,0 +1,102 @@
+/*
+ * xen/arch/arm/vpci.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include
+
+#include
+
+#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
>>>
>>> Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
>>> Also isn't this effectively part of the public interface (along with
>>> MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?
>>
>> I will move that in the next version to xen/pci.h and rename
>> itMMCFG_REG_OFFSET.
>> Would that be ok ?
>
> That would be okay and make sense when put next to MMCFG_BDF(), but
> it would not address my comment: That still wouldn't be the public
> interface. Otoh you only mimic hardware behavior, so perhaps the
> splitting of the address isn't as relevant to put there as would be
> GUEST_VPCI_ECAM_{BASE,SIZE}.
Ok now I get what you wanted.
You would actually like both MMCFG_BDF and MMCFG_REG_OFFSET to
be moved to arch-arm.h.
Then I am not quite sure to follow why.
Those are not macros coming out of a way we have to define this but from
how it works in standard PCI.
The base and size are needed to know where the PCI bus will be.
So why should those be needed in public headers ?
>
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
else
iommu_enable_device(pdev);
>>>
>>> Please note the context above; ...
>>>
+#ifdef CONFIG_ARM
+/*
+ * On ARM PCI devices discovery will be done by Dom0. Add vpci
handler when
+ * Dom0 inform XEN to add the PCI devices in XEN.
+ */
+ret = vpci_add_handlers(pdev);
+if ( ret )
+{
+printk(XENLOG_ERR "setup of vPCI failed: %d\n", ret);
+pci_cleanup_msi(pdev);
+ret = iommu_remove_device(pdev);
+if ( pdev->domain )
+list_del(&pdev->domain_list);
+free_pdev(pseg, pdev);
>>>
>>> ... you unconditionally undo the if() part of the earlier conditional;
>>> is there any guarantee that the other path can't ever be taken, now
>>> and forever? If it can't be taken now (which I think is the case as
>>> long as Dom0 wouldn't report the same device twice), then at least some
>>> precaution wants taking. Maybe moving your addition into that if()
>>> could be an option.
>>>
>>> Furthermore I continue to wonder whether this ordering is indeed
>>> preferable over doing software setup before hardware arrangements. This
>>> would address the above issue as well as long as vpci_add_handlers() is
>>> idempotent. And it would likely simplify error cleanup.
>>
>> I agree with you so I will move this code block before iommu part.
>>
>> But digging deeper into this, I would have 2 questions:
>>
>> - msi_cleanup was done there after a request from Stefano, but is not
>> done in case or iommu error, is there an issue to solve here ?
>
> Maybe, but I'm not sure. This very much depends on what a domain
> could in principle do with a partly set-up device. Plus let's
> not forget that we're talking of only Dom0 here (for now at least,
> i.e. not considering the dom0less case).
>
> But I'd also like to further defer to Stefano.
Ok, I must admit I do not really see at that stage why doing an MSI cleanup
could be needed so I will wait for Stefano to know if I need to keep this when
moving the block up (at the end it is theoretical right now as this is empty).
>
>> Same could also go for the free_pdev ?
>
> I think it's wrong to free_pdev() here. We want to internally keep
> record of the device, even if the device ends up unusable. The only
> place where free_pdev() ought to be called is imo pci_remove_device().
Ok.
>
>> - cleanup code was exactly the same as pci_remove_device code.
>> Should the question about the path also be checked there ?
>
> I'm sorry, but I'm afraid I don't see what "the path" refers to
> here. You can't mean the conditional in pci_add_device() selecting
> between iommu_add_device() and iommu_enable_device(), as "remove"
> can only mean "remove", never "disable".
I will try to explain: when we just enable we d
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
Hi Roger, + Oleksandr to have a better PCI expert then me. > On 11 Oct 2021, at 14:20, Roger Pau Monné wrote: > > On Mon, Oct 11, 2021 at 12:11:04PM +, Bertrand Marquis wrote: >> Hi Roger, >> >>> On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: >>> >>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: ARM architecture does not implement I/O ports. Ignore this call on ARM to avoid the overhead of making a hypercall just for Xen to return -ENOSYS. >>> >>> What is the cal trace of this function actually on Arm? >>> >>> AFAICT libxl will only call xc_domain_ioport_permission if there are >>> IO ports explicitly defined in the guest configuration, or if any of >>> the BARs of the PCI device is in the IO space, which is not possible >>> on Arm. >> >> PCI devices BARs can be in the IO space as the PCI devices are not >> Arm specific. There is not ioports on arm so to be used those can be >> in some cases remapped and accessed as MMIOs or are not possible >> to use at all. >> >> But the IO space does appear when BARs are listed even on Arm. > > Urg, I wonder whether those devices with IO BARs will work correctly > under Arm then. > > How do you know whether the BAR has been remapped from IO space into > MMIO? We cannot, I think the platform will define if this is the case and where. @oleksandr: I remember that this was discussed during some of our meetings but I have no idea of the details here, can you help ? > > IMO instead of faking a successful return value from > xc_domain_ioport_permission we should avoid the call completely in the > first place, specially if we need to instead issue a call to > xc_domain_iomem_permission. At the end we will never have to issue this because this will never be a matter of “iomem” permission as there would not be any way to cut on something under the page. If this is to be supported one day, it will probably have to be fully emulated to keep the isolation. Right now on arm you can just make the more simple assumption that ioports are just not supported. Regards Bertrand > > Thanks, Roger.
Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl
On Thu, Oct 07, 2021 at 05:11:47PM +0100, Ian Jackson wrote:
> Rahul Singh writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree
> node in libxl"):
> > As Stefano suggested in another email that we can remove the vpci
> > option, if we reach to conclusion that we need vpci option I will
> > move it to internal structure.
> ...
> > Yes I agree with you VPCI is necessary for hot plugged PCI device
> > and once we implement the hotplug in future we will use the
> > passthrough= option to enable VPCI.
>
> So, to summarise, I think the situation is:
>
> * VCPI is necessry for passthrough on ARM, whether coldplug or
>hotplug. It's part of the way that PCI-PT works on ARM.
>
> * Hotplug is not yet implemented.
>
> * VPCI is not necessary on x86 (evidently, since we don't have it
>there but we do have passthrough).
>
> So when hotplug is added, vpci will need to be turned on when
> passthrough=yes is selected. I don't fully understand the other
> possible values for passthrough= but maybe we can defer the question
> of whether they apply to ARM ?
>
> I think that means that yes, this should be an internal variable.
> Probably in libxl__domain_create_state. We don't currently arrange to
> elide arch-specific state in there, so perhaps it's fine just to
> invent a member called `arm_vpci`.
Seeing as we might want to also use this on x86, I wonder whether we
should allow to specify a backend to use for each passthrough device,
ie:
pci = [ '36:00.0,backend=vpci',
'36:01.0,backend=qemu',
... ]
In principle we should support different backends on a per-device
basis, albeit if not currently possible.
Iff we have to introduce a new struct member for vPCI it should be
shared between all arches, as it's likely x86 will also want to at
least have the option to use vPCI for passthrough.
Thanks, Roger.
Re: [XEN PATCH v7 14/51] xen: move include/asm-* to arch/*/include/asm
On Thu, Oct 07, 2021 at 04:17:28PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > --- a/tools/include/Makefile > > +++ b/tools/include/Makefile > > @@ -30,7 +30,7 @@ xen-dir: > > ln -s $(XEN_ROOT)/xen/include/acpi/platform acpi/ > > ln -s $(XEN_ROOT)/xen/include/acpi/ac*.h acpi/ > > ifeq ($(CONFIG_X86),y) > > - ln -s $(XEN_ROOT)/xen/include/asm-x86 xen/asm > > + ln -s $(XEN_ROOT)/xen/arch/x86/include/asm xen/asm > > I think this would now better be > > ln -s $(XEN_ROOT)/xen/arch/x86/include/asm xen/ > > matching what is visible in context. I'll change it. > > --- a/xen/arch/riscv/arch.mk > > +++ b/xen/arch/riscv/arch.mk > > @@ -12,3 +12,4 @@ riscv-march-$(CONFIG_RISCV_ISA_C) := > > $(riscv-march-y)c > > > > CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany > > CFLAGS += -I$(BASEDIR)/include > > +CFLAGS += -I$(BASEDIR)/arch/$(TARGET_ARCH)/include > > I find it odd that this needed repeating in every arch.mk. Can't > this be done once for all arches? I actually do that much later in the series with patch "build: Add headers path to CFLAGS once for all archs". > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -241,7 +241,7 @@ PAGE_LIST_HEAD(page_broken_list); > > > > /* > > * first_valid_mfn is exported because it is use in ARM specific NUMA > > - * helpers. See comment in asm-arm/numa.h. > > + * helpers. See comment in asm/numa.h. > > */ > > mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; > > I'm afraid that in this case it is relevant that it's Arm's header, > like ... Will fix. Thanks, -- Anthony PERARD
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
On Mon, Oct 11, 2021 at 01:40:30PM +, Bertrand Marquis wrote: > Hi Roger, > > + Oleksandr to have a better PCI expert then me. > > > On 11 Oct 2021, at 14:20, Roger Pau Monné wrote: > > > > On Mon, Oct 11, 2021 at 12:11:04PM +, Bertrand Marquis wrote: > >> Hi Roger, > >> > >>> On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: > >>> > >>> On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > ARM architecture does not implement I/O ports. Ignore this call on ARM > to avoid the overhead of making a hypercall just for Xen to return > -ENOSYS. > >>> > >>> What is the cal trace of this function actually on Arm? > >>> > >>> AFAICT libxl will only call xc_domain_ioport_permission if there are > >>> IO ports explicitly defined in the guest configuration, or if any of > >>> the BARs of the PCI device is in the IO space, which is not possible > >>> on Arm. > >> > >> PCI devices BARs can be in the IO space as the PCI devices are not > >> Arm specific. There is not ioports on arm so to be used those can be > >> in some cases remapped and accessed as MMIOs or are not possible > >> to use at all. > >> > >> But the IO space does appear when BARs are listed even on Arm. > > > > Urg, I wonder whether those devices with IO BARs will work correctly > > under Arm then. > > > > How do you know whether the BAR has been remapped from IO space into > > MMIO? > > We cannot, I think the platform will define if this is the case and where. > @oleksandr: I remember that this was discussed during some of our > meetings but I have no idea of the details here, can you help ? > > > > > IMO instead of faking a successful return value from > > xc_domain_ioport_permission we should avoid the call completely in the > > first place, specially if we need to instead issue a call to > > xc_domain_iomem_permission. > > At the end we will never have to issue this because this will never be a > matter > of “iomem” permission as there would not be any way to cut on something under > the page. If this is to be supported one day, it will probably have to be > fully emulated > to keep the isolation. So you have a set of memory pages that map accesses from MMIO into IO space but it's not possible to isolate specific IO port regions as they are all contiguous in the same page(s). > Right now on arm you can just make the more simple assumption that ioports are > just not supported. Would it make sense in the future to provide a memory region to guests in order to use for IO port accesses, and call xc_domain_ioport_permission to set which ports would be allowed? I think the commit message needs to at least be expanded in order to contain the information provided here. It might also be helpful to figure out whether we would have to handle IO port accesses in the future on Arm, or if it's fine to just ignore them. Thanks, Roger.
Re: [XEN PATCH v7 26/51] build: build everything from the root dir, use obj=$subdir
On 24.08.2021 12:50, Anthony PERARD wrote: > A subdirectory is now built by setting "$(obj)" instead of changing > directory. "$(obj)" should always be set when using "Rules.mk" and > thus a shortcut "$(build)" is introduced and should be used. > > A new variable "$(need-builtin)" is introduce. It is to be used > whenever a "built_in.o" is wanted from a subdirectory. "built_in.o" > isn't the main target anymore, and thus only needs to depends on the > objects that should be part of "built_in.o". How "good" are our chances that we hit a need-builtin variable from the environment? Its uses are simply using "ifdef". > --- a/xen/Rules.mk > +++ b/xen/Rules.mk > @@ -3,19 +3,29 @@ > # Makefile and are consumed by Rules.mk > # > > -obj := . > src := $(obj) > > +PHONY := __build > +__build: > + > -include $(BASEDIR)/include/config/auto.conf > > include $(XEN_ROOT)/Config.mk > include $(BASEDIR)/scripts/Kbuild.include > > +ifndef obj > +$(warning kbuild: Rules.mk is included improperly) > +endif Is there a particular reason for this to come only here, rather than before the include-s (e.g. right at where the assignment to the variable lived)? > @@ -51,27 +61,54 @@ cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $< $@ > quiet_cmd_binfile = BINFILE $@ > cmd_binfile = $(SHELL) $(BASEDIR)/tools/binfile $(BINFILE_FLAGS) $@ $(2) > > -define gendep > -ifneq ($(1),$(subst /,:,$(1))) > -DEPS += $(dir $(1)).$(notdir $(1)).d > -endif > -endef > -$(foreach o,$(filter-out %/,$(obj-y) $(obj-bin-y) $(extra-y)),$(eval $(call > gendep,$(o > +# Figure out what we need to build from the various variables > +# === > + > +# Libraries are always collected in one lib file. > +# Filter out objects already built-in > +lib-y := $(filter-out $(obj-y), $(sort $(lib-y))) > + > +# Subdirectories we need to descend into > +subdir-y := $(subdir-y) $(patsubst %/,%,$(filter %/, $(obj-y))) Deliberately or accidentally not += ? > @@ -156,21 +192,13 @@ endif > PHONY += FORCE > FORCE: > > -%/built_in.o %/lib.a: FORCE > - $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in.o > - > -%/built_in_bin.o: FORCE > - $(MAKE) -f $(BASEDIR)/Rules.mk -C $* built_in_bin.o > - > -SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) > - > quiet_cmd_cc_o_c = CC $@ > ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) > cmd_cc_o_c = $(CC) $(c_flags) -c $< -o $(dot-target).tmp -MQ $@ > ifeq ($(CONFIG_CC_IS_CLANG),y) > -cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< > $(dot-target).tmp $@ > +cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $( $(dot-target).tmp $@ Are you sure about the $< => $( else > -cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $( $(dot-target).tmp $@ > +cmd_objcopy_fix_sym = $(OBJCOPY) --redefine-sym $( $(dot-target).tmp $@ ... here. > @@ -251,6 +292,9 @@ existing-targets := $(wildcard $(sort $(targets))) > > -include $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).cmd) > > +DEPS:= $(foreach f,$(existing-targets),$(dir $(f)).$(notdir $(f)).d) Nit: Preferably blanks on both sides of := or none at all, please. > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -81,6 +81,9 @@ endif > extra-y += asm-macros.i > extra-y += xen.lds > > +# Allows usercopy.c to includes itself Nit: include > +$(obj)/usercopy.o: CFLAGS-y += -I. This is ugly, but presumably unavoidable. Preferably I would see us the more specific -iquote though, assuming clang also supports it. > --- a/xen/arch/x86/boot/Makefile > +++ b/xen/arch/x86/boot/Makefile > @@ -1,8 +1,8 @@ > obj-bin-y += head.o > > -DEFS_H_DEPS = $(src)/defs.h $(BASEDIR)/include/xen/stdbool.h > +DEFS_H_DEPS = $(BASEDIR)/$(src)/defs.h $(BASEDIR)/include/xen/stdbool.h > > -CMDLINE_DEPS = $(DEFS_H_DEPS) $(src)/video.h > +CMDLINE_DEPS = $(DEFS_H_DEPS) $(BASEDIR)/$(src)/video.h Hmm, new uses of $(BASEDIR) (a few more further down). Why not $(srctree)? Jan
Re: [PATCH v2] x86/PV32: fix physdev_op_compat handling
On 11/10/2021 11:06, Roger Pau Monné wrote:
> On Mon, Oct 11, 2021 at 10:20:41AM +0200, Jan Beulich wrote:
>> The conversion of the original code failed to recognize that the 32-bit
>> compat variant of this (sorry, two different meanings of "compat" here)
>> needs to continue to invoke the compat handler, not the native one.
>> Arrange for this by adding yet another #define.
>>
>> Affected functions (having existed prior to the introduction of the new
>> hypercall) are PHYSDEVOP_set_iobitmap and PHYSDEVOP_apic_{read,write}.
>> For all others the operand struct layout doesn't differ.
>>
>> Fixes: 1252e2823117 ("x86/pv: Export pv_hypercall_table[] rather than
>> working around it in several ways")
>> Signed-off-by: Jan Beulich
> Reviewed-by: Roger Pau Monné
Reviewed-by: Andrew Cooper
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
On 11.10.2021 15:34, Bertrand Marquis wrote:
>> On 11 Oct 2021, at 14:09, Jan Beulich wrote:
>> On 11.10.2021 14:41, Bertrand Marquis wrote:
On 7 Oct 2021, at 14:43, Jan Beulich wrote:
On 06.10.2021 19:40, Rahul Singh wrote:
> --- /dev/null
> +++ b/xen/arch/arm/vpci.c
> @@ -0,0 +1,102 @@
> +/*
> + * xen/arch/arm/vpci.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#include
> +
> +#include
> +
> +#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
Also isn't this effectively part of the public interface (along with
MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?
>>>
>>> I will move that in the next version to xen/pci.h and rename
>>> itMMCFG_REG_OFFSET.
>>> Would that be ok ?
>>
>> That would be okay and make sense when put next to MMCFG_BDF(), but
>> it would not address my comment: That still wouldn't be the public
>> interface. Otoh you only mimic hardware behavior, so perhaps the
>> splitting of the address isn't as relevant to put there as would be
>> GUEST_VPCI_ECAM_{BASE,SIZE}.
>
> Ok now I get what you wanted.
>
> You would actually like both MMCFG_BDF and MMCFG_REG_OFFSET to
> be moved to arch-arm.h.
>
> Then I am not quite sure to follow why.
> Those are not macros coming out of a way we have to define this but from
> how it works in standard PCI.
> The base and size are needed to know where the PCI bus will be.
>
> So why should those be needed in public headers ?
Well, see my "Otoh ..." in the earlier reply. Keeping the two
address splitting macros out of there is okay.
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>else
>iommu_enable_device(pdev);
Please note the context above; ...
> +#ifdef CONFIG_ARM
> +/*
> + * On ARM PCI devices discovery will be done by Dom0. Add vpci
> handler when
> + * Dom0 inform XEN to add the PCI devices in XEN.
> + */
> +ret = vpci_add_handlers(pdev);
> +if ( ret )
> +{
> +printk(XENLOG_ERR "setup of vPCI failed: %d\n", ret);
> +pci_cleanup_msi(pdev);
> +ret = iommu_remove_device(pdev);
> +if ( pdev->domain )
> +list_del(&pdev->domain_list);
> +free_pdev(pseg, pdev);
... you unconditionally undo the if() part of the earlier conditional;
is there any guarantee that the other path can't ever be taken, now
and forever? If it can't be taken now (which I think is the case as
long as Dom0 wouldn't report the same device twice), then at least some
precaution wants taking. Maybe moving your addition into that if()
could be an option.
Furthermore I continue to wonder whether this ordering is indeed
preferable over doing software setup before hardware arrangements. This
would address the above issue as well as long as vpci_add_handlers() is
idempotent. And it would likely simplify error cleanup.
>>>
>>> I agree with you so I will move this code block before iommu part.
>>>
>>> But digging deeper into this, I would have 2 questions:
>>>
>>> - msi_cleanup was done there after a request from Stefano, but is not
>>> done in case or iommu error, is there an issue to solve here ?
>>
>> Maybe, but I'm not sure. This very much depends on what a domain
>> could in principle do with a partly set-up device. Plus let's
>> not forget that we're talking of only Dom0 here (for now at least,
>> i.e. not considering the dom0less case).
>>
>> But I'd also like to further defer to Stefano.
>
> Ok, I must admit I do not really see at that stage why doing an MSI cleanup
> could be needed so I will wait for Stefano to know if I need to keep this when
> moving the block up (at the end it is theoretical right now as this is empty).
>
>>
>>> Same could also go for the free_pdev ?
>>
>> I think it's wrong to free_pdev() here. We want to internally keep
>> record of the device, even if the device ends up unusable. The only
>> place where free_pdev() ought to be called is imo pci_remove_device().
>
> Ok.
>
>>
>>> - cleanup code was exactly the same as pci_remove_device code.
>>> Should the question about the path also be checked there ?
>>
>> I'm sorry, but I'm afraid I d
Re: [PATCH v4 3/3] xen: Expose the PMU to the guests
Hi Julien,
> On 11 Oct 2021, at 11:21, Julien Grall wrote:
>
> Hi Michal,
>
> On 11/10/2021 10:00, Michal Orzel wrote:
>> Add parameter vpmu to xl domain configuration syntax
>> to enable the access to PMU registers by disabling
>> the PMU traps(currently only for ARM).
>> The current status is that the PMU registers are not
>> virtualized and the physical registers are directly
>> accessible when this parameter is enabled. There is no
>> interrupt support and Xen will not save/restore the
>> register values on context switches.
>> Please note that this feature is experimental.
>> Signed-off-by: Michal Orzel
>> Signed-off-by: Julien Grall
>> Reviewed-by: Bertrand Marquis
>> ---
>> Changes since v3:
>> -fail if vpmu is set but not supported
>> -rebase on top of latest staging
>> Changes since v2:
>> -remove redundant check from x86 code
>> -do not define bit position and mask separately
>> Changes since v1:
>> -modify vpmu parameter to be common rather than arch specific
>> ---
>> docs/man/xl.cfg.5.pod.in | 17 +
>> tools/golang/xenlight/helpers.gen.go | 6 ++
>> tools/golang/xenlight/types.gen.go | 1 +
>> tools/include/libxl.h| 6 ++
>> tools/libs/light/libxl_create.c | 10 ++
>> tools/libs/light/libxl_types.idl | 2 ++
>> tools/ocaml/libs/xc/xenctrl.ml | 1 +
>> tools/ocaml/libs/xc/xenctrl.mli | 1 +
>> tools/xl/xl_parse.c | 2 ++
>> xen/arch/arm/domain.c| 12 +---
>> xen/arch/arm/setup.c | 1 +
>> xen/common/domain.c | 10 +-
>> xen/include/asm-arm/domain.h | 1 +
>> xen/include/public/domctl.h | 4 +++-
>> 14 files changed, 69 insertions(+), 5 deletions(-)
>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>> index 4b1e3028d2..55c4881205 100644
>> --- a/docs/man/xl.cfg.5.pod.in
>> +++ b/docs/man/xl.cfg.5.pod.in
>> @@ -690,6 +690,23 @@ default.
>> B: Acceptable values are platform specific. For Intel Processor
>> Trace, this value must be a power of 2 between 4k and 16M.
>> +=item B
>> +
>> +Currently ARM only.
>> +
>> +Specifies whether to enable the access to PMU registers by disabling
>> +the PMU traps.
>> +
>> +The PMU registers are not virtualized and the physical registers are
>> directly
>> +accessible when this parameter is enabled. There is no interrupt support and
>> +Xen will not save/restore the register values on context switches.
>> +
>> +vPMU, by design and purpose, exposes system level performance
>> +information to the guest. Only to be used by sufficiently privileged
>> +domains. This feature is currently in experimental state.
>
> Please update SUPPORT.MD to mention the support of this feature.
>
>> +
>> +If this option is not specified then it will default to B.
>> +
>> =back
>>=head2 Devices
>> diff --git a/tools/golang/xenlight/helpers.gen.go
>> b/tools/golang/xenlight/helpers.gen.go
>> index c8669837d8..2449580bad 100644
>> --- a/tools/golang/xenlight/helpers.gen.go
>> +++ b/tools/golang/xenlight/helpers.gen.go
>> @@ -1119,6 +1119,9 @@ return fmt.Errorf("converting field
>> ArchX86.MsrRelaxed: %v", err)
>> }
>> x.Altp2M = Altp2MMode(xc.altp2m)
>> x.VmtraceBufKb = int(xc.vmtrace_buf_kb)
>> +if err := x.Vpmu.fromC(&xc.vpmu);err != nil {
>> +return fmt.Errorf("converting field Vpmu: %v", err)
>> +}
>> return nil}
>> @@ -1600,6 +1603,9 @@ return fmt.Errorf("converting field
>> ArchX86.MsrRelaxed: %v", err)
>> }
>> xc.altp2m = C.libxl_altp2m_mode(x.Altp2M)
>> xc.vmtrace_buf_kb = C.int(x.VmtraceBufKb)
>> +if err := x.Vpmu.toC(&xc.vpmu); err != nil {
>> +return fmt.Errorf("converting field Vpmu: %v", err)
>> +}
>> return nil
>> }
>> diff --git a/tools/golang/xenlight/types.gen.go
>> b/tools/golang/xenlight/types.gen.go
>> index 45f2cba3d2..b2e8bd1a85 100644
>> --- a/tools/golang/xenlight/types.gen.go
>> +++ b/tools/golang/xenlight/types.gen.go
>> @@ -521,6 +521,7 @@ MsrRelaxed Defbool
>> }
>> Altp2M Altp2MMode
>> VmtraceBufKb int
>> +Vpmu Defbool
>> }
>>type DomainBuildInfoTypeUnion interface {
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index ec5e3badae..ee73eb06f1 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -508,6 +508,12 @@
>> */
>> #define LIBXL_HAVE_PHYSINFO_CAP_VPMU 1
>> +/*
>> + * LIBXL_HAVE_VPMU indicates that libxl_domain_build_info has a vpmu
>> parameter,
>> + * which allows to enable the access to PMU registers.
>> + */
>> +#define LIBXL_HAVE_VPMU 1
>> +
>> /*
>> * libxl ABI compatibility
>> *
>> diff --git a/tools/libs/light/libxl_create.c
>> b/tools/libs/light/libxl_create.c
>> index e356b2106d..2a0234ec16 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -91,6 +91,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>> }
>>libxl_defbool_setdefault(&b_info->device_model_stubdomain, false);
>> +
Re: [XEN PATCH v7 16/51] build: generate "include/xen/compile.h" with if_changed
On Thu, Oct 07, 2021 at 04:55:01PM +0200, Jan Beulich wrote:
> On 24.08.2021 12:50, Anthony PERARD wrote:
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -332,7 +332,6 @@ xen/include/compat/*
> > xen/include/config/
> > xen/include/generated/
> > xen/include/public/public
> > -xen/include/xen/*.new
>
> While this indeed looks to only have been here for compile.h, I'm
> not convinced it is a good idea to delete the entry here. Does it
> cause any harm if left in place?
That's a complicated question. I would prefer to have in this file only
artefacts of the build system but other developer and maintainer
disagree. So it's fine I guess to leave the entry, it just hide any
*.new file from `git status` and make it harder to commit them.
> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -368,7 +368,7 @@ _clean: delete-unfresh-files
> > -o -name "*.gcno" -o -name ".*.cmd" -o -name "lib.a" \) -exec
> > rm -f {} \;
> > rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi
> > $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
> > rm -f asm-offsets.s arch/*/include/asm/asm-offsets.h
> > - rm -f .banner
> > + rm -f .banner include/xen/compile.h
>
> Isn't this redundant with ...
>
> > @@ -425,10 +419,16 @@ include/xen/compile.h: include/xen/compile.h.in
> > .banner
> > +
> > +include/xen/compile.h: include/xen/compile.h.in .banner FORCE
> > @cat .banner
> > - @sed -rf tools/process-banner.sed < .banner >> $@.new
> > - @mv -f $@.new $@
> > + $(call if_changed,compile.h)
> > +targets += include/xen/compile.h
>
> ... this? I would have hoped that $(targets) is included in the
> generic cleaning logic ...
Not yet. It's probably a good idea, I'll work on a patch.
Thanks,
--
Anthony PERARD
Re: [XEN PATCH v7 05/51] x86/mm: avoid building multiple .o from a single .c file
On 08/09/2021 12:44, Ian Jackson wrote:
> Anthony PERARD writes ("Re: [XEN PATCH v7 05/51] x86/mm: avoid building
> multiple .o from a single .c file"):
>> On Tue, Sep 07, 2021 at 08:14:14AM +0200, Jan Beulich wrote:
>>> Hmm, when replying to 00/51 I didn't recall I gave an R-b for this one
>>> already. I'd like to restrict it some: It should be taken to stand for
>>> the technical correctness of the change. Nevertheless I'm not really
>>> happy with the introduction of the various tiny source files. I've
>>> meanwhile been wondering: Can't these be generated (in the build tree,
>>> as soon as that's possible to be separate) rather than getting put in
>>> the repo?
>> Do we really need to generated those never to be change tiny source
>> file? Do we really need to make the build system a lot more complicated?
> I'm not an x86 maintainer, but my 2p anyway:
>
> I think the handful of tiny source files is probably better than some
> contraption in the build system. Much less risk of something funny
> and confusing going on.
I agree. This patch is definitely an improvement on the status quo.
> We could reduce the number of copies of the same text by making the
> copies of guest_walk*.c in hap/ be symlinks to ../guest_walk*.c.
The two guest_walk's are totally different logic. Adding a symlink
would be reintroducing "something funny and confusing".
>
>> Can't we commit this patch as is? What kind of issue is there with those
>> tiny source files? Should we add a warning in those tiny source files,
>> something like "No modification of this file allowed, it's part of the
>> build system." ?
> I don't think we need any such warning. No-one is going to take that
> tiny file and try to edit it to put functionality in it, and if they
> do it will be spotted on review.
Agreed.
FTR, this patch is Reviewed-by: Andrew Cooper
and fit to be committed.
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
Hi, all On 11.10.21 16:40, Bertrand Marquis wrote: > Hi Roger, > > + Oleksandr to have a better PCI expert then me. > >> On 11 Oct 2021, at 14:20, Roger Pau Monné wrote: >> >> On Mon, Oct 11, 2021 at 12:11:04PM +, Bertrand Marquis wrote: >>> Hi Roger, >>> On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: > ARM architecture does not implement I/O ports. Ignore this call on ARM > to avoid the overhead of making a hypercall just for Xen to return > -ENOSYS. What is the cal trace of this function actually on Arm? AFAICT libxl will only call xc_domain_ioport_permission if there are IO ports explicitly defined in the guest configuration, or if any of the BARs of the PCI device is in the IO space, which is not possible on Arm. >>> PCI devices BARs can be in the IO space as the PCI devices are not >>> Arm specific. There is not ioports on arm so to be used those can be >>> in some cases remapped and accessed as MMIOs or are not possible >>> to use at all. >>> >>> But the IO space does appear when BARs are listed even on Arm. >> Urg, I wonder whether those devices with IO BARs will work correctly >> under Arm then. >> >> How do you know whether the BAR has been remapped from IO space into >> MMIO? > We cannot, I think the platform will define if this is the case and where. > @oleksandr: I remember that this was discussed during some of our > meetings but I have no idea of the details here, can you help ? > For the guest domains we emulate a host bridge without IO, so the guest won't be able to assign any IO memory at all.
Re: [PATCH v5 01/11] xen/arm: xc_domain_ioport_permission(..) not supported on ARM.
Hi Roger, > On 11 Oct 2021, at 14:57, Roger Pau Monné wrote: > > On Mon, Oct 11, 2021 at 01:40:30PM +, Bertrand Marquis wrote: >> Hi Roger, >> >> + Oleksandr to have a better PCI expert then me. >> >>> On 11 Oct 2021, at 14:20, Roger Pau Monné wrote: >>> >>> On Mon, Oct 11, 2021 at 12:11:04PM +, Bertrand Marquis wrote: Hi Roger, > On 11 Oct 2021, at 12:47, Roger Pau Monné wrote: > > On Wed, Oct 06, 2021 at 06:40:27PM +0100, Rahul Singh wrote: >> ARM architecture does not implement I/O ports. Ignore this call on ARM >> to avoid the overhead of making a hypercall just for Xen to return >> -ENOSYS. > > What is the cal trace of this function actually on Arm? > > AFAICT libxl will only call xc_domain_ioport_permission if there are > IO ports explicitly defined in the guest configuration, or if any of > the BARs of the PCI device is in the IO space, which is not possible > on Arm. PCI devices BARs can be in the IO space as the PCI devices are not Arm specific. There is not ioports on arm so to be used those can be in some cases remapped and accessed as MMIOs or are not possible to use at all. But the IO space does appear when BARs are listed even on Arm. >>> >>> Urg, I wonder whether those devices with IO BARs will work correctly >>> under Arm then. >>> >>> How do you know whether the BAR has been remapped from IO space into >>> MMIO? >> >> We cannot, I think the platform will define if this is the case and where. >> @oleksandr: I remember that this was discussed during some of our >> meetings but I have no idea of the details here, can you help ? >> >>> >>> IMO instead of faking a successful return value from >>> xc_domain_ioport_permission we should avoid the call completely in the >>> first place, specially if we need to instead issue a call to >>> xc_domain_iomem_permission. >> >> At the end we will never have to issue this because this will never be a >> matter >> of “iomem” permission as there would not be any way to cut on something under >> the page. If this is to be supported one day, it will probably have to be >> fully emulated >> to keep the isolation. > > So you have a set of memory pages that map accesses from > MMIO into IO space but it's not possible to isolate specific IO port > regions as they are all contiguous in the same page(s). Exact. > >> Right now on arm you can just make the more simple assumption that ioports >> are >> just not supported. > > Would it make sense in the future to provide a memory region to guests > in order to use for IO port accesses, and call > xc_domain_ioport_permission to set which ports would be allowed? Right now we do not plan to support this at all and we will have to figure this out if we do this one day. > > I think the commit message needs to at least be expanded in order to > contain the information provided here. It might also be helpful to > figure out whether we would have to handle IO port accesses in the > future on Arm, or if it's fine to just ignore them. All our investigations and tests have been done without supporting it without any issues so this is not a critical feature (most devices can be operated without using the I/O ports). I can add the following to the commit message: I/O ports accessible through MMIO are currently not supported by Xen. Regards Bertrand > > Thanks, Roger.
Re: [XEN PATCH v7 27/51] build: introduce if_changed_deps
On 24.08.2021 12:50, Anthony PERARD wrote: > This macro does compare command line like if_changed, but it also > rewrite the dependencies generated by $(CC) in order to depend on a > CONFIG_* as generated by kconfig instead of depending on autoconf.h. > This allow to make a change in kconfig options and only rebuild the > object that uses that CONFIG_* option. > > cmd_and_record isn't needed anymore as it is replace by > cmd_and_fixdep. > > There's only one .*.d dependency file left which is explicitly > included as a workound, all the other are been absorb into the .*.cmd > dependency files via `fixdep`. So including .*.d can be removed from > the makefile. > > This imports fixdep.c and if_changed_deps macro from Linux v5.12. > > Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich with a question: > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -187,6 +187,13 @@ endif > export root-make-done := y > endif # root-make-done > > +# === > +# Rules shared between *config targets and build targets > + > +PHONY += tools_fixdep > +tools_fixdep: > + $(MAKE) -C tools fixdep > + > # Shorthand for kconfig > kconfig = -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) > SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" > > @@ -400,7 +407,7 @@ $(TARGET).gz: $(TARGET) > gzip -n -f -9 < $< > $@.new > mv $@.new $@ > > -$(TARGET): FORCE > +$(TARGET): tools_fixdep FORCE > $(MAKE) -C tools Shouldn't this include building fixdep, in which case the extra dependency here is unnecessary? I can see that it's needed ... > @@ -457,13 +464,13 @@ cscope: > _MAP: > $(NM) -n $(TARGET)-syms | grep -v '\(compiled\)\|\(\.o$$\)\|\( [aUw] > \)\|\(\.\.ng$$\)\|\(LASH[RL]DI\)' > System.map > > -%.o %.i %.s: %.c FORCE > +%.o %.i %.s: %.c tools_fixdep FORCE > $(MAKE) $(build)=$(*D) $(*D)/$(@F) ... in cases like this one. Jan
[xen-unstable-smoke test] 165465: tolerable all pass - PUSHED
flight 165465 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/165465/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 2fac4e3ea3bb301f87426e24b9dfd765c22c9cf9 baseline version: xen 664cc3c3d381e4f9a61dcb68cbd7a6a00070370e Last test of basis 165439 2021-10-09 03:01:40 Z2 days Testing same since 165465 2021-10-11 10:02:56 Z0 days1 attempts People who touched revisions under test: Christopher Clark Jan Beulich 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 664cc3c3d3..2fac4e3ea3 2fac4e3ea3bb301f87426e24b9dfd765c22c9cf9 -> smoke
Re: [XEN PATCH v7 17/51] build: set XEN_BUILD_EFI earlier
On Thu, Oct 07, 2021 at 06:14:33PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > $(nr-fixups) is renamed to $(efi-check-relocs) as the former might be > > a bit too generic. > > While I don't mind the prefix addition, may I please ask that the rest > of the name remain as is, i.e. $(efi-nr-fixups)? "nr" because that's > what the variable holds, and "fixups" to distinguish from full-fledged > relocations as well as to match commentary there. Will change. > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -123,41 +123,7 @@ $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > > mv $(TMP) $(TARGET) > > > > ifneq ($(efi-y),) > > - > > -# Check if the compiler supports the MS ABI. > > -export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o > > efi/check.o 2>/dev/null && echo y) > > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > > - > > -# Check if the linker supports PE. > > -EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(XEN_LDFLAGS)) --subsystem=10 > > -XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(call ld-option,$(EFI_LDFLAGS) > > --image-base=0x1 -o efi/check.efi efi/check.o)) > > -# If the above failed, it may be merely because of the linker not dealing > > well > > -# with debug info. Try again with stripping it. > > -ifeq ($(CONFIG_DEBUG_INFO)-$(XEN_BUILD_PE),y-n) > > -EFI_LDFLAGS += --strip-debug > > -XEN_BUILD_PE := $(call ld-option,$(EFI_LDFLAGS) --image-base=0x1 > > -o efi/check.efi efi/check.o) > > -endif > > - > > -ifeq ($(XEN_BUILD_PE),y) > > - > > -# Check if the linker produces fixups in PE by default > > -nr-fixups := $(shell $(OBJDUMP) -p efi/check.efi | grep > > '^[[:blank:]]*reloc[[:blank:]]*[0-9][[:blank:]].*DIR64$$' | wc -l) > > -ifeq ($(nr-fixups),2) > > -MKRELOC := : > > -relocs-dummy := > > -else > > -MKRELOC := efi/mkreloc > > -relocs-dummy := efi/relocs-dummy.o > > -# If the linker produced fixups but not precisely two of them, we need to > > -# disable it doing so. But if it didn't produce any fixups, it also > > wouldn't > > -# recognize the option. > > -ifneq ($(nr-fixups),0) > > -EFI_LDFLAGS += --disable-reloc-section > > -endif > > -endif > > - > > -endif # $(XEN_BUILD_PE) > > - > > endif # $(efi-y) > > Is the remaining if(,) block still warranted? I.e. can't the single line > > CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI > > live without the surrounding conditional? Let's see, $(efi-y) depends on CONFIG_PV_SHIM_EXCLUSIVE and `sudo make install`, but XEN_BUILD_EFI also depends on CONFIG_PV_SHIM_EXCLUSIVE and we don't want to build in `sudo make install` so CFLAGS shouldn't be used. So the single line without the if() block seems enough. I remove the surrounding conditional. Thanks, -- Anthony PERARD
Re: [XEN PATCH v7 18/51] build: fix $(TARGET).efi creation in arch/arm
On Mon, Oct 11, 2021 at 12:37:55PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > There is no need to try to guess a relative path to the "xen.efi" file, > > we can simply use $@. Also, there's no need to use `notdir`, make > > already do that work via $(@F). > > > > Signed-off-by: Anthony PERARD > > Reviewed-by: Jan Beulich > > As to the subject, I don't think "fix" is appropriate. How about "adjust" > or "simplify" or some such? "adjust" sound good, thanks. -- Anthony PERARD
Re: [XEN PATCH v7 28/51] build: rename __LINKER__ to LINKER_SCRIPT
On 24.08.2021 12:50, Anthony PERARD wrote: > For two reasons: this macro is used to generate a "linker script" and > is not by the linker, and name starting with an underscore '_' are > supposed to be reserved, so better avoid them when not needed. > > Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich
Re: [XEN PATCH v7 29/51] build: add an other explicite rules to not build $(XEN_ROOT)/.config
On 24.08.2021 12:50, Anthony PERARD wrote: > GNU Make will try to rebuild every Makefile included with the > "include" directive, so everytime Config.mk is used, make will try to > build ".config". This would normally not be an issue, unless we happen > to have a rules which match. This is the case with Kconfig in xen/. > > While we had a workaround in "xen/Makefile", this ".config" files > becomes an issue again in "xen/tools/kconfig/Makefile". It has a > target "%.config". "we had" sounds like we don't have such anymore, but I don't think I've seen it go away. I'm also not convinced working around an isolated issue in xen/tools/kconfig/Makefile is appropriate to be done by adding stuff to Rules.mk. Jan
Re: [PATCH v3 12/16] KVM: Move x86's perf guest info callbacks to generic KVM
On Mon, Oct 11, 2021, Marc Zyngier wrote:
> On Wed, 22 Sep 2021 01:05:29 +0100, Sean Christopherson
> wrote:
> > diff --git a/arch/arm64/include/asm/kvm_host.h
> > b/arch/arm64/include/asm/kvm_host.h
> > index ed940aec89e0..828b6eaa2c56 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -673,6 +673,14 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t
> > fault_ipa);
> > void kvm_perf_init(void);
> > void kvm_perf_teardown(void);
> >
> > +#ifdef CONFIG_GUEST_PERF_EVENTS
> > +static inline bool kvm_arch_pmi_in_guest(struct kvm_vcpu *vcpu)
>
> Pardon my x86 ignorance, what is PMI? PMU Interrupt?
Ya, Performance Monitoring Interrupt. I didn't realize the term wasn't common
perf terminology. Maybe kvm_arch_perf_events_in_guest() to be less x86-centric?
> > +{
> > + /* Any callback while a vCPU is loaded is considered to be in guest. */
> > + return !!vcpu;
> > +}
> > +#endif
>
> Do you really need this #ifdef?
Nope, should compile fine without it, though simply dropping the #ifdef would
make
make the semantics of the function wrong, even if nothing consumes it. Tweak it
to use IS_ENABLED()?
return IS_ENABLED(CONFIG_GUEST_PERF_EVENTS) && !!vcpu;
Re: [PATCH v5 08/11] xen/arm: Enable the existing x86 virtual PCI support for ARM.
Hi Jan,
> On 11 Oct 2021, at 15:10, Jan Beulich wrote:
>
> On 11.10.2021 15:34, Bertrand Marquis wrote:
>>> On 11 Oct 2021, at 14:09, Jan Beulich wrote:
>>> On 11.10.2021 14:41, Bertrand Marquis wrote:
> On 7 Oct 2021, at 14:43, Jan Beulich wrote:
> On 06.10.2021 19:40, Rahul Singh wrote:
>> --- /dev/null
>> +++ b/xen/arch/arm/vpci.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * xen/arch/arm/vpci.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +#include
>> +
>> +#include
>> +
>> +#define REGISTER_OFFSET(addr) ( (addr) & 0x0fff)
>
> Nit: Stray blank (like you had in an earlier version for MMCFG_BDF()).
> Also isn't this effectively part of the public interface (along with
> MMCFG_BDF()), alongside GUEST_VPCI_ECAM_{BASE,SIZE}?
I will move that in the next version to xen/pci.h and rename
itMMCFG_REG_OFFSET.
Would that be ok ?
>>>
>>> That would be okay and make sense when put next to MMCFG_BDF(), but
>>> it would not address my comment: That still wouldn't be the public
>>> interface. Otoh you only mimic hardware behavior, so perhaps the
>>> splitting of the address isn't as relevant to put there as would be
>>> GUEST_VPCI_ECAM_{BASE,SIZE}.
>>
>> Ok now I get what you wanted.
>>
>> You would actually like both MMCFG_BDF and MMCFG_REG_OFFSET to
>> be moved to arch-arm.h.
>>
>> Then I am not quite sure to follow why.
>> Those are not macros coming out of a way we have to define this but from
>> how it works in standard PCI.
>> The base and size are needed to know where the PCI bus will be.
>>
>> So why should those be needed in public headers ?
>
> Well, see my "Otoh ..." in the earlier reply. Keeping the two
> address splitting macros out of there is okay.
Ok.
>
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -766,6 +766,24 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>> else
>> iommu_enable_device(pdev);
>
> Please note the context above; ...
>
>> +#ifdef CONFIG_ARM
>> +/*
>> + * On ARM PCI devices discovery will be done by Dom0. Add vpci
>> handler when
>> + * Dom0 inform XEN to add the PCI devices in XEN.
>> + */
>> +ret = vpci_add_handlers(pdev);
>> +if ( ret )
>> +{
>> +printk(XENLOG_ERR "setup of vPCI failed: %d\n", ret);
>> +pci_cleanup_msi(pdev);
>> +ret = iommu_remove_device(pdev);
>> +if ( pdev->domain )
>> +list_del(&pdev->domain_list);
>> +free_pdev(pseg, pdev);
>
> ... you unconditionally undo the if() part of the earlier conditional;
> is there any guarantee that the other path can't ever be taken, now
> and forever? If it can't be taken now (which I think is the case as
> long as Dom0 wouldn't report the same device twice), then at least some
> precaution wants taking. Maybe moving your addition into that if()
> could be an option.
>
> Furthermore I continue to wonder whether this ordering is indeed
> preferable over doing software setup before hardware arrangements. This
> would address the above issue as well as long as vpci_add_handlers() is
> idempotent. And it would likely simplify error cleanup.
I agree with you so I will move this code block before iommu part.
But digging deeper into this, I would have 2 questions:
- msi_cleanup was done there after a request from Stefano, but is not
done in case or iommu error, is there an issue to solve here ?
>>>
>>> Maybe, but I'm not sure. This very much depends on what a domain
>>> could in principle do with a partly set-up device. Plus let's
>>> not forget that we're talking of only Dom0 here (for now at least,
>>> i.e. not considering the dom0less case).
>>>
>>> But I'd also like to further defer to Stefano.
>>
>> Ok, I must admit I do not really see at that stage why doing an MSI cleanup
>> could be needed so I will wait for Stefano to know if I need to keep this
>> when
>> moving the block up (at the end it is theoretical right now as this is
>> empty).
>>
>>>
Same could also go for the free_pdev ?
>>>
>>> I think it's wrong to free_pdev() here. We want to internally keep
>>> record of the device, even if the device ends up unusable. The only
>>> place where free_pdev() ought to be called i
[xen-unstable test] 165460: tolerable FAIL
flight 165460 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/165460/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 165452 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 165452 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 165452 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 165452 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 165452 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 165452 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 165452 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 165452 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 165452 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 165452 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 165452 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 165452 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass version targeted for testing: xen 664cc3c3d381e4f9a61dcb68cbd7a6a00070370e baseline version: xen 664cc3c3d381e4f9
Re: [XEN PATCH v7 20/51] build: avoid re-executing the main Makefile by introducing build.mk
On Mon, Oct 11, 2021 at 12:56:54PM +0200, Jan Beulich wrote: > On 24.08.2021 12:50, Anthony PERARD wrote: > > Currently, the xen/Makefile is re-parsed several times: once to start > > the build process, and several more time with Rules.mk including it. > > This makes it difficult to reason with a Makefile used for several > > purpose, and it actually slow down the build process. > > I'm struggling some with what you want to express here. What does > "to reason" refer to? I guess "to reason with something" isn't an expression. I mean that the main Makefile is difficult to work with as it setup the build process for the rest of the build. And it is difficult to understand what is happening when it recursed into itself, and thus possibly re-executing part of the build process setup. > > So this patch introduce "build.mk" which Rules.mk will use when > > present instead of the "Makefile" of a directory. (Linux's Kbuild > > named that file "Kbuild".) > > > > We have a few targets to move to "build.mk" identified by them been > > build via "make -f Rules.mk" without changing directory. > > > > As for the main targets like "build", we can have them depends on > > there underscore-prefix targets like "_build" without having to use > > "Rules.mk" while still retaining the check for unsupported > > architecture. (Those main rules are changed to be single-colon as > > there should only be a single recipe for them.) > > > > With nearly everything needed to move to "build.mk" moved, there is a > > single dependency left from "Rules.mk": $(TARGET), which is moved to > > the main Makefile. > > I'm having trouble identifying what this describes. Searching for > $(TARGET) in the patch doesn't yield any obvious match. Thinking > about it, do you perhaps mean the setting of that variable? Is > moving that guaranteed to not leave the variable undefined? Or in > other words is there no scenario at all where xen/Makefile might > get bypassed? (Aiui building an individual .o, .i, or .s would > continue to be fine, but it feels like something along these lines > might get broken.) I mean that "xen/Rules.mk" will never "include" "xen/Makefile" after this patch, but the variable "TARGET" is only set in "xen/Rules.mk". But "xen/Makefile" still needs "TARGET" to be set so I moved the assignment of the variable from "xen/Rules.mk" into "xen/Makefile". > > @@ -279,11 +281,13 @@ export CFLAGS_UBSAN > > > > endif # need-config > > > > -.PHONY: build install uninstall clean distclean MAP > > -build install uninstall debug clean distclean MAP:: > > +main-targets := build install uninstall clean distclean MAP > > +.PHONY: $(main-targets) > > ifneq ($(XEN_TARGET_ARCH),x86_32) > > - $(MAKE) -f Rules.mk _$@ > > +$(main-targets): %: _% > > + @: > > Isn't the conventional way to express "no commands" via > > $(main-targets): %: _% ; > > ? I guess, I'll change it. > > --- a/xen/Rules.mk > > +++ b/xen/Rules.mk > > @@ -9,8 +9,6 @@ include $(XEN_ROOT)/Config.mk > > include $(BASEDIR)/scripts/Kbuild.include > > > > > > -TARGET := $(BASEDIR)/xen > > - > > # Note that link order matters! > > Could I talk you into removing yet another blank line at this occasion? Will do. > > @@ -36,7 +34,9 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \ > > rodata.cst$(a)) \ > > $(foreach r,rel rel.ro,data.$(r).local) > > > > -include Makefile > > +# The filename build.mk has precedence over Makefile > > +mk-dir := . > > What's the goal of this variable? All I can spot for now it that ... It's just me thinking ahead where a folling patch will just have to write "$mk-dir := $(src)", instead of editing the following line. > > +include $(if $(wildcard > > $(mk-dir)/build.mk),$(mk-dir)/build.mk,$(mk-dir)/Makefile) > > ... this is harder to read than > > include $(if $(wildcard ./build.mk),./build.mk,./Makefile) > > which could be further simplified to > > include $(if $(wildcard build.mk),build.mk,Makefile) > > and then maybe altered to > > include $(firstword $(wildcard build.mk) Makefile) I can try with this last one, there is less repeating of "build.mk" so that sound like a good thing to do. > > --- /dev/null > > +++ b/xen/build.mk > > @@ -0,0 +1,58 @@ > > +quiet_cmd_banner = BANNER $@ > > +define cmd_banner > > +if which figlet >/dev/null 2>&1 ; then \ > > + echo " Xen $(XEN_FULLVERSION)" | figlet -f $< > $@.tmp; \ > > +else \ > > + echo " Xen $(XEN_FULLVERSION)" > $@.tmp; \ > > +fi; \ > > +mv -f $@.tmp $@ > > +endef > > + > > +.banner: tools/xen.flf FORCE > > + $(call if_changed,banner) > > +targets += .banner > > To make the end of the rule more easily recognizable, may I ask that > you either insert a blank line after the rule or that you move the += > up immediately ahead of the construct? Will do. Thanks, -- Anthony PERARD
Re: [XEN PATCH v7 20/51] build: avoid re-executing the main Makefile by introducing build.mk
On 11.10.2021 16:58, Anthony PERARD wrote: > On Mon, Oct 11, 2021 at 12:56:54PM +0200, Jan Beulich wrote: >> On 24.08.2021 12:50, Anthony PERARD wrote: >>> Currently, the xen/Makefile is re-parsed several times: once to start >>> the build process, and several more time with Rules.mk including it. >>> This makes it difficult to reason with a Makefile used for several >>> purpose, and it actually slow down the build process. >> >> I'm struggling some with what you want to express here. What does >> "to reason" refer to? > > I guess "to reason with something" isn't an expression. I mean that the > main Makefile is difficult to work with as it setup the build process > for the rest of the build. And it is difficult to understand what is > happening when it recursed into itself, and thus possibly re-executing > part of the build process setup. > >>> So this patch introduce "build.mk" which Rules.mk will use when >>> present instead of the "Makefile" of a directory. (Linux's Kbuild >>> named that file "Kbuild".) >>> >>> We have a few targets to move to "build.mk" identified by them been >>> build via "make -f Rules.mk" without changing directory. >>> >>> As for the main targets like "build", we can have them depends on >>> there underscore-prefix targets like "_build" without having to use >>> "Rules.mk" while still retaining the check for unsupported >>> architecture. (Those main rules are changed to be single-colon as >>> there should only be a single recipe for them.) >>> >>> With nearly everything needed to move to "build.mk" moved, there is a >>> single dependency left from "Rules.mk": $(TARGET), which is moved to >>> the main Makefile. >> >> I'm having trouble identifying what this describes. Searching for >> $(TARGET) in the patch doesn't yield any obvious match. Thinking >> about it, do you perhaps mean the setting of that variable? Is >> moving that guaranteed to not leave the variable undefined? Or in >> other words is there no scenario at all where xen/Makefile might >> get bypassed? (Aiui building an individual .o, .i, or .s would >> continue to be fine, but it feels like something along these lines >> might get broken.) > > I mean that "xen/Rules.mk" will never "include" "xen/Makefile" after > this patch, but the variable "TARGET" is only set in "xen/Rules.mk". But > "xen/Makefile" still needs "TARGET" to be set so I moved the assignment > of the variable from "xen/Rules.mk" into "xen/Makefile". Okay, thanks, this confirms the understanding I had developed; maybe you try to reword this some. What your reply doesn't address is my question, though. Jan
Re: [PATCH v3 12/16] KVM: Move x86's perf guest info callbacks to generic KVM
On Mon, 11 Oct 2021 15:46:25 +0100,
Sean Christopherson wrote:
>
> On Mon, Oct 11, 2021, Marc Zyngier wrote:
> > On Wed, 22 Sep 2021 01:05:29 +0100, Sean Christopherson
> > wrote:
> > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > b/arch/arm64/include/asm/kvm_host.h
> > > index ed940aec89e0..828b6eaa2c56 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -673,6 +673,14 @@ int io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t
> > > fault_ipa);
> > > void kvm_perf_init(void);
> > > void kvm_perf_teardown(void);
> > >
> > > +#ifdef CONFIG_GUEST_PERF_EVENTS
> > > +static inline bool kvm_arch_pmi_in_guest(struct kvm_vcpu *vcpu)
> >
> > Pardon my x86 ignorance, what is PMI? PMU Interrupt?
>
> Ya, Performance Monitoring Interrupt. I didn't realize the term wasn't
> common perf terminology. Maybe kvm_arch_perf_events_in_guest() to be
> less x86-centric?
Up to you. I would be happy with just a comment.
>
> > > +{
> > > + /* Any callback while a vCPU is loaded is considered to be in guest. */
> > > + return !!vcpu;
> > > +}
> > > +#endif
> >
> > Do you really need this #ifdef?
>
> Nope, should compile fine without it, though simply dropping the #ifdef
> would make make the semantics of the function wrong, even if nothing
> consumes it. Tweak it to use IS_ENABLED()?
>
> return IS_ENABLED(CONFIG_GUEST_PERF_EVENTS) && !!vcpu;
LGTM.
M.
--
Without deviation from the norm, progress is not possible.
Re: [XEN PATCH v7 30/51] build: hook kconfig into xen build system
On 24.08.2021 12:50, Anthony PERARD wrote: > Now that xen's build system is very close to Linux's ones, we can hook > "Makefile.host" into Xen's build system, and we can build Kconfig with > that. > > Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich
Re: [XEN PATCH v7 32/51] build: Remove KBUILD_ specific from Makefile.host
On 24.08.2021 12:50, Anthony PERARD wrote: > This will allow $(HOSTCFLAGS) to actually be used when building > programmes for the build-host. > > The other variable don't exist in our build system. > > Also remove $(KBUILD_EXTMOD) since it should always be empty. > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich I wonder though whether their use of KBUILD_ prefixes doesn't match our XEN_ ones (e.g. KBUILD_CFLAGS vs XEN_CFLAGS), in which case replacing rather than stripping might be the way to go. Jan
Re: [XEN PATCH v7 33/51] build: handle always-y and hostprogs-always-y
On 24.08.2021 12:50, Anthony PERARD wrote: > This will be used for xen/tools/. > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich
Re: [XEN PATCH v7 34/51] build: start building the tools with the main makefiles
On 24.08.2021 12:50, Anthony PERARD wrote: > This will make out-of-tree build easier. > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich
Re: [XEN PATCH v7 35/51] build: Add headers path to CFLAGS once for all archs
On 24.08.2021 12:50, Anthony PERARD wrote: > This just remove duplication. > > Signed-off-by: Anthony PERARD Ah, here we go. Reviewed-by: Jan Beulich
Re: [XEN PATCH v7 36/51] build: generate x86's asm-macros.h with filechk
On 24.08.2021 12:50, Anthony PERARD wrote: > When we will build out-of-tree, make is going to try to generate > "asm-macros.h" before the directories "arch/x86/include/asm" exist, > thus we would need to call `mkdir` explicitly. We will use "filechk" > for that as it does everything that the current recipe does and does > call `mkdir`. > > Also, they are no more "*.new" files generated in this directory. > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich
Re: [XEN PATCH v7 37/51] build: clean-up "clean" rules of duplication
On 24.08.2021 12:50, Anthony PERARD wrote: > All those files to be removed are already done in the main Makefile, > either by the "find" command or directly (for $(TARGET).efi). > > Signed-off-by: Anthony PERARD Acked-by: Jan Beulich
