Re: Patches on libvirt patchew are outdated

[Han Han]

The data of libvirt patchew has been synced. Thanks

On Mon, Dec 18, 2023 at 9:57 PM Paolo Bonzini  wrote:

> On Mon, Dec 18, 2023 at 2:14 PM Peter Krempa  wrote:
> > The patches are now picked up, but I've noticed that all series' are
> > marked as "does not apply to master".
> >
> > Unfortunately the new mailing list (mailman3 + hyperkitty) has a bad
> > habit of touching all the patches. Very specifically it recodes and
> > quotes CRLF, which makes it impossible to apply via 'git am' in default
> > configuration and requires using 'git am --quoted-cr=strip'. This
> > applies both in mail and also in archives.
>
> No, that was just a typo when switching away from git:// as the
> protocol used to fetch from the upstream repo. If you see a series
> where Mailman3's re-encoding is an issue please let me know, but I
> have reset most series now and they applied fine.
>
>
> Paolo
>
>
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [libvirt PATCH 3/5] Config some capabilities for loongarch virt machine

[lixianglai]

Hi Andrea :

On Thu, Dec 14, 2023 at 02:08:47PM +0800, xianglai li wrote:

+++ b/src/qemu/qemu_domain.c
@@ -5635,6 +5635,11 @@ 
qemuDomainControllerDefPostParse(virDomainControllerDef *cont,

cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+ } else if (ARCH_IS_LOONGARCH(def->os.arch)) {
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+ else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+ cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
}

I don't think you need to take into account the nec-xhci model for
loongarch. aarch64 needs it because qemu-xhci didn't exist when that
architecture was introduced, but that's not the case here so we can
keep things simpler.

I'm surprised that this code doesn't have handling for riscv64. Not
your problem, but likely an oversight that should be addressed.



Ok, I'll remove nec-xhci in the next version.


+static bool
+qemuDomainMachineIsLoongson(const char *machine,
+ const virArch arch)

The appropriate name for this function would be
qemuDomainMachineIsLoongArchVirt, to match the existing Arm and
RISC-V equivalents.



Ok, I'll correct that in the next version.


