libsel4vm, vgic: generate dist and cpu values #83

chrisguikema · 2022-10-27T02:54:43Z

This commit adds an "fdt_ori" and "gic_node" parameter to the vm struct, which allows the library to find the GIC_DIST and GIC_CPU register addresses required for emulating the vgic.

chrisguikema · 2022-10-27T02:56:55Z

This isn't perfect, and requires a change in camkes-vm to set the path and pointers. It could also use a check to handle address-cells and size-cells properly. I just wanted to put it out for review and discussion, and to show its possible to grab the values from the device tree. I'm open to other and better ways to do this.

chrisguikema · 2022-10-27T03:15:23Z

Test with: seL4/camkes-vm#52

libsel4vm/src/arch/arm/guest_irq_controller.c

libsel4vm/src/arch/arm/vgic/vgic_v2.c

axel-h · 2022-10-27T11:26:29Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

@@ -58,7 +58,8 @@
 #include "gicv2.h"
 #include "vdist.h"

-
+static uintptr_t vcpu_addr;
+static uintptr_t cpu_addr;


Can we avoid these global variables and change the code flow to pass this pas parameter? We could also wrap al this in a struct for a context and then pass a pointer around. I think I have tried something in #69

At this point, yes, it feels like this could be reworked to avoid the globals. Since you've already done some work with that in the PR you referenced, do you want to handle that?

I'd try to rebase my PR on this then.

My preference would be to avoid adding the globals in this PR. The vgic_dist_device struct is intended for storing these exact mapping addresses and so they should be added there. As they aren't the same for the gic_v3, it's fine to create a different vgic_dist_device that's specialized to the gicv2 as it's private to this compilation unit.

axel-h · 2022-10-27T17:35:58Z

libsel4vm/src/arch/arm/vgic/vgicv2_defs.h

+
+/* Contants for getting registers from Device Tree */
+#define NUM_GIC_REGS        4
+#define GIC_DIST_ENTRY      0


This should get a more verbose comment. How much of a device tree standard is this actually or is it just what everybody seems to use. I think it might be better to inline these values and not expose any constants. Then a comment there can explain the the magic values come from. We can factor this out when these constants are to be somewhere. If we provide defines here, we names must be much more specific to avoid conflicts or get confusing.

I found a link with the standard. Its not just what everyone seems to use.

Can you rename this to e.g. NUM_FDT_GIC_REGS and FDT_GIC_REG_IDX_<DIST|CPU|VCPU_CNTR|VCPU>? So we have names that are a bit less ambiguous and clearly state this is device tree related

libsel4vm/src/arch/arm/vgic/vgic_v2.c

This commit adds an "fdt_ori" and "gic_node" parameter to the vm struct, which allows the library to find the gic register addresses required for emulating the vgic. Signed-off-by: Chris Guikema <[email protected]>

kent-mcleod · 2022-10-27T20:53:32Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

