Re: [libvirt PATCH 3/5] Config some capabilities for loongarch virt machine
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.