+bool
+qemuDomainIsLoongson(const virDomainDef *def)
+{

Same here.



Ok, I'll correct that in the next version.



+++ b/src/qemu/qemu_domain_address.c
@@ -2093,6 +2143,11 @@ 
qemuDomainValidateDevicePCISlotsChipsets(virDomainDef *def,

return -1;
}

+ if (qemuDomainIsLoongson(def) &&
+ qemuDomainValidateDevicePCISlotsLoongson(def, addrs) < 0) {
+ return -1;
+ }

The existing qemuDomainValidateDevicePCISlots* functions are intended
to ensure that certain devices, that historically have been assigned
to specific PCI slots by QEMU, always show up at those addresses.

We haven't needed anything like that for non-x86 architectures so
far, and I believe that loongarch doesn't need it either.



Ok, I'll correct that in the next version.




+++ b/src/qemu/qemu_validate.c
@@ -100,7 +100,7 @@ qemuValidateDomainDefFeatures(const virDomainDef 
*def,

switch ((virDomainFeature) i) {
case VIR_DOMAIN_FEATURE_IOAPIC:
if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE) {
- if (!ARCH_IS_X86(def->os.arch)) {
+ if (!ARCH_IS_X86(def->os.arch) && !ARCH_IS_LOONGARCH(def->os.arch)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%1$s' feature is not supported for architecture '%2$s' or 
machine type '%3$s'"),

featureName,

So does loongarch actually have ioapic support? Just making sure. I'm
surprised because apparently no other non-x86 architecture supports
it...



Yes, loongarch does have IOAPIC, but this feature has no effect on 
loongarch at this stage, I will cut it first to simplify the committed code.


In addition, I have a question, if I understand correctly, the IOAPIC 
here should be the device interrupt controller, which is located in the 
bridge chip,


it is called IOAPIC under x86, PCH_PIC under loongarch, and GIC under arm.

The kernel_irqchip attribute of the machine parameter in qemu 
corresponding to the function VIR_DOMAIN_FEATURE_IOAPIC determines


whether the device interrupt controller is simulated in qemu or kvm. So 
arm also has such a need, but why doesn't arm add?



Thanks,

Xianglai.





___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [libvirt PATCH 3/5] Config some capabilities for loongarch virt machine

[lixianglai]

Hi Andrea :

On Thu, Dec 14, 2023 at 02:08:47PM +0800, xianglai li wrote:

+++ b/src/qemu/qemu_domain.c
@@ -5635,6 +5635,11 @@ qemuDomainControllerDefPostParse(virDomainControllerDef 
*cont,
  cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
  else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
  cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
+} else if (ARCH_IS_LOONGARCH(def->os.arch)) {
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QEMU_XHCI))
+cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_QEMU_XHCI;
+else if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NEC_USB_XHCI))
+cont->model = VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI;
  }

I don't think you need to take into account the nec-xhci model for
loongarch. aarch64 needs it because qemu-xhci didn't exist when that
architecture was introduced, but that's not the case here so we can
keep things simpler.

I'm surprised that this code doesn't have handling for riscv64. Not
your problem, but likely an oversight that should be addressed.



Ok, I'll remove nec-xhci in the next version.


+static bool
+qemuDomainMachineIsLoongson(const char *machine,
+const virArch arch)

The appropriate name for this function would be
qemuDomainMachineIsLoongArchVirt, to match the existing Arm and
RISC-V equivalents.



Ok, I'll correct that in the next version.


+bool
+qemuDomainIsLoongson(const virDomainDef *def)
+{

Same here.



Ok, I'll correct that in the next version.



+++ b/src/qemu/qemu_domain_address.c
@@ -2093,6 +2143,11 @@ qemuDomainValidateDevicePCISlotsChipsets(virDomainDef 
*def,
  return -1;
  }

+if (qemuDomainIsLoongson(def) &&
+qemuDomainValidateDevicePCISlotsLoongson(def, addrs) < 0) {
+return -1;
+}

The existing qemuDomainValidateDevicePCISlots* functions are intended
to ensure that certain devices, that historically have been assigned
to specific PCI slots by QEMU, always show up at those addresses.

We haven't needed anything like that for non-x86 architectures so
far, and I believe that loongarch doesn't need it either.



Ok, I'll correct that in the next version.




+++ b/src/qemu/qemu_validate.c
@@ -100,7 +100,7 @@ qemuValidateDomainDefFeatures(const virDomainDef *def,
  switch ((virDomainFeature) i) {
  case VIR_DOMAIN_FEATURE_IOAPIC:
  if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE) {
-if (!ARCH_IS_X86(def->os.arch)) {
+if (!ARCH_IS_X86(def->os.arch) && 
!ARCH_IS_LOONGARCH(def->os.arch)) {
  virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
 _("The '%1$s' feature is not supported for 
architecture '%2$s' or machine type '%3$s'"),
 featureName,

So does loongarch actually have ioapic support? Just making sure. I'm
surprised because apparently no other non-x86 architecture supports
it...



Yes, loongarch does have IOAPIC, but this feature has no effect on 
loongarch at this stage, I will cut it first to simplify the committed code.


In addition, I have a question, if I understand correctly, the IOAPIC 
here should be the device interrupt controller, which is located in the 
bridge chip,


it is called IOAPIC under x86, PCH_PIC under loongarch, and GIC under arm.

The kernel_irqchip attribute of the machine parameter in qemu 
corresponding to the function VIR_DOMAIN_FEATURE_IOAPIC determines


whether the device interrupt controller is simulated in qemu or kvm. So 
arm also has such a need, but why doesn't arm add?






___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH] apparmor: Add capabilities for PCI passthrough to virtxend profile

[Jim Fehlig]

When splitting out the apparmor modular daemon profiles from the
libvirtd profile, the net_admin and sys_admin capabilities were
dropped from the virtxend profile. It was not known at the time
that these capabilities were needed for PCI passthrough. Without
the capabilities, the following messages are emitted from the audit
subsystem

audit: type=1400 audit(1702939277.946:63): apparmor="DENIED" \
operation="capable" class="cap" profile="virtxend" pid=3611 \
comm="rpc-virtxend" capability=21  capname="sys_admin"
audit: type=1400 audit(1702940304.818:63): apparmor="DENIED" \
operation="capable" class="cap" profile="virtxend" pid=3731 \
comm="rpc-virtxend" capability=12  capname="net_admin"

It appears sys_admin is needed to simply read from the PCI dev's
sysfs config file. The net_admin capability is needed when setting
the MAC address of an SR-IOV virtual function.

Signed-off-by: Jim Fehlig 
---
 src/security/apparmor/usr.sbin.virtxend.in | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/security/apparmor/usr.sbin.virtxend.in 
b/src/security/apparmor/usr.sbin.virtxend.in
index 78a11305f5..77fedce352 100644
--- a/src/security/apparmor/usr.sbin.virtxend.in
+++ b/src/security/apparmor/usr.sbin.virtxend.in
@@ -5,8 +5,10 @@ profile virtxend @sbindir@/virtxend 
flags=(attach_disconnected) {
   #include 
 
   capability kill,
+  capability net_admin,
   capability setgid,
   capability setuid,
+  capability sys_admin,
   capability sys_pacct,
   capability ipc_lock,
 
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 7/8] test: add tests for the vf-token flag

[Vivek Kashyap]

Add tests for the vf-token flag to the qemuxml2argv and qemuxml2xml
test suites

Signed-off-by: Vivek Kashyap 
Signed-off-by: Ciara Loftus 
---
 .../hostdev-vfio-vf-token.x86_64-latest.args  | 34 
 .../hostdev-vfio-vf-token.xml | 22 ++
 tests/qemuxml2argvtest.c  |  1 +
 .../hostdev-vfio-vf-token.x86_64-latest.xml   | 40 +++
 tests/qemuxml2xmltest.c   |  1 +
 5 files changed, 98 insertions(+)
 create mode 100644 
tests/qemuxml2argvdata/hostdev-vfio-vf-token.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-vf-token.xml
 create mode 100644 
tests/qemuxml2xmloutdata/hostdev-vfio-vf-token.x86_64-latest.xml

diff --git a/tests/qemuxml2argvdata/hostdev-vfio-vf-token.x86_64-latest.args 
b/tests/qemuxml2argvdata/hostdev-vfio-vf-token.x86_64-latest.args
new file mode 100644
index 00..e449c84ea9
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-vf-token.x86_64-latest.args
@@ -0,0 +1,34 @@
+LC_ALL=C \
+PATH=/bin \
+HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1 \
+USER=test \
+LOGNAME=test \
+XDG_DATA_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.local/share \
+XDG_CACHE_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.cache \
+XDG_CONFIG_HOME=/var/lib/libvirt/qemu/domain--1-QEMUGuest1/.config \
+/usr/bin/qemu-system-x86_64 \
+-name guest=QEMUGuest1,debug-threads=on \
+-S \
+-object 
'{"qom-type":"secret","id":"masterKey0","format":"raw","file":"/var/lib/libvirt/qemu/domain--1-QEMUGuest1/master-key.aes"}'
 \
+-machine pc,usb=off,dump-guest-core=off,memory-backend=pc.ram,acpi=off \
+-accel tcg \
+-cpu qemu64 \
+-m size=219136k \
+-object '{"qom-type":"memory-backend-ram","id":"pc.ram","size":224395264}' \
+-overcommit mem-lock=off \
+-smp 1,sockets=1,cores=1,threads=1 \
+-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
+-display none \
+-no-user-config \
+-nodefaults \
+-chardev socket,id=charmonitor,fd=1729,server=on,wait=off \
+-mon chardev=charmonitor,id=monitor,mode=control \
+-rtc base=utc \
+-no-shutdown \
+-boot strict=on \
+-device 
'{"driver":"piix3-usb-uhci","id":"usb","bus":"pci.0","addr":"0x1.0x2"}' \
+-audiodev '{"id":"audio1","driver":"none"}' \
+-device 
'{"driver":"vfio-pci","host":":00:00.0","vf-token":"00112233-4455-6677-8899-aabbccddeeff","id":"hostdev0","bus":"pci.0","addr":"0x8"}'
 \
+-device 
'{"driver":"virtio-balloon-pci","id":"balloon0","bus":"pci.0","addr":"0x2"}' \
+-sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
+-msg timestamp=on
diff --git a/tests/qemuxml2argvdata/hostdev-vfio-vf-token.xml 
b/tests/qemuxml2argvdata/hostdev-vfio-vf-token.xml
new file mode 100644
index 00..87762a
--- /dev/null
+++ b/tests/qemuxml2argvdata/hostdev-vfio-vf-token.xml
@@ -0,0 +1,22 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  
+hvm
+
+  
+  
+/usr/bin/qemu-system-x86_64
+
+
+  
+  
+
+  
+
+  
+  
+
+  
+
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b2ea2191dc..20bc914748 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1547,6 +1547,7 @@ mymain(void)
 DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-pci-duplicate");
 DO_TEST_CAPS_LATEST("hostdev-vfio");
 DO_TEST_CAPS_LATEST("hostdev-vfio-multidomain");
+DO_TEST_CAPS_LATEST("hostdev-vfio-vf-token");
 DO_TEST_CAPS_LATEST("hostdev-mdev-precreated");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-mdev-src-address-invalid");
 DO_TEST_CAPS_LATEST_PARSE_ERROR("hostdev-mdev-invalid-target-address");
diff --git a/tests/qemuxml2xmloutdata/hostdev-vfio-vf-token.x86_64-latest.xml 
b/tests/qemuxml2xmloutdata/hostdev-vfio-vf-token.x86_64-latest.xml
new file mode 100644
index 00..65c4fc6a4a
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/hostdev-vfio-vf-token.x86_64-latest.xml
@@ -0,0 +1,40 @@
+
+  QEMUGuest1
+  c7a5fdbd-edaf-9455-926a-d65c16db1809
+  219100
+  219100
+  1
+  
+hvm
+
+  
+  
+qemu64
+  
+  
+  destroy
+  restart
+  destroy
+  
+/usr/bin/qemu-system-x86_64
+
+
+  
+
+
+
+
+
+  
+  
+
+  
+
+  
+  
+
+
+  
+
+  
+
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 1010b68ebc..4b2c0e980a 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -432,6 +432,7 @@ mymain(void)
 DO_TEST_CAPS_LATEST("hostdev-pci-address-unassigned");
 DO_TEST_CAPS_LATEST("hostdev-pci-multifunction");
 DO_TEST_CAPS_LATEST("hostdev-vfio");
+DO_TEST_CAPS_LATEST("hostdev-vfio-vf-token");
 DO_TEST_CAPS_ARCH_LATEST("hostdev-vfio-zpci", "s390x");
 DO_TEST_CAPS_ARCH_LATEST("hostdev-vfio-zpci-multidomain-many", "s390x");
 DO_TEST_CAPS_ARCH_LATEST("hostdev-vfio-zpci-autogenerate", "s390x");
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org

[vf-token 8/8] NEWS: Update news about vf-token

[Vivek Kashyap]

Update news about vf-token

Signed-off-by: Vivek Kashyap 
---
 NEWS.rst | 8 
 1 file changed, 8 insertions(+)

diff --git a/NEWS.rst b/NEWS.rst
index dc40602c72..5e6a7c3147 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -17,6 +17,14 @@ v10.0.0 (unreleased)
 
 * **New features**
 
+  * qemu: support VF tokens for vfio-pci
+
+"vf-token",implemented as a UUID is part of VFIO PCI ABI, and acts as
+a shared key between vfio PF and VF drivers. The token is set by the
+PF driver and the VF driver provides it to access the VF. The
+vfio vf-token uuid is included in the VM XML specification for the pci
+device, and the token is passed in qemu commandline on VM launch.
+
 * **Improvements**
 
 * **Bug fixes**
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 6/8] docs: Update documentation and vf-token schema

[Vivek Kashyap]

Provide information about the vf-token flag

Signed-off-by: Vivek Kashyap 
---
 docs/formatdomain.rst   | 3 +++
 src/conf/schemas/basictypes.rng | 7 +++
 2 files changed, 10 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
index 310d2bc427..29a7b3145e 100644
--- a/docs/formatdomain.rst
+++ b/docs/formatdomain.rst
@@ -3744,6 +3744,9 @@ control where on the bus the device will be placed:
between 0x0001 and 0x, inclusive), and ``fid`` (a hex value between
0x and 0x, inclusive) used by PCI devices on S390 for
User-defined Identifiers and Function Identifiers.
+   The ``vf-token`` element is supported in uuid format. The vf-token is a
+   shared secret between userspace vfio-pci PF driver and VF driver. The
+   token is set by the PF driver, and must be provided for VF access.
:since:`Since 1.3.5` , some hypervisor drivers may accept an
 element with no other attributes as an explicit
request to assign a PCI address for the device rather than some other type 
of
diff --git a/src/conf/schemas/basictypes.rng b/src/conf/schemas/basictypes.rng
index 26eb538077..bbb7484430 100644
--- a/src/conf/schemas/basictypes.rng
+++ b/src/conf/schemas/basictypes.rng
@@ -121,6 +121,13 @@
 
   
 
+
+ 
+   
+ 
+   
+ 
+
   
   
 
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 5/8] qemu: Introduce validation for vf-token

[Vivek Kashyap]

Introduce a validation function for vf-token support in qemu and
generate vf-token device attribute in qemu command line

Signed-off-by: Vivek Kashyap 
---
 src/qemu/qemu_command.c  |  8 
 src/qemu/qemu_validate.c | 20 
 2 files changed, 28 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 54fb8220e8..0e81a3ed73 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4706,6 +4706,7 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
 virDomainNetTeamingInfo *teaming;
 g_autofree char *host = virPCIDeviceAddressAsString(&pcisrc->addr);
 const char *failover_pair_id = NULL;
+g_autofree char *token = NULL;
 
 /* caller has to assign proper passthrough backend type */
 switch (pcisrc->backend) {
@@ -4732,9 +4733,16 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,
 teaming->persistent)
 failover_pair_id = teaming->persistent;
 
+if ((dev->info->pciAddrExtFlags & VIR_PCI_ADDRESS_EXTENSION_VFTOKEN) &&
+pcisrc->addr.token.isSet) {
+token = g_new0(char, VIR_UUID_STRING_BUFLEN);
+virUUIDFormat(pcisrc->addr.token.uuid, token);
+}
+
 if (virJSONValueObjectAdd(&props,
   "s:driver", "vfio-pci",
   "s:host", host,
+  "S:vf-token", token,
   "s:id", dev->info->alias,
   "p:bootindex", dev->info->effectiveBootIndex,
   "S:failover_pair_id", failover_pair_id,
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index e475ad035e..13114ca3d1 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -1364,6 +1364,24 @@ 
qemuValidateDomainDeviceDefZPCIAddress(virDomainDeviceInfo *info,
 }
 
 
+static int
+qemuValidateDomainDeviceDefVFTokenId(virDomainDeviceInfo *info,
+ virQEMUCaps *qemuCaps)
+{
+virPCIDeviceToken *vftoken = &info->addr.pci.token;
+
+if (virPCIVFIOTokenIDIsPresent(vftoken) &&
+!virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_VFTOKEN)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+   "%s",
+   _("This QEMU binary doesn't support vf token ids"));
+return -1;
+}
+
+return 0;
+}
+
+
 static int
 qemuValidateDomainDeviceDefAddressDrive(virDomainDeviceInfo *info,
 const virDomainDef *def,
@@ -1483,6 +1501,8 @@ qemuValidateDomainDeviceDefAddress(const 
virDomainDeviceDef *dev,
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI:
 if (qemuValidateDomainDeviceDefZPCIAddress(info, qemuCaps) < 0)
 return -1;
+ if (qemuValidateDomainDeviceDefVFTokenId(info, qemuCaps) < 0)
+return -1;
 break;
 
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE:
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 4/8] conf: XML parsing and formatting of vf-token

[Vivek Kashyap]

This patch introduces new XML parser/formatter functions for
parsing the vf-token

Signed-off-by: Vivek Kashyap 
Signed-off-by: Ciara Loftus 
---
 src/conf/device_conf.c   | 49 ++--
 src/conf/domain_conf.c   |  8 +++
 src/libvirt_private.syms |  1 +
 src/util/virpci.c|  7 ++
 src/util/virpci.h|  2 ++
 5 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
index f3d977f2b7..f490aeef9a 100644
--- a/src/conf/device_conf.c
+++ b/src/conf/device_conf.c
@@ -70,6 +70,21 @@ virZPCIDeviceAddressParseXML(xmlNodePtr node,
 return 0;
 }
 
+
+static int
+virPCIDeviceTokenParseXML(xmlNodePtr node,
+  virPCIDeviceAddress *addr)
+{
+if (virXMLPropUUID(node, "uuid", VIR_XML_PROP_NONE,
+   addr->token.uuid) < 0)
+   return -1;
+
+addr->token.isSet = 1;
+
+return 0;
+}
+
+
 void
 virDomainDeviceInfoClear(virDomainDeviceInfo *info)
 {
@@ -200,6 +215,7 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
 virPCIDeviceAddress *addr)
 {
 xmlNodePtr zpci;
+xmlNodePtr token;
 
 memset(addr, 0, sizeof(*addr));
 
@@ -231,6 +247,11 @@ virPCIDeviceAddressParseXML(xmlNodePtr node,
 return -1;
 }
 
+if ((token = virXMLNodeGetSubelement(node, "vf-token"))) {
+   if (virPCIDeviceTokenParseXML(token, addr) < 0)
+   return -1;
+}
+
 return 0;
 }
 
@@ -239,13 +260,27 @@ virPCIDeviceAddressFormat(virBuffer *buf,
   virPCIDeviceAddress addr,
   bool includeTypeInAddr)
 {
-virBufferAsprintf(buf, "\n",
-  includeTypeInAddr ? "type='pci' " : "",
-  addr.domain,
-  addr.bus,
-  addr.slot,
-  addr.function);
+g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER;
+g_auto(virBuffer) tokenBuf = VIR_BUFFER_INIT_CHILD(buf);
+virBuffer *tb = NULL;
+
+virBufferAsprintf(&attrBuf, " %sdomain='0x%04x' bus='0x%02x' "
+  "slot='0x%02x' function='0x%d'",
+   includeTypeInAddr ? "type='pci' " : "",
+   addr.domain,
+   addr.bus,
+   addr.slot,
+   addr.function);
+
+if (virPCIVFIOTokenIDIsPresent(&addr.token)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+virBufferAsprintf(&tokenBuf, "\n",
+  virUUIDFormat(addr.token.uuid, uuidstr));
+tb = &tokenBuf;
+}
+
+virXMLFormatElement(buf, "address", &attrBuf, tb);
 }
 
 int
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 22ad43e1d7..8bda81815a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5403,6 +5403,14 @@ virDomainDeviceInfoFormat(virBuffer *buf,
   info->addr.pci.zpci.uid.value,
   info->addr.pci.zpci.fid.value);
 }
+
+if (virPCIVFIOTokenIDIsPresent(&info->addr.pci.token)) {
+char uuidstr[VIR_UUID_STRING_BUFLEN];
+
+virBufferAsprintf(&childBuf, "\n",
+  virUUIDFormat(info->addr.pci.token.uuid,
+uuidstr));
+}
 break;
 
 case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE:
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 31c0f169c3..b2bc26c323 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3138,6 +3138,7 @@ virPCIHeaderTypeToString;
 virPCIIsVirtualFunction;
 virPCIStubDriverTypeFromString;
 virPCIStubDriverTypeToString;
+virPCIVFIOTokenIDIsPresent;
 virPCIVirtualFunctionListFree;
 virZPCIDeviceAddressIsIncomplete;
 virZPCIDeviceAddressIsPresent;
diff --git a/src/util/virpci.c b/src/util/virpci.c
index afce7b52b7..0a9ae7a881 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -2314,6 +2314,13 @@ virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress 
*addr)
 }
 
 
+bool
+virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token)
+{
+return token->isSet;
+}
+
+
 void
 virPCIVirtualFunctionListFree(virPCIVirtualFunctionList *list)
 {
diff --git a/src/util/virpci.h b/src/util/virpci.h
index da32c2f4d2..8510752e84 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -271,6 +271,8 @@ int virPCIDeviceAddressParse(char *address, 
virPCIDeviceAddress *bdf);
 bool virZPCIDeviceAddressIsIncomplete(const virZPCIDeviceAddress *addr);
 bool virZPCIDeviceAddressIsPresent(const virZPCIDeviceAddress *addr);
 
+bool virPCIVFIOTokenIDIsPresent(const virPCIDeviceToken *token);
+
 int virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path,
  int pfNetDevIdx,
  char **pfname,
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lis

[vf-token 3/8] conf: Define PCI address vf-token extension

[Vivek Kashyap]

This patch introduces the PCI address extension flag for vf-token

Signed-off-by: Vivek Kashyap 
---
 src/conf/domain_addr.h | 1 +
 src/qemu/qemu_domain_address.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
index e72fb48847..29e7257177 100644
--- a/src/conf/domain_addr.h
+++ b/src/conf/domain_addr.h
@@ -29,6 +29,7 @@
 typedef enum {
 VIR_PCI_ADDRESS_EXTENSION_NONE = 0, /* no extension */
 VIR_PCI_ADDRESS_EXTENSION_ZPCI = 1 << 0, /* zPCI support */
+VIR_PCI_ADDRESS_EXTENSION_VFTOKEN = 1 << 1, /* VF token support */
 } virPCIDeviceAddressExtensionFlags;
 
 typedef enum {
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 099778b2a8..3be5acbc9e 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -575,6 +575,9 @@ 
qemuDomainDeviceCalculatePCIAddressExtensionFlags(virQEMUCaps *qemuCaps,
 extFlags |= VIR_PCI_ADDRESS_EXTENSION_ZPCI;
 }
 
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VFIO_VFTOKEN))
+extFlags |= VIR_PCI_ADDRESS_EXTENSION_VFTOKEN;
+
 return extFlags;
 }
 
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 2/8] qemu: capabilities: Introduce QEMU_CAPS_VFIO_VFTOKEN

