Re: [PATCH v2 0/7] KVM: arm/arm64: gsi routing support
Hi Pavel, On 09/07/15 15:37, Pavel Fedin wrote: Hello! v1 - v2: - user API changed: x devid id passed in kvm_irq_routing_msi x kept the new routing entry type: KVM_IRQ_ROUTING_EXTENDED_MSI Andre, you never replied to my last comment to the previous series. Oh dear, my draft folder again :-( Sorry for that! Are you going to do the same change in your MSI API? Otherwise: 1. KVM_IRQ_LINE - we have completely own convention. Well, this was already done before us, we cannot fix it. 2. KVM_SIGNAL_MSI - we use VALID_DEVID flag plus devid Yes, because there is already a flag value and no other way to specify this, in contrast to ... 3. KVM_SET_GSI_ROUTING - we use KVM_IRQ_ROUTING_EXTENDED_MSI plus devid Here we already have a type field with some users, so lets piggy-back on this. Both ioctl extensions are coupled with a per-VM capability to let userland know that it needs to provide a device ID. Don't (2) and (3) together still look bad? Since we agreed on not using flags, i would suggest to have KVM_SIGNAL_EXTENDED_MSI counterpart, which also doesn't use flags. Using flags on its own (without an explicit capability) is what I opposed against, not flags in general. After all, that's what they are meant for, right? In case of KVM_SET_GSI_ROUTING it just seems awkward to me to use a flag when a different type would do as well. But after all, I don't have a strong opinion on that matter, so if others prefer using a flag I am also fine with that. Poka, Andre. I know, we were already talking about it, so, if this gets ignored for the second time, i assume the Architects decided that fancy APIs are cool, and i promise to stop this. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/7] KVM: arm/arm64: gsi routing support
Hello! v1 - v2: - user API changed: x devid id passed in kvm_irq_routing_msi x kept the new routing entry type: KVM_IRQ_ROUTING_EXTENDED_MSI Andre, you never replied to my last comment to the previous series. Are you going to do the same change in your MSI API? Otherwise: 1. KVM_IRQ_LINE - we have completely own convention. Well, this was already done before us, we cannot fix it. 2. KVM_SIGNAL_MSI - we use VALID_DEVID flag plus devid 3. KVM_SET_GSI_ROUTING - we use KVM_IRQ_ROUTING_EXTENDED_MSI plus devid Don't (2) and (3) together still look bad? Since we agreed on not using flags, i would suggest to have KVM_SIGNAL_EXTENDED_MSI counterpart, which also doesn't use flags. I know, we were already talking about it, so, if this gets ignored for the second time, i assume the Architects decided that fancy APIs are cool, and i promise to stop this. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/7] KVM: arm/arm64: gsi routing support
Hi!
3. KVM_SET_GSI_ROUTING - we use KVM_IRQ_ROUTING_EXTENDED_MSI plus devid
Here we already have a type field with some users, so lets piggy-back on
this.
We already have 'flags' there too.
Both ioctl extensions are coupled with a per-VM capability to let
userland know that it needs to provide a device ID.
Using flags on its own (without an explicit capability) is what I
opposed against, not flags in general.
Ok, and in your next respin you'll add the capability, correct? So that we
will finally have all
pieces in place.
In case of KVM_SET_GSI_ROUTING it just seems awkward
to me to use a flag when a different type would do as well.
Well, MSI vs Extended MSI are even not different types really. It's just MSI
which has devid. And
we *ALREADY* have VALID_DEVID flag.
But after all, I don't have a strong opinion on that matter, so if
others prefer using a flag I am also fine with that.
So, ok, to be short... My vote is for flag, because it's already there and it
keeps up with the
style we already have. Eric, this is my final statement about it. It's up to
you to accept or
ignore. In qemu code flag is a little bit nicer because it's just stored in a
variable and helps to
avoid several if-else's (however small ones). Compare:
--- cut ---
kroute.gsi = virq;
kroute.type = KVM_IRQ_ROUTING_MSI;
kroute.u.msi.address_lo = (uint32_t)msg.address;
kroute.u.msi.address_hi = msg.address 32;
kroute.u.msi.data = le32_to_cpu(msg.data);
kroute.flags = kvm_msi_flags;
if (kroute.flags KVM_MSI_VALID_DEVID) {
kroute.u.msi.devid = (pci_bus_num(dev-bus) 8) | dev-devfn;
}
--- cut ---
and:
--- cut ---
kroute.gsi = virq;
if (use_extended_msi) {
kroute.u.msi.devid = (pci_bus_num(dev-bus) 8) | dev-devfn;
kroute.type = KVM_IRQ_ROUTING_EXTENDED_MSI;
} else {
kroute.type = KVM_IRQ_ROUTING_MSI;
}
kroute.u.msi.address_lo = (uint32_t)msg.address;
kroute.u.msi.address_hi = msg.address 32;
kroute.u.msi.data = le32_to_cpu(msg.data);
kroute.flags = kvm_msi_flags;
--- cut ---
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/7] KVM: arm/arm64: gsi routing support
Hi Pavel, Andre,
On 07/09/2015 05:52 PM, Pavel Fedin wrote:
Hi!
3. KVM_SET_GSI_ROUTING - we use KVM_IRQ_ROUTING_EXTENDED_MSI plus devid
Here we already have a type field with some users, so lets piggy-back on
this.
We already have 'flags' there too.
Both ioctl extensions are coupled with a per-VM capability to let
userland know that it needs to provide a device ID.
Using flags on its own (without an explicit capability) is what I
opposed against, not flags in general.
Ok, and in your next respin you'll add the capability, correct? So that we
will finally have all
pieces in place.
In case of KVM_SET_GSI_ROUTING it just seems awkward
to me to use a flag when a different type would do as well.
Well, MSI vs Extended MSI are even not different types really. It's just
MSI which has devid. And
we *ALREADY* have VALID_DEVID flag.
But after all, I don't have a strong opinion on that matter, so if
others prefer using a flag I am also fine with that.
So, ok, to be short... My vote is for flag, because it's already there and
it keeps up with the
style we already have. Eric, this is my final statement about it. It's up to
you to accept or
ignore. In qemu code flag is a little bit nicer because it's just stored in a
variable and helps to
avoid several if-else's (however small ones). Compare:
Well personally I prefer the type thing and I don't see much difference
at userspace level anyway. But I am not this kind of hyperspace
architect guy. So, since there is no consensus here, I would say let's
wait for formal reviews of our maintainers and I will align.
The v2 update is not the outcome of a consensus so I made arbitrary
decisions to progress fix bugs and I hope this eventually works with
ITS ;-)
Best Regards
Eric
--- cut ---
kroute.gsi = virq;
kroute.type = KVM_IRQ_ROUTING_MSI;
kroute.u.msi.address_lo = (uint32_t)msg.address;
kroute.u.msi.address_hi = msg.address 32;
kroute.u.msi.data = le32_to_cpu(msg.data);
kroute.flags = kvm_msi_flags;
if (kroute.flags KVM_MSI_VALID_DEVID) {
kroute.u.msi.devid = (pci_bus_num(dev-bus) 8) | dev-devfn;
}
--- cut ---
and:
--- cut ---
kroute.gsi = virq;
if (use_extended_msi) {
kroute.u.msi.devid = (pci_bus_num(dev-bus) 8) | dev-devfn;
kroute.type = KVM_IRQ_ROUTING_EXTENDED_MSI;
} else {
kroute.type = KVM_IRQ_ROUTING_MSI;
}
kroute.u.msi.address_lo = (uint32_t)msg.address;
kroute.u.msi.address_hi = msg.address 32;
kroute.u.msi.data = le32_to_cpu(msg.data);
kroute.flags = kvm_msi_flags;
--- cut ---
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 0/7] KVM: arm/arm64: gsi routing support
Well personally I prefer the type thing and I don't see much difference at userspace level anyway. But I am not this kind of hyperspace architect guy. So, since there is no consensus here, I would say let's wait for formal reviews of our maintainers and I will align. Hah, okay... Don't take it for too much. Anyway your word weighs much more than my one... hope this eventually works with ITS ;-) You know... There are actually problems with irqfd. However i'm not sure whether they are result of some bug or of poorly made vGIC CPU interface software emulation. I don't have hardware with working vGICv3 here. With GICv2m everything is fine, but code path is quite different, and LPI != SPI. This is further overcomplicated by the fact that as soon as i start up the profiler, the problem goes away. Looks like increased number of context switches have impact. Actually, the problem is very poor vhost-net throughput, which jumps up when i start the profiler and goes back down when i stop it. Just to note... I'm off since now for a small vacation, will be back on monday or tuesday. See you guys! Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