@@ -236,6 +297,9 @@ static vm_frame_t vgic_vcpu_iterator(uintptr_t addr, void *cookie)
 */
 int vm_install_vgic(vm_t *vm)
 {
+    /* Read device tree to find memory regions */
+    gic_get_registers_from_fdt(vm, &dist_addr, &cpu_addr, &vcpu_addr);


We should probably check and return the error here if the GIC can't be found in the FDT.

kent-mcleod · 2022-10-27T20:57:44Z

libsel4vm/include/sel4vm/guest_vm.h

+    char *gic_path;
+    void *fdt_ori;


As these are both Arm specific, they should be added to the vm_arch struct.

kent-mcleod · 2022-10-27T21:04:08Z

This commit adds an "fdt_ori" and "gic_node" parameter to the vm struct,

This isn't an insignificant design change to the vm_t struct. If this is something that we should add, then it probably needs to be properly integrated into the rest of the library with proper initialization functions. If it's just an initial way to prototype associating a device tree with a guest for virtual devices to more easily find configuration information it should probably still have a proper way of installing it. (This would also allow the camkes-vm layer to more explicitly generate the fdt and then install it into the guest for the rest of the guest devices to use). Also, how is this expected to work with guests that don't use an FDT but want to use virtual devices that do use an fdt?

kent-mcleod · 2022-10-27T21:06:39Z

One more comment, if this isn't something that we want to tackle now, then maybe a smaller-impact change would be choosing to have device specific interfaces for vm_install_vgic() that could take the configuration arguments needed, allowing the caller (camkes-vm) to do the device tree parsing and pass in the address ranges via the newer vm_install_vgic() interface.

chrisguikema · 2022-10-27T22:04:10Z

Thanks for the comments, Kent. This was something that I just put together based on the comments on using the GIC path to find the phandle. Basically, its more of a proof of concept. If you want to separate this commit out into a draft, so we can figure out the best way to do this properly while merging in the gic phandle, I'm okay with that approach.

chrisguikema · 2022-10-27T22:09:53Z

Also, how is this expected to work with guests that don't use an FDT but want to use virtual devices that do use an fdt?

Doesn't the kernel pass an FDT to each VMM regardless of whether the guest needs to use the FDT? So I guess there's a disconnect between the guest's device tree, and the FDT used to describe the hardware resources of the system.

Would it be possible to just use the ps_io_ops interface to get the FDT? Then search for the node with the arm,gic-400 string?

kent-mcleod · 2022-10-27T22:38:37Z

Doesn't the kernel pass an FDT to each VMM regardless of whether the guest needs to use the FDT? So I guess there's a disconnect between the guest's device tree, and the FDT used to describe the hardware resources of the system.

In CAmkES, the DTB is given to the component if it has the dtb property set in its configuration, so it's not always expected to be present. It would be reasonable for a VM component to load a file from the fileserver component and not need the system DTB given to the component at all.

I think one question that needs to be decided is whether device trees are a concept that libsel4vm should know about and have a mechanism for managing a device tree and installing it into the guest. It seems to me that this is better suited for libsel4vmmplatsupport?

kent-mcleod · 2022-10-27T22:43:04Z

Thanks for the comments, Kent. This was something that I just put together based on the comments on using the GIC path to find the phandle

Yea, I'm not trying to say you shouldn't have proposed the changes, just pointing out that adding a way for vm devices to query a device tree associated with the guest is something that would be quite useful to many more devices than just the vgic, so we should consider the wider implications. EG, should this device tree be the system device tree, or the one that the guest sees? Are devices allowed to mutate it? If it's using a libfdt compatible object, then we could allow devices to install nodes in during their own initialization and have an explicit finalization step which is what Qemu does.

axel-h · 2022-10-27T23:04:20Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

+    }
+
+    int address_cells = fdt_address_cells(vm->fdt_ori, parent_offset);
+    int size_cells = fdt_address_cells(vm->fdt_ori, parent_offset);


Suggested change

int size_cells = fdt_address_cells(vm->fdt_ori, parent_offset);

int size_cells = fdt_size_cells(vm->fdt_ori, parent_offset);

axel-h · 2022-10-27T23:07:49Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

+     */
+    uint64_t gic_regs[NUM_GIC_REGS];
+    int calculated_size = size_cells * address_cells * NUM_GIC_REGS * sizeof(uint32_t);
+    int reg_offset = calculated_size / NUM_GIC_REGS;


Suggested change

int reg_offset = calculated_size / NUM_GIC_REGS;

int reg_size = calculated_size / NUM_GIC_REGS;

This is the size of one value, not the offset

axel-h · 2022-10-27T23:16:04Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