[Vivek Kashyap]

Introduce the vf-token qemu capability

Signed-off-by: Vivek Kashyap 
---
 src/qemu/qemu_capabilities.c | 3 +++
 src/qemu/qemu_capabilities.h | 1 +
 tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml  | 1 +
 tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml | 1 +
 tests/qemucapabilitiesdata/caps_8.2.0_x86_64.xml | 1 +
 5 files changed, 7 insertions(+)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 83119e871a..f4cacd48d0 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -698,6 +698,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
   /* 450 */
   "run-with.async-teardown", /* QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN 
*/
   "virtio-blk-vhost-vdpa", /* 
QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA */
+  "vf-token", /* QEMU_CAPS_VFIO_VFTOKEN */
 );
 
 
@@ -1385,6 +1386,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
 { "virtio-crypto-device", QEMU_CAPS_DEVICE_VIRTIO_CRYPTO },
 { "cryptodev-backend-lkcf", QEMU_CAPS_OBJECT_CRYPTO_LKCF },
 { "pvpanic-pci", QEMU_CAPS_DEVICE_PANIC_PCI },
+{ "vf-token", QEMU_CAPS_VFIO_VFTOKEN },
 };
 
 
@@ -1447,6 +1449,7 @@ static struct virQEMUCapsDevicePropsFlags 
virQEMUCapsDevicePropsVirtioSCSI[] = {
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVfioPCI[] = {
+{ "vf-token", QEMU_CAPS_VFIO_VFTOKEN, NULL },
 };
 
 static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsSCSIDisk[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 3c4f7f625b..f97b1c9fd5 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -677,6 +677,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for 
syntax-check */
 /* 450 */
 QEMU_CAPS_RUN_WITH_ASYNC_TEARDOWN, /* asynchronous teardown -run-with 
async-teardown=on|off */
 QEMU_CAPS_DEVICE_VIRTIO_BLK_VHOST_VDPA, /* virtio-blk-vhost-vdpa block 
driver */
+QEMU_CAPS_VFIO_VFTOKEN, /* vf-token support */
 
 QEMU_CAPS_LAST /* this must always be the last item */
 } virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml 
b/tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml
index 427ee9d5c7..f4a65a133f 100644
--- a/tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml
+++ b/tests/qemucapabilitiesdata/caps_8.1.0_s390x.xml
@@ -112,6 +112,7 @@
   
   
   
+  
   850
   39100245
   v8.0.0-1270-g1c12355b
diff --git a/tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml 
b/tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml
index d266dd0f31..202a2c7f8d 100644
--- a/tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_8.1.0_x86_64.xml
@@ -198,6 +198,7 @@
   
   
   
+  
   8001000
   43100245
   v8.1.0
diff --git a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.xml 
b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.xml
index ef3bd14597..24809ab70f 100644
--- a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.xml
@@ -199,6 +199,7 @@
   
   
   
+  
   8001050
   43100246
   v8.1.0-3111-gad6ef0a42e
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 0/8] Introduce vf-token when using userspace PF

[Vivek Kashyap]

The VFIO PCI ABI has been extended to require userspace PF driver to set 
a VF token to a known value. The VF drivers are then required to provide
this token to access the VF device. The vf-token is set by the PF driver
before VF drivers can access the device. The kernel provides no means to
retrieve the token in use; but there is no specification describing the 
distribution or level of confidentiality of the token.  Qemu has been 
extended to require the vf-token when vf device is used. An important 
point to note is that the vf-token is required only when both the PF and
VF are used in userspace. 

This patch series adds support to provide the vf-token (uuid format) in the
domain XML and to generate the qemu commandline including the vf-token.

To support vf-token the new element will be used as follows:
   
  
  



  
 


The generated commandline will include the following:

-device {"driver":"vfio-pci","host":":00:0.1",
 "vf-token":"00112233-4455-6677-8899-aabbccddeeff",
 "id":"hostdev0","bus":"pci.0","addr":"0x1"}

Changes since initial RFC:
1. Added documentation
2. Added test cases and ran successful test suite after each patch commit
3. fixed spaces, coding sytle, and uuid string format 
4. Used S:vftoken in virJSONValueObjectAdd instead of a conditional

Vivek Kashyap (8):
  Define the vf-token extension for PCI device
  Introduce the vf-token qemu capability
  This patch introduces the PCI address extension flag for vf-token
  This patch introduces new XML parser/formatter functions for parsing
the vf-token
  Introduce a validation function for vf-token support in qemu and
generate vf-token device attribute in qemu command line
  Provide information about the vf-token flag
  Add tests for the vf-token flag to the qemuxml2argv and qemuxml2xml
test suites
  Update news about vf-token

 NEWS.rst  |  8 +++
 docs/formatdomain.rst |  3 ++
 src/conf/device_conf.c| 49 ---
 src/conf/domain_addr.h|  1 +
 src/conf/domain_conf.c|  8 +++
 src/conf/schemas/basictypes.rng   |  7 +++
 src/libvirt_private.syms  |  1 +
 src/qemu/qemu_capabilities.c  |  3 ++
 src/qemu/qemu_capabilities.h  |  1 +
 src/qemu/qemu_command.c   |  8 +++
 src/qemu/qemu_domain_address.c|  3 ++
 src/qemu/qemu_validate.c  | 20 
 src/util/virpci.c |  7 +++
 src/util/virpci.h | 10 
 .../qemucapabilitiesdata/caps_8.1.0_s390x.xml |  1 +
 .../caps_8.1.0_x86_64.xml |  1 +
 .../caps_8.2.0_x86_64.xml |  1 +
 .../hostdev-vfio-vf-token.x86_64-latest.args  | 34 +
 .../hostdev-vfio-vf-token.xml | 22 +
 tests/qemuxml2argvtest.c  |  1 +
 .../hostdev-vfio-vf-token.x86_64-latest.xml   | 40 +++
 tests/qemuxml2xmltest.c   |  1 +
 22 files changed, 223 insertions(+), 7 deletions(-)
 create mode 100644 
tests/qemuxml2argvdata/hostdev-vfio-vf-token.x86_64-latest.args
 create mode 100644 tests/qemuxml2argvdata/hostdev-vfio-vf-token.xml
 create mode 100644 
