On 27.11.20 18:17, Catangiu, Adrian Costin wrote:
On 18/11/2020 12:30, Alexander Graf wrote:
On 16.11.20 16:34, Catangiu, Adrian Costin wrote:
- Future improvements
Ideally we would want the driver to register itself based on devices'
_CID and not _HID, but unfortunately I couldn't find a way to do that.
The problem is that ACPI device matching is done by
'__acpi_match_device()' which exclusively looks at
'acpi_hardware_id *hwid'.
There is a path for platform devices to match on _CID when _HID is
'PRP0001' - but this is not the case for the Qemu vmgenid device.
Guidance and help here would be greatly appreciated.
That one is pretty important IMHO. How about the following (probably
pretty mangled) patch? That seems to work for me. The ACPI change
would obviously need to be its own stand alone change and needs proper
assessment whether it could possibly break any existing systems.
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 1682f8b454a2..452443d79d87 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -748,7 +748,7 @@ static bool __acpi_match_device(struct acpi_device
*device,
/* First, check the ACPI/PNP IDs provided by the caller. */
if (acpi_ids) {
for (id = acpi_ids; id->id[0] || id->cls; id++) {
- if (id->id[0] && !strcmp((char *)id->id, hwid->id))
+ if (id->id[0] && !strncmp((char *)id->id, hwid->id,
ACPI_ID_LEN - 1))
goto out_acpi_match;
if (id->cls && __acpi_match_device_cls(id, hwid))
goto out_acpi_match;
diff --git a/drivers/virt/vmgenid.c b/drivers/virt/vmgenid.c
index 75a787da8aad..0bfa422cf094 100644
--- a/drivers/virt/vmgenid.c
+++ b/drivers/virt/vmgenid.c
@@ -356,7 +356,8 @@ static void vmgenid_acpi_notify(struct acpi_device
*device, u32 event)
}
static const struct acpi_device_id vmgenid_ids[] = {
- {"QEMUVGID", 0},
+ /* This really is VM_Gen_Counter, but we can only match 8
characters */
+ {"VM_GEN_C", 0},
{"", 0},
};
Looks legit. I can propose a patch with it, but how do we validate it
doesn't break any devices?
Mainly by proposing it and seeing what the ACPI maintainers say. Maybe
they have a better idea even. At least this explictly nudges them.
+2) ASYNC simplified example::
+
+ void handle_io_on_vmgenfd(int vmgenfd)
+ {
+ unsigned genid;
+
+ // because of VM generation change, we need to rebuild world
+ reseed_app_env();
+
+ // read new gen ID - we need it to confirm we've handled update
+ read(fd, &genid, sizeof(genid));
This is racy in case two consecutive snapshots happen. The read needs
to go before the reseed.
Switched them around like you suggest to avoid confusion.
But I don't see a problem with this race. The idea here is to trigger
reseed_app_env() which doesn't depend on the generation counter value.
Whether it gets incremented once or N times is irrelevant, we're just
interested that we pause execution and reseed before resuming (in
between these, whether N or M generation changes is the same thing).
+3) Mapped memory polling simplified example::
+
+ /*
+ * app/library function that provides cached secrets
+ */
+ char * safe_cached_secret(app_data_t *app)
+ {
+ char *secret;
+ volatile unsigned *const genid_ptr = get_vmgenid_mapping(app);
+ again:
+ secret = __cached_secret(app);
+
*genid_ptr = 1
cached_genid = 1
+ if (unlikely(*genid_ptr != app->cached_genid)) {
*genid_ptr = 2
cached_genid = 1
+ // rebuild world then confirm the genid update (thru write)
+ rebuild_caches(app);
hypervisor takes another snapshot during rebuild_caches(). Resume path
bumps genid
+ app->cached_genid = *genid_ptr;
*genid_ptr = 3
cached_genid = 3
This is racy again. You need to read the genid before rebuild and set
it here.
I don't see the race. Gen counter is read from volatile mapped mem, on
detected change we rebuild world, confirm the update back to the driver
then restart the loop. Loop will break when no more changes happen.
See above. After the outlined course of things, the snapshot will
contain data that will be identical between 2 snapshots.
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
