Re: [Qemu-devel] [v4 1/6] hw/msi: Allow platform devices to use explicit SID

[Peter Xu]

On Mon, Sep 12, 2016 at 01:08:04PM +0300, David Kiarie wrote:
> When using IOMMU platform devices like IOAPIC are required to make
> interrupt remapping requests using explicit SID.We affiliate an MSI
> route with a requester ID and a PCI device if present which ensures
> that platform devices can call IOMMU interrupt remapping code with
> explicit SID while maintaining compatility with the original code
> which mainly dealt with PCI devices.

Nit: IMHO it'll be cooler and easier to understand if we can avoid
using very long sentences like above...

[...]

> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 31791b0..f2d4c15 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -95,9 +95,17 @@ static void ioapic_entry_parse(uint64_t entry, struct 
> ioapic_entry_info *info)
>          (info->delivery_mode << MSI_DATA_DELIVERY_MODE_SHIFT);
>  }
>  
> -static void ioapic_service(IOAPICCommonState *s)
> +static void ioapic_as_write(IOAPICCommonState *s, uint32_t data, uint64_t 
> addr)
>  {
>      AddressSpace *ioapic_as = PC_MACHINE(qdev_get_machine())->ioapic_as;
> +    MemTxAttrs attrs;

Need to zero it first?

[...]

> @@ -383,14 +391,28 @@ static void ioapic_machine_done_notify(Notifier 
> *notifier, void *data)
>      IOAPICCommonState *s = container_of(notifier, IOAPICCommonState,
>                                          machine_done);
>  
> +    X86IOMMUState *iommu = x86_iommu_get_default();
> +    /* kernel_irqchip=off */
> +    if (iommu) {
> +        s->devid = iommu->ioapic_bdf;
> +    }

Move out of "#ifdef CONFIG_KVM"?

[...]

> @@ -407,7 +429,6 @@ static void ioapic_realize(DeviceState *dev, Error **errp)
>  
>      memory_region_init_io(&s->io_memory, OBJECT(s), &ioapic_io_ops, s,
>                            "ioapic", 0x1000);
> -

Useless change.

[...]

> -int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> +int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev, 
> uint16_t requester_id)
>  {
>      struct kvm_irq_routing_entry kroute = {};
>      int virq;
> @@ -1275,7 +1275,8 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, 
> PCIDevice *dev)
>      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);
> -    if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> +    if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data,
> +                                 requester_id)) {

Can we just remove the PCIDevice parameter directly? I didn't go
deeper, but it seems to be explicitly introduced here:

    commit dc9f06ca81e6e16d062ec382701142a3a2ab3f7d
    Author: Pavel Fedin <address@hidden>
    Date:   Thu Oct 15 16:44:52 2015 +0300

        kvm: Pass PCI device pointer to MSI routing functions

And we'd better make sure we really wanted to remove it.

Also, I think we need to modify all target-*/kvm.c for this interface
change?

[...]

Btw, could you explain a bit about what kind of tests have you carried
out for this series (maybe also the AMD IOMMU general one)?

Thanks,

-- peterx