tests/qemuxml2xmloutdata/hostdev-vfio-vf-token.x86_64-latest.xml

-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[vf-token 1/8] virpci: Define the vf-token extension for PCI device

[Vivek Kashyap]

Define the vf-token extension for PCI device

Signed-off-by: Vivek Kashyap 
---
 src/util/virpci.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/util/virpci.h b/src/util/virpci.h
index bc7cb2329f..da32c2f4d2 100644
--- a/src/util/virpci.h
+++ b/src/util/virpci.h
@@ -50,6 +50,13 @@ struct _virZPCIDeviceAddress {
 /* Don't forget to update virPCIDeviceAddressCopy if needed. */
 };
 
+typedef struct _virPCIDeviceToken virPCIDeviceToken;
+
+struct _virPCIDeviceToken {
+unsigned char uuid[VIR_UUID_BUFLEN];
+bool isSet;
+};
+
 struct _virPCIDeviceAddress {
 unsigned int domain;
 unsigned int bus;
@@ -58,6 +65,7 @@ struct _virPCIDeviceAddress {
 virTristateSwitch multi;
 int extFlags; /* enum virPCIDeviceAddressExtensionFlags */
 virZPCIDeviceAddress zpci;
+virPCIDeviceToken token;
 /* Don't forget to update virPCIDeviceAddressCopy if needed. */
 };
 
-- 
2.33.8
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

[Denis V. Lunev]

On 12/18/23 13:23, Daniel P. Berrangé wrote:

The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.

It releases the client lock while running:

virNetClientUnlock()
g_main_loop_run()
virNetClientLock()

If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.

This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.

This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.

Signed-off-by: Daniel P. Berrangé 
---

  src/rpc/virnetclient.c | 13 -
  1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient 
*client,
  }
  
  
+static gboolean virNetClientIOWakeup(gpointer opaque)

+{
+GMainLoop *loop = opaque;
+
+g_main_loop_quit(loop);
+
+return G_SOURCE_REMOVE;
+}
+
  /*
   * This function sends a message to remote server and awaits a reply
   *
@@ -1925,7 +1934,9 @@ static int virNetClientIO(virNetClient *client,
  /* Check to see if another thread is dispatching */
  if (client->haveTheBuck) {
  /* Force other thread to wakeup from poll */
-g_main_loop_quit(client->eventLoop);
+GSource *wakeup = g_idle_source_new();
+g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop, 
NULL);
+g_source_attach(wakeup, client->eventCtx);
  
  /* If we are non-blocking, detach the thread and keep the call in the

   * queue. */

Reviewed-by: Denis V. Lunev 
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 2/2] qemuxml2argvmock: Poison virCommandRun and virFork from test context

[Michal Prívozník]

On 12/18/23 15:26, Peter Krempa wrote:
> On Mon, Dec 18, 2023 at 15:17:01 +0100, Michal Prívozník wrote:
>> On 12/18/23 12:02, Peter Krempa wrote:
>>> We don't want to ever run an actuall command in qemuxml2argvtest poison
>>> the helper functions we have for running commands.
>>>
>>> Signed-off-by: Peter Krempa 
>>> ---
>>>  src/util/vircommand.h|  4 ++--
>>>  tests/qemuxml2argvmock.c | 19 +++
>>>  2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
>>> index 9bcdce35b9..566ac15947 100644
>>> --- a/src/util/vircommand.h
>>> +++ b/src/util/vircommand.h
>>> @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand;
>>>   * call any function that is not async-signal-safe.  */
>>>  typedef int (*virExecHook)(void *data);
>>>
>>> -pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT;
>>> +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
>>>
>>>  virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
>>>
>>> @@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args);
>>>  int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) 
>>> G_GNUC_WARN_UNUSED_RESULT;
>>>
>>>  int virCommandRun(virCommand *cmd,
>>> -  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT;
>>> +  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
>>>
>>>  int virCommandRunAsync(virCommand *cmd,
>>> pid_t *pid) G_GNUC_WARN_UNUSED_RESULT;
>>> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
>>> index 2deccd79c4..0ee8fbba79 100644
>>> --- a/tests/qemuxml2argvmock.c
>>> +++ b/tests/qemuxml2argvmock.c
>>> @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver 
>>> G_GNUC_UNUSED,
>>>  {
>>>  return 0;
>>>  }
>>> +
>>> +
>>> +int
>>> +virCommandRun(virCommand *cmd,
>>> +  int *exitstatus G_GNUC_UNUSED)
>>> +{
>>> +const char *path = virCommandGetBinaryPath(cmd);
>>> +
>>> +fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test 
>>> context\n", NULLSTR(path));
>>> +return -1;
>>> +}
>>> +
>>> +
>>> +pid_t
>>> +virFork(void)
>>> +{
>>> +fprintf(stderr, "\nattempted virFork() in test context\n");
>>> +return -1;
>>> +}
>>
>> Any reason to mock both? Or is it just to provide nicer error message?
> 
> Reasonable error message is reported already by virCommandRun. I just
> wanted to make sure that the test doesn't try to even fork for any other
> reason, thus I've picked our other helper to prevent that too (IIRC
> plain fork() is forbidden by syntax-check).

Yeah, we have a syntax-check to enforce virFork(), so mocking just
virFork() should be fine. In the end, virCommandRun() does call
virFork(). I don't mind and don't want to bikeshed. Feel free to push
your patch as is. I was just curious.

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

[Efim Shevrin]

Hello,

Your patch works on our stand without any freezing.

Tested by: Fima Shevrin 

From: Daniel P. Berrangé 
Sent: Monday, December 18, 2023 20:23
To: devel@lists.libvirt.org 
Cc: Efim Shevrin ; Denis V. Lunev 
; Daniel P. Berrangé 
Subject: [PATCH] rpc: fix race in waking up client event loop

[You don't often get email from berra...@redhat.com. Learn why this is 
important at https://aka.ms/LearnAboutSenderIdentification ]

The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.

It releases the client lock while running:

   virNetClientUnlock()
   g_main_loop_run()
   virNetClientLock()

If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.

This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.

This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.

Signed-off-by: Daniel P. Berrangé 
---

 src/rpc/virnetclient.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient 
*client,
 }


+static gboolean virNetClientIOWakeup(gpointer opaque)
+{
+GMainLoop *loop = opaque;
+
+g_main_loop_quit(loop);
+
+return G_SOURCE_REMOVE;
+}
+
 /*
  * This function sends a message to remote server and awaits a reply
  *
@@ -1925,7 +1934,9 @@ static int virNetClientIO(virNetClient *client,
 /* Check to see if another thread is dispatching */
 if (client->haveTheBuck) {
 /* Force other thread to wakeup from poll */
-g_main_loop_quit(client->eventLoop);
+GSource *wakeup = g_idle_source_new();
+g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop, 
NULL);
+g_source_attach(wakeup, client->eventCtx);

 /* If we are non-blocking, detach the thread and keep the call in the
  * queue. */
--
2.43.0

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH] rpc: fix race in waking up client event loop

[Michal Prívozník]

On 12/18/23 13:23, Daniel P. Berrangé wrote:
> The first thread to issue a client RPC request will own the event
> loop execution, sitting in the virNetClientIOEventLoop function.
> 
> It releases the client lock while running:
> 
>virNetClientUnlock()
>g_main_loop_run()
>virNetClientLock()
> 
> If a second thread arrives with an RPC request, it will queue it
> for the first thread to process. To inform the first thread that
> there's a new request it calls g_main_loop_quit() to break it out
> of the main loop.
> 
> This works if the first thread is in g_main_loop_run() at that
> time. There is a small window of opportunity, however, where
> the first thread has released the client lock, but not yet got
> into g_main_loop_run(). If that happens, the wakeup from the
> second thread is lost.
> 
> This patch deals with that by changing the way the wakeup is
> performed. Instead of directly calling g_main_loop_quit(), the
> second thread creates an idle source to run the quit function
> from within the first thread. This guarantees that the first
> thread will see the wakeup.
> 
> Signed-off-by: Daniel P. Berrangé 
> ---
> 
>  src/rpc/virnetclient.c | 13 -
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

Reviewed-by: Michal Privoznik 

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 2/2] qemuxml2argvmock: Poison virCommandRun and virFork from test context

[Peter Krempa]

On Mon, Dec 18, 2023 at 15:17:01 +0100, Michal Prívozník wrote:
> On 12/18/23 12:02, Peter Krempa wrote:
> > We don't want to ever run an actuall command in qemuxml2argvtest poison
> > the helper functions we have for running commands.
> > 
> > Signed-off-by: Peter Krempa 
> > ---
> >  src/util/vircommand.h|  4 ++--
> >  tests/qemuxml2argvmock.c | 19 +++
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> > index 9bcdce35b9..566ac15947 100644
> > --- a/src/util/vircommand.h
> > +++ b/src/util/vircommand.h
> > @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand;
> >   * call any function that is not async-signal-safe.  */
> >  typedef int (*virExecHook)(void *data);
> > 
> > -pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT;
> > +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
> > 
> >  virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
> > 
> > @@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args);
> >  int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) 
> > G_GNUC_WARN_UNUSED_RESULT;
> > 
> >  int virCommandRun(virCommand *cmd,
> > -  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT;
> > +  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
> > 
> >  int virCommandRunAsync(virCommand *cmd,
> > pid_t *pid) G_GNUC_WARN_UNUSED_RESULT;
> > diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> > index 2deccd79c4..0ee8fbba79 100644
> > --- a/tests/qemuxml2argvmock.c
> > +++ b/tests/qemuxml2argvmock.c
> > @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver 
> > G_GNUC_UNUSED,
> >  {
> >  return 0;
> >  }
> > +
> > +
> > +int
> > +virCommandRun(virCommand *cmd,
> > +  int *exitstatus G_GNUC_UNUSED)
> > +{
> > +const char *path = virCommandGetBinaryPath(cmd);
> > +
> > +fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test 
> > context\n", NULLSTR(path));
> > +return -1;
> > +}
> > +
> > +
> > +pid_t
> > +virFork(void)
> > +{
> > +fprintf(stderr, "\nattempted virFork() in test context\n");
> > +return -1;
> > +}
> 
> Any reason to mock both? Or is it just to provide nicer error message?

Reasonable error message is reported already by virCommandRun. I just
wanted to make sure that the test doesn't try to even fork for any other
reason, thus I've picked our other helper to prevent that too (IIRC
plain fork() is forbidden by syntax-check).

For catching the mistake fixed in 1/2 only the mock of virCommandRun is
necessary.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 0/2] qemuxml2argvtest: Don't exec '/usr/libexec/qemu/vhost-user/test-vhost-user-gpu'

[Michal Prívozník]

