Re: [PATCH v4 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common
On 10/31/23 06:56, Jan Beulich wrote:
> On 31.10.2023 00:52, Stewart Hildebrand wrote:
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct
>> xen_domctl_createdomain *config)
>> {
>> unsigned int max_vcpus;
>> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
>> -unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
>> XEN_DOMCTL_CDF_vpmu);
>> +unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
>> XEN_DOMCTL_CDF_vpmu |
>> + XEN_DOMCTL_CDF_vpci);
>
> Is the flag (going to be, with the initial work) okay to have for Dom0
> on Arm?
Hm. Allowing/enabling vPCI for dom0 on ARM should follow or be part of the PCI
passthrough SMMU series [1]. I'll move this change to the next patch ("xen/arm:
enable vPCI for dom0") and add a note over there.
[1] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg00210.html
>
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -712,7 +712,8 @@ int arch_sanitise_domain_config(struct
>> xen_domctl_createdomain *config)
>> return 0;
>> }
>>
>> -static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>> +static bool emulation_flags_ok(const struct domain *d, uint32_t emflags,
>> + uint32_t cdf)
>
> While apparently views differ, ./CODING_STYLE wants "unsigned int" to be
> used for the latter two arguments.
OK, I'll change both to unsigned int.
>
>> @@ -722,14 +723,17 @@ static bool emulation_flags_ok(const struct domain *d,
>> uint32_t emflags)
>> if ( is_hvm_domain(d) )
>> {
>> if ( is_hardware_domain(d) &&
>> - emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
>> + (!( cdf & XEN_DOMCTL_CDF_vpci ) ||
>
> Nit: Stray blanks inside the inner parentheses.
OK, I'll fix.
>
>> + emflags != (X86_EMU_LAPIC | X86_EMU_IOAPIC)) )
>> return false;
>> if ( !is_hardware_domain(d) &&
>> - emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
>> - emflags != X86_EMU_LAPIC )
>> + ((cdf & XEN_DOMCTL_CDF_vpci) ||
>> + (emflags != X86_EMU_ALL &&
>> + emflags != X86_EMU_LAPIC)) )
>> return false;
>> }
>> -else if ( emflags != 0 && emflags != X86_EMU_PIT )
>> +else if ( (cdf & XEN_DOMCTL_CDF_vpci) ||
>
> Wouldn't this better be enforced in common code?
Yes, I will move it to xen/common/domain.c:sanitise_domain_config().
>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -892,10 +892,11 @@ static struct domain *__init create_dom0(const
>> module_t *image,
>> {
>> dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
>> ((hvm_hap_supported() && !opt_dom0_shadow) ?
>> -XEN_DOMCTL_CDF_hap : 0));
>> +XEN_DOMCTL_CDF_hap : 0) |
>> + XEN_DOMCTL_CDF_vpci);
>
> Less of a change and imo slightly neater as a result would be to simply
> put the addition on the same line where CDF_hvm already is. But as with
> many style aspects, views may differ here of course ...
I'll change it.
>
> Jan
Re: [PATCH v4 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common
On 31.10.2023 00:52, Stewart Hildebrand wrote:
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> {
> unsigned int max_vcpus;
> unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
> -unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
> XEN_DOMCTL_CDF_vpmu);
> +unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu |
> XEN_DOMCTL_CDF_vpmu |
> + XEN_DOMCTL_CDF_vpci);
Is the flag (going to be, with the initial work) okay to have for Dom0
on Arm?
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -712,7 +712,8 @@ int arch_sanitise_domain_config(struct
> xen_domctl_createdomain *config)
> return 0;
> }
>
> -static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> +static bool emulation_flags_ok(const struct domain *d, uint32_t emflags,
> + uint32_t cdf)
While apparently views differ, ./CODING_STYLE wants "unsigned int" to be
used for the latter two arguments.
> @@ -722,14 +723,17 @@ static bool emulation_flags_ok(const struct domain *d,
> uint32_t emflags)
> if ( is_hvm_domain(d) )
> {
> if ( is_hardware_domain(d) &&
> - emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> + (!( cdf & XEN_DOMCTL_CDF_vpci ) ||
Nit: Stray blanks inside the inner parentheses.
> + emflags != (X86_EMU_LAPIC | X86_EMU_IOAPIC)) )
> return false;
> if ( !is_hardware_domain(d) &&
> - emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
> - emflags != X86_EMU_LAPIC )
> + ((cdf & XEN_DOMCTL_CDF_vpci) ||
> + (emflags != X86_EMU_ALL &&
> + emflags != X86_EMU_LAPIC)) )
> return false;
> }
> -else if ( emflags != 0 && emflags != X86_EMU_PIT )
> +else if ( (cdf & XEN_DOMCTL_CDF_vpci) ||
Wouldn't this better be enforced in common code?
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -892,10 +892,11 @@ static struct domain *__init create_dom0(const module_t
> *image,
> {
> dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
> ((hvm_hap_supported() && !opt_dom0_shadow) ?
> -XEN_DOMCTL_CDF_hap : 0));
> +XEN_DOMCTL_CDF_hap : 0) |
> + XEN_DOMCTL_CDF_vpci);
Less of a change and imo slightly neater as a result would be to simply
put the addition on the same line where CDF_hvm already is. But as with
many style aspects, views may differ here of course ...
Jan
[PATCH v4 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common
Both x86 and ARM need a way at domain creation time to specify whether
the domain needs vPCI emulation. Move the vPCI flag from x86
xen_domctl_createdomain.arch.emulation_flags to the common
xen_domctl_createdomain.flags.
Move has_vpci() macro to common header.
Bump XEN_DOMCTL_INTERFACE_VERSION since we're modifying flags inside
struct xen_domctl_createdomain and xen_arch_domainconfig.
Signed-off-by: Oleksandr Andrushchenko
Signed-off-by: Rahul Singh
Signed-off-by: Stewart Hildebrand
---
v3->v4:
* renamed, was:
("xen/arm: pci: plumb xen_arch_domainconfig with pci info")
* reworked: move x86 vPCI flag to common instead of adding another arch
specific vPCI flag
* folded ("xen/arm: make has_vpci() depend on d->arch.has_vpci") into
this patch (retain Signed-off-by's from [1] and [2])
v2->v3:
* new patch
[1]
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/27be1729ce8128dbe37275ce7946b2fbd2e5a382
[2]
https://github.com/xen-troops/xen/commit/bf12185e6fb2e31db0d8e6ea9ccd8a02abadec17
---
tools/libs/light/libxl_x86.c | 5 -
tools/ocaml/libs/xc/xenctrl.ml| 2 +-
tools/ocaml/libs/xc/xenctrl.mli | 2 +-
tools/python/xen/lowlevel/xc/xc.c | 5 -
xen/arch/arm/domain.c | 3 ++-
xen/arch/arm/include/asm/domain.h | 3 ---
xen/arch/x86/domain.c | 16 ++--
xen/arch/x86/include/asm/domain.h | 6 +-
xen/arch/x86/setup.c | 5 +++--
xen/common/domain.c | 10 +-
xen/include/public/arch-x86/xen.h | 5 +
xen/include/public/domctl.h | 7 +--
xen/include/xen/domain.h | 2 ++
13 files changed, 43 insertions(+), 28 deletions(-)
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index d16573e72cd4..ebce1552accd 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -8,13 +8,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
{
switch(d_config->c_info.type) {
case LIBXL_DOMAIN_TYPE_HVM:
-config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
+config->arch.emulation_flags = XEN_X86_EMU_ALL;
+config->flags &= ~XEN_DOMCTL_CDF_vpci;
break;
case LIBXL_DOMAIN_TYPE_PVH:
config->arch.emulation_flags = XEN_X86_EMU_LAPIC;
+config->flags &= ~XEN_DOMCTL_CDF_vpci;
break;
case LIBXL_DOMAIN_TYPE_PV:
config->arch.emulation_flags = 0;
+config->flags &= ~XEN_DOMCTL_CDF_vpci;
break;
default:
abort();
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml
index d6c6eb73db44..6f3da9c6e064 100644
--- a/tools/ocaml/libs/xc/xenctrl.ml
+++ b/tools/ocaml/libs/xc/xenctrl.ml
@@ -46,7 +46,6 @@ type x86_arch_emulation_flags =
| X86_EMU_IOMMU
| X86_EMU_PIT
| X86_EMU_USE_PIRQ
- | X86_EMU_VPCI
type x86_arch_misc_flags =
| X86_MSR_RELAXED
@@ -70,6 +69,7 @@ type domain_create_flag =
| CDF_IOMMU
| CDF_NESTED_VIRT
| CDF_VPMU
+ | CDF_VPCI
type domain_create_iommu_opts =
| IOMMU_NO_SHAREPT
diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli
index 3bfc16edba96..e2dd02bec962 100644
--- a/tools/ocaml/libs/xc/xenctrl.mli
+++ b/tools/ocaml/libs/xc/xenctrl.mli
@@ -40,7 +40,6 @@ type x86_arch_emulation_flags =
| X86_EMU_IOMMU
| X86_EMU_PIT
| X86_EMU_USE_PIRQ
- | X86_EMU_VPCI
type x86_arch_misc_flags =
| X86_MSR_RELAXED
@@ -63,6 +62,7 @@ type domain_create_flag =
| CDF_IOMMU
| CDF_NESTED_VIRT
| CDF_VPMU
+ | CDF_VPCI
type domain_create_iommu_opts =
| IOMMU_NO_SHAREPT
diff --git a/tools/python/xen/lowlevel/xc/xc.c
b/tools/python/xen/lowlevel/xc/xc.c
index d3ea350e07b9..e3623cdcb90d 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -159,7 +159,10 @@ static PyObject *pyxc_domain_create(XcObject *self,
#if defined (__i386) || defined(__x86_64__)
if ( config.flags & XEN_DOMCTL_CDF_hvm )
-config.arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
+{
+config.arch.emulation_flags = XEN_X86_EMU_ALL;
+config.flags &= ~XEN_DOMCTL_CDF_vpci;
+}
#elif defined (__arm__) || defined(__aarch64__)
config.arch.gic_version = XEN_DOMCTL_CONFIG_GIC_NATIVE;
#else
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 28e3aaa5e482..1409a4235e13 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct
xen_domctl_createdomain *config)
{
unsigned int max_vcpus;
unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
-unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu);
+unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu |
+ XEN_DOMCTL_CDF_vpci);
unsigned int sve_vl_bits = sve_decode_vl(config->arch.sve_vl);
if ( (config->flags & ~flags_optional) !=