+    }
+
+    for (int i = 0; i < NUM_GIC_REGS; i++) {
+        memcpy(&gic_regs[i], (void *)(c->data + (i * reg_offset)), sizeof(uint32_t) * address_cells);


I think this has an implicit assumption that address_cells is 2 and address_cells is 1. That may hold practically, so we could simply reject anything else for now.

This should work with any case of address/size being 1 or 2.

Right, extracting the value works for 1 or 2. But we need uint64_t gic_regs[NUM_GIC_REGS] = {0}; then to ensure the array is filled with zeros. Otherwise we might just copy 32 bits and the other 32-bit are random data,

axel-h · 2022-10-27T23:24:43Z

libsel4vm/src/arch/arm/vgic/vgic_v2.c

+     * 4th reg entry: GIC Virtual CPU Interface Register
+     */
+    uint64_t gic_regs[NUM_GIC_REGS];
+    int calculated_size = size_cells * address_cells * NUM_GIC_REGS * sizeof(uint32_t);


Suggested change

int calculated_size = size_cells * address_cells * NUM_GIC_REGS * sizeof(uint32_t);

int calculated_size = (address_cells + size_cells) * NUM_GIC_REGS * sizeof(uint32_t);

We have one address cell followed by a size cell for each element. This just work because 2 * 2 happens to equal 2 + 2

chrisguikema · 2022-10-27T23:42:20Z

I'm going to mark this as a draft, since like Kent mentioned, in order to do this, you need access to the FDT in the libraries, which is a bigger design choice than just this PR. I think this would still be useful as a reference for getting the GIC information out of the device tree once the FDT is available.

chrisguikema requested review from kent-mcleod and axel-h October 27, 2022 02:54

chrisguikema mentioned this pull request Oct 27, 2022

Dont hard-code GIC_IRQ_PHANDLE, read it from device tree instead seL4/camkes-vm#49

Closed

chrisguikema force-pushed the gic_addr_generation branch from 10783be to 40a0325 Compare October 27, 2022 03:15

axel-h reviewed Oct 27, 2022

View reviewed changes

libsel4vm/src/arch/arm/guest_irq_controller.c Outdated Show resolved Hide resolved

axel-h reviewed Oct 27, 2022

View reviewed changes

libsel4vm/src/arch/arm/vgic/vgic_v2.c Show resolved Hide resolved

axel-h reviewed Oct 27, 2022

View reviewed changes

chrisguikema force-pushed the gic_addr_generation branch from 40a0325 to 390bef1 Compare October 27, 2022 15:48

chrisguikema mentioned this pull request Oct 27, 2022

vm_arm: use gic path to find phandle seL4/camkes-vm#52

Merged

axel-h reviewed Oct 27, 2022

View reviewed changes

libsel4vm/src/arch/arm/vgic/vgic_v2.c Show resolved Hide resolved

chrisguikema force-pushed the gic_addr_generation branch from 390bef1 to 239562e Compare October 27, 2022 18:14

libsel4vm, vgic: read gic registers from fdt

46b5800

This commit adds an "fdt_ori" and "gic_node" parameter to the vm struct, which allows the library to find the gic register addresses required for emulating the vgic. Signed-off-by: Chris Guikema <[email protected]>

chrisguikema force-pushed the gic_addr_generation branch from 239562e to 46b5800 Compare October 27, 2022 18:17

kent-mcleod reviewed Oct 27, 2022

View reviewed changes

chrisguikema mentioned this pull request Oct 27, 2022

vm_arm: set fdt pointer and gic name seL4/camkes-vm#53

Draft

axel-h reviewed Oct 27, 2022

View reviewed changes

chrisguikema marked this pull request as draft October 27, 2022 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libsel4vm, vgic: generate dist and cpu values #83

libsel4vm, vgic: generate dist and cpu values #83

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

axel-h Oct 27, 2022

chrisguikema Oct 27, 2022

axel-h Oct 27, 2022

kent-mcleod Oct 27, 2022

axel-h Oct 27, 2022

chrisguikema Oct 27, 2022

axel-h Oct 27, 2022

kent-mcleod Oct 27, 2022

kent-mcleod Oct 27, 2022

kent-mcleod commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

axel-h Oct 27, 2022

axel-h Oct 27, 2022

axel-h Oct 27, 2022

chrisguikema Oct 27, 2022

axel-h Oct 27, 2022 •

edited

Loading

axel-h Oct 27, 2022

chrisguikema commented Oct 27, 2022

	int size_cells = fdt_address_cells(vm->fdt_ori, parent_offset);
	int size_cells = fdt_size_cells(vm->fdt_ori, parent_offset);

	int reg_offset = calculated_size / NUM_GIC_REGS;
	int reg_size = calculated_size / NUM_GIC_REGS;

	int calculated_size = size_cells * address_cells * NUM_GIC_REGS * sizeof(uint32_t);
	int calculated_size = (address_cells + size_cells) * NUM_GIC_REGS * sizeof(uint32_t);

libsel4vm, vgic: generate dist and cpu values #83

Are you sure you want to change the base?

libsel4vm, vgic: generate dist and cpu values #83

Conversation

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kent-mcleod commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

chrisguikema commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

kent-mcleod commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axel-h Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisguikema commented Oct 27, 2022

axel-h Oct 27, 2022 •

edited

Loading