On 12/18/23 12:02, Peter Krempa wrote:
> 'qemuExtVhostUserGPUPrepareDomain' breaks our design assumptions about
> the 'PrepareDomain' step which should not touch anything on the host.
> 
> This patchset for now fixes the symptom by mocking the function and
> poisons virFork and virCommandRun so that this doesn't happen in the
> future.
> 
> Proper fix will require splitting the vhost-user GPU prepare step to
> prepare the host-specific portion separately.
> 
> Peter Krempa (2):
>   qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain
>   qemuxml2argvmock: Poison virCommandRun and virFork from test context
> 
>  src/qemu/qemu_vhost_user_gpu.c |  4 
>  src/qemu/qemu_vhost_user_gpu.h |  2 +-
>  src/util/vircommand.h  |  4 ++--
>  tests/qemuxml2argvmock.c   | 28 
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 

Reviewed-by: Michal Privoznik 

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 2/2] qemuxml2argvmock: Poison virCommandRun and virFork from test context

[Michal Prívozník]

On 12/18/23 12:02, Peter Krempa wrote:
> We don't want to ever run an actuall command in qemuxml2argvtest poison
> the helper functions we have for running commands.
> 
> Signed-off-by: Peter Krempa 
> ---
>  src/util/vircommand.h|  4 ++--
>  tests/qemuxml2argvmock.c | 19 +++
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/vircommand.h b/src/util/vircommand.h
> index 9bcdce35b9..566ac15947 100644
> --- a/src/util/vircommand.h
> +++ b/src/util/vircommand.h
> @@ -39,7 +39,7 @@ typedef struct _virCommand virCommand;
>   * call any function that is not async-signal-safe.  */
>  typedef int (*virExecHook)(void *data);
> 
> -pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT;
> +pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
> 
>  virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);
> 
> @@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args);
>  int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) 
> G_GNUC_WARN_UNUSED_RESULT;
> 
>  int virCommandRun(virCommand *cmd,
> -  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT;
> +  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;
> 
>  int virCommandRunAsync(virCommand *cmd,
> pid_t *pid) G_GNUC_WARN_UNUSED_RESULT;
> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 2deccd79c4..0ee8fbba79 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver 
> G_GNUC_UNUSED,
>  {
>  return 0;
>  }
> +
> +
> +int
> +virCommandRun(virCommand *cmd,
> +  int *exitstatus G_GNUC_UNUSED)
> +{
> +const char *path = virCommandGetBinaryPath(cmd);
> +
> +fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test 
> context\n", NULLSTR(path));
> +return -1;
> +}
> +
> +
> +pid_t
> +virFork(void)
> +{
> +fprintf(stderr, "\nattempted virFork() in test context\n");
> +return -1;
> +}

Any reason to mock both? Or is it just to provide nicer error message?

Michal
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: Patches on libvirt patchew are outdated

[Paolo Bonzini]

On Mon, Dec 18, 2023 at 2:14 PM Peter Krempa  wrote:
> The patches are now picked up, but I've noticed that all series' are
> marked as "does not apply to master".
>
> Unfortunately the new mailing list (mailman3 + hyperkitty) has a bad
> habit of touching all the patches. Very specifically it recodes and
> quotes CRLF, which makes it impossible to apply via 'git am' in default
> configuration and requires using 'git am --quoted-cr=strip'. This
> applies both in mail and also in archives.

No, that was just a typo when switching away from git:// as the
protocol used to fetch from the upstream repo. If you see a series
where Mailman3's re-encoding is an issue please let me know, but I
have reset most series now and they applied fine.


Paolo
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: Patches on libvirt patchew are outdated

[Peter Krempa]

On Mon, Dec 18, 2023 at 10:27:54 +0100, Paolo Bonzini wrote:
> On Mon, Dec 18, 2023 at 9:33 AM Paolo Bonzini  wrote:
> >
> > On Mon, Dec 18, 2023 at 9:32 AM Peter Krempa  wrote:
> > >
> > > On Mon, Dec 18, 2023 at 14:42:57 +0800, Han Han wrote:
> > > > Hi,
> > > > The libvirt patchew https://patchew.org/Libvirt/ has no update for more
> > > > than one month. Is it dropped? If not, please sync the patches to it~
> > >
> > > The problem is likely caused by the switch to the new mailing list. The
> > > patch importer of patchew wasn't probably configured to use the new
> > > list.
> > >
> > > Since the patchew project doesn't seem to contain the actual
> > > configuration for the importer that is used on patchew.org we'll need to
> > > contact somebody to fix it.
> > >
> > > Paolo, Daniel, do you know who can fix this?
> >
> > Either I or Fam can fix it. We just need to subscribe to the new mailing 
> > list.
> 
> On top of that, the last month of patches was delivered, but not
> processed because Patchew did not recognize devel@lists.libvirt.org as
> a message for the Libvirt project. This will also sort itself out in
> an hour or so.

The patches are now picked up, but I've noticed that all series' are
marked as "does not apply to master".

Unfortunately the new mailing list (mailman3 + hyperkitty) has a bad
habit of touching all the patches. Very specifically it recodes and
quotes CRLF, which makes it impossible to apply via 'git am' in default
configuration and requires using 'git am --quoted-cr=strip'. This
applies both in mail and also in archives.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH] rpc: fix race in waking up client event loop

[Daniel P . Berrangé]

The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.

It releases the client lock while running:

   virNetClientUnlock()
   g_main_loop_run()
   virNetClientLock()

If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.

This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.

This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.

Signed-off-by: Daniel P. Berrangé 
---

 src/rpc/virnetclient.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 4ab8af68c5..68098b1c8d 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1848,6 +1848,15 @@ static void virNetClientIOUpdateCallback(virNetClient 
*client,
 }
 
 
+static gboolean virNetClientIOWakeup(gpointer opaque)
+{
+GMainLoop *loop = opaque;
+
+g_main_loop_quit(loop);
+
+return G_SOURCE_REMOVE;
+}
+
 /*
  * This function sends a message to remote server and awaits a reply
  *
@@ -1925,7 +1934,9 @@ static int virNetClientIO(virNetClient *client,
 /* Check to see if another thread is dispatching */
 if (client->haveTheBuck) {
 /* Force other thread to wakeup from poll */
-g_main_loop_quit(client->eventLoop);
+GSource *wakeup = g_idle_source_new();
+g_source_set_callback(wakeup, virNetClientIOWakeup, client->eventLoop, 
NULL);
+g_source_attach(wakeup, client->eventCtx);
 
 /* If we are non-blocking, detach the thread and keep the call in the
  * queue. */
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 1/3] rpc: create virNetClientIOWakeup helper

[Denis V. Lunev]

This functionality is to be extended, simple call to g_main_loop_quit()
is not enough. In order to make changes convinient, the helper is
required.

Signed-off-by: Denis V. Lunev 
---
 src/rpc/virnetclient.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index d29917df27..d9a508246f 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -150,6 +150,13 @@ void virNetClientSetCloseCallback(virNetClient *client,
 }
 
 
+static void
+virNetClientIOWakeup(virNetClient *client)
+{
+g_main_loop_quit(client->eventLoop);
+}
+
+
 static void virNetClientIncomingEvent(virNetSocket *sock,
   int events,
   void *opaque);
@@ -851,7 +858,7 @@ static void virNetClientCloseInternal(virNetClient *client,
  * queue and close the client because we set client->wantClose.
  */
 if (client->haveTheBuck) {
-g_main_loop_quit(client->eventLoop);
+virNetClientIOWakeup(client);
 } else {
 virNetClientIOEventLoopPassTheBuck(client, NULL);
 }
@@ -918,7 +925,7 @@ virNetClientIOEventTLS(int fd G_GNUC_UNUSED,
 virNetClient *client = opaque;
 
 if (!virNetClientTLSHandshake(client))
-g_main_loop_quit(client->eventLoop);
+virNetClientIOWakeup(client);
 
 return G_SOURCE_REMOVE;
 }
@@ -931,7 +938,7 @@ virNetClientIOEventTLSConfirm(int fd G_GNUC_UNUSED,
 {
 virNetClient *client = opaque;
 
-g_main_loop_quit(client->eventLoop);
+virNetClientIOWakeup(client);
 
 return G_SOURCE_REMOVE;
 }
@@ -1675,7 +1682,7 @@ virNetClientIOEventFD(int fd G_GNUC_UNUSED,
 {
 struct virNetClientIOEventData *data = opaque;
 data->rev = ev;
-g_main_loop_quit(data->client->eventLoop);
+virNetClientIOWakeup(data->client);
 return G_SOURCE_REMOVE;
 }
 
@@ -2006,8 +2013,7 @@ static int virNetClientIO(virNetClient *client,
 
 /* Check to see if another thread is dispatching */
 if (client->haveTheBuck) {
-/* Force other thread to wakeup from poll */
-g_main_loop_quit(client->eventLoop);
+virNetClientIOWakeup(client);
 
 /* If we are non-blocking, detach the thread and keep the call in the
  * queue. */
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 2/3] rpc: mark GSourceFuncs functions in vireventglibwatch.c as static

[Denis V. Lunev]

They are not exported from the module and thus should be static.

Signed-off-by: Denis V. Lunev 
---
 src/util/vireventglibwatch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
index b7f3a8786a..b21e505731 100644
--- a/src/util/vireventglibwatch.c
+++ b/src/util/vireventglibwatch.c
@@ -71,7 +71,7 @@ virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED)
 }
 
 
-GSourceFuncs virEventGLibFDSourceFuncs = {
+static GSourceFuncs virEventGLibFDSourceFuncs = {
 .prepare = virEventGLibFDSourcePrepare,
 .check = virEventGLibFDSourceCheck,
 .dispatch = virEventGLibFDSourceDispatch,
@@ -194,7 +194,7 @@ virEventGLibSocketSourceFinalize(GSource *source)
 }
 
 
-GSourceFuncs virEventGLibSocketSourceFuncs = {
+static GSourceFuncs virEventGLibSocketSourceFuncs = {
 .prepare = virEventGLibSocketSourcePrepare,
 .check = virEventGLibSocketSourceCheck,
 .dispatch = virEventGLibSocketSourceDispatch,
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 3/3] rpc: handle notifications properly

[Denis V. Lunev]

RPC client implementation uses the following paradigm. The critical
section is organized via virObjectLock(client)/virObjectUnlock(client)
braces. Though this is potentially problematic as
main thread:side thread:
virObjectUnlock(client);
virObjectLock(client);
g_main_loop_quit(client->eventLoop);
virObjectUnlock(client);
g_main_loop_run(client->eventLoop);

This means in particular that is the main thread is executing very long
request like VM migration, the wakeup from the side thread could be
stuck until the main request will be fully completed.

For us this means that Nova was reported as inaccessible. Anyway, this
case is easily reproducible with the simple python scripts doing slow
and fast requests in parallel from two different threads.

This problem has been introduced with the rework for dropping gnulib
inside the following commit:

commit 7d4350bcac251bab2ecf85bd19eb1181db87fd07
Author: Daniel P. Berrangé 
Date:   Thu Jan 16 11:21:44 2020 +
rpc: convert RPC client to use GMainLoop instead of poll

The cure is to revert back to old roots and use file descriptor for
wakeup notifications. The code written is well suited for Linux while it
is completely unclear how it will behave on Windows.

Signed-off-by: Denis V. Lunev 
---
 src/rpc/virnetclient.c | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index d9a508246f..7fff5a7017 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -90,6 +90,7 @@ struct _virNetClient {
 
 GMainLoop *eventLoop;
 GMainContext *eventCtx;
+int pipeLoopNotify[2];
 
 /*
  * List of calls currently waiting for dispatch
@@ -150,10 +151,25 @@ void virNetClientSetCloseCallback(virNetClient *client,
 }
 
 
+static gboolean
+virNetClientIOEventNotify(int fd G_GNUC_UNUSED,
+  GIOCondition ev G_GNUC_UNUSED,
+  gpointer opaque)
+{
+virNetClient *client = opaque;
+char buf[1024];
+
+while (saferead(client->pipeLoopNotify[0], buf, sizeof(buf)) > 0);
+g_main_loop_quit(client->eventLoop);
+
+return G_SOURCE_CONTINUE;
+}
+
 static void
 virNetClientIOWakeup(virNetClient *client)
 {
-g_main_loop_quit(client->eventLoop);
+char c = 0;
+ignore_value(safewrite(client->pipeLoopNotify[1], &c, sizeof(c)));
 }
 
 
@@ -305,10 +321,15 @@ static virNetClient *virNetClientNew(virNetSocket *sock,
const char *hostname)
 {
 virNetClient *client = NULL;
+int pipenotify[2] = {-1, -1};
+g_autoptr(GSource) G_GNUC_UNUSED source = NULL;
 
 if (virNetClientInitialize() < 0)
 goto error;
 
+if (virPipeNonBlock(pipenotify) < 0)
+goto error;
+
 if (!(client = virObjectLockableNew(virNetClientClass)))
 goto error;
 
@@ -320,12 +341,25 @@ static virNetClient *virNetClientNew(virNetSocket *sock,
 
 client->hostname = g_strdup(hostname);
 
+client->pipeLoopNotify[0] = pipenotify[0];
+client->pipeLoopNotify[1] = pipenotify[1];
+
+/* FIXME: good for Unix, buggy for Windows */
+source = virEventGLibAddSocketWatch(pipenotify[0],
+G_IO_IN | G_IO_ERR | G_IO_HUP,
+client->eventCtx,
+virNetClientIOEventNotify,
+client, NULL);
+
 PROBE(RPC_CLIENT_NEW,
   "client=%p sock=%p",
   client, client->sock);
 return client;
 
  error:
+VIR_FORCE_CLOSE(pipenotify[0]);
+VIR_FORCE_CLOSE(pipenotify[1]);
+
 virObjectUnref(client);
 virObjectUnref(sock);
 return NULL;
@@ -759,6 +793,9 @@ void virNetClientDispose(void *obj)
 g_main_loop_unref(client->eventLoop);
 g_main_context_unref(client->eventCtx);
 
+VIR_FORCE_CLOSE(client->pipeLoopNotify[0]);
+VIR_FORCE_CLOSE(client->pipeLoopNotify[1]);
+
 g_free(client->hostname);
 
 if (client->sock)
-- 
2.34.1
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH v-pipe 0/3] alternative approach to the fix

[Denis V. Lunev]

This is a dirty working code using pipes.

Sent for discussion of the approach. Made against our downstream
based on libvirt 8.5.

Signed-off-by: Denis V. Lunev 

___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 3/3] rpc: Rework rpc notifications in main and side thread

[Denis V. Lunev]

On 12/18/23 12:03, Daniel P. Berrangé wrote:

On Mon, Dec 18, 2023 at 10:32:07AM +, Daniel P. Berrangé wrote:

On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:

RPC client implementation uses the following paradigm. The critical
section is organized via virObjectLock(client)/virObjectUnlock(client)
braces. Though this is potentially problematic as
 main thread:side thread:
 virObjectUnlock(client);
 virObjectLock(client);
 g_main_loop_quit(client->eventLoop);
 virObjectUnlock(client);
 g_main_loop_run(client->eventLoop);

This means in particular that is the main thread is executing very long
request like VM migration, the wakeup from the side thread could be
stuck until the main request will be fully completed.

Ah, I understand this now. The flag set from g_main_loop_quit() is
ignored and overwritten by g_main_loop_run(). So the interruption
from the side thread never happens.

Your approach to solving this is by delaying the virObjectUnlock
call until g_main_loop_run is running, which is clever but I don't
much like playing games with the mutex locking.

We need a way to interrupt the main loop, even if it hasn't started
running yet. The previous impl in libvirt used a pipe for this trick,
effectively as a dummy event source that would be watched.

I think we can do the same here, though without needing an actual
pipe which causes Windows portability problems.

Glib already has an internal mechansim for breaking out of poll(),
via its (private) GWakeup object which encapsulates a pipe. We just
need a way of triggering this mechanism.

I believe this can be achieved using just an idle source ie

static gboolean
dummy_cb(void *opaque)
{

Opps, would still need a  g_main_loop_quit() call here


I have sent dirty (but working) series with a pipe
approach to come to the decision which approach
would be better - with prepare callback or
via pipes.

I am not against any of them :)

We need just to come to the decision which one would
be better.

Thank you in advance,
    Den
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 3/3] rpc: Rework rpc notifications in main and side thread

[Daniel P . Berrangé]

On Mon, Dec 18, 2023 at 10:32:07AM +, Daniel P. Berrangé wrote:
> On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
> > RPC client implementation uses the following paradigm. The critical
> > section is organized via virObjectLock(client)/virObjectUnlock(client)
> > braces. Though this is potentially problematic as
> > main thread:side thread:
> > virObjectUnlock(client);
> > virObjectLock(client);
> > g_main_loop_quit(client->eventLoop);
> > virObjectUnlock(client);
> > g_main_loop_run(client->eventLoop);
> > 
> > This means in particular that is the main thread is executing very long
> > request like VM migration, the wakeup from the side thread could be
> > stuck until the main request will be fully completed.
> 
> Ah, I understand this now. The flag set from g_main_loop_quit() is
> ignored and overwritten by g_main_loop_run(). So the interruption
> from the side thread never happens.
> 
> Your approach to solving this is by delaying the virObjectUnlock
> call until g_main_loop_run is running, which is clever but I don't
> much like playing games with the mutex locking.
> 
> We need a way to interrupt the main loop, even if it hasn't started
> running yet. The previous impl in libvirt used a pipe for this trick,
> effectively as a dummy event source that would be watched.
> 
> I think we can do the same here, though without needing an actual
> pipe which causes Windows portability problems.
> 
> Glib already has an internal mechansim for breaking out of poll(),
> via its (private) GWakeup object which encapsulates a pipe. We just
> need a way of triggering this mechanism.
> 
> I believe this can be achieved using just an idle source ie
> 
> static gboolean
> dummy_cb(void *opaque)
> {

Opps, would still need a  g_main_loop_quit() call here 

> return G_SOURCE_REMOVE;
> }
> 
> ...
> 
>GSource *dummy = g_idle_source_new()
>g_source_set_callback(dummy, dummy_cb, NULL, NULL);
>g_source_attach(dummy, client->eventCtx);
> 
> 
> The g_source_attach() method is what breaks the main loop out of
> its poll() sleep. If it wasn't currently in poll, it is effectively
> a no-op.

With regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 2/2] qemuxml2argvmock: Poison virCommandRun and virFork from test context

[Peter Krempa]

We don't want to ever run an actuall command in qemuxml2argvtest poison
the helper functions we have for running commands.

Signed-off-by: Peter Krempa 
---
 src/util/vircommand.h|  4 ++--
 tests/qemuxml2argvmock.c | 19 +++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index 9bcdce35b9..566ac15947 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -39,7 +39,7 @@ typedef struct _virCommand virCommand;
  * call any function that is not async-signal-safe.  */
 typedef int (*virExecHook)(void *data);

-pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT;
+pid_t virFork(void) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;

 virCommand *virCommandNew(const char *binary) ATTRIBUTE_NONNULL(1);

@@ -184,7 +184,7 @@ int virCommandGetArgList(virCommand *cmd, char ***args);
 int virCommandExec(virCommand *cmd, gid_t *groups, int ngroups) 
G_GNUC_WARN_UNUSED_RESULT;

 int virCommandRun(virCommand *cmd,
-  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT;
+  int *exitstatus) G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;

 int virCommandRunAsync(virCommand *cmd,
pid_t *pid) G_GNUC_WARN_UNUSED_RESULT;
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 2deccd79c4..0ee8fbba79 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -276,3 +276,22 @@ qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver 
G_GNUC_UNUSED,
 {
 return 0;
 }
+
+
+int
+virCommandRun(virCommand *cmd,
+  int *exitstatus G_GNUC_UNUSED)
+{
+const char *path = virCommandGetBinaryPath(cmd);
+
+fprintf(stderr, "\nattempted virCommandRun() (path=%s) from test 
context\n", NULLSTR(path));
+return -1;
+}
+
+
+pid_t
+virFork(void)
+{
+fprintf(stderr, "\nattempted virFork() in test context\n");
+return -1;
+}
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 1/2] qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain

[Peter Krempa]

Noticed when breaking on 'fork()' in qemuxml2argvtest (output slihtly
trimmed, of unimportant file locations:

  #0  0x775abf18 in fork () at /lib64/libc.so.6
  #1  0x77b88387 in virFork () at 
../../../libvirt/src/util/vircommand.c:282
  #2  0x77b8a7d2 in virExec (cmd=0x5740d440) at 
../../../libvirt/src/util/vircommand.c:741
  #3  virCommandRunAsync (cmd=cmd@entry=0x5740d440, pid=pid@entry=0x0) at 
../../../libvirt/src/util/vircommand.c:2658
  #4  0x77b8c04f in virCommandRun (cmd=cmd@entry=0x5740d440, 
exitstatus=exitstatus@entry=0x0)
  #5  0x779f6daa in qemuVhostUserFillDomainGPU 
(driver=driver@entry=0x55586120 , video=0x573e8d80)
  #6  0x779f73f5 in qemuExtVhostUserGPUPrepareDomain 
(driver=driver@entry=0x55586120 , video=)
  #7  0x7797c569 in qemuExtDevicesPrepareDomain 
(driver=driver@entry=0x55586120 , vm=vm@entry=0x573dfef0)
  #8  0x779d69ef in qemuProcessPrepareDomain
  #9  0x779dda36 in qemuProcessCreatePretendCmdPrepare
  #10 0x5556fa28 in testCompareXMLToArgvCreateArgs
  #11 testCompareXMLToArgv (data=0x573440f0) at 
../../../libvirt/tests/qemuxml2argvtest.c:733
  #12 0x55570f7a in virTestRun
  #13 0x55571201 in virTestRunLog
  (ret=0x7fffdb4c, title=0x5697c360 "QEMU XML-2-ARGV 
virtio-options.x86_64-latest", body=0x5556f820 , 
data=0x573440f0) at ../../../libvirt/tests/testutils.c:198
  #14 0xc1b9 in testRun

Code paths in 'qemuProcessPrepareDomain' should not invoke external
helpers. Note this in a comment and mock the function for now. It will
need a more complex refactor.

Signed-off-by: Peter Krempa 
---
 src/qemu/qemu_vhost_user_gpu.c | 4 
 src/qemu/qemu_vhost_user_gpu.h | 2 +-
 tests/qemuxml2argvmock.c   | 9 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_vhost_user_gpu.c b/src/qemu/qemu_vhost_user_gpu.c
index 933adfe8de..cca76526d1 100644
--- a/src/qemu/qemu_vhost_user_gpu.c
+++ b/src/qemu/qemu_vhost_user_gpu.c
@@ -78,6 +78,10 @@ qemuVhostUserGPUGetPid(const char *stateDir,
 }


+/** TODO: this is called from qemuProcessPrepareDomain which is NOT supposed to
+ * query the host in any way. This function is mocked in qemuxml2argvmock.so
+ * to prevent probing the host vgpu process capabilities.
+ */
 int qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver,
  virDomainVideoDef *video)
 {
diff --git a/src/qemu/qemu_vhost_user_gpu.h b/src/qemu/qemu_vhost_user_gpu.h
index 2b86982cb8..d19798d781 100644
--- a/src/qemu/qemu_vhost_user_gpu.h
+++ b/src/qemu/qemu_vhost_user_gpu.h
@@ -26,7 +26,7 @@
 int qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver,
  virDomainVideoDef *video)
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-G_GNUC_WARN_UNUSED_RESULT;
+G_GNUC_WARN_UNUSED_RESULT G_NO_INLINE;

 int qemuExtVhostUserGPUStart(virQEMUDriver *driver,
  virDomainObj *vm,
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index 52c44b2ed0..2deccd79c4 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -37,6 +37,7 @@
 #include "qemu/qemu_command.h"
 #include 
 #include 
+#include "qemu/qemu_vhost_user_gpu.h"

 #define VIR_FROM_THIS VIR_FROM_NONE

@@ -267,3 +268,11 @@ virIdentityEnsureSystemToken(void)
 {
 return g_strdup("3de80bcbf22d4833897f1638e01be9b2");
 }
+
+
+int
+qemuExtVhostUserGPUPrepareDomain(virQEMUDriver *driver G_GNUC_UNUSED,
+ virDomainVideoDef *video G_GNUC_UNUSED)
+{
+return 0;
+}
-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

[PATCH 0/2] qemuxml2argvtest: Don't exec '/usr/libexec/qemu/vhost-user/test-vhost-user-gpu'

[Peter Krempa]

'qemuExtVhostUserGPUPrepareDomain' breaks our design assumptions about
the 'PrepareDomain' step which should not touch anything on the host.

This patchset for now fixes the symptom by mocking the function and
poisons virFork and virCommandRun so that this doesn't happen in the
future.

Proper fix will require splitting the vhost-user GPU prepare step to
prepare the host-specific portion separately.

Peter Krempa (2):
  qemuxml2argvmock: Mock qemuExtVhostUserGPUPrepareDomain
  qemuxml2argvmock: Poison virCommandRun and virFork from test context

 src/qemu/qemu_vhost_user_gpu.c |  4 
 src/qemu/qemu_vhost_user_gpu.h |  2 +-
 src/util/vircommand.h  |  4 ++--
 tests/qemuxml2argvmock.c   | 28 
 4 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.43.0
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [PATCH 3/3] rpc: Rework rpc notifications in main and side thread

[Daniel P . Berrangé]

On Fri, Dec 15, 2023 at 02:32:19AM +0800, Fima Shevrin wrote:
> RPC client implementation uses the following paradigm. The critical
> section is organized via virObjectLock(client)/virObjectUnlock(client)
> braces. Though this is potentially problematic as
> main thread:side thread:
> virObjectUnlock(client);
> virObjectLock(client);
> g_main_loop_quit(client->eventLoop);
> virObjectUnlock(client);
> g_main_loop_run(client->eventLoop);
> 
> This means in particular that is the main thread is executing very long
> request like VM migration, the wakeup from the side thread could be
> stuck until the main request will be fully completed.

Ah, I understand this now. The flag set from g_main_loop_quit() is
ignored and overwritten by g_main_loop_run(). So the interruption
from the side thread never happens.

Your approach to solving this is by delaying the virObjectUnlock
call until g_main_loop_run is running, which is clever but I don't
much like playing games with the mutex locking.

We need a way to interrupt the main loop, even if it hasn't started
running yet. The previous impl in libvirt used a pipe for this trick,
effectively as a dummy event source that would be watched.

I think we can do the same here, though without needing an actual
pipe which causes Windows portability problems.

Glib already has an internal mechansim for breaking out of poll(),
via its (private) GWakeup object which encapsulates a pipe. We just
need a way of triggering this mechanism.

I believe this can be achieved using just an idle source ie

static gboolean
dummy_cb(void *opaque)
{
return G_SOURCE_REMOVE;
}

...

   GSource *dummy = g_idle_source_new()
   g_source_set_callback(dummy, dummy_cb, NULL, NULL);
   g_source_attach(dummy, client->eventCtx);


The g_source_attach() method is what breaks the main loop out of
its poll() sleep. If it wasn't currently in poll, it is effectively
a no-op.

> 
> Discrubed case is easily reproducible with the simple python scripts doing 
> slow
> and fast requests in parallel from two different threads.
> 
> Our idea is to release the lock at the prepare stage and avoid libvirt stuck
> during the interaction between main and side threads.
> 
> Co-authored-by: Denis V. Lunev 
> Co-authored-by: Nikolai Barybin 
> 
> Signed-off-by: Fima Shevrin 
> ---
>  src/rpc/virnetclient.c   | 17 -
>  src/util/vireventglibwatch.c | 28 ++--
>  2 files changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index de8ebc2da9..63bd42ed3a 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -987,6 +987,9 @@ int virNetClientSetTLSSession(virNetClient *client,
>   * etc.  If we make the grade, it will send us a '\1' byte.
>   */
>  
> +/* Here we are not passing the client to virEventGLibAddSocketWatch,
> + * since the entire virNetClientSetTLSSession function requires a lock.
> + */
>  source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
>  G_IO_IN,
>  client->eventCtx,
> @@ -1692,14 +1695,18 @@ static int virNetClientIOEventLoop(virNetClient 
> *client,
>  if (client->nstreams)
>  ev |= G_IO_IN;
>  
> +/*
> + * We don't need to call virObjectLock(client) here,
> + * since the .prepare function inside glib Main Loop
> + * will do this. virEventGLibAddSocketWatch is responsible
> + * for passing client var in glib .prepare
> + */
>  source = virEventGLibAddSocketWatch(virNetSocketGetFD(client->sock),
>  ev,
>  client->eventCtx,
> -virNetClientIOEventFD, &data, 
> NULL, NULL);
> -
> -/* Release lock while poll'ing so other threads
> - * can stuff themselves on the queue */
> -virObjectUnlock(client);
> +virNetClientIOEventFD, &data,
> +(virObjectLockable *)client,
> +NULL);
>  
>  #ifndef WIN32
>  /* Block SIGWINCH from interrupting poll in curses programs,
> diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> index 7680656ba2..641b772995 100644
> --- a/src/util/vireventglibwatch.c
> +++ b/src/util/vireventglibwatch.c
> @@ -34,11 +34,23 @@ struct virEventGLibFDSource {
>  
>  
>  static gboolean
> -virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED,
> +virEventGLibFDSourcePrepare(GSource *source,
>  gint *timeout)
>  {
> +virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
>  *tim

Re: Patches on libvirt patchew are outdated

[Paolo Bonzini]

On Mon, Dec 18, 2023 at 9:33 AM Paolo Bonzini  wrote:
>
> On Mon, Dec 18, 2023 at 9:32 AM Peter Krempa  wrote:
> >
> > On Mon, Dec 18, 2023 at 14:42:57 +0800, Han Han wrote:
> > > Hi,
> > > The libvirt patchew https://patchew.org/Libvirt/ has no update for more
> > > than one month. Is it dropped? If not, please sync the patches to it~
> >
> > The problem is likely caused by the switch to the new mailing list. The
> > patch importer of patchew wasn't probably configured to use the new
> > list.
> >
> > Since the patchew project doesn't seem to contain the actual
> > configuration for the importer that is used on patchew.org we'll need to
> > contact somebody to fix it.
> >
> > Paolo, Daniel, do you know who can fix this?
>
> Either I or Fam can fix it. We just need to subscribe to the new mailing list.

On top of that, the last month of patches was delivered, but not
processed because Patchew did not recognize devel@lists.libvirt.org as
a message for the Libvirt project. This will also sort itself out in
an hour or so.

Paolo
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: [libvirt PATCH v2 12/15] conf: support manually specifying VFIO variant driver in XML

[Peter Krempa]

On Mon, Dec 18, 2023 at 01:25:02 -0500, Laine Stump wrote:
> On 11/28/23 9:58 AM, Peter Krempa wrote:
> > On Mon, Nov 06, 2023 at 02:38:57 -0500, Laine Stump wrote:
> > > This patch makes it possible to manually specify which VFIO variant
> > > driver to use for PCI hostdev device assignment, so that, e.g. you
> > > could force use of the generic vfio-pci driver with
> > > 
> > >
> > > 
> > > when libvirt would have normally (after applying a subsequent patch)
> > > found a "better match" for a device in the active kernel's
> > > modules.alias file. (The main use of this manual override would be to
> > > work around a bug in a new VFIO variant driver by temporarily not
> > > using that driver).
> > > 
> > > This sounds like a simple addition to XML parsing/formatting, but of
> > > course it's messier than that, since the attribute we want to use was
> > > previously used in config for a now non-existent purpose (choosing a
> > > type of device assignment that was removed from the kernel nearly a
> > > decade ago), and continues to be used *internal to the C code*.
> > > 
> > > Background:
> > > 
> > > Beginning when VFIO device assignment support was added to 
> > > or ) in libvirt's QEMU driver, it was
> > > possible to specify which device assignment backend to use with
> > > "". This was only useful for a couple of
> > > years in the early 2010's when VFIO device assignment was newly
> > > introduced, but "legacy KVM" device assignment was still possible (in
> > > reality "" was only intended to be used (1) in
> > > the *very* early days of VFIO when "kvm" was still the default, or (2)
> > > when the host kernel was too old to have VFIO support (meaning it was
> > > e.g. pre-RHEL7) or (3) some bug was encountered with the then-new VFIO
> > > code that could have been avoided by switching back to the older style
> > > of device assignment (I don't recall anyone ever needing to set it for
> > > this reason, but that is one of the main reasons we added the knob).
> > > 
> > > Later, when the libxl (Xen) driver in libvirt began supporting "device
> > > passthrough" with , they added  to
> > > indicate that mode of operation. But in practice this was also never
> > > necessary in any config, since Xen only supports one type of device
> > > assignment, and so the attribute was anyway set in the C object by the
> > > libxl driver code prior to calling any hypervisor-common code
> > > (i.e. the stuff in hypervisor/hostdev.c and util/virpci.c) that needs
> > > to know which type of device assignment is being used - setting it in
> > > the XML config was superfluous (kind of like me saying "I am now
> > > describing this patch in a human language", ref: Perd Hapley on "Parks
> > > and Recreation").
> > > 
> > > So the setting was available in the XML, but never needed to be set by
> > > the user. Because it was automatically set in the C code though,
> > > sometimes libvirt-generated XML would contain the option even though
> > > the user hadn't included it in the original input XML (e.g. the libxl
> > > driver sets it in the PostParse, so all saved configs with a PCI
> > >  device will have ; also status XML from
> > > the QEMU and libxl drivers will contain  - in
> > > both cases completely unnecessary and redundant information).
> > > 
> > > When adding support for specifying a variant driver for VFIO device
> > > assignment, from the beginning I have wanted to use  > > name='blah'/> to force a specific variant driver (e.g.  > > name='mlx5_vfio_pci'/>), with the assumption that the name attribute
> > > could be easily repurposed because it had been *completely* unused for
> > > so long. What I discovered though, was that 1) the field in the C
> > > object corresponding to driver name was an enum value rather than a
> > > string (so parser and formatter need to be changed, not just the
> > > driver code looking at a string in the C object), and 2) even though
> > > the XML attribute was effectively unused (except in some output), the
> > > field in the C object was *always* being set internally as a way for
> > > the hypervisor driver code to inform the hypervisor-common hostdev
> > > manager (in src/hypervisor) which method of device assignment to
> > > use. So re-use wasn't as simple as I'd wished.
> > > 
> > > However.
> > > 
> > > What I have hit upon that permits us to use
> > >  is to do the following:
> > > 
> > > 1) rename the existing driver name attribute to driver *type* - this
> > > will now be the enum that is set internally by the hypervisor
> > > driver prior to calling the hostdev manager (and also is what will
> > > show up in status XML; the libxl driver was modified in a previous
> > > patch so that the setting isn't done until domain runtime (rather
> > > than during PostParse), so it will no longer show up in
> > > regurgitated libxml config)
> > > 
> > > 2) add a new attribute with the now-newly-unused name "name" - this
> > > will be a string that can be us

Re: [libvirt PATCH v2 10/15] xen: explicitly set hostdev driver.type at runtime, not in postparse

[Peter Krempa]

On Mon, Dec 18, 2023 at 01:14:55 -0500, Laine Stump wrote:
> On 11/27/23 10:12 AM, Peter Krempa wrote:
> > On Mon, Nov 06, 2023 at 02:38:55 -0500, Laine Stump wrote:
> > > Xen only supports a single type of PCI hostdev assignment, so it is
> > > superfluous to have  peppered throughout the
> > > config. It *is* necessary to have the driver type explicitly set in
> > > the hosdev object before calling into the hypervisor-agnostic "hostdev
> > > manager" though (otherwise the hostdev manager doesn't know whether it
> > > should do Xen-specific setup, or VFIO-specific setup).
> > > 
> > > Historically, the Xen driver has checked for "default" driver type
> > > (i.e. not set in the XML), and set it to "xen', during the XML
> > > postparse, thus guaranteeing that it will be set by the time the
> > > object is sent to the hostdev manager at runtime, but also setting it
> > > so early that a simple round-trip of parse-format results in the XML
> > > always containing an explicit , even if that
> > > wasn't specified in the original XML.
> > 
> > Generally stuff that is written to the definition at parse time is done
> > so that it doesn't change in the future, thus preserving ABI of machines
> > created with a config where the default was not entered yet.
> 
> I don't think there was any intentional "lock in the default setting in the
> config to preserve ABI" in this case - it all seems to just be
> convenience/serendipity. From what I can see in the original code adding
> "PCI passthrough" to the libxl driver, they were just filling in the driver
> name in the xml because 1) it was there, and 2) they had just split out some
> of the code from qemu into "common code" to be used by both libxl and QEMU,
> needed a way to specify which driver was calling into this common code, saw
> the driver name as a convenient way to do it, and noticed that lots of other
> values that weren't set by the user were being set in the postparse, so
> that's where they set it in their code.
> 
> (BTW, there was code in the original commit adding PCI passthrough to libxl
> (commit 6225cb3df5a5732868719462f0a602ef11f7fa03) that logged an error if
> the driver name was anything other than empty (in which case it was set to
> "xen") or "xen". That code is still there today (see
> libxlNodeDeviceDetachFlags), so the Xen driver definiteoy only supports the
> one type of PCI device assignment.)
> 
> > 
> > This commit message doesn't make an argument why the change is needed,
> > so I'm a bit reluctant...
> 
> Well... the less use of the old usage of  there is, the better,
> and preventing it from being written into every single new config file with
> a  means less use. Also this change makes the libxl driver more
> consistent with the behavior of the qemu driver (which has never set an
> explicit default in the postparse - it waits until the devices are being
> initialized for domain startup/hotplug before it sets the driver type (and
> it also always sets it to the same value).
> 
> I suppose I could omit this patch, but I think it's just perpetuating
> unintentional code that is inconsistent with the other driver (qemu) which
> serves to make it more difficult for a newcomer to understand what's going
> on with the  and what is the proper way to handle it in
> the case of some new hypervisor driver that decides to support .
> 
> > 
> > 
> > > The QEMU driver *doesn't* set driver.type during postparse though;
> > > instead, it waits until domain startup time (or device attach time for
> > > hotplug), and sets the driver.type then. The result is that a
> > > parse-format round trip of the XML in the QEMU driver *doesn't* add in
> > > the .
> > > 
> > > This patch modifies the Xen code to behave similarly to the QEMU code
> > > - the PostParse just checks for a driver.type that isn't supported by
> > > the Xen driver, and any explicit setting to "xen" is deferred until domain
> > > runtime rather than during the postparse.
> > > 
> > > This delayed setting of driver.type of course results in slightly
> > > different xml2xml parse-format results, so the unit test data is
> > > modified accordingly.
> > > 
> > > Signed-off-by: Laine Stump 
> > > ---
> > >   src/libxl/libxl_domain.c  | 65 +++
> > >   src/libxl/libxl_driver.c  | 25 ---
> > >   tests/libxlxml2domconfigdata/moredevs-hvm.xml |  1 -
> > >   tests/xlconfigdata/test-fullvirt-pci.xml  |  2 -
> > >   tests/xmconfigdata/test-pci-dev-syntax.xml|  2 -
> > >   tests/xmconfigdata/test-pci-devs.xml  |  2 -
> > >   6 files changed, 69 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> > > index 22482adfa6..ecdf57e655 100644
> > > --- a/src/libxl/libxl_domain.c
> > > +++ b/src/libxl/libxl_domain.c
> > > @@ -160,8 +160,15 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef 
> > > *dev,
> > >   if (hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS &&
> > >

Re: [libvirt PATCH v2 05/15] conf: put hostdev PCI backend into a struct

[Peter Krempa]

On Sun, Dec 17, 2023 at 21:28:46 -0500, Laine Stump wrote:
> On 11/27/23 9:53 AM, Peter Krempa wrote:
> > > @@ -29973,14 +29973,10 @@ virDomainNetDefActualToNetworkPort(virDomainDef 
> > > *dom,
> > >   break;
> > >   case VIR_DEVICE_HOSTDEV_PCI_DRIVER_TYPE_XEN:
> > > -virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > > -   _("Unexpected PCI backend 'xen'"));
> > > -break;
> > > -
> > 
> > Falling through to virReportEnumRangeError from such cases isn't
> > something we normally do as virReportEnumRangeError is usually meant for
> > cases when the value is unknown.
> 
> (Finally going through all of these)
> 
> the ..._XEN value does exist in the enum for the value in the NetDef object,
> but there's no equivalent for it in the virNetworkForwardDriverNameType enum
> that's used on the NetworkPort object being converted *to*, so this actually
> *is* a range error (although you're correct that it's not the normal case of
> a completely unknown value).

The virReportEnumRangeError error reporting macro is in all other cases
used to report values out of range of the enum, the 'switch' statement
uses as an argument. Whatever it's then doing with the value is IMO out
of scope as it is in range of the enum being tested for values.

If it doesn't have any mapping where it is being converted *to*, it's
either an UNSUPORTED_CONFIG or INTERNAL_ERROR, but clearly the value is
in range of the source enum.

> So I would take the time to make a separate case / error message for this
> particular value, however the patch immediately after this patch completely
> removes all this code anyway (it switches to using the same enum for both
> objects), so I'm just leaving it as it is.

Fair enough in this case, I just don't want to keep wrong examples in
the repo, so if you remove it it's okay. I was just reviewing these
chronologically.
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: Patches on libvirt patchew are outdated

[Paolo Bonzini]

On Mon, Dec 18, 2023 at 9:32 AM Peter Krempa  wrote:
>
> On Mon, Dec 18, 2023 at 14:42:57 +0800, Han Han wrote:
> > Hi,
> > The libvirt patchew https://patchew.org/Libvirt/ has no update for more
> > than one month. Is it dropped? If not, please sync the patches to it~
>
> The problem is likely caused by the switch to the new mailing list. The
> patch importer of patchew wasn't probably configured to use the new
> list.
>
> Since the patchew project doesn't seem to contain the actual
> configuration for the importer that is used on patchew.org we'll need to
> contact somebody to fix it.
>
> Paolo, Daniel, do you know who can fix this?

Either I or Fam can fix it. We just need to subscribe to the new mailing list.

Paolo
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org

Re: Patches on libvirt patchew are outdated

[Peter Krempa]

On Mon, Dec 18, 2023 at 14:42:57 +0800, Han Han wrote:
> Hi,
> The libvirt patchew https://patchew.org/Libvirt/ has no update for more
> than one month. Is it dropped? If not, please sync the patches to it~

The problem is likely caused by the switch to the new mailing list. The
patch importer of patchew wasn't probably configured to use the new
list.

Since the patchew project doesn't seem to contain the actual
configuration for the importer that is used on patchew.org we'll need to
contact somebody to fix it.

Paolo, Daniel, do you know who can fix this?
___
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-le...@lists.libvirt.org